c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/CdpManager.sol#L320-L328

Vulnerability details

Impact

I came across the following in the main invariants: R-07: TCR should not decrease after redemptions.

Proof of Concept

The first CDP with the lowest ICR higher than the MCR can be redeemed.

Let's imagine the following scenario: There are 3 CDPs with the same debts and different collateral shares.

ICR[0] = 130%, ICR[1] = 128%, ICR[2] = 105%

Then

originalTCR = 121% ( = (130 + 128 + 105) / 3 ) // 3 CDPs has the same debts (denominators)

This implies that the second CDP can be redeemable. After redeeming the second CDP, the TCR obviously falls below the original TCR.

TCR = (ICR[0] + ICR[2]) / 2 = 120% < 121% (originalTCR)

Tools Used

Recommended Mitigation Steps

We can add below check to the redeemCollateral function:

function redeemCollateral() {
    ...
    _syncGracePeriodForGivenValues(
        totals.systemCollSharesAtStart - totals.collSharesDrawn - totals.totalCollSharesSurplus,
        totals.systemDebtAtStart - totals.debtToRedeem,
        totals.price
    );

    + uint256 newTcr = _computeTCRWithGivenSystemValues(
    +     totals.systemCollSharesAtStart - totals.collSharesDrawn - totals.totalCollSharesSurplus,
    +     totals.systemDebtAtStart - totals.debtToRedeem,
    +     totals.price
    + );

    + require(totals.tcrAtStart <= newTcr, "");
}

After adding this, we observe that some tests will fail. This indicates that we are allowing the TCR to fall below the original TCR after redemption at this point, violating the main invariants.

Assessed type

Error

c4-pre-sort commented 11 months ago

bytes032 marked the issue as insufficient quality report

bytes032 commented 11 months ago

No proof of impact

c4-sponsor commented 11 months ago

GalloDaSballo (sponsor) disputed

GalloDaSballo commented 11 months ago

We have shared broken invariants and this one also doesn't demonstrate a valid impact, this is not weaponized

jhsagd76 commented 11 months ago

Just mark it as a dup of #199 , although they are a little different. Pls check the comment I left in the #199 and prove it should be a MED.