prb-math icon indicating copy to clipboard operation
prb-math copied to clipboard

overflow classification and error is inconsistent on divide by zero in mulDiv

Open thedavidmeister opened this issue 2 years ago • 1 comments

What version of PRBMath are you using?

5959ef59f906d689c2472ed08797872a1cc00644

What version of Solidity are you using?

0.8.19

Describe the bug

the unchecked division at the start of mulDiv actually jumps into a divide by zero check even though it is "unchecked".

this is probably a good thing because mulDiv(0, 1e18, 0) and mulDiv(1, 1e18, 0) and other examples cause prod1 to be 0 which is treated as a "non overflow case" according to the comments, but we still don't want to allow dividing by zero here.

further down in the same fn, divide by 0 triggers an "overflow" error if prod1 >= denominator and the denominator is 0.

i don't think this is dangerous but it leads to inconsistent errors which makes fuzzing downstream quite fiddly if we want to assert on the error selector with foundry.

the same error should be thrown for "divide by 0" regardless of the internal prod0 and prod1 logic.

thedavidmeister avatar Aug 21 '23 02:08 thedavidmeister

Thanks for opening the issue, @thedavidmeister.

For future reference, this is the context:

https://github.com/PaulRBerg/prb-math/blob/5959ef59f906d689c2472ed08797872a1cc00644/src/Common.sol#L399-L404

PaulRBerg avatar Aug 21 '23 12:08 PaulRBerg