0x73696d616f - Admin new issuance or user calling Vault::redeemExpiredLv() after Psm::redeemWithCt() will lead to stuck funds when trying to withdraw

[sherlock-admin4]

0x73696d616f

High

Admin new issuance or user calling Vault::redeemExpiredLv() after Psm::redeemWithCt() will lead to stuck funds when trying to withdraw

Summary

VaultLib::_liquidatedLp() calls PsmLib::lvRedeemRaWithCtDs(), which redeems ra with ct and ds. However, if PsmLib::_separateLiquidity() has already been called, this will lead to an incorrect tracking of funds as PsmLib::_separateLiquidity() checkpointed the total supply of ct, but the Vault will redeem some of it using PsmLib::lvRedeemRaWithCtDs(), leading to some pa that will never be withdrawn and the vault withdraws too many Ra, which means users will not be able to redeem their ct for ra and pa as ra has been withdrawn already and it reverts.

Root Cause

In VaultLib.sol:377, it calls PsmLib::lvRedeemRaWithCtDs() even if PsmLib::_separateLiquidity() has already been called. It should skip it in this case.

Internal pre-conditions

1. PsmLib::_separateLiquidity() needs to be called before VaultLib::_liquidatedLp(), which may be done on a new issuance when the admin calls ModuleCore::issueNewDs() or by users calling Psm::redeemWithCT() before Vault::redeemExpiredLv().

External pre-conditions

None.

Attack Path

1. Admin calls ModuleCore::issueNewDs().
   Or users call Psm::redeemWithCT() before Vault::redeemExpiredLv().

Impact

As the Vault withdraws Ra after the checkpoint and burns the corresponding Ct tokens, it will withdraw too many ra and not withdraw the pa it was entitled to.

PoC

ModuleCore::issueNewDs() calls PsmLib::onNewIssuance() before VaultLib::onNewIssuance() always triggering this bug.

function issueNewDs(Id id, uint256 expiry, uint256 exchangeRates, uint256 repurchaseFeePrecentage)
    external
    override
    onlyConfig
    onlyInitialized(id)
{
    ...
    PsmLibrary.onNewIssuance(state, ct, ds, ammPair, idx, prevIdx, repurchaseFeePrecentage);

    getRouterCore().onNewIssuance(id, idx, ds, ammPair, 0, ra, ct);

    VaultLibrary.onNewIssuance(state, prevIdx, getRouterCore(), getAmmRouter());
    ...
}

PsmLib::_separateLiquidity() checkpoints ra and pa based on ct supply:

function _separateLiquidity(State storage self, uint256 prevIdx) internal {
    ...
    self.psm.poolArchive[prevIdx] = PsmPoolArchive(availableRa, availablePa, IERC20(ds.ct).totalSupply());
    ...
}

VaultLib::_liquidatedLp() redeems ra with ct and ds when it should have skipped it has liquidity has already been checkpointed in PsmLib::_separateLiquidity().

function _liquidatedLp(
    State storage self,
    uint256 dsId,
    IUniswapV2Router02 ammRouter,
    IDsFlashSwapCore flashSwapRouter
) internal {
    ...
    PsmLibrary.lvRedeemRaWithCtDs(self, redeemAmount, dsId);

    // if the reserved DS is more than the CT that's available from liquidating the AMM LP
    // then there's no CT we can use to effectively redeem RA + PA from the PSM
    uint256 ctAttributedToPa = reservedDs >= ctAmm ? 0 : ctAmm - reservedDs;

    uint256 psmPa;
    uint256 psmRa;

    if (ctAttributedToPa != 0) {
        (psmPa, psmRa) = PsmLibrary.lvRedeemRaPaWithCt(self, ctAttributedToPa, dsId);
    }

    psmRa += redeemAmount;

    self.vault.pool.reserve(self.vault.lv.totalIssued(), raAmm + psmRa, psmPa);
}

Mitigation

If the liquidity has been separated, skip redeeming ra for ct and ds.

function _liquidatedLp(
    State storage self,
    uint256 dsId,
    IUniswapV2Router02 ammRouter,
    IDsFlashSwapCore flashSwapRouter
) internal {
    ...
    if (!self.psm.liquiditySeparated.get(prevIdx)) {
        PsmLibrary.lvRedeemRaWithCtDs(self, redeemAmount, dsId);
    }
    ...
}

