It is possible to liquidate troves when yangs are suspended

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

Liquidators can liquidate troves even if the collateral yang is suspended, which prevents users from restoring the health of their troves (loss of funds for the borrowers).

Proof of concept

In lending projects where users have to maintain their positions healthy, and where there is a "pause" functionality for the collateral tokens, it is mandatory that:

1. Liquidators can NOT liquidate positions where the collateral is paused (otherwise it is a loss of funds for the borrowers)
2. There must be a grace period in which users will be able to deposit more collateral to heal their positions after a collateral token is unsuspended BEFORE liquidators can liquidate such positions (otherwise it becomes a who-comes-first war between liquidators and borrowers)

However, this is not implemented in the project. As walking through the code would make this report very dense, I have modified the already written test_purger.cairo::test_liquidate_suspended_yang test (which did revert, although it had the labbel #[ignore] so it was not shown by default when you do scarb test) to the next one:

    #[test]
    fn test_liquidate_suspended_yang() {
        let (shrine, abbot, _, absorber, purger, yangs, gates) = purger_utils::purger_deploy_with_searcher(
            purger_utils::SEARCHER_YIN.into(), Option::None
        );

        // user 1 opens a trove with ETH and BTC that is close to liquidation
        // `funded_healthy_trove` supplies 2 ETH and 0.5 BTC totalling $9000 in value, so we
        // create $6000 of debt to ensure the trove is closer to liquidation
        let trove_debt: Wad = (6000 * WAD_ONE).into();
        let target_trove: u64 = purger_utils::funded_healthy_trove(abbot, yangs, gates, trove_debt);

        // Suspend BTC
        let btc: ContractAddress = *yangs[1];
        let current_timestamp: u64 = get_block_timestamp();

        start_prank(CheatTarget::One(shrine.contract_address), shrine_utils::admin());
        shrine.suspend_yang(btc);
        stop_prank(CheatTarget::One(shrine.contract_address));

        assert(shrine.is_healthy(target_trove), 'should still be healthy');

        // The trove has $6000 in debt and $9000 in collateral. BTC's value must decrease
        let target_trove_start_health: Health = shrine.get_trove_health(target_trove);

        start_prank(CheatTarget::One(shrine.contract_address), shrine_utils::admin());
        shrine.set_threshold(btc, Ray { val : 1}); // @audit just a handy way to make a trove unhealthy
        stop_prank(CheatTarget::One(shrine.contract_address));

        assert(!shrine.is_healthy(target_trove), 'should be unhealthy');

        // Liquidate the trove
        let searcher = purger_utils::searcher();
        start_prank(CheatTarget::One(purger.contract_address), searcher);
        purger.liquidate(target_trove, target_trove_start_health.debt, searcher);
    }

As you can see, the test passes and the trove is liquidated even though the BTC yang is suspended in the shrine at the time of liquidation.

Recommended Mitigation Steps

This is a complex solution, but there are two main points that must be addressed:

1. Reject liquidation of troves where the yang is suspended.
2. Let users with a grace period in which they can enter more yang in their troves to make them healthy and prevent liquidators from liquidating them after a yang is unsuspended (to prevent a front-running war between users and liquidators)

Assessed type

Other

[tserg]

It is intended for troves to be liquidatable even if it has deposited a yang that is now suspended.

In any event, the possibility of race conditions (between liquidators and borrowers) is mitigated by the fact that the thresholds of suspended yangs are gradually scaled down over time.

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[alex-ppg]

The Warden details how the suspension of a Yang (collateral) will still permit the liquidation of troves that contain it.

The Sponsor has clarified that this is desirable behavior, and I consider this to be a reasonable assumption as disallowing liquidations could cause toxic debt to fester. The mitigation the Sponsor has described is adequate in ensuring that the borrowers have adequate time to repay their positions as the threshold of their suspended collateral will gradually instead of instantly scale down.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[nethoxa]

Thanks so much for your time reviewing my submission. I would like to note that in the documentation it is said that:

If a collateral type fails to satisfy any of the requirements, an assessment should be made to determine if it is capable of being rectified at short notice. If this is not possible, or it was not rectified within a reasonable amount of time, the yang should be suspended immediately.

If a suspended collateral type subsequently satisfies all of the requirements again within the stipulated time period set out in the Shrine contract, then it may be unsuspended. Otherwise, the yang will be permanently delisted as described in the Shrine contract.

which implies that the suspended state can be temporal instead of permanent. Such a temporal state can be from a few hours to half a year, as the yangs' risk will be monitored by automated systems and updated accordingly. My point is that the system has three states for a given yang:

• YangSuspensionStatus::None
• YangSuspensionStatus::Temporary
• YangSuspensionStatus::Permanent

From the borrowers point of view, it is the same to have the yang used in their troves being in a temporary suspension state or in a permanent one, as abbot::deposit -> abbot::deposit_helper -> sentinel::enter -> sentinel::assert_can_enter will revert if the status is different than None:

        fn assert_can_enter(self: @ContractState, yang: ContractAddress, gate: IGateDispatcher, enter_amt: u128) {

            ...

            let suspension_status: YangSuspensionStatus = self.shrine.read().get_yang_suspension_status(yang);
            assert(suspension_status == YangSuspensionStatus::None, 'SE: Yang suspended');

             ...
        }

but liquidators will be able to liquidate their troves in both situations regardless. Such a state should not be acceptable, even if intended by the own developers, as it is a loss of funds for borrowers due to them not being able to do anything to make their troves healthy. Given the highly volatile nature of the crypto market and the current state of the system, the next situation would be pretty common:

1. The used yang suffers a big squeeze (something like what happened yesterday)
2. Gets paused by the automated systems
3. User's troves get liquidated on the bottom of the squeeze without them being able to do anything
4. The yang recovers its price soon after the squeeze (today)
5. The yang is unpaused after that by the automated systems

which is NOT a desirable situation, as it harms borrowers, which prevents further activity from their side and directly impacts the rewards lenders receive for their money. The correct state transition for a given yang should be the following one:

• YangSuspensionStatus::None -> liquidations, deposits and new troves can happen
• YangSuspensionStatus::Temporary -> only deposits in EXISTING troves can happen (as this state can be from a few hours to half a year)
• YangSuspensionStatus::Permanent -> only liquidations can happen

[alex-ppg]

Hey @nethoxa, thanks for detailing your concerns about this submission! I believe you are misinterpreting what a Trove owner can do in the situation that their Yang was suspended.

When the Yang enters a temporary suspension, it still has a value attached and is able to back the debt of a Trove. As such, a suspension does not permit sudden liquidations.

The Yang will gradually have its "effective" factor scaled down to incentivize users to replace their Yang with more favorable and still active collateral.

There is nothing wrong with this design choice, and a user who lets suspended collateral in their Trove for a significant time is at fault and cannot be protected by the protocol.

Depositing more collateral of a suspended asset is ill-advised, as the suspended asset might have been disabled for a myriad of issues including security / long-term viability ones.

As (per the documentation) users can withdraw the suspended collateral and replace it with healthy collateral, I do not consider this submission valid. Your response states that a borrower cannot do anything to make their troves healthy which is not true.