Dopex - flacko's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L221-L222 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L232-L235 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L250-L255

Vulnerability details

Impact

The ReLPContract#reLP() function is bound to always revert as the equation used to calculate the minimum amount of WETH to be received when removing liquidity from the LP pool on Uniswap:

    uint256 mintokenBAmount = ((tokenAToRemove * tokenAInfo.tokenAPrice) / 1e8) -
      ((tokenAToRemove * tokenAInfo.tokenAPrice) * liquiditySlippageTolerance) / 1e16;

assumes tokenAInfo.tokenAPrice to have a precision of 8 decimals but, as confirmed with the sponsor, it actually has a precision of 18 decimals.

Proof of Concept

Starting from the calculation of tokenAToRemove:

    uint256 tokenAToRemove = ((((_amount * 4) * 1e18) /
      tokenAInfo.tokenAReserve) *
      tokenAInfo.tokenALpReserve *
      baseReLpRatio) / (1e18 * DEFAULT_PRECISION * 1e2);

We have _amount that's to be in 18 decimals (as this is the amount of LP tokens to be removed from the Uniswap LP pool), tokenAReserve and tokenALpReserve in 18 decimals, and baseReLpRatio in 6 decimals. After evaluating the equation, we end up with a value of 14 decimals precision.

Continuing forward, we calculate lpToRemove:

    uint256 lpToRemove = (tokenAToRemove * totalLpSupply) /
      tokenAInfo.tokenALpReserve;

Where tokenAToRemove is in 14 decimals as we already found out, totalLpSupply and tokenALpReserve in 18 decimals. So after evaluating this equation, we have lpToRemove with 14 decimals precision.

Next mintokenBAmount is calculated:

    uint256 mintokenBAmount = ((tokenAToRemove * tokenAInfo.tokenAPrice) /
      1e8) -
      ((tokenAToRemove * tokenAInfo.tokenAPrice) * liquiditySlippageTolerance) /
      1e16;

Looking at this equation, we have tokenAToRemove with 16 decimals and tokenInfo.tokenAPrice in 18 decimals (when it's expected to be with 8 decimals). After running the numbers, we end up with mintokenBAmount having a value with a precision of 24 decimals.

Since tokenB is WETH which has a precision of 18 decimals, the reLP function will end up trying to remove liquidity from the Uniswap pool expecting 10**4 times more WETH in return than reasonable, which will cause the call to revert because of the inconsistency.

Tools Used

Manual review

Recommended Mitigation Steps

As discussed with the sponsor, there are 2 approaches that can be taken here:

1. Refactor the calculations in the reLP function to account for an RDPX price in 18 decimals precision, instead of 8
2. Introduce an intermediate contract that'd be used for conversions.

Assessed type

Decimal

Cache the value of nextFundingPaymentTimestamp() in updateFundingPaymentPointer()

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

In updateFundingPaymentPointer() (PerpetualAtlanticVault.sol), the nextFundingPaymentTimestamp() method is called 7 times in total every time the loop iterates (and the if statement in it evaluates to true).

Instead, the calls can be reduced to 2 by declaring a local variable and caching the value

  function updateFundingPaymentPointer() public {
    uint256 _nextFundingPaymentTimestamp = nextFundingPaymentTimestamp();
    ...
  }

at the top level of the function (before the while loop), and in the end of the the loop this variable can be updated with the result of a new call to the function.

   while (block.timestamp >= _nextFundingPaymentTimestamp) {
      ...
      latestFundingPaymentPointer += 1;

      _nextFundingPaymentTimestamp = nextFundingPaymentTimestamp();
      ...
   }
   ...
}

Cache the value of nextFundingPaymentTimestamp() in _updateFundingRate()

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L594-L614

In _updateFundingRate() (PerpetualAtlanticVault.sol), the nextFundingPaymentTimestamp() method is called 3 times in the first branch of the if-else statement.

Instead of doing that, the value can be cached in a local variable before the if-else statement and be used afterwards instead of doing additional calls.

    function _updateFundingRate(uint256 amount) private {
        uint256 _nextFundingPaymentTimestamp = nextFundingPaymentTimestamp();
        ...
    }

Unused storage variable liquiditySlippageTolerance

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

  uint256 public liquiditySlippageTolerance = 5e5; // 0.5%

The variable is seemingly unused neither in the RdpxV2Core contract nor in any of the other contracts.

Unnecessary casts to the IStableSwap interface in _curveSwap

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

At the top level of the function _curveSwap, there's a local variable holding the interface to the DPXETH curve pool contract.

    IStableSwap dpxEthCurvePool = IStableSwap(addresses.dpxEthCurvePool);

In the if statement of that function, the ETH and DPXETH balances are fetched using:

      uint256 ethBalance = IStableSwap(addresses.dpxEthCurvePool).balances(a);
      uint256 dpxEthBalance = IStableSwap(addresses.dpxEthCurvePool).balances(
        b
      );

Instead, the local dpxEthCurvePool variable can be used directly:

      uint256 ethBalance = dpxEthCurvePool.balances(a);
      uint256 dpxEthBalance = dpxEthCurvePool.balances(b);

Cache DPXETH and ETH prices in _curveSwap

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

When calculating the minimum out amount in _curveSwap, 2 calls are made in each branch of the ternary operation to fetch either the price of DPXETH or the price of ETH.

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

The prices, however, can be cached in 2 local variables:

    uint256 dpxEthPrice = getDpxEthPrice();
    uint256 ethPrice = getEthPrice();

and be used instead of doing 1 extra call each time.

Cache the value of IERC20WithBurn(reserveAsset[reservesIndex["RDPX"]].tokenAddress) in _transfer

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

In the else branch of the if-else statement in the _transfer function of RdpxV2Core, the cast to the IERC20WithBurn interface of the RDPX token address can be cached in a local variable:

      IERC20WithBurn rdpx = IERC20WithBurn(reserveAsset[reservesIndex["RDPX"]].tokenAddress);

and used in the following safeTransferFrom, burn and safeTransfer calls:

      rdpx.safeTransferFrom(msg.sender, address(this), _rdpxAmount);
      rdpx.burn(_rdpxAmount * rdpxBurnPercentage / 1e10);
      rdpx.safeTransfer(addresses.feeDistributor, _rdpxAmount * rdpxFeePercentage / 1e10);

Calculate strike and timeToExpiry only in the if (putOptionsRequired) statement in calculateBondCost

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

The strike and timeToExpiry local variables are only used if putOptionsRequired is true and the code inside the if statement executes, but if that if putOptionsRequired is false, these two variables are going to go unused very time the function is called.

So they can safely be moved inside the if statement since they are only used by the code inside it.

    if (putOptionsRequired) {
      uint256 strike = IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
        .roundUp(rdpxPrice - (rdpxPrice / 4)); // 25% below the current price

      uint256 timeToExpiry = IPerpetualAtlanticVault(
        addresses.perpetualAtlanticVault
      ).nextFundingPaymentTimestamp() - block.timestamp;

      wethRequired += IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
        .calculatePremium(strike, rdpxRequired, timeToExpiry, 0);
    }