Dopex - Toshii's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1189-L1190 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L576-L583

Vulnerability details

Impact

Due to a lack of adequate precision, the calculated strike price for a PUT option for rDPX is not guaranteed to be 25% OTM, which breaks core assumptions around (1) protecting downside price movement of the rDPX which makes up part of the collateral for dpxETH & (2) not overpaying for PUT option protection.

More specifically, the price of rDPX as used in the calculateBondCost function of the RdpxV2Core contract is represented as ETH / rDPX, and is given in 8 decimals of precision. To calculate the strike price which is 25% OTM based on the current price, the logic calls the roundUp function on what is effectively 75% of the current spot rDPX price. The issue is with the roundUp function of the PerpetualAtlanticVault contract, which effectively imposes a minimum value of 1e6.

Considering approximate recent market prices of $2000/ETH and $20/rDPX, the current price of rDPX in 8 decimals of precision would be exactly 1e6. Then to calculate the 25% OTM strike price, we would arrive at a strike price of 1e6 * 0.75 = 75e4. The roundUp function will then round up this value to 1e6 as the strike price, and issue the PUT option using that invalid strike price. Obviously this strike price is not 25% OTM, and since its an ITM option, the premium imposed will be significantly higher. Additionally this does not match the implementation as outlined in the docs.

Proof of Concept

When a user calls the bond function of the RdpxV2Core contract, it will calculate the rdpxRequired and wethRequired required by the user in order to mint a specific _amount of dpxETH, which is calculated using the calculateBondCost function:

function bond(
  uint256 _amount,
  uint256 rdpxBondId,
  address _to
) public returns (uint256 receiptTokenAmount) {
  _whenNotPaused();
  // Validate amount
  _validate(_amount > 0, 4);

  // Compute the bond cost
  (uint256 rdpxRequired, uint256 wethRequired) = calculateBondCost(
    _amount,
    rdpxBondId
  );
  ...
}

Along with the collateral requirements, the wethRequired will also include the ETH premium required to mint the PUT option. The amount of premium is calculated based on a strike price which represents 75% of the current price of rDPX (25% OTM PUT option). In the calculateBondCost function:

function calculateBondCost(
  uint256 _amount,
  uint256 _rdpxBondId
) public view returns (uint256 rdpxRequired, uint256 wethRequired) {
  uint256 rdpxPrice = getRdpxPrice();

  ...

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

  uint256 timeToExpiry = IPerpetualAtlanticVault(
    addresses.perpetualAtlanticVault
  ).nextFundingPaymentTimestamp() - block.timestamp;
  if (putOptionsRequired) {
    wethRequired += IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
      .calculatePremium(strike, rdpxRequired, timeToExpiry, 0);
  }
}

As shown, the strike price is calculated as:

uint256 strike = IPerpetualAtlanticVault(addresses.perpetualAtlanticVault).roundUp(rdpxPrice - (rdpxPrice / 4));

It uses the roundUp function of the PerpetualAtlanticVault contract which is defined as follows:

function roundUp(uint256 _strike) public view returns (uint256 strike) {
  uint256 remainder = _strike % roundingPrecision;
  if (remainder == 0) {
    return _strike;
  } else {
    return _strike - remainder + roundingPrecision;
  }
}

In this contract roundingPrecision is set to 1e6, and this is where the problem arises. As I mentioned earlier, take the following approximate market prices: $2000/ETH and $20/rDPX. This means the rdpxPrice, which is represented as ETH/rDPX in 8 decimals of precision, will be 1e6. To calculate the strike price, we get the following: 1e6 * 0.75 = 75e4. However this value is fed into the roundUp function which will convert the 75e4 to 1e6. This value of 1e6 is then used to calculate the premium, which is completely wrong. Not only is 1e6 not 25% OTM, but it is actually ITM, meaning the premium will be significantly higher than was intended by the protocol design.

Tools Used

Manual review

Recommended Mitigation Steps

The value of the roundingPrecision is too high considering reasonable market prices of ETH and rDPX. Consider decreasing it.

Assessed type

Math

Lines of code

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#L199-L205

Vulnerability details

Impact

When an option is ITM, the admin can call the settle function of the RdpxV2Core contract, which will call the settle function of the PerpetualAtlanticVault contract. This function checks that the WETH balance of the PerpetualAtlanticVaultLP contract is exactly equal to the cached _totalCollateral minus the taken WETH collateral (else the call reverts). An attacker can force this call to always revert by sending a single wei of WETH to the PerpetualAtlanticVaultLP contract, thus making it impossible for the option to be exercised. This in turn means there's no downside protection for the rDPX collateral portion of the dpxETH tokens, breaking the economic security of dpxETH.

Proof of Concept

Let's consider the case in which a single perpetual PUT option has been issued, and there's a single LP in the PerpetualAtlanticVaultLP contract which posted the required WETH collateral. Let's say that the option involved covering 100 rDPX with 0.75 ETH. At the time rDPX was $20 and ETH was $2000, therefore this would be the correct amount of ETH covering 100 rDPX for a 25% OTM PUT option.

When the LP first deposits the WETH into the PerpetualAtlanticVaultLP contract by using the deposit function, it will increase the _totalCollateral by the exact amount of WETH they deposited (0.75 ETH). When a user later bonds through calling the bond function of the RdpxV2Core, it will call the purchase function of the PerpetualAtlanticVault contract. Here we are again assuming that this involved covering 100 rDPX with the entire 0.75 ETH amount, assuming the prices I mentioned earlier. (note: as of this point, _activeCollateral=_totalCollateral=0.75 ETH).

Going forward I will just assume the funding rate is 0 to keep the math simple. Incorporating this will not change this attack path in any way. Now, after a period of time, let's say the price of rDPX has decreased and so the option purchased earlier is now ITM. To execute this perpetual put option, the admin will call the settle function of the RdpxV2Core contract, which then calls the settle function of the PerpetualAtlanticVault contract, which is defined as follows:

function settle(
  uint256[] memory optionIds
)
  external
  nonReentrant
  onlyRole(RDPXV2CORE_ROLE)
  returns (uint256 ethAmount, uint256 rdpxAmount)
{
  _whenNotPaused();
  _isEligibleSender();

  updateFunding();

  for (uint256 i = 0; i < optionIds.length; i++) {
    uint256 strike = optionPositions[optionIds[i]].strike;
    uint256 amount = optionPositions[optionIds[i]].amount;

    // check if strike is ITM
    _validate(strike >= getUnderlyingPrice(), 7);

    ethAmount += (amount * strike) / 1e8;
    rdpxAmount += amount;
    optionsPerStrike[strike] -= amount;
    totalActiveOptions -= amount;

    // Burn option tokens from user
    _burn(optionIds[i]);

    optionPositions[optionIds[i]].strike = 0;
  }

  // Transfer collateral token from perpetual vault to rdpx rdpxV2Core
  collateralToken.safeTransferFrom(
    addresses.perpetualAtlanticVaultLP,
    addresses.rdpxV2Core,
    ethAmount
  );
  // Transfer rdpx from rdpx rdpxV2Core to perpetual vault
  IERC20WithBurn(addresses.rdpx).safeTransferFrom(
    addresses.rdpxV2Core,
    addresses.perpetualAtlanticVaultLP,
    rdpxAmount
  );

  IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
    ethAmount
  );
  IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP)
    .unlockLiquidity(ethAmount);
  IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addRdpx(
    rdpxAmount
  );

  emit Settle(ethAmount, rdpxAmount, optionIds);
}

The most important part of this function call is the calculation of the ethAmount, which is the amount of ETH equivalent to the 25% OTM price of the rDPX when this option was first issued. As stated earlier, this is equal to 0.75 ETH. This function performs the exchange of ETH and rDPX tokens between the RdpxV2Core and PerpetualAtlanticVaultLP contracts, and then importantly writes off this loss by calling the subtractLoss function of the PerpetualAtlanticVaultLP contract, with the ethAmount value. This subtractLoss function is defined as follows:

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

