Missing deadline checks allow pending transactions to be maliciously executed

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-numoen/blob/main/src/core/Pair.sol#L116-L140

Vulnerability details

Summary

The Pair contract does not allow users to submit a deadline for their action. This missing feature enables pending transactions to be maliciously executed at a later point.

Proof of concept

AMMs should provide their users with an option to include a deadline for their pending actions such as swapping. The most common solution is to include a deadline timestamp as a parameter (for example see Uniswap V2). If such an option is not available, users can unknowingly perform bad trades:

function swap(address to, uint256 amount0Out, uint256 amount1Out, bytes calldata data) external override nonReentrant {
    if (amount0Out == 0 && amount1Out == 0) revert InsufficientOutputError();

    uint120 _reserve0 = reserve0; // SLOAD
    uint120 _reserve1 = reserve1; // SLOAD

    if (amount0Out > 0) SafeTransferLib.safeTransfer(token0, to, amount0Out);
    if (amount1Out > 0) SafeTransferLib.safeTransfer(token1, to, amount1Out);

    uint256 balance0Before = Balance.balance(token0);
    uint256 balance1Before = Balance.balance(token1);
    ISwapCallback(msg.sender).swapCallback(amount0Out, amount1Out, data);
    uint256 amount0In = Balance.balance(token0) - balance0Before;
    uint256 amount1In = Balance.balance(token1) - balance1Before;

    if (!invariant(_reserve0 + amount0In - amount0Out, _reserve1 + amount1In - amount1Out, totalLiquidity)) {
      revert InvariantError();
    }

    reserve0 = _reserve0 + SafeCast.toUint120(amount0In) - SafeCast.toUint120(amount0Out); // SSTORE
    reserve1 = _reserve1 + SafeCast.toUint120(amount1In) - SafeCast.toUint120(amount1Out); // SSTORE

    emit Swap(amount0Out, amount1Out, amount0In, amount1In, to);
  }
}

1. Alice wants to swap 100 Power Token for 1 WETH. She signs the transaction calling Pair.swap with to = 0xAlice , amount0Out = 100 and amount1Out = 0.
2. The transaction is submitted into the mempool, however, Alice chose a lower transaction fee which the miners are not interested at the moment. Hence, the transaction is not included in the block and remains pending for a long time — hours, days, weeks or even longer.
3. Alice’s transaction will be included in the block once the average gas fees fell low enough such that the miners include her transaction. However the price of both Power Token and WETH might have substantially changed overtime. She still receives WETH but may not favour what she originally wanted to swap. She has unknowingly performed a bad trade due to the pending transaction she forgot about.

Impact

Omitted in this case, since the exploit is solely based on the fact that there is no limit on how long a transaction is allowed to be pending, which can be clearly seen when looking at the mentioned functions.

How to fix

Introduce a deadline parameter to the mentioned functions. This is implemented in other functions such as LiquidityManager.sol#L135 and LiquidityManager.sol#L201 with the checkDeadline modifier at LiquidityManager.sol#L83-86.

[c4-judge]

berndartmueller marked the issue as primary issue

[c4-sponsor]

kyscott18 marked the issue as sponsor acknowledged

[kyscott18]

This function in the core contract is not meant to be called by an end user and actually must be called by a contract because a callback is called. Since swapping is going to be done by arbitrageurs, we expect them to write their own contracts with their own slippage and staleness logic.

[c4-judge]

berndartmueller changed the severity to 2 (Med Risk)

[c4-judge]

berndartmueller marked the issue as selected for report

[berndartmueller]

I'm revising my decision and consider this finding as QA (Low). As the sponsor noted in https://github.com/code-423n4/2023-01-numoen-findings/issues/160#issuecomment-1423070899, this swap function has to be called by a contract. Thus I see the responsibility of ensuring proper and safe usage of the swap function at the arbitrageurs' end.

[c4-judge]

berndartmueller changed the severity to QA (Quality Assurance)

[c4-judge]

berndartmueller marked the issue as grade-c