Setting threshold can be executed anytime which can cause unfair liquidation.

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L622-L629 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L1357-L1368 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L171-L176 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L1262-L1274 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/purger.cairo#L227-L231 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/purger.cairo#L549-L578

Vulnerability details

Vulnerability Detail

Setting liquidation threshold per yang (see below) has no restriction to when it should be executed. The only safe time is before opening trove with affected yang.

Since we know for sure there will be adjustment in the future, in which there are troves currently active and the admin decided to change the threshold of certain yangs (without proper checking), it can cause unfair liquidation to trove owners affected.

File: shrine.cairo
622:         fn set_threshold(                   
623:             ref self: ContractState, 
624:             yang: ContractAddress, 
625:             new_threshold: Ray) { 
626:             self.access_control.assert_has_role(shrine_roles::SET_THRESHOLD);
627: 
628:             self.set_threshold_helper(yang, new_threshold);
629:         }

File: shrine.cairo
1357:         fn set_threshold_helper(
1358:             ref self: ContractState, 
1359:             yang: ContractAddress, 
1360:             threshold: Ray) 
1361:             {
1362:             assert(threshold.val <= MAX_THRESHOLD, 'SH: Threshold > max');
1363:             self.thresholds.write(self.get_valid_yang_id(yang), threshold);
1364: 
1365:             // Event emission
1366:             self.emit(ThresholdUpdated { yang, threshold });
1367:         }

Let's further discuss on how untimely threshold setting could exactly affects the liquidation. In the function liquidate, the first step is to get trove health of the trove we are trying to liquidate. This can be executed by the function below:

