Lack of slippage protection when adding liquidity

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L475-L479
https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L995-L1081

Vulnerability details

Impact

The add_liquidity function allows the liquidity providers to provide extra liquidity into the pool, upon which they're given LP shares. The function however doesn't allow the users to define the minimum expected shares that they desire, nor is there a cap on the maximum amount of that can be deposited into the pool. This opens up opportunities for griefing and loss of funds for users, especially those providing lesser liquidity amounts into the pool.
This can also be excarcebated as the pool supports adding and removing liquidity in imbalanced proportions. If a user wants to deposit liquidity in an imbalanced ratio (only one token). A attacker can front run the user by doing the same and removing the liquidity directly after the deposit of the user. By doing so the attacker steals a significant percentage of the users funds.

For simplified example -

1. A pool with 8000 token X and 2000 token Y, as a consequence, creator receives 4000 LP shares in return.
2. User A sends a transaction to provide liquidity of 80 token X and 20 token Y to the pool, so he expects to receive 40 shares in return. Note that there's no option to enter this amount into the function.
3. Some seconds before transaction of user A is processed, user B swaps 12000 token X to 1200 token Y. The final balance in the
   pool is: 20000 token X and 800 token Y.
4. When transaction of user A is processed, he receives 16 shares in return, instead of 40 shares he was expecting, i.e.: less than 50%.

Proof of Concept

add_liquidity doesn't allow users to enter minimum needed shares.

		pub fn add_liquidity(
			origin: OriginFor<T>,
			pool_id: T::AssetId,
			assets: Vec<AssetAmount<T::AssetId>>,
		) -> DispatchResult {
			let who = ensure_signed(origin)?;

			let shares = Self::do_add_liquidity(&who, pool_id, &assets)?;

			Self::deposit_event(Event::LiquidityAdded {
				pool_id,
				who,
				shares,
				assets,
			});

			Ok(())
		}

Here, there's only a check to ensure that the share amount is not zero.

	fn do_add_liquidity(
		who: &T::AccountId,
		pool_id: T::AssetId,
		assets: &[AssetAmount<T::AssetId>],
	) -> Result<Balance, DispatchError> {
...
		let share_issuance = T::Currency::total_issuance(pool_id);
		let share_amount = hydra_dx_math::stableswap::calculate_shares::<D_ITERATIONS>(
			&initial_reserves,
			&updated_reserves,
			amplification,
			share_issuance,
			pool.fee,
		)
		.ok_or(ArithmeticError::Overflow)?;

		ensure!(!share_amount.is_zero(), Error::<T>::InvalidAssetAmount);
		let current_share_balance = T::Currency::free_balance(pool_id, who);

Tools Used

Manual code review

Recommended Mitigation Steps

Consider adding a min_shares parameter and making a comparison with the received shares, or adding a max limit to the amounts of each assets that can be deposited.

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #158

[c4-judge]

OpenCoreCH marked the issue as satisfactory