Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
thedavidmeister opened this issue Aug 21, 2023 · 1 comment
Labels
effort: medium Default level of effort. priority: 2 We will do our best to deal with this. type: refactor Change that neither fixes a bug nor adds a feature. work: complicated Sense-analyze-respond. The relationship between cause and effect requires analysis or expertise.

Comments

@thedavidmeister
Copy link

thedavidmeister commented Aug 21, 2023

What version of PRBMath are you using?

5959ef5

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.

@PaulRBerg
Copy link
Owner

PaulRBerg commented Aug 21, 2023

Thanks for opening the issue, @thedavidmeister.

For future reference, this is the context:

prb-math/src/Common.sol

Lines 399 to 404 in 5959ef5

// Handle non-overflow cases, 256 by 256 division.
if (prod1 == 0) {
unchecked {
return prod0 / denominator;
}
}

@PaulRBerg PaulRBerg added type: refactor Change that neither fixes a bug nor adds a feature. effort: medium Default level of effort. work: complicated Sense-analyze-respond. The relationship between cause and effect requires analysis or expertise. priority: 2 We will do our best to deal with this. and removed bug labels Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium Default level of effort. priority: 2 We will do our best to deal with this. type: refactor Change that neither fixes a bug nor adds a feature. work: complicated Sense-analyze-respond. The relationship between cause and effect requires analysis or expertise.
Projects
None yet
Development

No branches or pull requests

2 participants