logos-co / staking

SNT Staking contracts
Creative Commons Zero v1.0 Universal
0 stars 4 forks source link

Fix division precision loss #64

Closed 3esmit closed 8 months ago

3esmit commented 8 months ago

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 (#67).
  2. Use OpenZeppelin Math library (#66)
  3. Wait solidity implement fixed point

Read more:

3esmit commented 8 months ago

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 commented 8 months ago

Duplicate of #24

0x-r4bbit commented 8 months ago

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 commented 8 months ago

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

Advantages:

Disadvantage: