Dopex - ak1's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L764 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L315 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L359 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L199-L205

Vulnerability details

Impact

Options can not be settled.

Proof of Concept

From RdpxV2Core, admin would call the settle function to settle the options and send the collateral and rdpx to the respective contracts.

while traversing the array of options in RdpxV2Core, the function settle of PerpetualAtlanticVault would be called.

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L772-L774

    (amountOfWeth, rdpxAmount) = IPerpetualAtlanticVault(
      addresses.perpetualAtlanticVault
    ).settle(optionIds);

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/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( ------------>> audit find, follow.
      ethAmount
    );
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP)
      .unlockLiquidity(ethAmount);
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addRdpx(
      rdpxAmount
    );


    emit Settle(ethAmount, rdpxAmount, optionIds);
  }

in above settle function, the options array is traversed and ethAmount and rdpxAmount amounts are called.

collateral is sent from perpetualAtlanticVaultLP to rdpxV2Core.

collateralToken.safeTransferFrom(
  addresses.perpetualAtlanticVaultLP,
  addresses.rdpxV2Core,
  ethAmount
);

Transfer rdpx from rdpx rdpxV2Core to perpetual vault

// Transfer rdpx from rdpx rdpxV2Core to perpetual vault
IERC20WithBurn(addresses.rdpx).safeTransferFrom(
  addresses.rdpxV2Core,
  addresses.perpetualAtlanticVaultLP,
  rdpxAmount

After transferring the collateral amount, the loss of this would be updated in perpetualAtlanticVaultLP by calling the subtractloss

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

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

in above call, the total collateral value after deducting the loss is checked strictly with the contract balance.

above condition can be break, by simply donating small amount of find to the PerpetualAtlanticVaultLP

Tools Used

Manual review.

Recommended Mitigation Steps

Update the condition check in

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

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

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L238 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L286 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L346 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L160-L178

Vulnerability details

Impact

1. The purpose of the uniswapV2 integration would not be fully utilized.
2. Missing of timely sync with rdpxV2Core will impact in the following operations in rdpxV2Core _purchaseOptions _stake settle provideFunding lowerDepeg

Proof of Concept

Whenever any interaction is done with UniSwapV2, _sendTokensToRdpxV2Core is called to send the asset to the rdxV2Core. Once the asset is sent, the reserve of respective asset should be updated by calling the syc. This is done in UniSwap V3 flow, but not in the uniswap v2.

we can see the _sendTokensToRdpxV2Core function calling in UniV2LiquidityAmo contract in following places.

1. addLiquidity - unused tokens are sent back to rdpxV2core.
2. removeLiquidity - liquidity is sent to rdpxV2Core.
3. swap - tokens are sent to rdpxV2Core after swap.

At the end of above function call, the tokens are sent to rdpxV2Core contract.

Tools Used

Manual review.

Recommended Mitigation Steps

call the rdxV2Core's sync function in _sendTokensToRdpxV2Core function.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L119-L133

Vulnerability details

Impact

Missing of the reserve asset update in rdpxV2Core. Lack of asset update would impact the functions which depend the asset reserve. For example, one place would be lowerDepeg where one of the reserve asset is use to swap for other and one is burned.

Proof of Concept

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L119-L133

  function collectFees() external onlyRole(DEFAULT_ADMIN_ROLE) {
    for (uint i = 0; i < positions_array.length; i++) {
      Position memory current_position = positions_array[i];
      INonfungiblePositionManager.CollectParams
        memory collect_params = INonfungiblePositionManager.CollectParams(
          current_position.token_id,
          rdpxV2Core,
          type(uint128).max,
          type(uint128).max
        );


      // Send to custodian address
      univ3_positions.collect(collect_params);
    }
  }

collectFees would be called to collect the fee and this fee would be sent to rdpxCore.

But the sync of rdpxCore' is missed.

One of the place where it is implemented correctly is removeLiquidity, in this function call, at the end of the function after fee collection, _sendTokensToRdpxV2Core is called. this function would call the rdpxV2Core's sync.

Tools Used

Manual review.

Recommended Mitigation Steps

Update the collectfee function as shown below.

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L119-L133

  function collectFees() external onlyRole(DEFAULT_ADMIN_ROLE) {
    for (uint i = 0; i < positions_array.length; i++) {
      Position memory current_position = positions_array[i];
      INonfungiblePositionManager.CollectParams
        memory collect_params = INonfungiblePositionManager.CollectParams(
          current_position.token_id,
          rdpxV2Core,
          type(uint128).max,
          type(uint128).max
        );


      // Send to custodian address
      univ3_positions.collect(collect_params);
    }
    
    IRdpxV2Core(rdpxV2Core).sync();  +++++++     audit update

  }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Allowing the redemption of bonds would lead to transfer of receipet tokens to the user.

This receiptTokenAmount would be used to convert into ETH and dpxEth.

when any one of the asset depreciates, redeeming the other one would lead to lowering the value of the depreciating asset further.

Proof of Concept

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L1016-L1028

  function redeem(
    uint256 id,
    address to
  ) external returns (uint256 receiptTokenAmount) {
    // Validate bond ID
    _validate(bonds[id].timestamp > 0, 6);
    // Validate if bond has matured
    _validate(block.timestamp > bonds[id].maturity, 7);


    _validate(
      msg.sender == RdpxV2Bond(addresses.receiptTokenBonds).ownerOf(id),
      9
    );

Above redeem of bond does not have whenNotPaused protection.

when we look at the bond issue method which is done through bond. But, the redeem call does not have this pause protection.

Tools Used

Manual review.

Recommended Mitigation Steps

use whenNotPause modifier for redeem call also.

Assessed type

Access Control