oxelmiguel - Premature LV Redemption Leading to Asset Loss For user

[sherlock-admin4]

oxelmiguel

Medium

Premature LV Redemption Leading to Asset Loss For user

Summary

According to the Cork protocol's litepaper, Liquidity Vault tokenholders can submit a withdrawal request prior to expiry, which will be processed at expiry and can be claimed by burning the Liquidity Vault Token. However, the current implementation allows users to call VaultCore::redeemExpiredLv before expiry, resulting in the complete loss of user funds.

Root Cause

In VaultLibrary::redeemExpired, if a user calls the function before the expiry and their userEligible balance is greater than or equal to the requested amount, the transaction proceeds without invoking _liquidatedLp(self, dsId, ammRouter, flashSwapRouter).

https://github.com/sherlock-audit/2024-08-cork-protocol/blob/db23bf67e45781b00ee6de5f6f23e621af16bd7e/Depeg-swap/contracts/libraries/VaultLib.sol#L514

 {
            uint256 dsId = self.globalAssetIdx;
            DepegSwap storage ds = self.ds[dsId];

            uint256 userEligible = self.vault.pool.withdrawEligible[owner];

            if (userEligible == 0 && !ds.isExpired()) {
                revert Unauthorized(owner);
            }

            // user can only redeem up to the amount they requested, when there's a DS active
            // if there's no DS active, then there's no cap on the amount of LV that can be redeemed
            if (!ds.isExpired() && userEligible < amount) {
                revert InsufficientBalance(owner, amount, userEligible);
            }

            if (ds.isExpired() && !self.vault.lpLiquidated.get(dsId)) {
                _liquidatedLp(self, dsId, ammRouter, flashSwapRouter);
                assert(self.vault.balances.ra.locked == 0);
            }
        }

Since this function (_liquidatedLp) is responsible for setting the exchange rates (withdrawalPool.raExchangeRate and withdrawalPool.paExchangeRate), these rates remain uninitialized (set to 0).

https://github.com/sherlock-audit/2024-08-cork-protocol/blob/db23bf67e45781b00ee6de5f6f23e621af16bd7e/Depeg-swap/contracts/libraries/VaultLib.sol#L392

As a result, when calculating the user's allocated Redemption Asset (RA) and Pegged Asset (PA), the allocation is zero.

https://github.com/sherlock-audit/2024-08-cork-protocol/blob/db23bf67e45781b00ee6de5f6f23e621af16bd7e/Depeg-swap/contracts/libraries/VaultPoolLib.sol#L137

The function burns Liquidity Vault (LV) tokens, decreases userEligible, but sends 0 RA and PA to the user, leading to a complete loss of funds.

Impact

Users who attempt to call redeemExpiredLv before the Depeg Swap (DS) expiry risk losing all their funds.

PoC

Run the poc in test/contracts/LvCore.ts

 describe("LossFund", function () {
    it("call redeemExpiredLv before expiry ", async function () {

      const { Id, dsId } = await issueNewSwapAssets(expiry);

      let paBalance = await fixture.pa.read.balanceOf(
        [secondSigner.account.address],
      );
      let rabalance = await fixture.ra.read.balanceOf([
        secondSigner.account.address,
      ]);

      console.log("Ra balance of user before deposit ",rabalance);

      await moduleCore.write.depositLv([Id, depositAmount]);
      await moduleCore.write.depositLv([Id, depositAmount], {
        account: secondSigner.account,
      });

      const msgPermit = await helper.permit({
        amount: depositAmount,
        deadline,
        erc20contractAddress: fixture.lv.address!,
        psmAddress: moduleCore.address,
        signer: secondSigner,
      });

      await moduleCore.write.requestRedemption(
        [Id, depositAmount, msgPermit, deadline],
        {
          account: secondSigner.account,
        }
      );

      let lvBalance = await moduleCore.read.lockedLvfor(
        [Id, secondSigner.account.address],
        {
          account: secondSigner.account,
        }
      );

      console.log("LV locked for user before request redemption ",lvBalance);

      await moduleCore.write.redeemExpiredLv(
        [Id, secondSigner.account.address, depositAmount],
        {
          account: secondSigner.account,
        }
      );

      paBalance = await fixture.pa.read.balanceOf(
        [secondSigner.account.address],
      );
      rabalance = await fixture.ra.read.balanceOf([
        secondSigner.account.address,
      ]);

      lvBalance = await moduleCore.read.lockedLvfor(
        [Id, secondSigner.account.address],
        {
          account: secondSigner.account,
        }
      );

      console.log("LV locked for user after redemption ", lvBalance);

      console.log("PA balance of user after redemption ", paBalance);
      console.log("RA balance of user after redemption ", rabalance);
    });
  });

Output

Mitigation

The redeemExpired function is intended to be called at DS expiry. To prevent misuse, a simple fix would be to add a check to ensure it reverts when called before expiry. The redeemEarlyLv function already handles LV redemption before expiry. However, if the current behavior is intended, the internal function logic should be refactored to properly calculate and set the exchange rates even before expiry to avoid zero allocations and loss of user funds.

[ziankork]

This is a valid bug, but we decided to remove this functionality altogether in favor of redeemEarly being the default, implication of this is that user can remove their liquidity at any moment with no fee and this simplifies the LV withdrawals logic

[SakshamGuruji3]

Escalate

The loss of funds here is a consequence of a user performing a particular action which according to Sherlock's rules should be invalidated (user errors are invalid)

[sherlock-admin3]

Escalate

The loss of funds here is a consequence of a user performing a particular action which according to Sherlock's rules should be invalidated (user errors are invalid)

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.

[0xsimao]

The comment above is correct, the user should not make this call if it leads to receiving 0 assets, it's his responsability.

This issue misses the fact that the code as is, which is the source of truth, allows users to redeem before it expires, by using the exchange rates calculated when the last issuance ended. In the first issuance, there was no issuance that ended prior to it, so the exchange rate is 0.

This issue boils down to adding a safety check to ensure users do not withdraw at the first issuance as the exchange rate will be 0. However, users are responsible for their transactions and they should not withdraw before expiry at the first issuance.

[0xNirix]

This issue causes loss of protocol fee and breakage of a core functionality and therefore should be considered valid.

There are two impact of this issue:

1. On the first DS issuance when RA/PA exchange rates are uninitialized, it results in loss of funds for users. This scenario is detailed in the current issue report
2. However in other cases e.g. after subsequent DS issuance, when a user calls redeemExpired before expiry, the protocol suffers fee loss and user gains by not paying any fees for early redemptions. This impact has been detailed in the dup https://github.com/sherlock-audit/2024-08-cork-protocol-judging/issues/88

Details on the second impact: For early redemptions, users must pay a fee. However by exploiting this issue, the users can perform early redemptions without any fees. As a result, the protocol suffers a loss of fees that should have been collected for early redemptions. Users gains as they can receive redemption without incurring the intended early redemption fees, even for requests made within the current active DS period. This undermines the protocol's fee structure and intended mechanics for early redemptions. It could result in significant financial losses for the protocol, especially if many users exploit this issue or if it's exploited during periods of high volatility.

[oxelsubzero]

The root cause of this issue is not due to user actions but rather the protocol’s implementation. The function is designed in a way to be called before expiry and there is an edge case where calling this function will lead to complete loss of funds of user.

// user can only redeem up to the amount they requested, when there's a DS active
    // if there's no DS active, then there's no cap on the amount of LV that can be redeemed
@>         if (!ds.isExpired() && userEligible < amount) {
                revert InsufficientBalance(owner, amount, userEligible);
            }

            if (ds.isExpired() && !self.vault.lpLiquidated.get(dsId)) {
                _liquidatedLp(self, dsId, ammRouter, flashSwapRouter);
                assert(self.vault.balances.ra.locked == 0);
            }

While Sherlock’s rules may consider certain user errors invalid, this bug touches on a design principle

It should be up to the protocol to check if it's first issuance or not.

[AtanasDimulski]

Issue #83 describes how this can be exploited by a malicious actor and results in a theft of funds. This issue is valid although I don't believe the correct report is selected as the primary one, and the issue should be high.

[0xsimao]

83 is actually a different issue and is a duplicate of #166, the ra amount must be decreased in lvRedeemRaWithCtDs(). If this is done, #83 is fixed.

This issue, #27, does not break anything, it works as intended, explained here. The protocol chose to not collect a fee when withdrawing before expiry, using funds from previous issuances, which is valid.

