Submission Details
Severity:
Increment will break the calculation in `MathMasters::mulWadUp`
Description
MathMasters::mulWalUp contains the block of code below.
function mulWadUp(uint256 x, uint256 y) internal pure returns (uint256 z) {
/// @solidity memory-safe-assembly
assembly {
...
@> if iszero(sub(div(add(z, x), y), 1)) {
@> x := add(x, 1)
@> }
...
}
}
Since z is always 0 at this step, it has the following behavior:
If , increment x by 1.
Because of the rounded-down division of the EVM, every time , the condition will be true.
This increment will break the calculation because it makes no sense to increment in order to find the result of a multiplication and a division.
Impact
Likelihood: High
Every time
Impact: High
Function will return a wrong answer, leading to unexpected behavior on all contracts using this library.
Proof of Concept
Foundry PoC added in MathMasters.t.sol
import {Math} from "lib/openzeppelin-contracts/contracts/utils/math/Math.sol";
...
function testMulWadUpFail() public {
assertEq(
MathMasters.mulWadUp(
56023067736567210695140920363957329,
// (above_number/2) + 1
28011533868283605347570460181978665
),
Math.mulDiv(
56023067736567210695140920363957329,
28011533868283605347570460181978665,
uint256(1e18),
Math.Rounding.Ceil
)
);
}
Recommended Mitigation
Remove all the parts incrementing x.
function mulWadUp(uint256 x, uint256 y) internal pure returns (uint256 z) {
/// @solidity memory-safe-assembly
assembly {
...
- if iszero(sub(div(add(z, x), y), 1)) {
- x := add(x, 1)
- }
...
}
}
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.