search

sherlock-audit / 2024-08-winnables-raffles-judging

6 stars 2 forks source link

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

Open sherlock-admin3 opened 2 months ago

sherlock-admin3 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

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 commented 2 months ago

340 is a duplicate of this issue.

0xweb333 commented 2 months ago

https://github.com/sherlock-audit/2024-08-winnables-raffles-judging/issues/330 is a duplicate

kuprumxyz commented 2 months ago

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:

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

mystery0x commented 2 months ago

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

kuprumxyz commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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 commented 1 month ago

@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.

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

@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 commented 1 month ago

@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 commented 1 month ago

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:

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 commented 1 month ago

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 commented 1 month ago

hey @WangSecurity

114 is a dup of this

0xsimao commented 1 month ago

@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 commented 1 month ago

475 is duplicate of this issue

WangSecurity commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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

WangSecurity commented 1 month ago

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 commented 1 month ago

Result: Medium Has duplicates

sherlock-admin2 commented 1 month ago

Escalations have been resolved successfully!

Escalation status:

shikhar229169 commented 1 month ago

@WangSecurity https://github.com/sherlock-audit/2024-08-winnables-raffles-judging/issues/270 The above is also a dup of this.

sherlock-admin2 commented 1 month ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/Winnables/public-contracts/pull/23