MohammedRizwan - Use `unchecked` in `TickMath.sol` which is extensively used in `LiquidityManager.sol`

[sherlock-admin2]

MohammedRizwan

high

Use unchecked in TickMath.sol which is extensively used in LiquidityManager.sol

Summary

Use unchecked in TickMath.sol which is extensively used in LiquidityManager.sol

Vulnerability Detail

The TickMath.sol library was taken from Uniswap. However, the original solidity version that was used was in this TickMath.sol library was < 0.8.0, Which means that the execution didn't revert when an overflow was reached. Now, TickMath.sol has used 0.8.4, the arithmetic operations should be wrapped in an unchecked block.

getSqrtRatioAtTick() has been extensively used in below LiquidityManager.sol functions,

1) _getSingleSideRoundUpBorrowedAmount() 2) _restoreLiquidity() 3) _getHoldTokenAmountIn()

The values returned from these functions wont be correct and the execution of these functions will be reverted when overflow occurs.

Impact

getSqrtRatioAtTick() has been extensively used in LiquidityManager.sol functions, but the library TickMath.sol is compiled with pragma solidity ^0.8.4 which can be checked here which doesn't allow for overflows, and since the function is not unchecked.

getSqrtRatioAtTick() used in LiquidityManager.sol will not behave as intended since it relies implicitly on overflows.

Uniswap v3 with version 0.8.0 TickMath.sol can be checked as below, https://github.com/Uniswap/v3-core/blob/0.8/contracts/libraries/TickMath.sol

and Current implementation of TickMath.sol used in wagmi contracts can be checked below, https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/vendor0.8/uniswap/TickMath.sol

At first glance, if you can see both contracts are identical, but the TickMath.sol used in wagmi contracts will not let overflows happens, every time this function is used in the code it could revert and break the functionality.

Code Snippet

Code reference: https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/vendor0.8/uniswap/TickMath.sol#L24-L25

File: wagmi-leverage/contracts/vendor0.8/uniswap/TickMath.sol

    function getSqrtRatioAtTick(int24 tick) internal pure returns (uint160 sqrtPriceX96) {         
        uint256 absTick = tick < 0 ? uint256(-int256(tick)) : uint256(int256(tick));    @audit // unchecked block is missing

        // EDIT: 0.8 compatibility
        require(absTick <= uint256(int256(MAX_TICK)), "T");

       // some code

        sqrtPriceX96 = uint160((ratio >> 32) + (ratio % (1 << 32) == 0 ? 0 : 1));
    }

    function getTickAtSqrtRatio(uint160 sqrtPriceX96) internal pure returns (int24 tick) {
        // second inequality must be < because the price can never reach the price at the max tick
        require(sqrtPriceX96 >= MIN_SQRT_RATIO && sqrtPriceX96 < MAX_SQRT_RATIO, "R");   @audit // unchecked block is missing

      // some code

        tick = tickLow == tickHi ? tickLow : getSqrtRatioAtTick(tickHi) <= sqrtPriceX96
            ? tickHi
            : tickLow;
    }

Tool used

Manual Review

Recommendation

It is advised to put the entire function bodies of getSqrtRatioAtTick() and getTickAtSqrtRatio() in an unchecked block. A modified version of the original TickMath library that uses unchecked blocks to handle the overflow, can be found in the 0.8 branch of the Uniswap v3-core repo.

Duplicate of #138

[nevillehuang]

Escalate:

I am abit confused on where the overflow can exactly happen, since the watson is simply stating it could happen without any concrete examples. I think this issue shouldn't be a concern due to the following:

1. getSqrtRatioAtTick(): There is a explicit check here

2. getTickAtSqrtRatio(): Solidity compiler overflow checks does not apply to assembly code, see here

[sherlock-admin2]

Escalate:

I am abit confused on where the overflow can exactly happen, since the watson is simply stating it could happen without any concrete examples. I think this issue shouldn't be a concern due to the following:

1. getSqrtRatioAtTick(): There is a explicit check here

2. getTickAtSqrtRatio(): Solidity compiler overflow checks does not apply to assembly code, see here

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xRizwan]

The issue is correct and it is self explanatory. The issue is happening due to wrong use of TickMath.sol which was basically used and designed for contracts using solidity version <0.8.0 since the solidity version ^0.8.0 handles overflow and underflow by default therefore the functions inside TickMath.sol should be kept in unchecked block. To be noted, wagmi contracts has used solidity version 0.8.4, Therefore the fix is a must to resolve the issue.

