HydraDX - J4X's results

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/runtime/hydradx/src/system.rs#L65-L87

Vulnerability details

Bug Description

The EMA oracle, designed to utilize HydraDX's Omnipools and StableSwap for exchange rate information, operates by monitoring activities within these liquidity pools. It looks for specific operations like exchanges, deposits, and withdrawals to adjust the assets' exchange rates accordingly. This updating process is not continuous but occurs when the responsible hooks are called by the StableSwap/Omnipool

The system, although thorough, does not account for price update triggers in the event of direct asset transfers to Stableswap, as these do not set off any hooks within the oracle. This lapse means that such direct transfers can alter asset prices within the liquidity pools without the oracle's knowledge, potentially leading to misleading exchange rates.

Moreover, there's a risk of manipulation by bad actors who might use direct transfers to StableSwap in an effort to sway the arbitrage process, especially during periods of network congestion. Such interference could unjustly prevent necessary liquidations within lending protocols.

Impact

The issue allows a malicious user to change the price of the AMM without updating the oracle.

Proof of Concept

The issue can be validated when looking at the runtime configuration. The configuration only restricts transfers to the Omnipool address, but not to the StableSwap address.

// filter transfers of LRNA and omnipool assets to the omnipool account
if let RuntimeCall::Tokens(orml_tokens::Call::transfer { dest, currency_id, .. })
| RuntimeCall::Tokens(orml_tokens::Call::transfer_keep_alive { dest, currency_id, .. })
| RuntimeCall::Tokens(orml_tokens::Call::transfer_all { dest, currency_id, .. })
| RuntimeCall::Currencies(pallet_currencies::Call::transfer { dest, currency_id, .. }) = call
{
	// Lookup::lookup() is not necessary thanks to IdentityLookup
	if dest == &Omnipool::protocol_account() && (*currency_id == hub_asset_id || Omnipool::exists(*currency_id))
	{
		return false;
	}
}
// filter transfers of HDX to the omnipool account
if let RuntimeCall::Balances(pallet_balances::Call::transfer { dest, .. })
| RuntimeCall::Balances(pallet_balances::Call::transfer_keep_alive { dest, .. })
| RuntimeCall::Balances(pallet_balances::Call::transfer_all { dest, .. })
| RuntimeCall::Currencies(pallet_currencies::Call::transfer_native_currency { dest, .. }) = call
{
	// Lookup::lookup() is not necessary thanks to IdentityLookup
	if dest == &Omnipool::protocol_account() {
		return false;
	}
}

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by disabling transfers to the StableSwap pools, similar to how it is implemented for the Omnipool.

Assessed type

Oracle

Lines of code

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

Vulnerability details

Bug Description

The StableSwap AMM of the HydraDx protocol implements safeguards against low liquidity so that too high price fluctuations are prevented, and manipulating the price becomes harder. These safeguards are enforced based on the MinPoolLiquidity which is a constant that describes the minimum liquidity that should be in a pool. Additionally, a pool is allowed to have a liquidity of 0, which would occur in the case of the creation of the pool, or by users withdrawing all their liquidity. This could also be defined as an invariant.

$totalPoolIssuance(poolId) >= MinPoolLiquidity || totalPoolIssuance(poolId) == 0$

When a user wants to withdraw his liquidity, he can use either the remove_liquidity_one_asset() function or the withdraw_asset_amount() function.

remove_liquidity_one_asset()

To ensure holding the invariant 2 checks are implemented in the remove_liquidity_one_asset() function.

The first check checks if the user either leaves more than MinPoolLiquidity shares in the pool or withdraws all his shares.

let current_share_balance = T::Currency::free_balance(pool_id, &who);

ensure!(
	current_share_balance == share_amount
		|| current_share_balance.saturating_sub(share_amount) >= T::MinPoolLiquidity::get(),
	Error::<T>::InsufficientShareBalance
);

The second check checks if the total liquidity in the pool would fall below the intended amount of shares.

let share_issuance = T::Currency::total_issuance(pool_id);

ensure!(
	share_issuance == share_amount
		|| share_issuance.saturating_sub(share_amount) >= T::MinPoolLiquidity::get(),
	Error::<T>::InsufficientLiquidityRemaining
);

These 2 checks work perfectly at holding the invariant at all times.

withdraw_asset_amount()

Unfortunately the second function for withdrawing liquidity withdraw_asset_amount() omits one of the checks. The function only checks if the user either withdraws all his shares or leaves more than the MinPoolLiquidity shares.

let current_share_balance = T::Currency::free_balance(pool_id, &who);

ensure!(
	current_share_balance == shares
		|| current_share_balance.saturating_sub(shares) >= T::MinPoolLiquidity::get(),
	Error::<T>::InsufficientShareBalance
);

One might state now that this could never break the invariant, as if every user's shares are either more than MinPoolLiquidity or zero, the total liquidity can never fall below MinPoolLiquidity without being 0. Unfortunately, this approach forgets that users can transfer their shares to other addresses. This allows a user to transfer an amount as low as 1 share to another address, and then withdraw all his shares. As the check would only ensure that he is withdrawing all his shares it would pass. If he was the only liquidity provider, there now would only be 1 share of liquidity left in the pool breaking the invariant of $totalPoolIssuance(poolId) >= MinPoolLiquidity$.

Impact

The issue allows a user to break the invariant about the MinPoolLiquidity and either push the pool into a state where it can easily be manipulated, or prevent other users from withdrawing their shares.

Proof of Concept

There are 2 ways how this could be exploited:

1. Breaking the invariant and letting the pool Liquidity fall below MinPoolLiquidity

A malicious LP could abuse this functionality to push the pool into a state where its total liquidity is below MinPoolLiquidity, and could be as low as 1 share, allowing for easy price manipulation. To achieve this the attacker would do the following:

1. User deposits MinPoolLiquidty using one of the functions
2. User transfers 1 share to another address controlled by him (so he does not lose any value)
3. User withdraws all his shares using withdraw_asset_amount()
4. The function will pass as it does not check the whole pool liquidity
5. The pool now only has 1 share of liquidity inside and can easily be manipulated

2. DOSing withdrawing through remove_liquidity_one_asset for others

Let's consider a case where there are only 2 liquidity providers and one of them is malicious and wants to prevent the other from withdrawing through remove_liquidity_one_asset(). This could for example be the case if the other is a smart contract, that can only withdraw through this function.

1. Both deposit MinPoolLiquidty
2. Malicious user transfers 1 share to another address controlled by him (so he does not lose any value)
3. Malicious user withdraws all his liquidity using withdraw_asset_amount()
4. Normal user now tries to withdraw all of his liquidity using remove_liquidity_one_asset()
5. The call fails as the total pool liquidity (which is checked in this one) would fall below MinPoolLiquidty
6. The user is forced to keep his liquidity in the pool until someone else adds liquidity.

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by also adding a check for the total pool liquidity to withdraw_asset_amount()

let share_issuance = T::Currency::total_issuance(pool_id);

ensure!(
	share_issuance == share_amount
		|| share_issuance.saturating_sub(share_amount) >= T::MinPoolLiquidity::get(),
	Error::<T>::InsufficientLiquidityRemaining
);

Assessed type

Other

High - A core invariant of the protocol can be broken for an extended duration.

Medium - A non-core invariant of the protocol can be broken for an extended duration or at scale, or an otherwise high-severity issue is reduced due to hypotheticals or external factors affecting likelihood.

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

Lines of code

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

Vulnerability details

Bug Description

The stableswap AMM in the HydraDX protocol allows users to deposit liquidity to facilitate the market maker functionality. It is a well known issue that calls to add/withdraw liquidty on vaults/amm pools can be frontrun to reduce the users received shares. In the add_liquidity_shares() this is already accounted for as the user provides the amount of shares he wants to receive, and can control his slippage by setting the max_asset_amount variable.

Unfortunately, in the add_liquidity() function, where the user sets how many assets he wants to provide, he is not able to set the minimum amount of assets that he wants to receive. This way the user could have to endure up to 100% slippage, resulting in massive losses.

Impact

This issue allows a malicious attacker to frontrun calls to add_Liquidity() so that users incur high slippage losses. The users can't protect themselves against this as they are not able to set a slippage parameter in the function.

Proof of Concept

The issue can already be spotted when looking at add_liquidty()'s function signature.

#[pallet::call_index(3)]
#[pallet::weight(<T as Config>::WeightInfo::add_liquidity()
					.saturating_add(T::Hooks::on_liquidity_changed_weight(MAX_ASSETS_IN_POOL as usize)))]
