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

mulWadUp never catches arithmatic overflow

rowan

Summary

The mulWadUp never catches arithmatic overflow.

Vulnerability Details

The assembly operation in line 52 of MathMasters.sol always results in 0. Thus code block below it never runs, which means it cannot catch overflow.

The reason is

  1. or(div(not(0), y), x) is always greater than or equal to x.

  2. x never be greater than or(div(not(0), y), x), gt(x, or(div(not(0), y), x)) returns 0.

  3. mul(y, 0) returns 0.

  4. 0 is false in boolean, thus code inside if block is unreachable.

The test below represents the case when revert by overflow must happen but mulWadUp doesn't catch it.

function testMulWadUpNeverCatchOverflow() public {
uint256 x = MathMasters.sqrt(type(uint256).max) + 1; // 340282366920938463463374607431768211456
uint256 y = x;
​
uint256 z = MathMasters.mulWadUp(x, y);
​
uint256 expected;
uint256 modulo = 1e18;
​
assembly {
expected := add(
iszero(iszero(mod(mul(add(x, 1), y), modulo))), // 1
div(mul(add(x, 1), y), modulo) // 340282366920938463463
)
}
​
assertEq(z, expected); // 340282366920938463464
​
// arithmetic overflow
vm.expectRevert(stdError.arithmeticError);
​
z = x * y; // 2**256
}

Add the test in MathMasters.t.sol and run forge test -vvvv --mt testMulWadUpNeverCatchOverflow to see the result.

$$ forge test -vvvv --mt testMulWadUpNeverCatchOverflow
[⠒] Compiling...
[⠔] Compiling 1 files with 0.8.23
[⠒] Solc 0.8.23 finished in 1.76s
Compiler run successful!
​
Running 1 test for test/MathMasters.t.sol:MathMastersTest
[PASS] testMulWadUpNeverCatchOverflow() (gas: 4456)
Traces:
[4456] MathMastersTest::testMulWadUpNeverCatchOverflow()
├─ [0] VM::expectRevert(panic: arithmetic underflow or overflow (0x11))
│ └─ ← ()
└─ ← panic: arithmetic underflow or overflow (0x11)
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.10ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

All other contracts utilizing this library will be broken by unexpected overflow.

Tools Used

  • Foundry

Recommendations

Remove the or opcode.

if mul(y, gt(x, div(not(0), y))) {
mstore(0x40, 0xbac65e5b)
revert(0x1c, 0x04)
}
Updates

Lead Judging Commences

inallhonesty Lead Judge almost 2 years 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.

Give us feedback!