safe_withdrawal does not correctly protect against frontrunning

[c4-bot-2]

Lines of code

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

Vulnerability details

Bug Description

The HydraDx Protocol's Omnipool functionality is designed to safeguard users against front-running attacks during liquidity withdrawals. It employs a check against price deviations exceeding 1% from the oracle-provided average price. This mechanism is intended to revert transactions if such deviations occur, potentially due to front-running. However, the introduction of a safe_withdrawal boolean parameter compromises this protection under certain conditions. The safe_withdrawal flag is set to true when the authority disables market trading, based on the assumption that front-running is not possible without active trading.

pub(crate) fn is_safe_withdrawal(&self) -> bool {
	*self == Tradability::ADD_LIQUIDITY | Tradability::REMOVE_LIQUIDITY || *self == Tradability::REMOVE_LIQUIDITY
}

The flaw arises if an attacker identifies transactions aimed at withdrawing liquidity alongside an authority's decision to suspend trading. By front-running these transactions and manipulating the price before trading is halted, the attacker can exploit the safe_withdrawal condition. This allows the withdrawal to proceed without doing the 1% deviation check, leading to potential greater losses.

if !safe_withdrawal {
	T::PriceBarrier::ensure_price(
		&who,
		T::HubAssetId::get(),
		asset_id,
		EmaPrice::new(asset_state.hub_reserve, asset_state.reserve),
	)
	.map_err(|_| Error::<T>::PriceDifferenceTooHigh)?;
}

This mechanism is predicated on the notion that disabling trading removed the possibility of front-running. Nonetheless, this overlooks the brief window wherein transactions are visible but unexecuted, allowing an attacker to manipulate prices prior to the trading halt.

Impact

This oversight reverses the intended effect of the safe_withdrawal mechanism. Far from offering protection against front-running, it facilitates more severe exploitation by attackers during the interim between the detection of liquidity withdrawal transactions and the enforcement of trading restrictions.

Proof of Concept

Consider a scenario where an attacker observes a pending liquidity withdrawal transaction together with an authority call to disable trading. The attacker can front-run both transactions to significantly alter the market price before the trading halt is enacted. Once trading is suspended and the safe_withdrawal flag is engaged, the original withdrawal transaction proceeds without the protective price deviation check.

#[test]
fn safe_withdrawal_allows_for_frontrunning() {
	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(Omnipool::protocol_account(), DAI, 1000 * ONE),
			(Omnipool::protocol_account(), HDX, NATIVE_AMOUNT),
			(LP2, 1_000, 2000 * ONE),
			(LP2, DAI, 1000 * 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_min_withdrawal_fee(Permill::from_float(0.01))
		.build()
		.execute_with(|| {

			//--------------------- SETUP -----------------------
			let liq_added = 400 * ONE;
			let current_position_id = <NextPositionId<Test>>::get();

			//LP1 adds liquidity
			assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), 1_000, liq_added));

			//---------------------- ATTACK -----------------------
			//Malicious actor sees that the asset will be restricted soon, and that LP1 wants to remove liquidity in the mempool
			//Malicious actor frontruns both transactions and manipulates the price before the pool gets restricted

			//Malicious actor frontruns the calls and deviates the price into a territory above the 1% threshold
			EXT_PRICE_ADJUSTMENT.with(|v| {
				*v.borrow_mut() = (3, 100, true);
			});

			//The asset gets restricted (potentially due to market risk etc.) by the authority
			assert_ok!(Omnipool::set_asset_tradable_state(
				RuntimeOrigin::root(),
				1_000,
				Tradability::ADD_LIQUIDITY | Tradability::REMOVE_LIQUIDITY
			));

			let position = Positions::<Test>::get(current_position_id).unwrap();

			//LP1's removal of liquidity now also passes the mempool and is executed
			assert_ok!(Omnipool::remove_liquidity(
				RuntimeOrigin::signed(LP1),
				current_position_id,
				position.shares,
			));

			//LP1's removal was now executed at a price deviation worse than the 1%, which it should intentionally be protected against.
		});
}

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

To address this vulnerability, it's advisable to eliminate the safe_withdrawal flag, ensuring continuous protection against price manipulation and front-running, regardless of trading status. This adjustment will maintain the integrity of the liquidity withdrawal process, safeguarding users from potential exploitation.

Assessed type

MEV

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #158

[c4-judge]

OpenCoreCH marked the issue as satisfactory

[J4X-98]

Hey @OpenCoreCH,

I just saw that the accepted issues were changed during PJQA. This is a dup of the accepted #22 and was incorrectly duplicated to #93. I know this is very late but #22 was not accepted during PJQA when I commented on all issues, so I didn't flag the wrong deduplication as I though it was considered invalid. I kindly ask you to consider it.

Thanks for all your effort and best,
J4X

[c4-judge]

OpenCoreCH marked the issue as not a duplicate

[c4-judge]

OpenCoreCH marked the issue as duplicate of #22