Malicious liquidity provider can put pool into highly manipulatable state

[c4-bot-10]

Lines of code

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

Vulnerability details

Bug Description

The StableSwap AMM of the HydraDx protocol implements safeguards against low liquidity so that too high price fluctuations are prevented, and manipulating the price becomes harder. These safeguards are enforced based on the MinPoolLiquidity which is a constant that describes the minimum liquidity that should be in a pool. Additionally, a pool is allowed to have a liquidity of 0, which would occur in the case of the creation of the pool, or by users withdrawing all their liquidity. This could also be defined as an invariant.

$$totalPoolIssuance(poolId) >= MinPoolLiquidity || totalPoolIssuance(poolId) == 0$$

When a user wants to withdraw his liquidity, he can use either the remove_liquidity_one_asset() function or the withdraw_asset_amount() function.

remove_liquidity_one_asset()

To ensure holding the invariant 2 checks are implemented in the remove_liquidity_one_asset() function.

The first check checks if the user either leaves more than MinPoolLiquidity shares in the pool or withdraws all his shares.

let current_share_balance = T::Currency::free_balance(pool_id, &who);

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

The second check checks if the total liquidity in the pool would fall below the intended amount of shares.

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

These 2 checks work perfectly at holding the invariant at all times.

withdraw_asset_amount()

Unfortunately the second function for withdrawing liquidity withdraw_asset_amount() omits one of the checks. The function only checks if the user either withdraws all his shares or leaves more than the MinPoolLiquidity shares.

let current_share_balance = T::Currency::free_balance(pool_id, &who);

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

One might state now that this could never break the invariant, as if every user's shares are either more than MinPoolLiquidity or zero, the total liquidity can never fall below MinPoolLiquidity without being 0. Unfortunately, this approach forgets that users can transfer their shares to other addresses. This allows a user to transfer an amount as low as 1 share to another address, and then withdraw all his shares. As the check would only ensure that he is withdrawing all his shares it would pass. If he was the only liquidity provider, there now would only be 1 share of liquidity left in the pool breaking the invariant of $totalPoolIssuance(poolId) >= MinPoolLiquidity$.

Impact

The issue allows a user to break the invariant about the MinPoolLiquidity and either push the pool into a state where it can easily be manipulated, or prevent other users from withdrawing their shares.

Proof of Concept

There are 2 ways how this could be exploited:

1. Breaking the invariant and letting the pool Liquidity fall below MinPoolLiquidity

A malicious LP could abuse this functionality to push the pool into a state where its total liquidity is below MinPoolLiquidity, and could be as low as 1 share, allowing for easy price manipulation. To achieve this the attacker would do the following:

1. User deposits MinPoolLiquidty using one of the functions
2. User transfers 1 share to another address controlled by him (so he does not lose any value)
3. User withdraws all his shares using withdraw_asset_amount()
4. The function will pass as it does not check the whole pool liquidity
5. The pool now only has 1 share of liquidity inside and can easily be manipulated

2. DOSing withdrawing through remove_liquidity_one_asset for others

Let's consider a case where there are only 2 liquidity providers and one of them is malicious and wants to prevent the other from withdrawing through remove_liquidity_one_asset(). This could for example be the case if the other is a smart contract, that can only withdraw through this function.

1. Both deposit MinPoolLiquidty
2. Malicious user transfers 1 share to another address controlled by him (so he does not lose any value)
3. Malicious user withdraws all his liquidity using withdraw_asset_amount()
4. Normal user now tries to withdraw all of his liquidity using remove_liquidity_one_asset()
5. The call fails as the total pool liquidity (which is checked in this one) would fall below MinPoolLiquidty
6. The user is forced to keep his liquidity in the pool until someone else adds liquidity.

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by also adding a check for the total pool liquidity to withdraw_asset_amount()

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

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as high quality report

[c4-sponsor]

enthusiastmartin (sponsor) confirmed

[c4-sponsor]

enthusiastmartin marked the issue as disagree with severity

[enthusiastmartin]

Although the check is missing,the issue is not high risk. Any limit that we have in our AMM are soft limits, meaning it is designed to protect mainly users, they dont have to be always respected.

there is no evidence, that the state of pool would be exploitable.

[OpenCoreCH]

The warden identified how a security limit can be circumvented in some rare edge cases and how this could lead to a temporary DoS, Medium is appropriate here.

[c4-judge]

OpenCoreCH marked the issue as selected for report

[Frankcastleauditor]

Hi @OpenCoreCH
I believe the severity of this issue should be reconsidered due to the impact it has:

1. This issue will not lead to a DoS or a lock of funds, as the liquidity provider can withdraw all their liquidity by calling the function withdraw_asset_amount() instead of remove_liquidity_one_asset, which encounters an issue with the limit of minimum liquidity. Thus, the user can simply withdraw all their liquidity in the same manner the attacker has, since the function withdraw_asset_amount() does not check for a minimum limit of shares remaining in the pool. Therefore, there is no risk of funds being locked or DoS for the liquidity providers.
   the report mentioned that :

3. A malicious user withdraws all their liquidity using withdraw_asset_amount().
4. A normal user then tries to withdraw all of their liquidity using remove_liquidity_one_asset().

Here, the normal user can use the function withdraw_asset_amount() instead of remove_liquidity_one_asset(), and the entire liquidity removal will be completed.

Therefore, the only impact of this issue is allowing dust accounts to exist in the pool without any other impact, which should not be considered a medium severity issue.

[J4X-98]

Hey @Frankcastleauditor,

you are correct that the user could use remove_liquidity_one_asset() to withdraw his shares, but this would require him to abuse the same issue as the malicious user.

1. DOS

Regarding the DOS, this issue still leads to a DOS on one of the functions of the protocol, which suffices medium, as per the severity guidelines Med requires "Assets not at direct risk, but the function of the protocol or its availability could be impacted". In this case the function of remove_liquidity_one_asset() is clearly impacted and not usable. I mentioned in my issue that the user could be a contract, which is programmed to interact through the remove_liquidity_one_asset() function. As a lot of the interactions with an AMM are actually contracts and not EOA, this is a very usual case. The contract can't be changed later on so it would never be able to withdraw its shares again, although being 100% correctly programmed.

2. Broken Invariant

From the code, one can see that the intended invariant for the liquidity in a pool is sharesInPool == 0 || sharesInPool >= MinPoolLiquidty. This is done so that pools with very low liquidity can't exist as they can easily be manipulated, which a lot of other AMMs do too. The sponsor described this as "to protect mainly the users" in the comment above. This issue shows in "1. Breaking the invariant and letting the pool Liquidity fall below MinPoolLiquidity" how this invariant can be broken so that the pool is easily manipulatable. How this should be graded can be seen in the Supreme Court decisions:

High - A core invariant of the protocol can be broken for an extended duration.

Medium - A non-core invariant of the protocol can be broken for an extended duration or at scale, or an otherwise high-severity issue is reduced due to hypotheticals or external factors affecting likelihood.

3. Conclusion

So in total, the issue leads to 2 impacts, which both would be (at least) of medium severity:

1. DOS of the remove_liquidity_one_asset() function.
2. Breaking of the Pool liquidity invariant