code-423n4 / 2023-08-pooltogether-findings

4 stars 3 forks source link

It is possible that function `rngComplete()` does not iterate through all rewards #143

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngRelayAuction.sol#L157-L173

Vulnerability details

Impact

In RngRelayAuction.sol we have rngComplete():

function rngComplete(
    uint256 _randomNumber,
    uint256 _rngCompletedAt,
    address _rewardRecipient,
    uint32 _sequenceId,
    AuctionResult calldata _rngAuctionResult
  ) external returns (bytes32) {
    if (_sequenceHasCompleted(_sequenceId)) revert SequenceAlreadyCompleted();
    uint64 _auctionElapsedSeconds = uint64(block.timestamp < _rngCompletedAt ? 0 : block.timestamp - _rngCompletedAt);
    if (_auctionElapsedSeconds > (_auctionDurationSeconds-1)) revert AuctionExpired();
    // Calculate the reward fraction and set the draw auction results
    UD2x18 rewardFraction = _fractionalReward(_auctionElapsedSeconds);
    _auctionResults.rewardFraction = rewardFraction;
    _auctionResults.recipient = _rewardRecipient;
    _lastSequenceId = _sequenceId;

    AuctionResult[] memory auctionResults = new AuctionResult[](2);
    auctionResults[0] = _rngAuctionResult;
    auctionResults[1] = AuctionResult({
      rewardFraction: rewardFraction,
      recipient: _rewardRecipient
    });

    uint32 drawId = prizePool.closeDraw(_randomNumber);

    uint256 futureReserve = prizePool.reserve() + prizePool.reserveForOpenDraw();
    uint256[] memory _rewards = RewardLib.rewards(auctionResults, futureReserve);

    emit RngSequenceCompleted(
      _sequenceId,
      drawId,
      _rewardRecipient,
      _auctionElapsedSeconds,
      rewardFraction
    );

    for (uint8 i = 0; i < _rewards.length; i++) {
      uint104 _reward = uint104(_rewards[i]);
      if (_reward > 0) {
        prizePool.withdrawReserve(auctionResults[i].recipient, _reward);
        emit AuctionRewardDistributed(_sequenceId, auctionResults[i].recipient, i, _reward);
      }
    }

    return bytes32(uint(drawId));
  }

This function handles the completion of an RNG relay auction. The problem is this block of code:

for (uint8 i = 0; i < _rewards.length; i++) {
      uint104 _reward = uint104(_rewards[i]);
      if (_reward > 0) {
        prizePool.withdrawReserve(auctionResults[i].recipient, _reward);
        emit AuctionRewardDistributed(_sequenceId, auctionResults[i].recipient, i, _reward);
      }
    }

The variable i is declared as a uint8, which means it can only hold values from 0 to 255. If _rewards.length were to be more than 255, the function will revert.

Proof of Concept

The issue lies in the use of uint8 as the loop iterator, which can only represent values between 0 to 255. If the length of the _rewards array exceeds 255, the loop will revert.

In rngComplete() we see the _rewards is uint256:

157: uint256[] memory _rewards = RewardLib.rewards(auctionResults, futureReserve);

After this in the for loop i is uint8:

for (uint8 i = 0; i < _rewards.length; i++) {
      uint104 _reward = uint104(_rewards[i]);
      if (_reward > 0) {
        prizePool.withdrawReserve(auctionResults[i].recipient, _reward);
        emit AuctionRewardDistributed(_sequenceId, auctionResults[i].recipient, i, _reward);
      }
    }

But _rewards.length can be bigger than uint8. And if so the function will revert.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

I recommend changing the data type of the iterator from uint8 to uint256, as these data types can handle a much larger range of values. By making this change, the functions would be able to process any number of rewards.

Assessed type

Loop

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

raymondfam commented 1 year ago

QA at best. And, it complements the out-of-bound known issue in the bot.

HickupHH3 commented 1 year ago

see #34

c4-judge commented 1 year ago

HickupHH3 marked the issue as unsatisfactory: Insufficient proof