Dopex - QiuhaoLi's results

Lines of code

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

Vulnerability details

Impact

Attackers can DoS settle/subtractLoss by transferring collateral tokens to the contract directly.

Proof of Concept

In subtractLoss we use a strict check to see if collateral.balanceOf(address(this)) == _totalCollateral - loss. Since we use balanceOf(address(this)), an attacker can send the weth (or other collateral tokens) to this contract to break the check, thus DoS the settle/subtractLoss.

Tools Used

Manual Review.

Recommended Mitigation Steps

Use >= instead of ==.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Liquidity providers can get zero shares when they deposit into PerpetualAtlanticVaultLP.

Proof of Concept

In deposit(), we first check if (shares = previewDeposit(assets)) != 0, and then we call perpetualAtlanticVault.updateFunding().

In updateFunding(), perpetualAtlanticVault will transfer funding into the PerpetualAtlanticVaultLP in a drip-vested manner, which will call addProceeds():

  function addProceeds(uint256 proceeds) public onlyPerpVault {
    require(
      collateral.balanceOf(address(this)) >= _totalCollateral + proceeds,
      "Not enough collateral token was sent"
    );
    _totalCollateral += proceeds;
  }

As we can see, _totalCollateral is increased. So the assets.mulDivDown(supply, totalVaultCollateral) in convertToShares is possible to round down to zero now. Especially when updateFunding transfers a lots funding into the PerpetualAtlanticVaultLP.

Tools Used

Manual Review.

Recommended Mitigation Steps

Reverse the order:

    perpetualAtlanticVault.updateFunding();
    
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1206 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1238 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L373 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 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L276 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L335 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L550

Vulnerability details

Impact

Wrong decimals/price if we use RdpxEthOracle.sol as the oracle.

Proof of Concept

rdpx/eth oracle is not in the scope of this audit, so we can assure they are correct and only check if we use the API right. According to lib/rdpx-eth-oracle/src/RdpxEthOracle.sol, both the getLpPriceInRdpx() and getRdpxPriceInEth() return price in 1e18 decimals:

    /// @dev Returns the price of LP in ETH in 1e18 decimals
    function getLpPriceInEth() external view override returns (uint) {
        // .....
        return (lpPriceIn112x112 * 1e18) >> 112;
    }
    
    /// @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");
    }

However, we assumed the decimals is 1e18 in the current implementation:

  /**
   * @notice Returns the price of a rDPX/ETH Lp token against the ETH token
   * @dev    Price is in 1e8 Precision @audit-issue 1e18
   * @return uint256 LP price
   **/
  function getLpPrice() public view returns (uint256) {
    return
      IRdpxEthOracle(pricingOracleAddresses.rdpxPriceOracle).getLpPriceInEth();
  }
  
  function getRdpxPrice() public view returns (uint256) {
    return
      IRdpxEthOracle(pricingOracleAddresses.rdpxPriceOracle)
        .getRdpxPriceInEth();
  }

Thie can lead to wrong decimals/price if we use RdpxEthOracle.sol as the oracle, for example in _calculateAmounts():

  function _calculateAmounts(
    uint256 _wethRequired,
    uint256 _rdpxRequired,
    uint256 _amount,
    uint256 _delegateFee
  ) internal view returns (uint256 amount1, uint256 amount2) {
    // Commented below for better clarity
    uint256 rdpxRequiredInWeth = (_rdpxRequired * getRdpxPrice()) / 1e8; // @audit-issue 1e18

Tools Used

Manual Review.

Recommended Mitigation Steps

Change 1e8 to 1e18.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Attackers can call addToDelegate and withdraw multiple times (or one time with a huge amount) to inflate totalWethDelegated, which leads to underflow in sync(). Since sync() is called by UniV3LiquidityAmo and Relp() is called by bond and bondwithdelegate, the core of Depox can suffer from DoS.

Proof of Concept

In withdraw(), we only update the delegate.amount but not decrease the totalWethDelegated:

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

So Attackers can call addToDelegate and withdraw multiple times (or one time with a huge amount) to inflate totalWethDelegated, which leads to underflow in sync():

  function sync() external {
    for (uint256 i = 1; i < reserveAsset.length; i++) {
      uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
        address(this)
      );

      if (weth == reserveAsset[i].tokenAddress) {
        balance = balance - totalWethDelegated; <--- underflow！
      }
      reserveAsset[i].tokenBalance = balance;
    }

I wrote a PoC using tests/rdpxV2-core/Periphery.t.sol:testReLpContract():

// .....
    rdpxV2Core.setIsreLP(true);
    
    // attacker calls addToDelegate/withdraw to inflate totalWethDelegated
    for (uint i = 0; i < 10; i++) {
      uint256 delegate_id = rdpxV2Core.addToDelegate(1 * 1e18, 10e8);
      rdpxV2Core.withdraw(delegate_id);
    }

    rdpxV2Core.bond(1 * 1e18, 0, address(this));
// .....

Output:

$ forge test -vv --match-test testReLpContract
[⠰] Compiling...
No files changed, compilation skipped

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: Arithmetic over/underflow] testReLpContract() (gas: 5425565)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.57s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: Arithmetic over/underflow] testReLpContract() (gas: 5425565)

