mstpr-brainbot - Division before multiplication can result to rounding errors

[sherlock-admin]

mstpr-brainbot

medium

Division before multiplication can result to rounding errors

Summary

Throughout the code there are some part of the code that does division before multiplication which could end up in loss of precision.

Vulnerability Detail

Let's take an example of this part of the code:

uint256 part2 = k * (V0) / (V1) * (V0) + (i * (delta)); // kQ0^2/Q1-i*deltaB

so the above math is simply: kQ^^2 / Q1 - (i deltaB). So what we should be doing is first getting the square of the Q2 which is V0 in the above code and then multiply it by k. However, the code first does the k V0 and then divides it to V1 and back to multiplication with V1. If the "k" value is significantly lesser than 1e18, then k V0 / V1 can result on precision error and even in rounding to "0". Assume k = 1, V0 = 50 1e18 and V1 = 100 * 1e18, then the result would be:

1 50 1e18 / 100 * 1e18 = 0 in solidity. Which would result that the entire part2 to be calculated mistakenly.

Also, we can see that the "k" value can be any value between 0 < 1e18 here: https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/af43d39f6a89e5084843e196fc0185abffe6304d/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSP.sol#L58-L59

so the above scenario becomes realistic.

Impact

Although having a "k" value 1 is not very realistic, it still can happen since the k is in range of 0 < 1e18, in such cases this rounding error will make the amounts to be calculated mistakenly. Hence, I'll label this as medium.

Code Snippet

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/af43d39f6a89e5084843e196fc0185abffe6304d/dodo-gassaving-pool/contracts/lib/DODOMath.sol#L83 https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/af43d39f6a89e5084843e196fc0185abffe6304d/dodo-gassaving-pool/contracts/lib/DODOMath.sol#L147 https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/af43d39f6a89e5084843e196fc0185abffe6304d/dodo-gassaving-pool/contracts/lib/DODOMath.sol#L159

Tool used

Manual Review

Recommendation

First multiply then divide throughout in the code

[nevillehuang]

Good job to watsons, all of the highlighted and gave an example of the possible precision loss from division before multiplication as required by sherlock guidelines, so medium severity seems appropriate.

[CergyK]

Escalate

uint256 part2 = k (V0) / (V1) (V0) + (i (delta)); // kQ0^2/Q1-ideltaB

Seems clear enough from the comment (the formula has all multiplications before division) that the devs knew what they were doing when changing the order of parameters. The intent is to avoid revert by overflow if k*V0*V0 is too large.

[sherlock-admin2]

Escalate

uint256 part2 = k (V0) / (V1) (V0) + (i (delta)); // kQ0^2/Q1-ideltaB

Seems clear enough from the comment (the formula has all multiplications before division) that the devs knew what they were doing when changing the order of parameters. The intent is to avoid revert by overflow if k*V0*V0 is too large.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Czar102]

1 50 1e18 / 100 * 1e18 = 0 in solidity. Which would result that the entire part2 to be calculated mistakenly.

That is false, the division is done before the last multiplication:

$ chisel
Welcome to Chisel! Type `!help` to show available commands.
➜ 1 * 50 * 1e18 / 100 * 1e18
Type: uint256
├ Hex: 0x0000000000000000000000000000000000604be73de4838ad9a5cf8800000000
├ Hex (full word): 0x0000000000000000000000000000000000604be73de4838ad9a5cf8800000000
└ Decimal: 500000000000000000000000000000000000

Anyway, would like to see a PoC where this rounding issue causes a material loss of funds.

[nevillehuang]

@mstpr Any comments? If not I think I am inclined to have to agree with @CergyK and @Czar102. This is dependent on what is a reasonable value of k.

@Skyewwww, can the value of k be that low of a value such as 1 mentioned in the issue?

[mstpr]

1 50 1e18 / 100 * 1e18

yes, so this is something very weird and I also recently figured out this. In solidity if you use literals I think it follows the typical paper math approach (first division then multiplication) so this is why you see it like this

but if you use variables and do the same math it won't be doing that.

that's weird, I'd like to know more about this tbh

and here the PoC, please copy this to remix and run the function

function weirdSolidity() external view returns (uint256 weird, uint256 moreWeird) {
        uint256 a = 1 * 50 * 1e18;
        uint256 b = 100 * 1e18;

        weird = a / b;
        moreWeird = 1 * 50 * 1e18 / 100 * 1e18;  
    }

first one will be 0 and second will be 500000000000000000000000000000000000 which that number is extremely high and our expected result is 1/2

[mstpr]

@mstpr Any comments? If not I think I am inclined to have to agree with @CergyK and @Czar102. This is dependent on what is a reasonable value of k.

@Skyewwww, can the value of k be that low of a value such as 1 mentioned in the issue?

so while I was doing the review I tested with some values so I'll share that with you too. In general, I see the "k" value can be any value between 0 < 1e18 so I assumed it can be even "1". Also, "V0" and "V1" are reserves, so one can be in 1e6 decimals and other can be 1e18 decimals which made me feel like this rounding issue can occur even more frequent. That basically means even k is 1e10 there can be underflows due to decimal scaling. Here the values I tried:

function kisses() external view returns (uint part2, uint part3) {
        uint k = 1e9; // this says "0 < k < 1e18" in code let me go with 1e9
        uint V0 = 500 * 1e6; // this is parallel with reserves, can be 1e6 or 1e18
        uint V1 = 1000 * 1e18; // this is parallel with reserves, can be 1e6 or 1e18
        part2 = k * (V0) / (V1) * (V0);

        part3 = k * (V0) * (V0) / (V1);
        // part2 = k * (V0) / (V1) * (V0) + (i * (delta));
    }

    function morekisses() external view returns (uint part2, uint part3) {
        uint k = 1; // this says "0 < k < 1e18" in code
        uint V0 = 500 * 1e6; // this is parallel with reserves, can be 1e6 or 1e18
        uint V1 = 1000 * 1e6; // this is parallel with reserves, can be 1e6 or 1e18
        part2 = k * (V0) / (V1) * (V0);

        part3 = k * (V0) * (V0) / (V1);
        // part2 = k * (V0) / (V1) * (V0) + (i * (delta));
    }

part2 = 0 and part3 = actual value above tests

[CergyK]

@mstpr @Czar102 @nevillehuang

A few clarifications: 1/ V1, V0 represent reserves of the same currency, and thus have the same number of decimals; Additionally this function is only called in cases where V0 >= V1;

So the absolute worst case can happen if k == 1, in which case the value would be rounded down by 50%

k is a factor expressed in 1e18 precision, so for example 1e9 actually represent a 1-e9 factor, and does not makes much more sense than using 1 (1e-18). Overall instead of using these values, an honest pool creator should use k == 0 which is handled by a separate code path.

So yes there is a precision loss, but for all practical values of K, the impact is imperceptible.

2/ Also we need to consider as per my original comment, that this choice (of inverting order of operands) was intentionally made to avoid overflowing k*V0*V0;

[Czar102]

that's weird, I'd like to know more about this tbh

That's how all programming languages I know would behave. You can read more here.

@mstpr @Skyewwww is there any use case for small nonzero k, for example depening on token/market parameters? Should it be perceived as a 0-1 value (linearly regulated)? Who sets the k value?

[mstpr]

@mstpr @Czar102 @nevillehuang

A few clarifications: 1/ V1, V0 represent reserves of the same currency, and thus have the same number of decimals; Additionally this function is only called in cases where V0 >= V1;

So the absolute worst case can happen if k == 1, in which case the value would be rounded down by 50%

k is a factor expressed in 1e18 precision, so for example 1e9 actually represent a 1-e9 factor, and does not makes much more sense than using 1 (1e-18). Overall instead of using these values, an honest pool creator should use k == 0 which is handled by a separate code path.

So yes there is a precision loss, but for all practical values of K, the impact is imperceptible.

2/ Also we need to consider as per my original comment, that this choice (of inverting order of operands) was intentionally made to avoid overflowing k/V0/V0;

Agree with this. As long as sponsor team says using k = 1 can be a case this can be a valid medium imo but if not, then imo this is invalid

[Czar102]

Planning to invalidate the finding if there is no PoC of a loss of funds caused by this issue within 24h.

[Czar102]

After a comment with the submitter, he agreed that it shouldn't be an issue as long as k will be reasonably high if nonzero. Will consider the issue a valid low.

[Czar102]

Result: Low Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• CergyK: accepted