Mint/Burn amount during LP addition/removal changes with time

[code423n4]

Lines of code

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

Vulnerability details

Impact

When a user adds LP to the pool, they get LP tokens which they can later use to redeem their positions. The issue is that since the utility and the curve parameters change with time, so does the value of the LP tokens.

Say a user wants to deposit 1 ETH to the pool. The pool has a curve with certain parameters, where the curve changes over time. If the user deposits 1 ETH in the beginning, they get a certain number of LP tokens. If the user waits and deposits the same 1 ETH later, they can get more LP tokens depending on the change in price.

This leads to users providing LP to lose tokens. In the POC below, we show that a user deposits 1 ETH at the beginning, and the same 1 ETH at the end. We show that if they deposit at the very end, they will get 10x more LP tokens. This means that if the user deposited that 1 ETH early, they can withdraw 1/10th of the funds from the LP.

Proof of Concept

A simple POC is developed showing the following steps:

1. User deposits 1 ETH and records amount of LP tokens minted to them
2. vm.warp to the end
3. User deposits 1 ETH and records amount of LP tokens minted to them again
4. LP minted in step 3 is much higher than in step 1.

The parameters used for the price is the same as present in the EvolvingProteusProperties.t.sol file.

function testAttack() public {
    uint256 x0 = 100e18;
    uint256 y0 = 100e18;
    uint256 s0 = 100e18;
    uint256 depositedAmount = 1e18;

    SpecifiedToken depositedToken = SpecifiedToken.X;
    uint256 minted1 = DUT.depositGivenInputAmount(
        x0,
        y0,
        s0,
        depositedAmount,
        depositedToken
    );

    emit log_named_uint("minted1", minted1);

    vm.warp(DUT.tFinal() + 1);

    uint256 minted2 = DUT.depositGivenInputAmount(
        x0,
        y0,
        s0,
        depositedAmount,
        depositedToken
    );

    emit log_named_uint("minted2", minted2);
}

The output shows:

Running 1 test for src/test/EvolvingProteusProperties.t.sol:EvolvingProteusProperties
[PASS] testAttack() (gas: 96394)
Logs:
  minted1: 6801162225772413
  minted2: 245982734610703745

This shows user gains more LP depositing later, and thus depositing early loses the user purchasing power. Even though both users deposited 1 ETH, at the time of removal, the later user can claim more tokens from the LP.

Tools Used

Foundry

Recommended Mitigation Steps

The LP tokens in the user's wallet must be scaled over time to keep its stake of the pool constant over time. This might done by scaling the LP tokens by the ratio of the current utility to the utility at the time of minting, but would require more testing.

Assessed type

Math

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as high quality report

[c4-sponsor]

ishaansinghal (sponsor) disputed

[ishaansinghal]

The pool's weights for the reserve tokens change over time, which is why this behavior occurs. In a normal liquidity pool, this would be similar to depositing at time X, waiting for some time as the pool's composition changes, and then making the same deposit at time Y. You would receive different amount of LP tokens in this situation based on the pool's new composition.

[viraj124]

all this is interrelated in a way to the price configurations and of course, the pool liquidity state at the time so don't think it's valid

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid

[carrotsmuggler]

The point of this issue was that users eat impermanent loss even without the change of the composition of the pool. The composition of the pool only changes via swaps and liquidity addition/removals. However, even if a pool is completely untouched, and its composition and price does not change at all, liquidity providers can still lose tokens!

To better illustrate this point, i reworked the above test slightly, but the core issue remains the same. For this test, the parameters are chosen form one of the test conditions given in the test file.

// x & y both decrease
py_init_val = 6951000000000000;
px_init_val = 69510000000000;
py_final_val = 695100000000000;
px_final_val = 6951000000000;

The test has three steps:

1. Alice deposits 1 ETH of liquidity
2. Alice waits for the curve to change
3. Alice removes the liquidity she had added and counts the amount of tokens she gets from it.

During this procedure, no others interact with the pool. Thus the price and the composition cannot change. Even though the parameters a and b change over time, the actual composition (x,y) stays the same if there are no swaps done in the meantime. It will be shown here that Alice can make a profit by just adding liquidity and removing it later, with no swaps taking place in between at all.

Test case:

function testAttack1() public {
        uint256 x0 = 100e18;
        uint256 y0 = 100e18;
        uint256 s0 = 100e18;
        uint256 depositedAmount = 1e18;
        SpecifiedToken depositedToken = SpecifiedToken.X;
        uint256 minted1 = DUT.depositGivenInputAmount(
            x0,
            y0,
            s0,
            depositedAmount,
            depositedToken
        );
        emit log_named_uint("input amount ", depositedAmount);
        vm.warp(DUT.tFinal());
        uint256 outAmount = DUT.withdrawGivenInputAmount(
            x0 + depositedAmount,
            y0,
            s0 + minted1,
            minted1,
            depositedToken
        );
        emit log_named_uint("Output amount", outAmount);
    }

Output:

[PASS] testAttack1() (gas: 102269)
Logs:
  input amount   : 1000000000000000000
  Output amount: 9777908374322145132

This shows Alice makes a profit even if there has been no interactions with the pool or any change in the composition. This profit comes from the earlier liquidity providers of the pool. The issue is this design is very bad for attracting any kind of liquidity. The argument In a normal liquidity pool, this would be similar to depositing at time X, waiting for some time as the pool's composition changes, and then making the same deposit at time Y. does not apply here, since no others had interacted with the pool in this time span.

In reality, if this system is deployed and if the protocol owners choose values similar to the test shown above, this can allow a strange attack vector which will allow attackers to extract value from the pool at 0 risk.

An attacker will provide liquidity to the pool while the proteus curve is evolving. As shown in the test above, provided no swaps happen, they will passively gain tokens since if they burn all their LP tokens, they will get back more eth than they put in. Now the attacker just has to evade impermanent loss due to price change in case of any swaps. This can be done by gaming their transaction position in the block , by removing liquidity as the first transaction in a block and adding liquidity as the last transaction, taking advantage of periods of low volume, or straight up sandwiching swap transactions (which isn't viable on arbitrum, but is viable on other chains). They can sandwich any swap transaction by taking out liquidity before the swap, and adding it back in after the swap, and for every second they hold LP tokens at a constant price, they can steal other LP provider's tokens by exiting the pool.

In most cases, protocol treasuries set up the initial liquidity and lock them. Thus there will always be tokens for the attacker to steal.

Here I have shown that:

1. Under certain configurations which are allowed by the system, users can gain tokens even if no one is interacting with he pool at all at the cost of other users (test case above)
2. This condition, if present on chain, can be abused by an attacker to drain liquidity from the protocol.
3. The attacker has 0 risk at times of low volume. If at high volume, attacker only suffers impermanent loss from the tokens.
4. Earlier withdrawers get more tokens. If the attacker withdraws from the pool, they basically are gaining tokens from liquidity providers who have not yet withdrawn from the pool. So there is a constant incentive to withdraw early form the pool.

Due to all these reasons, this should be a valid high. While it is true that the parameters of the curve change over time, value can be extracted form the pool even if the composition isn't changing over time. Here, composition refers to the number of x tokens and y tokens in the pool, which only changes during pool interactions, and not automatically with time.