Dopex - 0x3b's results

Lines of code

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

Vulnerability details

Impact

roundUp rounds the price up with precision of 1e6, unfortunately the RDPX price also has small decimal number and the rounding actually inflates the price.

Proof of Concept

1. RDPX's price is not in USD, but in WETH

2. The oracle returns in with 8 decimals

3. roundUp rounds up with 6 decimals of precision

This all combined lead to an issue, because the current price of RDPX is 13.5 USD and the price of WETH is 1677 USD. This means that the oracle will return 0.008e8 as a price of RDPX represented in WETH. Now when we input this price into the strike formula this happens:

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

(0.008e8 - (0.008e8 / 4)) => 0.006e8

And when we call roundUp on this value, It will try and perform this equation to get the remainder:

0.006e8 % 1e6 => 0.6e6 % 1e6 => 0.6e6

And afterwards it will turn to the else where it will return _strike - remainder + roundingPrecision

0.6e6 - 0.6e6 + 1e6 => 1e6

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

This actually makes the bond from put to call, as it increases the strike price above our current one.

Also found in purchase.

PoC

Place it in Unit.t.sol

  function test_calculate_cost() public {
    address user1 = address(111);
    rdpxPriceOracle.updateRdpxPrice(8e5); // 0.008e8
    vm.prank(user1);
    rdpxV2Core.calculateBondCost(1e18, 0);
  }

When run with -vvvv we see that we input 6e5 (0.008e8 - 25%) into roundUp and get 1e6.

    │   ├─ [2876] PerpetualAtlanticVault::roundUp(600000 [6e5]) [staticcall]
    │   │   └─ ← 1000000 [1e6]

Tools Used

Manual review

Recommended Mitigation Steps

Use lower precision for the rounding -- 1e4 as an example.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

subtractLoss require statement makes it possible to DoS this function and makes PerpetualAtlanticVault settle unusable.

Proof of Concept

The require statement in subtractLoss allows an attacker to exploit it by donating a small amount the collateral token, causing the require condition to fail consistently.

    require(
      collateral.balanceOf(address(this)) == _totalCollateral - loss,
      "Not enough collateral was sent out"
    );

The issue arises because this statement strictly checks if the collateral held by the contract ( as balance ) matches the storage variable _totalCollateral. By donating any amount of the collateral token to the contract, the value of _totalCollateral remains unchanged, while collateral.balanceOf(address(this)) changes.

As this function is used in settle process, it can cause repeated reverting, leading to a DoS scenario and preventing the admin from settling any bond.

Tools Used

Manual review

Recommended Mitigation Steps

You can modify the require statement from == to >=, or use an another check method.

    require(
-     collateral.balanceOf(address(this)) == _totalCollateral - loss,
+     collateral.balanceOf(address(this)) >= _totalCollateral - loss,
      "Not enough collateral was sent out"
    );

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 recoverERC721 function is utilized to recover an NFT and transfer it to the V2Core. V2Core lacks the capability to return the NFT, as well as the functionality to utilize it for collecting LP rewards or removing/adding liquidity.

Proof of Concept

Although the likelihood to be called regularly is low, the existence of the recoverERC721 suggests that it is expected to be called upon when necessary.

• In the event of a hack requiring the rescue of NFTs.
• If the current AMO contract is replaced with a new one and the NFTs need to be transferred away to the new AMO.

The recoverERC721 within UniV3LiquidityAmo is designed for NFT recovery. However, in practice, it does the opposite. When invoked by an admin, the NFT is transferred to V2Core. While this might appear beneficial in theory, it's worth noting that V2Core is equipped with a function for receiving NFTs, but not for sending them. The only available withdrawal function is for ERC20 tokens (emergencyWithdraw), which won't work as it withdraws the balance of address(this) rather than the NFT ID.

When the NFT is transferred, it becomes perpetually stuck. Furthermore, since this contract fails to implement the UNIv3 AMO, it remains unable to use the NFT to collectFees, addLiquidity or removeLiquidity.

Tools Used

Manual review

Recommended Mitigation Steps

Add a function to also handle NFT withdraws to different addresses.

  function emergencyWithdraw(address nft,address to, uint ID,) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _whenPaused();

    ERC721(nft).safeTransfer(to, ID);
    emit LogEmergencyNftWithdraw(msg.sender, nft,to,ID);
  }

Or make recoverERC721 to send the NFT to any address you need.

-  function recoverERC721(address tokenAddress,uint256 token_id) external onlyRole(DEFAULT_ADMIN_ROLE) {
+  function recoverERC721(address tokenAddress,address to, uint256 token_id) external onlyRole(DEFAULT_ADMIN_ROLE) {
    INonfungiblePositionManager(tokenAddress).safeTransferFrom(
      address(this),
-     rdpxV2Core,
+     to,
      token_id
    );
    emit RecoveredERC721(tokenAddress, token_id);
  }

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

addLiquidity has a slippage input that in neglected in the current code, which combined with the non-existent deadline leads to losses and unpredictable trades.

Proof of Concept

addLiquidity has 2 parameters (amountAMin and amountBMin), that account for slippage losses, if the trade executes at a bad time, or protect against MEV. However in the current implementation, the slippage check in not accounted for and in it's place 0 is used, meaning any change in price will satisfy the call. This combined with no deadline (as block.timestamp means the TX will pass anytime, 1h, 1 day or even 1 week after is sent) means that the trade can execute at unprecedented time, and unknown prices.

A likely scenario would be for the call to remain stuck in the mem-pool for some days, executing at bad prices - Protocol receives less LP

    (, , uint256 lp) = IUniswapV2Router(addresses.ammRouter).addLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      tokenAAmountOut,
      amountB / 2,
      0,
      0,//@audit should should be `amountBMin`
      address(this),
      block.timestamp + 10//@audit why?
    )

Tools Used

Manual review

Recommended Mitigation Steps

You can either add minA and minB to the function parameters, or like in other contract implement slippage tolerance calculation. Here is my pseudo-code that you can add here, before the addLiquidity call:

uint minA = tokenAAmountOut - (tokenAAmountOut * slippageTolerance);
uint minB = amountB / 2 - ((amountB / 2) * slippageTolerance);

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

The reLP function within the ReLPContract does not return excess amounts, potentially resulting in small amounts of WETH being left trapped within the contract.

Proof of Concept

The reLP function includes an addliquidity call to UniSwap v2. Subsequently, the function returns the remaining tokenA (RDPX) to the RdpxV2Core. However, it neglects to return the remaining tokenB (WETH) held within the contract. Over time, although the un-returned WETH accumulates, but remains permanently trapped within the contract.

Tools Used

Manual review

Recommended Mitigation Steps

A simple solution would be to modify the code to also return the unused WETH by adding the following changes:

     IERC20WithBurn(addresses.tokenA).safeTransfer(
       addresses.rdpxV2Core,
       IERC20WithBurn(addresses.tokenA).balanceOf(address(this))
     );
+    IERC20WithBurn(addresses.tokenB).safeTransfer(
+      addresses.rdpxV2Core,
+      IERC20WithBurn(addresses.tokenB).balanceOf(address(this))
+    );

Assessed type

ETH-Transfer