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

6 stars 2 forks source link

Cheery Tangerine Bear - Lack of constraints on edge cases #621

Closed sherlock-admin4 closed 3 months ago

sherlock-admin4 commented 3 months ago

Cheery Tangerine Bear

Low/Info

Lack of constraints on edge cases

Summary

The lack of constraints on the edge case where currentTicketSold == minTicketsThreshold allows the caller to freely choose between canceling the raffle or drawing a winner.

Vulnerability Detail

The contract does not restrict whether the raffle should be canceled or a winner should be drawn when currentTicketSold == minTicketsThreshold, allowing the caller to freely choose between the two options.

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

Impact

Since anyone can call the functions to cancel the raffle or draw a winner, the choice made when currentTicketSold == minTicketsThreshold might go against the project's intent.

Code Snippet

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

Tool used

Manual Review

Recommendation

    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 (block.timestamp <= raffle.endsAt) {
            if (currentTicketSold < raffle.maxTicketSupply) revert RaffleIsStillOpen();
        }
-       if (currentTicketSold < raffle.minTicketsThreshold) revert TargetTicketsNotReached(); 
+       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();
    }
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