MohammedRizwan - The `FullMath` library used in `LiquidityBorrowingManager.sol` and `DailyRateAndCollateral.sol` is unable to handle intermediate overflows due to overflow

[sherlock-admin2]

MohammedRizwan

high

The FullMath library used in LiquidityBorrowingManager.sol and DailyRateAndCollateral.sol is unable to handle intermediate overflows due to overflow

Summary

The FullMath.sol library used in LiquidityBorrowingManager.sol and DailyRateAndCollateral.sol contracts doesn't correctly handle the case when an intermediate value overflows 256 bits. This happens because an overflow is desired in this case but it's never reached.

Vulnerability Detail

The FullMath.sol library was taken from Uniswap. However, the original solidity version that was used was in this FullMath.sol library was < 0.8.0, Which means that the execution didn't revert when an overflow was reached. This effectively means that when a phantom overflow (a multiplication and division where an intermediate value overflows 256 bits) occurs the execution will revert and the correct result won't be returned.

mulDivRoundingUp() from the FullMath.sol has been used in below in scope contracts,

1) In LiquidityBorrowingManager.sol having functions calculateCollateralAmtForLifetime(), _precalculateBorrowing() and _getDebtInfo()

2) In DailyRateAndCollateral.sol having function _calculateCollateralBalance()

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

Impact

The correct result isn't returned in this case and the execution gets reverted when a phantom overflows occurs.

Code Snippet

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

The FullMath.sol having mulDivRoundingUp() function used LiquidityBorrowingManager.sol and DailyRateAndCollateral.sol doesn't use an unchecked block and it is shown as below(removed comments to shorten the code),

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

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.4;

library FullMath {

    function mulDiv(
        uint256 a,
        uint256 b,
        uint256 denominator
    ) internal pure returns (uint256 result) {
        unchecked {                                 @audit // used unchecked block here but not used in mulDivRoundingUp()
            uint256 prod0; // Least significant 256 bits of the product
            uint256 prod1; // Most significant 256 bits of the product
            assembly {
                let mm := mulmod(a, b, not(0))
                prod0 := mul(a, b)
                prod1 := sub(sub(mm, prod0), lt(mm, prod0))
            }

           // some code

            result = prod0 * inv;
            return result;
        }
    }

    function mulDivRoundingUp(
        uint256 a,
        uint256 b,
        uint256 denominator
    ) internal pure returns (uint256 result) {
        result = mulDiv(a, b, denominator);                @audit // does not use unchecked block similar to mulDiv()
        if (mulmod(a, b, denominator) > 0) {
            require(result < type(uint256).max);
            result++;
        }
    }
}

Tool used

Manual Review

Recommendation

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

Per Uniswap-v3-core, Do below changes in FullMath.sol

    function mulDivRoundingUp(
        uint256 a,
        uint256 b,
        uint256 denominator
    ) internal pure returns (uint256 result) {
+       unchecked {
            result = mulDiv(a, b, denominator);
            if (mulmod(a, b, denominator) > 0) {
                require(result < type(uint256).max);
                result++;
           }
+       }
    }

[nevillehuang]

Escalate

I don't see how this is an issue. There are 2 scenarios where overflow could happen in mulDivRoundingUp():

1. mulDiv(): where a * b could cause an overflow. However, it should be noted that there is an unchecked block in mulDiv() so this is correctly handled
2. mulmod(): Solidity in built function available on a global scope. This function does not overflow from solidity v0.8.0 upwards, see here. Or you can simply do a test on remix to verify it.

[sherlock-admin2]

Escalate

I don't see how this is an issue. There are 2 scenarios where overflow could happen in mulDivRoundingUp():

1. mulDiv(): where a * b could cause an overflow. However, it should be noted that there is an unchecked block in mulDiv() so this is correctly handled
2. mulmod(): Solidity in built function available on a global scope. This function does not overflow from solidity v0.8.0 upwards, see here. Or you can simply do a test on remix to verify it.

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.

[Oot2k]

I think the same as nevil! I don't see this as an medium.

[fann95]

Fixed: https://github.com/RealWagmi/wagmi-leverage/commit/55a5e8a08ce26a679c50bb104193d7cb47b63251

[0xRizwan]

The issue is correct. The issue is happening due to wrong use of mulDivRoundingUp() function as given in uniswap FullMath.sol contracts used in solidity version ^0.8.0. The function differs here due to missing unchecked block as the detail is present in report.

Old FullMath.sol mulDivRoundingUp() from uniswap used in <0.8.0 can be checked here

Recommended TickMath.sol mulDivRoundingUp() 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 here and here

Further, this similar issue was accepted at Sherlock which can be checked here

@fann95 Thanks for fixing this issue.

[Czar102]

A few points:

• the Spearbit links don't work for me, I am required to log in.
• the "similar issue" accepted by Sherlock didn't have an unchecked box for the entire contents of the mulDiv function, which is not the case here.
• mulmod() is a solidity function that utilizes the mulmod opcode which handles phantom overflow and it doesn't perform any overflow checks (and didn't perform any in the past). The only check Solidity adds is making sure that the denominator (modulo) is nonzero.
• the only arithmetic that could overflow in mulDivRoundingUp is result++;, which could be within an unchecked block because of the preceding check, but it is not a vulnerability if the overflow is checked twice.

NOTE: Per Solidity docs, the unchecked blocks affect only statements that are syntactically within them.

I am leaning towards accepting the escalation for the above reasons. @0xRizwan please let me know if I got something wrong. A PoC may be a good idea to show the validity of this issue.

[Evert0x]

Plan to accept escalation and make issue invalid @0xRizwan

[0xRizwan]

@Evert0x

If the issue is invalid then i request the sponsor to revert the changes made to contract. Invalid issue with its recommendation being fixed at the same time can not coexist.

Thank you!

[Evert0x]

Result: Invalid Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: accepted

[IAm0x52]

Fix looks good. General reformatting