complete liquidity removal will result in permanent disable of the liquidity addition and prevent minting shares for the liquidity providers .

mediumCode4rena

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/omnipool/src/lib.rs#L612-L621 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/omnipool/math.rs#L267-L269

Vulnerability details

Impact

This vulnerability will lead to prevent any liquidity provider from adding liquidity and prevent them from minting new shares , so this is considered a huge loss of funds for the users and the protocol .

1. No New Liquidity: Users can no longer add liquidity to the pool, hindering its growth and potential.
2. Complete liquidity removal shuts down the pool, preventing any future activity.
3. Financial losses for the protocol : It loses the benefits of increased liquidity and potential fees from user activity.

Proof of Concept

Adding a new token to the omnipool requires an initial liquidity deposit. This initial deposit mints the first batch of shares. Subsequent liquidity additions mint new shares proportionally to the existing total shares, ensuring a fair distribution based on the pool's current size. .

In the function remove_liquidity it is allowed to remove all amount of liquidity from the pool , which means burning all amount of the shares from the pool , as shown here

rust📋
                        let state_changes = hydra_dx_math::omnipool::calculate_remove_liquidity_state_changes(
                                &(&asset_state).into(),
                                amount,
                                &(&position).into(),
                                I129 {
                                        value: current_imbalance.value,
                                        negative: current_imbalance.negative,
                                },
                                current_hub_asset_liquidity,
                                withdrawal_fee,
                        )
                        .ok_or(ArithmeticError::Overflow)?;


                        let new_asset_state = asset_state
                                .clone()
                                .delta_update(&state_changes.asset)
                                .ok_or(ArithmeticError::Overflow)?;

the function calculate_remove_liquidity_state_changes calculates the shares to be burnt and the delta_update function removes them .

If all liquidity shares have been removed from any pool ,protocol shares and user shares are removed , the asset_reserve equal to zero and shares equal to zero , this will prevent any liquidity from been added , because the function add_liquidity does not handle the situation where there is no liquidity in the pool , as shown in add_liquidity function it calls calculate_add_liquidity_state_changes which calculate the shares to be minted to the lp as shown here

rust📋
        let delta_shares_hp = shares_hp
                .checked_mul(amount_hp)
                .and_then(|v| v.checked_div(reserve_hp))?;

the state of the pool after all liquidity have been removed asset_reserve = 0 , shares = 0 Since there is no liquidity in the pool so the reserve_hp will be equal to zero , so this part will always return error , so this function will always revert .

even if any user donates some assets to prevent this function from reverting , the liquidity provider will always receive zero shares , since the delta_shares_hp will always equal to zero , which is considered loss of funds for the user and the protocol .

the bad state that will cause this vulnerability : token has been added to the pool but all liquidity have been removed from it .

Coded Poc :

consider add this test in remove_liquidity.rs test file , and run it to see the logs .

rust📋
#[test]
fn full_liquidity_removal_then_add_liquidity() {
	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(Omnipool::protocol_account(), DAI, 1000 * ONE),
			(Omnipool::protocol_account(), HDX, NATIVE_AMOUNT),
			(LP2, 1_000, 2000 * ONE),
			(LP1, 1_000, 5000 * ONE),
		])
		.with_initial_pool(FixedU128::from_float(0.5), FixedU128::from(1))
		.with_token(1_000, FixedU128::from_float(0.65), LP2, 2000 * ONE)
		.build()
		.execute_with(|| {
			// let token_amount = 2000 * ONE;

			let liq_added = 400 * ONE;
			let lp1_position_id = <NextPositionId<Test>>::get();
			assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), 1_000, liq_added));

			let liq_removed = 400 * ONE;
			println!(
				"asset state before liquidity removal {:?} ",
				Omnipool::load_asset_state(1000).unwrap()
			);

			assert_ok!(Omnipool::remove_liquidity(
				RuntimeOrigin::signed(LP1),
				lp1_position_id,
				liq_removed
			));

			assert!(
				Positions::<Test>::get(lp1_position_id).is_none(),
				"Position still found"
			);
			assert!(
				get_mock_minted_position(lp1_position_id).is_none(),
				"Position instance was not burned"
			);
			let pos = Positions::<Test>::get(2);
			println!(" the lp2_position before all liquidity removal : {:?}", pos.unwrap());
			// lp2 remove his all initial liquidity
			assert_ok!(Omnipool::remove_liquidity(RuntimeOrigin::signed(LP2), 2, 2000 * ONE));
			let lp2_position = lp1_position_id - 1;

			assert!(Positions::<Test>::get(lp2_position).is_none(), "Position still found");

			println!(
				"the final state after all liquidity has been removed : {:?} ",
				Omnipool::load_asset_state(1000).unwrap()
			);
			let liq_added = 400 * ONE;
			assert_noop!(
				Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), 1_000, liq_added),
				ArithmeticError::Overflow
			);
			// this make sure that there is no position created after
			assert!(Positions::<Test>::get(lp2_position).is_none(), "Position still found");

			println!(
				"the new state after liquidity provision reverted : {:?}",
				Omnipool::load_asset_state(1000).unwrap()
			);
		});
}

the logs will be

rust📋
running 1 test
asset state before liquidity removal AssetReserveState { reserve: 2400000000000000, hub_reserve: 1560000000000000, shares: 2400000000000000, protocol_shares: 0, cap: 1000000000000000000, tradable: SELL | BUY | ADD_LIQUIDITY | REMOVE_LIQUIDITY } 
 the lp2_position before all liquidity removal : Position { asset_id: 1000, amount: 2000000000000000, shares: 2000000000000000, price: (650000000000000000, 1000000000000000000) }
the final state after all liquidity has been removed : AssetReserveState { reserve: 0, hub_reserve: 0, shares: 0, protocol_shares: 0, cap: 1000000000000000000, tradable: SELL | BUY | ADD_LIQUIDITY | REMOVE_LIQUIDITY } 
the new state after liquidity provision reverted : AssetReserveState { reserve: 0, hub_reserve: 0, shares: 0, protocol_shares: 0, cap: 1000000000000000000, tradable: SELL | BUY | ADD_LIQUIDITY | REMOVE_LIQUIDITY }

which illustrates that add_liquidity function failed after lp2 and lp1 removed their entire liquidity from the pool .

Tools Used

vs code and manual review

Recommended Mitigation Steps

add a special behaviour to the function add_liquidity to handle the situation of no initial liquidity the mitigation can be done by

• When the pool initially has no shares (total shares equal zero), newly added assets from a liquidity provider trigger the minting of shares in an amount equal to the added asset value as happened in the function add_token() here

rust📋

                                delta_shares: BalanceUpdate::Increase(amount),

Assessed type

Context