File: shrine.cairo
1051:         // Returns a tuple of a trove's threshold, LTV based on compounded debt, trove value and compounded debt
1052:         // Returns a Health struct comprising the trove's threshold, LTV based on compounded debt,
1053:         // trove value and compounded debt;
1054:         fn get_trove_health(self: @ContractState, trove_id: u64) -> Health {
1055:             let interval: u64 = now();
1056: 
1057:             // Get threshold and trove collateral value
1058:             let trove_yang_balances: Span<YangBalance> = self.get_trove_deposits(trove_id);
1059:             let (mut threshold, mut value) = self.get_threshold_and_value(trove_yang_balances, interval);
1060:             threshold = self.scale_threshold_for_recovery_mode(threshold);
1061: 
1062:             let trove: Trove = self.troves.read(trove_id); //pertains to mapping trove 
1063: 

Let's focus on line 1059 (see above) and discuss further the function get_threshold_and_value, in which we can found this function (see below).

File: shrine.cairo
1262:         fn get_yang_threshold_helper(self: @ContractState, yang_id: u32) -> Ray {
1263:             let base_threshold: Ray = self.thresholds.read(yang_id);
1264:             match self.get_yang_suspension_status_helper(yang_id) {
1265:                 YangSuspensionStatus::None => { base_threshold },
1266:                 YangSuspensionStatus::Temporary => {
1267:                     // linearly decrease the threshold from base_threshold to 0
1268:                     // based on the time passed since suspension started
1269:                     let ts_diff: u64 = get_block_timestamp() - self.yang_suspension.read(yang_id);
1270:                     base_threshold * ((SUSPENSION_GRACE_PERIOD - ts_diff).into() / SUSPENSION_GRACE_PERIOD.into())
1271:                 },
1272:                 YangSuspensionStatus::Permanent => { RayZeroable::zero() },
1273:             }
1274:         }

Please see line 1263 in above, it reads the value of threshold for a certain yang ID. Please remember that this thresholds mapping is being updated by set_threshold function that would be serve as a basis for triggering liquidation (ltv > threshold).

As you can see, in the process flow above, there are no timing restriction throughout the process and adjustment of threshold can be problematic, disrupting the threshold data in which the liquidation process is dependent upon.

Impact

Unfair liquidation of troves due to sudden change in threshold.

Proof of Concept

Here's the coded POC to illustrate the scenario of setting threshold that can cause unfair liquidation. Before running this POC, please do the following:

1. Insert this constant below at utils.cairo under purger test folder const TARGET_TROVE_YIN_10K: u128 = 10000000000000000000000;
2. Copy the coded POC below and insert at test_purger.cairo under purger test folder.
3. Run the test by typing this in terminal, snforge test opus::tests::purger::test_purger::test_purger::test_liquidate_threshold_change --exact

 #[test]
    fn test_liquidate_threshold_change() {
        // Searcher liquidator has 10_000 yin (wad)
        let searcher_start_yin: Wad = purger_utils::SEARCHER_YIN.into(); 
        // deploy with searcher
        let (shrine, abbot, seer, _, purger, yangs, gates) = purger_utils::purger_deploy_with_searcher( 
            searcher_start_yin, Option::None
        );
        let mut spy = spy_events(SpyOn::One(purger.contract_address)); // emit events

        // create trove with a lot of collateral to avoid recovery mode, 700 BTC AND 700 ETH
        purger_utils::create_whale_trove(abbot, yangs, gates); 

        // 10_000 yin target debt
        let initial_trove_debt: Wad = purger_utils::TARGET_TROVE_YIN_10K.into(); 

        // creates target trove and return trove ID
        let target_trove: u64 = purger_utils::funded_healthy_trove(abbot, yangs, gates, initial_trove_debt); 

        // Get status of target trove health at the beginning
         println!("First Part: Trove Initiate");
        let target_trove_initial_health: Health = shrine.get_trove_health(target_trove);
        let target_trove_initial_health_ltv = target_trove_initial_health.ltv; 
        println!("ltv:       {}", target_trove_initial_health_ltv);
        let target_trove_initial_health_threshold = target_trove_initial_health.threshold; 
        println!("threshold: {}", target_trove_initial_health_threshold);
        let target_trove_initial_health_value: Wad = target_trove_initial_health.value;
        println!("collateral : {}", target_trove_initial_health_value);
        let target_trove_initial_health_debt: Wad = target_trove_initial_health.debt;
        println!("debt amount: {}", target_trove_initial_health_debt);

        // accrues some interest after 500 intervals (30 mins per interval)
        common::advance_intervals_and_refresh_prices_and_multiplier(shrine, yangs, 500);

        // Get status of Shrine health
        let shrine_health: Health = shrine.get_shrine_health();

        // Total debt of shrine
        let before_total_debt: Wad = shrine_health.debt;

        // Get status of target trove health after 500 intervals
         println!("----------------------------------");
        println!("2nd Part: Trove After 500 intervals");
        let target_trove_start_health: Health = shrine.get_trove_health(target_trove);
        let target_trove_start_health_ltv = target_trove_start_health.ltv; // ltv before lowering threshold
        println!("ltv:       {}", target_trove_start_health_ltv);
        let target_trove_start_health_threshold = target_trove_start_health.threshold; // threshold before lowering threshold
        println!("threshold: {}", target_trove_start_health_threshold);
        let target_trove_start_health_value: Wad = target_trove_start_health.value;
        println!("collateral : {}", target_trove_start_health_value);
        let target_trove_start_health_debt: Wad = target_trove_start_health.debt;
        println!("debt amount: {}", target_trove_start_health_debt);

        // Compute accrued interest from initial trove up to target trove
        let accrued_interest: Wad = target_trove_start_health.debt - initial_trove_debt;

        // Sanity check that some interest has accrued
        assert(accrued_interest.is_non_zero(), 'no interest accrued');

        // Admin change the threshold into 70%
        let new_threshold: Ray = (70 * RAY_PERCENT).into(); 
        purger_utils::lower_threshold_to_raise_trove_ltv(
            shrine, yangs, new_threshold
        );

        // Sanity check that LTV is at the target liquidation LTV
        println!("----------------------------------");
        println!("3rd Part: Trove After Lowering Threshold"); 
        let target_trove_updated_start_health: Health = shrine.get_trove_health(target_trove);
        let target_trove_ltv_after_lowering_threshold = target_trove_updated_start_health.ltv;
        println!("ltv:       {}", target_trove_ltv_after_lowering_threshold);
        let target_trove_threshold_after_lowering_threshold = target_trove_updated_start_health.threshold;
        println!("threshold: {}", target_trove_threshold_after_lowering_threshold);
        let target_trove_collateralvalue_after_lowering_threshold: Wad = target_trove_updated_start_health.value;
        println!("collateral : {}", target_trove_collateralvalue_after_lowering_threshold);
        let target_trove_debtamount_after_lowering_threshold: Wad = target_trove_updated_start_health.debt;
        println!("debt amount: {}", target_trove_debtamount_after_lowering_threshold);

        // Checking that the trove is liquidatable
        purger_utils::assert_trove_is_liquidatable(shrine, purger, target_trove, target_trove_updated_start_health.ltv); 

        let (penalty, max_close_amt) = purger.preview_liquidate(target_trove).expect('Should be liquidatable');
        let penalty_amt = penalty; //Positive penalty amount means the trove is liquidatable
        let max_amt = max_close_amt;

        let searcher: ContractAddress = purger_utils::searcher();

        let before_searcher_asset_bals: Span<Span<u128>> = common::get_token_balances(yangs, array![searcher].span());

        start_prank(CheatTarget::One(purger.contract_address), searcher);
        // Liquidation activated here
        let freed_assets: Span<AssetBalance> = purger.liquidate(target_trove, BoundedWad::max(), searcher); 

        // Assert that total debt includes accrued interest on liquidated trove
        let shrine_health: Health = shrine.get_shrine_health();
        let after_total_debt: Wad = shrine_health.debt;
        assert(after_total_debt == before_total_debt + accrued_interest - max_close_amt, 'wrong total debt');

        // Check that LTV is close to safety margin and check the health status after liquidation
         println!("----------------------------------");
        println!("4th Part: Trove After Liquidation");
        let target_trove_after_health: Health = shrine.get_trove_health(target_trove);
        let target_trove_ltv_after_liquidation = target_trove_after_health.ltv;
        println!("ltv:       {}", target_trove_ltv_after_liquidation);
        let target_trove_threshold_after_liquidation = target_trove_after_health.threshold;
        println!("threshold: {}", target_trove_threshold_after_liquidation);
        let target_trove_collateral_value_after_liquidation: Wad = target_trove_after_health.value;
        println!("collateral : {}", target_trove_collateral_value_after_liquidation);
        let target_trove_debt_amount_after_liquidation: Wad = target_trove_after_health.debt;
        println!("debt amount: {}", target_trove_debt_amount_after_liquidation);

        assert(
            target_trove_after_health.debt == target_trove_updated_start_health.debt - max_close_amt,
            'wrong debt after liquidation'
        );

        purger_utils::assert_ltv_at_safety_margin(target_trove_updated_start_health.threshold, target_trove_after_health.ltv);

        // Check searcher yin balance
        assert(shrine.get_yin(searcher) == searcher_start_yin - max_close_amt, 'wrong searcher yin balance');

        let (expected_freed_pct, expected_freed_amts) = purger_utils::get_expected_liquidation_assets(
            purger_utils::target_trove_yang_asset_amts(),
            target_trove_updated_start_health.value,
            max_close_amt,
            penalty,
            Option::None
        );
        let expected_freed_assets: Span<AssetBalance> = common::combine_assets_and_amts(yangs, expected_freed_amts);

        // Check that searcher has received collateral
        purger_utils::assert_received_assets(
            before_searcher_asset_bals,
            common::get_token_balances(yangs, array![searcher].span()),
            expected_freed_assets,
            10_u128, // error margin
            'wrong searcher asset balance',
        );

        common::assert_asset_balances_equalish(
            freed_assets, expected_freed_assets, 10_u128, // error margin
             'wrong freed asset amount'
        );

        let expected_events = array![
            (
                purger.contract_address,
                purger_contract::Event::Purged(
                    purger_contract::Purged {
                        trove_id: target_trove,
                        purge_amt: max_close_amt,
                        percentage_freed: expected_freed_pct,
                        funder: searcher,
                        recipient: searcher,
                        freed_assets: freed_assets
                    }
                )
            ),
        ];
        spy.assert_emitted(@expected_events);

        shrine_utils::assert_shrine_invariants(shrine, yangs, abbot.get_troves_count());
    }

Result: Here we can see that the liquidation is successful after a sudden lowering of the threshold into 70% (threshold is lower than LTV triggers liquidation in 3rd part)

Collected 1 test(s) from opus package
Running 1 test(s) from src/
First Part: Trove Initiate
ltv:       725900116144018583042973286
threshold: 763704994192799070847851335
collateral : 13776000000000000000000
debt amount: 10000000000000000000000
----------------------------------
2nd Part: Trove After 500 intervals
ltv:       726465042250509565185830429
threshold: 763704994192799070847851335
collateral : 13776000000000000000000
debt amount: 10007782422043019770000
----------------------------------
3rd Part: Trove After Lowering Threshold
ltv:       726465042250509565185830429
threshold: 700000000000000000000000000
collateral : 13776000000000000000000
debt amount: 10007782422043019770000
----------------------------------
4th Part: Trove After Liquidation
ltv:       629999999999999999873291451
threshold: 700000000000000000000000000
collateral : 9440247006028722445632
debt amount: 5947355613798095139552
[PASS] opus::tests::purger::test_purger::test_purger::test_liquidate_threshold_change, gas: ~247082
Tests: 1 passed, 0 failed, 0 skipped, 0 ignored, 391 filtered out

Tools Used

Manual Review, Starknet-Foundry

Recommended Mitigation Steps

There should be a requirement checking in executing the set_threshold function. This function should revert if there are current active troves that will be affected by the changes in threshold setting.

Apply also suspension of liquidation process for the troves that will be affected while making critical changes in threshold setting so we can avoid unfair liquidations.

Assessed type

Timing

[tserg]

This is intended - see honest admin assumption.

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[alex-ppg]

The Warden details how the liquidation threshold for troves can be arbitrarily changed by the administrators of the Opus system.

This falls under centralization concerns and should be outlined in an Analysis report rather than submitted as an HM finding per the relevant SC verdict.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[c4-judge]

alex-ppg marked the issue as primary issue

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid