c4-bot-10 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/pallets/stableswap/src/lib.rs#L584 https://github.com/code-423n4/2024-02-hydradx/blob/main/HydraDX-node/math/src/stableswap/math.rs#L160

Vulnerability details

Impact

The last LP in a stable swap pool can't remove all the liquidity or remove all his shares for an assest.

Proof of Concept

When an LP removes liquidity from a stable swap pool, we make sure the shares left are bigger than MinPoolLiquidity or equal to all the issued shares:

            ensure!(
                share_issuance == share_amount // <===
                    || share_issuance.saturating_sub(share_amount) >= T::MinPoolLiquidity::get(),
                Error::<T>::InsufficientLiquidityRemaining
            );

However, this is actually impossible because we will fail in math functions calculating for assets. Especially, if an LP tries to remove all the shares, the calculation of adjusted_d in calculate_shares will fail because not all reserves in adjusted_reserves are zero.

Let's try it in test remove_liquidity_should_work_when_withdrawing_all_shares by removing the Alice (the first and last LP) all shares:

fn remove_liquidity_should_work_when_withdrawing_all_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, 200 * ONE),
            (ALICE, asset_a, 100 * ONE),
            (ALICE, asset_b, 200 * ONE),
            (ALICE, asset_c, 300 * ONE),
        ])
        .with_registered_asset("one".as_bytes().to_vec(), asset_a, 12)
        .with_registered_asset("two".as_bytes().to_vec(), asset_b, 12)
        .with_registered_asset("three".as_bytes().to_vec(), asset_c, 12)
        .with_pool(
            ALICE,
            PoolInfo::<AssetId, u64> {
                assets: vec![asset_a, asset_b, asset_c].try_into().unwrap(),
                initial_amplification: NonZeroU16::new(100).unwrap(),
                final_amplification: NonZeroU16::new(100).unwrap(),
                initial_block: 0,
                final_block: 0,
                fee: Permill::from_percent(0),
            },
            InitialLiquidity {
                account: ALICE,
                assets: vec![
                    AssetAmount::new(asset_a, 100 * ONE),
                    AssetAmount::new(asset_b, 200 * ONE),
                    AssetAmount::new(asset_c, 300 * ONE),
                ],
            },
        )
        .build()
        .execute_with(|| {
            let pool_id = get_pool_id_at(0);

            let amount_added = 200 * ONE;

            let pool_account = pool_account(pool_id);

            assert_ok!(Stableswap::add_liquidity(
                RuntimeOrigin::signed(BOB),
                pool_id,
                vec![AssetAmount::new(asset_a, amount_added),]
            ));

            let shares = Tokens::free_balance(pool_id, &BOB);

            assert_ok!(Stableswap::remove_liquidity_one_asset(
                RuntimeOrigin::signed(BOB),
                pool_id,
                asset_c,
                shares,
                0,
            ));

            let amount_received = Tokens::free_balance(asset_c, &BOB);
            assert_balance!(BOB, asset_a, 0u128);
            assert_balance!(BOB, asset_c, 199999999999999u128);
            assert_balance!(BOB, pool_id, 0u128);
            assert_balance!(pool_account, asset_a, 100 * ONE + amount_added);
            assert_balance!(pool_account, asset_c, 300 * ONE - amount_received);

            let alice_shares = Tokens::free_balance(pool_id, &ALICE);
            assert_ok!(Stableswap::remove_liquidity_one_asset(
                RuntimeOrigin::signed(ALICE),
                pool_id,
                asset_b,
                alice_shares,
                0,
            ));
        });
}

Then it will fail.

Tools Used

Manual Review.

Recommended Mitigation Steps

When an LP remove all the issued shared, we should send all the assets out and reset the pool.

Assessed type

Context

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as primary issue

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as insufficient quality report

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as remove high or low quality report

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as duplicate of #86