PoolFee of StableSwap can be bypassed allowing for a DOS of all swaps

[c4-bot-10]

Lines of code

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

Vulnerability details

Bug Description

Step 1: Circumvent Fee

The Stableswap AMM imposes a Poolfee on each trade. This fee can be set by the Authority in the range of $0% <= fee <= 100%$. The fee is imposed on each trade, and in case of a call to sell(), will be deducted from the amount going out. In the case of a buy, it will be deducted from the amount going in.

For this issue, we will take a closer look at the functionality of the sell() function. When a sell call happens, the output is calculated by the calculate_out_given_in_with_fee() function.

pub fn calculate_out_given_in_with_fee<const D: u8, const Y: u8>(
	initial_reserves: &[AssetReserve],
	idx_in: usize,
	idx_out: usize,
	amount_in: Balance,
	amplification: Balance,
	fee: Permill,
) -> Option<(Balance, Balance)> {
	let amount_out = calculate_out_given_in::<D, Y>(initial_reserves, idx_in, idx_out, amount_in, amplification)?;
	let fee_amount = calculate_fee_amount(amount_out, fee, Rounding::Down);
	let amount_out = amount_out.checked_sub(fee_amount)?;
	Some((amount_out, fee_amount))
}

As one can see, this calls the calculate_fee_amount() with rounding down as the rounding direction.

fn calculate_fee_amount(amount: Balance, fee: Permill, rounding: Rounding) -> Balance {
	match rounding {
		Rounding::Down => fee.mul_floor(amount),
		Rounding::Up => fee.mul_ceil(amount),
	}
}

Unfortunately, this leads to the issue that low amounts being swapped will lead to the fee rounding down to 0, resulting in no fee being paid on the swap. Even for bigger swaps, the fee is rounded down on the call to sell() while it is rounded up on the call to buy(). This makes the sell() function overall cheaper to call for any swap where rounding of the fee will occur.

So a malicious user can split one big swap into many smaller swaps, to circumvent the protocol fee and get a cheaper swap. The maximum swap amount for this to work will be:

$$amountToWithdraw < 1/feePercentage$$

For a fee of 0.01%, this would for example be:

$$amountToWithdraw < 1/0.0001$$

$$amountToWithdraw < 10000$$

Step2: DOS Swaps

The HydraDx protocol also implements a circuit breaker that will halt trading and liquidity transfers if a preset value of transfers is crossed in a single block. To ensure that users can not artificially inflate the value of total swaps in a block, a fee is imposed. Unfortunately, as seen above, users can circumvent this fee. So a user can swap small amounts often enough without incurring a fee (besides the gas fee) and artificially inflate the total swaps. As soon as the threshold is passed, all other swaps will be DOSd.

To understand the viability of this attack path one also needs to understand the core functionality of the StableSwap. On normal AMMs A user would still lose funds on each swap, as the price is newly calculated each time. For StableSwap AMMs, as long as the reserves don't differentiate too much, the exchange rate stays constant at 1:1 for the tokens. So the users only "loss" on the swaps would be the fee, which can be circumvented.

Impact

The issue allows a malicious user to bundle his swap into multiple smaller swaps to circumvent the pool fee. This in turn allows the attacker to artificially trigger the circuit breaker.

Proof of Concept

The following testcase shows an example where the attacker is able to curcumvent the fee:

#[test]
fn fee_on_sell_can_be_bypassed() {
	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_float(0.0001), // Fee of 0.01%
			},
			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);

			//User sells value exactly below the benchmark
			assert_ok!(Stableswap::sell(
				RuntimeOrigin::signed(BOB),
				pool_id,
				asset_a,
				asset_b,
				9999,
				0,
			));

			let expected = 9997; // Value with no fee deducted (can be checked by setting the fee on top to 0)
			let pool_account = pool_account(pool_id);
			assert_balance!(BOB, asset_a, 200 * ONE - 9999);
			//User did not have to pay any fees
			assert_balance!(BOB, asset_b, expected);
		});
}

The test can be run by adding it to the pallets/stableswap/src/tests/trades.rs file.

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by changing the rounding direction so that the calculation always rounds up on the fee.

pub fn calculate_out_given_in_with_fee<const D: u8, const Y: u8>(
	initial_reserves: &[AssetReserve],
	idx_in: usize,
	idx_out: usize,
	amount_in: Balance,
	amplification: Balance,
	fee: Permill,
) -> Option<(Balance, Balance)> {
	let amount_out = calculate_out_given_in::<D, Y>(initial_reserves, idx_in, idx_out, amount_in, amplification)?;
	let fee_amount = calculate_fee_amount(amount_out, fee, Rounding::Up);
	let amount_out = amount_out.checked_sub(fee_amount)?;
	Some((amount_out, fee_amount))
}

Assessed type

DoS

[0xRobocop]

Consider QA, worst impact is temporally DoS. Leaving for sponsor review.

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

Although the rounding issue migh be correct, it has minimal impact on protocol.

But the DoS is incorrect, Stableswap is not "protected" by circuit breaker.

[OpenCoreCH]

Rounding down (i.e. in favor of the user) in this scenario might not be ideal conceptually, but the impact is very low in practice. With 12 decimals, we are talking about differences of roughly 10^-12, which is (way) smaller than the transaction fee of ~1.6 HDX. Still a good QA recommendation

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-a