merlin - The `isCalm()` function incorrectly checks whether the current price is within a certain deviation

[sherlock-admin2]

merlin

medium

The isCalm() function incorrectly checks whether the current price is within a certain deviation

Summary

The StrategyPassiveManagerVelodrome.twap() function does not round to negative infinity for negative ticks, which affects the calculation of minCalmTick and maxCalmTick in isCalm().

Vulnerability Detail

The problem is that in case if (tickCuml[1] - tickCuml[0]) is negative, then the tick should be rounded down as it's done in the uniswap library. In this case returned tick will be bigger then it should be.

(int56[] memory tickCuml,) = IVeloPool(pool).observe(secondsAgo);
twapTick = (tickCuml[1] - tickCuml[0]) / int32(twapInterval);

The twapTick will be larger, which will affect the calculation of minCalmTick and maxCalmTick in isCalm(), leading to transactions failing:

        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;

Impact

All function calls that have the onlyCalmPeriods modifier will fail due to the incorrect check for whether the current price is within a certain deviation.

Code Snippet

contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L747

Tool used

Manual Review

Recommendation

Consider modifying the twap function as follows:

function twap() public view returns (int56 twapTick) {
        uint32[] memory secondsAgo = new uint32[](2);
        secondsAgo[0] = uint32(twapInterval);
        secondsAgo[1] = 0;

        (int56[] memory tickCuml,) = IVeloPool(pool).observe(secondsAgo);
        twapTick = (tickCuml[1] - tickCuml[0]) / int32(twapInterval);
+     if ((tickCuml[1] - tickCuml[0]) < 0 && ((tickCuml[1] - tickCuml[0]) % twapInterval != 0)) twapTick--;        
    }

[sherlock-admin3]

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

WildSniper commented:

borderLine medium low

WildSniper commented:

seems like optimizing the protocol other than a valid bug and taking into consideration how the main position is two sided not one sided

[z3s]

How to identify a medium issue:

1. Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.
2. Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

I don't think so; it reaches the medium severity requirements.