Missing slippage on Stableswap add_Liquidity()

[c4-bot-1]

Lines of code

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

Vulnerability details

Bug Description

The stableswap AMM in the HydraDX protocol allows users to deposit liquidity to facilitate the market maker functionality. It is a well known issue that calls to add/withdraw liquidty on vaults/amm pools can be frontrun to reduce the users received shares. In the add_liquidity_shares() this is already accounted for as the user provides the amount of shares he wants to receive, and can control his slippage by setting the max_asset_amount variable.

Unfortunately, in the add_liquidity() function, where the user sets how many assets he wants to provide, he is not able to set the minimum amount of assets that he wants to receive. This way the user could have to endure up to 100% slippage, resulting in massive losses.

Impact

This issue allows a malicious attacker to frontrun calls to add_Liquidity() so that users incur high slippage losses. The users can't protect themselves against this as they are not able to set a slippage parameter in the function.

Proof of Concept

The issue can already be spotted when looking at add_liquidty()'s function signature.

#[pallet::call_index(3)]
#[pallet::weight(<T as Config>::WeightInfo::add_liquidity()
					.saturating_add(T::Hooks::on_liquidity_changed_weight(MAX_ASSETS_IN_POOL as usize)))]
#[transactional]
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(())
}

An exemplary situation can be described as follows

1. Alice wants to add liquidity and calls the add_liquidity() function
2. Bob spots this call and front runs it
3. He adds a lot of liquidity/trades so that Alice gets less shares than expected
4. Alice can't set slippage so her transaction will still execute

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by allowing the user to pass a minShares parameter. At the end of the function, it should check if the user receives more shares than this parameter, and otherwise revert.

ensure!(minShares <= shares, Error::<T>::SlippageLimit);

Assessed type

MEV

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