0x73696d616f - Users redeeming early will withdraw Ra without decreasing the amount locked, which will lead to stolen funds when withdrawing after expiry

[sherlock-admin3]

0x73696d616f

High

Users redeeming early will withdraw Ra without decreasing the amount locked, which will lead to stolen funds when withdrawing after expiry

Summary

VaultLib::redeemEarly() is called when users redeem early via Vault::redeemEarlyLv(), which allows users to redeem Lv for Ra and pay a fee.

In the process, the Vault burns Ct and Ds in VaultLib::_redeemCtDsAndSellExcessCt() for Ra, by calling PsmLib::PsmLibrary.lvRedeemRaWithCtDs(). However, it never calls RedemptionAssetManagerLib::decLocked() to decrease the tracked locked Ra, but the Ra leaves the Vault for the user redeeming.

This means that when a new Ds is issued in the PsmLib or users call PsmLib::redeemWithCt(), PsmLib::_separateLiquidity() will be called and it will calculated the exchange rate to withdraw Ra and Pa as if the Ra amount withdrawn earlier was still there. When it calls self.psm.balances.ra.convertAllToFree(), it converts the locked amount to free and assumes these funds are available, when in reality they have been withdrawn earlier. As such, the Ra and Pa checkpoint will be incorrect and users will redeem more Ra than they should, such that the last users will not be able to withdraw and the first ones will profit.

Root Cause

In PsmLib.sol:125, self.psm.balances.ra.decLocked(amount); is not called.

Internal pre-conditions

None.

External pre-conditions

None.

Attack Path

1. User calls Vault::redeemEarlyLv() or ModuleCore::issueNewDs() is called by the admin.

Impact

Users withdraw more funds then they should via PsmLib::redeemWithCt() meaning the last users can not withdraw.

PoC

PsmLib::lvRedeemRaWithCtDs() does not reduce the amount of Ra locked.

function lvRedeemRaWithCtDs(State storage self, uint256 amount, uint256 dsId) internal {
    DepegSwap storage ds = self.ds[dsId];
    ds.burnBothforSelf(amount);
}

Mitigation

PsmLib::lvRedeemRaWithCtDs() must reduce the amount of Ra locked.

function lvRedeemRaWithCtDs(State storage self, uint256 amount, uint256 dsId) internal {
    self.psm.balances.ra.decLocked(amount);
    DepegSwap storage ds = self.ds[dsId];
    ds.burnBothforSelf(amount);
}

[ziankork]

dup of #44 and related #156 . will fix

[SakshamGuruji3]

Escalate

It should be dupe of #126 , Even though this report revolves around lvRedeemRaWithCtDs, the root cause concerns incorrect state updates for redemption assets.
There are reports that discuss the same incorrect state updates for ra but for different parts of the protocol, involving different functions. I believe we need to group all of these together. For example, the protocol misses exchange range accounting in multiple areas, and all of these instances are grouped in a single report (#119). We should do the same with this issue.

[sherlock-admin3]

Escalate

It should be dupe of #126 , Even though this report revolves around lvRedeemRaWithCtDs, the root cause concerns incorrect state updates for redemption assets.
There are reports that discuss the same incorrect state updates for ra but for different parts of the protocol, involving different functions. I believe we need to group all of these together. For example, the protocol misses exchange range accounting in multiple areas, and all of these instances are grouped in a single report (#119). We should do the same with this issue.

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 fix here is different in both issues, adding self.psm.balances.ra.decLocked(amount); here and self.psm.balances.ra.lockfrom(amount, buyer); instead of self.psm.balances.ra.lockUnchecked(amount, buyer); in the other.

#119 uses the same fix for everything, fetching the exchange rate and multiplying/dividing by it.

[SakshamGuruji3]

But in the end it falls under the same thing "locked/Unlocked accounting" just like multiplying/dividing the exchange rate , isnt that the same thing

[0xsimao]

There are multiple ways to do incorrect ra tracking but not taking the exchange rate into account is always the same bug.

[KupiaSecAdmin]

I agree with @SakshamGuruji3.

Issues #126, #155, and #166 (this one) all share the same root cause. They should be consolidated into a single issue.

[0xsimao]

"Wrong accounting" is not a root cause. The fixes are all different. Both in the code and conceptually.

[sherlock-admin2]

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

[AtanasDimulski]

Agree with @0xsimao wrong accounting can't be considered as the same root cause. Moreover, if you don't agree with how #119 and its dups are grouped you should have escalated each one of them. However not taking the exchange rate into account is the same as lack of slippage protection.

[SakshamGuruji3]

The argument is still not convincing , first of all 119 has been reported correctly we can not submit 5 vulnerabilities separately for the same root cause (incorrect exchange rate accounting) , secondly the root cause is here the same "not accounting for locked/unlocked RA correctly" and should be grouped together

[0xsimao]

The fixes are all different, in the code and conceptually.

#126 calls the wrong function, lockUnchecked() instead of lockFrom().
#155 receives an amount of ra and increases the ra locked by this full amount, without discounting the fee.
#166 it's missing a call to self.psm.balances.ra.decLocked(amount); when redeeming ra.

The exchange rate issues are all missing taking into account the exchange rate and the fix is the exact same for all.

These 3 mentioned issues have different fixes because we may need to add more or less ra depending on the function (the wrong amount is not always the same) whereas taking into account the exchange rate is a fixed amount and always the same fix.

[WangSecurity]

From my understanding, reports #126 and #155 lead to a similar impact, but the underlying reason and root causes are different in all situations. I also agree with @0xsimao and @AtanasDimulski that wrong accounting is not a correct root cause and is too broad. Hence, all the issues have to remain separate, because while the reports lead to similar impacts, they happen due to different root causes, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result:
High
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• SakshamGuruji3: rejected