#[transactional]
pub fn add_liquidity(
	origin: OriginFor<T>,
	pool_id: T::AssetId,
	assets: Vec<AssetAmount<T::AssetId>>,
) -> DispatchResult {
	let who = ensure_signed(origin)?;

	let shares = Self::do_add_liquidity(&who, pool_id, &assets)?;

	Self::deposit_event(Event::LiquidityAdded {
		pool_id,
		who,
		shares,
		assets,
	});

	Ok(())
}

An exemplary situation can be described as follows

1. Alice wants to add liquidity and calls the add_liquidity() function
2. Bob spots this call and front runs it
3. He adds a lot of liquidity/trades so that Alice gets less shares than expected
4. Alice can't set slippage so her transaction will still execute

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by allowing the user to pass a minShares parameter. At the end of the function, it should check if the user receives more shares than this parameter, and otherwise revert.

ensure!(minShares <= shares, Error::<T>::SlippageLimit);

Assessed type

MEV

Lines of code

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

Vulnerability details

Bug Description

HydraDx Protocol's Automated Market Maker (AMM), known as the Omnipool, enables user transactions like adding liquidity. To combat potential front-running of add_liquidity() calls, the protocol uses the T::PriceBarrier::ensure_price() mechanism. This function checks if the current price, which may have been manipulated by front-running, deviates from the oracle's average price by no more than 1%, a limit set 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 safeguard only protects against price manipulations that exceed a 1% deviation, leaving a gap for smaller front-running attacks to occur without detection.

Impact

This vulnerability allows adversaries to execute front-running attacks on add_liquidity() transactions that result in less than a 1% loss for the targeted users, thereby exposing them to repeated minor financial losses that are not mitigated by the current protocol defenses.

Proof of Concept

The proof of concept demonstrates a scenario where a transaction to add_liquidity() is successfully front-run, leading to a slight price change within the 1% tolerance. The protocol fails to prevent the completion of the transaction, showcasing its susceptibility to such front-running strategies.

#[test]
fn add_liquidity_should_pass_when_frontrun_below() {
	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::add_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

A practical solution involves allowing users to set their own slippage tolerance level. By adopting a customizable slippage parameter, similar to the approach used in StableSwap's liquidity withdrawal functions, users can better control their acceptable loss margins, potentially avoiding the default up to 1% loss currently in place.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/math/src/stableswap/math.rs#L288

Vulnerability details

Bug Description

Whenever a new liquidity pool on StableSwap gets generated, a user has to deposit the initial liquidity for it. This is done by the user calling addLiquidity() and providing the initial liquidity for all assets.

Unfortunately due to an underflow in the calculation of the shares, a user trying to withdraw all of his initial liquidity again will never be able to. This is due to the calculation of the reserves in calculate_withdraw_one_asset() underflowing if the reserve of one asset is 0.

reserve.checked_mul(d1)?.checked_div(d_hp)?.checked_sub(y_hp)?

Impact

Due to this issue, the first caller of add_liquidity() can never fully withdraw it again.

Proof of Concept

The following POC showcases the issue

#[test]
fn withdraw_initial_liquidity() {
	let asset_a: AssetId = 1;
	let asset_b: AssetId = 2;

	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(BOB, 1, 200 * ONE),
			(BOB, 2, 200 * ONE),
			(ALICE, 1, 200 * ONE),
			(ALICE, 2, 200 * ONE),
		])
		.with_registered_asset("one".as_bytes().to_vec(), asset_a, 12)
		.with_registered_asset("two".as_bytes().to_vec(), asset_b, 12)
		.with_pool(
			ALICE,
			PoolInfo::<AssetId, u64> {
				assets: vec![asset_a, asset_b].try_into().unwrap(),
				initial_amplification: NonZeroU16::new(1000).unwrap(),
				final_amplification: NonZeroU16::new(1000).unwrap(),
				initial_block: 0,
				final_block: 0,
				fee: Permill::from_percent(0),
			},
			InitialLiquidity {
				account: ALICE,
				assets: vec![
					AssetAmount::new(asset_a, 100 * ONE),
					AssetAmount::new(asset_b, 100 * ONE),
				],
			},
		)
		.build()
		.execute_with(|| {
			let pool_id = get_pool_id_at(0);
			let amount_added = 100 * ONE;

			//Alice wants to withdraw her initial liquidity again
			assert_noop!(Stableswap::withdraw_asset_amount(
				RuntimeOrigin::signed(ALICE),
				pool_id,
				asset_a,
				100 * ONE,
				340282366920938463463374607431768211455,
			),
			Error::<Test>::InvalidInitialLiquidity);

		});
}

