Deadline is missing in update_position

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/SeawaterAMM.sol#L262-L278 https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/SeawaterAMM.sol#L464-L484

Vulnerability details

Impact

There is no deadline available when position is being modified, which can harm LPs due to the sharp price movements before their transaction gets included.

Proof of Concept

Both increase/decrease functions are lacking deadlines:

SeawaterAMM.sol

function incrPositionC3AC7CAA(
        address /* pool */,
        uint256 /* id */,
        uint256 /* amount0Min */,
        uint256 /* amount1Min */,
        uint256 /* amount0Desired */,
        uint256 /* amount1Desired */
    ) external returns (uint256, uint256) {
        directDelegate(_getExecutorUpdatePosition());
    }

    /// @inheritdoc ISeawaterExecutorUpdatePosition
    function decrPosition09293696(
        uint256 /* id */,
        uint256 /* amount0Min */,
        uint256 /* amount1Min */,
        uint256 /* amount0Max */,
        uint256 /* amount1Max */
    ) external returns (uint256, uint256) {
        directDelegate(_getExecutorUpdatePosition());
    }

Missing deadline can harm positions because the amount that they will provide when increasing or receive when decreasing can fluctuate a lot between the moment they initiate the transaction and it get included in the block. In fact, how much tokens will be needed is defined by the tick of the pool - here, and the tick itself moves either to the left or right representing the price curve. Position, created with tight tick ranges will be harmed by the lack of deadline because the moment their tx gets executed the tick can be in completely different place, out of their range which will automatically make them to not accumulate any fees.

Also all of the swap functions are missing deadlines as well:

SeawaterAMM.sol

function swapIn32502CA71(address token, uint256 amountIn, uint256 minOut) external returns (int256, int256) {
        (bool success, bytes memory data) = _getExecutorSwap().delegatecall(abi.encodeCall(
            ISeawaterExecutorSwap.swap904369BE,
            (
                token,
                true,
                int256(amountIn),
                type(uint256).max
            )
        ));
        require(success, string(data));

        (int256 swapAmountIn, int256 swapAmountOut) = abi.decode(data, (int256, int256));
        // this contract uses checked arithmetic, this negate can revert
        require(-swapAmountOut >= int256(minOut), "min out not reached!");
        return (swapAmountIn, swapAmountOut);
    }

function swapOut5E08A399(address token, uint256 amountIn, uint256 minOut) external returns (int256, int256) {
        (bool success, bytes memory data) = _getExecutorSwap().delegatecall(abi.encodeCall(
            ISeawaterExecutorSwap.swap904369BE,
            (
                token,
                false,
                int256(amountIn),
                type(uint256).max
            )
        ));
        require(success, string(data));

        (int256 swapAmountIn, int256 swapAmountOut) = abi.decode(data, (int256, int256));
        require(swapAmountOut >= int256(minOut), "min out not reached!");
        return (swapAmountIn, swapAmountOut);
    }

function swap904369BE(address /* pool */, bool /* zeroForOne */, int256 /* amount */, uint256 /* priceLimit */) external returns (int256, int256) {
        directDelegate(_getExecutorSwap());
    }

Only permit functions expose deadlines but it’s not effective for the swap and is used only for checking the signature validity

Missing deadline in swaps will harm users, exposing them to MEV, because their slippage params will be outdated at the time of execution. Transactions that are not being processed for long time will render the slippage ineffective because price can increase but the minOut will be based on the old price, making it vulnerable to sandwich attacks.

For example current rate of ETH/fUSDC is $3000, Alice wants 1 ETH and performs exactIn swap with 3000 fUSDC and minOut 0.99 ETH, due to missing deadline transaction is not processed for some time and rate increases to 1.05 for 3000 fUSDC, bot sees the transaction and sandwich it because Alice’s slippage tolerance is outdated allowing that to happen.

UniswapV3 has deadline and enforces users for both LP position management and swaps:

NFPM.sol

function increaseLiquidity(IncreaseLiquidityParams calldata params)
      external
      payable
      override
      checkDeadline(params.deadline)
      returns (
          uint128 liquidity,
          uint256 amount0,
          uint256 amount1
      )

SwapRouter.sol

function exactInputSingle(ExactInputSingleParams calldata params)
        external
        payable
        override
        checkDeadline(params.deadline)
        returns (uint256 amountOut)
    {

Tools Used

Manual Review

Recommended Mitigation Steps

Add deadline params for both LP and swap functionalities, in order to protect users from MEV.

Assessed type

Uniswap

[blckhv]

Hello @alex-ppg,

Reading the validator's answer:

"Invalid. update position is a core function, for slippage protection you should use periphery functions such as increase or decrease position"

I think he has a slight misunderstanding of the issue, probably our title is not the best, but the rest of the sections explain it well enough. We explain exactly that both increase and decrease position, which are periphery functions, lack deadline checks and how this will affect the slippage as well because of the prolonged time without tx being executed.

Thank you for revisiting this report.

[alex-ppg]

Hey @blckhv, thank you for your PJQA contribution. I would like to clarify that validation repository findings are not directly evaluated by the judge of a contest if they do not pass the validation phase.

The system will still honor the minimum output (and general minimum/maximum limitations enforced by the user) meaning that a deadline is not a crucial security feature.

[blckhv]

Hey @alex-ppg, thanks for your response,

I would like to re-clarify that the problem will not happen when the price of the asset is being decreasing, but when it increases, this will make the min out value specified stale and will open MEV for that swap. That's the exact purpose deadline to be used in all the AMMs, it protects the users from being sandwiched when the price of the asset has increased while their transaction was pending.

Here is great reference finding that explains how the slippage will become ineffective, exactly as we do in our report: https://github.com/code-423n4/2022-12-backed-findings/issues/64

Thanks!

[alex-ppg]

Hey @blckhv, thanks for following up. The judge noted that they are on the fence between an L and M rating. For this vulnerability to take effect, a pending transaction of a user with a significantly outdated minimum output would have to be present. This means the user has not performed another interaction with the blockchain even though their original transaction was pending and they expressed their desire to perform the swap.

The likelihood of such an event is extremely unlikely, and even so, the user already agreed to a minimum output they are happy with. As such, I do not consider this to merit an HM severity rating.