Breeje - Deadline check is still not effective, allowing outdated slippage and pending transaction to be unexpected executed later

[sherlock-admin2]

Breeje

high

Deadline check is still not effective, allowing outdated slippage and pending transaction to be unexpected executed later

Summary

Referencing to the issue in Previous Audit: here, where sponsor confirmed it and marked it Will Fix. However, the attempted solution remains inadequate, as it relies on the utilization of block.timestamp instead of the user-provided deadline. Consequently, the vulnerability highlighted in the finding continues to pose a potential risk within the existing contract.

Vulnerability Detail

IchiSpell uses Uniswap V3 Router in _withdraw function to Swap withdrawn tokens to debt token.

File: IchiSpell.sol

    IUniswapV3Router.ExactInputSingleParams
    memory params = IUniswapV3Router.ExactInputSingleParams({
        tokenIn: swapPath[0],
        tokenOut: swapPath[1],
        fee: IUniswapV3Pool(vault.pool()).fee(),
        recipient: address(this),
        deadline: block.timestamp, // @audit-issue Deadline not effective
        amountIn: amountIn,
        amountOutMinimum: param.amountOutMin,
        sqrtPriceLimitX96: 0
    });

Link to Code

But here, the deadline parameter is simply passed in as current block.timestamp in which transaction occurs. This effectively means that transaction has no deadline, which means that the transaction may be included anytime by validators and remain pending in mempool, potentially exposing users to sandwich attacks by attackers or MEV bots.

An even worse way this issue can be maliciously exploited is through MEV:

The transaction is still pending in the mempool. Average fees are still too high for validators to be interested in it. The price of ETH has gone up significantly since the transaction was signed, meaning Alice would receive a lot more when the trade is executed.

Even Lead Watson 0x52 rightly mentioned:

This fix does not address this issue. 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.

Impact

Stale value of slippage allows Sandwich attacks causing users to lose assets

Code Snippet

Shown above

Tool used

Manual Review

Recommendation

Use a user supplied deadline here in place of block.timestamp.

Duplicate of #14