Incorrect checking for minimum pool liquidity in `stableswap` pallet

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

In the stableswap pallet there are 2 functions that allow LP to withdraw the liquidity: remove_liquidity_one_asset() and withdraw_asset_amount(). Both functions burn user's shares in the end. However, in the first function there are 2 checks on whether the amount of burnt shares are bigger than minimal pool liquidity. This can lead to a situation where LP can't remove all of his liquidity even if the pool total shares are bigger than minimal pool liqudiity.

Proof of Concept

Let's consider the pool consisted of 2 assets - USDC, USDT and the amounts provided by LPs are [100, 150].

1. Let's say minimal pool liquidity is 5.
2. There are 2 users who equally provided 50 tokens for USDC token.
3. Imagine there are no trades happened and the first user just decided to withdraw his 50 tokens. He won't be able to do it as his current_share_balance will fall below minimal pool liquidity:

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

ensure!(
                current_share_balance == share_amount
                    || current_share_balance.saturating_sub(share_amount) >= T::MinPoolLiquidity::get(),
                Error::<T>::InsufficientShareBalance
            );

But the problem is that the pool has minimal liquidity that's enough for it to continue functioning. There is another check that makes sure that the total issuance of shares is bigger than minimal pool liquidity. This should be used as the pointer of whether the user can withdraw his liquidity or can't:

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

let share_issuance = T::Currency::total_issuance(pool_id);

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

Tools Used

Manual review.

Recommended Mitigation Steps

Remove the check that makes sure that the current share balance of the user is bigger than minimal pool liquidity.

Assessed type

Other

[0xRobocop]

Similar to #154

This one seems invalid. The user can burn all his shares as long there is a minimum overall amount of shares.

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[OpenCoreCH]

Invalid, because of current_share_balance == share_amount, the user can withdraw all shares.

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid