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

Precision loss in division #85

Closed
3esmit opened this issue Dec 2, 2024 · 1 comment · Fixed by #92
Closed

Precision loss in division #85

3esmit opened this issue Dec 2, 2024 · 1 comment · Fixed by #92
Assignees

Comments

@3esmit
Copy link
Contributor

3esmit commented Dec 2, 2024

Summary

Currently there is some difference in the calculations for MP due the precision loss of using SCALE_FACTOR.

General security recommendation is to use Math.mulDiv for these type of calculations, which also comes with advantage of reducing changes of integer overflow: SCALE_FACTOR scales values which can lead to uint256 overflow.

While it has an advantage in gas costs, finance applications generally should optimize precision over gas costs.

Details

Potential problems with SCALE_FACTOR

Using SCALE_FACTOR is a common technique to handle fixed-point arithmetic in Solidity, but it can fail in certain situations. Below are some examples where using SCALE_FACTOR might lead to failure due to precision loss, overflow, or inaccurate results:

1. Overflow in Large Multiplications

One of the most significant risks with using SCALE_FACTOR is the possibility of overflow during multiplication when numbers are too large. Solidity operates on 256-bit integers, and multiplying large numbers without using techniques like mulDiv can easily cause overflow.

Example:

Let's assume we are using SCALE_FACTOR = $10^{18}$

uint256 x = 2 * 10**38; // Large number
uint256 y = 3 * 10**38; // Another large number
uint256 denominator = 10**18;
uint256 result = (x * y) / denominator; // This will overflow

Here, the product $x \times y = 6 \times 10^{76}$, which exceeds Solidity's 256-bit integer limit. Without a function like mulDiv, this operation would result in an overflow, and the transaction would revert.

2. Loss of Precision in Division

In Solidity, when you divide integers, the result is automatically floored to the nearest integer. If you multiply first by SCALE_FACTOR and then divide, the flooring can lead to precision loss, especially if the values are not perfectly divisible.

Example:
uint256 x = 3;
uint256 y = 2;
uint256 scaleFactor = 10**18;
uint256 result = (x * scaleFactor) / y; // Expecting 1.5, but result will be floored to 1

Here, $\frac{3}{2}$ is expected to give $1.5$, but since Solidity floors the result, you get $1 \times 10^{18}$, which effectively represents $1$. The fractional part is lost, leading to inaccurate calculations.


3. Precision Loss in Multiple Operations

When performing a sequence of operations, every time you apply a SCALE_FACTOR and then divide, you risk accumulating small errors due to repeated flooring, which can compound and lead to significant inaccuracies in complex calculations.

Example:
uint256 scaleFactor = 10**18;
uint256 a = 7;
uint256 b = 3;
uint256 c = 5;

uint256 result = ((a * scaleFactor) / b) * scaleFactor / c;

Expected mathematical result:

$$\left( \frac{7}{3} \right) \times \frac{1}{5} = 0.46666\ldots$$

However, due to the flooring effect after each division, the result is more likely to be 0.4×10180.4 \times 10^{18}0.4×1018, not capturing the full precision of the expected result. Each division results in some precision loss.


4. Error Due to Incorrect Use of SCALE_FACTOR

In some cases, developers might incorrectly apply SCALE_FACTOR in either the multiplication or division step, leading to highly inaccurate results.

Example:

If a developer forgets to apply the SCALE_FACTOR consistently throughout a calculation:

uint256 x = 500 * scaleFactor;
uint256 y = 3;

uint256 result = (x / y) / scaleFactor; // Incorrectly applying scaleFactor

The result here will be much smaller than expected because the SCALE_FACTOR was incorrectly applied in the division step, leading to incorrect scaling.

The correct way would have been:

uint256 result = (x / y); // No need to apply scaleFactor again

5. Handling Very Small Fractions

When working with very small fractional values, SCALE_FACTOR might not provide enough precision to represent the result accurately. This is especially problematic when you need to represent values smaller than the smallest unit supported by SCALE_FACTOR.

Example:
uint256 smallValue = 1;
uint256 scaleFactor = 10**18;

uint256 result = (smallValue * scaleFactor) / 10000000000000000000; // Attempting to represent a very small fraction

Here, you are trying to represent a very small fraction ( 1/10191 / 10^{19}1/1019 ), but the result will be floored to zero because Solidity cannot represent such small values with a precision limited by SCALE_FACTOR.


6. Complex Financial Calculations (e.g., Compound Interest)

For financial applications, even small precision errors can accumulate and lead to significant discrepancies. In calculations like compound interest or bond pricing, the cumulative effect of losing precision can result in large differences from the expected outcome.

Example:

Calculating compound interest for a series of periods:

uint256 principal = 1000 * scaleFactor;
uint256 rate = 5 * scaleFactor / 100;
uint256 periods = 10;

uint256 result = principal;
for (uint256 i = 0; i < periods; i++) {
    result = result + (result * rate) / scaleFactor; // Each iteration introduces precision loss
}

After 10 periods, the accumulated rounding errors from multiple divisions can make the final result inaccurate compared to the expected result using exact math (like mulDiv).


@0x-r4bbit 0x-r4bbit mentioned this issue Dec 3, 2024
3 tasks
0x-r4bbit added a commit that referenced this issue Dec 4, 2024
0x-r4bbit added a commit that referenced this issue Dec 4, 2024
0x-r4bbit added a commit that referenced this issue Dec 4, 2024
@0x-r4bbit 0x-r4bbit self-assigned this Dec 4, 2024
@0x-r4bbit 0x-r4bbit moved this to In Progress in Tasks - Smart Contracts Dec 4, 2024
@0x-r4bbit 0x-r4bbit moved this from In Progress to Needs Review in Tasks - Smart Contracts Dec 4, 2024
@0x-r4bbit
Copy link
Collaborator

Made a PR that uses Math.mulDiv in #92

0x-r4bbit added a commit that referenced this issue Dec 5, 2024
0x-r4bbit added a commit that referenced this issue Dec 5, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in Tasks - Smart Contracts Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

2 participants