Summary
The mulWadUp function doesn't handle if (x * y) would owerflow because of an incorrect condition, and the relevant test misses the check for the revert in an owerflow case.
Vulnerability Details
None of the operands can be greater than the output of a bitwise OR operation. Therefore the statement in the mulWadUp function that checks the overflow condtion will never be true.
# This condition will always will be false:
# mul(y, gt(x, or(div(not(0), y), x)))
The relevant test completed with the missing check:
function testMulWadUpFuzz(uint256 x, uint256 y) public {
// We want to skip the case where x * y would overflow.
// Since Solidity 0.8.0 checks for overflows by default,
// we cannot just multiply x and y as this could revert.
// Instead, we can ensure x or y is 0, or
// that y is less than or equal to the maximum uint256 value divided by x
if (x == 0 || y == 0 || y <= type(uint256).max / x) {
uint256 result = MathMasters.mulWadUp(x, y);
uint256 expected = x * y == 0 ? 0 : (x * y - 1) / 1e18 + 1;
assertEq(result, expected);
}
// If the conditions for x and y are such that x * y would overflow,
// this function will simply not perform the assertion.
// In a testing context, you might want to handle this case differently,
// depending on whether you want to consider such an overflow case as passing or failing.
+ else {
+ // Should check if revert
+ vm.expectRevert();
+ MathMasters.mulWadUp(x, y);
+ }
}
# Result:
# Running 1 test for test/MathMasters.t.sol:MathMastersTest
# [FAIL. Reason: call did not revert as expected; counterexample: calldata=0xfb6ea94f0000000000000000000000000000000000000000000000000000000000000002800000000000000000000000000000000000ffffffffffffffffffffffffffff args=[2, 57896044618658097711785492504343953926634997525117140554556420534452894040063 [5.789e76]]] testMulWadUpFuzz(uint256,uint256) (runs: 5, μ: 1220, ~: 1220)
# Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 9.23ms
Impact
When a contract uses this libary needs to check for owerflow by itself otherwise it can be ended in lost funds, miscalculated results and exploit possibilites.
Tools Used
Manual review, Forge test
Recommendations
Fix the condition to check if x <= type(uint256).max / y like in the code below and add the revert check to the tests as it mentioned and showed in the 'Vulnerability Details' section.
/// @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))) {
+ 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))
}
}