[0xNirix]

According to Cork Litepaper, this https://github.com/sherlock-audit/2024-08-cork-protocol-judging/issues/27#issuecomment-2379659101 is incorrect.

The cork litepaper specifically mentions: Prior to the expiry,

Liquidity Vault tokenholders can request a withdrawal. The request will be processed at expiry and can thereafter be claimed in exchange for burning the Liquidity Vault Token.

And if an early withdrawals is being done, then a fee must be levied.

The amount of Redemption Asset attributed per Liquidity Vault token from steps 1-4, minus a fee can be claimed by the Liquidity Vault tokenholder in exchange for burning their Liquidity Vault token

PS: The cork litepaper online has apparently now (due to the change sponsor mentioned in https://github.com/sherlock-audit/2024-08-cork-protocol-judging/issues/27#issuecomment-2367464118) but this is the text at the time of audit.

[0xsimao]

As mentioned before, the docs are not the source of truth, the code is, and it was built to allow redeeming before expiry.

[oxelsubzero]

Although the code allows for exchange before expiration, it does not handle the case where the rate is zero, resulting in loss of user funds.

[oxelsubzero]

If users are supposed to call this function before expiration, I don't see the "user fault" that invalidates the issue.

[0xNirix]

The Sherlock rules says README takes precedence over code.

Hierarchy of truth: If the protocol team provides no specific information, the default rules apply (judging guidelines).

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules. In case of contradictions between the README and CODE COMMENTS, the README is the chosen source of truth.

And readme specifically refers to litepaper as design decisions

Please discuss any design choices you made.

Design decisions are discussed in the litepaper: https://corkfi.notion.site/Depeg-Swaps-Litepaper-f21a57d5c19d48209dfa0f0c2ab776c4

Also, agree with https://github.com/sherlock-audit/2024-08-cork-protocol-judging/issues/27#issuecomment-2379735614, either ways it should be considered a valid issue.

[0xsimao]

Yes but it's not the readme, which is what matters. This whole issue is because the user accepts sending a transaction that gives him 0 assets in return. In the following issuances, it is correct and expected.

[oxelsubzero]

Yes but it's not the readme, which is what matters. This whole issue is because the user accepts sending a transaction that gives him 0 assets in return. In the following issuances, it is correct and expected.

This is obviously not correct or expected behavior.

[cvetanovv]

I had hesitations whether to leave it Medium or invalid(user error).

The reason I decided it was valid is because it is the expected behavior from the protocol, and it is in the documentation.

If the protocol tells the users that they can call the function before it expires,s and they do, but because a bug in the code causes them to lose funds, then is it their fault, or is the problem in the code?

[0xsimao]

The reason I decided it was valid is because it is the expected behavior from the protocol, and it is in the documentation.

It is not, the code is the source of truth. Look at the function code's and comment:

// user can only redeem up to the amount they requested, when there's a DS active
    // if there's no DS active, then there's no cap on the amount of LV that can be redeemed
           if (!ds.isExpired() && userEligible < amount) {
                revert InsufficientBalance(owner, amount, userEligible);
            }

            if (ds.isExpired() && !self.vault.lpLiquidated.get(dsId)) {
                _liquidatedLp(self, dsId, ammRouter, flashSwapRouter);
                assert(self.vault.balances.ra.locked == 0);
            }

The code handles the case before expiry explicitly and even adds a comment saying

user can only redeem up to the amount they requested, when there's a DS active

When there's a DS active means it is before expiry. So they consider withdrawals before expiry. The docs are outdated and the code is the source of truth.

Now that this is clear, users only get 0 if the exchange rate is 0 and they still proceed, which is obviously their mistake.

[oxelsubzero]

The current code and documentation imply that a user can call this function before expiry, but the fact is that the exchangeRate is not set during the first issuance results in the user losing all their funds. This is clearly a bug, not a user error. If this behavior is intended, the documentation should explicitly state that the function can only be called before expiry when it's not the first issuance. Alternatively, the code should include a check to revert the transaction if it is the first issuance. It is not reasonable to expect users to redeem their LV tokens for zero allocation (maybe the documentation or comment should say in this case it is a donation)

[oxelsubzero]

The documentation and the code currently suggests that users can call this function to redeem their LV tokens, implying that it should work as intended in all cases before expiry. However, if the documentation were to specify that users can only call this function when it's not the first issuance, then it would shift the responsibility to the user, making it a user error if they attempt to redeem during the first issuance. This distinction is crucial—without clear guidance or safeguards in the code, users may face unintended loss of funds.

[oxelsubzero]

When there's a DS active means it is before expiry. So they consider withdrawals before expiry. The docs are outdated and the code is the source of truth.

Since the code allows withdrawals before expiry, it should handle every scenario properly, correct? Does this mean it's an intended feature for users to lose their LV tokens, or is this actually a bug in the code?

[0xsimao]

It works as intended, it withdraws according to the exchange rate. As the exchange rate is 0, users get 0. Users must check the exchange rate before withdrawing.

[oxelsubzero]

It works as intended, it withdraws according to the exchange rate. As the exchange rate is 0, users get 0. Users must check the exchange rate before withdrawing.

How can users check the exchange rate if there are no entry points to check the exchange rate? You assume this is the expected behavior when the developer has commented that this is not expected and nowhere in the code or documentation does it say that calling this function in some cases can lead to loss of funds and that this is acceptable. If the function is supposed to be called before expiration, it should not result in loss of funds for users in some cases or this behavior should be clearly documented because the impact is not negligible.

[0xsimao]

How can users check the exchange rate if there are no entry points to check the exchange rate?

Just because there may not be a getter does not mean this information can not be fetched (events, direct storage reads).

You assume this is the expected behavior when the developer has commented

The expected behaviour is returning an amount according to the exchange rate, which is what is done.

If the function is supposed to be called before expiration, it should not result in loss of funds for users in some cases or this behavior should be clearly documented because the impact is not negligible.

Users know they get their funds according to the exchange rate, so it is known.

According to the Sherlock rules this is user mistake, they should not call it if they do not like the exchange rate, whether is is 0, 0.1, 0.2, and so on.

[oxelsubzero]

Can you state the event? Are you talking as if the regular user of the protocol is going to read the storage directly from the blockchain? If users are supposed to use this feature, they should clearly state that it may result in loss of funds or maybe implement getters to allow the user to use it at a comfortable exchange rate? This is not intended and even the sponsor says it is not intended. I don't understand then why you say this is user error. Nothing in the protocols suggests this is user error

[0xsimao]

Regular users should use the frontend, which has the exchange rate info. Users that interact directly with the protocol should get the exchange rate themselves.

[oxelsubzero]

Regular users should use the frontend, which has the exchange rate info

So why didn't the sponsor say this and say this behavior was not expected??

[0xsimao]

So why didn't the sponsor say this and say this behavior was not expected??

They decided to remove some of the the functionality, but it is user mistake anyway. The sponsor's input is not required here as 1. The code handles redemptions before expiry. 2. Redemptions use the exchange rate. 3. Users should not redeem with an exchange rate they do not like.

[oxelsubzero]

Regular users should use the frontend, which has the exchange rate info

If this is to be handled by the frontend, why does the sponsor say it's a valid bug and not say so?

3. Users should not redeem with an exchange rate they do not like.

why did you say that if neither the code, nor the litepaper, nor the readme indicate it

You are making a statement without any proof.

[0xsimao]

You are making a statement without any proof.

I do not know what proof you are referring to. The withdrawal is processed with this exchange rate. Obviously if users do not like the exchange rate they should not call it.

[oxelsubzero]

Obviously if users do not like the exchange rate they should not call it.

users should use the frontend, which has the exchange rate info.

So why did you say that the exchange rate can be retrieved on the frontend and if the user doesn't like the rate, he shouldn't call the function? I don't know where you got that statement from, can the sponsor confirm it? If so, I can't argue anymore. Anyway, the judge has the last word.

[0xsimao]

So why did you say that the exchange rate can be retrieved on the frontend

If the frontend does not show what users would receive when they redeem, then it is the frontend that is missing a key functionality. Users should not blindly call functions without knowing how much they will receive in return.

This issue is basically a safety check. The exchange rate goes from 0 to something. If it is 0, we can assume users will never want to redeem, so we can revert if someone tries. But, users should not redeem if they do not agree with the exchange rate in the first place.