Old TickMath.sol from uniswap used in <0.8.0 can be checked here

Recommended TickMath.sol from uniswap used in ^0.8.0 can be checked here

This issue has also found by Spearbit as a High severity, some of the references can be checked below,

Reference 1

Reference 2

Reference 3

I would also request to see the possibility of considering it as High severity.

@fann95 This should not be duplicate to #138. This should be considered as separate issue. I can see #138 is fixed and Please fix this issue too as it is in different contract, may be it missed due to duplicate label. Thanks.

[Czar102]

@0xRizwan could you name examples of operations in the code you described that could revert due to overflow, but shouldn't? It seems those are lacking in your report. Otherwise will accept the escalation and consider this invalid.

[0xRizwan]

@Czar102

I think, Its already mentioned in report. getSqrtRatioAtTick() has been extensively used in below LiquidityManager.sol functions,

1) _getSingleSideRoundUpBorrowedAmount() which is further used in _extractLiquidity() and _upRestoreLiquidityCache() 2) _restoreLiquidity() 3) _getHoldTokenAmountIn() which is further used in _restoreLiquidity()

All these functions are the part of abstract LiquidityManager.sol and LiquidityManager.sol is inherited by LiquidityBorrowingManager.sol, Further, _restoreLiquidity() is used in repay() of LiquidityBorrowingManager and

TickMath.sol was designed with consideration to overflow edge cases by uniswap with version 0.7.6 where solidity compiler does not used to handle overflow by default, however this is can be handled by default with solidity version 0.8.4. Please refer this similar issue judged as High severity at Code4rena. With current implementation, the values returned from above highlighted functions won't be correct and the execution of these functions will be reverted when overflow occurs as it is breaking the overflow logic. checkout this uniswap issue for reference.

This is kind of another similar overflow issue and in both of the uniswap libraries as they were designed for solidity version <0.8.0 with due consideration to overflow. But as i said solidity version 0.8.0 and above (in this project 0.8.4) handles overflow and underflow by default therefore TickMath.sol functions must be unchecked. Further the above spearbit issues can be referred.

[nevillehuang]

I get what @0xRizwan is saying now, but considering the following, this issue should remain as low severity:

1. In the original submission, the impact was not fully explored and proven, the watson is simply indicating an overflow can happen. Not to put Spearbit in bad light or anything, but they too also lack a detailed explanation of the issue. However, those are private audits that may have internal communications between them, not a public audit contest.

2. Sherlock has different judging guidelines as Code4rena. In Sherlock, an issue like this causing DoS is only considered medium severity if it breaks the [core functionality permanently](). Not sure if Sherlock consider additional information on top of original submissions after contests periods end, but imo, unless @0xRizwan can prove a permanent DoS arising from the use of TickMath library, this should be low severity.

Breaks core contract functionality, rendering the contract useless (should not be easily replaced without loss of funds) or leading to unknown potential exploits/loss of funds.

I think I have given sufficient details with regards to this issue, and will respect any outcome to this issue you guys decide @Czar102 @Evert0x

[Czar102]

@nevillehuang thank you for your comment. I looked into the code and I can't see any overflow checks that would cause an unwanted revert. @0xRizwan also dmed me, but he wasn't able to specify which arithmetic operation can overflow. Hence, planning to accept the escalation and make the issue invalid.

It's difficult to judge those submissions because the core issue isn't shown, but on the off-chance that they may be right, I need to analyze all references and try to understand watson's stance. Maybe in the future we'll have a better way to deal with this kind of submissions, feeling kind of DoSed here.

[nevillehuang]

@Czar102 I think burden of proof shouldn’t be on you, but the watson itself. If he cannot prove the issue, or other watsons cannot help facilitate the validity of the issue during the escalation period, we shouldn’t validate the issue to keep the contest results fair. Perhaps an explicit description revolving this topic on burden of proof could be included in sherlocks rules.

This is by no means bringing down the efforts of @0xRizwan , but if we foresee audit contests rising in popularity going forward, we need to start enforcing better quality submissions as one way to reduce judging overhead.

[Czar102]

Yes, but in this case those are different things whether the issue is valid and whether is should be fixed. I want to make sure it's fixed iff needed. Goal is to make smart contracts secure. And then, distribute rewards fairly. And it's not watsons' job to check the validity of submissions, ultimately they have a goal of making the system more secure, not make the scoring system fair wrt them.