Tools Used

Manual Review.

Recommended Mitigation Steps

When a delegate is withdrawn, decrease the totalWethDelegated with amountWithdrawn.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L233

Vulnerability details

Impact

More rdpx is removed, and the price will go higher than intended. In extreme situations where _amount is big enough (msg.sender has enough rdpx and eth), tokenAToRemove will be bigger than tokenALpReserve, then lpToRemove >totalSupply, revert the transaction.

Proof of Concept

In the official doc: https://dopex.notion.site/rDPX-V2-RI-b45b5b402af54bcab758d62fb7c69cb4#23161c577fd1449085436e5a601358ae. We calculate the rDPX to remove as: ((amount_lp * 4) / rdpx_supply) * lp_rdpx_reserves * base_relp_percent.

However, in reLP(), we calculate the rdpx to remove as:

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

As we can see, we used tokenAInfo.tokenAReserve (IRdpxReserve(addresses.tokenAReserve).rdpxReserve(); // rdpx reserves) instead of the rdpx supply. This leads to a smaller divisor and bigger tokenAToRemove.

Tools Used

Manual Review.

Recommended Mitigation Steps

Replace tokenAInfo.tokenAReserve with IERC20WithBurn(tokenA).totalSupply().

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L273

Vulnerability details

Impact

The slippage protection may fail because mintokenAAmount is bigger than designed.

Proof of Concept

In reLP(), when we swap half weth for rdpx, we calculate the min amount of tokenA (rdpx) to be received as:

    mintokenAAmount =
      (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
      (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);

However, tokenAInfo.tokenAPrice is calculated as IRdpxEthOracle(addresses.rdpxOracle).getRdpxPriceInEth(), which is the rdpx price in eth (rdpx/eth). But we need to multiply eth's price in rdpx (eth/rdpx).

Tools Used

Manual Review.

Recommended Mitigation Steps

Use IDpxEthOracle(pricingOracleAddresses.dpxEthPriceOracle).getEthPriceInDpxEth() instead of tokenAInfo.tokenAPrice.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

When _curveSwap is called with minAmount = 0 (upperDepeg/lowerDepeg use the default slippage 0.5%), the minOut will be a wrong value which leads to slippage protect failure.

Proof of Concept

In _curveSwap, the getDpxEthPrice and getEthPrice is in wrong order:

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

When _ethToDpxEth is true, we swap eth for dpxeth. However, we use getDpxEthPrice, which is dpxeth's price in eth (dpxeth/eth). Also, when we swap dpxeth for eth, we use getEthPrice which is eth's price in dpxeth.

Below is a PoC that demonstrates we get the wrong slippage when calling upperDepeg with default slippage tolerance:

// add this to _curveSwap
    amountOut = dpxEthCurvePool.exchange(
      _ethToDpxEth ? int128(int256(a)) : int128(int256(b)),
      _ethToDpxEth ? int128(int256(b)) : int128(int256(a)),
      _amount,
      minAmount > 0 ? minAmount : minOut
    );
    if (!_ethToDpxEth) {
      console.log("getDpxEthPrice = %s > 1e8, swap %s dpxEth for Eth (0.5% slippage):", getDpxEthPrice(), _amount);
      console.log("minOut (wrong slippage) = \t%s", minOut);
      uint256 ideal_Out = (_amount * getDpxEthPrice()) / 1e8;
      uint256 correct_minOut = (ideal_Out - (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16));
      console.log("minOut (correct slippage) = \t%s", correct_minOut);
      console.log("Actual got Eth amountOut = \t%s", amountOut);
      console.log("Actual slippage = \t\t%s% > 0.5%", (ideal_Out - amountOut)*1e2/ideal_Out);
    }

// tests/rdpxV2-core/Unit.t.sol
function testUpperDepeg_default_slippageTolerance() public {
    // swap weth for dpxETH (price after swap 180000000)
    dpxETH.approve(address(curvePool), 200 * 1e18);
    address coin0 = IStableSwap(curvePool).coins(0);
    if (coin0 == address(weth)) {
      IStableSwap(curvePool).exchange(0, 1, 200 * 1e18, 0);
    } else {
      IStableSwap(curvePool).exchange(1, 0, 200 * 1e18, 0);
    }
    // dpxEth : Eth = 1.8 : 1
    dpxEthPriceOracle.updateDpxEthPrice(180000000);

    // mint dpxEth and swap for Eth, using default slippage (0.5%)
    rdpxV2Core.upperDepeg(10e18, 0);
}

Output:

$ forge test -vv --match-test "testUpperDepeg_default_slippageTolerance"
Running 1 test for tests/rdpxV2-core/Unit.t.sol:Unit
[PASS] testUpperDepeg_default_slippageTolerance() (gas: 246264)
Logs:
  getDpxEthPrice = 180000000 > 1e8, swap 10000000000000000000 dpxEth for Eth (0.5% slippage):
  minOut (wrong slippage) =     5527777722500000000
  minOut (correct slippage) =   17910000000000000000
  Actual got Eth amountOut =    15885460869832720611
  Actual slippage =             11% > 0.5%

Tools Used

Manual Review.

Recommended Mitigation Steps

Reverse the order:

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

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

PerpetualAtlanticVault pays more/less weth to LP than designed (premium + funding), this can lead to tokenomics harm or DoS.

Proof of Concept

In updateFundingDuration(), we only update the fundingDuration:

  function updateFundingDuration(
    uint256 _fundingDuration
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    fundingDuration = _fundingDuration;
  }

However, when we pay the funding to LP, we use the fundingRates to multiply the remaining unfunded duration to calculate the transfer amount, e.g.:

  function updateFundingPaymentPointer() public {
    while (block.timestamp >= nextFundingPaymentTimestamp()) {
      if (lastUpdateTime < nextFundingPaymentTimestamp()) {
        uint256 currentFundingRate = fundingRates[latestFundingPaymentPointer];

        uint256 startTime = lastUpdateTime == 0
          ? (nextFundingPaymentTimestamp() - fundingDuration)
          : lastUpdateTime;

        lastUpdateTime = nextFundingPaymentTimestamp();

        collateralToken.safeTransfer(
          addresses.perpetualAtlanticVaultLP,
          (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) /
            1e18
        );

This will lead to two situations:

1. If fundingDuration is increased and fundingRates is calculated before the updateFundingDuration() call. Then PerpetualAtlanticVault will pay more funding to the LP, which can lead to DoS in the end (insufficient assets).
2. On the reverse side, PerpetualAtlanticVault will pay less funding to the LP, which is a loss to the collateral providers.

Tools Used

Manual Review.

Recommended Mitigation Steps

When updating the fundingDuration, we should also update the last fundingRates relatively.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

1. _updateFundingRate (called by purchase and payFunding) will revert with an underflow error because startTime > endTime. The functions of PerpetualAtlanticVault will be DoS.
2. _updateFundingRate won't update fundingRates because endTime == startTime, which leads to a loss of options' collateral providers.

Proof of Concept

In updateFundingDuration, the admin can change fundingDuration directly:

  function updateFundingDuration(
    uint256 _fundingDuration
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    fundingDuration = _fundingDuration;
  }

And in nextFundingPaymentTimestamp(), we calculate the next payment timestamp as:

genesis + (latestFundingPaymentPointer * fundingDuration);

This can lead to a situation where the next payment timestamp is smaller than the last funding payment timestamp when we set fundingDuration to a smaller value. For example:

1. genesis = 1600000000, latestFundingPaymentPointer = 101, and fundingDuration is 600000. Current lastUpdateTime = 1660000000, block.timestamp = 1660400000, and nextFundingPaymentTimestamp() = 1660600000.
2. updateFundingDuration is called and changed fundingDuration to 100000. Now if we call nextFundingPaymentTimestamp(), it will return 1600000000 + 100000*101 = 1610100000 which is less than lastUpdateTime.
3. If _updateFundingRate is called by purchase or payFunding and fundingRates[latestFundingPaymentPointer] == 0, the startTime will be lastUpdateTime (1660000000) and endTime will be 1610100000. Later, an underflow error will occur when we update fundingRates with (amount * 1e18) / (endTime - startTime):

  function _updateFundingRate(uint256 amount) private {
    if (fundingRates[latestFundingPaymentPointer] == 0) {
      uint256 startTime;
      if (lastUpdateTime > nextFundingPaymentTimestamp() - fundingDuration) {
        startTime = lastUpdateTime;
      } else {
        startTime = nextFundingPaymentTimestamp() - fundingDuration;
      }
      uint256 endTime = nextFundingPaymentTimestamp();
      fundingRates[latestFundingPaymentPointer] =
        (amount * 1e18) /
        (endTime - startTime); // <---
    }

In special cases where the new nextFundingPaymentTimestamp() equals lastUpdateTime. If fundingRates[latestFundingPaymentPointer] != 0 (e.g., purchased or payFunding is already called in the same epoch), since we will return directly to mitigate zero division, the fundingRates won't be updated, and options' collateral providers will loss fundings.

else {
      uint256 startTime = lastUpdateTime;
      uint256 endTime = nextFundingPaymentTimestamp();
      if (endTime == startTime) return; // <---
      fundingRates[latestFundingPaymentPointer] =
        fundingRates[latestFundingPaymentPointer] +
        ((amount * 1e18) / (endTime - startTime));
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

When the fundingDuration is changed, we should first update genesis to the last payment timestamp and then reset latestFundingPaymentPointer and lastUpdateTime. (Also, fundingRates should be updated. But this is another issue.)

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Users are unable to withdraw their rdpx tokens when there are no weth (collateral tokens) in the vault.

Proof of Concept

In redeem(), to mitigate the rounding error in previewRedeem, we revert if the returned weth amount is zero:

  function redeem(
    uint256 shares,
    address receiver,
    address owner
  ) public returns (uint256 assets, uint256 rdpxAmount) {
    perpetualAtlanticVault.updateFunding();

    if (msg.sender != owner) {
      uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.

      if (allowed != type(uint256).max) {
        allowance[owner][msg.sender] = allowed - shares;
      }
    }
    (assets, rdpxAmount) = redeemPreview(shares);

    // Check for rounding error since we round down in previewRedeem.
    require(assets != 0, "ZERO_ASSETS");

However, this is a possible situation that can happen: When all the options are settled in the first epoch and there is no funding from PerpetualAtlanticVault, the contract will only have rdpx and no weth. However, the poor liquidity providers can't withdraw the rdpx since the require(assets != 0, "ZERO_ASSETS") will revert.

Tools Used

Manual Review.

Recommended Mitigation Steps

Change the check to require(assets != 0 || rdpxAmount != 0, "ZERO_ASSETS");.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L306

Vulnerability details

Impact

The left weth tokens after (addresses.ammRouter).addLiquidity() are locked in ReLPContract.

Proof of Concept

In reLp(), after we add liquidity to the AMM, we only transfer the LP/rdpx to the rdpxV2Core contract:

    (, , uint256 lp) = IUniswapV2Router(addresses.ammRouter).addLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      tokenAAmountOut,
      amountB / 2,
      0,
      0,
      address(this),
      block.timestamp + 10
    );

    // transfer the lp to the amo
    IERC20WithBurn(addresses.pair).safeTransfer(addresses.amo, lp);
    IUniV2LiquidityAmo(addresses.amo).sync();

    // transfer rdpx to rdpxV2Core
    IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      IERC20WithBurn(addresses.tokenA).balanceOf(address(this))
    );
    IRdpxV2Core(addresses.rdpxV2Core).sync();
  }

The weth tokens are left in this contract, and since there is no token withdraw function like emergencywithdrawal, these tokens are locked here.

I wrote a PoC by adding console.log in reLP() to prove this:

// ......
    uint256 weth_balance_0B = IERC20WithBurn(addresses.tokenB).balanceOf(address(this));
    uint256 weth_balance_0A = IERC20WithBurn(addresses.tokenA).balanceOf(address(this));
    console.log("rdpx_balance_start = %s", weth_balance_0A);
    console.log("weth_balance_start = %s\n", weth_balance_0B);

    (, uint256 amountB) = IUniswapV2Router(addresses.ammRouter).removeLiquidity(
      addresses.tokenA,
      addresses.tokenB,
// ......
    console.log("tokenAAmountOut = \t%s", tokenAAmountOut);
    console.log("amount_tokenA_sent =  %s", amount_tokenA_sent);
    console.log("amountB / 2 = \t%s", amountB / 2);
    console.log("amount_tokenB_sent =  %s\n", amount_tokenB_sent);

    // transfer the lp to the amo
    IERC20WithBurn(addresses.pair).safeTransfer(addresses.amo, lp);
    IUniV2LiquidityAmo(addresses.amo).sync();
// ......

    uint256 weth_balance_3B = IERC20WithBurn(addresses.tokenB).balanceOf(address(this));
    uint256 weth_balance_3A = IERC20WithBurn(addresses.tokenA).balanceOf(address(this));
    console.log("rdpx_balance_end = %s", weth_balance_3A);
    console.log("weth_balance_end = %s", weth_balance_3B);
  }

Output:

$ forge test -vv --match-test testReLpContract

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[PASS] testReLpContract() (gas: 4037937)
Logs:
  rdpx_balance_start = 0
  weth_balance_start = 0

  tokenAAmountOut =     1806007704320917659
  amount_tokenA_sent =  1806007704320917659
  amountB / 2 =         362643377803882543
  amount_tokenB_sent =  361620970285555063

  rdpx_balance_end = 0
  weth_balance_end = 1022407518327480

As we can see, there are weth tokens left.

Tools Used

Manual Review.

Recommended Mitigation Steps

Transfer the weth left to rdpxV2Core in reLP().

Assessed type

Token-Transfer

[low] RdpxV2Core.sol _transfer should update reserveAsset[reservesIndex["RDPX"]] if (rdpxBurnPercentage + rdpxFeePercentage) < 1e10, or add constraints in setRdpxBurnPercentage/setRdpxFeePercentage

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

In the setters of rdpxBurnPercentage and rdpxFeePercentage, we only check if the value is bigger than zero, not check if they sum up to 1e10.And in _transfer(), we didn't check if rdpxBurnPercentage + rdpxFeePercentage) == 1e10 and didn't update reserveAsset[reservesIndex["RDPX"]].tokenBalance relatively. This can lead to two results if the admin changes rdpxBurnPercentage/rdpxFeePercentage wrongly:

1. When (rdpxBurnPercentage + rdpxFeePercentage) < 1e10, reserveAsset[reservesIndex["RDPX"]].tokenBalance will be smaller than the actual value.
2. When (rdpxBurnPercentage + rdpxFeePercentage) > 1e10, more rdpx is spent than designed. Eventually, the function of the core can be stopped since no more rdpx is usable.

To solve this, we need to make sure (rdpxBurnPercentage + rdpxFeePercentage) == 1e10 in setters or _transfer, and update the reserveAsset[reservesIndex["RDPX"]].tokenBalance if so.

[low] PerpetualAtlanticVault should round up when calculating requiredCollateral

In purchase(), we round down the requiredCollateral. This can lead to insufficient collateral, even zero collateral when (amount * strike) < 1e8. Rounding up is safer.

[low] UniV3LiquidityAmo should add setter for univ3_factory, univ3_positions, and univ3_router

In UniV3LiquidityAmo's constructor, we hardcoded the uniswap contract's address. However, these addresses are different on other blockchains like celo: https://docs.uniswap.org/contracts/v3/reference/deployments. To help the project migrate to more blockchains, we should have setters for these state variables.

[low] UniV3LiquidityAmo:liquidityInPool() should check if get_pool is zero

In https://docs.uniswap.org/contracts/v3/reference/core/interfaces/IUniswapV3Factory#getpool, the univ3_factory.getPool(address(rdpx), _collateral_address, _fee) will return a zero address if the pool doesn't exist. Should add a check and revert with the info.

[low] provideFunding should be hardcoded to decrease weth balance after payFunding

In provideFunding, there is a reserveAsset[reservesIndex["WETH"]].tokenBalance -= fundingAmount; after we called PerpetualAtlanticVault(addresses.perpetualAtlanticVault).payFunding();. And in payFunding, collateralToken can be set by admin, which means it can be a token other than weth. This can lead to a wrong token decrease in provideFunding if that happens. Should use a setter or get the collateralToken first and then decrease the relatively reserve asset in proivdeFuding.

[low] UniV2LiquidityAmo/UniV3LiquidityAmo and ReLPContract is vulernable to MEV attack

There are multiple uniswap calls in UniV2LiquidityAmo/UniV3LiquidityAmo that use block.timestamp + 1 and type(uint256).max or large timestamp as the deadline. They are vulnerable to MEV attack, for example:

    ISwapRouter.ExactInputSingleParams memory swap_params = ISwapRouter
      .ExactInputSingleParams(
        _tokenA,
        _tokenB,
        _fee_tier,
        address(this),
        2105300114, // Expiration: a long time from now @audit-issue deadline vulnerable to MEV
        _amountAtoB,
        _amountOutMinimum,
        _sqrtPriceLimitX96
      );

    ISwapRouter.ExactInputSingleParams memory swap_params = ISwapRouter
      .ExactInputSingleParams(
        _tokenA,
        _tokenB,
        _fee_tier,
        address(this),
        2105300114, // Expiration: a long time from now @audit-issue deadline vulnerable to MEV
        _amountAtoB,
        _amountOutMinimum,
        _sqrtPriceLimitX96
      );

    // remove liquidity
    (tokenAReceived, tokenBReceived) = IUniswapV2Router(addresses.ammRouter)
      .removeLiquidity(
        addresses.tokenA,
        addresses.tokenB,
        lpAmount,
        tokenAAmountMin,
        tokenBAmountMin,
        address(this),
        block.timestamp + 1
      );

    (, uint256 amountB) = IUniswapV2Router(addresses.ammRouter).removeLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      lpToRemove,
      mintokenAAmount,
      mintokenBAmount,
      address(this),
      block.timestamp + 10

    );

Note: They are not mentioned in https://github.com/code-423n4/2023-08-dopex/blob/main/bot-report.md#m02-using-blocktimestamp-as-the-deadlineexpiry-invites-mev.

[low] UniV2LiquidityAmo.sol/RdpxV2Core.sol:approveContractToSpend is vulnerable to front-run attack

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L411 (also in UniV2LiquidityAmo.sol)

In approveContractToSpend, we used IERC20WithBurn(_token).approve(_spender, _amount) directly without setting it to zero first or using increaseAllowance or decreaseAllowance. This is vulnerable to a front-run attack where a user can front-run the approve and spend old_allowance+new_allowance totally.

  function approveContractToSpend(
    address _token,
    address _spender,
    uint256 _amount
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _validate(_token != address(0), 17);
    _validate(_spender != address(0), 17);
    _validate(_amount > 0, 17);
    IERC20WithBurn(_token).approve(_spender, _amount); <--
  }

It's better to use increase or decrease APIs or set it to zero first.

[non-critical] RdpxDecayingBonds.sol: decreaseAmount should make sure amout is less than bonds[bondId].rdpxAmount

The comment of decreaseAmount() says Decreases the bond amount but it directly sets bonds[bondId].rdpxAmount to the amount passed in. It should add checks to make sure it's a decrease (bonds[bondId].rdpxAmount > amount).

[non-critical] ReLPContract.sol:reLP wrong comment of baseReLpRatio

uint256 baseReLpRatio = (reLPFactor *
      Math.sqrt(tokenAInfo.tokenAReserve) *
      1e2) / (Math.sqrt(1e18)); // 1e6 precision

The comment is wrong, should be 1e8 according to the developer and context.

[non-critical] PerpetualAtlanticVaultLP.sol approve _perpetualAtlanticVault rdpx with type(uint256).max but never used

In the constructor of PerpetualAtlanticVaultLP, we approve _perpetualAtlanticVault rdpx with type(uint256).max: ERC20(rdpx).approve(_perpetualAtlanticVault, type(uint256).max);. But _perpetualAtlanticVault never uses PerpetualAtlanticVaultLP's rdpx. So this approval is uselss and should be removed.

[non-critical] UniV2LiquidityAmo.sol/RdpxV2Core.sol:approveContractToSpend should allow zero amount

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L410 (also in UniV2LiquidityAmo.sol)

In approveContractToSpend, we validated amount > 0, this should be allowed since setting allowance to zero is a normal operation to cancel all the approved amounts.

[non-critical] UniV2LiquidityAmo.sol:emergencyWithdraw() should only be callable when paused

Currently, we don't have a pausable function in UniV2LiquidityAmo.sol, and the admin role can call emergencyWithdraw anytime. It's better to add a _only_paused check in it.

We can use a put option for all delegate bonds instead of one for every option

In bondWithDelegate we can delecate more than once, and for every delegate we buy the option for the bond. Since all the bonds are in the same transaction, the strike price will be the same. To save gas, we can buy a single option outside the for loop in bondWithDelegate. This can save gas in two ways:

1. When bonding with a delegate, we can save gas by only calling _purchaseOptions once.
2. When we settle, instead of calling multiple times on different options, we can do that in one call.

This can save considerable gas if there are many delegates when calling bondWithDelegate.

When delegate.amount = 0, we can delete the slot or reuse it

Currently, we don't reuse the delegate ID (mapping) if a delegate is fully withdrawn by the owner, which brings a linear increase in storage used. This can be optimized to reuse slots or delete the mapping index for gas refund.

When a bond is withdrawn from RdpxV2Core, we can delete bonds[id] for gas refund

Currently, we don't reuse the bonds[id] or delete them if a user redeem the bond, which brings a linear increase in storage used. This can be optimized to resue the slot or delete the slot for gas refund in redeem().

Only calculate strike and timeToExpiry if putOptionsRequired

Currently, in calculateBondCost(), we always calculate the strike and timeToExpiry even put options is not required. This can be optimized to only calculated them if putOptionsRequired is set.

owner field of struct Bond in RdpxDecayingBonds is not used

In contracts/decaying-bonds/RdpxDecayingBonds.sol, the struct Bond has an owner field, but this field is never used. For example, in RdpxV2Core.sol:_transfer, we only use the expiry and amount fields. Deleting this field can reduce storage/gas used.

RdpxDecayingBonds:mint() has redundant hasRole check

In mint(), we have a onlyRole(MINTER_ROLE) modifier, but then we have a require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");. This is redundant and costs more gas.

delete optionPositions[optionIds[i]] for more gas refund in settle()

Currently, we only set optionPositions[optionIds[i]].strike to zero at the end of settle, leaving amount and positionId unchanged. We can delete optionPositions[optionIds[i]] to clear them also, thus getting more gas refunded.

Calculate (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) only once in updateFundingPaymentPointer()

In updateFundingPaymentPointer(), we calculated the time spent in one epoch multiple times:

        collateralToken.safeTransfer(
          addresses.perpetualAtlanticVaultLP,
          (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) / // @audit-issue GAS this can be calculated only once
            1e18
        );

        IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP)
          .addProceeds(
            (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) /
              1e18
          );

        emit FundingPaid(
          msg.sender,
          ((currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) /
            1e18),
          latestFundingPaymentPointer
        );

The gas can be saved by calculating (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) only once.

call nextFundingPaymentTimestamp only once in _updateFundingRate()

In _updateFundingRate(), we use nextFundingPaymentTimestamp() multiple times. Since the latestFundingPaymentPointer and fundingDuration is not changed during the call, we can read the result to a local variable and use it later to save gas.