Loss of precision due to division occurring before multiplication across multiple statements leads to lesser number of receiving tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L750 https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L781

Vulnerability details

Impact

Swaps and Deposits work with two tokens X and Y. The computed amount of tokens on the receiving end decrease due to this multi-statement loss of precision occurring due to division before multiplication.

Note: This finding is different from the [L-06] bot finding due to two reasons: 1. The bot finding in the first place is not a division before multiplication issue. 2. This issue is due to loss of precision occurring across three statements.

Proof of Concept

In the equations below, u = utility ac = a_convert bc = b_convert M = MULTIPLIER (Rest of the symbols are self explanatory according to the current implementation in function _getPointGivenYandUtility(). The following applies to _getPointGivenXandUtility() function as well but we'll be talking about _getPointGivenYandUtility() function here).

The loss of precision occurs here due to division occurring before multiplication across the three statements below. If we take a look at this simplified equation (represents internal calculations of Lines 781-783) for f_2, we can see division occurs before multiplication on step 4: https://drive.google.com/file/d/1qVgf9ycl4tnCrRR1TR19WIt47YM-xHpd/view

File: src/proteus/EvolvingProteus.sol
781:    int256 f_0 = (( y0  * MULTIPLIER ) / utility) + b_convert;
782:    int256 f_1 = ( ((MULTIPLIER)*(MULTIPLIER) / f_0) - a_convert );
783:    int256 f_2 = (f_1 * utility) / (MULTIPLIER); 

This leads to the return value f_2 (aka x0 in _getPointGivenYandUtility()) being smaller than expected. The deviance or loss of precision of the current three-step implementation above can be compared to the simplified equation (present on the last line of the image).

Here is the coded POC, which compares loss of precision of the simplified equation with the current three-step implementation:

1. Add this test to the end of EvolvingProteusProperties.t.sol file
2. Run the following command to test: forge test --match-test testLossOfPrecisionDifference

    function testLossOfPrecisionDifference() public {
        int256 utility = 200 * 10**18;
        int256 y = 1000 * 10**18; 

        vm.expectRevert();// We expect curve error but that is fine since the intention of this test is to compare the return value with that of the simplified equation for loss of precision
        (int256 x0, ) = DUT.getPointGivenYandUtility(utility, y);//We only care about the value of f_2 or x0 //Also note x0 is the returned value from the current three-step implementation

        int256 a_convert = 1;
        int256 b_convert = 1;

        //Simplified Equation (refer to last line of image for equation) - This is the new implementation
        int256 uM = (utility*1e18);// 1e18 for MULTIPLIER
        int256 f_2 = (uM*uM - uM * a_convert * y + utility*utility*a_convert*b_convert)/1e18; //f_2 is the return value from new implementation

        assertLt(x0, f_2);//This test passes which means there is lesser loss of precision occurring in the simplified equation than the current method being used
    }

In the test, we can see that assertLt(x0, f_2); passes meaning that the loss of precision is greater in the current three-step implementation than the simplified equation on the last line of the image.

Tools Used

Manual Review

Recommended Mitigation Steps

The solution is the simplified equation on the last line of the image.

The following changes below are done with respect to the _getPointGivenYandUtility() function. The same can be done for the _getPointGivenXandUtility() function.

Solution:

File: src/proteus/EvolvingProteus.sol
781:    int256 uM = (utility*MULTIPLIER);
782:    int256 f_2 = (uM*uM - uM * a_convert * y0 + utility*utility*a_convert*b_convert)/MULTIPLIER;

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[0xRobocop]

May be QA.

Description lacks detail in quantifying how many tokens are lost due to this error. In my experience, these types of errors are negligible.

[c4-sponsor]

ishaansinghal (sponsor) disputed

[ishaansinghal]

We do the multiplication operation first since the numerator, denominator, and result are in 1e18 format. Consider y0 was 1.5 x 10^18 and util was 1 x 10^18. Performing the division first would erase the decimal in the result.

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid