aslanbek - Anyone can cancel a raffle with tickets == minTicketsThreshold, griefing all participants

[sherlock-admin3]

aslanbek

Medium

Anyone can cancel a raffle with tickets == minTicketsThreshold, griefing all participants

Summary

    function _checkShouldDraw(uint256 raffleId) internal view {
        Raffle storage raffle = _raffles[raffleId];
        if (raffle.status != RaffleStatus.IDLE) revert InvalidRaffle();
        uint256 currentTicketSold = IWinnablesTicket(TICKETS_CONTRACT).supplyOf(raffleId);
        if (currentTicketSold == 0) revert NoParticipants();
        if (block.timestamp < raffle.endsAt) {
            if (currentTicketSold < raffle.maxTicketSupply) revert RaffleIsStillOpen();
        }
        if (currentTicketSold < raffle.minTicketsThreshold) revert TargetTicketsNotReached();
    }

    function _checkShouldCancel(uint256 raffleId) internal view {
        Raffle storage raffle = _raffles[raffleId];
        if (raffle.status == RaffleStatus.PRIZE_LOCKED) return;
        if (raffle.status != RaffleStatus.IDLE) revert InvalidRaffle();
        if (raffle.endsAt > block.timestamp) revert RaffleIsStillOpen();
        uint256 supply = IWinnablesTicket(TICKETS_CONTRACT).supplyOf(raffleId);
        if (supply > raffle.minTicketsThreshold) revert TargetTicketsReached();
    }

As we can see, once the ticket sale is over, if exactly minTicketsThreshold were sold, both cancelRaffle and drawWinner are available. If anyone (e.g. a participant who does not want to participate anymore, or a griefer) manages to call cancelRaffle before anyone calls drawWinner for that raffleId, it would cancel a raffle that should have been drawn, according to the code comment.

Root Cause

_checkShouldCancel does not revert when supply == raffle.minTicketsThreshold.

Internal pre-conditions

currentTicketSold == raffle.minTicketsThreshold
block.timestamp >= raffle.endsAt
raffleStatus == RaffleStatus.IDLE

Attack Path

Anyone calls cancelRaffle when block.timestamp >= raffle.endsAt, and before drawWinner is called.

Impact

Raffle that should be drawn is cancelled.

Mitigation

    function _checkShouldCancel(uint256 raffleId) internal view {
        Raffle storage raffle = _raffles[raffleId];
        if (raffle.status == RaffleStatus.PRIZE_LOCKED) return;
        if (raffle.status != RaffleStatus.IDLE) revert InvalidRaffle();
        if (raffle.endsAt > block.timestamp) revert RaffleIsStillOpen();
        uint256 supply = IWinnablesTicket(TICKETS_CONTRACT).supplyOf(raffleId);
-       if (supply > raffle.minTicketsThreshold) revert TargetTicketsReached();
+       if (supply >= raffle.minTicketsThreshold) revert TargetTicketsReached();
    }

[aslanbekaibimov]

Escalate

Permanent DoS of drawWinner for a subset of drawable raffles (which is arguably a time-sensitive function).

Breaks core contract functionality (drawing a winner) for a subset of drawable raffles, (arguably) rendering the contract useless.

Loss of funds in the form of lost EV for all participants if total value of the tickets was smaller than the prize.

[sherlock-admin3]

Escalate

Permanent DoS of drawWinner for a subset of drawable raffles (which is arguably a time-sensitive function).

Breaks core contract functionality (drawing a winner) for a subset of drawable raffles, (arguably) rendering the contract useless.

Loss of funds in the form of lost EV for all participants if total value of the tickets was smaller than the prize.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Brivan-26]

The root cause is indeed valid, but what's the impact here? Only if someone calls cancelRaffle before drawWinner the raffle will be canceled?

• There is no loss of funds, refundPlayers can be called to refund the bidders
• The next raffle can start normally, so there is no permanent DoS as stated by the waston

The impact is LOW, and the likelihood is between Low/medium as it requires both:

1. the number of tickets sold should be exactly minTicketsThreshold
2. Someone needs to call cancelRaffle before drawWinner

[jsmi0703]

#340 is a duplicate of this issue.

[0xweb333]

#330 is a duplicate

[kuprumxyz]

#550 is a partial duplicate of this (the second vulnerability + the second attack path); or this is a partial duplicate of #550, whatever fits.

According to Sherlock rules:

DoS has two separate scores on which it can become an issue:

• The issue causes locking of funds for users for more than a week.
• The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly.

Both of the above apply:

• The user funds are locked for more than a week: From the link I supplied in my finding, Long Running Raffle (scroll under the "Activity" tab): user funds have been locked for a month under this raffle. Anyone can cancel the raffle, thus the funds have been locked for so long in vein.
• The issue impacts the availability of time-sensitive functions: cancelling a raffle means that drawWinner can't be called anymore, i.e. it becomes unavailable. As this function can be called only before the raffle is canceled, it is time-sensitive.

Thus, according to the Sherlock rules this may be even a valid High; but it is clearly at least a valid Medium.

[mystery0x]

A fixed is desired to perfect the conditional check but it's deemed low given the trivial change needed.

[kuprumxyz]

@mystery0x I would kindly ask to point to the Sherlock docs which allow to qualify the finding as Low if the fix is trivial.

To the best of my knowledge no such docs exist. Moreover, they can't exist. There are literally billions of funds lost due to bugs with trivial fixes. As an example, take a look at the Wormhole Bridge Exploit Incident Analysis. The exploit was for over $320M, but the fix is trivial: replace the deprecated function load_current_index with load_current_index_checked.

[Brivan-26]

It's not about whether the fix is trivial or not, the impact is low and the likelihood is low/medium. This doesn't quality for Medium severity

[kuprumxyz]

It's not about whether the fix is trivial or not, the impact is low and the likelihood is low/medium. This doesn't quality for Medium severity

To that I have already elaborated above; have nothing to add. The severity is at least Medium; arguably can be a High.

[WangSecurity]

@kuprumxyz about your points in the previous comment:

The user funds are locked for more than a week: From the link I supplied in my finding, Long Running Raffle (scroll under the "Activity" tab): user funds have been locked for a month under this raffle. Anyone can cancel the raffle, thus the funds have been locked for so long in vein

I don't think this "lock" can be considered a DOS here, because it's how the protocol should work, i.e. you enter the raffle and your funds are locked until the raffle has finished or gets cancelled. Moreover, I don't think the finding is about this, the finding is about cancelling the raffle before the winner was drawn and I don't think any funds are locked here, since as noted above anyone can call refundPlayers.

The issue impacts the availability of time-sensitive functions: cancelling a raffle means that drawWinner can't be called anymore, i.e. it becomes unavailable. As this function can be called only before the raffle is canceled, it is time-sensitive

But, in that sense, just cancelling the raffle is DOSing the drawing of the winner. Could you elaborate a bit more on why you consider this a time-sensitive function, I don't think the ability to cancel a raffle, makes the function of picking a winner time-sensitive.

[kuprumxyz]

@WangSecurity the scenario is as follows:

1. A raffle is created.
2. Users are buying tickets. When users are buying tickets, their funds are locked in the raffle. As I demonstrated above, a month is a normal duration for a raffle, so the funds are locked for a month.
3. The raffle reaches the end of its duration (a month), and the number of tickets sold reaches minTicketsThreshold.
4. At that point, a raffle is both draw-able, but also cancel-able, while it should be only draw-able; this is the bug.

We see that "the user funds are locked for more than a week". Yes, this is how the protocol works, but the point is that the users lock their funds not just because of some weird desire to lock them for a month; they lock them because each of them wishes to participate in a raffle, and may be win. Depriving them of the right to draw a winner, and of winning a raffle, is the same as DoS of their funds for a month.

But, in that sense, just cancelling the raffle is DOSing the drawing of the winner. Could you elaborate a bit more on why you consider this a time-sensitive function, I don't think the ability to cancel a raffle, makes the function of picking a winner time-sensitive.

Point 4. above is the bifurcation point, at which only one of the two alternatives is possible: either the winner will be drawn xor the raffle will be canceled. From the time point when one happens, another can't happen anymore; thus drawing a winner is time-sensitive for this specific combination of parameters.

[kuprumxyz]

@WangSecurity, here is another point to consider:

If the above scenario happens, i.e. users were waiting for a month, and then the raffle got canceled though it should have been drawn, the trust of users into the protocol will be severely undermined, and most likely they won't like to participate anymore. This may easily lead to the protocol collapse.

[aslanbekaibimov]

I would like to add that in some raffles, at the end of the buying period, someone will buy just enough tickets so the raffle reaches minTicketsThreshold, reasonably expecting that it would guarantee that the raffle will be drawn.

[Brivan-26]

@WangSecurity, here is another point to consider:

If the above scenario happens, i.e. users were waiting for a month, and then the raffle got canceled though it should have been drawn, the trust of users into the protocol will be severely undermined, and most likely they won't like to participate anymore. This may easily lead to the protocol collapse.

This is not a valid impact.

• There is no loss of funds, refundPlayers can be called to refund the bidders
• The next raffle can start normally, so there is no permanent DoS neither

The likelihood is between Low/medium as it requires both:

• the number of tickets sold should be exactly minTicketsThreshold
• Someone needs to call cancelRaffle before drawWinner

@aslanbekaibimov

I would like to add that in some raffles, at the end of the buying period, someone will buy just enough tickets so the raffle reaches minTicketsThreshold, reasonably expecting that it would guarantee that the raffle will be drawn.

Even in such a case, if someone calls drawWinner, the draw will happen.

[aslanbekaibimov]

@Brivan-26

Even in such a case, if someone calls drawWinner, the draw will happen.

It will happen only if cancelRaffle was not called. For raffles that reached minTicketsThreshold, drawWinner should always be available.

[WangSecurity]

After additionally considering this, indeed the raffle shouldn't be cancellable when the minTickets threshold is reached. Therefore, this is a break of the contract functionality, since you can cancel the raffle when you shouldn't be able to. Hence, it qualifies for the following:

Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

