code-423n4 / 2023-05-maia-findings

20 stars 12 forks source link

Calculation during rebalancing can overflow #892

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolVariables.sol#L231-L252

Vulnerability details

Proof of Concept

Rebalancing logic in TalosBaseStrategy will start by the strategy manager calling TalosBaseStrategy.rebalance() to swap imbalanced tokens. This function will call TalosStrategySimple.doRebalance()

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L317

Next, PoolActions.swapEqualAmounts() will be called.

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/strategies/TalosStrategySimple.sol#L42

Finally, PoolVariables.getSwapToEqualAmountsParams() will be called to calculate the amounts base of ticks from the uniswap v3 pool.

(bool zeroForOne, int256 amountSpecified, uint160 sqrtPriceLimitX96) = actionParams
    .pool
    .getSwapToEqualAmountsParams(
    actionParams.optimizer, actionParams.tickSpacing, baseThreshold, actionParams.token0, actionParams.token1
);

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L41

The issue is that sqrtPriceX96 in PoolVariables.getSwapToEqualAmountsParams() can overflow.

(uint160 sqrtPriceX96, int24 currentTick,,,,,) = _pool.slot0();

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolVariables.sol#L231-L252

We can see that Uniswap getQuoteAtTicks() handles the case for sqrtRatioX96 <= type(uint128).max.

Impact

Aside from sqrtPriceX96 being subject to overflow, it's also possible for sqrtPriceX96 to be smaller than 128 bits but to exactSqrtPriceImpact to overflow since it's being multiplied by _strategy.priceImpactPercentage() and the resulting value will be incremented or decremented depending on zeroForOne.

uint160 exactSqrtPriceImpact = (sqrtPriceX96 * (_strategy.priceImpactPercentage() / 2)) / GLOBAL_DIVISIONER;
sqrtPriceLimitX96 = zeroForOne ? sqrtPriceX96 - exactSqrtPriceImpact : sqrtPriceX96 + exactSqrtPriceImpact;

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolVariables.sol#L251-L252

On either case, the value returned by PoolVariables.getSwapToEqualAmountsParams() will be wrong and strategies might be rebalanced incorrectly since an incorrect sqrtPriceLimitX96 would be passed to pool.swap.

actionParams.pool.swap(
    address(this),
    zeroForOne,
    amountSpecified,
    sqrtPriceLimitX96,
    abi.encode(SwapCallbackData({zeroForOne: zeroForOne}))
);

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L46-L52

Tools Used

Manual review.

Recommended Mitigation Steps

Account for potential overflow in sqrtPriceX96 and exactSqrtPriceImpact inside PoolVariables.getSwapToEqualAmountsParams().

Assessed type

Under/Overflow

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Insufficient proof