Submission Details
Severity:
Irrelevant statement incrementing x inside `mulWadUp` leads to incorrect answer
Summary
Irrelevant statement incrementing x inside mulWadUp leads to incorrect answer.
Vulnerability Details
The vulnerability occurs due to the statement inside mulWadUp at line 56 performing irrelevant increment of
x.
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))
}
}
Impact
Returns incorrect answer.
PoC
Add the test in the file:
test/MathMasters.t.solSet the
runsto100000000under[fuzz]insidefoundry.tomland comment out all the other parameters.
Run the test:
forge test --mt test_MulWadUp_ReturnsWrongAnswer -vv
function test_MulWadUp_ReturnsWrongAnswer(uint256 x, uint256 y) public view {
vm.assume(y == 0 || x <= type(uint256).max / y);
uint256 ans1 = MathMasters.mulWadUp(x, y);
uint256 ans2 = x * y == 0 ? 0 : (x * y - 1) / 1e18 + 1;
console2.log('Actual Answer:', ans1);
console2.log('Expected Answer:', ans2);
assert(ans1 == ans2);
}
Failed CounterExample:
Running 1 test for test/MathMasters.t.sol:MathMastersTest
[FAIL. Reason: panic: assertion failed (0x01); counterexample: calldata=0xf494bb07000000000000000000000000000000000000000000061e597cef96fd49e369c0000000000000000000000000000000000000000000030f2cbe77cb7ea4f1b4e1 args=[7396876674976619302185408 [7.396e24], 3698438337488309651092705 [3.698e24]]] test_MulWadUp_ReturnsWrongAnswer(uint256,uint256) (runs: 512, μ: 7693, ~: 7744)
Logs:
Actual Answer: 27356892272406583674190305384389
Expected Answer: 27356892272406583674190301685951
Traces:
[7785] MathMastersTest::test_MulWadUp_ReturnsWrongAnswer(7396876674976619302185408 [7.396e24], 3698438337488309651092705 [3.698e24])
├─ [0] VM::assume(true) [staticcall]
│ └─ ← ()
├─ [0] console::log("Actual Answer:", 27356892272406583674190305384389 [2.735e31]) [staticcall]
│ └─ ← ()
├─ [0] console::log("Expected Answer:", 27356892272406583674190301685951 [2.735e31]) [staticcall]
│ └─ ← ()
└─ ← panic: assertion failed (0x01)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 39.77ms
Tools Used
Manual Review
Recommendations
Remove the irrelevant statement
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 over 1 year ago
Submission Judgement Published Assigned finding tags:
`mulWadUp` has an unnecessary line that makes the result wrong for some inputs
Rewards breakdown
nSLOC
46This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.