HydraDX - QiuhaoLi's results

Lines of code

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

Vulnerability details

Impact

LP may receive fewer shares than expected because of changed fees.

Proof of Concept

In the stable swap pallet, we apply fees on the different pools which depend on the pool.fee value and the number of coins.

/// Calculate amount of shares to be given to LP after LP provided liquidity of some assets to the pool.
pub fn calculate_shares<const D: u8>(
	initial_reserves: &[AssetReserve],
	updated_reserves: &[AssetReserve],
	amplification: Balance,
	share_issuance: Balance,
	fee: Permill,
) -> Option<Balance> {
	// ...
	let fixed_fee = FixedU128::from(fee);
	let fee = fixed_fee
		.checked_mul(&FixedU128::from(n_coins as u128))?
		.checked_div(&FixedU128::from(4 * (n_coins - 1) as u128))?;
// ...
		let amplification = Self::get_amplification(&pool);
		let share_issuance = T::Currency::total_issuance(pool_id);
		let share_amount = hydra_dx_math::stableswap::calculate_shares::<D_ITERATIONS>(
			&initial_reserves,
			&updated_reserves,
			amplification,
			share_issuance,
			pool.fee,
		)

The fee is taken into account when the shares are calculated later, so when the fee increases, the LP receives fewer shares. Also, the current amplification has impacts on the D which is the liquidity value (shares). However, there is no (slippage) protection for LP, so if an LP calls add_liquidity and there is a AuthorityOrigin update amplification or fee, the LP may receive fewer shares than expected.

Tools Used

Manual Review.

Recommended Mitigation Steps

Add a minimal shares parameter to add_liquidity function.

Assessed type

Uniswap

Lines of code

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

Vulnerability details

Impact

The last LP in a stable swap pool can't remove all the liquidity or remove all his shares for an assest.

Proof of Concept

When an LP removes liquidity from a stable swap pool, we make sure the shares left are bigger than MinPoolLiquidity or equal to all the issued shares:

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

However, this is actually impossible because we will fail in math functions calculating for assets. Especially, if an LP tries to remove all the shares, the calculation of adjusted_d in calculate_shares will fail because not all reserves in adjusted_reserves are zero.

Let's try it in test remove_liquidity_should_work_when_withdrawing_all_shares by removing the Alice (the first and last LP) all shares:

fn remove_liquidity_should_work_when_withdrawing_all_shares() {
	let asset_a: AssetId = 1;
	let asset_b: AssetId = 2;
	let asset_c: AssetId = 3;

	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(BOB, asset_a, 200 * ONE),
			(ALICE, asset_a, 100 * ONE),
			(ALICE, asset_b, 200 * ONE),
			(ALICE, asset_c, 300 * ONE),
		])
		.with_registered_asset("one".as_bytes().to_vec(), asset_a, 12)
		.with_registered_asset("two".as_bytes().to_vec(), asset_b, 12)
		.with_registered_asset("three".as_bytes().to_vec(), asset_c, 12)
		.with_pool(
			ALICE,
			PoolInfo::<AssetId, u64> {
				assets: vec![asset_a, asset_b, asset_c].try_into().unwrap(),
				initial_amplification: NonZeroU16::new(100).unwrap(),
				final_amplification: NonZeroU16::new(100).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, 200 * ONE),
					AssetAmount::new(asset_c, 300 * ONE),
				],
			},
		)
		.build()
		.execute_with(|| {
			let pool_id = get_pool_id_at(0);

			let amount_added = 200 * ONE;

			let pool_account = pool_account(pool_id);

			assert_ok!(Stableswap::add_liquidity(
				RuntimeOrigin::signed(BOB),
				pool_id,
				vec![AssetAmount::new(asset_a, amount_added),]
			));

			let shares = Tokens::free_balance(pool_id, &BOB);

			assert_ok!(Stableswap::remove_liquidity_one_asset(
				RuntimeOrigin::signed(BOB),
				pool_id,
				asset_c,
				shares,
				0,
			));

			let amount_received = Tokens::free_balance(asset_c, &BOB);
			assert_balance!(BOB, asset_a, 0u128);
			assert_balance!(BOB, asset_c, 199999999999999u128);
			assert_balance!(BOB, pool_id, 0u128);
			assert_balance!(pool_account, asset_a, 100 * ONE + amount_added);
			assert_balance!(pool_account, asset_c, 300 * ONE - amount_received);

			let alice_shares = Tokens::free_balance(pool_id, &ALICE);
			assert_ok!(Stableswap::remove_liquidity_one_asset(
				RuntimeOrigin::signed(ALICE),
				pool_id,
				asset_b,
				alice_shares,
				0,
			));
		});
}

Then it will fail.

Tools Used

Manual Review.

Recommended Mitigation Steps

When an LP remove all the issued shared, we should send all the assets out and reset the pool.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

When the authority calls withdraw_protocol_liquidity to remove protocol liquidity, it's vulnerable to sandwich attacks.

Proof of Concept