The test can be added to the pallets/stableswap/src/test/add_liquidity.rs file.

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by requiring the creator of the pool (admin) to deposit/donate MinPoolLiquidity of each token into the pool and burn the shares when calling create_pool. This way the first user calling addLiquidity() can withdraw all his liquidity again.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Bug Description

When using the substrate framework, it is one of the main goals of developers to prevent storage bloat. If storage can easily be bloated by users, this can lead to high costs for the maintainers of the chain and a potential DOS. A more in detail explanation can be found here.

The Omnipool allows users to deposit liquidity to earn fees on swaps. Whenever a user deposits liquidity through add_liquidity(), he gets an NFT minted and the details of his deposit are stored in the Positions map.

let instance_id = Self::create_and_mint_position_instance(&position_owner)?;

<Positions<T>>::insert(instance_id, lp_position);

To ensure that this storage is only used for serious deposits, it is ensured to be above MinimumPoolLiquidity which is 1,000,000 tokens in the runtime configuration.

ensure!(
	amount >= T::MinimumPoolLiquidity::get() && amount > 0,
	Error::<T>::MissingBalance
);

Additionally, whenever a deposit gets fully withdrawn, the storage entry is removed.

if updated_position.shares == Balance::zero() {
	// All liquidity removed, remove position and burn NFT instance

	<Positions<T>>::remove(position_id);
	T::NFTHandler::burn(&T::NFTCollectionId::get(), &position_id, Some(&who))?;

	Self::deposit_event(Event::PositionDestroyed {
		position_id,
		owner: who.clone(),
	});
}

Unfortunately, this implementation does not take into account that a malicious user can add MinimumPoolLiquidity tokens, and then instantly withdraw all but 1. In that case, he has incurred almost no cost for bloating the storage (besides the 1 token and gas fees) and can keep on doing this countless times.

Impact

The issue allows a malicious attacker to bloat the storage in a cheap way. If done often enough this allows him to DOS the functionality of the HydraDX protocol by bloating the storage significantly until it can't be maintained anymore. If the attacker uses a very low-value token, he only incurs the gas fee for each new entry.

If we consider that the intended cost for adding a new position entry (to potentially DOS) as defined by the MinimumPoolLiquidity should be 1_000_000 tokens, this issue allows an attacker to get the same storage bloat for 1/1_000_000 or 0.0001% of the intended cost.

Proof of Concept

The following testcase showcases the issue.

#[test]
fn remove_liquidity_but_one() {
	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)
		.build()
		.execute_with(|| {

			let liq_added = 1_000_000; //Exactly MinimumPoolLiquidity
			let current_position_id = <NextPositionId<Test>>::get();

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

			let liq_removed = 200 * ONE;
			assert_ok!(Omnipool::remove_liquidity(
				RuntimeOrigin::signed(LP1),
				current_position_id,
				1_000_000-1 //Remove all but one asset
			));

			let position = Positions::<Test>::get(current_position_id).unwrap();
			let expected = Position::<Balance, AssetId> {
				asset_id: 1_000,
				amount: 1,
				shares: 1,
				price: (1300000000650000, 2000000001000000),
			};

			//There now is a position with 1 single Wei bloating the storage
			assert_eq!(position, expected);
		});
}

The testcase can be added to the pallets/omnipool/src/tests/remove_liquidity.rs file.

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by not letting the amount in an open position fall below MinimumPoolLiquidity. This can be enforced as follows in the remove_liquidity() function.

ensure!(
	updated_position.amount >= T::MinimumPoolLiquidity::get() || updated_position.amount == 0,
	Error::<T>::InsufficientLiquidity
);

Assessed type

