Unhealthy troves with LTV > 90% cannot always be absorbed as intended

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/purger.cairo#L467

Vulnerability details

Impact

Unhealthy troves with ltv > 90% and threshold < 90% cannot always be absorbed due to a wrong if-condition.
According to Priority of liquidation methods it should always be possible to absorb unhealthy troves with ltv > 90%:

Absorption can happen only after an unhealthy trove's LTV has exceeded the LTV at which the maximum possible penalty is reached, or if it has exceeded 90% LTV. The liquidation penalty in this case will similarly be capped to the maximum of 12.5% or the maximum possible penalty.

However, the purger::get_absorption_penalty_internal(...) method mistakenly checks the threshold instead of the ltv against the ABSORPTION_THRESHOLD (90%) in L467:

fn get_absorption_penalty_internal(
    self: @ContractState, threshold: Ray, ltv: Ray, ltv_after_compensation: Ray
) -> Option<Ray> {
    if ltv <= threshold {
        return Option::None;
    }

    ...

    let mut max_possible_penalty: Ray = min(
        (RAY_ONE.into() - ltv_after_compensation) / ltv_after_compensation, MAX_PENALTY.into()
    );

    if threshold > ABSORPTION_THRESHOLD.into() { // @audit ltv instead
        let s = self.penalty_scalar.read();
        let penalty = min(MIN_PENALTY.into() + s * ltv / threshold - RAY_ONE.into(), max_possible_penalty);

        return Option::Some(penalty);
    }

    let penalty = min(MIN_PENALTY.into() + ltv / threshold - RAY_ONE.into(), max_possible_penalty);

    if penalty == max_possible_penalty {
        Option::Some(penalty)
    } else {
        Option::None
    }
}

As a consequence, unhealthy troves can only be absorbed if they reach the maximum possible penalty although the condition ltv > 90% is already satisfied. This is against the protocol's intended liquidation/absorption incentives and therefore endangers the solvency of the protocol.

By observing the sponsor's graph for liquidation penalty it becomes evident that the MAX_PENALTY can only be achieved for ltv up to 89%. For even higher ltv up to 100%, the penalty approaches 0% due to max_possible_penalty (see code above), which lowers the incentives for liquidation and makes absorption a necessity.
In case of threshold > 83% there is a window where 90% < ltv < ltv@max_possible_penalty causing absorptions to be impossible due to the present bug. This linked graph visualizes the present issue.

Proof of Concept

In the following, a numerical example is provided to demonstrate the above claims.

Initial assumptions:

threshold              = 88%    (reasonable because shrine::MAX_THRESHOLD is 100%)
ltv                    = 91%    (should be eligible for absorption in any case)
ltv_after_compensation = 93%    (reasonable because ltv becomes worse due to compensation)

Let's do the math step-by-step:

fn get_absorption_penalty_internal(
    self: @ContractState, threshold: Ray, ltv: Ray, ltv_after_compensation: Ray
) -> Option<Ray> {
    // @PoC: 91% <= 88% --> false, skip body
    if ltv <= threshold {
        return Option::None;
    }

    ...

    let mut max_possible_penalty: Ray = min(
        (RAY_ONE.into() - ltv_after_compensation) / ltv_after_compensation, MAX_PENALTY.into()
    );
    // @PoC: max_possible_penalty = 7.53%
    
    // @PoC: 88% > 90% --> false, skip body due to wrong check
    if threshold > ABSORPTION_THRESHOLD.into() { // @audit ltv instead
        let s = self.penalty_scalar.read();
        let penalty = min(MIN_PENALTY.into() + s * ltv / threshold - RAY_ONE.into(), max_possible_penalty);

        return Option::Some(penalty);
    }

    let penalty = min(MIN_PENALTY.into() + ltv / threshold - RAY_ONE.into(), max_possible_penalty);
    // @PoC: penalty = 6.41%
    
    // @PoC: 6.41% == 7.53% --> false, go in else
    if penalty == max_possible_penalty {
        Option::Some(penalty)
    } else {
        Option::None    // @PoC: trove is not eligible for absorption
    }
}

