Dopex - josephdara's results

Lines of code

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

Vulnerability details

Impact

The recoverERC721 function in the UNiV3LiquidityAmo is used to send NFTs from the amo contract to the rdpxV2Core. However there is a critical issue here, any NFT sent to the rdpxV2Core is lost forever. It is locked in without any method to retrieve it. There is no transfer or approve function for NFTs that are sent to the rdpxV2Core contract. The only tokens that can be extracted are ERC20 tokens. This permanently bricks the removeLiquidity() function in the UNiV3LiquidityAmo contract since it has to burn the NFT to complete the redeem.

    univ3_positions.decreaseLiquidity(decreaseLiquidityParams);


    univ3_positions.collect(collect_params);


    univ3_positions.burn(pos.token_id);

Proof of Concept

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

Although the ERC721 holder is inherited from open zeppelin, there is no method implemented to transfer it out

Tools Used

Manual Review

Recommended Mitigation Steps

I suggest that the NFT should be transferred to the msg.sender, or a withdrawNFT function should be added to the rdpxV2Core

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

The liquidity pooled from the PerpetualAtlanticVaultLP is used by the core contract. This liquidity is provided by anyone, and after each epoch 1 week an incentive is paid after to further incentivise liquidity provision. However, the funds can be stolen from the Lp contract immediately they are deposited.

Proof of Concept

Once the rdpx incentive is paid into the contract,

1. A malicious user creates a contract that calls both deposit and redeem in the same function
2. They acquire large funds through a flash loan to deposit an exact amount that would allow them to withdraw a large share of the new rdpx collateral tokens that was paid to former liquidity providers.

Tools Used

Manual Review

Recommended Mitigation Steps

Use a TWAP method to allocate rdpx incentives when users are withdrawing collateral. Or instead pay the incentive in the collateral token to the simply increase the value of every holders share.

Assessed type

ERC20

Lines of code

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

Vulnerability details

Impact

The function convertToShares is used to get the asset conversion to shares, however there is a mathematical oversight here. The prices gotten from the oracle are in 18 decimals, however the calculations are done in 8 decimals

Proof of Concept

In the perpetuallatlanticvaultlp.convertToShares()

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


    uint256 totalVaultCollateral = totalCollateral() +
      ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8); //@audit price conversion here assumes 8 decimals for the price returned
    return
      supply == 0 ? assets : assets.mulDivDown(supply, totalVaultCollateral);
  }

In the perpetualAtlanticVault.getUnderlyingPrice():

 /// @inheritdoc	IPerpetualAtlanticVault
  function getUnderlyingPrice() public view returns (uint256) {
    return IRdpxEthOracle(addresses.assetPriceOracle).getRdpxPriceInEth();
  }

from the interface comment see that it assumes a return price of 8 decimals https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/IPerpetualAtlanticVault.sol#L20-L24

However in the IRdpxEthOracle.getRdpxPriceInEth()

 /// @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");
    }

The price is returned in 18 decimals : https://github.com/dopex-io/rdpx-eth-oracle/blob/5762c2339b1c45b87ff4db172e43cef4a0ff603a/src/RdpxEthOracle.sol#L241C2-L253C6

This causes an issue because the decimals are handled based on the assumption that the oracle returns 8 decimals which it returns 18

Tools Used

Manual Review

Recommended Mitigation Steps

Perform the accurate scaling by dividing by 1e18 instead of 1e8

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L218-L229 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L274-L284

Vulnerability details

Impact

Rounding of shares to assets do not conform to the ERC4626 standard: EIP 4626's Security Considerations (https://eips.ethereum.org/EIPS/eip-4626)

Finally, ERC-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:

If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down. If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.

This shows the rounding direction required for deposit/redeem. However this is not the case here even though this contract is an ERC4626 contract : https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L112-L121 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L137-L149

Proof of Concept

  /// @notice Returns the amount of shares recieved for a given amount of assets
  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);
  }



function _convertToAssets(
    uint256 shares
  ) internal view virtual returns (uint256 assets, uint256 rdpxAmount) {
    uint256 supply = totalSupply;
    return
      (supply == 0)
        ? (shares, 0)
        : (
          shares.mulDivDown(totalCollateral(), supply),
          shares.mulDivDown(_rdpxCollateral, supply)
        );
  }

As seen above, both deposits and withdrawals are a being rounded down. This is an issue for the users when they are to redeem their tokens. Since both the rdpx amount and the collateral amount is being rounded down

Tools Used

Manual Review , Solodit

Recommended Mitigation Steps

I recommend that deposits should be left as they are, however redeeming should roundup the amount being sent back to the user.

Assessed type

Math