In recovery mode, liquidation can bypass the grace period.

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

A CDP with a ICR below the MCR may be liquidated without undergoing grace period checks in the recovery mode. This significantly violates the protocol.

Proof of Concept

When a user attempts to liquidate an individual CDP, we only check the grace period when the ICR is greater than the MCR.

function _liquidateIndividualCdpSetup(
    bytes32 _cdpId,
    uint256 _partialAmount,
    bytes32 _upperPartialHint,
    bytes32 _lowerPartialHint
) internal {
    if (_ICR >= MCR) {
       // We must be in RM
       require(
           _checkICRAgainstLiqThreshold(_ICR, _TCR),
            "LiquidationLibrary: ICR is not below liquidation threshold in current mode"
       );

       // == Grace Period == //
       uint128 cachedLastGracePeriodStartTimestamp = lastGracePeriodStartTimestamp;
       require(
           cachedLastGracePeriodStartTimestamp != UNSET_TIMESTAMP,
            "LiquidationLibrary: Recovery Mode grace period not started"
       );
       require(
           block.timestamp >
               cachedLastGracePeriodStartTimestamp + recoveryModeGracePeriodDuration,
           "LiquidationLibrary: Recovery mode grace period still in effect"
       );
    } // Implicit Else Case, Implies ICR < MRC, meaning the CDP is liquidatable
}

However, there is also a scenario where a user is attempting to liquidate a CDP in recovery mode, even when the ICR is below the MCR. In such a case, the grace period check will be skipped, and the CDP can be liquidated within the recovery mode duration.

Additionally, the grace period check is absent in the _getTotalFromBatchLiquidate_RecoveryMode function.

if (
    !vars.backToNormalMode &&
    (_checkICRAgainstMCR(vars.ICR) || canLiquidateRecoveryMode(vars.ICR, _TCR))
)

Here, it's important to note that ICR < MCR doesn't necessarily imply that the mode is not in recovery mode.

Tools Used

Recommended Mitigation Steps

Please modify _liquidateIndividualCdpSetup as below:

        - if (_ICR >= MCR) {
        -     // We must be in RM
        -     require(
        -         _checkICRAgainstLiqThreshold(_ICR, _TCR),
        -         "LiquidationLibrary: ICR is not below liquidation threshold in current mode"
        -     );

        -     // == Grace Period == //
        -     uint128 cachedLastGracePeriodStartTimestamp = lastGracePeriodStartTimestamp;
        -     require(
        -         cachedLastGracePeriodStartTimestamp != UNSET_TIMESTAMP,
        -         "LiquidationLibrary: Recovery Mode grace period not started"
        -     );
        -     require(
        -         block.timestamp >
        -             cachedLastGracePeriodStartTimestamp + recoveryModeGracePeriodDuration,
        -         "LiquidationLibrary: Recovery mode grace period still in effect"
        -     );
        - } // Implicit Else Case, Implies ICR < MRC, meaning the CDP is liquidatable

        + require(
        +     _checkICRAgainstLiqThreshold(_ICR, _TCR),
        +     "LiquidationLibrary: ICR is not below liquidation threshold in current mode"
        + );

        + if (_TCR < CCR) {
        +     // == Grace Period == //
        +     uint128 cachedLastGracePeriodStartTimestamp = lastGracePeriodStartTimestamp;
        +     require(
        +         cachedLastGracePeriodStartTimestamp != UNSET_TIMESTAMP,
        +         "LiquidationLibrary: Recovery Mode grace period not started"
        +     );
        +     require(
        +         block.timestamp >
        +             cachedLastGracePeriodStartTimestamp + recoveryModeGracePeriodDuration,
        +         "LiquidationLibrary: Recovery mode grace period still in effect"
        +     );
        + }