We can see that the trove has not reached its maximum possible penalty yet, therefore it cannot be absorbed as expected, although ltv > 90%.

Tools Used

Manual review

Recommended Mitigation Steps

Make sure the absorption threshold is checked against the ltv as intended:

diff --git a/src/core/purger.cairo b/src/core/purger.cairo
index 6a36bbc..820aff5 100644
--- a/src/core/purger.cairo
+++ b/src/core/purger.cairo
@@ -464,7 +464,7 @@ mod purger {
                 (RAY_ONE.into() - ltv_after_compensation) / ltv_after_compensation, MAX_PENALTY.into()
             );
 
-            if threshold > ABSORPTION_THRESHOLD.into() {
+            if ltv > ABSORPTION_THRESHOLD.into() {
                 let s = self.penalty_scalar.read();
                 let penalty = min(MIN_PENALTY.into() + s * ltv / threshold - RAY_ONE.into(), max_possible_penalty);
 

Assessed type

Math

[c4-pre-sort]

bytes032 marked the issue as duplicate of #13

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The Warden has demonstrated how a contradiction between the documentation and the implementation of the project will cause certain troves to not be liquidate-able temporarily.

I confirmed this submission as the documentation of the project states in the priority of liquidation methods chapter that a trove should be liquidate-able if its TVL exceeds 90% (i.e. the ABSORPTION_THRESHOLD). The code incorrectly validates the trove's threshold rather than LTV, rendering the submission to be valid.

I consider a medium-risk severity apt for this finding as the DoS is temporary.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report

[tserg]

This is an error in the documentation.

The correct wording should be:

Absorption can happen only after an unhealthy trove's LTV has exceeded the LTV at which the maximum possible penalty is reached, or if its threshold exceeds 90% LTV and its LTV has exceeded the threshold

[alex-ppg]

The Sponsor has clarified that the documentation was incorrect and that the code behaves as expected, however, per C4 standards I will accept this submission as valid given that the documentation serves as the source of truth for the Wardens to validate.

[MarioPoneder]

Dear Judge & Sponsor,

First of all, thanks for keeping the issue valid due to the source of truth consideration. I can confirm that's how we handle such cases on C4 and the present case serves as good exmaple of fair judging.

Anyways, I still want to provide further insights about this since it might be relevant for the sponsor.

According to the sponsor's update:

Absorption can happen only after an unhealthy trove's LTV has exceeded the LTV at which the maximum possible penalty is reached, or if its threshold exceeds 90% LTV and its LTV has exceeded the threshold

The following would be true:

• A trove with 88% threshold could only be absorbed > 92.5% LTV (due to max. penalty)
• A trove with > 90% threshold could already be absorbed > 90% LTV (due to 90% threshold)

This is contradictory and the discrepancy starts arising for thresholds > 83% effectively creating an "absorption gap", see main report and graph.

As far as I understood the mechanics of the protocol, the initially documented LTV criteria of absorption seemed to be the most reasonable while the code is subject to the above discrepancy.
Therefore, I still recommend to go with the implementation according to the main report's mitigation measures.

For anyone wanting to play around with threshold vs. LTV vs. max. penalty, I've created this graph which is based on the sponsor's initial graph from the docs.

I hope I could provide further insights and value!

Kind regards,
0xTheC0der

[tserg]

To give some context, the alternative condition of "if its threshold exceeds 90% LTV and its LTV has exceeded the threshold" for absorption was added because:

1. as a matter of convenience, because at thresholds greater than 90%, there is relatively less room for the LTV to increase before the maximum penalty is reached; and
2. at these high thresholds, the balance lies in favour of securing the protocol by liquidating these positions however the method (searcher or absorber) before they become underwater.

IMO, the "absorption gap" for thresholds > 83%, while conceptually contradictory, is acceptable because searcher liquidations is already available once LTV > threshold. Of course, the contradiction is more jarring the closer we get to 90% e.g. 88% threshold as you pointed, but a line has to be drawn somewhere and 90% is a convenient round figure.

[c4-sponsor]

milancermak (sponsor) confirmed