PoolTogether V5: Part Deux - MohammedRizwan's results

Lines of code

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

Vulnerability details

Impact

In LiquidationRouter.sol, swapExactAmountOut() function has no deadline for the transaction when swapping.

File: src/LiquidationRouter.sol

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

As seen above, there is no deadline checks while swapping. This can be further checked in LiquidationRouter.sol where deadline parameter is also missing. 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:

1. Alice wants to swap 100 tokens for 1 ETH and later sell the 1 ETH for 1000 DAI.
2. 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.
3. 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.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Introduce a deadline parameter to all functions which potentially perform a swap.

Assessed type

Other

Lines of code

https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/LiquidationPairFactory.sol#L65-L108

Vulnerability details

Impact

Malicious _liquidationPair owner can deploy _liquidationPair using malicious _source(liquidation source that the pair will use) and other insufficient input validations that can put users' funds at risk.

File: src/LiquidationPairFactory.sol

  function createPair(
    ILiquidationSource _source,
    address _tokenIn,
    address _tokenOut,
    uint32 _periodLength,
    uint32 _periodOffset,
    uint32 _targetFirstSaleTime,
    SD59x18 _decayConstant,
    uint112 _initialAmountIn,
    uint112 _initialAmountOut,
    uint256 _minimumAuctionAmount
  ) external returns (LiquidationPair) {
    LiquidationPair _liquidationPair = new LiquidationPair(
      _source,
      _tokenIn,
      _tokenOut,
      _periodLength,
      _periodOffset,
      _targetFirstSaleTime,
      _decayConstant,
      _initialAmountIn,
      _initialAmountOut,
      _minimumAuctionAmount
    );

    allPairs.push(_liquidationPair);
    deployedPairs[_liquidationPair] = true;

   // Some code

  }

In LiquidationPairFactory.sol, createPair() function is used to deploy a new LiquidationPair and then push it to allPairs array.

  LiquidationPair[] public allPairs;

It also have a mapping to verify that a _liquidationPair has been deployed via the LiquidationPairFactory.sol

  mapping(LiquidationPair => bool) public deployedPairs;

This allows users to check the authenticity of a _liquidationPair to ensure the that implementation of a _liquidationPair is authentic but the issue arises when the addresses of the _source and other parameters are passed as input in the function but they are not validated before the _liquidationPair being deployed. In current implementation, users can see the authenticity of _liquidationPair but they can not authenticate the _source(liquidation source).

LiquidationPair.sol constructor only checks that uint related parameters. It does not check or validate the _source, _tokenIn, _tokenOut addresses in constructor. It is recommended to validate before the deployment.

Proof of Concept

https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/LiquidationPairFactory.sol#L65-L108

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure proper input validations since these are passed manually.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The createVaultBooster() function deploys a new VaultBooster contract using the create, where the address derivation depends only on the VaultBoosterFactory nonce.

Re-orgs can happen in all EVM chains and as confirmed the contracts will be deployed on most EVM compatible L2s including Arbitrum, etc. It is also planned to be deployed on ZKSync in future. In ethereum, where this is deployed, Re-orgs has already been happened. For more info, check here

This issue will increase as some of the chains like Arbitrum and Polygon are suspicious of the reorg attacks.

Polygon re-org reference: click here This one happened this year in February, 2023.

Polygon blocks forked: check here

The issue would happen when users rely on the address derivation in advance or try to deploy the position clone with the same address on different EVM chains, any funds sent to the new contract could potentially be withdrawn by anyone else. All in all, it could lead to the theft of user funds.

File: src/VaultBoosterFactory.sol

    function createVaultBooster(PrizePool _prizePool, address _vault, address _owner) external returns (VaultBooster) {
>>        VaultBooster booster = new VaultBooster(_prizePool, _vault, _owner);

        emit CreatedVaultBooster(booster, _prizePool, _vault, _owner);

        return booster;
    }

Optimistic rollups (Optimism/Arbitrum) are also suspect to reorgs since if someone finds a fraud the blocks will be reverted, even though the user receives a confirmation.

Attack Scenario Imagine that Alice deploys a new VaultBooster, and then sends funds to it. Bob sees that the network block reorg happens and calls createVaultBooster. Thus, it creates VaultBooster with an address to which Alice sends funds. Then Alices’ transactions are executed and Alice transfers funds to Bob’s controlled VaultBooster.

This is a Medium severity issue that has been referenced from below Code4rena reports, https://code4rena.com/reports/2023-01-rabbithole/#m-01-questfactory-is-suspicious-of-the-reorg-attack https://code4rena.com/reports/2023-04-frankencoin#m-14-re-org-attack-in-factory

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Deploy such contracts via create2 with salt that includes msg.sender

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

_liquidationPair are created from the factory via CREATE1, an attacker can frontrun createPair() to deploy at the same address but with different config. If the deployed chain reorg, a different vault might also be deployed at the same address.

File: src/LiquidationPairFactory.sol

  function createPair(
    ILiquidationSource _source,
    address _tokenIn,
    address _tokenOut,
    uint32 _periodLength,
    uint32 _periodOffset,
    uint32 _targetFirstSaleTime,
    SD59x18 _decayConstant,
    uint112 _initialAmountIn,
    uint112 _initialAmountOut,
    uint256 _minimumAuctionAmount
  ) external returns (LiquidationPair) {
>>    LiquidationPair _liquidationPair = new LiquidationPair(
      _source,
      _tokenIn,
      _tokenOut,
      _periodLength,
      _periodOffset,
      _targetFirstSaleTime,
      _decayConstant,
      _initialAmountIn,
      _initialAmountOut,
      _minimumAuctionAmount
    );


   // Some code

  }

Proof of Concept

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

1. Charlie setup a bot to monitor the mempool when Bob deploy a new _liquidationPair
2. Charlie's bot saw a deployment by Bob at 0x1234, fire a tx to deposit immediately
3. Alice frontrun Bob's deployment by deploying a malicious _liquidationPair at 0x1234
4. Charlie's transaction ended up deposited into Alice's malicious vault

Tools Used

Manual Review

Recommended Mitigation Steps

Use CREATE2 for such contracts with msg.sender containing salt.

Assessed type

Other

Summary

Low Risk Issues

[L‑01] Prevent setOriginChainOwner() from calling more than once

In RemoteOwner.sol, Per the Natspec of setOriginChainOwner() it can be called only once,

   * @dev Can only be called once.

However, with current impelementation it can be called more than once. To prevent this happening, a validation checks must be added to setOriginChainOwner() function.

There is 1 instance of this issue:

File: src/RemoteOwner.sol

  function setOriginChainOwner(address _newOriginChainOwner) external {
    _checkSender();
    _setOriginChainOwner(_newOriginChainOwner);
  }

Recommended Mitigation Steps

File: src/RemoteOwner.sol

+  bool initialize;

  function setOriginChainOwner(address _newOriginChainOwner) external {
+    require(!initialize, "can be called only once");
    _checkSender();
    _setOriginChainOwner(_newOriginChainOwner);
+    initialize = true;
  }

[L‑02] Prevent createVaultBooster() from setting incorrect inputs

In VaultBoosterFactory.sol, createVaultBooster() is used to create a new vault booster contract. As confirmed by sponsor, this can be called by anyone. However it has some missing sanity validations which should be added.

There is 1 instance of this issue:

Recommended Mitigation Steps

    function createVaultBooster(PrizePool _prizePool, address _vault, address _owner) external returns (VaultBooster) {
+       require(address(_prizePool) != address(0), "invalid address");
+       require(_vault != address(0), "invalid address");
+       require(_owner != address(0), "invalid address");

        VaultBooster booster = new VaultBooster(_prizePool, _vault, _owner);

        emit CreatedVaultBooster(booster, _prizePool, _vault, _owner);

        return booster;
    }

[L‑03] update openzeppelin version to 4.9.3

The contracts has used openzeppelin version 4.9.2 which is checked from package.json. External libraries which are being used in contracts must be upto date and should not have any known vulnerabilities. Lates OZ features can be checked here

Recommended Mitigation Steps

Update openzeppelin version to 4.9.3

[L‑04] unsafe downcast may lead to accounting errors

In contracts, while down casting the data type of data value, It is directly processed without any safety. As seen in contract, such unsafe downcasting can lead to accounting errors which can seriously hamper the overall system.

There is 3 instances of this issue:

Recommended Mitigation Steps

Use openzeppelin safeCast.sol for safe casting.