0xnbvc - Incorrect amount parameter passed to RedemptionAssetManagerLibrary.incLocked() leads to incorrect amount of Redemption Asset tokens being locked and DoS of PsmCore.redeemRaWithDs() and PsmCore.redeemRaWithCtDs()

[sherlock-admin3]

0xnbvc

High

Incorrect amount parameter passed to RedemptionAssetManagerLibrary.incLocked() leads to incorrect amount of Redemption Asset tokens being locked and DoS of PsmCore.redeemRaWithDs() and PsmCore.redeemRaWithCtDs()

Summary

ctAmount is passed to RedemptionAssetManagerLibrary.incLocked() instead of raAmount, leading to incorrect amount of Redemption Asset tokens being locked, leading to DoS of PsmCore.redeemRaWithDs() and PsmCore.redeemRaWithCtDs()

Vulnerability Detail

RedemptionAssetManagerLibrary.incLocked() is called by PsmLibrary.unsafeIssueToLv() to increase the value of the locked Redemption Asset tokens variable, and should be increased by the raAmount passed in VaultLibrary.__provideLiquidity().
However it is passed the ctAmount instead.

function __provideLiquidity(
    State storage self,
->  uint256 raAmount,
->  uint256 ctAmount,
    IDsFlashSwapCore flashSwapRouter,
    address ctAddress,
    IUniswapV2Router02 ammRouter,
    uint256 dsId
) internal {
    // no need to provide liquidity if the amount is 0
    if (raAmount == 0 && ctAmount == 0) {
        return;
    }

->  PsmLibrary.unsafeIssueToLv(self, ctAmount);

->  function unsafeIssueToLv(State storage self, uint256 amount) internal {
        uint256 dsId = self.globalAssetIdx;

        DepegSwap storage ds = self.ds[dsId];

->      self.psm.balances.ra.incLocked(amount);

Thus, when raAmount is > ctAmount, the locked variable will be increased by the incorrect (lower than what it should be) amount of Redemption Asset tokens.
It will lead to a DoS of PsmCore.redeemRaWithDs() and PsmCore.redeemRaWithCtDs() that both call RedemptionAssetManagerLibrary.unlockTo() to decrease the value of the locked variable, by a superior amount than the locked variable actually contains.

function unlockTo(PsmRedemptionAssetManager storage self, address to, uint256 amount) internal {
->  decLocked(self, amount);
    unlockToUnchecked(self, amount, to);
}

function decLocked(PsmRedemptionAssetManager storage self, uint256 amount) internal {
->  self.locked = self.locked - amount;
}

Impact

DoS due to wrong accounting of the locked variable.

Code Snippet

PsmLib.sol#L120

Tool used

Manual Review

Recommendation

Modify PsmLibrary.unsafeIssueToLv() to accept 2 parameters: raAmount and ctAmount and pass the correct raAmount to RedemptionAssetManagerLibrary.incLocked().

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

For the beginning of flow you gave Library functions. So I can't track if it's valid. Escalate with a better explanation if you want.

[0xnbvc]

Escalate
This is a duplicate of #155

[sherlock-admin3]

Escalate
This is a duplicate of #155

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page,
in the Sherlock webapp.

[cvetanovv]

Escalate above comment

[sherlock-admin3]

Escalate above comment

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 code is correct, this issue got it wrong.
When liquidity is provided, the ra is split according to the ratio to the amount to go to liquidity and the amount to issue ct + ds. The ratio at which they are split is given by

function calculateProvideLiquidityAmountBasedOnCtPrice(uint256 amountra, uint256 priceRatio)
    external
    pure
    returns (uint256 ra, uint256 ct)
{
    ct = (amountra * 1e18) / (priceRatio + 1e18);
    ra = (amountra - ct);

    assert((ct + ra) == amountra);
}

Where amountra is the ra provided. As we can see, the ra amount is (amountra - ct);, which means the ct amount that is provided to the amm is also the ra amount (because they add up to the ra provided) leftover from adding liquidity to the amm. This leftover ra is equal to the ct provided and has not been provided to the amm, so it is used to issue ct + ds.

[WangSecurity]

As I understand, the above comment is correct and the issue is invalid. Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• cvetanovv: rejected