`claimPrizesForRound` transfers the entire amount deposited for a prize regardless of the number of winners

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

claimPrizesForRound() transfers the entire amount of a prize to a winner without considering the total number of winners for that prize.

The prize for a given round and prize index can be set by calling the setPrize() function, which pulls the amounts from the caller (the owner) and stores the prize data in the prizes array:

ThrusterTreasure.sol#L163-L184

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

function depositPrize(address _from, uint256 _amountWETH, uint256 _amountUSDB) internal {
    WETH.transferFrom(_from, address(this), _amountWETH);
    USDB.transferFrom(_from, address(this), _amountUSDB);
    emit DepositedPrizes(_amountWETH, _amountUSDB);
}

However, the claimPrizesForRound() function always transfers the full prize amounts to the first caller, regardless of the number of winners for the prize. Once the prize for a specific index is claimed, other winners of that prize cannot claim their share (or winners of other prizes may end up not being able to claim theirs), effectively being denied their winnings:

ThrusterTreasure.sol#L102-L134

function claimPrizesForRound(uint256 roundToClaim) external {
    ...
    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);
            }
        }
    }
    ...
}

function _claimPrize(Prize memory _prize, address _receiver, uint256 _winningTicket) internal {
    uint256 amountETH = _prize.amountWETH;
    uint256 amountUSDB = _prize.amountUSDB;
    WETH.transfer(_receiver, amountETH);
    USDB.transfer(_receiver, amountUSDB);
    emit ClaimedPrize(_receiver, _prize.round, _prize.prizeIndex, amountETH, amountUSDB, _winningTicket);
}

This approach can lead to scenarios where the amount available to be distributed among prize winners is less than that represented by the prizes stored in the prizes array.

This is considered medium severity because:

• claimPrizesForRound() will revert if there aren't enough funds to transfer the prize to the winner, altering the user
• the owner can mitigate this by simply "refilling" the prize as many times as needed

Proof of Concept

1. A prize is set with a certain amount of WETH and USDB for a specific round and prize index, intended for multiple winners.
2. Multiple users enter the round with tickets that end up winning this prize.
3. The first user to call claimPrizesForRound() for this round and prize index successfully claims the entire prize amount.
4. Subsequent winners attempting to claim their share of the prize for the same round and prize index find that they cannot, as the prize has already been fully distributed to the first caller.

Tools Used

Manual review

Recommended Mitigation Steps

It is unclear whether the amounts passed to setPrize() are meant to be distributed among all winners of the given prize or to be paid out to each winner, but the cleaner approach would be the latter. In that case, the amount pulled from the owner can simply be scaled by the number of winners:

function depositPrize(address _from, uint64 _numWinners, uint256 _amountWETH, uint256 _amountUSDB) internal {
    WETH.transferFrom(_from, address(this), _amountWETH * _numWinners);
    USDB.transferFrom(_from, address(this), _amountUSDB * _numWinners);
    emit DepositedPrizes(_numWinners, _amountWETH, _amountUSDB);
}

Assessed type

Other

[jooleseth]

I agree with the Warden's evaluation

[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