The `update_oracle()` function can fail silently and thus result in a stale price

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/ema-oracle/src/lib.rs#L296-L338

Vulnerability details

Impact

The oracle will retain its previous value, leading to stale data that does not reflect the most recent market conditions. Thus, a hacker can take advantage of the situation and at a key moment steal a large amount of tokens because the price is outdated.

Proof of Concept

update_oracle() function is used in on finalize() and update the oracle value:

    fn update_oracle(
        src: Source,
        assets: (AssetId, AssetId),
        period: OraclePeriod,
        incoming_entry: OracleEntry<BlockNumberFor<T>>,
    ) {
        Oracles::<T>::mutate((src, assets, period), |oracle| {
            // initialize the oracle entry if it doesn't exist
            if oracle.is_none() {
                *oracle = Some((incoming_entry.clone(), T::BlockNumberProvider::current_block_number()));
                return;
            }
            if let Some((prev_entry, _)) = oracle.as_mut() {
                let parent = T::BlockNumberProvider::current_block_number().saturating_sub(One::one());
                // update the entry to the parent block if it hasn't been updated for a while
                if parent > prev_entry.updated_at {
                    Self::last_block_oracle(src, assets, parent)
                        .and_then(|(last_block, _)| {
                            prev_entry.update_outdated_to_current(period, &last_block).map(|_| ())
                        })
                        .unwrap_or_else(|| {
                            log::warn!(
                                target: LOG_TARGET,

                                "Updating EMA oracle ({src:?}, {assets:?}, {period:?}) to parent block failed. Defaulting to previous value."
                            );
                            debug_assert!(false, "Updating to parent block should not fail.");
                        })
                }
                // calculate the actual update with the new value
                prev_entry
                    .update_to_new_by_integrating_incoming(period, &incoming_entry)
                    .map(|_| ())
                    .unwrap_or_else(|| {
                        log::warn!(
                            target: LOG_TARGET,
                            "Updating EMA oracle ({src:?}, {assets:?}, {period:?}) to new value failed. Defaulting to previous value."
                        );
                        debug_assert!(false, "Updating to new value should not fail."); 
                    });
            };
        });
    }

The problem is when the update_outdated_to_current() and update_to_new_by_integrating_incoming() functions are called it is possible to fail. If this happens then the failure is silent because it is used debug_assert!.

The function uses unwrap_or_else to handle potential errors when updating the oracle. If an update fails, it logs a warning message and triggers a debug assertion. But debug_assert! statements have no effect in release builds. When updates fail, outdated price will continue to be used.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Establish monitoring to track how long the price has been outdated.

Assessed type

Oracle

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

There is no evidence to support the warden's claim:

Thus, a hacker can take advantage of the situation and at a key moment steal a large amount of tokens because the price is outdated.

[OpenCoreCH]

No impact demonstrated and the alternative (failing not silently) would result in the exact same system state.

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Insufficient proof