Dopex - Inspex's results

Lines of code

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

Vulnerability details

Impact

Admin won't be able to settle the options.

Proof of Concept

The PerpetualAtlanticVaultLP.subtractLoss() function is designed to handle loss deductions from the _totalCollateral balance, and it ensures that the contract's collateral balance matches the expected balance after the loss deduction at line 201.

PerpetualAtlanticVaultLP.sol

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

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

However, the attacker can directly transfer the collateral token to the PerpetualAtlanticVaultLP contract which will not increase the _totalCollateral balance. As a result, the contract's collateral balance won't match the expected balance after a loss deduction. This causes the subtractLoss() function always to revert when called.

Since the RdpxV2Core.settle() function includes a call to the PerpetualAtlanticVaultLP.subtractLoss() function when the subtractLoss() function fails, it will cause the entire transaction to revert. As a result, the admin won't be able to settle the options.

RdpxV2Core.sol

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

function settle(
  uint256[] memory optionIds
)
  external
  onlyRole(DEFAULT_ADMIN_ROLE)
  returns (uint256 amountOfWeth, uint256 rdpxAmount)
{
  _whenNotPaused();
  (amountOfWeth, rdpxAmount) = IPerpetualAtlanticVault(
    addresses.perpetualAtlanticVault
  ).settle(optionIds);
  for (uint256 i = 0; i < optionIds.length; i++) {
    optionsOwned[optionIds[i]] = false;
  }

  reserveAsset[reservesIndex["WETH"]].tokenBalance += amountOfWeth;
  reserveAsset[reservesIndex["RDPX"]].tokenBalance -= rdpxAmount;

  emit LogSettle(optionIds);
}

PerpetualAtlanticVaultLP.sol

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

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

Tools Used

Manual Review

Recommended Mitigation Steps

We suggest changing the validation of the collateral sent out tracking in subtractLoss() function from == into >= at line 201. For example:

PerpetualAtlanticVaultLP.sol

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

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/UniV3LiquidityAmo.sol#L324-L336

Vulnerability details

Impact

The UniswapV3 NFT will stuck in the RdpxV2Core contract forever, resulting in the admin not being able to close the position and retrive the liqudity back.

Proof of Concept

The UniV3LiquidityAmo.recoverERC721() function is used to transfered the UniswapV3 NFT to the RdpxV2Core contract.

UniV3LiquidityAmo.sol

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L324-L336

  function recoverERC721(
    address tokenAddress,
    uint256 token_id
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    // Only the owner address can ever receive the recovery withdrawal
    // INonfungiblePositionManager inherits IERC721 so the latter does not need to be imported
    INonfungiblePositionManager(tokenAddress).safeTransferFrom(
      address(this),
      rdpxV2Core,
      token_id
    );
    emit RecoveredERC721(tokenAddress, token_id);
  }

However, there is no approval or transfer UniswapV3 NFT methods in the RdpxV2Core contract. As a result, the NFT will be stuck in the RdpxV2Core contract forever. Thus, the owner not being able to close the position and retrive the liqudity back since the owner of the NFT is RdpxV2Core contract.

Tools Used

Manual Review

Recommended Mitigation Steps

We suggest implementing the recovery UniswapV3 NFT method in the RdpxV2Core contract.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

The functions that using sync() in RdpxV2Core will always revert.

Proof of Concept

The withdraw() function use for withdraw unused WETH of delegate.

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

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

However, the function does not decrease the totalWethDelegated state, this state was increased in the addToDelegate() function, Resulting in the sync() function will revert due to an arithmetic underflow when it calculate balance with totalWethDelegated because the balance of weth in contract is less than totalWethDelegated.

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

function addToDelegate(
    uint256 _amount,
    uint256 _fee
) external returns (uint256) {
    _whenNotPaused();
    // fee less than 100%
    _validate(_fee < 100e8, 8);
    // amount greater than 0.01 WETH
    _validate(_amount > 1e16, 4);
    // fee greater than 1%
    _validate(_fee >= 1e8, 8);

    IERC20WithBurn(weth).safeTransferFrom(msg.sender, address(this), _amount);

    Delegate memory delegatePosition = Delegate({
      amount: _amount,
      fee: _fee,
      owner: msg.sender,
      activeCollateral: 0
    });
    delegates.push(delegatePosition);

    // add amount to total weth delegated
    totalWethDelegated += _amount;

    emit LogAddToDelegate(_amount, _fee, delegates.length - 1);
    return (delegates.length - 1);
}

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

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

The sync() function was used in both the reLP() and _sendTokensToRdpxV2Core() functions, as a result, these functions will become unusable.

Tools Used

Manual Review

Recommended Mitigation Steps

Adding the deduction of the totalWethDelegated state in the withdraw() function.

RdpxV2Core.sol

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;
+    totalWethDelegated -= amountWithdrawn;
    IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
}

Assessed type

DoS