Oblivionis - raffle is both drawable and cancelable when currentTicketSold = minTicketsThreshold

[sherlock-admin4]

Oblivionis

Medium

raffle is both drawable and cancelable when currentTicketSold = minTicketsThreshold

Summary

Now there's a game-theory issue in the protocol: a raffle is both drawable and cancelable when currentTicketSold = minTicketsThreshold. This issue enables an attacker to force cancel a lottery that should have drawn results

Vulnerability Detail

WinnablesTicketManager.sol:

    function _checkTicketPurchaseable(uint256 raffleId, uint256 ticketCount) internal view {
        Raffle storage raffle = _raffles[raffleId];
        if (raffle.start sAt > block.timestamp) revert RaffleHasNotStarted(); 
        if (raffle.status != RaffleStatus.IDLE) revert RaffleHasEnded();
        if (block.timestamp > raffle.endsAt) revert RaffleHasEnded();
        uint256 ticketPurchased = uint256(uint32(uint256(raffle.participations[msg.sender]) >> 128));
        unchecked {
            if (ticketPurchased + ticketCount > raffle.maxHoldings) revert TooManyTickets();
        }
        uint256 supply = IWinnablesTicket(TICKETS_CONTRACT).supplyOf(raffleId);
        unchecked {
            if (supply + ticketCount > raffle.maxTicketSupply) revert TooManyTickets();
        }
    }

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

From the logic above, we can conclude that:

1. when block.timestamp >= raffle.endsAt, the raffle is both drawable and cancelable.
2. when currentTicketSold == minTicketsThreshold, the raffle is both drawable and cancelable.

In terms of game theory, we are in an awkward situation -- A raffle that should have been cancelled can be drawn, a raffle that should have been drawn can be cancelled. This means that an attacker can deprive participants' right to earn a reward by calling cancelRaffle.

Impact

impact: medium - A raffle that should have been cancelled can be drawn, a raffle that should have been drawn can be cancelled.

likelihood: medium - this situation happens everytime when currentTicketSold = minTicketsThreshold.

Severity: boardline medium

Code Snippet

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

Tool used

Manual Review

Recommendation

shouldDrawRaffle and shouldCancelRaffle should never return true at same time.

Duplicate of #26

[Oblivionis214]

escalate

not sure why this is invalid.

[sherlock-admin3]

escalate

not sure why this is invalid.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page,
in the Sherlock webapp.

[BoRonG0d]

escalate
delegate to @Oblivionis214

[sherlock-admin3]

escalate
delegate to @Oblivionis214

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.

[mystery0x]

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

[WangSecurity]

I'm not sure I understand the medium impact:

impact: medium - A raffle that should have been cancelled can be drawn, a raffle that should have been drawn can be cancelled

Why do you think it shouldn't be a case where you can either cancel or draw the raffle when the min ticket threshold is met?

[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:

• BoRonG0d: accepted

[sherlock-admin2]

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