#6 swap() deadline incorrectly set to block.timestamp

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L223

Summary

swap() function executes the ISwapRouter.ExactInputSingleParams() function with deadline as block.timestamp. This has basically no effect and should rather be an actual uint256 value.

    function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
        uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
        address inToken = getSwapAddressFor(_inToken);
        uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
                tokenIn: inToken,
                tokenOut: getSwapAddressFor(_outToken),
                fee: 3000,
                recipient: address(this),
@----->         deadline: block.timestamp,
                amountIn: _amount - swapFee,
                amountOutMinimum: minimumAmountOut,
                sqrtPriceLimitX96: 0
            });
        inToken == ISmartVaultManagerV3(manager).weth() ?
            executeNativeSwapAndFee(params, swapFee) :
            executeERC20SwapAndFee(params, swapFee);
    }

Since block.timestamp is always relative, using it in any way is equivalent to using no deadline at all. Needs to use a user defined input to effectively enforce any deadline.
Without a deadline, the transaction might be left hanging in the mempool and be executed way later than the user wanted. That could lead to user getting a worse price, because a validator can just hold onto the transaction. And when it does get around to putting the transaction in a block, it'll be block.timestamp, so they've got no protection there.

Vulnerability Details

This is a well-known vulnerability and here's an article detailing the impact and how Uniswap has a very clear deadline set, in addition to the minimumOut check - https://web.archive.org/web/20230525014603/https://blog.bytes032.xyz/p/why-you-should-stop-using-block-timestamp-as-deadline-in-swaps

A Simple Example To Explain The Exploit

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

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 token 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 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.

Impact

This could lead to users getting a worse price, because a validator can just hold onto the transaction. And when it does get around to putting the transaction in a block, it'll be block.timestamp (which is always relative), so they've got no protection there.

Tools Used

Manual inspection.

Recommendations

Add an additional param deadline inside the swap function and make use of that:

-   function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
+   function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount, uint256 deadline) external onlyOwner {
        uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
        address inToken = getSwapAddressFor(_inToken);
        uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
                tokenIn: inToken,
                tokenOut: getSwapAddressFor(_outToken),
                fee: 3000,
                recipient: address(this),
-               deadline: block.timestamp,
+               deadline: deadline,
                amountIn: _amount - swapFee,
                amountOutMinimum: minimumAmountOut,
                sqrtPriceLimitX96: 0
            });
        inToken == ISmartVaultManagerV3(manager).weth() ?
            executeNativeSwapAndFee(params, swapFee) :
            executeERC20SwapAndFee(params, swapFee);
    }