Forced price updates can lead to price discontinuity and absorption DoS

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L726
https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/external/pragma.cairo#L195-L200
https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/seer.cairo#L220-L230

Vulnerability details

Summary

Although the current codebase intends to take advantage of multiple oracles, the mechanism of forced price updates is fundamentally incompatible with multiple oracles. The impacts range from abrupt "jumps" between oracles (discontinuity) to potential DoS the absorption/redistribution mechanism.

Introduction

Every time an unhealthy trove cannot be fully absorbed by the absorber pool, the debt and assets get redistributed among other troves.
Thereby it's crucial that the purger::absorb(...) method updates the assets' prices which implicitly makes sure that the increased yang/asset ratios are correctly accounted for all troves.

fn absorb(ref self: ContractState, trove_id: u64) -> Span<AssetBalance> {
    ...
    // If it is not a full absorption, perform redistribution.
    if !is_fully_absorbed {
        ...
        // Update yang prices due to an appreciation in ratio of asset to yang from
        // redistribution
        self.seer.read().update_prices();    // <----
    }
    ...
}

This price update is invoked via the seer:update_prices() method which sets the force_update flag to ensure that the yang/asset ratios are accounted for in any case.
Subsequently, the seer::update_prices_internal(...) method proceeds to get the prices of all assets while using only the primary oracle due to force_update being set (more about that below).

fn update_prices(ref self: ContractState) {
    self.access_control.assert_has_role(seer_roles::UPDATE_PRICES);
    self.update_prices_internal(true);      // <---- force_update = true
}

fn update_prices_internal(ref self: ContractState, force_update: bool) {
    ...
    // loop through all yangs
    // for each yang, loop through all oracles until a
    // valid price update is fetched, in which case, call shrine.advance()
    // the expectation is that the primary oracle will provide a
    // valid price in most cases, but if not, we can fallback to other oracles
    ...
    loop {
        match yangs.pop_front() {
            Option::Some(yang) => {
                ...
                loop {
                    ...
                    match oracle.fetch_price(*yang, force_update) {  // <---- force_update = true
                        Result::Ok(oracle_price) => {
                            let asset_amt_per_yang: Wad = sentinel.get_asset_amt_per_yang(*yang);
                            let price: Wad = oracle_price * asset_amt_per_yang;
                            shrine.advance(*yang, price);            // <----
                            ...
                            break;
                        },
                        // try next oracle for this yang
                        Result::Err(_) => { oracle_index += 1; }
                    }
                };
            },
            Option::None => { break; }
        };
    };
    ...
}

When force_update is set, the pragma::fetch_price(...) method will always return Result::Ok(price) and never any Result::Err(...) (see code below).
Only exception: The oracle reverts, which would DoS the whole absorption process and all other normal price updates anyways.

This was intended to make sure that the yang/asset ratios are accounted for in any case, even if the oracle price is outdated or doesn't have enough sources.
Keep in mind: The oracle price could also be invalid/zero and will still be successfully returned.

Looking back at the code above, one can now see that only the primary oracle will be used due to always returning Result::Ok(oracle_price) and breaking out of the loop when force_update is set.

fn fetch_price(ref self: ContractState, yang: ContractAddress, force_update: bool) -> Result<Wad, felt252> {
    ...
    let response: PragmaPricesResponse = self.oracle.read().get_data_median(DataType::SpotEntry(pair_id));
    ...
    // if we receive what we consider a valid price from the oracle,
    // return it back, otherwise emit an event about the update being invalid
    // the check can be overridden with the `force_update` flag
    if force_update || self.is_valid_price_update(response) {
        return Result::Ok(price);            // <----
    }
    ...
}

Once an asset's price was fetched and the corresponding yang's price was correctly computed using the new ratio, the new price is applied and stored via the shrine::advance(...) method.
Note that this method will revert the whole absorption transaction if the price of any asset is zero due to the assertion below.

fn advance(ref self: ContractState, yang: ContractAddress, price: Wad) {
    ...
    assert(price.is_non_zero(), 'SH: Price cannot be 0');
    ...
}

Impacts

The current mechanism around absorption and forced price updates leads to the following impacts:

Forced price updates only use the primary oracle

Subpar price data (stalling, insufficient sources, invalid/zero) is immediately accepted, although a secondary oracle might have provided better data. The severity of this depends on the actual deviation of the oracle price from the "real" price.

For example, consider the following scenarios:

