DecorativePineapple - The `FullMath` library is unable to handle intermediate overflows due to overflow that's desired but never reached

[github-actions[bot]]

DecorativePineapple

medium

The FullMath library is unable to handle intermediate overflows due to overflow that's desired but never reached

Summary

The FullMath library 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 library was taken from Uniswap v3-core. However, the original solidity version that was used was < 0.8.0, meaning that the execution didn't revert wan 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. The original library was designed in a way that could handle intermediate overflows. The FullMath library is used in the MathLib library in order to format an 18-decimal number to a FixedPoint 96.Q96 number.

Impact

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

Code Snippet

The FullMath library which doesn't use an unchecked block:

library FullMath {
    /// @notice Calculates floor(a×b÷denominator) with full precision. Throws if result overflows a uint256 or denominator == 0
    /// @param a The multiplicand
    /// @param b The multiplier
    /// @param denominator The divisor
    /// @return result The 256-bit result
    /// @dev Credit to Remco Bloemen under MIT license https://xn--2-umb.com/21/muldiv
    function mulDiv(
        uint256 a,
        uint256 b,
        uint256 denominator
    ) internal pure returns (uint256 result) {
        // 512-bit multiply [prod1 prod0] = a * b
        // Compute the product mod 2**256 and mod 2**256 - 1
        // then use the Chinese Remainder Theorem to reconstruct
        // the 512 bit result. The result is stored in two 256
        // variables such that product = prod1 * 2**256 + prod0
        uint256 prod0; // Least significant 256 bits of the product

        ...

      result = prod0 * inv;
        return result;
    }

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

Tool used

Manual Code Review

Recommendation

It is advised to put the entire function bodies of mulDiv and mulDivRoundingUp 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.

[WarTech9]

Unchecked doesn't handle the error, any overflow/underflow would fail silently. upgrading to the newer version of Fullmath should would be good though but is low priority.

[hrishibhat]

Agree with sponsor comment. Considering the issue as informational

[christos-eth]

Escalate for 15 USDC

As noted in the FullMath library, it was designed in order to handle "phantom overflow" i.e., allows multiplication and division where an intermediate value overflows 256 bits. This means that the original library can handle statements like : FullMath.mulDiv(type(uint256).max / 2, 3, 111)) . This a feature of the library, as statements like this cannot be handled by solidity >0.8 directly as the execution will revert due to an overflow. Because the original library was created with solidity version <0.8.0(which doesn't revert on overflows) this behaviour was allowed as the expected intermediatory overflow could be reached.

However, due to the fact that the FullMath Library was ported to solidity version 0.8.9, which is >0.8, this operation would revert as the intermediate calculations would overflow, meaning that it can't handle those multiplication and division where an intermediate value overflows the 256 bits. The fix is to mark the full body in an unchecked block, in order to leverage the fact that the original version was designed in order to allow the intermediate overflow.

I think that the comment left by the sponsor: Unchecked doesn't handle the error, , doesn't reflect the issue, as by using the unchecked block the desired behaviour could be reached.

A modified version of the original Fullmath library that uses unchecked blocks to handle the intended overflow, can be found in the 0.8 branch of the Uniswap v3-core repo.

[sherlock-admin]

Escalate for 15 USDC

As noted in the FullMath library, it was designed in order to handle "phantom overflow" i.e., allows multiplication and division where an intermediate value overflows 256 bits. This means that the original library can handle statements like : FullMath.mulDiv(type(uint256).max / 2, 3, 111)) . This a feature of the library, as statements like this cannot be handled by solidity >0.8 directly as the execution will revert due to an overflow. Because the original library was created with solidity version <0.8.0(which doesn't revert on overflows) this behaviour was allowed as the expected intermediatory overflow could be reached.

However, due to the fact that the FullMath Library was ported to solidity version 0.8.9, which is >0.8, this operation would revert as the intermediate calculations would overflow, meaning that it can't handle those multiplication and division where an intermediate value overflows the 256 bits. The fix is to mark the full body in an unchecked block, in order to leverage the fact that the original version was designed in order to allow the intermediate overflow.

I think that the comment left by the sponsor: Unchecked doesn't handle the error, , doesn't reflect the issue, as by using the unchecked block the desired behaviour could be reached.

A modified version of the original Fullmath library that uses unchecked blocks to handle the intended overflow, can be found in the 0.8 branch of the Uniswap v3-core repo.

You've created a valid escalation for 15 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

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

[hrishibhat]

Escalation accepted

Agree that there could a potential overflow, & it must be updated to latest FullMath version. Considering this a valid medium issue.

[sherlock-admin]

Escalation accepted

Agree that there could a potential overflow, & it must be updated to latest FullMath version. Considering this a valid medium issue

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[sherlock-admin]

Escalation accepted

Agree that there could a potential overflow, & it must be updated to latest FullMath version. Considering this a valid medium issue.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[hrishibhat]

Fix: https://github.com/UXDProtocol/uxd-evm/pull/30

[IAm0x52]

Fix looks good. Library has been updated