PaulRBerg / PaulRBerg/prb-math

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

Open
#201 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

effort: medium priority: 2 type: refactor work: complicated
Dominant language
Solidity
Stars
1k
Forks
145
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by locating the mulDiv function and reproduce the divide-by-zero examples mulDiv(0, 1e18, 0) and mulDiv(1, 1e18, 0). Trace how prod0 and prod1 affect the checks, then add or update coverage so every divide-by-zero path produces the same error selector.

Written by the indexing model from the issue text.

Assessment

Tech stack
solidity
Domain
blockchain
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.