PoolTogether V5: Part Deux - dirk_y's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngRelayAuction.sol#L178-L184 https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngRelayAuction.sol#L154-L157 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/26557afa439934afc080eca6165fe3ce5d4b63cd/src/PrizePool.sol#L366 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/26557afa439934afc080eca6165fe3ce5d4b63cd/src/abstract/TieredLiquidityDistributor.sol#L374

Vulnerability details

Impact

A relayer completes a prize pool draw by calling rngComplete in RngRelayAuction.sol. This method closes the prize pool draw with the relayed random number and distributes the rewards to the RNG auction recipient and the RNG relay auction recipient. These rewards are calculated based on a fraction of the prize pool reserve rather than an actual value.

However, the current reward calculation mistakenly includes an extra reserveForOpenDraw amount just after the draw has been closed. Therefore the fraction over which the rewards are being calculated includes tokens that have not been added to the reserve and will actually only be added to the reserve when the next draw is finalised. As a result, the reward recipients are rewarded too many tokens.

Proof of Concept

Before deciding whether or not to relay an auction result, a bot can call computeRewards to calculate how many rewards they'll be getting based on the size of the reserve, the state of the auction and the reward fraction of the RNG auction recipient:

  function computeRewards(AuctionResult[] calldata __auctionResults) external returns (uint256[] memory) {
    uint256 totalReserve = prizePool.reserve() + prizePool.reserveForOpenDraw();
    return _computeRewards(__auctionResults, totalReserve);
  }

Here, the total reserve is calculated as the sum of the current reserve and and amount of new tokens that will be added to the reserve once the currently open draw is closed. This method is correct and correctly calculates how many rewards should be distributed when a draw is closed.

A bot can choose to close the draw by calling rngComplete (via a relayer), at which point the rewards are calculated and distributed. Below is the interesting part of this method:

    uint32 drawId = prizePool.closeDraw(_randomNumber);

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

As you can see, the draw is first closed and then the future reserve is used to calculate the rewards that should be distributed. However, when closeDraw is called on the pool, the reserveForOpenDraw for the previously open draw is added to the existing reserves. So reserve() is now equal to the totalReserve value in the earlier call to computeRewards. By including reserveForOpenDraw() when computing the actual reward to be distributed we've accidentally counted the tokens that are only going to be added in when the next draw is closed. So now the rewards distribution calculation includes the pending reserves for 2 draws rather than 1.

Tools Used

Manual review

Recommended Mitigation Steps

When distributing rewards in the call to rngComplete, the rewards should not be calculated with the new value of reserveForOpenDraw because the previous reserveForOpenDraw value has already been added to the reserves when closeDraw is called on the prize pool. Below is a suggested diff:

diff --git a/src/RngRelayAuction.sol b/src/RngRelayAuction.sol
index 8085169..cf3c210 100644
--- a/src/RngRelayAuction.sol
+++ b/src/RngRelayAuction.sol
@@ -153,8 +153,8 @@ contract RngRelayAuction is IRngAuctionRelayListener, IAuction {
 
     uint32 drawId = prizePool.closeDraw(_randomNumber);
 
-    uint256 futureReserve = prizePool.reserve() + prizePool.reserveForOpenDraw();
-    uint256[] memory _rewards = RewardLib.rewards(auctionResults, futureReserve);
+    uint256 reserve = prizePool.reserve();
+    uint256[] memory _rewards = RewardLib.rewards(auctionResults, reserve);
 
     emit RngSequenceCompleted(
       _sequenceId,

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The RngRelayAuction contract deployed on each chain has a rngComplete method that is supposed to be called by the relayer in order to close/complete a prize draw.

However this method doesn't have any access control and can therefore be called by anyone with arbitrary random numbers or sequence ids. And arbitrary random number can be supplied to ensure that the caller always wins a prize. And the maximum _sequenceId value can be supplied to perpetually brick the relay auction contract.

Proof of Concept

This vulnerability exists due to lack of access control:

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

Supplying an arbitrary _randomNumber value is clearly an issue since the number is now not random and doesn't originate from an on-chain random number generator. As stated above, a malicious user can ensure they win every round by supplying a random number that always places them in the winning zone.

In terms of bricking the contract, this can be achieved due to the call to _sequenceHasCompleted on the first line of the above method:

  function _sequenceHasCompleted(uint32 _sequenceId) internal view returns (bool) {
    return _lastSequenceId >= _sequenceId;
  }

This method checks that the supplied _sequenceId is greater than the last sequence id. Due to the lack of access control a malicious user can call rngComplete with _sequenceId = type(uint32).max to permanently brick the contract going forwards.

Tools Used

Manual review

Recommended Mitigation Steps

The rngComplete method should only be able to be called by the auction relayer on the respective chain. Below is a suggested diff:

diff --git a/src/RngRelayAuction.sol b/src/RngRelayAuction.sol
index 8085169..56d794a 100644
--- a/src/RngRelayAuction.sol
+++ b/src/RngRelayAuction.sol
@@ -135,6 +135,7 @@ contract RngRelayAuction is IRngAuctionRelayListener, IAuction {
     uint32 _sequenceId,
     AuctionResult calldata _rngAuctionResult
   ) external returns (bytes32) {
+    require(msg.sender == rngAuctionRelayer, "Not authorized");
     if (_sequenceHasCompleted(_sequenceId)) revert SequenceAlreadyCompleted();
     uint64 _auctionElapsedSeconds = uint64(block.timestamp < _rngCompletedAt ? 0 : block.timestamp - _rngCompletedAt);
     if (_auctionElapsedSeconds > (_auctionDurationSeconds-1)) revert AuctionExpired();

Assessed type

Access Control

Lines of code

https://github.com/GenerationSoftware/remote-owner/blob/9c093dbd36c1f18ab7083549d10ac601d91630df/src/RemoteOwner.sol#L58 https://github.com/GenerationSoftware/remote-owner/blob/9c093dbd36c1f18ab7083549d10ac601d91630df/src/RemoteOwner.sol#L120 https://github.com/GenerationSoftware/remote-owner/blob/9c093dbd36c1f18ab7083549d10ac601d91630df/src/RemoteOwner.sol#L96-L99 https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngAuctionRelayerRemoteOwner.sol#L47 https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngAuctionRelayerRemoteOwner.sol#L64

Vulnerability details

Impact

The RemoteOwner.sol contract has a security measure that ensures the sender from the remote/origin chain was the origin chain owner (i.e. a RngAuctionRelayerRemoteOwner.sol deployment), and this address is set at deployment time in the constructor. The RngAuctionRelayerRemoteOwner contract also has a security measure to ensure that messages are only dispatched across chain to the RemoteOwner contract deployed in the destination chain, and this address is set at deployment time in the constructor.

Clearly there is a circular dependency here that means the deployment phase will fail. There is a setOriginChainOwner method on the RemoteOwner contract, however this can only be called by the address on the origin chain specified in the constructor. This method is never called from the origin chain either. In summary, the circular dependency prevents the contracts from being deployed and ever initialised properly.

It is possible that there is an intermediary __originChainOwner used in the constructor when deploying RemoteOwner, but since I couldn't find any deployment scripts to verify this I have assumed that this is an unintended bug. The severity of this report depends on whether or not this was intended.

Proof of Concept

In the RemoteOwner.sol contract, the origin chain owner is set in the constructor:

  constructor(
    uint256 originChainId_,
    address executor_,
    address __originChainOwner
  ) ExecutorAware(executor_) {
    if (originChainId_ == 0) revert OriginChainIdZero();
    _originChainId = originChainId_;
    _setOriginChainOwner(__originChainOwner);
  }

Any calls to the RemoteOwner contract are protected by the _checkSender view:

  function _checkSender() internal view {
    if (!isTrustedExecutor(msg.sender)) revert LocalSenderNotExecutor(msg.sender);
    if (_fromChainId() != _originChainId) revert OriginChainIdUnsupported(_fromChainId());
    if (_msgSender() != address(_originChainOwner)) revert OriginSenderNotOwner(_msgSender());
  }

Now, if we have a look at the RngAuctionRelayerRemoteOwner.sol contract, we can see that the remote owner address is also specified in the constructor:

    constructor(
        RngAuction _rngAuction,
        ISingleMessageDispatcher _messageDispatcher,
        RemoteOwner _remoteOwner,
        uint256 _toChainId
    ) RngAuctionRelayer(_rngAuction) {
        messageDispatcher = _messageDispatcher;
        account = _remoteOwner;
        toChainId = _toChainId;
    }

This account address is now hard-coded and used with any calls to relay:

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

There is a circular dependency here due to the reliance on specifying the relevant addresses in the constructor.

Tools Used

Manual review

Recommended Mitigation Steps

To remove the circular dependency and reliance on a very specific deployment pipeline that requires a specific call from a remote chain address, I would make the following change to the RemoteOwner contract:

diff --git a/src/RemoteOwner.sol b/src/RemoteOwner.sol
index 7c1de6d..a6cb8f1 100644
--- a/src/RemoteOwner.sol
+++ b/src/RemoteOwner.sol
@@ -55,7 +55,6 @@ contract RemoteOwner is ExecutorAware {
   ) ExecutorAware(executor_) {
     if (originChainId_ == 0) revert OriginChainIdZero();
     _originChainId = originChainId_;
-    _setOriginChainOwner(__originChainOwner);
   }
 
   /* ============ External Functions ============ */
@@ -94,7 +93,7 @@ contract RemoteOwner is ExecutorAware {
    *      If the transaction get front-run at deployment, we can always re-deploy the contract.
    */
   function setOriginChainOwner(address _newOriginChainOwner) external {
-    _checkSender();
+    require(_originChainOwner == address(0), "Already initialized");
     _setOriginChainOwner(_newOriginChainOwner);
   }
 

However I can understand how the current deployment pipeline functionality would make it harder to frontrun setOriginChainOwner if this was done deliberately, so alternatively you could keep the functionality the same but just provide better comments.

Assessed type

Other

Lines of code

https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/LiquidationPair.sol#L266-L270 https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L201-L203 https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L249-L257 https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L276-L279

Vulnerability details

Impact

When a user/bot wants to perform a liquidation for a liquidation pair, they can call view methods like maxAmountOut to see what balance of the output token they are able to liquidate at the current time. This makes an underlying call to liquidatableBalanceOf that reports the maximum balance of output token that is held by the Vault or VaultBooster.

However the VaultBooster contract has an issue where the return value of liquidatableBalanceOf can actually be higher than the balance of output token held by the contract. As a result, a user/bot might try to perform a liquidation for the liquidatableBalanceOf of the VaultBooster, but this call will revert because the resulting token transfer will revert due to insufficient balance.

Proof of Concept

A liquidator is able to compute the profitability of a liquidation before performing the action by calling view methods like maxAmountOut, maxAmountIn, computeExactAmountIn and estimateAmountOut. All of these view methods make an underlying call to _maxAmountOut:

  /// @notice Computes the maximum amount of output tokens that can be purchased
  /// @return Maximum amount of output tokens
  function _maxAmountOut() internal returns (uint256) {
    uint emissions = uint(convert(_emissionRate.mul(_getElapsedTime())));
    uint liquidatable = source.liquidatableBalanceOf(tokenOut);
    return emissions > liquidatable ? liquidatable : emissions;
  }

This method returns the maximum amount of output tokens that can be purchased based on the emission rate and the liquidatable balance of the source (i.e. the Vault or VaultBooster). In the case where the emissions have exceeded the liquidatable balance of the source, the max amount that can be purchased is equal to the liquidatable balance of the source.

For the VaultBooster contract, liquidatableBalanceOf looks like:

  function liquidatableBalanceOf(address _tokenOut) external override returns (uint256) {
    return _accrue(IERC20(_tokenOut));
  }

where the underlying call to _accrue looks like:

  function _accrue(IERC20 _tokenOut) internal returns (uint256) {
    uint256 available = _computeAvailable(_tokenOut);
    _boosts[_tokenOut].available = available.toUint144();
    _boosts[_tokenOut].lastAccruedAt = uint48(block.timestamp);

    emit BoostAccrued(_tokenOut, available);

    return available;
  }

As you can see, the liquidatableBalanceOf of the VaultBooster contract is equal to the return value of _computeAvailable. The last 3 lines of this method have an issue:

    uint256 availableBalance = _tokenOut.balanceOf(address(this));
    deltaAmount = availableBalance > deltaAmount ? deltaAmount : availableBalance;
    return boost.available + deltaAmount;

Here the deltaAmount is capped by the balance of the token, however boost.available can also be non-zero (and often will be). As a result, the return value can actually be greater than the token balance of the contract.

Now, when anyone calls liquidatableBalanceOf they will see a larger value than is actually held by the VaultBooster contract and therefore any liquidations that are triggered based on this return value will revert. Since most callers will be sophisticated bots, this is quite a critical issue to fix to ensure the bots are performing as expected.

Tools Used

Manual review

Recommended Mitigation Steps

The return value of _computeAvailable should be updated to cap the whole boost balance by the token balance of the contract, rather than just the delta. Below is a suggested diff:

diff --git a/src/VaultBooster.sol b/src/VaultBooster.sol
index 951c58a..8cc9cdd 100644
--- a/src/VaultBooster.sol
+++ b/src/VaultBooster.sol
@@ -263,6 +263,7 @@ contract VaultBooster is Ownable, ILiquidationSource {
     Boost memory boost = _boosts[_tokenOut];
     uint256 deltaTime = block.timestamp - boost.lastAccruedAt;
     uint256 deltaAmount;
+    uint256 available;
     if (deltaTime == 0) {
       return boost.available;
     }
@@ -274,8 +275,8 @@ contract VaultBooster is Ownable, ILiquidationSource {
       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;
+    available = availableBalance > boost.available + deltaAmount ? boost.available + deltaAmount : availableBalance;
+    return available;
   }
 
   /// @notice Requires the given token to be the prize token

Assessed type

Invalid Validation

Lines of code

https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/libraries/ContinuousGDA.sol#L39-L41 https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/libraries/ContinuousGDA.sol#L65 https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/libraries/ContinuousGDA.sol#L86

Vulnerability details

Impact

The LiquidationPair contract facilitates Periodic Continuous Gradual Dutch Auctions for yield. This uses the underlying ContinuousGDA.sol library in order to correctly price the auctions.

However this library incorrectly implements the formula, using the emission rate in a few places where it should use the decay constant. Since the decay constant is usually less than the emission rate (as can also be seen from the test suite), this means that the purchasePrice calculation is lower than it should be, meaning that liquidations are over-incentivised.

Proof of Concept

This is difficult to demonstrate given the issue is basically just that the formula in https://www.paradigm.xyz/2022/04/gda has been wrongly implemented. However I'll point out a few issues in the code:

  function purchasePrice(
    SD59x18 _amount,
    SD59x18 _emissionRate,
    SD59x18 _k,
    SD59x18 _decayConstant,
    SD59x18 _timeSinceLastAuctionStart
  ) internal pure returns (SD59x18) {
    if (_amount.unwrap() == 0) {
      return SD59x18.wrap(0);
    }
    SD59x18 topE = _decayConstant.mul(_amount).div(_emissionRate);
    topE = topE.exp().sub(ONE);
    SD59x18 bottomE = _decayConstant.mul(_timeSinceLastAuctionStart);
    bottomE = bottomE.exp();
    SD59x18 result;
    if (_emissionRate.unwrap() > 1e18) {
      result = _k.div(_emissionRate).mul(topE).div(bottomE);
    } else {
      result = _k.mul(topE.div(_emissionRate.mul(bottomE)));
    }
    return result;
  }

In the result calculation you can see that _k is divided by _emissionRate. However, according to the proper formula, _k should be divided by _decayConstant.

Another issue occurs in purchaseAmount where _k is added to lnParam instead of ONE and price is multiplied by _emissionRate instead of _decayConstant:

  function purchaseAmount(
    SD59x18 _price,
    SD59x18 _emissionRate,
    SD59x18 _k,
    SD59x18 _decayConstant,
    SD59x18 _timeSinceLastAuctionStart
  ) internal pure returns (SD59x18) {
    if (_price.unwrap() == 0) {
      return SD59x18.wrap(0);
    }
    SD59x18 exp = _decayConstant.mul(_timeSinceLastAuctionStart).exp();
    SD59x18 lnParam = _k.add(_price.mul(_emissionRate).mul(exp)).div(_k);
    SD59x18 numerator = _emissionRate.mul(lnParam.ln());
    SD59x18 amount = numerator.div(_decayConstant);
    return amount;
  }

I would suggest double checking the formula and the derivation of the complementary formulas to calculate amount/k. The correct implementation is show in the diff below.

Tools Used

Manual review

Recommended Mitigation Steps

The implementation should be updated to correctly calculate the price for a continuous GDA. I have made the required fixes in the diff below:

diff --git a/src/libraries/ContinuousGDA.sol b/src/libraries/ContinuousGDA.sol
index 721d626..7e2bb61 100644
--- a/src/libraries/ContinuousGDA.sol
+++ b/src/libraries/ContinuousGDA.sol
@@ -36,9 +36,9 @@ library ContinuousGDA {
     bottomE = bottomE.exp();
     SD59x18 result;
     if (_emissionRate.unwrap() > 1e18) {
-      result = _k.div(_emissionRate).mul(topE).div(bottomE);
+      result = _k.div(_decayConstant).mul(topE).div(bottomE);
     } else {
-      result = _k.mul(topE.div(_emissionRate.mul(bottomE)));
+      result = _k.mul(topE.div(_decayConstant.mul(bottomE)));
     }
     return result;
   }
@@ -62,7 +62,7 @@ library ContinuousGDA {
       return SD59x18.wrap(0);
     }
     SD59x18 exp = _decayConstant.mul(_timeSinceLastAuctionStart).exp();
-    SD59x18 lnParam = _k.add(_price.mul(_emissionRate).mul(exp)).div(_k);
+    SD59x18 lnParam = ONE.add(_price.mul(_decayConstant).mul(exp)).div(_k);
     SD59x18 numerator = _emissionRate.mul(lnParam.ln());
     SD59x18 amount = numerator.div(_decayConstant);
     return amount;
@@ -83,7 +83,7 @@ library ContinuousGDA {
   ) internal pure returns (SD59x18) {
     SD59x18 exponent = _decayConstant.mul(_targetFirstSaleTime);
     SD59x18 eValue = exponent.exp();
-    SD59x18 multiplier = _emissionRate.mul(_price);
+    SD59x18 multiplier = _decayConstant.mul(_price);
     SD59x18 denominator = (_decayConstant.mul(_purchaseAmount).div(_emissionRate)).exp().sub(ONE);
     SD59x18 result = eValue.div(denominator);
     return result.mul(multiplier);

Assessed type

Math