[M02] Complete liquidity removals fail from stableswap pools

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

The contracts for stableswap has 2 functions dealing with removal of liquidity: remove_liquidity_one_asset and withdraw_asset_amount. However, both these functions allow redeeming LP tokens and pay out in only one token. Critically, this contract is missing Curve protocol's remove_liquidity function, which allows redeeming LP tokens for all the different tokens in the pool.

The result of this decision is that when the complete liquidity of a pool is to be removed, the contract reverts with an arithmetic overflow. In curve protocol, when removing the complete liquidity, the composing tokens are removed from the pool. However here, they also need to be converted to a single token, using a liqudity which wont exist anymore. This leads to an issue somewhere in the mathematics of the curve liquidity calculation, and thus reverts.

Proof of Concept

A simple POC to remove the complete liquidity is coded up below. This POC reverts when the entire amount of shares is being redeemed.

#[test]
fn test_Attack_min_shares() {
	let asset_a: AssetId = 1;
	let asset_b: AssetId = 2;
	let asset_c: AssetId = 3;

	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(BOB, asset_a, 2*ONE),
			(ALICE, asset_a, 1*ONE),
			(ALICE, asset_b, 1*ONE),
			(ALICE, asset_c, 1*ONE),
		])
		.with_registered_asset("one".as_bytes().to_vec(), asset_a, 18)
		.with_registered_asset("two".as_bytes().to_vec(), asset_b, 6)
		.with_registered_asset("three".as_bytes().to_vec(), asset_c, 6)
		.with_pool(
			ALICE,
			PoolInfo::<AssetId, u64> {
				assets: vec![asset_a, asset_b, asset_c].try_into().unwrap(),
				initial_amplification: NonZeroU16::new(2000).unwrap(),
				final_amplification: NonZeroU16::new(2000).unwrap(),
				initial_block: 0,
				final_block: 0,
				fee: Permill::zero(),
			},
			InitialLiquidity {
				account: ALICE,
				assets: vec![
					AssetAmount::new(asset_a, 1*ONE),
					AssetAmount::new(asset_b, 1*ONE),
					AssetAmount::new(asset_c, 1*ONE),
				],
			},
		)
		.build()
		.execute_with(|| {
			let pool_id = get_pool_id_at(0);

			let received = Tokens::free_balance(pool_id, &ALICE);
            println!("LP tokens received: {}", received);


            assert_ok!(Stableswap::remove_liquidity_one_asset(
                RuntimeOrigin::signed(ALICE),
                pool_id,
                asset_a,
                received,
                0
            ));

            let asset_a_remliq_bal = Tokens::free_balance(asset_a, &ALICE);
            println!("asset a rem: {}", asset_a_remliq_bal);
		});
}

Here ALICE adds liquidity, and is trying to redeem all her LP tokens. This reverts with the following:

running 1 test
LP tokens received: 23786876415280195891619
thread 'tests::add_liquidity::test_Attack_min_shares' panicked at 'Expected Ok(_). Got Err(
    Arithmetic(
        Overflow,
    ),
)', pallets/stableswap/src/tests/add_liquidity.rs:889:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tests::add_liquidity::test_Attack_min_shares ... FAILED

This is because the internal math of the stableswap algorithm fails when there is no more liquidity.

Tools Used

Substrate

Recommended Mitigation Steps

Allow multi-token liquidity withdrawal, which would allow complete redeeming of all LP tokens.

Assessed type

Under/Overflow

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

It is not issue, and it is by design as we dont need the multi-token withdrawal functionality.

[OpenCoreCH]

The warden demonstrated that the initial liquidity cannot be removed from the system because of an overflow. This can lead to (temporary) locked funds in edge cases, so Medium is appropriate here.

[c4-judge]

OpenCoreCH marked issue #198 as primary and marked this issue as a duplicate of 198

[c4-judge]

OpenCoreCH marked the issue as selected for report