Re-adding assets to the omnipool can cause a problem with the oracle

mediumCode4rena

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L1541-L1574 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/traits.rs#L164-L190 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/ema-oracle/src/lib.rs#L558-L566

Vulnerability details

Summary

If an asset is removed from the omnipool, it is ensured that all data records in the omnipool are deleted and also all positions from liquidity providers. However, the data records in the Oracle are not reset. This means that if the asset is to be added again after some time and it then has a different price, the price in the Oracle is falsified.

POC

Here is a poc which can be inserted into the file “integration-tests/src/omnipool_init.rs” and that can be started with the following command: SKIP_WASM_BUILD=1 cargo test -p runtime-integration-tests poc -- --nocapture

rust📋
#[test]
pub fn poc() {
	TestNet::reset();

	Hydra::execute_with(|| {
		let omnipool_account = hydradx_runtime::Omnipool::protocol_account();

		init_omnipool();

		let position_id_init = hydradx_runtime::Omnipool::next_position_id();
		assert_ok!(hydradx_runtime::Omnipool::add_token( //DOT token is added for the first time
			hydradx_runtime::RuntimeOrigin::root(),
			DOT,
			FixedU128::from_float(1.0),
			Permill::from_percent(100),
			AccountId::from(ALICE)
		));
		hydradx_run_to_next_block();

		assert_ok!(hydradx_runtime::Omnipool::sacrifice_position( //The shares from the initial liquidity are assigned to the protocol so that the token can be removed
			hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
			position_id_init
		));
		hydradx_run_to_next_block();

		assert_ok!(hydradx_runtime::Omnipool::set_asset_tradable_state( //The asset is frozen so that the token can be removed
			hydradx_runtime::RuntimeOrigin::root(),
			DOT,
			Tradability::FROZEN
		));

		assert_ok!(hydradx_runtime::Omnipool::remove_token( //This removes the token
			hydradx_runtime::RuntimeOrigin::root(),
			DOT,
			AccountId::from(ALICE)
		));

		//This is simply to skip a few blocks to show that some time has passed since the token was removed and the prices are still there
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();
		hydradx_run_to_next_block();

		assert_ok!(hydradx_runtime::Tokens::transfer( //The initial liquidity is sent to the pool to add DOT the second time
			hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
			omnipool_account,
			DOT,
			500 * UNITS
		));

		assert_ok!(hydradx_runtime::Omnipool::add_token( //DOT is added the second time
			hydradx_runtime::RuntimeOrigin::root(),
			DOT,
			FixedU128::from_float(10.0), //The price has increased from 1.0 to 10.0
			Permill::from_percent(100),
			AccountId::from(ALICE)
		));
		hydradx_run_to_next_block();

		let return_value = hydradx_runtime::Omnipool::add_liquidity( //A liquidity provider tries to add liquidity
			hydradx_runtime::RuntimeOrigin::signed(ALICE.into()),
			DOT,
			1 * UNITS
		);
		println!("return_value: {:?}", return_value); //shows that adding liquidity didn't work because the price difference was too big
		hydradx_run_to_next_block();

		let (price_short, _) = hydradx_runtime::EmaOracle::get_price(LRNA, DOT, OraclePeriod::Short, OMNIPOOL_SOURCE).unwrap();
		println!("price_short: {:?}", price_short); //shows that the price for the period short is not 10, which it should actually be since the liquidity has not changed since DOT was added for the second time
	});
}

Description

If an asset is removed from the Omnipool, all of its data will be deleted from the Omnipool. However, the data from the asset remains in the Oracle and is not deleted there. The Oracle then continues to store the data of the removed asset in this StorageMap:

rust📋
pub type Oracles<T: Config> = StorageNMap<
                _,
                (
                        NMapKey<Twox64Concat, Source>,
                        NMapKey<Twox64Concat, (AssetId, AssetId)>,
                        NMapKey<Twox64Concat, OraclePeriod>,
                ),
                (OracleEntry<BlockNumberFor<T>>, BlockNumberFor<T>),
                OptionQuery,
        >;

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

The price is stored here once for the last block and once with the exponential moving average logic for a few blocks in the past. (This is the short period)

The problem now is when an asset is added again and its price has changed. As a result, the new price is calculated using the exponential moving average logic with the entries of the blocks before the asset was removed and the new entries that have been added since the asset was re-added, which leads to an incorrect price. The wrong price can also cause ensure_price in add_liquidity to fail and therefore no liquidity can be added:

rust📋
T::PriceBarrier::ensure_price(
			&who,
      T::HubAssetId::get(),
		  asset,
	    EmaPrice::new(asset_state.hub_reserve, asset_state.reserve),
)
.map_err(|_| Error::<T>::PriceDifferenceTooHigh)?;

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

So any protocol that uses this oracle as a price source would receive an incorrect price for the re-added asset for a short period of time.

Recommendation

Even if the price balances out again after some time (the length of the short period), an incorrect price is initially calculated after re-adding an asset for which the price has changed. I would recommend deleting the Oracle entries when removing an asset. This means that the price of the asset can be calculated correctly from the start when it is added again.

Assessed type

Oracle