It checks that the balance of WETH in the contract is exactly equal to _totalCollateral minus this loss, which is 0.75 ETH. Normally, this would not revert - with this example: 0 ETH == 0.75 ETH - 0.75 ETH. However, an attacker can DOS this function by sending 1 wei of WETH to the PerpetualAtlanticVaultLP contract because: 1 wei ETH == 0.75 ETH - 0.75 ETH is not a valid statement.

Tools Used

Manual review

Recommended Mitigation Steps

The subtractLoss function should not use an exact comparison.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

An attacker is able to atomically steal large amounts of the funding yield from the PerpetualAtlanticVaultLP contract. This is due to the fact that the deposit function of the PerpetualAtlanticVaultLP contract will first issue the attacker shares based on the current assets in the contract, and then call perpetualAtlanticVault.updateFunding(), which sends the funding yield to this contract (which increases the assets in the contract). The Attacker is then able to withdraw their initial funds from the protocol, while also getting an equivalent amount of the funding yield which was just deposited into the contract (same percentage of the outstanding shares they minted atomically).

Proof of Concept

The deposit function of the PerpetualAtlanticVaultLP contract is defined as follows:

function deposit(
  uint256 assets,
  address receiver
) public virtual returns (uint256 shares) {
  // Check for rounding error since we round down in previewDeposit.
  require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

  perpetualAtlanticVault.updateFunding();

  // Need to transfer before minting or ERC777s could reenter.
  collateral.transferFrom(msg.sender, address(this), assets);

  _mint(receiver, shares);

  _totalCollateral += assets;

  emit Deposit(msg.sender, receiver, assets, shares);
}

As mentioned earlier, the shares are calculated using the current total amount of assets in the protocol at the time that the user calls deposit. When there is no rDPX in this contract, the total amount of assets is effectively equivalent to the output of the totalCollateral function, which just returns _totalCollateral.

Following this calculation of shares, perpetualAtlanticVault.updateFunding() is called, where the updateFunding function is defined as follows:

function updateFunding() public {
  updateFundingPaymentPointer();
  uint256 currentFundingRate = fundingRates[latestFundingPaymentPointer];
  uint256 startTime = lastUpdateTime == 0
    ? (nextFundingPaymentTimestamp() - fundingDuration)
    : lastUpdateTime;
  lastUpdateTime = block.timestamp;

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

  IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addProceeds(
    (currentFundingRate * (block.timestamp - startTime)) / 1e18
  );
  ...
}

Importantly, at the end of this function, the addProceeds function of the PerpetualAtlanticVaultLP contract is called, which will increment the _totalCollateral amount with the latest funding yield.

At this point, after the execution of the deposit function, the attacker can then call redeem, which will send them back their initial deposit & at the same time send them a portion of the funding yield equivalent to the ratio of their balance of shares vs the total supply of shares minted.

Tools Used

Manual review

Recommended Mitigation Steps

In the deposit function, shares should be calculated after the call to perpetualAtlanticVault.updateFunding();.

Assessed type

MEV

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/core/RdpxV2Core.sol#L1001-L1003

Vulnerability details

Impact

The sync function of the RdpxV2Core contract is critical for ensuring that the cached balances of the tokens in the contract are up to date. For example, all of the AMO logic involves sending tokens directly to the RdpxV2Core contract, meaning there's no function which is used to both transfer the tokens and update the token balances accordingly. This sync function is therefore used to ensure that the token balances are up to date following the token transfers, allowing the RdpxV2Core contract to function smoothly.

The issue is that an attacker can trivially DOS this sync function by making it effectively always revert, which will in turn brick the entire system. Among other issues, it means that any of the Uniswap V2 AMO actions will not be able to update the RdpxV2Core contract state - and the Uniswap V3 AMO actions will always revert, as they call this sync function directly.

Proof of Concept

Consider how the sync function is implemented:

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;
    }
    reserveAsset[i].tokenBalance = balance;
  }

  emit LogSync();
}

Importantly notice that when the reserve asset being updated is WETH, the balance is calculated as balance = balance - totalWethDelegated;. The issue with the current implementation is that it's trivial to force totalWethDelegated to be greater than balance, causing this subtraction to underflow and the function to revert. This attack can be done in the following way:

