Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No slippage protection for adding liquidity in stableswap pool for add_liquidity() #152

Closed
c4-bot-2 opened this issue Mar 1, 2024 · 2 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-93 🤖_15_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-bot-2
Copy link
Contributor

c4-bot-2 commented Mar 1, 2024

Lines of code

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

Vulnerability details

Impact

In stableswap pool, add_liquidity() lacks slippage protection, and users might lose shares due to trading activities or price manipulation.

Proof of Concept

In stableswap pool, slippage protection is not consistently implemented. Notably all other trading and liquidity management functions such as (sell(),buy(),add_liquidity_shares(), remove_liquidity_one_asset(),etc) have slippage protection, except for add_liquidity().

add_liquidity() takes an array of asset amount to add liquidity but allows for any calculated share amount (share_amount) to pass.

//HydraDX-node/pallets/stableswap/src/lib.rs
		pub fn add_liquidity(
			origin: OriginFor<T>,
			pool_id: T::AssetId,
|>			assets: Vec<AssetAmount<T::AssetId>>,//@audit no slippage protection parameter (e.g. min_share_amount)
		) -> DispatchResult {
...
			let shares = Self::do_add_liquidity(&who, pool_id, &assets)?;
...
}

	fn do_add_liquidity(
		who: &T::AccountId,
		pool_id: T::AssetId,
		assets: &[AssetAmount<T::AssetId>],
	) -> Result<Balance, DispatchError> {
...
		let share_amount = hydra_dx_math::stableswap::calculate_shares::<D_ITERATIONS>(
			&initial_reserves,//@audit-info note: old reserves
			&updated_reserves,//@audit-info note: new reserves
			amplification,
			share_issuance,
			pool.fee,
		)
		.ok_or(ArithmeticError::Overflow)?;
                //@audit only non-zero value is checked
|>		ensure!(!share_amount.is_zero(), Error::<T>::InvalidAssetAmount);
		let current_share_balance = T::Currency::free_balance(pool_id, who);
                //@audit only checked that share_amount will not result in dust liquidity
|>		ensure!(
			current_share_balance.saturating_add(share_amount) >= T::MinPoolLiquidity::get(),
			Error::<T>::InsufficientShareBalance
		);

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

As seen above, any non_zero/non_dust share_amount will be accepted. The user is unprotected from slippage when adding liquidity.

Tools Used

Manual

Recommended Mitigation Steps

Add slippage protection in add_liquidity(), similar to other trading and liquidity management methods.

Assessed type

Other

@c4-bot-2 c4-bot-2 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Mar 1, 2024
c4-bot-7 added a commit that referenced this issue Mar 1, 2024
@c4-bot-11 c4-bot-11 added the 🤖_15_group AI based duplicate group recommendation label Mar 1, 2024
@c4-pre-sort
Copy link

0xRobocop marked the issue as duplicate of #158

@c4-judge c4-judge added duplicate-93 satisfactory satisfies C4 submission criteria; eligible for awards and removed duplicate-158 labels Mar 7, 2024
@c4-judge
Copy link

c4-judge commented Mar 7, 2024

OpenCoreCH marked the issue as satisfactory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-93 🤖_15_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

4 participants