Lottery winners might lose some of their entitled prize due to vulnerable implementation in claimPrizesForRound()

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

Winners might lose some of their lottery prize rewards when calling claimPrizesForRound().

Proof of Concept

In ThrusterTreasure.sol, for each round there are likely multiple tiers of prizes (<=maxPrizeCount). Based on thruster doc, one example prize tier set-up for a round might have a grand prize, 2nd prize (2 winners), 3rd prize (5 winners), 4th prize (30 winners). There could be fewer or more prize indexes. And one user can win multiple prizes.

There are two vulnerabilities at play:

(1) The current checks in claimPrizesForRound() are vulnerable with this multi-tier prize setup, because it allows winner claiming as long as the 1st prize winners are drawn, regardless of whether prizes for all tiers are drawn.

//thruster-protocol/thruster-treasure/contracts/ThrusterTreasure.sol
    function claimPrizesForRound(uint256 roundToClaim) external {
        require(
            roundStart[roundToClaim] + MAX_ROUND_TIME >= block.timestamp,
            "ICT"
        );
        //@audit require statement only checks the 1st prize (prizeIdx=0), and ignore if other prize indexes for the round are also drawn
|>      require(winningTickets[roundToClaim][0].length > 0, "NWT");
        Round memory round = entered[msg.sender][roundToClaim];
        require(round.ticketEnd > round.ticketStart, "NTE");
        uint256 maxPrizeCount_ = maxPrizeCount;
        for (uint256 i = 0; i < maxPrizeCount_; i++) {
            Prize memory prize = prizes[roundToClaim][i];
            uint256[] memory winningTicketsRoundPrize = winningTickets[
                roundToClaim
            ][i];
            for (uint256 j = 0; j < winningTicketsRoundPrize.length; j++) {
                uint256 winningTicket = winningTicketsRoundPrize[j];
                if (round.ticketStart <= winningTicket && round.ticketEnd > winningTicket) {
                    _claimPrize(prize, msg.sender, winningTicket);
                }
            }
        }
        //@audit As long as the 1st prize is drawn, the caller's entry for the round will be deleted, if caller 's selected for the 2nd prize, they cannot claim it
|>      entered[msg.sender][roundToClaim] = Round(0, 0, roundToClaim); // Clear user's tickets for the round
        emit CheckedPrizesForRound(msg.sender, roundToClaim);
    }

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

(2) The winner drawing process for all tiers is most likely not atomic, because setWinningTickets() can only update one _prizeIndex per call. And onlyOwner is by default the deployer EOA. In addition, depending on the total number of winners, the winning tickets setting might also be split into separate transactions.

    function setWinningTickets(
        uint256 _round,
        uint256 _prizeIndex,
        uint64[] calldata sequenceNumbers,
        bytes32[] calldata userRandoms,
        bytes32[] calldata providerRandoms
    ) external onlyOwner {
...

(https://github.com/code-423n4/2024-02-thruster/blob/3896779349f90a44b46f2646094cb34fffd7f66e/thruster-protocol/thruster-treasure/contracts/ThrusterTreasure.sol#L269-L275)

When setWinningTickets are separated by prizeIndex, the sequencer might pick any prizeIndex's tx first. In the case where winningTickets[roundToClaim][0] is set, claimPrizesForRound() will allow user claiming.

However, the impact is that the 1st prize winner (msg.sender) also get selected for lower-tier prizes. 1st prize winner calling claimPrizesForRound() will claim the 1st prize but will lose their lower-tier prizes. Or the 2nd prize winner loses their 3rd prize win, etc.

Tools Used

Manual

Recommended Mitigation Steps

In claimPrizesForRound(), check whether all prize indexes for a given round have been drawn instead of only index 0.

Assessed type

Other

[jooleseth]

This is the same as #28

[c4-judge]

0xleastwood marked the issue as duplicate of #28

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood