sherlock-audit / 2023-10-aloe-judging

9 stars 6 forks source link

Chinmay - Position cannot be modified when probe price a rounds down to MIN_SQRT_RATIO #143

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

Chinmay

high

Position cannot be modified when probe price a rounds down to MIN_SQRT_RATIO

Summary

The probe price calculation for prices.a can round down to zero and then get capped by the MIN_SQRT_RATIO, which can lead to a failing health check preventing the modification of a user's position in some cases.

Vulnerability Detail

a = uint160((sqrtMeanPriceX96 * 1e12).rawDiv(sqrtScaler).max(TickMath.MIN_SQRT_RATIO)); : this calculation can go to zero if sqrtScaler has its maximum value and a gets capped to MIN_SQRT_RATIO. In other cases, the value of a may simply be below MIN_SQRT_RATIO and get capped to it. If a has such a small value it means price of token0 in terms of token1 will be very small. Now check a call to modify position. After the user has done his stuff, it calls for a check to ensure the User's borrower account is healthy ie. total assets > liabilities for both probe prices a and b. Now since the probe price can get a value very small, the health check can return false when the modified position is actually healthy.

Now consider a position which has all its liabilities in token1 and most assets as token0. When the logic check enters the BalanceSheet.sol#isHealthy function , probe price a may have been incorrectly rounded down and passed on to this function. On BalanceSheet.sol Line 71, the liabilities and assets are valued at priceX128 which is the probe price a. Since the position we have assumed has liabilities0 as minimum and assets0 as maximum, the total liabilities calculation will be correct but the total assets will be grossly undervalued than the deserving probe price it should have been evaluated for. This may result in liabilities > assets => return false, thus failing the health check and reverting the modify call for no mistake of the user.

Impact

User's call to modify has been blocked for no mistake of theirs. This modify procedure maybe important for the user, especially if he has been warned for liquidation or wants to withdraw liquidity. This is unfair to the user as he may get liquidated then. The rounding down is definitely possible because sqrtScaler has max value upto 3.07e12 and sqrtMeanPriceX96 can be small depending on asset pair.

Important note : Keep in mind that this bug only exists if you use the correct formula of prices, that I have reported separately. In current calculation, the price doesn't divide by the decimal difference between assets which may bring down the valuation of assets at BalanceSheet.sol Line71. Right now the calculation goes by directly squaring the price numbers which yields a high enough value to not cause undervaluation of assets. But as soon as you use the correct price calculation formula, the assets will be undervalued because there will be chances of probe price a rounding down.

Code Snippet

https://github.com/sherlock-audit/2023-10-aloe/blob/b60c21af24738d517941f18f7caa8c7272f771c5/aloe-ii/core/src/libraries/BalanceSheet.sol#L110

Tool used

Manual Review

Recommendation

Check that the calculated probe prices are in a definite safe range from the sqrtMeanPrice instead of capping it to MIN_SQRT_RATIO which can be abrupt to the sqrtScaler calculation at the time. Or change the formula in such a way that it can not round down to such small values.

sherlock-admin2 commented 1 year ago

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

panprog commented:

invalid, because MIN_SQRT_RATIO is the minimum price the uniswap pool can have (minimum price corresponding to minimum tick value), so it doesn't make sense to use any prices below it (including checking health)

tsvetanovv commented:

I think the formula work correct

MohammedRizwan commented:

valid