#8 Swappers will continue to have unfavorable condition when swapping

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/c12272f2eec533019f2d255ab690f6892027f112/contracts/SmartVaultV3.sol#L214C5-L231C6

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

Summary

Advanced protocols like Automated Market Makers (AMMs) can allow users to specify a deadline parameter that enforces a time limit by which the transaction must be executed. Without a deadline parameter, the transaction may sit in the mempool and be executed at a much later time potentially resulting in a worse price for the user.

It's worth noting that the majority of the functions that interact with AMM (automated market maker) pools do not have a deadline parameter. However, there is one function that does pass the block.timestamp to a pool, which means that the transaction will be considered valid whenever a miner decides to include it in a block. This is because block.timestamp will reflect the current timestamp at the time of inclusion. It's important to keep this in mind when working with AMM pools.

Vulnerability Details

 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, <--- @audit front-running will occur here
                amountIn: _amount - swapFee,
                amountOutMinimum: minimumAmountOut,
                sqrtPriceLimitX96: 0
            });
        inToken == ISmartVaultManagerV3(manager).weth() ?
            executeNativeSwapAndFee(params, swapFee) :
            executeERC20SwapAndFee(params, swapFee);
    }

Proof of Concept

Alice, the owner of the contract, decides to execute a swap transaction using the swap function. She submits the transaction with the following parameters: _inToken: Token A _outToken: Token B _amount: 10 ETH The deadline is set to block.timestamp in ExactInputSingleParams() The transaction is broadcasted to the network but is still pending and not yet included in a block. Front-Running Attack: Eve, a malicious actor, monitors pending transactions and notices Alice's pending swap transaction.

Pending Transaction: The transaction is broadcasted to the network but is still pending and not yet included in a block. Front-Running Attack:

Eve, a malicious actor, monitors pending transactions and notices Alice's pending swap transaction. Eve quickly submits a similar transaction but with a higher gas price and adjusted parameters:

_inToken: Token A _outToken: Token B _amount: 10 ETH deadline: A value slightly higher than block.timestamp

Miners prioritize transactions with higher gas prices. Due to the higher gas price, miners select Eve's transaction for inclusion in the next block before Alice's transaction.

Execution of Malicious Transaction: Eve's transaction gets included in the block, executing the swap and possibly taking advantage of price changes that occurred while Alice's transaction was still pending.

Alice's Transaction Included Later: Eventually, Alice's transaction gets included in a subsequent block. However, by this time, the market conditions may have changed, resulting in less favorable exchange rates or higher slippage.

If the market price of Token A to Token B moves adversely between the time of Alice's transaction submission and its actual inclusion in a block, Alice might receive fewer Token B than expected.

Eve, by front-running Alice's transaction, might exploit market changes in her favor, executing the swap under more favorable conditions and potentially profiting at the expense of Alice.

In essence: Users will experience losses due to front-running, as malicious actors exploit price changes and execute trades more favorably before legitimate users' transactions are processed.

Impact

It is advisable not to set the deadline for protocols based on block.timestamp. This is because a validator can intentionally delay the transaction and eventually include it in a block with a timestamp that meets the deadline. As a result, relying solely on block.timestamp for deadline protection can be ineffective.

Tools Used

Manual review

Recommendations

Set a deadline parameter in the swap() and add block.timestamp to the deadline parameter or better still let the front-end handle that, since the one protocol team said there is noting on the front-end relating with the swap() function

 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, <--- @committed 
                amountIn: _amount - swapFee,
                amountOutMinimum: minimumAmountOut,
                sqrtPriceLimitX96: 0
            });
        inToken == ISmartVaultManagerV3(manager).weth() ?
            executeNativeSwapAndFee(params, swapFee) :
            executeERC20SwapAndFee(params, swapFee);
    }