PoolTogether V5: Part Deux - 0xbepresent's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngRelayAuction.sol#L131 https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngRelayAuction.sol#L154 https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngRelayAuction.sol#L170

Vulnerability details

Impact

The rngComplete() function is called by the RngAuctionRelayerDirect contract or the RngAuctionRelayerRemoteOwner contract. The rngComplete() function receives the RNG results from the rng auction.

The problem here is that the rngComplete() function can be called by anyone not only the relayers. This is very dangerous because malicious actors can close draws without any restriction using their own malicious randomNumber. Additionally the malicious actor can receive the rewards.

Proof of Concept

The rngComplete() function does not have any restriction about who can call it so in the code line 154 the prizePool will close the draw using a custom number specified in the code line 132 by the malicious actor.

File: RngRelayAuction.sol
131:   function rngComplete(
132:     uint256 _randomNumber,
133:     uint256 _rngCompletedAt,
134:     address _rewardRecipient,
135:     uint32 _sequenceId,
136:     AuctionResult calldata _rngAuctionResult
137:   ) external returns (bytes32) {
138:     if (_sequenceHasCompleted(_sequenceId)) revert SequenceAlreadyCompleted();
139:     uint64 _auctionElapsedSeconds = uint64(block.timestamp < _rngCompletedAt ? 0 : block.timestamp - _rngCompletedAt);
140:     if (_auctionElapsedSeconds > (_auctionDurationSeconds-1)) revert AuctionExpired();
141:     // Calculate the reward fraction and set the draw auction results
142:     UD2x18 rewardFraction = _fractionalReward(_auctionElapsedSeconds);
143:     _auctionResults.rewardFraction = rewardFraction;
144:     _auctionResults.recipient = _rewardRecipient;
145:     _lastSequenceId = _sequenceId;
146: 
147:     AuctionResult[] memory auctionResults = new AuctionResult[](2);
148:     auctionResults[0] = _rngAuctionResult;
149:     auctionResults[1] = AuctionResult({
150:       rewardFraction: rewardFraction,
151:       recipient: _rewardRecipient
152:     });
153: 
154:     uint32 drawId = prizePool.closeDraw(_randomNumber);
155: 
156:     uint256 futureReserve = prizePool.reserve() + prizePool.reserveForOpenDraw();
157:     uint256[] memory _rewards = RewardLib.rewards(auctionResults, futureReserve);
158: 
159:     emit RngSequenceCompleted(
160:       _sequenceId,
161:       drawId,
162:       _rewardRecipient,
163:       _auctionElapsedSeconds,
164:       rewardFraction
165:     );
166: 
167:     for (uint8 i = 0; i < _rewards.length; i++) {
168:       uint104 _reward = uint104(_rewards[i]);
169:       if (_reward > 0) {
170:         prizePool.withdrawReserve(auctionResults[i].recipient, _reward);
171:         emit AuctionRewardDistributed(_sequenceId, auctionResults[i].recipient, i, _reward);
172:       }
173:     }
174: 
175:     return bytes32(uint(drawId));
176:   }

Tools used

Manual review

Recommended Mitigation Steps

Restrict only the relayers can call the rngComplete() function.

Assessed type

Access Control

Lines of code

https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngRelayAuction.sol#L170 https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngAuction.sol#L170 https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngAuctionRelayerDirect.sol#L36 https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngAuctionRelayerRemoteOwner.sol#L59

Vulnerability details

Impact

The rngComplete() function receives the RNG request results, it closes the draw using the randomNumber generated by the RNG request auction and it transfer the rewards.

The problem is that a malicious actor can introduce a address zero in the recipient reward causing the revert of the rngComplete() function, this will cause a destabilization because:

• Draws will not be closed causing problems for the winners to claim prizes
• The protocol will be unable to close draws and start the next one.

Proof of Concept

