Missing slippage checks on Omnipool.remove_liquidity()

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/omnipool/src/lib.rs#L577
https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/omnipool/src/lib.rs#L716

Vulnerability details

Bug Description

The HydraDx Protocol facilitates user interactions with its AMM, the Omnipool, by offering liquidity addition and withdrawal functions. A key security feature aimed at preventing front-running attacks on remove_liquidity() transactions is the implementation of the T::PriceBarrier::ensure_price() function.

This function compares the current price, potentially altered by front-running, with the average price provided by the oracle, allowing a maximum deviation of 1% as defined in the runtime configuration.

fn ensure_price(who: &AccountId, asset_a: AssetId, asset_b: AssetId, current_price: EmaPrice) -> Result<(), ()> {
	if WhitelistedAccounts::contains(who) {
		return Ok(());
	}

	let max_allowed = FixedU128::from(MaxAllowed::get());

	let oracle_price = ExternalOracle::get_price(asset_a, asset_b).map_err(|_| ())?;
	let external_price = FixedU128::checked_from_rational(oracle_price.n, oracle_price.d).ok_or(())?;
	let current_spot_price = FixedU128::checked_from_rational(current_price.n, current_price.d).ok_or(())?;

	let max_allowed_difference = max_allowed
		.checked_mul(&current_spot_price.checked_add(&external_price).ok_or(())?)
		.ok_or(())?;

	let diff = if current_spot_price >= external_price {
		current_spot_price.saturating_sub(external_price)
	} else {
		external_price.saturating_sub(current_spot_price)
	};

	ensure!(
		diff.checked_mul(&FixedU128::from(2)).ok_or(())? <= max_allowed_difference,
		()
	);
	Ok(())
}

However, this mechanism only guards against front-run-induced losses exceeding 1%, leaving users vulnerable to smaller, yet potentially frequent, front-running attacks.

Impact

Malicious actors can exploit this vulnerability to front-run remove_liquidity() and transactions, imposing losses on legitimate users within the 1% deviation threshold. This risk exposes users to potential financial losses from transactions that the protocol does not adequately protect.

Proof of Concept

A proof of concept demonstrates a scenario where a call to remove_liquidity() is front-run, causing a minor price deviation. Due to the deviation being within the 1% threshold, the protocol's current implementation does not prevent the transaction, illustrating the vulnerability to such front-running attacks.

#[test]
fn remove_liquidity_should_pass_when_frontrun_below_1() {
	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(Omnipool::protocol_account(), DAI, 1000 * ONE),
			(Omnipool::protocol_account(), HDX, NATIVE_AMOUNT),
			(LP2, 1_000, 2000 * ONE),
			(LP1, 1_000, 5000 * ONE),
		])
		.with_initial_pool(FixedU128::from_float(0.5), FixedU128::from(1))
		.with_token(1_000, FixedU128::from_float(0.65), LP2, 2000 * ONE)
		.with_max_allowed_price_difference(Permill::from_percent(1))
		.build()
		.execute_with(|| {
			let current_position_id = <NextPositionId<Test>>::get();
			assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), 1_000, 400 * ONE));

			//Minor adjustment of 99/100 
			EXT_PRICE_ADJUSTMENT.with(|v| {
				*v.borrow_mut() = (99, 100, true);
			});

			assert_noop!(
				Omnipool::remove_liquidity(RuntimeOrigin::signed(LP1), current_position_id, 200 * ONE,),
				Error::<Test>::PriceDifferenceTooHigh
			);
		});
}

To run the test it needs to be added to the omnipool/src/tests/remove_liquidity.rs file.

Tools Used

Manual Review

Recommended Mitigation Steps

This issue can be fixed by allowing the user to define a slippage parameter, similar to the one implemented in StableSwaps functions for withdrawing liquidity. This way a user does not have to endure a loss of up to 1%.

Assessed type

MEV

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #158

[c4-judge]

OpenCoreCH marked the issue as satisfactory

[c4-judge]

OpenCoreCH changed the severity to 2 (Med Risk)