Summary
The mulWadUp() function has a condition to check if x / y == 1, and in that case it adds 1 to x. This is unnecessary, and can lead to errors.
Vulnerability Details
The condition if iszero(sub(div(add(z, x), y), 1)) checks if x / y == 1. This may have been added to adjust rounding upwards, but it is unnecessary and for big numbers may cause errors and overflows.
function mulWadUp(uint256 x, uint256 y) internal pure returns (uint256 z) {
assembly {
if mul(y, gt(x, div(not(0), y))) {
mstore(0x40, 0xbac65e5b)
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
The function will return incorrect calculations, which may lead to loss of funds if used in monetary transactions.
Tools Used
Foundry, Halmos, Manual review
Proof of Concept
This test should pass but will fail.
function testMulWadUpOverflow2() public {
uint256 x2 = 149453833408801632100269689951836288089;
uint256 y2 = 79700981937175649427451356571001433277;
assertEq(MathMasters.mulWadUp(x2, y2), (x2 * y2 - 1) / 1e18 + 1);
}
This test will overflow.
function testMulWadUpOverflow3() public {
uint256 y = MathMasters.sqrt(type(uint256).max);
uint256 x = type(uint256).max / y + 1;
uint256 r = MathMasters.mulWadUp(x, y);
console2.log("r", r);
}
forge test --mt testMulWadUpOverflow2
...
[FAIL. Reason: assertion failed] testMulWadUpOverflow2() (gas: 15259)
Logs:
Error: a == b not satisfied [uint]
Left: 11911617276956557497108380364885387729253693854339561184817
Right: 11911617276956557497108380364885387729173992872402385535389
...
```bash
forge test --mt testMulWadUpOverflow3
[PASS] testMulWadUpOverflow3() (gas: 3788)
Logs:
r 680564733841876926927
Recommended Mitigation
Remove the incorrect line.
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, div(not(0), y))) {
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))
}
}
Previous test passes, confirming that the mitigation is correct.
forge test --mt testMulWadUpOverflow2
...
[PASS] testMulWadUpOverflow2() (gas: 681)
