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

Invalid calculation when rounding up

0xrektified

Summary

The mulWadUp method is not rounding up correctly the (x * y) / WAD result.

By incrementing the value of x when the result of (z+x)/y is equal to 1, it introduces a new calculation ((x +1) * y) / WAD.

This results for a specific range of numbers, have a final outcome completely wrong increased by 2 instead of 1 in the best case scenario or returning a huge difference of result.

Vulnerability Details

For this POC we will use a simple division with small numbers to show demonstrate the issue, at the end of the POC we will show how far off this calculation could be.

The issue arises in the mulWadUp method as shown in the provided code snippet

function mulWadUp(uint256 x, uint256 y) public 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)
}
//@note 1 is added to x if `(z+x)/y` is equal to 1
@> if iszero(sub(div(add(z, x), y), 1)) { x := add(x, 1) }
//@note we are now doing `((x +1) * y) / WAD` the new result is increased by one
@> z := add(iszero(iszero(mod(mul(x, y), WAD))), div(mul(x, y), WAD))
}
}
​

The flaw lies in the conditional increment of x (if iszero(sub(div(add(z, x), y), 1)) { x := add(x, 1) }).

This adjustment is meant to round up x, which is actually not necessary at all.

We can demonstrate the vulnerability with the following variable values:
x = 576684777797582847 and y = 288511851128422423

/*
* Push 1 to stack
* Push y to stack
* Push x to stack
* Push z to stack <- equal to zero
* add z to x in stack
* div (z + x) by y <-| if X divided by Y result is 1 (576684777797582847/288511851128422423) -> 1.9988252667682924
* sub 1 from div result
* if result is 0 add 1 to x
*/
if iszero(sub(div(add(z, x), y), 1)) { x := add(x, 1) }

At this step we have the following equation ((x +1) * y) / WAD

/*
* mul x and y
* div mul result by WAD
*
* mul x and y
* mod mul result by WAD
* iszero if mod is not null result is 0
* iszero if iszero is 0 result is 1
* add 1 to result to round up
*/
z := add(iszero(iszero(mod(mul(x, y), WAD))), div(mul(x, y), WAD))

The result will be ((x + 1) * y / WAD) + 1) instead of (x * y / WAD) + 1)

Giving us the wrong result 166380392759963588 instead of 166380392759963587

If we use bigger numbers as the following one

x = 223108379911240396122621124461961929566
y = 137428086083331686137894327482105279507

The result will be 30661357640354615014511215001663630841895493678062493593460 instead of 30661357640354615014511215001663630841758065591979161907322

Which is off by 137428086083331686138 wei

Impact

This vulnerability can lead to inaccurate calculations in transactions relying on mulWadUp.

The impact could range from minor discrepancies in values to more significant financial inaccuracies, depending on the use case of the contract.

As this method is used in the library and the probability to happen is low to medium and the impact is medium to high i ll qualify it as a medium severity.

Tools Used

halmos
manual review

Recommendations

Remove the if iszero(sub(div(add(z, x), y), 1)) { x := add(x, 1) } method that lead to this bug and doesn't add any solution to the method:

function mulWadUp(uint256 x, uint256 y) public 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))
}
}
Updates

Lead Judging Commences

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

`mulWadUp` has an unnecessary line that makes the result wrong for some inputs

Support

FAQs

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