c4-submissions commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/LiquidationLibrary.sol#L153-L156

Vulnerability details

Impact

I came across the following in the main invariants: L-12: TCR must increase after liquidation with no redistributions But in some cases, the TCR may fall below the original TCR when partially liquidating.

Proof of Concept

Let's consider a scenario where there is a CDP with an ICR below the LICR of 103%. A user attempts to partially liquidate this CDP. In this case, the ICR falls below the original ICR, resulting in a decrease in the TCR as well.

old_ICR = 102%
old_eBTC = X
old_coll = 1.02 X

partial_eBTC = 0.5 X
collToLiquidator = 0.5 * 1.03 X = 0.515 X

new_eBTC = 0.5X
new_coll = 1.02 X - collToLiquidator = 0.505 X
new_ICR = new_coll / new_eBTC = 101%

new_ICR < old_ICR

Tools Used

Recommended Mitigation Steps

Add below check to _liquidateIndividualCdpSetupCDP function:

function _liquidateIndividualCdpSetupCDP() {
    if (
        liquidationValues.totalCollToSendToLiquidator == 0 &&
        liquidationValues.debtToBurn == 0
    ) {
        ...
    }
    + else {
    +       uint256 newTCR = _computeTCRWithGivenSystemValues(
    +           _recoveryState.entireSystemColl - liquidationValues.totalCollToSendToLiquidator,
    +           _recoveryState.entireSystemDebt - liquidationValues.debtToBurn,
    +           _recoveryState.price
    +       );
    +       require(_liqState.TCR <= 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 partial liquidation at this point, violating the main invariants.