First Flight #8: Math Master
First Flight #8
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Incorrect overflow detection in `MathMasters.sol::mulWadUp()` function leads to inaccurate results

stefanlatinovic

Summary

The MathMasters.sol::mulWadUp() function has an incorrectly implemented overflow risk check.

Vulnerability Details

The MathMasters.sol::mulWadUp() function implements an overflow check, which condition will never be evaluated to be true. This means that if the input values to the function cause an overflow, the function will still produce a result, but it will be incorrect. As a consequence, calls to this function with such input values will not be reverted as expected, and the output will be unreliable.

Impact

The function call will not revert if the input overflows and will return an incorrect result.

Proof of Concept (PoC)

Add the following test in MathMasters.t.sol:

function test_MulWadUpShouldRevertOnOverflow(uint256 x, uint256 y) public {
vm.assume(y != 0 && x > type(uint256).max / y); // Input values that will result in overflow
​
vm.expectRevert(); // Expected to revert due to overflow
MathMasters.mulWadUp(x, y);
}

Run a test with forge test --mt test_MulWadUpShouldRevertOnOverflow.

We should see an output similar to this in the terminal:

Running 1 test for test/MathMasters.t.sol:MathMastersTest
[FAIL. Reason: call did not revert as expected; counterexample: calldata=0x00a981f80000000000000000000000000000000000000000000000000000000000000002800000000000000000000000000000000000ffffffffffffffffffffffffffff args=[2, 57896044618658097711785492504343953926634997525117140554556420534452894040063 [5.789e76]]] test_MulWadUpShouldRevertOnOverflow(uint256,uint256) (runs: 0, μ: 0, ~: 0)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 9.17ms

The test has failed because the function call did not revert as expected.

Tools Used

  • Manual review

  • Foundry

Recommendations

It is necessary to correct the implementation of the overflow condition check.

Recommended changes to MathMasters.sol::mulWadUp() function:

function mulWadUp(uint256 x, uint256 y) internal pure returns (uint256 z) {
/// @solidity memory-safe-assembly
assembly {
// Equivalent to `require(y == 0 || x <= type(uint256).max / y)`.
- if mul(y, gt(x, or(div(not(0), y), x))) {
+ if mul(y, gt(x, div(not(0), y))) {
mstore(0x40, 0xbac65e5b) // `MathMasters__MulWadFailed()`.
revert(0x1c, 0x04)
}
if iszero(sub(div(add(z, x), y), 1)) { x := add(x, 1) }
z := add(iszero(iszero(mod(mul(x, y), WAD))), div(mul(x, y), WAD))
}
}

If we attempt to execute the same test that was used in the PoC, we can see that it now passes as the function call reverted as expected:

Running 1 test for test/MathMasters.t.sol:MathMastersTest
[PASS] test_MulWadUpShouldRevertOnOverflow(uint256,uint256) (runs: 100, μ: 3665, ~: 3665)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.60ms
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`mulWadUp` has a bad overflow check

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.