[M02] Complete liquidity removals fail from stableswap pools

mediumCode4rena

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.

rust📋
#[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:

bash📋
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