neko_nyaa - Raffles with exactly minTicketsThreshold tickets sold can still be cancelled

[sherlock-admin3]

neko_nyaa

Medium

Raffles with exactly minTicketsThreshold tickets sold can still be cancelled

Summary

Wrong conditional check/comparison in minTicketsThreshold will cause raffles with exactly minTicketsThreshold tickets to still be cancellable.

Root Cause

In checkShouldCancel():

function _checkShouldCancel(uint256 raffleId) internal view {
    // ...
    if (supply > raffle.minTicketsThreshold) revert TargetTicketsReached(); // @audit if supply == minTicketsThreshold then still cancellable
}

https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesTicketManager.sol#L440

Note that when the number of tickets sold is equal to minTicketsThreshold of the raffle, then the raffle is still cancellable. This is inconsistent with the code documentation about the use of minTicketsThreshold.

/// @return minTicketsThreshold minimum number of tickets that needs to be sold before
///         this raffle is elligible for drawing a winner

https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesTicketManager.sol#L75-L76

Furthermore, this is also inconsistent with _checkShouldDraw() for checking if a raffle should be drawable.

function _checkShouldDraw(uint256 raffleId) internal view {
    // ...
    if (currentTicketSold < raffle.minTicketsThreshold) revert TargetTicketsNotReached(); // @audit if target ticket is strictly less, then raffle is not drawable
}

One must also note that cancelRaffle() was designed to be callable by anyone (so as to prevent prize locking).

https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesTicketManager.sol#L278

Therefore, when a raffle ends up with exactly minTicketsThreshold, anyone can still cancel the raffle and deny the potential winner their winnings, even if the raffle was supposed to be drawable.

Internal pre-conditions

1. The raffle needs to have had exactly minTicketsThreshold sold

External pre-conditions

No response

Attack Path

No response

Impact

If a raffle ends up with exactly minTicketsThreshold, anyone can still cancel the raffle and deny the potential winner their winnings, even if the raffle was supposed to be drawable.

PoC

No response

Mitigation

The correct comparison should be if (supply >= raffle.minTicketsThreshold) revert TargetTicketsReached();

Duplicate of #26

[neko-nyaa]

Escalate. There are multiple escalated issues that can be duped with this. This is to bring attention to also validate this in case the other duped issues are accepted.

[sherlock-admin3]

Escalate. There are multiple escalated issues that can be duped with this. This is to bring attention to also validate this in case the other duped issues are accepted.

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]

A similar issue with involved discussion is going on #26

[mystery0x]

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

[v-kirilov]

A similar issue with involved discussion is #395

[WangSecurity]

Planning to accept the escalation, validate with medium severity and duplicate with #26. For more details, see the discussion under #26.

[WangSecurity]

Result:
Medium
Duplicate of #26

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• neko-nyaa: accepted

[sherlock-admin2]

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