Dopex - sces60107's results

Lines of code

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

Vulnerability details

Impact

When the admin of RdpxV2Core calls RdpxV2Core.settle. PerpetualAtlanticVault.settle would be called to settle the put options. It would transfer rdpx from RdpxV2Core to perpetualAtlanticVaultLP and transfer weth from perpetualAtlanticVaultLP to RdpxV2Core. Afterwards, it invokes perpetualAtlanticVaultLP.subtractLoss to record the decrease in weth. However, the strict equality check in perpetualAtlanticVaultLP.subtractLoss makes itself vulnerable to potential DoS attack.

Proof of Concept

PerpetualAtlanticVault.settle calculates the weth amount transferring from PerpetualAtlanticVaultLP. And it calls PerpetualAtlanticVaultLP.subtractLoss to record the loss. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L359

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

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

PerpetualAtlanticVaultLP.subtractLoss has a strict equality check collateral.balanceOf(address(this)) == _totalCollateral - loss https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L201

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

It seems to be a reasonable check. But the problems is that collateral.balanceOf(address(this)) can be attacked by directly sending 1 wei of weth to PerpetualAtlanticVaultLP. _totalCollateral won’t increase when directly sending weth. The attacker can easily make the check always fail.

Tools Used

Manual Review

Recommended Mitigation Steps

There two methods can mitigate the issues depending on what Dopex really wants to do:

1. Simply change collateral.balanceOf(address(this)) == _totalCollateral - loss to collateral.balanceOf(address(this)) >= _totalCollateral - loss
2. If Dopex just wants to ensure that there is enough collateral. Change collateral.balanceOf(address(this)) == _totalCollateral - loss to _totalCollateral >= loss

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L125 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L274 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L515 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L480

Vulnerability details

Impact

When calling PerpetualAtlanticVaultLP.deposit(), it calls previewDeposit() to calculate the minted shares to the receiver. However, it doesn’t call perpetualAtlanticVault.updateFunding() first, leading to the unfair shares for the early depositors.

Proof of Concept

Users can call PerpetualAtlanticVaultLP.deposit() to deposit collateral and mint LP tokens. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L125

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

It uses previewDeposit to calculate the minted shares. And previewDeposit calls convertToShares to do the actual calculation. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L274 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L480

  function convertToShares(
    uint256 assets
  ) public view returns (uint256 shares) {
    uint256 supply = totalSupply;
    uint256 rdpxPriceInAlphaToken = perpetualAtlanticVault.getUnderlyingPrice();

    uint256 totalVaultCollateral = totalCollateral() +
      ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8);
    return
      supply == 0 ? assets : assets.mulDivDown(supply, totalVaultCollateral);
  }

