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

2 stars 1 forks source link

Prize deposited through setPrize() is insufficient to be distributed to all winners #18

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#L163-L171 https://github.com/code-423n4/2024-02-thruster/blob/3896779349f90a44b46f2646094cb34fffd7f66e/thruster-protocol/thruster-treasure/contracts/ThrusterTreasure.sol#L102-L120

Vulnerability details

Impact

Prize amount deposited through setPrize() in ThrusterTreasure.sol will always mismatch the real amount to be claimed by winners, and only the first winner for each prizeIndex to claim can get the prize.

Proof of Concept

When a user tried to claim the prize, claimPrizesForRound will loop through each prizeIndex, and for each prizeIndex it will loop through all the winner tickets and compare to the msg.sender's tickets, if the winner ticket is in the range of the msg.sender tickets, _claimPrize will be called to claim the prize.

        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);
                }
            }
        }

As we can see, the prize amount to be claimed is based on prizes[roundToClaim][i], so if we have more than one winner, each winner should get the amount of _prize.amountWETH and _prize.amountUSDB. The question is, when the admin set the prize and deposit the amount, the fund they provide is only enough for just one winner.

    function setPrize(uint256 _round, uint64 _prizeIndex, uint256 _amountWETH, uint256 _amountUSDB, uint64 _numWinners)
        external
        onlyOwner
    {
        require(_round >= currentRound, "ICR");
        require(_prizeIndex < maxPrizeCount, "IPC");
        depositPrize(msg.sender, _amountWETH, _amountUSDB);
        prizes[_round][_prizeIndex] = Prize(_amountWETH, _amountUSDB, _numWinners, _prizeIndex, uint64(_round));
    }

We can see the prizes[_round][_prizeIndex] is directly set as the amount of sent funds, and those amounts are only enough for one winner to be claimed. If _numWinners is larger than one, all other winners except the first one to claim cannot receive their prize due to insufficient funds.

Tools Used

Manual Review

Recommended Mitigation Steps

When set the prize, the actual amount should be _amountWETH _numWinners and _amountUSDB _numWinners

Assessed type

Other

jooleseth commented 6 months ago

We agree with #27 on their evaluation, and label it as Medium not High, as it is possible to still deposit sufficient funds into the contract. Anyone is able to send prize funds into the contract, as we draw the prizes from the existing balance

c4-judge commented 6 months ago

0xleastwood changed the severity to 2 (Med Risk)

c4-judge commented 6 months ago

0xleastwood marked the issue as duplicate of #27

c4-judge commented 6 months ago

0xleastwood marked the issue as satisfactory