Dopex - Udsen's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L200-L203 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L347-L351 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L359-L361 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L191-L194

Vulnerability details

Impact

The RdpxV2Core.settle function is used to settle the options. This function calls the perpetualAtlanticVault.settle function where the actual option settling logic is executed. The perpetualAtlanticVault.settle function calculates the amount of collateral token to be transferred to the RdpxV2Core contract from the perpetualAtlanticVaultLP contract. Once the collateral token amount has been transferred to the RdpxV2Core contract the perpetualAtlanticVaultLP contract _totalCollateral state variable is updated by subtracting the transferred amount inside the perpetualAtlanticVaultLP.subtractLoss function. It is shown below:

require(
  collateral.balanceOf(address(this)) == _totalCollateral - loss,
  "Not enough collateral was sent out"
);
_totalCollateral -= loss;

As it is evident from the above code snippet there is a require statement with a conditional check to verify collateral.balanceOf(address(this)) == _totalCollateral - loss. The issue is if this condition fails then the entire RdpxV2Core.settle transaction will revert. Hence the options will not be able to be settled.

Since the above condition (collateral.balanceOf(address(this)) == _totalCollateral - loss) is checking exact equality it can be easily broken by front running the RdpxV2Core.settle transaction and depositing collateral token amount to the perpetualAtlanticVaultLP contract externally. Hence this will increase the collateral.balanceOf(address(this) amount more than the _totalCollateral amount. Hence the collateral.balanceOf(address(this)) == _totalCollateral - loss condition will be false. As a result the RdpxV2Core.settle transaction will revert. Hence an attacker can front run every RdpxV2Core.settle function by depositing small amount of collateral token to the perpetualAtlanticVaultLP contract and keep on reverting the RdpxV2Core.settle transaction thus permanently blocking the RdpxV2Core contract to settle the options.

Furthermore when the perpetualAtlanticVaultLP.addProceeds function was called to update the _totalCollateral variable for the deposited collateral tokens during option purchasing, it performed the following require check.

require(
  collateral.balanceOf(address(this)) >= _totalCollateral + proceeds,
  "Not enough collateral token was sent"
);
_totalCollateral += proceeds;    

It checks the >= condition rather than == condition. Hence it can be derived that the contract accepts collateral.balanceOf(address(this)) can be greater than the _totalCollateral amount. But in the perpetualAtlanticVaultLP.subtractLoss it checks == condition which is erroneous.

Proof of Concept

    require(
      collateral.balanceOf(address(this)) == _totalCollateral - loss,
      "Not enough collateral was sent out"
    );

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L200-L203

    collateralToken.safeTransferFrom(
      addresses.perpetualAtlanticVaultLP,
      addresses.rdpxV2Core,
      ethAmount
    );

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L347-L351

    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
      ethAmount
    );

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L359-L361

    require(
      collateral.balanceOf(address(this)) >= _totalCollateral + proceeds,
      "Not enough collateral token was sent"
    );

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L191-L194

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to update the conditional logic inside the require statement of the perpetualAtlanticVaultLP.subtractLoss to check for the >= condition rather than the == check. This will ensure an attacker can not break the RdpxV2Core.settle transaction by depositing a small collateral token amount to the perpetualAtlanticVaultLP contract externally.

The modified require statement is given below:

require(
  collateral.balanceOf(address(this)) >= _totalCollateral - loss,
  "Not enough collateral was sent out"
);

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1233-L1242 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L605 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L669 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L673-L674 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1160 https://github.com/dopex-io/rdpx-eth-oracle/blob/5762c2339b1c45b87ff4db172e43cef4a0ff603a/src/RdpxEthOracle.sol#L250

Vulnerability details

Impact

The RdpxV2Core.getRdpxPrice() function is used to return the price of rDPX against ETH. The value returned by the RdpxV2Core.getRdpxPrice() is of very high importance since it is used for calculations in critical functions such as RdpxV2Core._transfer(), RdpxV2Core.calculateBondCost(), and RdpxV2Core._calculateAmounts. And those functions are used in main protocol use case implemented in the bonding functions such as RdpxV2Core.bondWithDelegate() and RdpxV2Core.bond().

The issue here is that the RdpxV2Core contract functions treat the value returned from the getRdpxPrice function to be in 1e8 precision as mentioned in its Natspec comments. But the returned value is actually in 1e18 precision. The getRdpxPriceInEth function is implemented in the RdpxEthOracle contract and its function implementation upgrades the precision to 1e18 before value is returned as shown below:

    price = consult(token0, 1e18);

Hence all the critical functions which use the return value of getRdpxPrice use the wrong precision for their calculations thus breaking the accounting of the entire protocol. Even though the RdpxEthOracle.getRdpxPriceInEth is not in scope for this audit, I included this as a high severity finding here because the returned Rdpx price is very important for the internal calculations within the critical functions of the RdpxV2Core contract.

Proof of Concept

  /**
   * @notice Returns the price of rDPX against ETH
   * @dev    Price is in 1e8 Precision
   * @return rdpxPriceInEth rDPX price in ETH
   **/
  function getRdpxPrice() public view returns (uint256) {
    return
      IRdpxEthOracle(pricingOracleAddresses.rdpxPriceOracle)
        .getRdpxPriceInEth();
  }

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1233-L1242

    uint256 rdpxRequiredInWeth = (_rdpxRequired * getRdpxPrice()) / 1e8;

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L605

      uint256 rdpxAmountInWeth = (_rdpxAmount * getRdpxPrice()) / 1e8;

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L669

      uint256 extraRdpxToWithdraw = (discountReceivedInWeth * 1e8) /
        getRdpxPrice();

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L673-L674

    uint256 rdpxPrice = getRdpxPrice();

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1160

        price = consult(token0, 1e18);

https://github.com/dopex-io/rdpx-eth-oracle/blob/5762c2339b1c45b87ff4db172e43cef4a0ff603a/src/RdpxEthOracle.sol#L250

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to either change the RdpxEthOracle.getRdpxPriceInEth function to return the value in 1e8 precision by modifying the relevant line of code as shown below:

    price = consult(token0, 1e8);

Else it is recommended to change the RdpxV2Core functions which uses the getRdpxPrice() returned value, to work with 1e18 precision which is the correct current precision of the returned value.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L372-L374 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L381-L383 https://github.com/dopex-io/rdpx-eth-oracle/blob/5762c2339b1c45b87ff4db172e43cef4a0ff603a/src/RdpxEthOracle.sol#L217

Vulnerability details

Impact

The UniV2LiquidityAmo.getLpTokenBalanceInWeth function is used to return the LP token balance of the contract in weth. It calls the getLpPrice() function which is expected to return the LP price in 1e8 precision.

getLpPrice() function calls the RdpxEthOracle.getLpPriceInEth function to get the LP price value. The issue here is that the RdpxEthOracle.getLpPriceInEth returned value is in 1e18 precision as shown below (code snippet taken from the RdpxEthOracle.getLpPriceInEth function).

    return (lpPriceIn112x112 * 1e18) >> 112;

But the UniV2LiquidityAmo.getLpTokenBalanceInWeth calculates the LP token balance considering the getLpPrice() returned value is in 1e8 precision as shown below:

return (lpTokenBalance * getLpPrice()) / 1e8;

Hence the returned value of UniV2LiquidityAmo.getLpTokenBalanceInWeth is erroneous and would provide the wrong value for anyone using the UniV2LiquidityAmo.getLpTokenBalanceInWeth function to read the LP token balance of the UniV2LiquidityAmo contract in weth.

Proof of Concept

  function getLpTokenBalanceInWeth() external view returns (uint256) {
    return (lpTokenBalance * getLpPrice()) / 1e8;
  }

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L372-L374

  function getLpPrice() public view returns (uint256) {
    return IRdpxEthOracle(addresses.rdpxOracle).getLpPriceInEth();
  }

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L381-L383

        return (lpPriceIn112x112 * 1e18) >> 112;

https://github.com/dopex-io/rdpx-eth-oracle/blob/5762c2339b1c45b87ff4db172e43cef4a0ff603a/src/RdpxEthOracle.sol#L217

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to modify the UniV2LiquidityAmo.getLpTokenBalanceInWeth function as shown below to account for 1e18 precision.

return (lpTokenBalance * getLpPrice()) / 1e18;

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L545-L549

Vulnerability details

Impact

The RdpxV2Core._curveSwap function is used to swap token A to token B based on the value of the _ethToDpxEth Boolean variable. _ethToDpxEth value denotes whether swap is from ETH to dpxETH or dpxETH to ETH.

Before the actual dpxEthCurvePool.exchange happens the minOut value is calculated as slippage protection as shown below:

uint256 minOut = _ethToDpxEth
  ? (((_amount * getDpxEthPrice()) / 1e8) - 
    (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16)) 
  : (((_amount * getEthPrice()) / 1e8) -
    (((_amount * getEthPrice()) * slippageTolerance) / 1e16));

If the _ethToDpxEth is true which means the swap is from Eth to dpxEth, the _amount is multiplied by value returned from getDpxEthPrice(). Here the _amount is in Eth and getDpxEthPrice() value is in Eth/DpxEth unit with 1e8 precision. Hence it is clear this calculation is wrong. Because we want to multiply the _amount (Eth value) by the getEthPrice() value and not by the getDpxEthPrice() value. Because getEthPrice() returns the dpxEth/Eth value in 1e8 precision which is the correct value to use to multiply the Eth _amount to obtain the minOut value in dpxEth units.

Similarly if the _ethToDpxEth is false which means the swap is from dpxEth to Eth, the _amount is multiplied by value returned from getEthPrice(). Here the _amount is in dpxEth and getEthPrice() value is in dpxEth/Eth unit with 1e8 precision. Hence it is clear this calculation is wrong. Because we want to multiply the _amount (dpxEth value) by the getDpxEthPrice() value and not by the getEthPrice() value. Because getDpxEthPrice() returns the Eth/dpxEth value in 1e8 precision which is the correct value to use to multiply the dpxEth _amount to obtain the minOut value in Eth units.

Above error in calculating the value of minOut could prompt the dpxEthCurvePool.exchange transaction to revert since we are providing the wrong slippage protection value to the actual swap happening in the exchange transaction.

This will prompt the _curveSwap transaction to behave in unexpected manner thus breaking the protocol unexpectedly.

Proof of Concept

    uint256 minOut = _ethToDpxEth
      ? (((_amount * getDpxEthPrice()) / 1e8) -
        (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))
      : (((_amount * getEthPrice()) / 1e8) -
        (((_amount * getEthPrice()) * slippageTolerance) / 1e16));

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L545-L549

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to correct the minOut value in the RdpxV2Core._curveSwap function as shown below:

uint256 minOut = _ethToDpxEth
  ? (((_amount * getEthPrice()) / 1e8) - 
    (((_amount * getEthPrice()) * slippageTolerance) / 1e16)) 
  : (((_amount * getDpxEthPrice()) / 1e8) -
    (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16));

The above change will ensure the correct minOut value is calculated based on the Boolean value of _ethToDpxEth thus ensuring successful execution of the RdpxV2Core._curveSwap transaction.

Assessed type

Other