Planning to accept the escalation and validate with medium severity. This will be the main issue of the new family, the duplicates are:

• Oblivionis - raffle is both drawable and cancelable when currentTicketSold = minTicketsThreshold #126
• p0wd3r - Drawable raffle can be canceled. #210
• Silvermist - WinnablesTicketManager.sol#_checkShouldCancel - Wrong check allows raffle to be canceled even though it shouldn't be #272
• rbserver - Because WinnablesTicketManager._checkShouldCancel function does not revert when raffle's total number of sold tickets equals its minTicketsThreshold after its purchase period is ended, such raffle can be canceled though it should not be cancellable, and such raffle's potential winner becomes unable to win #312
• anonymousjoe - Raffle can be cancelled even if minTicketsThreshold is reached #479
• neko_nyaa - Raffles with exactly minTicketsThreshold tickets sold can still be cancelled #507
• kuprum - A raffle may be guaranteed won, or canceled despite reaching maxTicketSupply, thus depriving users from winning it #550 (I see there are 2 issues, but I believe this report focuses slightly more on this issue. FYI, in the future, put these as two issues, since the first scenario breaks the README statement, while the second is a different issue. Hence, in this contest, it would have been 2 valid issues, but we can't separate one report and reward it twice).
• jsmi - Attacker can cancel raffle when there is exactly minimum participants. #340
• 0x0x0xw3 - Raffle can be canceled even if minTicketsThreshold is reached #330
• Offensive021 - Incorrect Raffle Ticket Threshold Logic Allows Simultaneous Cancellation and Drawing #114
• almurhasan - minTicketsThreshold check inconsistency in function _checkShouldDraw and function _checkShouldCancel. #475

Note: if there are other missing duplicates, let me know. Also, big thanks to Watsons who didn't escalate their issues and just flagged under the escalation.

[kuprumxyz]

I see there are 2 issues, but I believe this report focuses slightly more on this issue. FYI, in the future, put these as two issues, since the first scenario breaks the README statement, while the second is a different issue. Hence, in this contest, it would have been 2 valid issues, but we can't separate one report and reward it twice

@WangSecurity, thank you; the advice is highly appreciated.

[yashar0x]

hey @WangSecurity
#114 is a dup of this

[0xsimao]

@WangSecurity the mentioned comment says

Minimum number of tickets required to be sold for this raffle

Which means the raffle can draw a winner if this amount is reached, which is true.
The only thing enforced by this statement is that if not enough tickets are bought, the round can not be drawn, which holds.
Just because the round may also be cancelled at the exact same number of tickets, it does not mean the comment is broken.

There is no vulnerability in allowing the round to be both drawable and cancelled when the tickets are exactly minTicketsThreshold, unless there was a comment specifically saying that rounds should be cancelled only when minTicketsThreshold is exceeded, which is not the case.

We should assume people act in their best interest and if they want to ensure the round is drawn, they can just buy the number of tickets left to minTicketsThreshold + 1 to guarantee it is not cancelled. This is because it is only possible to cancel after the raffle ends, so they have plenty of time to do so.

[Almur100]

#475 is duplicate of this issue

[WangSecurity]

I didn't say the comment is broken and that's why the issue is validated. The problem here is that the intended design of the protocol is that the raffle shouldn't be cancellable when the min tickets threshold requirement is passed (i.e. tickets == minTicketsThreshold). Therefore, this issue qualifies for medium severity since it breaks the functionality (the raffle can be cancelled when it shouldn't be) and thus the raffle that should've been finalised, wouldn't be finalised:

Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

The decision remains the same, accept the escalation and validate with medium severity. The duplication list has been updated.

[0xsimao]

@WangSecurity could you elaborate on this part?

We should assume people act in their best interest and if they want to ensure the round is drawn, they can just buy the number of tickets left to minTicketsThreshold + 1 to guarantee it is not cancelled. This is because it is only possible to cancel after the raffle ends, so they have plenty of time to do so.

Due to this, this will never happen unless users want to, which is fair.

Also, this boils down to just 1 difference in the number of tickets needed to cancel, I don't see how this is breaking core functionality as it is a very small error.

[yashar0x]

hey @WangSecurity
i don't see my report (#114) in the duplicate list

[WangSecurity]

We should assume people act in their best interest and if they want to ensure the round is drawn, they can just buy the number of tickets left to minTicketsThreshold + 1 to guarantee it is not cancelled. This is because it is only possible to cancel after the raffle ends, so they have plenty of time to do so

It's fair argument, but still, the scenario where the bought tickets equal exactly the minimum tickets threshold and no one wants to buy more is completely viable. I understand that it's only one ticket difference, but this can fairly happen.
About the broken functionality, the raffle shouldn't be cancellable when the tickets reach that min threshold, but this report clearly shows how it can be reached. Thus, the raffle that should've been drawn, got cancelled. That's why it qualifies for medium severity as a broken core contract functionality, rendering the contract useless.

The decision remains, accept the escalation and validate with medium severity. The duplication list has been updated.

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• aslanbekaibimov: accepted

[shikhar229169]

@WangSecurity
#270
The above is also a dup of this.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
Winnables/public-contracts#23