1. In one tx, flashloan borrow WETH (from wherever there's no fees) and first call addToDelegate on the RdpxV2Core contract with this amount of WETH. This will update the totalWethDelegated in the following way: totalWethDelegated += _amount;.
2. At the end of the tx, call withdraw with delegateId equal to the position you created with the earlier call to addToDelegate. This will send you the entire amount of WETH back, which you will repay the flashloan with. Notably, this function will not decrement totalWethDelegated, meaning you have just increased it by a significant amount, without changing the balance of the WETH in the contract post this tx.

Following this attack, totalWethDelegated will be significantly larger than the balance of WETH in the RdpxV2Core contract, and so the subtraction in the sync function will always revert.

Tools Used

Manual review

Recommended Mitigation Steps

Update the withdraw function of the RdpxV2Core contract so that it properly decrements totalWethDelegated.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Depending on the reserves of rDPX, bonding discounts are given both on the rDPX and WETH collateral requirements for minting dpxETH. The bonding discounts (for both rDPX and WETH portions) are provided as rDPX which is taken from the treasury. The issue with this is that the RdpxV2Core contract only functions properly when the full amount of WETH is provided (75% of the dpxETH value), because as per docs, 50% of the value of the dpxETH you are minting is required in WETH to add liquidity to the dpxETH-WETH pool.

This discount in WETH collateral requirements can be as high as 2/3 * 75% = 50% of the entire value of dpxETH. The logic in the RdpxV2Core essentially then steals this extra WETH from the current balance of the contract, which can eventually drain this entire contract of WETH, leaving only rDPX tokens, which is highly problematic in terms of economic security. This will also DOS all future attempts to mint dpxETH once the WETH reserves are depleted.

Proof of Concept

When a user bonds using rDPX directly they first call the bond function of the RdpxV2Core contract:

function bond(
  uint256 _amount,
  uint256 rdpxBondId,
  address _to
) public returns (uint256 receiptTokenAmount) {
  _whenNotPaused();
  // Validate amount
  _validate(_amount > 0, 4);

  // Compute the bond cost
  (uint256 rdpxRequired, uint256 wethRequired) = calculateBondCost(
    _amount,
    rdpxBondId
  );
  ...
  // purchase options
  uint256 premium;
  if (putOptionsRequired) {
    premium = _purchaseOptions(rdpxRequired);
  }

  _transfer(rdpxRequired, wethRequired - premium, _amount, rdpxBondId);
  
  // Stake the ETH in the ReceiptToken contract
  receiptTokenAmount = _stake(_to, _amount);
  ...
}

The amount of WETH required, wethRequired, is calculated by the calculateBondCost function with the main logic being the following (note: the cost of the premium for the purchased amount of rDPX PUT option coverage is also included in this amount, but i am not showing that code here):

wethRequired =
  ((ETH_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount) /
  (DEFAULT_PRECISION * 1e2);

As can be seen, there is a potential bondDiscount, which can be at most 100e8. Given that ETH_RATIO_PERCENTAGE is set to 75e8, the minimum amount of wethRequired would be 25% of the _amount of dpxETH being minted. The remainder, which in this case would be 50% of the value of the dpxETH, will be provided by the reserve. Note: if this math didn't make sense, recall that 75% of the value of the minted dpxETH needs to be provided in ETH, and this discount can be as much as 2/3 of the ETH amount, meaning the minimum ETH requirement of the user will be 75% * 2/3 = 25%.

The remainder of the ETH portion (discountReceivedInWeth) is then paid for by the reserve in the _transfer function, which is defined as follows:

function _transfer(
  uint256 _rdpxAmount,
  uint256 _wethAmount,
  uint256 _bondAmount,
  uint256 _bondId
) internal {
  if (_bondId != 0) {
    ...
  } else {
    ...
    // calculate extra rdpx to withdraw to compensate for discount
    uint256 rdpxAmountInWeth = (_rdpxAmount * getRdpxPrice()) / 1e8;
    uint256 discountReceivedInWeth = _bondAmount -
      _wethAmount -
      rdpxAmountInWeth;
    uint256 extraRdpxToWithdraw = (discountReceivedInWeth * 1e8) /
      getRdpxPrice();

    // withdraw the rdpx
    IRdpxReserve(addresses.rdpxReserve).withdraw(
      _rdpxAmount + extraRdpxToWithdraw
    );

    reserveAsset[reservesIndex["RDPX"]].tokenBalance +=
      _rdpxAmount +
      extraRdpxToWithdraw;
  }
}

As can be seen, the discountReceivedInWeth, which is an amount of ETH, will then be converted into an equivalent amount of rDPX (extraRdpxToWithdraw), and then this amount of rDPX is withdrawn to serve as the collateral for the minted rDPX.

The issue with this is the following call to _stake within the flow of the bond function, which has the following line of code:

reserveAsset[reservesIndex["WETH"]].tokenBalance -= _amount / 2;

Essentially, irrespective of the actual amount of ETH which has been deposited by the user, for the specified _amount of dpxETH to mint, _amount / 2 of ETH will be taken from the token reserves of the RdpxV2Core contract. This means that as more and more ETH discounts are given, this ETH amount will be drained to zero. At this point, future calls to bond will revert.

Tools Used

Manual review

Recommended Mitigation Steps

The discount on the WETH portion during minting should be provided in WETH rather than in an equivalent amount of rDPX.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The wrong units are used to calculate mintokenAAmount in the reLP function of the ReLPContract contract, which is the slippage parameter used when swapping WETH to rDPX. The issue is that due to the wrong units, the mintokenAAmount value will be much less than expected, which can result in much greater than expected slippage, which can ultimately result in loss of funds during the swap due to MEV. This slippage value is also always auto-calculated, meaning this issue cannot be circumvented by e.g. admin actions.

Proof of Concept

The equation to calculate the mintokenAAmount slippage value in the reLP function is defined as follows:

// get rdpx price
tokenAInfo.tokenAPrice = IRdpxEthOracle(addresses.rdpxOracle)
  .getRdpxPriceInEth();
...
mintokenAAmount =
  (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
  (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);

Here, tokenB refers to WETH, while token A refers to rDPX. The tokenAInfo.tokenAPrice is defined as ETH / rDPX. The current price for ETH is $2000 and $20 for rDPX, so the rDPX price would be approximately 1/100.

Firstly, we can already tell there is an issue as the units don't match up. (amountB / 2) * tokenAInfo.tokenAPrice) in units is equal to ETH * ETH / rDPX, which is equal to ETH^2 / rDPX, instead of the expected rDPX (rDPX is the output of the swap, therefore mintokenAAmount units should be denoted in rDPX). In practice what this means is that the calculated mintokenAAmount value is significantly lower than expected, which can lead to more losses due to MEV. Here if we are swapping 1e18 WETH to rDPX, the mintokenAAmount is effectively equal to (1e18 * 1/100) - ((1e18 * 1/100) * 0.005) = ~0.00995e18, which in layman's terms means we expect 0.01e18 in rDPX, which is insane considering we are swapping in 1e18 WETH.

The actual mintokenAAmount value should be (1e18 * 100/1) - ((1e18 * 100/1) * 0.005) = ~99.5e18 rDPX. The magnitude of difference between the correct slippage amount and the current amount being calculated is huge.

Tools Used

Manual review

Recommended Mitigation Steps

Rather than using tokenAInfo.tokenAPrice, the price used should be denoted in rDPX/ETH.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The wrong units are used to calculate minOut in the _curveSwap function of the RdpxV2Core contract, which is the slippage parameter used when swapping tokens in Curve (assuming minAmount is not set). The issue is that due to the wrong units, the minOut value will be much less than expected, which can result in greater than expected slippage, which can ultimately result in loss of funds during the swap due to MEV. Effectively, the fallback method for slippage control fails to protect against slippage to the extent which it was intended.

Proof of Concept

The equation to calculate the minOut slippage value in the _curveSwap function is defined as follows:

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

Let's take the case where _ethToDpxEth is false, meaning we are swapping dpxETH to ETH in the Curve pool. This would only be triggered in the upperDepeg function, which is only called when dpxETH is priced greater than ETH. Let's consider the case where dpxETH is worth 1.01 ETH, which will mean the output of getEthPrice(), which is the price of ETH in dpxETH, will be 1/1.01 which is < 1.

Firstly, we can already tell there is an issue as the units don't match up. (_amount * getEthPrice()) in units is equal to dpxETH * dpxETH / ETH, which is equal to dpxETH^2/ETH, instead of the expected ETH (ETH is the output of the swap, therefore the minOut units should be denoted in ETH). In practice what this means is that the calculated minOut value is significantly lower than expected, which can lead to more losses due to MEV. Here if we are swapping 1e18 dpxETH, the minOut is effectively equal to (1e18 * 1/1.01) - ((1e18 * 1/1.01) * 0.005) = ~0.985e18. Since dpxETH is worth more than ETH, having a minOut value significantly lower than 1e18 in this case is completely invalid.

The actually minOut value should be (1e18 * 1.01/1) - ((1e18 * 1.01/1) * 0.005) = ~1.005e18. This is nearly a 2% difference in the calculated allowable slippage, which is huge for pegged assets.

Tools Used

Manual review

Recommended Mitigation Steps

The snippet should be updated to the following:

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

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The current implementation of the logic for issuing perpetual PUT options to cover the rDPX being used as collateral for minting dpxETH effectively does not allow the LPs deposited in the PerpetualAtlanticVaultLP contract to ever exit their position. The core reason behind this is to ensure that the rDPX tokens in the protocol are always fully covered by an equivalent amount of PUT options. The only way in which LPs can fully exit the system is if their options are all exercised, wherein they will receive rDPX equivalent to 75% of the value of the ETH that they deposited.

The implicit assumption here is that the LPs will continue to get paid a fair funding rate for the collateral in which they have deposited. However, there is actually no hard requirement that this funding rate is actually paid. Although if this funding isn't paid, it will mean that the options cannot be exercised, this is still not fair for the LPs, and does not match normal perpetual option implementations, where the position would be closed if funding is not provided. If the funding is not being paid, then the LPs should be able to withdraw their ETH collateral for said options.

Proof of Concept

The only function of the PerpetualAtlanticVaultLP contract which allows withdrawing deposits is the redeem function, but this does not allow removing ETH collateral from the PerpetualAtlanticVault when the funding is not being paid. Instead it will simply revert, as there's no way to decrease the _activeCollateral amount even if the funding is not being paid in the PerpetualAtlanticVault contract.

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

  ...
  
  (assets, rdpxAmount) = redeemPreview(shares);

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

  _rdpxCollateral -= rdpxAmount;

  beforeWithdraw(assets, shares);

  _burn(owner, shares);

  collateral.transfer(receiver, assets);

  IERC20WithBurn(rdpx).safeTransfer(receiver, rdpxAmount);

  emit Withdraw(msg.sender, receiver, owner, assets, shares);
}

Tools Used

Manual review

Recommended Mitigation Steps

Provide functionality to allow LPs to withdraw ETH if the funding is not paid for an extended period of time.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L302-L303 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L440 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L376-L380

Vulnerability details

Impact

A malicious user can DOS the funding payment logic of the PerpetualAtlanticVault, meaning for a given funding cycle, they can force LPs in the PerpetualAtlanticVaultLP contract to not receive any funding. Specifically, the attacker can force the payFunding function of the PerpetualAtlanticVault to revert on all calls for that funding period. This is the function where the admin is actually sending the funding payment for that period from the RdpxV2Core contract. The only requirement for this attack is there being a new funding period for which the admin has not yet paid the funding for & there being a call to settle an ITM option. These are simple requirements which have a fairly high likelihood of occurring in practice.

Proof of Concept

The attack begins with the beginning of a new funding period for which the payFunding function has not yet been triggered by the admin. It is highly likely that in general a call to this function will be delayed, as the logic allows for paying out the entire period's yield, irrespective of when the function is actually called during a funding period (e.g. no incentive to call it right at the beginning of the period). Let's assume that at this period in time, totalActiveOptions is equal to 1_000e18, which is just the amount of rDPX being covered. Let's also assume for simplicity that no new options have been purchased during this new funding period, meaning fundingPaymentsAccountedFor[latestFundingPaymentPointer] is equal to 0. Here, I am assuming that latestFundingPaymentPointer is pointing to this new funding cycle (note: this will happen only after the next call to updateFunding, but that detail is unimportant).

Now let's assume that there's a call to settle an ITM option. This could have either been triggered during this cycle, or triggered in the previous cycle, but the tx was not yet mined due to blockchain congestion. The attacker will frontrun this call to settle with a call to calculateFunding which is defined as follows:

function calculateFunding(
  uint256[] memory strikes
) external nonReentrant returns (uint256 fundingAmount) {
  _whenNotPaused();
  _isEligibleSender();

  updateFundingPaymentPointer();

  for (uint256 i = 0; i < strikes.length; i++) {
    _validate(optionsPerStrike[strikes[i]] > 0, 4);
    _validate(
      latestFundingPerStrike[strikes[i]] != latestFundingPaymentPointer,
      5
    );
    uint256 strike = strikes[i];

    uint256 amount = optionsPerStrike[strike] -
      fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][
        strike
      ];

    ...

    // Record number of options that funding payments were accounted for, for this epoch
    fundingPaymentsAccountedFor[latestFundingPaymentPointer] += amount;

    // record the number of options funding has been accounted for the epoch and strike
    fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][
      strike
    ] += amount;

    ...
  }
}

