Bypassing Minimum Pool Liquidity Limit via Liquidity Removal Could Lead To DoS

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L820-L843

Vulnerability details

Impact

A user is able to bypass the minimum pool liquidity limit by first depositing liquidity to the pool using add_liquidity() and then removing liquidity using the remove_liquidity() function to remove shares, which results in the pool ending up with dust shares.
Other possible impacts:

• With the passage of time, a large amount of positions with dust shares would be accumulated in the pool either through malicious intent or simply by mistake (e.g a user withdraw tokens but forget a little amount, than he notices that transaction costs is bigger than the left value) leading to the exhausting of protocol resources.
• Griefing the protocol by leaving 1 share attack before a token is frozen to prevent protocol from removing it.

Proof of Concept

The issue can be observed in the remove_liquidity() function in the pallets/omnipool/src/lib.rs file. Specifically, the function does not ensure that the left amount after withdrawal of the token is at least equal to the minimum pool liquidity limit. remove_liquidity() solely checks if the amount of shares to be withdrawn is non zero or the amount of shares left in the position position after after withdrawal is either zero or non-zero
https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L820C4-L843C5

if updated_position.shares == Balance::zero() {
				// All liquidity removed, remove position and burn NFT instance

				<Positions<T>>::remove(position_id);
				T::NFTHandler::burn(&T::NFTCollectionId::get(), &position_id, Some(&who))?;

				Self::deposit_event(Event::PositionDestroyed {
					position_id,
					owner: who.clone(),
				});
			} else {
                // @audit no check for the size of the amt left in pool
				Self::deposit_event(Event::PositionUpdated {
					position_id,
					owner: who.clone(),
					asset: asset_id,
					amount: updated_position.amount,
					shares: updated_position.shares,
					price: updated_position
						.price_from_rational()
						.ok_or(ArithmeticError::DivisionByZero)?,
				});

				<Positions<T>>::insert(position_id, updated_position);
			}

To run the Proof of concept, add the following test to pallets/omnipool/src/tests/add_liquidity.rs

#[test]
fn bypass_insufficient_minimum_liquidity_check(){
	ExtBuilder::default()
		.add_endowed_accounts((LP1, 1_000, 5000 * ONE))
		.with_min_added_liquidity(5 * ONE)
		.with_asset_weight_cap(Permill::from_float(0.1))
		.with_initial_pool(FixedU128::from_float(0.5), FixedU128::from(1))
		.with_token(1_000, FixedU128::from_float(0.65), LP1, 2000 * ONE)
		.build()
		.execute_with(|| {
            // @audit  add_liquidity with minimum liquidity needed
			Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), 1_000, 5 * ONE,);

			// @audit remove liquidity, resulting in the liquidity deposited being less than the minimum liquidity limit
			Omnipool::remove_liquidity(RuntimeOrigin::signed(LP1), 1_000, 4 * ONE,);
		});

}

Result:

test tests::add_liquidity::bypass_insufficient_balance_check ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 128 filtered out; finished in 0.00s

Tools Used

Manual Review, Substrate

Recommended Mitigation Steps

To mitigate this issue, it is recommended to enforce a check during the withdrawal process to ensure that the remaining amount of the token after withdrawal is at least equal to the minimum pool liquidity limit.

The easiest hot-fix, to implement to mitigate this would be to ensure that the remaining position shares are bigger than the minimum liquidity limit

     ensure!(
				updated_position.shares >= T::MinimumPoolLiquidity::get(),
				Error::<T>::InsufficientLiquidity
			);        

To correctly mitigate this, it is necessary to implement the math to calculate the balance of Tokens left and to compare it to the minimum liquidity limit.

Assessed type

Invalid Validation

[0xRobocop]

Similar to the issue reported in #154. But it points to other part of the codebase. The report fails to point where the DoS will come from.

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[OpenCoreCH]

Report just points out that dust can end up in a pool, which is no vulnerability

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory:
Invalid