Circuit breaker is vulnerable to `on_finalize()` miss or fail, at risk of outdated trading or liquidity limit leading to over-trading

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/circuit-breaker/src/lib.rs#L430

Vulnerability details

Impact

Circuit breaker is vulnerable to on_finalize() miss or fail, at risk of outdated trading or liquidity limit leading to over-trading.

Proof of Concept

Both ema-oracle pallet and circuit-breaker pallet are dependent on on_finalize() call at the end of a block.

The problem is that ema-oracle pallet has a mechanism to handle on_finalize()miss or fail, but circuit-breaker pallet doesn't have any mechanism to handle it.

The impact is if one or more on_finalize() is missed or failed, the block-based trading limit(AllowedTradeVolumeLimitPerAsset) will continue to use a value calculated based on a historic block.

At a beginning of a new block, AllowedTradeVolumnLimitPerAsset value is supposed to be clear already by on_finalize() of the previous block. However, if on_finalize() fails, AllowedTradeVolumnLimitPerAsset is not cleared and initialize_trade_limit() will do nothing and continually use an old limit.

//HydraDX-node/pallets/circuit-breaker/src/lib.rs
    fn initialize_trade_limit(asset_id: T::AssetId, initial_asset_reserve: T::Balance) -> DispatchResult {
        //@audit note: the if block will only run if AllowedTradeVolumnLimitPerAsset for asset_id is not set.
            //@audit note: if previous on_finalize() failed, AllowedTradeVolumnLimitPerAsset will not be cleared, `initialize_trade_limit` will do nothing.
|>      if asset_id != T::OmnipoolHubAsset::get() && !<AllowedTradeVolumeLimitPerAsset<T>>::contains_key(asset_id) {
            let limit = Self::calculate_limit(
                initial_asset_reserve,
                Pallet::<T>::trade_volume_limit_per_asset(asset_id),
            )?;

            <AllowedTradeVolumeLimitPerAsset<T>>::insert(
                asset_id,
                TradeVolumeLimit::<T> {
                    limit,
                    volume_in: Zero::zero(),
                    volume_out: Zero::zero(),
                },
            );
        }
        Ok(())
    }

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/circuit-breaker/src/lib.rs#L430)

If the situation persists, initialize_trade_limit() will continue to be skipped. If the old limit is too large relative to the reserve of current block, users can over-trade; Or if the old limit is small or comparable relative to the current reserve, trading can be DOSsed.

Tools Used

Manual

Recommended Mitigation Steps

In initialize_trade_limit(), add an additional check on the current block number, if the current block number changes, run the if body to initialize the trade limit.

Assessed type

Other

[0xRobocop]

Consider QA. Report does not give evidence on why the below statement will happen.

The impact is if one or more on_finalize() is missed or failed, the block-based trading limit(AllowedTradeVolumeLimitPerAsset) will continue to use a value calculated based on a historic block.

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[OpenCoreCH]

Why should the on_finalize call be "missed" or fail (with its very simple logic that clears 3 values)? It is required that this function is always executed at the end of a block: https://docs.substrate.io/learn/transaction-lifecycle/

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid