0x0x0xw3 - Raffle can be canceled even if minTicketsThreshold is reached

[sherlock-admin4]

0x0x0xw3

Medium

Raffle can be canceled even if minTicketsThreshold is reached

Summary

WinnablesTicketManager::cancelRaffle will allow anyone to cancel a raffle even if minTicketsThreshold is reached due to a bad comparission in WinnablesTicketManager::_checkShouldCancel

Vulnerability Detail

The vulnerability reside in WinnablesTicketManager::_checkShouldCancel (called by WinnablesTicketManager::cancelRaffle).
This function returns that a raffle cannot be canceled if ticket's supply is greater than raffle.minTicketsThreshold, but if ticket's supply is equal to minTicketsThreshold it allows to be canceled.
This is because it doesnt take into account where the minTicketsThreshold is reached due to a bad comparission:

function _checkShouldCancel(uint256 raffleId) internal view {
    Raffle storage raffle = _raffles[raffleId];
    //...snippet
    if (supply > raffle.minTicketsThreshold) revert TargetTicketsReached(); // <- bad comparission
}

So if raffle ends with minTicketsThreshold reached, still can be canceled by anyone breaking invariant defined in IWinnables's Raffle struct.

Impact

Invariant breakage about raffle conditionals
DoS when raffle reaches minTicketsThreshold

Code Snippet

https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/81b28633d0f450e33a8b32976e17122418f5d47e/public-contracts/contracts/WinnablesTicketManager.sol#L434-L441

Tool used

Manual Review

Recommendation

Change comparator in WinnablesTicketManager::_checkShouldCancel to greater or equal:

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();  //<- changed
}

Duplicate of #26

[WangSecurity]

This will be duplicated with #26 and validated with Medium severity.

[sherlock-admin2]

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