DoS

	/// Storage: `Omnipool::Positions` (r:0 w:1)  <===
	/// Proof: `Omnipool::Positions` (`max_values`: None, `max_size`: Some(100), added: 2575, mode: `MaxEncodedLen`)
	fn add_liquidity() -> Weight {
		// Proof Size summary in bytes:
		//  Measured:  `3919`
		//  Estimated: `8739`
		// Minimum execution time: 220_969_000 picoseconds.
		Weight::from_parts(222_574_000, 8739)
			.saturating_add(T::DbWeight::get().reads(20))
			.saturating_add(T::DbWeight::get().writes(14)) // <===
	}

Lines of code

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

Vulnerability details

Bug Description

The HydraDx protocol includes an oracle. This oracle generates prices, based upon the information it receives from its sources (of which Omnipool is one). The Omnipool provides information to the oracle through the on_liquidity_changed and on_trade hooks. Whenever a trade happens or the liquidity in one of the pools changes the corresponding hooks need to be called with the updated values.

The Omnipool contract also includes the remove_token() function. This function can only be called by the authority and can be only called on an asset which is FROZEN and where all the liquidity shares are owned by the protocol.

ensure!(asset_state.tradable == Tradability::FROZEN, Error::<T>::AssetNotFrozen);
ensure!(
	asset_state.shares == asset_state.protocol_shares,
	Error::<T>::SharesRemaining
);

When the function gets called it transfers all remaining liquidity to the beneficiary and removes the token. This is a change in liquidity in the Omnipool. The functionality in terms of liquidity change is similar to the withdraw_protocol_liquidity() where the protocol also withdraws liquidity in the form of protocol_shares from the pool. When looking at the withdraw_protocol_liquidity() function, one can see that it calls the on_liquidity_changed hook at the end, so that the oracle receives the information about the liquidity change.

T::OmnipoolHooks::on_liquidity_changed(origin, info)?;

Unfortunately, the remove_token() function does not call this hook, keeping the oracle in an outdated state. As the token is removed later on, the oracle will calculate based on liquidity that does not exist anymore in the Omnipool.

Impact

The issue results in the oracle receiving incorrect information and calculating new prices, based on an outdated state of the Omnipool.

Proof of Concept

The issue can be viewed when looking at the code of remove_token() where one can see that no call to the hook happens.

#[pallet::call_index(12)]
#[pallet::weight(<T as Config>::WeightInfo::remove_token())]
#[transactional]
pub fn remove_token(origin: OriginFor<T>, asset_id: T::AssetId, beneficiary: T::AccountId) -> DispatchResult {
	T::AuthorityOrigin::ensure_origin(origin)?;
	let asset_state = Self::load_asset_state(asset_id)?;

	// Allow only if no shares are owned by LPs and the asset is frozen.
	ensure!(asset_state.tradable == Tradability::FROZEN, Error::<T>::AssetNotFrozen);
	ensure!(
		asset_state.shares == asset_state.protocol_shares,
		Error::<T>::SharesRemaining
	);
	// Imbalance update
	let imbalance = <HubAssetImbalance<T>>::get();
	let hub_asset_liquidity = Self::get_hub_asset_balance_of_protocol_account();
	let delta_imbalance = hydra_dx_math::omnipool::calculate_delta_imbalance(
		asset_state.hub_reserve,
		I129 {
			value: imbalance.value,
			negative: imbalance.negative,
		},
		hub_asset_liquidity,
	)
	.ok_or(ArithmeticError::Overflow)?;
	Self::update_imbalance(BalanceUpdate::Increase(delta_imbalance))?;

	T::Currency::withdraw(T::HubAssetId::get(), &Self::protocol_account(), asset_state.hub_reserve)?;
	T::Currency::transfer(asset_id, &Self::protocol_account(), &beneficiary, asset_state.reserve)?;
	<Assets<T>>::remove(asset_id);
	Self::deposit_event(Event::TokenRemoved {
		asset_id,
		amount: asset_state.reserve,
		hub_withdrawn: asset_state.hub_reserve,
	});
	Ok(())
}

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by forwarding the updated asset state to the oracle by calling the on_liquidity_changed hook.

Assessed type

Oracle

T::Currency::transfer(asset_id, &Self::protocol_account(), &beneficiary, asset_state.reserve)?;

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

Low

[L-01] Formula for $\Delta b$ is not implemented according to documentation

OmipoolMath Line 344

Issue Description:

