Lines of code

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/settlements/UniswapSettlement.sol#L50 https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/settlements/UniswapSettlement.sol#L34

Vulnerability details

Impact

Loss of asset to the user when a validator holds a swap transaction indefinately till it incurs maximum slippage loss.

Proof of Concept

The major issue is that block.timestamp is dynamic and depends on any time of execution. Meanwhile the deadline parameter expected by the Uniswap swap function is the unix timestamp after which a swap will fail, to protect against long-pending swap transaction.

According to Uniswap docs:

deadline: the unix time after which a swap will fail, to protect against long-pending transactions and wild swings in prices

The swapExactOut(...) and swapExactIn(...) functions uses block.timestamp as deadline for Uniswap swaps.

File: UniswapSettlement.sol
function swapExactOut(
        address quoteToken,
        address,
        bytes memory data,
        uint256 amountOut,
        uint256 amountInMaximum,
        address recipient
    ) external override returns (uint256 amountIn) {
        ERC20(quoteToken).safeTransferFrom(msg.sender, address(this), amountInMaximum);
        ERC20(quoteToken).approve(address(_swapRouter), amountInMaximum);

        amountIn = _swapRouter.exactOutput(
@>            ISwapRouter.ExactOutputParams(data, recipient, block.timestamp, amountOut, amountInMaximum)
        );

        if (amountInMaximum > amountIn) {
            ERC20(quoteToken).safeTransfer(msg.sender, amountInMaximum - amountIn);
        }
    }

Using block.timestamp as deadline will allow malicious validators to hold a transaction indefinately till it incurs maximum slippage.

Passing block.timestamp as deadline will pass the modifier validation below even after 48 hours that the transaction is submitted.

Below is the modifier used by Uniswap to validate the deadline parameter to the Uniswap swap(...) function.

Link: 
modifier checkDeadline(uint256 deadline) {
        require(_blockTimestamp() <= deadline, 'Transaction too old');
        _;
    }

With the above checkDeadline modifier, passing block.timestamp as deadline will make this validation useless.

Consider allowing users pass unix timestamp literal as deadline parameter. It can be easily implemented on the frontend by getting the current timestamp and adding 20 minutes to it before sending the transaction seperately. This way, the transaction will revert if it exceeds 20 minutes before it is executed.

Exploit Scenario

Bob submits a transaction to swapExactOut function with amountInMaximum
Alice is a malicious validator
Alice holds Bobs transaction till it is profitable for sandwich attack based on the amountInMaximum specified.
Alice frontruns and backruns Bobs swap to ensure all the amountInMaximum is used up while she profits from it.
This way, Bob suffers slippage loss and only gets zero token refund on the last line of code in the swapExactOut(...) function.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider allowing the user pass a timestamp literal value as input parameter.

function swapExactOut(
        address quoteToken,
        address,
        bytes memory data,
        uint256 amountOut,
        uint256 amountInMaximum,
        address recipient,
++      uint256 deadline
    ) external override returns (uint256 amountIn) {
        ERC20(quoteToken).safeTransferFrom(msg.sender, address(this), amountInMaximum);
        ERC20(quoteToken).approve(address(_swapRouter), amountInMaximum);

        amountIn = _swapRouter.exactOutput(
--            ISwapRouter.ExactOutputParams(data, recipient, block.timestamp, amountOut, amountInMaximum)
++            ISwapRouter.ExactOutputParams(data, recipient, deadline, amountOut, amountInMaximum)
        );

        if (amountInMaximum > amountIn) {
            ERC20(quoteToken).safeTransfer(msg.sender, amountInMaximum - amountIn);
        }
    }

Assessed type

Uniswap

code-423n4 / 2024-05-predy-findings

Using `block.timestamp` as `deadline` for swap allows the transaction to be held indefinitely till it incurs maximum slippage loss #219