Incorrect modulo calculation in secondary position ticks leads to active position and division by zero

Summary

The Strategy#_setSecondaryPositionsTicks function incorrectly handles negative ticks when calculating modulo, causing the secondary position to be active when it should be out-of-range and potentially leading to division by zero in LiquidityAmounts#getLiquidityForAmounts.

Relevant Context

The strategy maintains two Uniswap V3 positions:

1. A main position that should be balanced (50:50)
2. A secondary position that should always be out-of-range and filled with only one token

The secondary position's ticks are set based on the current tick and token balances to ensure it remains out-of-range. The main position's ticks are calculated with proper handling of negative modulos, but this correction is missing in the secondary position calculation.

_setMainTicks

    /// @notice Calculates and sets the ticks of the main position
    /// @dev Checks if the nearest initializable tick is lower or higher than the current tick
    function _setMainTicks(int24 tick) internal {
        int24 halfWidth = int24(positionWidth / 2);
        int24 modulo = tick % tickSpacing;
        if (modulo < 0) modulo += tickSpacing; // if tick is negative, modulo is also negative
        bool isLowerSided = modulo < (tickSpacing / 2);

        int24 tickBorder = tick - modulo;
        if (!isLowerSided) tickBorder += tickSpacing;
        mainPosition.tickLower = tickBorder - halfWidth;
        mainPosition.tickUpper = tickBorder + halfWidth;

        emit NewMainTicks(tickBorder - halfWidth, tickBorder + halfWidth);
    }

_setSecondaryPositionsTicks

    /// @notice Sets the secondary position ticks.
    /// @dev This position should always be created consisting of just one of the tokens
    /// @dev Should initially be Out-Of-Range
    function _setSecondaryPositionsTicks(int24 tick) internal {
        int24 modulo = tick % tickSpacing;
        uint256 bal0 = IERC20(token0).balanceOf(address(this));
        uint256 bal1 = IERC20(token1).balanceOf(address(this));
        uint256 _price = price();
        uint256 bal0in1 = bal0 * _price / PRECISION; // usually either of them should be 0, but might be non-zero due to rounding when minting

        if (bal0in1 < bal1) {
            secondaryPosition.tickLower = mainPosition.tickLower;
            secondaryPosition.tickUpper = tick - modulo;
        } else {
            secondaryPosition.tickLower = tick - modulo + tickSpacing;
            secondaryPosition.tickUpper = mainPosition.tickUpper; // TODO check if these need to be reversed.
        }

        emit NewSecondaryTicks(secondaryPosition.tickLower, secondaryPosition.tickUpper);
    }

Root Cause

In Strategy#_setSecondaryPositionsTicks, the modulo calculation for negative ticks is incorrect:

Strategy.sol#L367

int24 modulo = tick % tickSpacing;

Unlike Strategy#_setMainTicks which handles negative modulos correctly:

Strategy.sol#L235

if (modulo < 0) modulo += tickSpacing;

This leads to two issues:

1. When bal0in1 < bal1, the secondary position becomes active because tick < tickUpper = tick - modulo
2. When bal0in1 >= bal1, the secondary position range is shorter than intended because tick - (tickSpacing + modulo) + tickSpacing < tick - modulo + tickSpacing

Additionally, when positionWidth = 4 * tickSpacing and isLowerSided is true, tickLower equals tickUpper, causing a division by zero in LiquidityAmounts#getLiquidityForAmounts.

Attack Path

1. Initial conditions:

• positionWidth = 4 * tickSpacing
• Current tick is negative
• bal0in1 >= bal1
• Current tick results in isLowerSided = true in main position

2. When rebalance() is called:

• _setSecondaryPositionsTicks calculates incorrect modulo
• Sets tickLower = tick - modulo + tickSpacing
• Sets tickUpper = mainPosition.tickUpper
• Due to incorrect modulo, tickLower = tickUpper

3. _addLiquidityToSecondaryPosition attempts to add liquidity:

• Calls LiquidityAmounts#getLiquidityForAmounts
• Division by zero occurs due to equal ticks
• Transaction reverts

Impact Explanation

High. The vulnerability has two severe impacts:

1. Secondary position becomes active when it should be out-of-range, breaking a core invariant of the strategy
2. Strategy becomes unusable when specific conditions are met due to division by zero

Proof of Concept

Case 1: bal0in1 < bal1

Consider:

1. tickSpacing = 60
2. Current tick = -121
3. modulo = -121 % 60 = -1 (incorrect)
4. bal0in1 < bal1

Calculations:

• secondaryPosition.tickLower = mainPosition.tickLower
• secondaryPosition.tickUpper = -121 - (-1) = -120
• Current tick (-121) is less than tickUpper (-120)
• Result: Secondary position becomes active when it should be out-of-range

With correct modulo handling:

• modulo = -1 + 60 = 59
• secondaryPosition.tickUpper = -121 - 59 = -180
• Current tick (-121) would be greater than tickUpper (-180)
• Result: Secondary position remains out-of-range as intended

Case 2: bal0in1 >= bal1 (with isLowerSided = true)

Consider:

1. tickSpacing = 60
2. Current tick = -91
3. positionWidth = 240 (4 * tickSpacing)
4. modulo = -91 % 60 = -31 (incorrect)
5. bal0in1 >= bal1

Main position calculations:

• modulo = -31 + 60 = 29 (corrected in _setMainTicks)
• isLowerSided = 29 < (60/2) = true
• tickBorder = -91 - 29 = -120
• mainPosition.tickUpper = -120 + 120 = 0

Secondary position calculations with incorrect modulo:

• tickLower = -91 - (-31) + 60 = 0
• tickUpper = mainPosition.tickUpper = 0
• Result: tickLower = tickUpper = 0, causing division by zero

With correct modulo handling:

• modulo = -31 + 60 = 29
• tickLower = -91 - 29 + 60 = -60
• tickUpper = mainPosition.tickUpper = 0
• Result: Valid position range (-60, 0)

Recommendation

Add the same modulo correction as in _setMainTicks:

function _setSecondaryPositionsTicks(int24 tick) internal {
    int24 modulo = tick % tickSpacing;
+   if (modulo < 0) modulo += tickSpacing;
    
    uint256 bal0 = IERC20(token0).balanceOf(address(this));
    uint256 bal1 = IERC20(token1).balanceOf(address(this));
    uint256 _price = price();
    uint256 bal0in1 = bal0 * _price / PRECISION;

    if (bal0in1 < bal1) {
        secondaryPosition.tickLower = mainPosition.tickLower;
        secondaryPosition.tickUpper = tick - modulo;
    } else {
        secondaryPosition.tickLower = tick - modulo + tickSpacing;
        secondaryPosition.tickUpper = mainPosition.tickUpper;
    }

    emit NewSecondaryTicks(secondaryPosition.tickLower, secondaryPosition.tickUpper);
}