Missing slippage protection in omnipool::add_liquidity and omnipool::remove_liquidity

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

The omnipool pallet is missing slippage protection when adding and removing liquidity. That makes users vulnerable from receiving less shares/assets than intended, as the amount depends on the current state of the pool, which can be manipulated to a certain point. That's why Uniswap makes it possible for users to specify the minimum amount they are willing to receive (check this and this), so that their transaction will revert if they receive less than intended.

Proof of Concept

NOTE $\Rightarrow$ I will work with the addition of liquidity as a general example. The same applies (with different state equations) to the remove of liquidity.

If we go to the math implementation of the state transition when adding liquidity in

omnipool/math.rs, function calculate_add_liquidity_state_changes

/// Calculate delta changes of add liqudiity given current asset state
pub fn calculate_add_liquidity_state_changes(
	asset_state: &AssetReserveState<Balance>,
	amount: Balance,
	imbalance: I129<Balance>,
	total_hub_reserve: Balance,
) -> Option<LiquidityStateChange<Balance>> {
	...

	let (amount_hp, shares_hp, reserve_hp) = to_u256!(amount, asset_state.shares, asset_state.reserve);

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

	...

	let delta_shares = to_balance!(delta_shares_hp).ok()?;

	...
}

we can see that the amount of shares users will receive for their assets will be, roughly:

$$issued_shares = shares_prev * amount_deposited \ / \ reserves$$

which depend directly on the "current" state of the chain. On top of that, such value is never checked to be above a certain threshold specified either by the caller or the system in omnipool, function add_liquidity, whcih effectively makes users receive ANY amount of shares, even 0, for their deposited assets, which is indeed a loss of funds.

The runnable POC is the next one (put it inside the tests folder, file add_liquidity.rs):

#[test]
fn poc_add_liquidity_slippage() {
	ExtBuilder::default()
		.add_endowed_accounts((LP1, 1_000, 5000 * ONE))
		.add_endowed_accounts((LP2, 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;

            // @audit say a transaction from other user got first to the HydraDX node
			assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP2), 1_000, liq_added));

			// ACT
			let position_id = last_position_id();
			assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), 1_000, liq_added));
            
            @audit what LP1 user got
			let position = Positions::<Test>::get(position_id).unwrap();

            // @audit what the LP1 user expected
			let expected = Position::<Balance, AssetId> {
				asset_id: 1_000,
				amount: liq_added,
				shares: liq_added,
				price: (1560 * ONE, token_amount + liq_added),
			};

            // @audit they are definetly not the same, slippage control missing
			assert_ne!(position, expected);
		});
}

Recommended Mitigation Steps

Make it possible for users to specify a certain threshold by which the transaction will revert if they receive les shares/assets for their funds, as done in stableswap, function remove_liquidity_one_asset or the Uniswap example I mentioned above.

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #158

[c4-judge]

OpenCoreCH marked the issue as satisfactory

[c4-judge]

OpenCoreCH changed the severity to 2 (Med Risk)