[ziankork]

related to #44, the function that's problematic here is PsmLibrary::lvRedeemRaWithCtDs. will fix

[0xsimao]

escalate

This issue is high severity because it has no external dependencies, it will happen whenver new DS tokens are issued.

[sherlock-admin2]

escalate

This issue is high severity because it has no external dependencies, it will happen whenver new DS tokens are issued.

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.

[sherlock-admin4]

Escalate,
This issue is invalid

You've deleted an escalation for this issue.

[0xsimao]

The problem is not in PsmLibrary::lvRedeemRaWithCtDs, but in VaultLib::_liquidatedLp(). Even if it was, just because it is the same function does not mean they are dups.

[cvetanovv]

The reason why this issue is Medium is because it has an external condition. This issue can only happen under certain conditions mentioned in the issue.

PsmLib::_separateLiquidity() needs to be called before VaultLib::_liquidatedLp(), which may be done on a new issuance when the admin calls ModuleCore::issueNewDs() or by users calling Psm::redeemWithCT() before Vault::redeemExpiredLv().

[0xsimao]

@cvetanovv that is not an external condition, it's within the protocol (same contract to be more precise).
There are 2 ways for this to happen:

1. Any user can force this order of events at no cost by calling Psm::redeemWithCT() before Vault::redeemExpiredLv().
2. ModuleCore::issueNewDs() is called, in which it always calls PsmLib::_separateLiquidity() before VaultLib::_liquidatedLp().

So 2 means it always happens and 1 means anyone may trigger this. Which means this has internal conditions but they are not extensive, can be easily triggered by 1 and will always be triggered by 2.

[davies0212]

Skipping lvRedeemRaWithCtDs() is not a correct mitigation. Instead, the raAccrued and paAccrued values of the self.psm.poolArchive[dsId] struct should be reduced.

This issue arises from improper tracking of RA and PA amounts and should be considered a duplicate of #166, #126, or #155.

[0xsimao]

Skipping lvRedeemRaWithCtDs() is not a correct mitigation. Instead, the raAccrued and paAccrued values of the self.psm.poolArchive[dsId] struct should be reduced.

There may be several mitigations, it's easier to verify the pr.
But these linked issues are different than this one.

[davies0212]

There may be several mitigations, it's easier to verify the pr.
But these linked issues are different than this one.

For this issue, there are not two mitigations. Reducing the raAccrued and paAccrued values of self.psm.poolArchive[dsId] is the correct approach.

Additionally, mitigating in a different area does not mean that the issues are separate; instead, we should focus on the root cause.

[0xsimao]

For this issue, there are not two mitigations. Reducing the raAccrued and paAccrued values of self.psm.poolArchive[dsId] is the correct approach.

I would have to check the pr to confirm.

For this issue, there are not two mitigations. Reducing the raAccrued and paAccrued values of self.psm.poolArchive[dsId] is the correct approach.

Sherlock duplication rules require the root cause and attack path to be the same, which they are not. There are many different ways to make the accounting wrong. Even then, there are several assets in the protocol, ra, pa and ct.

This issue is related to ct accounting specifically, not ra or pa, as the ct attributed to the vault would not be used to redeem via lvRedeemRaPaWithCt(), but using lvRedeemRaWithCtDs() instead. This means a portion of self.psm.poolArchive[prevIdx] would never be redeemed, being stuck, as the ct that was supposed to redeem it pro-rata would be burned in lvRedeemRaWithCtDs().

[davies0212]

Sherlock duplication rules require the root cause and attack path to be the same, which they are not. There are many different ways to make the accounting wrong. Even then, there are several assets in the protocol, ra, pa and ct.

Issues related to accounting for the exchange rate as 1:1 were all grouped together. If you assert as stated above, they should also be separated.

This issue is related to ct accounting specifically, not ra or pa, as the ct attributed to the vault would not be used to redeem via lvRedeemRaPaWithCt(), but using lvRedeemRaWithCtDs() instead. This means a portion of self.psm.poolArchive[prevIdx] would never be redeemed, being stuck, as the ct that was supposed to redeem it pro-rata would be burned in lvRedeemRaWithCtDs().