• The primary oracle is stalling/faulty, therefore the secondary oracle is automatically used for the normal continuous price updates. Once an absorption with redistribution happens, the primary oracle is forcefully used again which leads to a discontinuity in price that creates arbitrage opportunities and puts the protocol and users at risk due to a sudden price jump or even an invalid price.
• The primary oracle is stalling/faulty and an absorption with redistribution needs to be performed. Exactly in this case it would be crucial to fall back to a secondary oracle, but this is not possible due to current forced updated mechanism.

DoS of absorptions with redistributions

In case the primary oracle returns a zero price due to malfunction for just one single asset (asset might even be unrelated to the absorption), the whole absorption transaction will revert due the assertion in shrine::advance(...). This is possible since there are no prior sanity checks of the price when force_update is set.

As a consequence, being unable to execute an absorption due to the present DoS scenario puts the protocol and users at risk. Especially because the protocol might already be in a critical state when the absorption pool cannot fully cover absorptions anymore and therefore has to rely on redistribution.

Proof of Concept

No runnable PoC is provided as it would not add much value to prove the stated impacts, because:

• The impact of setting force_update and therefore skipping all sanity checks can be clearly seen in pragma::fetch_price(...) and subsequent seer::update_prices_internal(...).
• The fact that a failed assertion, see shrine::advance(...), reverts a whole transaction is nothing novel that would profit from an explicit proof.

Tools Used

Manual review

Recommended Mitigation Steps

I recommend the following changes:

Immediate solution for the absorption DoS

Remove the zero price assertion from shrine::advance(...). Instead, explicitly check for zero price and in this case re-use the previous price to apply the new yang/asset ratio.

Improvement of forced price updates

Redesign the price update mechanism such that even forced updates can profit from a secondary oracle. In case no secondary oracle can provide valid data, fall back to the primary oracle again when force_update is set. This requires the previous fix to be already implemented, otherwise the absorption could still be subject to DoS.

Assessed type

Oracle

[c4-pre-sort]

bytes032 marked the issue as primary issue

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-sponsor]

tserg (sponsor) acknowledged

[alex-ppg]

The Warden details how forcing price updates to occur from a single oracle may result in deviations depending on the number of oracles utilized in the system and their parity.

The described behavior, while acknowledged by the Sponsor, is more of a systemic risk and better suited as an Analysis report rather than an HM vulnerability. Redistribution should always use correct values, and falling back to the previously reported value of an asset in case the oracle is not working would cause the system to use stale data.

There is no real software-based solution to this flaw, and as such it presents a systemic risk that will solely arise in case the oracles integrated misbehave, deviate between them greatly, etc.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[MarioPoneder]

Dear Judge,

It seems to me that the present issue was mostly judged by the 1st of 2 impacts:

The Warden details how forcing price updates to occur from a single oracle may result in deviations depending on the number of oracles utilized in the system and their parity.

I fully agree that the first impact alone would be better suited for an Analysis report.

However, I'd appreciate your opinion about the second - more severe - impact:

DoS of absorptions with redistributions

Short version of dangerous scenario (please see main report for details):

• First oracle provides invalid data (0 price of an asset)
• Protocol uses fallback oracle that provides valid data
• Absorption with redistribution happens
• Forced price update always uses the first orcale
• The invalid 0 price of the first orcale triggers an assertion in the shrine::advance method
• The absorption with redistribution is subject to DoS

The risk of this happening could be reduced by using the same mechanism and sanity checks for oracle choice in the forced case like in the regular update case.

I'd appreciate if you could have another look on the issue from this perspective.

Thanks & kind regards,
0xTheC0der

[alex-ppg]

Hey @MarioPoneder, thanks for sharing your thoughts around the issue. The second case once again relies on the oracle misbehaving which is a systemic risk. As I specified in #230, you are more than welcome to open a C4 organizational issue to discuss the case of Oracles reporting invalid prices and how we should evaluate them as judges but I will retain my verdict for this particular case.

I understand that you may have reservations about my judgment, but I believe that a finding concerning an oracle misbehaving is identical to a DEX trade performed on a pair that may not have any liquidity (and thus fail) and so on. All these cases are circumstantial and beyond the control of the project, thereby representing systemic risks.

All systemic risks can be mitigated by the project's code, however, they can never be truly resolved (i.e. in this instance if all oracles misbehave and yield 0 for a price the flaw would still be present).