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

Unnecessary modification of input parameter in mulWadUp

easyFloyd

Summary

The mulWadUp function contains an if statement which modifies the x input under certain conditions. This modifiaction is completely unnecessary and in some cases leads to miscalculation.

Vulnerability Details

In the case when the parameters of the mulWadUp function fulfill the next condition the result of the function will be wrong:

x >= 1e18 && y > x / 2 && y <= x

function testMulWadUpCounterExample() public {
uint256 x = 90e18; // x >= 1e18
uint256 y = x / 2 +1; // y > x / 2 && y <= x
uint256 expected = x * y == 0 ? 0 : (x * y - 1) / 1e18 + 1;
assertFalse(MathMasters.mulWadUp(x, y) == expected);
}
​
# Result:
​
# [PASS] testMulWadUpCounterExample() (gas: 1033)
#Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.57ms

Notice: This condition of course is understood for x and y parameters where x * y wouldn't owerflow. (Owerflow is handled by the function in a separate branch.)

The test testMulWadUpFuzz(uint256 x, uint256 y) also recognize this bug with runs = 1000000 and can give counterexample for the affected inputs:

# foundry.toml
...
[fuzz]
runs = 1000000 # Use this one for prod
...
​
# Result
​
# Running 1 test for test/MathMasters.t.sol:MathMastersTest
# [FAIL. Reason: assertion failed; counterexample: calldata=0xfb6ea94f00000000000000000000000000000000000000000002bfc66f73219d55eb3274000000000000000000000000000000000000000000015fe337b990ceaaf5993b args=[3323484123583475243233908 [3.323e24], 1661742061791737621616955 [1.661e24]]] testMulWadUpFuzz(uint256,uint256) (runs: 3979, μ: 1894, ~: 1172)
# Logs:
# Error: a == b not satisfied [uint]
# Left: 5522773359855710271721683078203
# Right: 5522773359855710271721681416461
#
# Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 115.80ms

Impact

Contracts that use this libary can have a miscalculated result from the mulWadUp function in some cases detailed above which can lead to loss funds and possible exploit scenarios.

Tools Used

Manual review, Math review, Forge test

Recommendations

Remove the unnecessary branch from the function:

/// @dev Equivalent to `(x * y) / WAD` rounded up.
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))
}
}
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.