Risk of losing prizes for early claims in `ThrusterTreasure`

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-thruster/blob/3896779349f90a44b46f2646094cb34fffd7f66e/thruster-protocol/thruster-treasure/contracts/ThrusterTreasure.sol#L98-L104 https://github.com/code-423n4/2024-02-thruster/blob/3896779349f90a44b46f2646094cb34fffd7f66e/thruster-protocol/thruster-treasure/contracts/ThrusterTreasure.sol#L261-L277

Vulnerability details

Impact

The ThrusterTreasure contract facilitates a lottery game where users can enter tickets and claim prizes based on random draws. The contract uses a combination of user ticket entries, Merkle proofs for verification, and an entropy source for drawing winning tickets.

The claimPrizesForRound() function allows users to claim their prizes for a specific round. However, there's a significant issue where a user can claim their prize as soon as the winning tickets for the first prize index are set, without waiting for all prizes within the round to be determined. This premature claiming could lead to a scenario where users' tickets are cleared from the round after calling claimPrizesForRound() without receiving any prize, even if they had one or more winning tickets.

ThrusterTreasure.sol#L98-L104

    /**
     * Claim prizes for a round
     * @param roundToClaim - The round to claim prizes for
     */
    function claimPrizesForRound(uint256 roundToClaim) external {
        require(roundStart[roundToClaim] + MAX_ROUND_TIME >= block.timestamp, "ICT");
        require(winningTickets[roundToClaim][0].length > 0, "NWT");

        ...

        entered[msg.sender][roundToClaim] = Round(0, 0, roundToClaim); // Clear user's tickets for the round
        emit CheckedPrizesForRound(msg.sender, roundToClaim);
    }

ThrusterTreasure.sol#L261-L277

    /**
     *
     * @param _round - The round to claim the prize for
     * @param _prizeIndex - The index of the prize to claim
     * @param sequenceNumbers - The sequence numbers of the random number requests
     * @param userRandoms - The user random numbers
     * @param providerRandoms - The provider random numbers
     */
    function setWinningTickets(
        uint256 _round,
        uint256 _prizeIndex,
        uint64[] calldata sequenceNumbers,
        bytes32[] calldata userRandoms,
        bytes32[] calldata providerRandoms
    ) external onlyOwner {
        require(roundStart[_round] + MAX_ROUND_TIME >= block.timestamp, "ICT");
        require(winningTickets[_round][_prizeIndex].length == 0, "WTS");
        ...

This is considered high severity because it:

• leads to loss of (potentially matured) yield/rewards
• does not require an error on either the owner or the user's part
• can happen to the user without them ever becoming aware of it

Proof of Concept

1. The owner sets the winning tickets for the prize with index 0 of a round using setWinningTickets().
2. A user, who has entered tickets for the round and who has a winning ticket for the prize with index 1, calls claimPrizesForRound() and claims their prize.
3. The owner then sets the winning tickets for the remaining prize indexes of the round.
4. The user is now unable to claim their prize because their tickets have been cleared from the round after calling claimPrizesForRound().

Tools Used

Manual review

Recommended Mitigation Steps

To address this issue, it is recommended to implement a mechanism that ensures all prizes for a round are set before any prize claims can be made. This could be achieved by:

1. Introducing a state variable that tracks whether all prizes for a round have been set.
2. Modifying the claimPrizesForRound() function to check this state variable before allowing any prize claims.

A possible implementation could look like this:

// Add a state variable to track if all prizes for the round have been set
mapping(uint256 => bool) public allPrizesSetForRound;

// Modify the setPrize function to set allPrizesSetForRound to true when the last prize is set
// This requires setting prizes in sequential order
function setPrize(uint256 _round, uint64 _prizeIndex, uint256 _amountWETH, uint256 _amountUSDB, uint64 _numWinners) external onlyOwner {
    require(_round >= currentRound, "ICR");
    require(_prizeIndex < maxPrizeCount, "IPC");
    // Existing implementation
    // ...
    if (_prizeIndex == maxPrizeCount - 1) {
        allPrizesSetForRound[_round] = true;
    }
}

// Modify the claimPrizesForRound function to check if all prizes have been set
function claimPrizesForRound(uint256 roundToClaim) external {
    require(allPrizesSetForRound[roundToClaim], "Not all prizes set");
    // Existing implementation
    // ...
}

This solution ensures that users can only claim prizes once all prizes for the round have been determined, preserving the fairness and integrity of the lottery.

Assessed type

Other

[jooleseth]

I don't consider this a high risk issue, because:

• Rewards should be set atomically in the same transaction call by an admin script. The expectation is that if the 0 index is set, then all should be set for the round, hence the check for the zero index.

• User would need to directly interact with the contract knowing that they have 0 rewards to claim, as the UI would not prompt the user to execute a transaction if they do not have any winning tickets.

I would consider this a Quality Assurance to improve the require check, or Medium at most, as reported by #17

[jooleseth]

We also use this 0 index check in other methods, like the enterTickets function because we have the expectation mentioned in bullet point 1

[c4-judge]

0xleastwood marked the issue as satisfactory

[jooleseth]

This is a duplicate of #27

[c4-sponsor]

jooleseth marked the issue as disagree with severity

[c4-sponsor]

jooleseth (sponsor) acknowledged

[0xleastwood]

Same reason as #28

[c4-judge]

0xleastwood removed the grade

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[0xleastwood]

I guess i should be consistent here even if the admin would atomically set the prizes all at once such that this would not happen. It's not documented so this can't be assumed.

[0xleastwood]

But this is something only the admin has access to do, so if their initial plan was to atomically set the prizes then i don't believe this to be valid for medium severity still. This is not likely to happen.

[0xleastwood]

Keeping it as is.