nobody2018 - In IchiSpell._withdraw, it is invalid to use block.timestamp as deadline

[sherlock-admin2]

nobody2018

high

In IchiSpell._withdraw, it is invalid to use block.timestamp as deadline

Summary

deadline of IUniswapV3Router.ExactInputSingleParams is set to block.timestamp, which means that deadline will never expire. Since block.timestamp is always relative, using it in any way is equivalent to using no deadline at all.

Vulnerability Detail

File: blueberry-core\contracts\spell\IchiSpell.sol
229:             IUniswapV3Router.ExactInputSingleParams
230:                 memory params = IUniswapV3Router.ExactInputSingleParams({
231:                     tokenIn: swapPath[0],
232:                     tokenOut: swapPath[1],
233:                     fee: IUniswapV3Pool(vault.pool()).fee(),
234:                     recipient: address(this),
                         //@audit-issue invalid deadline, it should be from offchain.
235:->                   deadline: block.timestamp,
236:                     amountIn: amountIn,
237:                     amountOutMinimum: param.amountOutMin,
238:                     sqrtPriceLimitX96: 0
239:                 });
240: 
241:             _ensureApprove(params.tokenIn, address(uniV3Router), amountIn);
242:             uniV3Router.exactInputSingle(params);

the deadline check is set to block.timestamp, which means the deadline check is disabled! Because block.timestamp is obtained when tx is executed, such a deadline value will never expire.

Impact

AMMs provide their users with an option to limit the execution of their pending actions, such as swaps or adding and removing liquidity. The most common solution is to include a deadline timestamp as a parameter (for example see Uniswap V2 and Uniswap V3). If such an option is not present, users can unknowingly perform bad trades:

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

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 DAI 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 (sqrtPriceLimitX96 and minOut in terms of the Spell contracts) 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.

Code Snippet

https://github.com/sherlock-audit/2023-07-blueberry/blob/main/blueberry-core/contracts/spell/IchiSpell.sol#L235

Tool used

Manual Review

Recommendation

--- a/blueberry-core/contracts/spell/IchiSpell.sol
+++ b/blueberry-core/contracts/spell/IchiSpell.sol
@@ -196,7 +196,7 @@ contract IchiSpell is BasicSpell {
     ///         from the ICHI vault, swapping tokens, and repaying the debt.
     /// @param param Parameters required for the withdrawal operation.
     /// @dev param struct found in {BasicSpell}.
-    function _withdraw(ClosePosParam calldata param) internal {
+    function _withdraw(ClosePosParam calldata param, uint256 deadline) internal {
         Strategy memory strategy = strategies[param.strategyId];
         IICHIVault vault = IICHIVault(strategy.vault);

@@ -232,7 +232,7 @@ contract IchiSpell is BasicSpell {
                     tokenOut: swapPath[1],
                     fee: IUniswapV3Pool(vault.pool()).fee(),
                     recipient: address(this),
-                    deadline: block.timestamp,
+                    deadline: deadline,
                     amountIn: amountIn,
                     amountOutMinimum: param.amountOutMin,
                     sqrtPriceLimitX96: 0
@@ -279,7 +279,8 @@ contract IchiSpell is BasicSpell {
     /// @param param Parameters required for the close position operation.
     /// @dev param struct found in {BasicSpell}.
     function closePositionFarm(
-        ClosePosParam calldata param
+        ClosePosParam calldata param,
+        uint256 deadline
     )
         external
         existingStrategy(param.strategyId)
@@ -300,7 +301,7 @@ contract IchiSpell is BasicSpell {
         _doRefundRewards(ICHI);

         /// 2-8. Remove liquidity
-        _withdraw(param);
+        _withdraw(param, deadline);

         /// 9. Refund ichi token
         _doRefund(ICHI);

Duplicate of #14