Afterwards, deposit() calls perpetualAtlanticVault.updateFunding(). It would transfer the funding from perpetualAtlanticVault to PerpetualAtlanticVaultLP. It increases totalVaultCollateral . https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L515 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L480

  function updateFunding() public {
    updateFundingPaymentPointer();
    …

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

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

    …
  }

  function updateFundingPaymentPointer() public {
    while (block.timestamp >= nextFundingPaymentTimestamp()) {
      if (lastUpdateTime < nextFundingPaymentTimestamp()) {
        …

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

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

        …
    }
  }

Suppose Alice and Bob both minted 100 shares by depositing 100 weth. And there are 100 weth funds remaining in perpetualAtlanticVault. Those funds come from option premiums and RdpxV2Core funding. And Eve is going to call deposit().

• Eve call deposit() with 100 weth
• previewDeposit() calls convertToShares() and return 100 shares

assets.mulDivDown(supply, totalVaultCollateral) => 100*200 / 200 = 100

• perpetualAtlanticVault.updateFunding() is called, _totalCollateral then increases. totalVaultCollateral is now 400.
• Eve successfully deposit 100 weth and get 100 shares
• Eve then immediately call redeem() with 100 shares and receive 100*400/300=133 weth.

The result is clearly unfair to early depositors. Later depositors should not earn the funds generated before their deposits.

Tools Used

Manual Review

Recommended Mitigation Steps

PerpetualAtlanticVaultLP.deposit() should call perpetualAtlanticVault.updateFunding() before previewDeposit()

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

ReLPContract.reLP calculates min amount of tokenA to be received before doing swap. However, the calculation is wrong.

Proof of Concept

ReLPContract.reLP calculates min amount of tokenA to be received based on amountB and tokenAInfo.tokenAPrice https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L273

  function reLP(uint256 _amount) external onlyRole(RDPXV2CORE_ROLE) {
    …

    // calculate min amount of tokenA to be received
    mintokenAAmount =
      (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
      (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);

    uint256 tokenAAmountOut = IUniswapV2Router(addresses.ammRouter)
      .swapExactTokensForTokens(
        amountB / 2,
        mintokenAAmount,
        path,
        address(this),
        block.timestamp + 10
      )[path.length - 1];

    …

tokenAInfo.tokenAPrice is the price of tokenA against tokenB. In other words, it indicates how many tokenB that one tokenA is equivalent to. It makes no sense to multiply the amount of tokenB with the price of tokenA against tokenB. It should multiply amountB with the price of tokenB against tokenA.

Tools Used

Manual Review

Recommended Mitigation Steps

There are two ways to mitigate the issue.

1. Use price of tokenB instead of price of tokenA. But it needs another oracle to get the price.
2. Use (amountB / 2) / tokenAInfo.tokenAPrice instead of (amountB / 2) * tokenAInfo.tokenAPrice)

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

If minAmount is zero in RdpxV2Core._curveSwap. It would calculate the reasonable minOut based on getDpxEthPrice or getEthPrice. However, it use the wrong price to calculate the minOut

Proof of Concept

if minAmount is zero, RdpxV2Core._curveSwap would calculate minOut based on getDpxEthPrice or getEthPrice. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L545

  function _curveSwap(
    uint256 _amount,
    bool _ethToDpxEth,
    bool validate,
    uint256 minAmount
  ) internal returns (uint256 amountOut) {
    …

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

    // Swap the tokens
    amountOut = dpxEthCurvePool.exchange(
      _ethToDpxEth ? int128(int256(a)) : int128(int256(b)),
      _ethToDpxEth ? int128(int256(b)) : int128(int256(a)),
      _amount,
      minAmount > 0 ? minAmount : minOut
    );
  }

_ethToDpxEth should determine which price would be used. However, _curveSwap implements it incorrectly. When _ethToDpxEth is true, _amount means the amount of eth. and minOut should be the amount of DpxEth. The correct calculation should be:

(((_amount * (price of ETH against DpxEth)) / 1e8) -
        (((_amount *(price of ETH against DpxEth)) * slippageTolerance) / 1e16))

Therefore, getEthPrice() should be used. But _curveSwap uses getDpxEthPrice()

Tools Used

Manual Review

Recommended Mitigation Steps

  function _curveSwap(
    uint256 _amount,
    bool _ethToDpxEth,
    bool validate,
    uint256 minAmount
  ) internal returns (uint256 amountOut) {
    …

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



    // Swap the tokens
    amountOut = dpxEthCurvePool.exchange(
      _ethToDpxEth ? int128(int256(a)) : int128(int256(b)),
      _ethToDpxEth ? int128(int256(b)) : int128(int256(a)),
      _amount,
      minAmount > 0 ? minAmount : minOut
    );
  }

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

After an epoch has ended the admin of RdpxV2Core calls calculateFunding to calculate the funding generated at the last epoch. Then, calls payFunding to send weth from RdpxV2Core to PerpetualAtlanticVault. However, if purchase and settle happens before payFunding. It could lead to wrong calculation of generated funding and possible revert.

Proof of Concept

payFunding computes amount of options using optionsPerStrike[strike] - fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][strike] https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L421

  function calculateFunding(
    uint256[] memory strikes
  ) external nonReentrant returns (uint256 fundingAmount) {
    …

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

      uint256 timeToExpiry = nextFundingPaymentTimestamp() -
        (genesis + ((latestFundingPaymentPointer - 1) * fundingDuration));

      uint256 premium = calculatePremium(
        strike,
        amount,
        timeToExpiry,
        getUnderlyingPrice()
      );

      …
  }

purchase increases both optionsPerStrike[strike] and fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][strike] https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L303

  function purchase(
    uint256 amount,
    address to
  )
    external
    nonReentrant
    onlyRole(RDPXV2CORE_ROLE)
    returns (uint256 premium, uint256 tokenId)
  {
    …

    totalActiveOptions += amount;
    fundingPaymentsAccountedFor[latestFundingPaymentPointer] += amount;
    optionsPerStrike[strike] += amount;

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

    emit Purchase(strike, amount, premium, to, msg.sender);
  }

And settle reduces only optionsPerStrike[strike]. https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L337

  function settle(
    uint256[] memory optionIds
  )
    external
    nonReentrant
    onlyRole(RDPXV2CORE_ROLE)
    returns (uint256 ethAmount, uint256 rdpxAmount)
  {
    …
      optionsPerStrike[strike] -= amount;
      totalActiveOptions -= amount;

     …
  }

Suppose there are two options purchased at epoch 1. Those two options has the same strike price “A”. And the total amount of options is 200:

// at epoch 1
optionsPerStrike[A] = 200
fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][A] = 200

After an epoch has ended and latestFundingPaymentPointer is updated.

// at epoch 2
optionsPerStrike[A] = 200
fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][A] = 0

If nothing happens before calculateFunding. The amount used to calculate funding should be 200;

// calculateFunding
uint256 amount = optionsPerStrike[strike] -  fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][strike] = 200 - 0 = 200

However, if there is a new option with 200 amount purchased at epoch 2 and settle at the same epoch. And these actions happen before calculateFunding. The funding becomes 0.

// purchase a new option with 200 amount and strike price is still A.
optionsPerStrike[A] = 200 + 200 = 400
fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][A] = 0 + 200 = 200

// settle the new option
optionsPerStrike[A] = 400 - 200 = 200

// calculateFunding
uint256 amount = optionsPerStrike[strike] -  fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][strike] = 200 - 200 = 0

Moreover, if the amount of new option is 300. calculateFunding would revert.

// purchase a new option with 300 amount and strike price is still A.
optionsPerStrike[A] = 200 + 300 = 500
fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][A] = 0 + 300 = 300

// settle the new option
optionsPerStrike[A] = 500 - 300 = 200

// calculateFunding
uint256 amount = optionsPerStrike[strike] -  fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][strike] = 200 - 300 // revert.

Tools Used

Manual Review

Recommended Mitigation Steps

The best way to mitigate the issue is ensuring that calculateFunding is alway the first action at the beginning of epoch. But I’m not sure if it is easy to do without large modification of codes. Since it needs to record all the active strike prices.

Assessed type

Timing