In withdraw_protocol_liquidity, we didn't make sure the spot price was not too far from the EMA oracle price. Which can lead to:

1. The authority calls withdraw_protocol_liquidity.
2. Attakcers call buy or sell, making the spot price and EMA price diff bigger than the max allowed one.
3. The withdrawal is executed, for example, the authority is supposed to get all the asset tokens, but now he can only get some asset tokens and LRNA, which in all worth less than expected.

Tools Used

Manual Review.

Recommended Mitigation Steps

Like remote_liquidity, we should ensure spot/oracle price diff is not bigger than the max allowed one:

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

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

Users can lose all the withdrawn assets as fees when the asset is in safe withdraw mode.

Proof of Concept

In remove_liquidity, we will skip the spot/oracle price check if it's a safe mode withdraw (i.e. sell and buy are disabled).

			let safe_withdrawal = asset_state.tradable.is_safe_withdrawal();
			// Skip price check if safe withdrawal - trading disabled.
			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)?;
			}

To prevent liquidity attacks, we apply a fee (max 100%) on the withdrawal action, which depends on the spot/oracle price diff.

			let ext_asset_price = T::ExternalPriceOracle::get_price(T::HubAssetId::get(), asset_id)?;

			if ext_asset_price.is_zero() {
				return Err(Error::<T>::InvalidOraclePrice.into());
			}
			let withdrawal_fee = hydra_dx_math::omnipool::calculate_withdrawal_fee(
				asset_state.price().ok_or(ArithmeticError::DivisionByZero)?,
				FixedU128::checked_from_rational(ext_asset_price.n, ext_asset_price.d)
					.defensive_ok_or(Error::<T>::InvalidOraclePrice)?,
				T::MinWithdrawalFee::get(),
			);

It seems we assume if the asset is in safe mode, spot/oracle price diff should be minimal since the trade is disabled and other operations don't change the price.

However, in set_asset_tradable_state which can set an asset to the safe mode, we don't check if the spot/oracle diff is big (T::PriceBarrier::ensure_price), which can lead to:

1. The authority sets asset A to safe mode by disabling sell and buy operations by calling set_asset_tradable_state.
2. An attacker front-running the call above and buy/sell A, making spot/oracle price diff bigger.
3. A is in safe mode, Alice calls the remove liquidity function, and ensure_price is not triggered this time and Alice will suffer from a big fee.
4. (optional) Attacker does a reversed sell/buy call on asset A.

Tools Used

Manual Review.

Recommended Mitigation Steps

Call ensure_price in set_asset_tradable_state when we set an asset to the safe mode.

Assessed type

Context

[LOW] stableswap: Attackers can prevent the first add_liquidity by transferring one token to the pool directly

When a user first calls add_liquidity, the function will fail in calculate_shares if there are non-zero tokens in the pool (initial_reserves should be all zero or all non-zero). So an attacker can front-run the add_liquidity after the pool is created, and then the LP will suffer a DoS.

The LP can mitigate this by sending some dust to the zero assets in the pool and then calling the add_liquidity, but that's unexpected behavior for the users.

[LOW] Function-Level Swap DoC invariants should consider the fees

https://github.com/galacticcouncil/HydraDX-security/blob/main/invariants/function-level/Omnipool.md#swap

• $R_iQ_i$ should be invariant, but one is calculated from the other. If e.g. $R_i^+$ is calculated it may have error up to $1$, in which case the product $R_i^+Q_i^+$ may have error up to $Q_i^+$. If $Q_i^+$ is calculated, then the product has error up to $R_i^+$. Thus we should always be able to bound the error by $max(R_i^+,Q_i^+)$, giving us

However, we didn't consider the fees here. For example, there are 100:100 lrna:A in the pool, the asset fee is 20%, and Alice wants to buy 30 A with lrna. In that case, the asset fee will be 7+1=8 A. And Alice needs to sell 61 lrna (10030/50 + 1). In process_trade_fee, 8-1=7 A fees can be transferred out at most. So the subpool will be 161:63. And 100100 + 161 > 16163 > 100100, which is correct. However, suppose referrals_used is 4, so only 3 A asset fees are transferred out, the subpool will be 161:67, now 100100 + 161 is not bigger than 16167.

[LOW] weight cap can be bypassed by removing other tokens liquidity

A malicious attacker can bypass the weight cap in a block restriction by:

1. Provide liquidity to asset A until it reaches the weight cap.
2. Remove all the liquidity of other assets until the trading volume protection.
3. Now the actual weight of asset A is above the weight cap.

[INFO] circuit-breaker:validate_limit should make sure numerator <= denominator

There is no check if numerator <= denominator in circuit-breaker:validate_limit(), should add one.

[INFO] stableswap: calculate_out/in_amount should make sure the pool has been initialzed (shares > 0)

Otherwise, it will fail later in calculate_out_given_in.

[INFO] omnipool: withdraw_protocol_liquidity weight should add on_liquidity_changed_weight()

Only callable by AuthorityOrigin, so not a big problem.