[nevillehuang]

Yes, but in this case those are different things whether the issue is valid and whether is should be fixed. I want to make sure it's fixed only if needed. Goal is to make smart contracts secure. And then, distribute rewards fairly. And it's not watsons' job to check the validity of submissions, ultimately they have a goal of making the system more secure, not make the scoring system fair wrt them.

100% agree, thanks for the insights 🙏

[0xRizwan]

The issue is valid and it must be fixed, i might be missing few detail but please see below additional context too.

While i was drafting this issue, i referred both the spearbit as well as code4rena issue which is judged as High severity. Both the platforms are reputed and does not approve low quality or invalid issue as i see this issue is being debated for its validation despite giving number of references. I agree its watsons responsibility to provide enough burden of proof and i have provided more than that to describe the issue to the best. I request to recheck both spearbit and code4rena reports and see this report as it provide more detail about issue as compared to those reports.

The issue is in the TickMath.sol so use of its function within any function is vulnerable to this overflow issue. The contract has used incorrect library for the project which i have already explained above. This is one of the rare issue and i think if it was common wouldnt have been debated this much, however the references in this case should not be ignored.

Similar unchecked issue was escalated in past at sherlock and see the judgment of that issue here, the issue is not closely similar but would be worth checking to get the context.

The issue in TickMath.sol can be resolved by two ways, 1) Add the unchecked block per the recommendation 2) Restrict the compiler version of TickMath.sol to <0.8.0 instead of ^0.8.4

In both of the way, it won't break the designed overflow logic done by uniswap. This recommendation of this issue is not harming the protocol, instead it prevent the issue from happening, Also request to check TickMath.sol before <0.8.0 and after >0.8.0 to get the issue.

The issue should not be left unfixed by considering the monetary compensation that would be rewarded to watsons but i think here the issue is important more than the rewards. My intension to submit this report was to make more secure contracts after checking the reference reports also i see these issues are only submitted by me as it does not have duplicate by other watsons.

I would be closing my discussion on this issue here and would agree with @Evert0x final judgement.

Thank you!

[Czar102]

I understand your argument, but I don't see an exploit there.

I would be closing my discussion on this issue here and would agree with @Evert0x final judgement.

I am judging escalations for this contest and those decisions should be considered final.

As mentioned before, planning to accept the escalation and make the issue invalid.

[fann95]

I don’t think there is any vulnerability here. The unchecked {} was added for optimization. I fixed this so that the code matches the Uniswap option. Fixed: https://github.com/RealWagmi/wagmi-leverage/commit/aa44fa23b8d385eaa196316036fce88cb6eb43e8

I also made changes to the LiquidityAmounts, which clearly shows that unchecked is used for optimization. Fixed+: https://github.com/RealWagmi/wagmi-leverage/commit/c971b1bcae5718fc4e8ebd15ae0178f882f87688

[Evert0x]

The report and comments fail to pinpoint the affected logic in the contracts. Plan to stick to @Czar102 his judgment.

[0xRizwan]

@Evert0x

The issue is in the library which is used in inscope contract and i have given enough burden of proofs for its validation. Reputed platforms like spearbit and code4rena judged this similar issue as High severity.

On a serious note, I am unable to understand this flow---> Sponsor has confirmed the issue ---> sherlock plans to reject it --> sponsor disputes the issue --> sponsor fixes the issue --> sponsor re-confirms the issue --> Sherlock plans to reject the issue.

The comment by sponsor I fixed this so that the code matches the Uniswap option. First of all if this issue is not a vulnerability and if it is a invalid issue why it is being fixed here. The fix which is made by sponsor is my recommendation in report. Why the need of matching the code with uniswap has arised ONLY after this issue submission and not before that?

If the report has to be invalid then fixes made by sponsor should be reverted as the issue is being judged as invalid here.

I am sure if the references were from previous sherlock contest instead of spearbit or code4rena then the issue would have been valid and no such discussion would have been done.

[Evert0x]

Result: Invalid Duplicate of #138

I am sure if the references were from previous sherlock contest instead of spearbit or code4rena then the issue would have been valid and no such discussion would have been done.

The issue would have been valid if you were able to pinpoint logic in the contracts that would result in broken logic.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: accepted

[IAm0x52]

Fix looks good. Unchecked blocks added for gas optimization as well as adding custom errors and general reformatting