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

Incorrect overflow check inside `mulWadUp` evaluates revert condition to false even if there is a overflow.

shikhar229169

Summary

mulWadUp function should revert if there is a overflow but due to incorrect overflow checks it will not revert and returns a wrong answer because of overflow.

Vulnerability Details

The vulnerability occurs due to the wrong overflow condition check inside mulWadUp function inside MathMasters.sol

It is assumed that the overflow check at line 52 is equivalent to require(y == 0 || x <= type(uint256).max / y) but it is actually not.

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))) {
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))
}
}

The incorrect overflow check at line 52 is mul(y, gt(x, or(div(not(0), y), x)))

Here first it gets the maximum safe value of x by div(not(0), y) and then performs or operation with x.

We know that or operation performed on x and div(not(0), y) will yield at least x as the answer and the condition gt(x, or(div(not(0), y), x)) will always be false, because or(div(not(0), y), x) will always be >=x.

Thus, even if there is a overflow it will not revert and still returns incorrect answer.

Impact

In case of overflow, the function mulWadUp will not revert and still returns incorrect answer.

PoC

Add the test in the file: test/MathMasters.t.sol

Run the test:

forge test --mt test_MulWadUp_NotReverts_Even_IfThereIsAOverflow -vv
function test_MulWadUp_NotReverts_Even_IfThereIsAOverflow() public view {
// Both the args are maximum uint256 values
uint256 ans = MathMasters.mulWadUp(type(uint256).max, type(uint256).max);
​
// the function doesn't revert on overflow and still gives the incorrect answer
console2.log(ans);
}

Tools Used

Manual Review

Recommendations

Eliminate the unnecessary or operation.

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))
}
}
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.