Dynamic modification of `maxPrizeCount` affects prize claims

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

The ThrusterTreasure contract is designed to manage rounds of a lottery game, where participants can enter tickets and claim prizes based on random draws. The contract includes a variable maxPrizeCount which dictates the maximum number of prizes that can be set for any given round. This variable can be modified by the contract owner at any time through the setMaxPrizeCount(uint256 _maxPrizeCount) function:

ThrusterTreasure.sol#L139-L142

    function setMaxPrizeCount(uint256 _maxPrizeCount) external onlyOwner {
        maxPrizeCount = _maxPrizeCount;
        emit SetMaxPrizeCount(_maxPrizeCount);
    }

The issue arises when maxPrizeCount is decreased after prizes for a round have been set but before they have been claimed. Since the claimPrizesForRound(uint256 roundToClaim) function iterates over prize indices up to maxPrizeCount, reducing this count means that winners of prizes with indices higher than the new maxPrizeCount will be unable to claim their winnings:

ThrusterTreasure.sol#L102-L120

    function claimPrizesForRound(uint256 roundToClaim) external {
        ...

        uint256 maxPrizeCount_ = maxPrizeCount;
        for (uint256 i = 0; i < maxPrizeCount_; i++) {
            [claim prize]
        }
        entered[msg.sender][roundToClaim] = Round(0, 0, roundToClaim); // Clear user's tickets for the round
        emit CheckedPrizesForRound(msg.sender, roundToClaim);
    }

This could lead to a scenario where legitimate winners are denied their prizes due to a change in contract state that is unrelated to the rules of the game or their actions. Moreover, since calling claimPrizesForRound() clears the user's entries for the round, reverting maxPrizeCount to its previous state does not allow them to claim the remaining tickets. This means they will effectively never be able to claim their prize.

Proof of Concept

1. The contract owner sets maxPrizeCount to 5 and configures five prizes for a given round.
2. Users participate in the round, and the round concludes with winners determined for all five prizes.
3. The contract owner reduces maxPrizeCount to 3 for the next round.
4. Winners of prizes 4 and 5 attempt to claim their prizes but are unable to do so because the claimPrizesForRound(uint256 roundToClaim) function now iterates only up to the new maxPrizeCount of 3.

Tools Used

Manual review

Recommended Mitigation Steps

To address this issue, implementing a checkpoint pattern for the maxPrizeCount variable is suggested. This method involves tracking changes to maxPrizeCount with checkpoints that record the value and the round number when the change occurs.

A possible implementation could look like this:

// Add a struct to store checkpoints for maxPrizeCount changes
struct MaxPrizeCountCheckpoint {
    uint256 round;
    uint256 maxPrizeCount;
}

// Use an array to keep track of all checkpoints
MaxPrizeCountCheckpoint[] public maxPrizeCountCheckpoints;

constructor(
    ...
) Ownable(msg.sender) {
    maxPrizeCountCheckpoints.push(
        MaxPrizeCountCheckpoint(0, _maxPrizeCount)
    );
    ...
}

// Modify setMaxPrizeCount to push a new checkpoint to the array
function setMaxPrizeCount(uint256 _maxPrizeCount) external onlyOwner {
    require(_maxPrizeCount != getMaxPrizeCountForRound(currentRound), "same value")
    maxPrizeCountCheckpoints.push(
        MaxPrizeCountCheckpoint(currentRound, _maxPrizeCount)
    );
    emit SetMaxPrizeCount(_maxPrizeCount);
}

// Helper function to get the maxPrizeCount for a given round
// Assumes more recent rounds will be queried more often
function getMaxPrizeCountForRound(uint256 _round) public view returns (uint256) {
    uint256 length = maxPrizeCountCheckpoints.length;
    for (uint256 i = length; i > 0; i--) {
        MaxPrizeCountCheckpoint storage checkpoint = maxPrizeCountCheckpoints[i - 1];
        if (checkpoint.round <= _round) {
            return checkpoint.maxPrizeCount;
        }
    }
    return 0;
}

// Disallow setting prizes for future rounds since the maxPrizeCount could change
function setPrize(uint64 _prizeIndex, uint256 _amountWETH, uint256 _amountUSDB, uint64 _numWinners) external onlyOwner {
    uint256 maxPrizeCount = getMaxPrizeCountForRound(currentRound);
    require(_prizeIndex < maxPrizeCount, "IPC");
    ...
}

function claimPrizesForRound(uint256 roundToClaim) external {
    uint256 maxPrizeCount = getMaxPrizeCountForRound(roundToClaim);
    ...
}

This change ensures that each round's prize structure is fixed upon the round's creation, preventing post-hoc alterations that could negatively impact participants. Note that this implementation still requires attention is paid to not calling setMaxPrizeCount() for a given round if prizes have already been set for higher indices.

Assessed type

Other

[jooleseth]

I would consider this QA to ensure to add a require check that maxPrizeCount cannot be decreased as that is the intention

[0xEVom]

That's indeed a much simpler fix!

I would still say Medium severity is appropriate seeing as that could not have been inferred from the audit scope and given the potential impact, but either way I'll accept the judge's decision.

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-sponsor]

jooleseth (sponsor) confirmed

[c4-judge]

0xleastwood marked the issue as selected for report