EgisSecurity - StrategyPassiveManagerVelodrome.sol#_removeLiquidity() - The function has no slippage/deadline protection

[sherlock-admin2]

EgisSecurity

high

StrategyPassiveManagerVelodrome.sol#_removeLiquidity() - The function has no slippage/deadline protection

Summary

StrategyPassiveManagerVelodrome.sol#_removeLiquidity() - The function has no slippage/deadline protection

Vulnerability Detail

_removeLiquidity is used to remove all liquidity from the current main and/or alt positions, collect all the fees from the nftManager and burn the nft.

function _removeLiquidity() private {
        uint128 liquidity;
        uint128 liquidityAlt;
        if (positionMain.nftId != 0) {
            (,,,,,,,liquidity,,,,) = INftPositionManager(nftManager).positions(positionMain.nftId);
            ICLGauge(gauge).withdraw(positionMain.nftId);
        } 

        if (positionAlt.nftId != 0) {
            (,,,,,,,liquidityAlt,,,,) = INftPositionManager(nftManager).positions(positionAlt.nftId);
            ICLGauge(gauge).withdraw(positionAlt.nftId);
        }

        // init our params
        INftPositionManager.DecreaseLiquidityParams memory decreaseLiquidityParams;
        INftPositionManager.CollectParams memory collectParams;

        // If we have liquidity in the positions we remove it and collect our tokens.
        if (liquidity > 0) {
            decreaseLiquidityParams = INftPositionManager.DecreaseLiquidityParams({
                tokenId: positionMain.nftId,
                liquidity: liquidity,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            });

            collectParams = INftPositionManager.CollectParams({
                tokenId: positionMain.nftId,
                recipient: address(this),
                amount0Max: type(uint128).max,
                amount1Max: type(uint128).max
            });

            INftPositionManager(nftManager).decreaseLiquidity(decreaseLiquidityParams);
            INftPositionManager(nftManager).collect(collectParams);
            INftPositionManager(nftManager).burn(positionMain.nftId);
            positionMain.nftId = 0;
        }

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

            collectParams = INftPositionManager.CollectParams({
                tokenId: positionAlt.nftId,
                recipient: address(this),
                amount0Max: type(uint128).max,
                amount1Max: type(uint128).max
            });

            INftPositionManager(nftManager).decreaseLiquidity(decreaseLiquidityParams);
            INftPositionManager(nftManager).collect(collectParams);
            INftPositionManager(nftManager).burn(positionAlt.nftId);
            positionAlt.nftId = 0;

        }
    }

You can see that when calling nftManager.decreaseLiquidity, we build a DecreaseLiquidityParams struct that is then passed to the function.

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

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

You can see that for both the main and alt positions, there is 0 slippage and no deadline (passing block.timestamp as deadline means: "There is no deadline on this tx")

This introduces opportunities for sandwich attacks which allows malicious actors to steal large amounts of the funds, when decreaseLiquidity is called.

This is especially prevalent when panic and withdraw (when the strategy is paused).

Since the onlyCalmPeriods modifier somewhat defends from sandwich attacks, in these 2 cases the modifier isn't called, which increases the chances of lost funds tremendously.

Impact

Sandwich attacks which will cause loss of funds for the protocol and it's users.

Code Snippet

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

Tool used

Manual Review

Recommendation

Add slippage and a deadline when calling nftManager.decreaseLiquidity

Duplicate of #8

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

WildSniper commented:

invalid due to impossibility of such attack with tick lower and upper as params