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

Fix division precision loss #64

Closed
3esmit opened this issue Feb 23, 2024 · 4 comments · Fixed by #66
Closed

Fix division precision loss #64

3esmit opened this issue Feb 23, 2024 · 4 comments · Fixed by #66
Assignees
Labels
priority: high type: bug Something isn't working

Comments

@3esmit
Copy link
Collaborator

3esmit commented Feb 23, 2024

In StakeManager, there are some divisions, but solidity does not support any type of real numbers, so we are using integers where division always rounds down.

For the MP logic, this would break the minting, as it usually would get something like .1, .01, and try to multiply that by the balance to find how much to increase. This will always return zero in current code.

Possible solutions:

  1. Multiply by a value (like 100, or 100000, or whatever precision we need) before division, and after divide back this multiplier (fix(StakeManager): use mul by PRECISION and div back later to avoid precision loss in int divisions #67).
  2. Use OpenZeppelin Math library (fix(StakeManager): use OpenZeppelin Math to avoid precision loss in int divisions #66)
  3. Wait solidity implement fixed point

Read more:

@3esmit
Copy link
Collaborator Author

3esmit commented Feb 24, 2024

For now, my analysis favors #66 to be approved over #67.

OpenZeppelin mulDiv function uses a smart trick to perform these operations with reduced gas cost, and seems a more safe approch overall.

I'll try to dig more into these solutions to see if I can break one or other in certain scenarios, or if I can improve #67 formulas.

@3esmit
Copy link
Collaborator Author

3esmit commented Feb 25, 2024

Duplicate of #24

@0x-r4bbit
Copy link
Collaborator

I think going with #66 is a good decision here. It's a battle-tested solution to the problem, this also makes it potentially easier to audit.

@3esmit
Copy link
Collaborator Author

3esmit commented Feb 26, 2024

We ended up using OpenZeppelin Math library (#66).

Advantages:

  • Audited and battle tested
  • Cheaper in gas (at least for the test cases that were created)
  • More clear code
  • Safer against overflows (if we use a precision multiplier, than for huge values it could overflow)

Disadvantage:

  • External library, one more dependency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment