Incorrect Scaling in sqrt Function of Fixed.sol Leads to Incorrect Square Root Calculation

[c4-bot-7]

Lines of code

https://github.com/reserve-protocol/protocol/blob/4.0.0/contracts/libraries/Fixed.sol#L333

Vulnerability details

Impact

echidna test logs show that the testSqrt(uint192) function failed when trying to calculate the square root of 1 (the input value x was 1):

Input value (x): 1 Calculated square root (result): 1000000000 Square of result: 1000000000000000000 Square of (result + 1): 1000000002000000001 Analyzing the Output Square Root Calculation:

1. The calculated square root (result) is 1000000000, which corresponds to 1e9. This seems incorrect because the square root of 1 should be 1, not 1e9.

Square of Result:

1. The square of the calculated result is 1000000000000000000, which corresponds to 1e18. This is what we would expect if the input was 1e18 (not 1). Square of (Result + 1):

The square of result + 1 is 1000000002000000001, which further confirms that the result was incorrectly calculated.

Proof of Concept

Echidna test used to Test the square root function with logging: function testSqrt(uint192 x) public { emit LogUint("Input value (x)", x);

    uint192 result = x.sqrt();

    emit LogUint("Calculated square root (result)", result);
    emit LogUint("Square of result", uint192(result * result));
    emit LogUint("Square of (result + 1)", uint192((result + 1) * (result + 1)));

    assert(result * result <= x); // Square of the result should not exceed x

    // Allow a small tolerance in the comparison
    assert((result + 1) * (result + 1) >= x || (result + 1) * (result + 1) - 1 <= x);
}

MAX_UINT192(): passing testSafeWrap(uint256): passing testMultiplication(uint192,uint192): passing testAdditionAndSubtraction(uint192,uint192): passing testDivision(uint192,uint192): passing testToUint(uint192): passing testSqrt(uint192): failed!💥 Call sequence: FixLibTest.testSqrt(1)

Tools Used

echidna and vs code

Recommended Mitigation Steps

Root Cause: The issue stems from how the square root calculation is implemented in the Fixed.sol contract. Specifically, it appears that the input value x is being treated as if it's already scaled by 1e18 (fixed-point representation), rather than a simple integer. The sqrt function then incorrectly scales it further by 1e18 when calling sqrt256(x * FIX_ONE_256).

Fixing the Issue To fix the issue, we need to ensure that the sqrt function correctly handles values that are not already in fixed-point format. The function should take x as a uint192 value and directly calculate the square root without additional scaling.

Replace: function sqrt(uint192 x) internal pure returns (uint192) { return _safeWrap(sqrt256(x * FIX_ONE_256)); // FLOOR } with: function sqrt(uint192 x) internal pure returns (uint192) { return _safeWrap(sqrt256(uint256(x))); }

Explanation of the Updated Function:

Why This Works:

1. Remove Incorrect Scaling: The original code incorrectly scales x by multiplying it with FIX_ONE_256 before passing it to sqrt256, which leads to an incorrect result. By removing this scaling, the function now correctly calculates the square root of x as intended.

When fixed, it PASSES all echidna test:

MAX_UINT192(): passing testSafeWrap(uint256): passing testMultiplication(uint192,uint192): passing testAdditionAndSubtraction(uint192,uint192): passing testDivision(uint192,uint192): passing testToUint(uint192): passing testSqrt(uint192): passing wrap(uint256): passing testShift(uint192,int8): passing AssertionFailed(..): passing

Summary

1. Correct Functionality Restored: The updated sqrt function now correctly handles values that are not in fixed-point format, ensuring accurate square root calculations.
2. Successful Tests: After applying the fix, all Echidna tests, including testSqrt(uint192), pass successfully, confirming the robustness of the solution.

Assessed type

Library