code-423n4 / 2024-02-thruster-findings

2 stars 1 forks source link

Weired design when setting the winner tickets allows one tickets to win different prizeIndex and win more than time within the same prizeIndex #19

Closed c4-bot-3 closed 6 months ago

c4-bot-3 commented 7 months ago

Lines of code

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

Vulnerability details

Impact

Under current design, there is weired behavior that one tickets can win more than one prizeIndex (like win the 0 and 1 prizeIndex at the same time) and win more than one time within the same prizeIndex.

Proof of Concept

When setting the winngin tickets for a certain prizeIndex, setWinningTickets will loop through the length of numWinners and set the winner tickets number through a random number generator.

for (uint256 i = 0; i < numWinners; i++) {
            _winningTickets[i] = revealRandomNumber(sequenceNumbers[i], userRandoms[i], providerRandoms[i]);
            emit SetWinningTicket(_round, _prizeIndex, _winningTickets[i], i);
        }

The issue is it doesn't check if the generated number is selected before, and with this it's possible for one tickets to win different prizeIndex and win more than one time within the same prizeIndex. Generally speaking, the behavior should like this: Assuming have 10 tickets and we have 1 tickets for 0 prizeIndex and 2 tickets for 1 prizeIndex, then three different tickets should be selected.

Tools Used

Manual Review

Recommended Mitigation Steps

Check if the random generated tickets number has been selected before

Assessed type

Other

jooleseth commented 6 months ago

Yes this is intentional. We allow the same ticket to be selected as the winner as it reduces complications with checking the set of winning ticket state every time a number is drawn and having to trigger a new random number to be called. This introduces a lot of overhead that we felt was not worth it, and we believe it is fine if a user is lucky enough to have his number drawn multiple times. We consider this a feature and intentional.

So this is not a bug

c4-judge commented 6 months ago

0xleastwood marked the issue as satisfactory

c4-sponsor commented 6 months ago

jooleseth (sponsor) acknowledged

c4-sponsor commented 6 months ago

jooleseth (sponsor) disputed

0xleastwood commented 6 months ago

It seems intentional that a ticket should be eligible for each prize index.

c4-judge commented 6 months ago

0xleastwood removed the grade

c4-judge commented 6 months ago

0xleastwood changed the severity to QA (Quality Assurance)