No. The ct attributed to the vault should be used to redeem ra. The ct attributed to the vault belongs to the lv holders (who contributed to the Uniswap V2 pool). They sent ra to the vault, and some of that ra was exchanged for (ct + ds). The generated ct was provided to the Uniswap V2 pool along with the remaining ra, and the generated ds were sent to the FlashSwapRouter.

Therefore, when issuing new ds, the ct liquidated from the Uniswap V2 pool and the ds from the FlashSwapRouter should be reexchanged for ra. If they are not reexchanged, it will result in a loss of funds for the lv holders.

Thus, skipping lvRedeemRaWithCtDs() is not a correct mitigation. Instead, the raAccrued value of self.psm.poolArchive[prevIdx] should be reduced.

[0xsimao]

Issues related to accounting for the exchange rate as 1:1 were all grouped together. If you assert as stated above, they should also be separated.

But that is always the same fix, taking the exchange rate into account.

No. The ct attributed to the vault should be used to redeem ra. The ct attributed to the vault belongs to the lv holders (who contributed to the Uniswap V2 pool).

Lv holders just hold lv, the lps are held by the vault. Users do not hold all ct at this point, some of it is in the univ2 pool, a part of it owned by the vault and the corresponding lps.

They sent ra to the vault, and some of that ra was exchanged for (ct + ds). The generated ct was provided to the Uniswap V2 pool along with the remaining ra, and the generated ds were sent to the FlashSwapRouter.

Exactly, the vault holds the lps, whose ct is in the uni v2 pool.

Therefore, when issuing new ds, the ct liquidated from the Uniswap V2 pool and the ds from the FlashSwapRouter should be reexchanged for ra. If they are not reexchanged, it will result in a loss of funds for the lv holders.
Thus, skipping lvRedeemRaWithCtDs() is not a correct mitigation. Instead, the raAccrued value of self.psm.poolArchive[prevIdx] should be reduced.

Again, I am not discussing the mitigation, but the bug is in the ct amount held by the vault. Look at the order of events

    PsmLibrary.onNewIssuance(state, ct, ds, ammPair, idx, prevIdx, repurchaseFeePrecentage);

    getRouterCore().onNewIssuance(id, idx, ds, ammPair, 0, ra, ct);

    VaultLibrary.onNewIssuance(state, prevIdx, getRouterCore(), getAmmRouter());

Firstly, PsmLibrary.onNewIssuance() takes a checkpoint of the ct balances self.psm.poolArchive[prevIdx] = PsmPoolArchive(availableRa, availablePa, IERC20(ds.ct).totalSupply());. However, the vault was not liquidated yet, so some of the ct is still in the univ2 pool, and some of that is owned by the vault, that has lps. But the checkpoint is using the full ct totalSupply(). So IERC20(ds.ct).totalSupply()); includes ct owned by the vault (as LP). Let's say that the total ct supply is 100 and the vault's lp is equivalent to 20 ct.

Secondly, VaultLibrary.onNewIssuance() is called, which calls _liquidatedLp(). It burns the lps and gets the corresponding ct and ra via __liquidateUnchecked(). Assume it received the mentioned 20 ct. Then it calls PsmLibrary.lvRedeemRaWithCtDs(), which burns the 20 ct. So the total supply of ct is 80.

Lastly, as there was enough reserve, no ct is left to redeem with lvRedeemRaPaWithCt().