According to the Omnipool documentation, the formula for calculating $\Delta b$ (the amount of protocol shares) in the case of removed liquidity with the variables Pi (current price), $P\alpha$ (old price) and $S\alpha$ (shares removed) is:

$\Delta B = max(\frac{Pi-P\alpha}{Pi+P\alpha} * (- S\alpha),0)$

This is not the way the function is implemented in the code. The function is implemented in code as:

$\Delta b = \frac{P\alpha * currentReserveOfAsset - currentHubReserves}{P\alpha * currentReserveOfAsset + currentHubReserves} * S\alpha + 1$

This could be further simplified by dividing the upper and lower term of the fraction by currentReserveOfAsset and then splitting at the -/+.

$\Delta b = \frac{P\alpha * \frac{currentReserveOfAsset}{currentReserveOfAsset} - \frac{ currentHubReserves}{currentReserveOfAsset}}{P\alpha * \frac{currentReserveOfAsset}{currentReserveOfAsset} + \frac{ currentHubReserves}{currentReserveOfAsset}} * S\alpha + 1$ Now this can be simplified by removing the $\frac{currentReserveOfAsset}{currentReserveOfAsset}$ as it is equal to 1 and replacing $\frac{ currentHubReserves}{currentReserveOfAsset}$ with Pi as it would be the way to calculate the current price.

$\Delta b = \frac{P\alpha - Pi}{P\alpha + Pi} * S\alpha + 1$ If one now adds the negation added in the original form one ends up with a formula pretty close to the documented one.

$\Delta b = \frac{Pi - P\alpha }{Pi + P\alpha} * (- S\alpha) + 1$ As we know that $Pi < P\alpha$ and that $S\alpha$ is a positive value, the result of $\frac{Pi - P\alpha }{Pi + P\alpha} * - S\alpha$ could potentially go very close to zero, but could never be negative. So by adding the +1 at the end, the minimum we could reach (with rounding) is 1 not 0.

Recommended Mitigation Steps

As rounding in the favor of the protocol (by adding 1) should be intended behavior, the documentation of the formula is incorrect and needs to be adapted. The correct implemented formula is:

$\Delta B = max(\frac{Pi-P\alpha}{Pi+P\alpha} * (- S\alpha),1)$

This formula should also be documented.

[L-02] Missing check for swap to 0 when selling hub asset

omnipool/src/lib.rs Line 1704

Issue Description:

The omnipool swapping mechanism uses the sell() function so that a user can swap tokens and wants to specify how many token he wants to sell. One of the safeguards in this functions is the check if the user would get 0 tokens out and throwing an ZeroAmountOut error in that case, as this would mean a 100% loss on the swap for the user.

ensure!(
	*state_changes.asset_out.delta_reserve > Balance::zero(),
	Error::<T>::ZeroAmountOut
);

When the user wants to sell the Hub Asset Token for another token, the underlying sell_hub_asset() function is used. Unfortunately this function misses to do the mentioned check and does not return the ZeroAmountOut error on state_changes.asset.delta_reserve == 0. This leads to potential 100% losses for users on swaps.

Recommended Mitigation Steps

The issue can be mitigated by adding the same check that is used in the sell() function to the sell_hub_asset() function.

ensure!(
	*state_changes.asset.delta_reserve > Balance::zero(),
	Error::<T>::ZeroAmountOut
);

[L-03] Missing balance check in add_liquidity_shares()

StableSwap Line 512

**Issue Description:

When a user deposits funds into the protocol, either for a swap or to add liquidity, the users balance must first be checked. In the add_liquidty() function this is checked as follows.

ensure!(
	T::Currency::free_balance(asset.asset_id, who) >= asset.amount,
	Error::<T>::InsufficientBalance
);

Unfortunately the do_add_liquidity_shares() function does not implement any check on the balance.

**Recommended Mitigation Steps

The issue can be mitigated by implementing the same check in the do_add_liquidity_shares() function.

[L-04] Incorrect error returned for double adding of liquidity

StableSwap Line 1017

**Issue Description:

The StableSwap allows users to add liquidity for multiple assets at once. This is done by passing an array of assets and amount tuples to the do_add_liquidity() function. When an asset is more than once in that array, an error is correctly thrown. Unfortunately the error that is thrown is IncorrectAssets which is described as "Creating a pool with same assets or less than 2 assets is not allowed.".

