Deadline check is not effective in the UniV2LiquidityAmo and UniV3LiquidityAmo contracts, allowing outdated slippage and allow pending transaction to be unexpected executed

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/amo/UniV2LiquidityAmo.sol#L231
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L190
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L250
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L295
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L257-L265
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L277-L295

Vulnerability details

Summary

Deadline check is not effective, allowing outdated slippage and allow pending transaction to be unexpected executed.

Vulnerability Detail

In the current implementation in UniV3LiquidityAmo.addLiquidity:

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

    INonfungiblePositionManager.MintParams
      memory mintParams = INonfungiblePositionManager.MintParams(
        params._tokenA,
        params._tokenB,
        params._fee,
        params._tickLower,
        params._tickUpper,
        params._amount0Desired,
        params._amount1Desired,
        params._amount0Min,
        params._amount1Min,
        address(this),
        type(uint256).max // @auidit deadline check is disabled
      );

    (uint256 tokenId, uint128 amountLiquidity, , ) = univ3_positions.mint(
      mintParams
    );

the deadline check is set to type(uint256).max, which means the deadline check is disabled!

Others implementations like UniV3LiquidityAmo.removeLiquidity:

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

    // remove liquidity
    INonfungiblePositionManager.DecreaseLiquidityParams
      memory decreaseLiquidityParams = INonfungiblePositionManager
        .DecreaseLiquidityParams(
          pos.token_id,
          liquidity,
          minAmount0,
          minAmount1,
          block.timestamp // @audit Deadline set block.timestamp can be problematic 
        );

UnitV3LiquidityAmo.swap:

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

    ISwapRouter.ExactInputSingleParams memory swap_params = ISwapRouter
      .ExactInputSingleParams(
        _tokenA,
        _tokenB,
        _fee_tier,
        address(this),
        2105300114, // Expiration: a long time from now  // @audit can be problematic
        _amountAtoB,
        _amountOutMinimum,
        _sqrtPriceLimitX96
      );

and the last in the UniV2LiquidityAmo.addLiquidity:

• https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/amo/UniV2LiquidityAmo.sol#L231

    // add Liquidity
    (tokenAUsed, tokenBUsed, lpReceived) = IUniswapV2Router(addresses.ammRouter)
      .addLiquidity(
        addresses.tokenA,
        addresses.tokenB,
        tokenAAmount,
        tokenBAmount,
        tokenAAmountMin,
        tokenBAmountMin,
        address(this),
        block.timestamp + 1 // @audit timestamp issue again
      );

Also in the following lines in ReLPContract:

• https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L257C1-L265C7
• https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L277C1-L295C7

Impact

AMMs provide their users with an option to limit the execution of their pending actions, such as swaps or adding and removing liquidity. The most common solution is to include a deadline timestamp as a parameter (for example see Uniswap V2 and Uniswap V3). If such an option is not present, DEFAULT_ADMIN_ROLE can unknowingly perform bad trades, i made this example to illustrate the issue:

Alice wants to swap 100 tokens for 1 ETH and later sell the 1 ETH for 1000 DAI.

The transaction is submitted to the mempool, however, Alice chose a transaction fee that is too low for miners to be interested in including her transaction in a block. The transaction stays pending in the mempool for extended periods, which could be hours, days, weeks, or even longer.

When the average gas fee dropped far enough for Alice's transaction to become interesting again for miners to include it, her swap will be executed. In the meantime, the price of ETH could have drastically changed. She will still get 1 ETH but the DAI value of that output might be significantly lower.

She has unknowingly performed a bad trade due to the pending transaction she forgot about.

An even worse way this issue can be maliciously exploited is through MEV:

The swap transaction is still pending in the mempool. Average fees are still too high for miners to be interested in it.

The price of tokens has gone up significantly since the transaction was signed, meaning Alice would receive a lot more ETH when the swap is executed. But that also means that her maximum slippage value (sqrtPriceLimitX96 and minOut in terms of the Spell contracts) is outdated and would allow for significant slippage.

A MEV bot detects the pending transaction. Since the outdated maximum slippage value now allows for high slippage, the bot sandwiches Alice, resulting in significant profit for the bot and significant loss for Alice.

Tools Used

Manual review

Recommended Mitigation Steps

Fixed deadline as param, and also never set deadline to block.timestamp or type(uint256).max.

This article is very helpful to try and understand this issue.

Assessed type

MEV

[c4-pre-sort]

bytes032 marked the issue as duplicate of #2217

[c4-pre-sort]

bytes032 marked the issue as duplicate of #898

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)