bughuntoor - Changing `positionWidth` while protocol is paused will lose most of the contract's funds

[sherlock-admin2]

bughuntoor

high

Changing positionWidth while protocol is paused will lose most of the contract's funds

Summary

Changing positionWidth while protocol is paused will lose most of the contract's funds

Vulnerability Detail

When the protocol's paused, all liquidity is removed from the pool up until it is unpaused. However, if during the pause positionWidth is changed, this will still add liquidity to the pool.

    function setPositionWidth(int24 _width) external onlyOwner {
        emit SetPositionWidth(positionWidth, _width);
        _claimEarnings();
        _removeLiquidity();
        positionWidth = _width;
        _setTicks();
        _addLiquidity();
    }

While this is a problem not only because funds are deposited during emergency mode, but also because then after some time, when unpause is called, _addLiquidity will be called again and will overwrite the nftId values, making old NFT ids inaccessible and forever lost.

    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
        });

        (uint256 nftId,,,) = INftPositionManager(nftManager).mint(mintParams);

        if (_mainPosition) positionMain.nftId = nftId;
        else positionAlt.nftId = nftId;

        IERC721(nftManager).approve(gauge, nftId);
    }

Since the two functions are callable by different wallets/ timelocks, a bad timing of transactions might cause this to happen. Furthermore, if timelock is used for the calling of these functions, a mismatch of transaction order might also cause this.

Same issue with moveTicks which is callable by Rebalancer

Impact

Loss of funds

Code Snippet

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

Tool used

Manual Review

Recommendation

If positionWidth is changed while contract is paused, do not add liquidity

[sherlock-admin2]

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

z3s commented:

_addLiquidity has _whenStrategyNotPaused(); check

DHTNS commented:

Invalid -> If the contract is paused all positions are burned so why would admin call setPositionWidth if there is no position to begin with?