Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: low
Valid

Revert logic in assembly block is incorrect

Summary

The reverting logic in both mulWad and mulWadUp functions is not correct. It's using the free memory pointer improperly, the offset in revert is invalid and the length of the revert reason is not correct.

Vulnerability Details

The revert logic has multiple errors:

  1. The revert reason, in this case custom error identifier, is stored in 0x40 which will override the free memory pointer. This may be dangerous as later we could not be able to access the free memory pointer or the value stored in 0x40 would be wrongly used as a free memory pointer,

  2. The offset for the revert reason is not correct, as we need to point out the start of the revert reason and then pass the length of the revert reason. The current value of the offset is 0x1c, which may point out to arbitrary data held in the scratch space (0x00-0x40).

  3. Revert reason length is not correct, as storing the error identifier like 0xbac65e5b will right-pad it, which means it will be written in the memory with a zero fill on the left side: 0x000000000000...000bac65e5b

Impact

This may be classified as a medium impact vulnerability, as it won't affect the protocol, but would return an out-of-bounds error instead of the error intended to describe why the revert happened.

Tools Used

Foundry

Recommendations

The proposed fix would solve the issues mentioned above as follows:

  1. The free memory pointer would be properly used and not overridden,

  2. The revert reason would be properly stored in the memory starting from the free memory pointer and the value would be properly set to avoid right-padding,

  3. The offset for the revert reason would be properly set to point out to the start of the revert reason and the length of the revert reason would be properly set to avoid right-padding.

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)
+ // Get free memory pointer
+ let ptr := mload(0x40)
+ // Store the function selector for `MathMasters__MulWadFailed`
+ mstore(ptr, 0xa56044f700000000000000000000000000000000000000000000000000000000)
+ // Revert with the error
+ revert(ptr, 0x20) // as the error is 32 bytes long, we read `0x20`
}
z := div(mul(x, y), WAD)
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Wrong error selector

Wrong error storage

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.