Ema-oracle will ignore accumulator spot prices in some cases, resulting in incorrect ema prices

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/ema-oracle/src/lib.rs#L276-L277

Vulnerability details

Impact

Ema-oracle will ignore accumulator spot prices in some cases, resulting in incorrect ema prices.

Proof of Concept

Ema-oracle has two mechanisms that need to work in tandem at all times to report accurate ema prices for various periods.

The two mechanisms: accumulator oracle entry (spot price entry, Accumulator) and update_oracles (period-based ema oracles, Oracles).

The above two are triggered and updated at different times: (1) Accumulator: triggered by liquidity/trade hooks and atomically writes a spot oracle entry; Also, last_block oracle is the last Accumulator from the previous block. e.g. last_blockoracle (@1002 block) = Accumulator (@1002 block). (2) Oracles: only updated at the end of each block to aggregate the last accumulator entry of the block to ema oracle values. This is triggered by on_finalize()-> update_oracles_from_accumulator()->last_block_oracle().

The problem is (2) might not always use the correct Accumulator in some cases. When on_finalize() is missed or failed for one or more blocks, (2) will use an Accumulator value from an incorrect block.

Scenario: Suppose last_block oracle (previous block Accumulator) hasn't been updated for a while.

For example,last_block oracle in storage is updated at 1000 block, and current block number is 1003, previous block is 1002. And each block has new trading activities. Since there is no last_block oracle for 1001 and 1002, last_block_oracle() will equate last_block at 1002 with Accumulator at 1000.

Step A: So last_block oracle (1002)= Accumulator(1000). This disregards trading activities in 1001 and 1002.

//HydraDX-node/pallets/ema-oracle/src/lib.rs
    pub(crate) fn last_block_oracle(
        source: Source,
        assets: (AssetId, AssetId),
        block: BlockNumberFor<T>,
    ) -> Option<(OracleEntry<BlockNumberFor<T>>, BlockNumberFor<T>)> {
        Self::oracle((source, assets, LastBlock)).map(|(mut last_block, init)| {
            // update the `LastBlock` oracle to the last block if it hasn't been updated for a while
            // price and liquidity stay constant, volume becomes zero
                        //@audit note: last_block` oracle (1002)= `Accumulator`(1000)
            if last_block.updated_at != block {
|>              last_block.fast_forward_to(block);
            }
            (last_block, init)
        })
    }
//HydraDX-node/pallets/ema-oracle/src/types.rs
    pub fn fast_forward_to(&mut self, new_updated_at: BlockNumber) {
                //@audit note: this only updates blockNumber and refresh trading volume. However,  old price and liquidity persist regardless.
                //@audit note: last_block` oracle (1002)= `Accumulator`(1000)
|>      self.updated_at = new_updated_at;
        self.volume = Volume::default();
    }

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/ema-oracle/src/lib.rs#L276-L277)

Step B: in update_oracle() following last_block_oracle(), all other period-based oracles will be updated with the incorrect last_block oracle value(Accumulator(1000)).

//HydraDX-node/pallets/ema-oracle/src/lib.rs
    fn update_oracle(
        src: Source,
        assets: (AssetId, AssetId),
        period: OraclePeriod,
        incoming_entry: OracleEntry<BlockNumberFor<T>>,
    ) {
...
                if parent > prev_entry.updated_at {
                                        //@audit note: `last_block` oracle (1002)= `Accumulator`(1000)
|>                  Self::last_block_oracle(src, assets, parent)
                        .and_then(|(last_block, _)| {
                    //@audit note: other period oracle(1002) are updated based on `Accumulator`(1000) as well. 
|>  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.");
                        })
                }
...

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/ema-oracle/src/lib.rs#L313-L315)

The above effectively skewed all other period-based oracle values because they are accumulative.

Missed or failed on_finialize() can be caused by reasons like consensus stall or other failures, but current code handles the case in a way that leads to skewed ema prices.

Tools Used

Manual

Recommended Mitigation Steps

Consider storing historic Accumulator values (similar to UniswapV3's observations), and use the correct historic Accumulator entry to handle missed ema entries.

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #191

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid