Dangerous use of deadline parameter

Summary

The protocol is using block.timestamp as the deadline argument while interacting with the Velodrome NFT Position Manager, which completely defeats the purpose of using a deadline.

Vulnerability Detail

Most of the functions that interact with AMM pools do not have a deadline parameter, but specifically the shown below is passing block.timestamp to a pool, which means that whenever the miner decides to include the txn in a block, it will be valid at that time, since block.timestamp will be the current timestamp.

  function _mintPosition(int24 _tickLower, int24 _tickUpper, uint256 _amount0, uint256 _amount1, bool _mainPosition) private {
   INftPositionManager.MintParams memory mintParams = INftPositionManager.MintParams({
            token0: lpToken0,
            token1: lpToken1,
            tickSpacing: _tickDistance(),
            tickLower: _tickLower,
            tickUpper: _tickUpper,
            amount0Desired: _amount0,
            amount1Desired: _amount1,
            amount0Min: 0,
            amount1Min: 0,
            recipient: address(this),
            deadline: block.timestamp,
            sqrtPriceX96: 0
        });

   ...//
  }

   function _removeLiquidity() private {

        if (liquidity > 0) {
            decreaseLiquidityParams = INftPositionManager.DecreaseLiquidityParams({
                tokenId: positionMain.nftId,
                liquidity: liquidity,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            });

          }
        ...//

          if (liquidityAlt > 0) {
            decreaseLiquidityParams = INftPositionManager.DecreaseLiquidityParams({
                tokenId: positionAlt.nftId,
                liquidity: liquidityAlt,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            });

           }

         ...//

   }

Front-running is a key aspect of AMM design, deadline is a useful tool to ensure that your tx cannot be “saved for later”. Due to the removal of the check, it may be more profitable for a miner to deny the transaction from being mined until the transaction incurs the maximum amount of slippage.

Impact

Failure to provide a proper deadline value enables pending transactions to be maliciously executed at a later point. Transactions that provide an insufficient amount of gas such that they are not mined within a reasonable amount of time, can be picked by malicious actors or MEV bots and executed later in detriment of the submitter.

Code Snippet

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L317

Tool used

Manual Review

Recommendation

Add a deadline parameter to the functions that are used to manage the liquidity position. Forward this parameter to the corresponding underlying calls to the StrategyPassiveManagerVelodrome contract.

Duplicate of #130

sherlock-audit / 2024-05-beefy-cowcentrated-liquidity-manager-judging

Naresh - Dangerous use of deadline parameter #66