Abundant Navy Kestrel
High
Setting the secondary position will not work properly, negative ticks are not handled properly.
Upon rebalancing, we create a secondary position which is supposed to handle the leftover of one of the tokens, the position should be created consisting just one of the tokens, as written in the comment above it:
/// @dev This position should always be created consisting of just one of the tokens
The function does not handle negative ticks properly and will lead to unexpected results, such as loss of funds for stakers and the protocol.
This is a part of the function:
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;
...
}
We calculate a modulo
, then the first check is when we have to handle token1
, thus the current tick must be >= the upper tick of the position for that to happen. This properly works when the tick is not negative, however if it is, the following would happen:
modulo = -5 % 10 = -5
(assuming tick equals -5 and tick spacing is 10)position.tickUpper = -5 - (-5) = -5 + 5 = 0
Now, when we compare the upper tick of the position and the current tick, we will see that the current tick is below the upper tick, thus liquidity will be provided in both tokens.
For the other if case, it still doesn't work properly but the impact is different. There, we provide the liquidity with the proper token, however we will incorrectly skip one of the ticks, thus our liquidity will be active at a much different price (e.g. our lower tick will be 10 instead of 0).
No external pre-conditions
- Tick is negative, completely normal and likely
No attack path necessary.
In the first case, we would provide no liquidity as we will add liquidity from both tokens when we only usually have 1, taking the lower ratio which is 0, our leftover token1 will stay idle, hence loss of funds as we won't generate fees using those tokens.
In the second case, we provide liquidity at an incorrect lower tick, which makes us miss out on fees as we will only generate fee when the price moves further, going over the skipped tick and its corresponding tick spacing (which might never happen, especially on more stable pairs), this causes loss of funds for users and the protocol.
No response
Handle a negative tick appropriately. If not mistaken, we simply have to get the absolute value of modulo
in all cases, however this should be tested as I didn't really put much effort to think it through.