Please modify _getTotalFromBatchLiquidate_RecoveryMode function as below:

    - if (
    -     !vars.backToNormalMode &&
    -     (_checkICRAgainstMCR(vars.ICR) || canLiquidateRecoveryMode(vars.ICR, _TCR))
    - ) {

    + if (!vars.backToNormalMode && canLiquidateRecoveryMode(vars.ICR, _TCR)) {

Assessed type

Error

[bytes032]

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[c4-sponsor]

GalloDaSballo (sponsor) disputed

[jhsagd76]

The issue is true. There is a controversial statement here in the official document https://docs.ebtc.finance/ebtc/protocol-mechanics/liquidations :

... will trigger a 15 minutes period during which liquidations are blocked. This is known as the Grace Period.

But the quesion is that, should the cdp, which should have been liquidated in normal mode, be blocked for liquidation?

I think it's a valide QA, but for doc instead of code.

[c4-judge]

jhsagd76 changed the severity to QA (Quality Assurance)

[c4-judge]

jhsagd76 marked the issue as grade-a

[etherSky111]

Hi @jhsagd76 , thanks for your reviewing. Please review my comment.

According to the documentation, the Grace Period is defined as a minimum 15-minute period during which all liquidations are supposed to be blocked. This implies that all CDPs should be inaccessible for liquidation during the grace period, regardless of whether their ICR is higher or lower than the MCR.

Allow me to provide an example to clarify the issue. Users understandably do not want their CDPs to be liquidated by others, as this would result in a loss of funds through the liquidation process. Consider a scenario where some users maintain their CDPs with an ICR higher than the MCR, and the current mode is normal. (At this point, their cdps are safe.)

Due to certain events such as price fluctuations, the value of stEth decreases while the value of eBTC increases. As a result, the TCR becomes lower than the CCR, triggering the system to enter recovery mode. Users with a ICR lower than the TCR will aim to enhance their ICR during the grace period. Consequently, before the end of the grace period, the TCR may once again surpass the CCR.

In this situation, some users may find that their ICRs become lower than the MCR. The user attempts to collect some stEth and increase the collateral of their CDPs to surpass the MCR. However, before they can do so, other active users have the ability to liquidate that CDP because the grace period does not seem to work for that particular CDP.

Please correct me if I have misunderstood any aspect of the situation.

Thank you.

[jhsagd76]

I need to explain a fundamental security guideline for over-collateralized DeFi protocols.

Liquidation is the cornerstone of the protocol security, ensuring prompt liquidation for any loan that falls below MCR. This prevents bad debt and guarantees that the system's overall collateralization ratio does not enter a death spiral.

The protocol should also operate a fast and stable liquidator bot to handle black swan. Even in cases of severe price fluctuations, this liquidator bot needs to ensure smooth liquidation, even if it incurs losses. This is because a lack of economic incentives for liquidation would imply that there are no external liquidators available. kindly ping sponsor to ensure that they are aware of this @CodingNameKiki .

In conclusion, I do not believe that the grace period should be applicable to loans below MCR. This would significantly increase the system's risk of bad debt. So as I said, it's a typo in the documentation

[CodingNameKiki]

Yes we are aware and this is how the system is supposed to work, will explain why in a bit just waking up. Also agree that there is typo in the documentation.

[CodingNameKiki]

In conclusion, I do not believe that the grace period should be applicable to loans below MCR.

On short explanation the main use of the grace period is to ensure that if the system goes in recovery mode and TCR < CCR, any Cdp with collateral ratio between MCR and CCR have time to top up their positions and safe them otherwise they can be further liquidated to increase the system TCR.

Any other Cdp with ICR <= MCR should not be affected by the grace period, as further liquidating this positions can increase the TCR and there is a chance that at the end of the 15 mins, the system will be back to normal mode and no further position with collateral ratio between MCR and CCR will be liquidated.

As said by @jhsagd76 this should be a typo in the documentation.

[etherSky111]

Thanks @jhsagd76 , @CodingNameKiki

[c4-judge]

jhsagd76 marked the issue as grade-b