Exact input functions yield less value than exact output for the same parameters

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L272
https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L353
https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L463

Vulnerability details

Impact

swapGivenInputAmount, depositGivenInputAmount and withdrawGivenInputAmount functions yield less value than their counterparts swapGivenOutputAmount, depositGivenOutputAmount, and withdrawGivenOutputAmount for the same parameters.
This results in integrators losing money when using the exact input functions.

Proof of Concept

Alice calls depositGivenInputAmount with the following parameters:

    function testCrossDeposit() public {
        py_init_val = 6951000000000000;
        px_init_val = 69510000000000;
        py_final_val = 695100000000000000;
        px_final_val = 695100000000000;

        py_init = ABDKMath64x64.divu(py_init_val, 1e18);
        px_init = ABDKMath64x64.divu(px_init_val, 1e18);
        py_final = ABDKMath64x64.divu(py_final_val, 1e18);
        px_final = ABDKMath64x64.divu(px_final_val, 1e18);

        duration = T_DURATION;

        EvolvingInstrumentedProteus DUT = new EvolvingInstrumentedProteus(py_init, px_init, py_final, px_final, duration);
        
        uint256 totalSupply = 10e18;
        int256 xBalance = 50 * 1e18;
        int256 yBalance = 100 * 1e18;

        uint256 depositedXAmount = 1e18;
        uint256 mintedAmountOfLpTokens = DUT.depositGivenInputAmount(
            uint256(xBalance), uint256(yBalance), totalSupply, depositedXAmount, SpecifiedToken.X
        );
        emit log_named_uint("mintedAmountOfLpTokens", mintedAmountOfLpTokens);

        uint256 amountOfXToDeposit = DUT.depositGivenOutputAmount(
            uint256(xBalance), uint256(yBalance), totalSupply, mintedAmountOfLpTokens, SpecifiedToken.X
        );
        emit log_named_uint("amountOfXToDeposit", amountOfXToDeposit);
        
        emit log_named_uint("diff", depositedXAmount - amountOfXToDeposit);
    }

The depositGivenInputAmount function returns the minted amount of LP tokens for depositing 1e18 X tokens.
If you call the depositGivenOutputAmount and pass this minted amount of LP tokens, you need a lesser amount of X tokens to deposit.
This makes it meaningless for anyone to use the exact input functions since they would be losing value.

The same flaw is present in the swapGivenInputAmount vs swapGivenOutputAmount and withdrawGivenInputAmount vs withdrawGivenOutputAmount functions.

To find the magnitude of the discrepancy between the exact input and exact output functions, here are invariant tests that provide the maximum difference.
https://gist.github.com/josipkoncurat/4cc3cd48403d880efd766938e85aeae7

The maximum differences we were able to spot were:

• depositGivenInputAmount vs depositGivenOutputAmount - 0.000418%
• swapGivenInputAmount vs swapGivenOutputAmount - 0.000312679%
• withdrawGivenInputAmount vs withdrawGivenOutputAmount - 0.169%

Note: Try running invariant tests several times to get the maximum difference.

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

The discrepancy is caused by the incorrect fee accounting between the different functions. For cases where fees are omitted, there is still some discrepancy, but it is rather small.

An option that doesn't require a whole reengineering of the fees is either removing the input functions or adding a warning in the documentation(comments) that the exact input functions yield less value than the exact output functions for the same parameters.

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as high quality report

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[0xRobocop]

Description fails to point out precisely what the root cause is.

Seems that this is acknowledged in the documentation:

Failure of soft invariants (see above), especially those where the user is expected to net more tokens than they started with, but don’t.

[c4-sponsor]

ishaansinghal (sponsor) acknowledged

[c4-sponsor]

viraj124 marked the issue as disagree with severity

[viraj124]

probably can be low/QA since we've already encountered this and have been aware of this as the fee mechanism is intended

[c4-judge]

JustDravee changed the severity to QA (Quality Assurance)

[JustDravee]

To be merged with #222

[c4-judge]

JustDravee marked the issue as grade-b