Dopex - 0xvj's results

Lines of code

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

Vulnerability details

Impact

An attacker can brick the subtractLoss function, which is called in the settle function of the PerpetualAtlanticVault contract, so settle will also be bricked.

Proof of Concept

1. Attacker transfers 1 Wei to the PerpetualAtlanticVaultLP contract directly.
2. Because of that below require statement in subtractLoss function will always fail require( collateral.balanceOf(address(this)) == _totalCollateral - loss);
3. So subtractLoss function will always revert.

function subtractLoss(uint256 loss) public onlyPerpVault {
    require(
      // @audit strict equality will break this require if an attacker makes direct transfer
      collateral.balanceOf(address(this)) == _totalCollateral - loss,
      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
  }

Tools Used

Manual Review

Recommended Mitigation Steps

function subtractLoss(uint256 loss) public onlyPerpVault {
    require(
+     collateral.balanceOf(address(this)) >= _totalCollateral - loss,
-     collateral.balanceOf(address(this)) == _totalCollateral - loss,
      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
  }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

getLpTokenBalanceInWeth() will return incorrect value. Because if precision of getLpPrice() is 1e18 then (lpTokenBalance * getLpPrice()) in getLpTokenBalanceInWeth() should be divided by 1e18 as the number of decimals in a LP token are 1e18.

Vulnerability Detail

  function getLpPrice() public view returns (uint256) {
    // @audit here getLpPriceInEth's precision is not 1e8 its 1e18
    return IRdpxEthOracle(addresses.rdpxOracle).getLpPriceInEth();
  }

1. In UniV2LiquidityAmo.sol contract's getLpPrice() function it is assumed that the precision getLpPriceInEth() function of RdpxEthOracle is 1e8 but the actual precision of that function is 1e18.

2. Due to this false assumption instead of dividing (lpTokenBalance * getLpPrice()) by 1e18 they are dividing it by 1e8 in getLpTokenBalanceInWeth() function.

3. Due to this getLpTokenBalanceInWeth() precision will 1e28 but the precision of actual WETH token is 1e18

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

Tools Used

Manual Review

Recommended Mitigation Steps

If the return value of getLpPrice() needs to be in 1e8 precision divide the return value by 1e10 before returning.

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

calls to settle function will fail even though underlying price is less than the strike price.

Proof of Concept

function getUnderlyingPrice() public view returns (uint256) {
    // @audit returns price in 1e18 precision
    return IRdpxEthOracle(addresses.assetPriceOracle).getRdpxPriceInEth();
}

RdpxOracle.sol

/// @notice Returns the price of rDPX in ETH
    /// @return price price of rDPX in ETH in 1e18 decimals
    function getRdpxPriceInEth() external view override returns (uint price) {
        require(
            blockTimestampLast + timePeriod + nonUpdateTolerance >
                block.timestamp,
            "RdpxEthOracle: UPDATE_TOLERANCE_EXCEEDED"
        );

        price = consult(token0, 1e18);

        require(price > 0, "RdpxEthOracle: PRICE_ZERO");
    }

getUnderlyingPrice() function in PerpetualAtlanticVault contract returns the price of Rdpx(underlying) in 1e18 precision.

The precision of strike prices is 1e8 but the precision of underlying price 1e18 as we can see it in the above code.

Because of this calls to settle function will fail even though the underlying price is less than strike price as the below validation will fail because of inconsistent precisions. _validate(strike >= getUnderlyingPrice(), 7)

Due this precision inconsistency calculations many places will go wrong.

Tools Used

Manual Review

Recommended Mitigation Steps

Divide the return value of getUnderlyingPrice() by 1e10 to scale it down from 1e18 precision to 1e8 precision.

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L964 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L975-L990 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L306 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L873

Vulnerability details

Impact

1. The sync function will be bricked.
2. The reLP function of ReLp contract , which calls the sync function, will also be bricked.
3. So , both the bond and bondWithDelegate functions, which call the reLP function, will also be bricked.

Proof of Concept

1. An attacker get a large amount of WETH using a flashloan.
2. Now he delegates that WETH by calling addToDelegate method.
3. totalWethDelegated will be incremented by that amount because of this line totalWethDelegated += _amount;.
4. Now attacker withdraws all the delegated WETH using withdraw function.
5. But totalWethDelegated will not be decreased as it is not being decremented in withdraw function.
6. Due to this sync function will always revert because of underflow in this line.

Tools Used

Manual Review

Recommended Mitigation Steps

  function withdraw(
    uint256 delegateId
  ) external returns (uint256 amountWithdrawn) {
    _whenNotPaused();
    _validate(delegateId < delegates.length, 14);
    Delegate storage delegate = delegates[delegateId];
    _validate(delegate.owner == msg.sender, 9);

    amountWithdrawn = delegate.amount - delegate.activeCollateral;
    _validate(amountWithdrawn > 0, 15);
    delegate.amount = delegate.activeCollateral;

    IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);
+   totalWethDelegated -= amountWithdrawn

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

swaps will fail or execute with higher slippage than intended.

Proof of Concept

In _curveSwap() function of RdpxV2Core contract , getEthPrice() should be used in place of getDpxEthPrice(), and getDpxEthPrice() should be used in place of getEthPrice().

/**
   * @notice Returns the price of dpxETH against ETH
   * @dev    Price is in 1e8 Precision
   * @return dpxEthPrice dpxETH price
   **/
  function getDpxEthPrice() public view returns (uint256 dpxEthPrice) {
    return
      IDpxEthOracle(pricingOracleAddresses.dpxEthPriceOracle)
        .getDpxEthPriceInEth();
  }

getDpxEthPrice function returns the price of 1 dpxETH in ETH.

/**
   * @notice Returns the price of ETH token against the DpxEth
   * @dev    Price is in 1e8 Precision
   * @return ethPrice ethToken price
   **/
  function getEthPrice() public view returns (uint256 ethPrice) {
    return
      IDpxEthOracle(pricingOracleAddresses.dpxEthPriceOracle)
        .getEthPriceInDpxEth();
  }

getEthPrice returns the price of 1 ETH in dpxETH.

lets say 1 ETH = 2 dpxETH so 1 dpxETH = 0.5 ETH

So getEthPrice function returns 2 because 1 ETH = 2 dpxETH. And getDpxEthPrice function returns 0.5 because 1 dpxETH = 0.5 ETH.

If we try to swap 1 ETH into dpxETH the minAmount should represent how much dpxETH we should get so it should be around 2 with a slippage of 0.5 percent.

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

But if we calculate minOut according above code

 minOut = 1 * getDpxEthPrice - 1 * getDpxEthPrice * (0.5/100) 
        = (1 * 0.5)  - (1 * 0.5 * 0.5/100)
        = 0.4975

avoiding all the decimals hell and considering slippage as 0.5

Here we are getting minOut as 0.4975 instead of getting a value around 2. If the slippage calculation is correct we should get a value around 2 because current market value of 1 ETH is 2 dpxETH.So if we are swapping from 1 ETH to dpxETH minOut should be around 2 with 0.5% slippage.

Tools Used

Manual Review

Recommended Mitigation Steps

     // calculate minimum amount out
     uint256 minOut = _ethToDpxEth
-      ? (((_amount * getDpxEthPrice()) / 1e8) -
-        (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))
-      : (((_amount * getEthPrice()) / 1e8) -
-        (((_amount * getEthPrice()) * slippageTolerance) / 1e16));
+      ? (((_amount * getEthPrice()) / 1e8) -
+        (((_amount * getEthPrice()) * slippageTolerance) / 1e16))
+      : (((_amount * getDpxEthPrice()) / 1e8) -
+        (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16));

Assessed type

Math