In the RNG request auction there is not any restriction about to use address(0) in the _rewardRecipient parameter.

  function startRngRequest(address _rewardRecipient) external {
    if (!_canStartNextSequence()) revert CannotStartNextSequence();

    RNGInterface rng = _nextRng;

    uint64 _auctionElapsedTimeSeconds = _auctionElapsedTime();
    if (_auctionElapsedTimeSeconds > auctionDuration) revert AuctionExpired();

    (address _feeToken, uint256 _requestFee) = rng.getRequestFee();
    if (_feeToken != address(0) && _requestFee > 0) {
      if (IERC20(_feeToken).balanceOf(address(this)) < _requestFee) {
        // Transfer tokens from caller to this contract before continuing
        IERC20(_feeToken).transferFrom(msg.sender, address(this), _requestFee);
      }
      // Increase allowance for the RNG service to take the request fee
      IERC20(_feeToken).safeIncreaseAllowance(address(rng), _requestFee);
    }

    (uint32 rngRequestId,) = rng.requestRandomNumber();
    uint32 sequenceId = _openSequenceId();
    UD2x18 rewardFraction = _currentFractionalReward();

    _lastAuction = RngAuctionResult({
      recipient: _rewardRecipient,
      rewardFraction: rewardFraction,
      sequenceId: sequenceId,
      rng: rng,
      rngRequestId: rngRequestId
    });

    emit RngAuctionCompleted(
      _rewardRecipient,
      sequenceId,
      rng,
      rngRequestId,
      _auctionElapsedTimeSeconds,
      rewardFraction
    );
  }

Additionally, the relayer process can be to other chains using the 5164 message dispatcher. So a malicious actor can introduce an address(0) in the rewardRecipient parameter causing the rngComplete() revert in the specified chain.

    function relay(
        IRngAuctionRelayListener _remoteRngAuctionRelayListener,
        address rewardRecipient
    ) external returns (bytes32) {
        bytes memory listenerCalldata = encodeCalldata(rewardRecipient);
        bytes32 messageId = messageDispatcher.dispatchMessage(
            toChainId,
            address(account),
            RemoteOwnerCallEncoder.encodeCalldata(address(_remoteRngAuctionRelayListener), 0, listenerCalldata)
        );
        emit RelayedToDispatcher(rewardRecipient, messageId);
        return messageId;
    }

In the rngComplete() function, the reward process will revert the function when the reward recipient is zero. The transfer to address(0) reverts the transaction.

File: RngRelayAuction.sol
167:     for (uint8 i = 0; i < _rewards.length; i++) {
168:       uint104 _reward = uint104(_rewards[i]);
169:       if (_reward > 0) {
170:         prizePool.withdrawReserve(auctionResults[i].recipient, _reward);
171:         emit AuctionRewardDistributed(_sequenceId, auctionResults[i].recipient, i, _reward);
172:       }
173:     }

Tools used

Manual review

Recommended Mitigation Steps

Add an address zero validation for the rewardReicipient paramter in the startRngRequest() and the RngAuctionRelayerRemoteOwner.relay() functions. Additionally the remapTo() function must check the address zero in the _destination paramter.

Assessed type

Invalid Validation

1 - The emission rate in the LiquidationPair is not updated when the vault balance is updated

The LiquidationPair.swapExactAmountOut() helps to anyone to introduce prize tokens and get the underlying token out.

When the Auction is started, it calculates the emission rate using the current liquidatable balance. Once the auction period has started, the swapExactAmountOut() function will not update the emission rate because the _checkUpdateAuction() function only allow the data calculation once per period.

The problem is that the liquidatable balance can be changed while the auction is in execution causing the wrong data in emission rate and the wrong calculation in the swap purchase price.

The vault's liquidatable balance can be changed becuase anyone can deposit to the vault and affect the accrue balance.

The emission rate should be recalculated if the vault liquidatable balance is changed.

2 - There is not validation that the LiquidationPair period length and offest match with the prize pool draw length and offset.

The deployment documentation says: The pair's period length and offset will match the prize pool draw length and offset.. So in the deployment, there will be a validation for that. The problem is that after that anyone can create a LiquidationPair using the LiquidationPairFactory and those checks are not in the LiquidationPair contract. Those validations are crucial because the liquidation auctions should be syncronized with the prize pool.

Add a validation that the prize pool draw length and offsett must match with the LiquidationPair period length and offset. The validation should occur in the liquidation pair creation.