PoolTogether V5: Part Deux - bin2chen's results

Lines of code

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

Vulnerability details

Impact

rngComplete() Lack of access control , anyone can supply any random number

Proof of Concept

RngRelayAuction.rngComplete() Used to provide random numbers and distribute rewards In particular, providing random numbers is critical However, there is currently a lack of access control, so anyone can call it and specify random numbers at will

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

Tools Used

Recommended Mitigation Steps

Only rngAuctionRelayer can call it

      function rngComplete(
        uint256 _randomNumber,
        uint256 _rngCompletedAt,
        address _rewardRecipient,
        uint32 _sequenceId,
        AuctionResult calldata _rngAuctionResult
      ) external returns (bytes32) {

+    require(msg.sender == rngAuctionRelayer,"invald sender");  

Assessed type

Access Control

Lines of code

https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/LiquidationRouter.sol#L63

Vulnerability details

Impact

no deadline protection , users may suffer losses

Proof of Concept

LiquidationRouter.swapExactAmountOut() use for swap tokens. However, currently only _amountInMax slippage protection is provided, and not deadline protection like the common AMMS protection.

The role of the deadline protection is described below

AMMs provide their users with an option to limit the execution of their pending actions, such as swaps or adding and removing liquidity. The most common solution is to include a deadline timestamp as a parameter (for example see Uniswap V2 and Uniswap V3). If such an option is not present, users can unknowingly perform bad trades:

Alice wants to swap 100 tokens for 1 ETH and later sell the 1 ETH for 1000 DAI.

The transaction is submitted to the mempool, however, Alice chose a transaction fee that is too low for miners to be interested in including her transaction in a block. The transaction stays pending in the mempool for extended periods, which could be hours, days, weeks, or even longer.

When the average gas fee dropped far enough for Alice's transaction to become interesting again for miners to include it, her swap will be executed. In the meantime, the price of ETH could have drastically changed. She will still get 1 ETH but the DAI value of that output might be significantly lower.

She has unknowingly performed a bad trade due to the pending transaction she forgot about.

An even worse way this issue can be maliciously exploited is through MEV:

The swap transaction is still pending in the mempool. Average fees are still too high for miners to be interested in it.

The price of tokens has gone up significantly since the transaction was signed, meaning Alice would receive a lot more ETH when the swap is executed. But that also means that her maximum slippage value (sqrtPriceLimitX96 and minOut in terms of the Spell contracts) is outdated and would allow for significant slippage.

A MEV bot detects the pending transaction. Since the outdated maximum slippage value now allows for high slippage, the bot sandwiches Alice, resulting in significant profit for the bot and significant loss for Alice.

  function swapExactAmountOut(
    LiquidationPair _liquidationPair,
    address _receiver,
    uint256 _amountOut,
    uint256 _amountInMax
  ) external onlyTrustedLiquidationPair(_liquidationPair) returns (uint256) {
    IERC20(_liquidationPair.tokenIn()).safeTransferFrom(
      msg.sender,
      _liquidationPair.target(),
      _liquidationPair.computeExactAmountIn(_amountOut)
    );

    uint256 amountIn = _liquidationPair.swapExactAmountOut(_receiver, _amountOut, _amountInMax);

    emit SwappedExactAmountOut(_liquidationPair, _receiver, _amountOut, _amountInMax, amountIn);

    return amountIn;
  }

Tools Used

Recommended Mitigation Steps

Add deadline protection

      function swapExactAmountOut(
        LiquidationPair _liquidationPair,
        address _receiver,
        uint256 _amountOut,
        uint256 _amountInMax,
    +   uint256 deadline
      ) external onlyTrustedLiquidationPair(_liquidationPair) returns (uint256) {
+     require(block.timestamp<=deadline,"invalid timestamp");
...

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

If the recipient maliciously enters the blacklist of priceToken, it may cause rngComplete() to fail to execute successfully

Proof of Concept

The current implementation of RngRelayAuction.rngComplete() immediately transfers the prizeToken to the recipient

  function rngComplete(
    uint256 _randomNumber,
    uint256 _rngCompletedAt,
    address _rewardRecipient,
    uint32 _sequenceId,
    AuctionResult calldata _rngAuctionResult
  ) external returns (bytes32) {
...
    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);
      }
    }  
..

