Tickets can be entered after prizes for current round have partially been distributed

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

The ThrusterTreasure contract is designed to facilitate a lottery game where users can enter tickets to win prizes based on entropy. The contract includes mechanisms for entering tickets into rounds (enterTickets()), setting prizes for rounds (setPrize()), and claiming prizes (claimPrizesForRound()). A critical aspect of the game's integrity is ensuring each ticket has an equal chance to win every prize.

However, there is a significant flaw in enterTickets(). The function checks if winning tickets for the prize index 0 have been set by verifying that winningTickets[currentRound_][0].length == 0. This check is intended to prevent users from entering tickets after prizes have begun to be distributed, but it does not account for prizes with higher indices that may already have been distributed. As a result, users can still enter tickets after some prizes have been distributed, but these late-entered tickets will not have a chance to win the already distributed prizes: ThrusterTreasure.sol#L83-L96

function enterTickets(uint256 _amount, bytes32[] calldata _proof) external {
    ...
    require(winningTickets[currentRound_][0].length == 0, "ET");
    ...
}

Proof of Concept

1. The contract owner sets up a new round with multiple prizes.
2. User A enters tickets early in the round.
3. The contract owner distributes prizes for index 1.
4. User B enters tickets into the round.
5. Due to the flawed logic in enterTickets(), User B's tickets are accepted, even though the prizes for indices 1 and above have already been distributed. User B's tickets, therefore, have no chance of winning those prizes and are worth less than user A's, but the system incorrectly allows their participation for the undistributed prize at index 0.

Tools Used

Manual review

Recommended Mitigation Steps

Freeze ticket entry for the current round once any prize has been set.

Assessed type

Other

[jooleseth]

This is an issue reported in a few other Medium's as well

• 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.

We also use this zero index check in the claimPrizesForRound call

I would consider this a Quality Assurance to improve the require check, or Medium at most, as reported by https://github.com/code-423n4/2024-02-thruster-findings/issues/17

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-sponsor]

jooleseth (sponsor) confirmed

[c4-sponsor]

jooleseth marked the issue as disagree with severity

[0xleastwood]

It seems to me that users would have to intentionally be negligible and enter tickets into a round that they are not eligible for. Downgrading to QA.

[c4-judge]

0xleastwood removed the grade

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[0xEVom]

@0xleastwood all users are eligible for prizes as long as they have valid tickets. The user cannot know that prizes have already been set - even if they checked before entering their tickets, an owner call to setWinningTickets() may end up being included in a block before their transaction.

Setting rewards atomically via a script addresses the issue, but it is an OOS mitigation. Considering that setting them non-atomically in both ascending and descending order is problematic and that this was not documented, Medium severity seems reasonable.

[0xleastwood]

Understandably, this issue would not be mitigated by having all prizes set at the same time because any tickets entered prior would not be eligible for any reward. There needs to be some clear distinction at which a round ends and when prizes are distributed to prevent this from happening. Would typically class front-running issues as QA but this leads to user's spending funds with no expected return.

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood marked the issue as selected for report