Large users can potentially prevent other smaller users to conduct omnipool trades in critical moments.

[c4-bot-3]

Lines of code

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

Vulnerability details

Detailed Explanation

The protocol is imposing net trading volume limit based on percentage of current liquidity. However this limitation can cause issues during critical moments affecting small volume users. As you can see below, the function set_trade_volume_limit sets trading volume limit per asset.

The critical moments we are talking about here is during the times when users are eagerly trying to convert risky assets into more stable assets like BTC and ETH or stablecoins. When there is current set limit of 50% of an already small liquidity of risky asset pool, this can be easily filled by just one single large trade, preventing other succeeding small users to trade in critical moments, causes loss of funds to them due to quick degrading value of risky asset they want to trade.

If the large users want to intentionally DOS the trading activity of a particular asset, they can easily do it anytime specially during critical period, provided that the volume limit figure can be easily meet by them.

Although the trading volume limit is being reset per block, a large user with resources can still abuse this issue in several blocks, specially to those pools with very small liquidity.

#[pallet::call_index(0)]
		#[pallet::weight(<T as Config>::WeightInfo::set_trade_volume_limit())]
		pub fn set_trade_volume_limit(
			origin: OriginFor<T>,
			asset_id: T::AssetId,
			trade_volume_limit: (u32, u32),
		) -> DispatchResult {
			T::TechnicalOrigin::ensure_origin(origin)?;

			ensure!(asset_id != T::OmnipoolHubAsset::get(), Error::<T>::NotAllowed);

			Self::validate_limit(trade_volume_limit)?;

			<TradeVolumeLimitPerAsset<T>>::insert(asset_id, trade_volume_limit);

			Self::deposit_event(Event::TradeVolumeLimitChanged {
				asset_id,
				trade_volume_limit,
			});

			Ok(())
		}

pub struct TradeVolumeLimit<T: Config> {
	pub volume_in: T::Balance,
	pub volume_out: T::Balance,
	pub limit: T::Balance,
}

Impact

Small volume users can't trade in critical moments causing loss of funds to them.

Proof of Concept

Let's illustrate below a sample scenario wherein a large volume trader just filled in a single transaction the volume limit set by the protocol which is 200,00 units of HDX. Then, the subsequent small volume trader transaction failed in executing his transaction.

Please insert and run this test under file trade_volume.rs in circuit breaker folder.

#[test]
fn ensure_and_update_trade_volume_limit_should_fail_when_large_user_filled() {
	ExtBuilder::default().build().execute_with(|| {
		//Arrange
		assert_ok!(CircuitBreaker::initialize_trade_limit(HDX, INITIAL_LIQUIDITY));
		assert_ok!(CircuitBreaker::initialize_trade_limit(DOT, INITIAL_LIQUIDITY));
		
		assert_eq!(
			CircuitBreaker::allowed_trade_volume_limit_per_asset(HDX).unwrap(),
			TradeVolumeLimit {
				volume_in: 0,
				volume_out: 0,
				limit: 200_000,
			}
		);

		assert_ok!(CircuitBreaker::ensure_and_update_trade_volume_limit( // large user trade volume in
			HDX, 200_000, DOT, 0
		));

		assert_eq!(
			CircuitBreaker::allowed_trade_volume_limit_per_asset(HDX).unwrap(), // volume status after large user trade
			TradeVolumeLimit {
				volume_in: 200__000,
				volume_out: 0,
				limit: 200_000,
			}
		);
		
		// Act & Assert
		assert_noop!(
			CircuitBreaker::ensure_and_update_trade_volume_limit(HDX, 1_000, DOT, 0), // small user trade volume in fail
			Error::<Test>::TokenInfluxLimitReached
		);
	});
}

Tools Used

Manual Review

Recommended Mitigation Steps

Do not allow a single large user to grab the whole volume trade limit in single block transaction. Impose a trading limitation per asset per user account to avoid this abuse on small users.

Assessed type

DoS

[0xRobocop]

Seems like a design decision

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[OpenCoreCH]

Design decision, that's typically how circuit breakers work. Downgrading to QA

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-a