Division before multiplication can cause incorrect calculation causing less tokens being sent to the user

Summary

There can be precision loss because of division before multiplication which can lead to calculation error. It is a basic prevention to do multiplication first and then divison so as to avoid prescision loss.

Vulnerability Detail

Vulnerability lies in the following function

 function _SolveQuadraticFunctionForTrade(
        uint256 V0,
        uint256 V1,
        uint256 delta,
        uint256 i,
        uint256 k
    ) internal pure returns (uint256) {
        require(V0 > 0, "TARGET_IS_ZERO");
        if (delta == 0) {
            return 0;
        }

        if (k == 0) {
            // why v1
            return DecimalMath.mulFloor(i, delta) > V1 ? V1 : DecimalMath.mulFloor(i, delta);
        }

        if (k == DecimalMath.ONE) {
            // if k==1
            // Q2=Q1/(1+ideltaBQ1/Q0/Q0)
            // temp = ideltaBQ1/Q0/Q0
            // Q2 = Q1/(1+temp)
            // Q1-Q2 = Q1*(1-1/(1+temp)) = Q1*(temp/(1+temp))
            // uint256 temp = i.mul(delta).mul(V1).div(V0.mul(V0));
            uint256 temp;
            uint256 idelta = i * (delta);
            if (idelta == 0) {
                temp = 0;
            } else if ((idelta * V1) / idelta == V1) {
                temp = (idelta * V1) / (V0 * V0);
            } else {
                temp = delta * (V1) / (V0) * (i) / (V0);
            }
            return V1 * (temp) / (temp + (DecimalMath.ONE));
        }

        // calculate -b value and sig
        // b = kQ0^2/Q1-i*deltaB-(1-k)Q1
        // part1 = (1-k)Q1 >=0
        // part2 = kQ0^2/Q1-i*deltaB >=0
        // bAbs = abs(part1-part2)
        // if part1>part2 => b is negative => bSig is false
        // if part2>part1 => b is positive => bSig is true
        uint256 part2 = k * (V0) / (V1) * (V0) + (i * (delta)); // kQ0^2/Q1-i*deltaB
        uint256 bAbs = (DecimalMath.ONE - k) * (V1); // (1-k)Q1

        bool bSig;
        if (bAbs >= part2) {
            bAbs = bAbs - part2;
            bSig = false;
        } else {
            bAbs = part2 - bAbs;
            bSig = true;
        }
        bAbs = bAbs / (DecimalMath.ONE);

        // calculate sqrt
        uint256 squareRoot = DecimalMath.mulFloor((DecimalMath.ONE - k) * (4), DecimalMath.mulFloor(k, V0) * (V0)); // 4(1-k)kQ0^2
        squareRoot = Math.sqrt((bAbs * bAbs) + squareRoot); // sqrt(b*b+4(1-k)kQ0*Q0)

        // final res
        uint256 denominator = (DecimalMath.ONE - k) * 2; // 2(1-k)
        uint256 numerator;
        if (bSig) {
            numerator = squareRoot - bAbs;
        } else {
            numerator = bAbs + squareRoot;
        }

        uint256 V2 = DecimalMath.divCeil(numerator, denominator);
        if (V2 > V1) {
            return 0;
        } else {
            return V1 - V2;
        }
    }
}

More specifically in the following line

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

when part2 is calculated first V0 is multiplied with k and then it is divided by V1 and then it is multiplied by V0 this can cause the value of part2 to be less than the actual amount because of precision error

Now if the value of part2 is less than actual now if the following calculation is done it can cause some issues For simplicity actual value of part2 if there hadn't been division before multiplication = 3 But due to above calculation it comes out to be 2 And let's assume bAbs = 4 Then the following will execute

 if (bAbs >= part2) {
            bAbs = bAbs - part2;
            bSig = false;

Now bAbs according to the code = 4-2 = 2 Otherwise it would have been 4-3 = 1 so value of bAbs is more as compared to actual(actual mean multiplication first then divison) then square root is calulated using the following

squareRoot = Math.sqrt((bAbs * bAbs) + squareRoot);

as you can see we do bAbs*bAbs so this value also gets increased then finally when following is done

   uint256 denominator = (DecimalMath.ONE - k) * 2; // 2(1-k)
        uint256 numerator;
        if (bSig) {
            numerator = squareRoot - bAbs;
        } else {
            numerator = bAbs + squareRoot;
        }

as we have taken else case so the value of numerator is bAbs + squareRoot so this shows greater value than the actual

uint256 V2 = DecimalMath.divCeil(numerator, denominator);
        if (V2 > V1) {
            return 0;
        } else {
            return V1 - V2;
        }
    }

as from above it can be seen V2 value would be larger than the case when multiplication would be done first and then division and as Value of V2 is greater than expected , value of V1-V2 comes out to be lesser than expected.

Impact

Causes loss to the users, can also affect the values of bAbs and part2 where there can be case when due to precision loss causes bSig to become false instead it should have been true

Code Snippet

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

Tool used

Manual Review

Recommendation

Calculate part2 as follows uint256 part2 = (k (V0) (V0))/ (V1) + (i * (delta));

Duplicate of #116

sherlock-audit / 2023-12-dodo-gsp-judging

Varun_05 - Division before multiplication can cause incorrect calculation causing less tokens being sent to the user #168