If we look into PsmLibrary.lvRedeemRaPaWithCt(), the amounts to redeem are calculated as (it boils down to MathHelper::calculateAccrued():

accrued = (amount * (available * 1e18) / totalCtIssued) / 1e18;

totalCtIssued, is the checkpointed ct before the vault was liquidated, 100. But, there is only 80 ct in circulation due to the vault having burned it. Thus, 20% of this will be stuck.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
Cork-Technology/Depeg-swap#82

[WangSecurity]

About the severity, @0xsimao could you elaborate on the impact:

As the Vault withdraws Ra after the checkpoint and burns the corresponding Ct tokens, it will withdraw too many ra and not withdraw the pa it was entitled to.

What would be the approximate loss here?

About the duplication, #166, #126 and #155 have different root causes, and this report shouldn't be a duplicate of any of these. Please correct me if it's wrong.

[0xsimao]

What would be the approximate loss here?

Whatever the % the vault has of the ct total supply, as it will never be redeemed. Should be a high % given that users are incentivized to provide liquidity through the vault which accrues additional fees. Liquidity is provided by sending ra and ct, so by providing liquidity the vault also holds more ct when it burns the lps in the end.

[WangSecurity]

Thank you for this clarification. Since the loss can indeed be high and the constraints in the report are not extensive, planning to accept the escalation and upgrade the severity to high.

[davies0212]

In the PsmLibrary.lvRedeemRaPaWithCt() function, it calls _beforeCtRedeem() function

    function lvRedeemRaPaWithCt(State storage self, uint256 amount, uint256 dsId)
        internal
        returns (uint256 accruedPa, uint256 accruedRa)
    {
        // we separate the liquidity here, that means, LP liquidation on the LV also triggers
        _separateLiquidity(self, dsId);

        uint256 totalCtIssued = self.psm.poolArchive[dsId].ctAttributed;
        PsmPoolArchive storage archive = self.psm.poolArchive[dsId];

        (accruedPa, accruedRa) = _calcRedeemAmount(amount, totalCtIssued, archive.raAccrued, archive.paAccrued);

@>    _beforeCtRedeem(self, self.ds[dsId], dsId, amount, accruedPa, accruedRa);
    }

and in the _beforeCtRedeem() function, it reduces ctAttributed (which is the totalCtIssued).

    function _beforeCtRedeem(
        State storage self,
        DepegSwap storage ds,
        uint256 dsId,
        uint256 amount,
        uint256 accruedPa,
        uint256 accruedRa
    ) internal {
        ds.ctRedeemed += amount;
@>    self.psm.poolArchive[dsId].ctAttributed -= amount;
        self.psm.poolArchive[dsId].paAccrued -= accruedPa;
        self.psm.poolArchive[dsId].raAccrued -= accruedRa;
    }

So, no stuck amount.

[0xsimao]

The stuck amount is significant because all the ct the vault held will not be able to redeem the ra and pa tokens.

It is stuck because the checkpoint is taken before the vault burns its ct. So the decrease mentioned here will never fully happen because the vault has burned its ct. So ra and pa will be stuck.

The checkpoint is taken before, on PsmLibrary.onNewIssuance(), which calls PsmLib::_separateLiquidity(), taking the checkpoint.

Then, it calls VaultLib::onNewIssuance(), calling VaultLib::_liquidatedLp(), which calls first PsmLib::lvRedeemRaWithCtDs(), calling ds.burnBothFromSelf(), which burns the vault's ct amount.

As such, the checkpointed total ct will have a component that was burned and will never be redeemed.

[davies0212]

@0xsimao, you are correct that ctAttributed does not fully decrease. However, I disagree with the assertion that the severity is high.

To summarize, both raAccrued and ctAttributed do not fully decrease.

As a result, we cannot conclude that ra will always be stuck, since both raAccrued and ctAttributed remain unchanged.

Regarding pa, the undecreased ct amount depends on the ds amount from the FlashSwapRouter. If the ds amount from the FlashSwapRouter is small, the stuck amount will also be small. In fact, if the ds amount is zero, there will be no stuck amount at all.

Therefore, the severity of this issue should be considered medium.

[0xsimao]

Both ra and pa accrued will never decrease, as there is not enough ct to redeem them.
There is a high chance there will be reserve so it is not a significant condition. Every time liquidity is deposited in the vault, it adds the ra amount not sent to the univ2 pool to the reserve. So unless everyone stops depositing to the vault and a lot of swaps start occuring that drain the reserve, this issue will occur.

[WangSecurity]

My decision remains unchanged, even though there are some constraints to trigger the issue. They're not extensive, and the issue could easily happen, leading to completely stuck tokens. Hence, the decision remains: accept the escalation and upgrade severity to high.

[WangSecurity]

Result:
High
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xsimao: accepted