The relevant logic here is that by front-running a call to settle, the attacker is able to run the following logic (for all options, including the option which is about to be exercised): fundingPaymentsAccountedFor[latestFundingPaymentPointer] += amount;. Let's assume for this example that there were two outstanding options, each with different strike prices (one is ITM and one is OTM). The attacker has called the calculateFunding function with both these strike values, which will mean fundingPaymentsAccountedFor[latestFundingPaymentPointer] is now 1_000e18. This is because there's no check of whether these options are ITM or not.

Following this call, the settle function is called:

function settle(
  uint256[] memory optionIds
)
  external
  nonReentrant
  onlyRole(RDPXV2CORE_ROLE)
  returns (uint256 ethAmount, uint256 rdpxAmount)
{
  _whenNotPaused();
  _isEligibleSender();

  updateFunding();

  for (uint256 i = 0; i < optionIds.length; i++) {
    uint256 strike = optionPositions[optionIds[i]].strike;
    uint256 amount = optionPositions[optionIds[i]].amount;

    // check if strike is ITM
    _validate(strike >= getUnderlyingPrice(), 7);

    ethAmount += (amount * strike) / 1e8;
    rdpxAmount += amount;
    optionsPerStrike[strike] -= amount;
    totalActiveOptions -= amount;

    // Burn option tokens from user
    _burn(optionIds[i]);

    optionPositions[optionIds[i]].strike = 0;
  }

  ...
}

The relevant logic here is that for the option is being settled, the totalActiveOptions amount is being decremented by the amount of rDPX that option was covering: totalActiveOptions -= amount;. Let's assume for this example that it was 500e18 rDPX, meaning totalActiveOptions now equals 1_000e18-500e18=500e18.

Now with this done, let's see what happens when the admin attempts to call payFunding in order to pay the funding yield for this cycle. This has the following check, which is intended to check that the funding amount being paid covers all obligations:

_validate(
  totalActiveOptions == 
    fundingPaymentsAccountedFor[latestFundingPaymentPointer],
  6
);

We can see now that there is an issue. As shown earlier, totalActiveOptions is 500e18, while fundingPaymentsAccountedFor[latestFundingPaymentPointer] is 1_000e18. This check will revert, and so this payFunding function will not be able to be called.

Tools Used

Manual review

Recommended Mitigation Steps

Consider reworking this strict validation check in the payFunding function to account for when options are being settled.

Assessed type

DoS