Tickets will be lost if they're entered after `MAX_ROUND_TIME`

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-02-thruster/blob/3896779349f90a44b46f2646094cb34fffd7f66e/thruster-protocol/thruster-treasure/contracts/ThrusterTreasure.sol#L83 https://github.com/code-423n4/2024-02-thruster/blob/3896779349f90a44b46f2646094cb34fffd7f66e/thruster-protocol/thruster-treasure/contracts/ThrusterTreasure.sol#L276

Vulnerability details

Impact

The ThrusterTreasure specifies a maximum round time (MAX_ROUND_TIME) intended to limit the period during which winners can be chosen and prizes claimed for a given round. However, the enterTickets() function does not enforce this time limit: ThrusterTreasure.sol#L83-L96

    function enterTickets(uint256 _amount, bytes32[] calldata _proof) external {
        uint256 currentRound_ = currentRound;
        require(winningTickets[currentRound_][0].length == 0, "ET");
        bytes32 node = keccak256(abi.encodePacked(msg.sender, _amount));
        require(MerkleProof.verify(_proof, root, node), "IP");
        uint256 ticketsToEnter = _amount - cumulativeTickets[msg.sender];
        require(ticketsToEnter > 0, "NTE");
        uint256 currentTickets_ = currentTickets;
        Round memory round = Round(currentTickets_, currentTickets_ + ticketsToEnter, currentRound_);
        entered[msg.sender][currentRound_] = round;
        cumulativeTickets[msg.sender] = _amount; // Ensure user can only enter tickets once, no partials
        currentTickets += ticketsToEnter;
        emit EnteredTickets(msg.sender, currentTickets_, currentTickets_ + ticketsToEnter, currentRound_);
    }

This means users can continue to enter tickets even after the MAX_ROUND_TIME has elapsed, provided that the winning tickets for the round have not been set. This can lead to scenarios where tickets are entered into a round that cannot have winners determined, effectively rendering these tickets useless.

Proof of Concept

1. MAX_ROUND_TIME passes for a lottery round in ThrusterTreasure without setting winning tickets, possibly due to no prizes.
2. A user (Bob) enters tickets after this period.
3. Since the round's prize setting period has expired, Bob's tickets cannot win, effectively rendering them useless.

Tools Used

Manual review

Recommended Mitigation Steps

Add a check in the enterTickets() function to ensure that the current time is within the MAX_ROUND_TIME from the round's start time. Ideally this is combined with a check that none of the prizes have been set yet, as detailed in another finding.

Assessed type

Other

[jooleseth]

This isn't an issue. We have a check that winning tickets have not been drawn yet. If there are no winning tickets and the max round time is over, then the entire round will be invalidated.

[0xEVom]

Yes, this issue only appears in case there are no winning tickets.

How would the entire round be invalidated? The way I see it, the main issue here is that this updates the caller's cumulativeTickets, which cannot be corrected and can happen up until the point a new round is started.

You could potentially counteract that by adding the amount of tickets the user entered for the round to their leaf again, but that's rather patchy and you could always miss a user because they called enterTickets at the very last moment.

[flowercrimson]

The way I see it: Owner/Admin has full control over when to roll-over to a new round (before MAX_ROUND_TIME). Based on Thruster Doc, there's mention of a weekly cycle of lottery drawing (much less than 30-day MAX_ROUNT_TIME). And to roll over to a new round, owner/admin can call setRoot(), which increments currentRound. Any cumulativeTickets added after setRoot() will not be lost, and effectively counted towards a new round.

24 hours ahead of the draw, users with eligible activity on Thruster from the six days prior will get a notification to claim their tickets for that week. At the end of that 24 hours, a random selection of numbers / tickets will be selected through Pyth Entropy (on-chain verifiable randomness solution).

[0xEVom]

Those are the current advertised terms, but this is assuming they may change in the future and not always happen on a weekly basis. The implementation allows for more flexibility than what is currently described in the docs, both in terms of prizes and in terms of length. If at any time prizes become a more sporadic occurrence rather than taking place on such a regular basis, this will become an issue.

[c4-judge]

0xleastwood marked the issue as satisfactory

[jooleseth]

I acknowledge that the design is fit for a consistent period of time of at most 30 days to enter and draw, as specified. However, there are several ways to mitigate the issue in the off chance that the round is not finalized within 30 days, since admin has control of the merkle evaluations.

I consider this issue a Low

[c4-sponsor]

jooleseth marked the issue as disagree with severity

[c4-sponsor]

jooleseth (sponsor) acknowledged

[0xleastwood]

Seems more like a user error to be entering into a round after MAX_ROUND_TIME has passed and there are paths to recovery when a round is not finalized within MAX_ROUND_TIME. Downgrading to low severity.

[c4-judge]

0xleastwood removed the grade

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)