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

Bad check for `x <= type(uint256).max / y` in mulWadUp can lead to overflow

ereynier

Summary

The assembly for "Equivalent to require(y == 0 || x <= type(uint256).max / y)." is wrong.

Vulnerability Details

if mul(y, gt(x, or(div(not(0), y), x))) { is used to test require(y == 0 || x <= type(uint256).max / y). The or statement breaks the check because gt(x, div(not(0), y) is equivalent to x > (UINT256_MAX / y) ? 1 : 0 the or will scramble the result of UINT256_MAX / y.

Impact

The function can continue instead of revert and lead to an overflow and a wrong result.

PoC (Proof of Code)

Add theses lines at the end of MathMastersTest.

function testMulWadUpOverflow() public {
uint256 x = 4;
uint256 y = type(uint256).max / 2;
uint256 result = MathMasters.mulWadUp(x, y); // = 1.157e59
console2.log("result: ", result);
uint256 expected = x * y == 0 ? 0 : (x * y - 1) / 1e18 + 1; // overflow (= 1.157e95)
assertEq(result, expected);
}
​
function testMulWadUpShouldRevert() public {
uint256 x = 4;
uint256 y = type(uint256).max / 2;
vm.expectRevert();
MathMasters.mulWadUp(x, y); // mulWadUp have "Equivalent to `require(y == 0 || x <= type(uint256).max / y)`." | UINT_MAX / (UINT_MAX / 2) = 2 | 4 > 2
}

What the check if mul(y, gt(x, or(div(not(0), y), x))) { does:

x = 4
y = UINT256_MAX / 2
​
y == 0 ? // NO
​
UINT256_MAX / y = 2
2 | 4 = 6 // (010 | 100) = 110
​
4 > 6 ? // NO - It should be 4 > 2 ? // YES
​
Continue
​

Tools Used

  • Foundry

Recommendations

You can use the same verification as the MathMasters:mulWad() function: if mul(y, gt(x, div(not(0), y))).

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.