EgisSecurity - `maxDevation` for some pools can be at most 3, which may be easily weaponized

[sherlock-admin2]

EgisSecurity

high

maxDevation for some pools can be at most 3, which may be easily weaponized

Summary

maxDeviation value is set so calmPeriod is checked on important actions such as deposit, withdraw and _setTicks. It protects against pool price manipulations using the current price, twap for a period and decide whether price has been manipulated by comparing it to the price deviation:

    function isCalm() public view returns (bool) { 
        int24 tick = currentTick();                
        int56 twapTick = twap();

        int56 minCalmTick = int56(SignedMath.max(twapTick - maxTickDeviation, MIN_TICK));
        int56 maxCalmTick = int56(SignedMath.min(twapTick + maxTickDeviation, MAX_TICK));

        // Calculate if tick move more than allowed from twap and revert if it did. 
        if(minCalmTick > tick  || maxCalmTick < tick) return false;
        else return true;
    }

The problem here is that if we are overprotective, a path for new vulnerabilities is uncovered. Here this is the case.

Vulnerability Detail

If we check veldrome pools and their liquidity, we see that second and third most active pools are USDC/sUSD and USDC/USDC . We assume that those pools are of interest to the protocol, because they are from the most active pools. Both has a fee of 0.01% and a tickSpacing = 1, which means that protocol can set maxTickDeviation to at most 3, because of the following lines:

    function setDeviation(int56 _maxDeviation) external onlyOwner {
        emit SetDeviation(_maxDeviation);

        // Require the deviation to be less than or equal to 4 times the tick spacing.
        if (_maxDeviation >= _tickDistance() * 4) revert InvalidInput();

        maxTickDeviation = _maxDeviation;
    }

Here we can also notice that the comment is wrong by claiming that deviation could be equal to 4 times tick spacing, when the reality is that it can be at most 3.

This is a very tight range, which can easily be moved even for stablecoin pairs without opening arbitrage oppertunities, because it is capital neglectable, but such action would cause important strategy actions to revert.

We can provide the following real values:

• For pool USDC/sUSD we have tick = 276339, which corresponds to price of 1.001498402659
• An attacker can make a large between the tokens and move the tick with just 4
• Tick becomes 276343, which corresponds to a value of 1.001899062114, which is so neglectable that nobody has an interest of "rebalancing" back the price.
• When twap reaches the new tick, the attacker can do the reverse swap and return tick with another 4 and so on.
• This a DoS path, which requires only small capital from the attack (just enough to move the price with 4 ticks). Also, he doesn't lose it and can do it over and over again. He can delay/DoS different user actions such as deposit/moveTicks/_setTicks/setPositionWidth/unpause , which checks if the pool is calm. Also rebalancer actions moveTicks may be DoSed. Those are crucial and may result in user losing staked tokens, if current tick is not favourable. Malicious actor can DoS moving ticks and when the range has moved, he can call withdraw, which doesn't check the calm period, but withdraws all funds and then again minting a position with old tick params (which result in minting a position with zero liquidity, because there is also no slippage check on minAmountOut when trying to mint a position)

  Impact

  Temporary DoS, which can develop into funds loss for protocol and users

  Code Snippet

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

  Tool used

Manual Review

Recommendation

Make maxDeviation limit larger.

Duplicate of #58