Recommended Mitigation Steps:

The issue can be mitigated by adding a custom error called DuplicateEntry for this error case.

[L-05] Incorrect access for Authority

Omnipool/lib.rs Line156

Issue Description:

The Omnipools documentation states that the Authorities callable functions as

// Origin that can add token, refund refused asset and withdraw protocol liquidity.

Unfortunately by implementation the AuthorityOrigin is also able to call the function to remove tokens using `remove_token().

Recommended Mitigation Steps:

If it is intended that the AuthorityOrigin can remove tokens it must be added to the documentation. Otherwise the requirement needs to be removed/changed.

Non-Critical

[NC-01] Incorrect documentation of Error in add_liquidity

omnipool/src/lib.rs Line 560

Issue Description:

The documentation of the add_liquidity() function states:

//Asset weight cap must be respected, otherwise `AssetWeightExceeded` error is returned.

Unfortunately this is incorrect as the error is called AssetWeightCapExceeded not AssetWeightExceeded.

Recommended Mitigation Steps

The issue can be mitigated by correcting the documentation:

//Asset weight cap must be respected, otherwise `AssetWeightCapExceeded` error is returned.

[NC-02] Typo in documentation of calculate_remove_liquidity_state_changes

math/omnipool/math.rs Line 310

Issue Description:

The documentation of the calculate_remove_liquidity_state_changes() function includes a typo on the word liqudiity.

/// Calculate delta changes of remove liqudiity given current asset state and position from which liquidity should be removed.

Recommended Mitigation Steps

The issue can be mitigated by correcting the documentation:

/// Calculate delta changes of remove liquidity given current asset state and position from which liquidity should be removed.```

[NC-03] Incorrect ratio mentioned in buy_asset_for_hub_asset()

omnipool/src/lib.rs Line 1833

**Issue Description:

In the calculation if the MaxOutRatio is followed in the buy_asset_for_hub_asset() function, the comment states the wrong case for the error as failing if the MaxInRatio is zero.

ensure!(
	amount
		<= asset_state
			.reserve
			.checked_div(T::MaxOutRatio::get())
			.ok_or(ArithmeticError::DivisionByZero)?, // Note: this can only fail if MaxInRatio is zero.
	Error::<T>::MaxOutRatioExceeded
);

Recommended Mitigation Steps

The Issue can be mitigated by correcting the comment.

ensure!(
	amount
		<= asset_state
			.reserve
			.checked_div(T::MaxOutRatio::get())
			.ok_or(ArithmeticError::DivisionByZero)?, // Note: this can only fail if MaxOutRatio is zero.
	Error::<T>::MaxOutRatioExceeded
);

[NC-04] Incorrect ratio mentioned in sell_hub_asset()

omnipool/src/lib.rs Line 1756

**Issue Description:

In the calculation if the MaxOutRatio is followed in the sell_hub_asset() function, the comment states the wrong case for the error as failing if the MaxInRatio is zero.

ensure!(
	*state_changes.asset.delta_reserve
		<= asset_state
			.reserve
			.checked_div(T::MaxOutRatio::get())
			.ok_or(ArithmeticError::DivisionByZero)?, // Note: this can only fail if MaxInRatio is zero.
	Error::<T>::MaxOutRatioExceeded
);

Recommended Mitigation Steps

The Issue can be mitigated by correcting the comment.

ensure!(
	*state_changes.asset.delta_reserve
		<= asset_state
			.reserve
			.checked_div(T::MaxOutRatio::get())
			.ok_or(ArithmeticError::DivisionByZero)?, // Note: this can only fail if MaxOutRatio is zero.
	Error::<T>::MaxOutRatioExceeded
);

[NC-05] Incorrect documentation of calculate_shares_for_amount()

math/src/stableswap/math.rs Line 172

Issue Description:

The documentation of the calculate_shares_for_amount() incorrectly states:

/// Calculate amount of shares to be given to LP after LP provided liquidity of one asset with given amount.

This is incorrect as the function is used to calculate the shares that a user will have to burn when withdrawing liquidity.

Recommended Mitigation Steps

The issue can be fixed by adapting the comment as follows:

/// Calculate amount of shares to be burned from LP after LP withdraws liquidity.