```solidity
contract PrizePool is TieredLiquidityDistributor {
  function withdrawReserve(address _to, uint104 _amount) external onlyDrawManager {
    if (_amount > _reserve) {
      revert InsufficientReserve(_amount, _reserve);
    }
    _reserve -= _amount;
@>  _transfer(_to, _amount);
    emit WithdrawReserve(_to, _amount);
  }

  function _transfer(address _to, uint256 _amount) internal {
    _totalWithdrawn += _amount;
@>  prizeToken.safeTransfer(_to, _amount);
  }

There is a risk that if prizeToken is a token with a blacklisting mechanism, such as :USDC. then recipient can block rngComplete() by maliciously entering the USDC blacklist. Since RngAuctionRelayer supports AddressRemapper, users can more simply specify blacklisted addresses via remapTo()

Tools Used

Recommended Mitigation Steps

Add claims mechanism, rngComplete() only record cliamable[token][user]+=rewards Users themselves go to claims

Assessed type

Context

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L277

Vulnerability details

Impact

_computeAvailable() incorrect calculations that result in a return value greater than the current balance, causing methods such as liquidate to fail

Proof of Concept

VaultBooster._computeAvailable() used to count the number of tokens currently available There are two conditions

1. accrue continuously according to time
2. the final cumulative value cannot be greater than the current contract balance

The code is as follows:

  function _computeAvailable(IERC20 _tokenOut) internal view returns (uint256) {
    Boost memory boost = _boosts[_tokenOut];
    uint256 deltaTime = block.timestamp - boost.lastAccruedAt;
    uint256 deltaAmount;
    if (deltaTime == 0) {
      return boost.available;
    }
    if (boost.tokensPerSecond > 0) {
      deltaAmount = boost.tokensPerSecond * deltaTime;
    }
    if (boost.multiplierOfTotalSupplyPerSecond.unwrap() > 0) {
      uint256 totalSupply = twabController.getTotalSupplyTwabBetween(address(vault), uint32(boost.lastAccruedAt), uint32(block.timestamp));
      deltaAmount += convert(boost.multiplierOfTotalSupplyPerSecond.intoUD60x18().mul(convert(deltaTime)).mul(convert(totalSupply)));
    }
@>  uint256 availableBalance = _tokenOut.balanceOf(address(this));
@>  deltaAmount = availableBalance > deltaAmount ? deltaAmount : availableBalance;
    return boost.available + deltaAmount;
  }

The current implementation code, limiting the maximum value of deltaAmount is wrong, using the minimum value compared to the current balance _tokenOut.balanceOf(address(this)). But the current balance includes the previously accumulated boost.available, so normally it should be compared to the difference between the current balance and boost.available.

so the value returned may be larger than the current balance, and LiquidationPair.sol performs source.liquidatableBalanceOf() and source.liquidate() with too large a number, resulting in a failed transfer.

Tools Used

Recommended Mitigation Steps

The maximum value returned should not exceed the current balance

  function _computeAvailable(IERC20 _tokenOut) internal view returns (uint256) {
    Boost memory boost = _boosts[_tokenOut];
    uint256 deltaTime = block.timestamp - boost.lastAccruedAt;
    uint256 deltaAmount;
    if (deltaTime == 0) {
      return boost.available;
    }
    if (boost.tokensPerSecond > 0) {
      deltaAmount = boost.tokensPerSecond * deltaTime;
    }
    if (boost.multiplierOfTotalSupplyPerSecond.unwrap() > 0) {
      uint256 totalSupply = twabController.getTotalSupplyTwabBetween(address(vault), uint32(boost.lastAccruedAt), uint32(block.timestamp));
      deltaAmount += convert(boost.multiplierOfTotalSupplyPerSecond.intoUD60x18().mul(convert(deltaTime)).mul(convert(totalSupply)));
    }
    uint256 availableBalance = _tokenOut.balanceOf(address(this));
-   deltaAmount = availableBalance > deltaAmount ? deltaAmount : availableBalance;
-   return boost.available + deltaAmount;
+   uint256 result = boost.available + deltaAmount;
+   if (result > availableBalance) result = availableBalance;
+   return result;

  }

Assessed type

Context

L-01: RngAuctionRelayerRemoteOwner.relay() Lack of mechanism to prevent duplicate execution

RngAuctionRelayerRemoteOwner.relay() is used for cross-chain execution of rngComplete() for cross-chain message sending by means of ERC-5164. The current implementation can be called successfully by anyone, and will only conflict and fail when the message reaches the corresponding chain (there will be a time lag for this) This way, if many people execute relay() at the same time, everyone can succeed, wasting a lot of GAS and cross-chain costs. Suggest to add that there can only be one short term for the same sequenceId, and it can only be executed again after a timeout. Or have the user decide whether to force it or not based on isForce, regardless of whether someone else has already done it or not.

    function relay(
        IRngAuctionRelayListener _remoteRngAuctionRelayListener,
        address rewardRecipient
+       bool isForce        
    ) external returns (bytes32) {
        bytes memory listenerCalldata = encodeCalldata(rewardRecipient);
+       require(!isForce && block.timestamp < lastReplyTime[rngAuction.openSequenceId()] + 1 days);
+       lastReplyTime[rngAuction.openSequenceId()] = block.timestamp;

        bytes32 messageId = messageDispatcher.dispatchMessage(
            toChainId,
            address(account),
            RemoteOwnerCallEncoder.encodeCalldata(address(_remoteRngAuctionRelayListener), 0, listenerCalldata)
        );
        emit RelayedToDispatcher(rewardRecipient, messageId);
        return messageId;
    }

L-02: startRngRequest() transfer more feeToken

in RngAuction.startRngRequest() , If the contract balance is less than the required _requestFee, the missing token needs to be transferred from the user.

The code is as follows.

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

The value transferred is the full _requestFee, it makes more sense to suggest transferring only the insufficient part _requestFee - IERC20(_feeToken).balanceOf(address(this)) .

Suggestion.

  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);
+       IERC20(_feeToken).transferFrom(msg.sender, address(this), _requestFee - IERC20(_feeToken).balanceOf(address(this));
      }
      // Increase allowance for the RNG service to take the request fee
      IERC20(_feeToken).safeIncreaseAllowance(address(rng), _requestFee);
    }