Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: high
Valid

mulWadUp buffer overflow

Summary

The mulWadUp function exhibits a buffer overflow vulnerability due to an incorrect use of an OR bitwise operation
which alters the intended behavior of checking for overflow conditions.

The comment in the code suggests a check equivalent to require(y == 0 || x <= type(uint256).max / y),
but the actual implementation does not adhere to this logic.

This vulnerability is particularly critical because the overflow protection normally provided by Solidity (version 0.8.0 and above) does not apply to assembly code.

Vulnerability Details

The vulnerability is present in the following line of the mulWadUp function:

if mul(y, gt(x, or(div(not(0), y), x))) { // <- WRONG
mstore(0x40, 0xbac65e5b) // `MathMasters__MulWadFailed()`.
revert(0x1c, 0x04)
}

This can be demonstrated with the following test:

function test_output_mulWadUp4() public {
// max uint -> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
// x = 0x000000000000ffffffffffff000000000000ffffffffffff0000000000ffffff
uint256 x = 411376139330300049037104964741611719419386059307161732541054975;
uint256 y = 281474976710657; // 0x0000000000000000000000000000000000000000000000000001000000000001
// max uint / y
//0x000000000000ffffffffffff000000000000ffffffffffff000000000000ffff
uint256 mathMasters = mulWadUp(x, y);
console2.log("mathMasters: %s", mathMasters);
console2.log("demo 1: %s", (x * y / WAD) + 1);
/*
* LOG RESULT
*
* mathMasters: 4704
* demo 1: panic: arithmetic underflow or overflow (0x11)
*
* */
}

while executing this code the result of the division is:

0x000000000000ffffffffffff000000000000ffffffffffff000000000000ffff

by using OR on x which is greater:

0x000000000000ffffffffffff000000000000ffffffffffff0000000000ffffff

the result of the division after OR is

0x000000000000ffffffffffff000000000000ffffffffffff0000000000ffffff

As a result, the gt condition returns zero, and the mul by zero is zero, so the intended revert condition is not triggered.

Impact

The impact of this vulnerability is significant as it allows arithmetic operations to overflow without triggering the necessary safety checks, when most user using a version of solidity above 0.8.0 don't expect an overflow being possible.

Tools Used

Test Cases

Recommendations

To address this vulnerability, the overflow check should be corrected to accurately reflect the intended condition require(y == 0 || x <= type(uint256).max / y).
This means replacing the incorrect bitwise OR operation with a proper logical comparison.

- if mul(y, gt(x, or(div(not(0), y), x))) {
+ if mul(y, gt(x, div(not(0), y))) {
// Store the function selector of `MulWadFailed()`.
mstore(0x40, 0xbac65e5b)
// Revert with (offset, size).
revert(0x1c, 0x04)
}
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.