HydraDX - oakcobalt's results

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-L719

Vulnerability details

Impact

Omnipool only have slippage protection for trading(sell/buy) but no slippage protections for liquidity managements(add/remove).

Liquidity providers are exposed to risk of loss in share amount or underlying asset amount due to dynamic price changes.

Proof of Concept

The vulnerability is that in Omnipool slippage protections are not consistently implemented. Notably, slippage protection are only implemented for trading(sell()/buy()) but missed in add_liquidity() and remove_liquidity().

(1) add_liquidity() Users will input an amount of asset token to add to the pool. The amount of lp share(delta_shares_hp) is calculated based on the ratio of asset amount (amount_hp) and total reserve amount (reserve_hp). When trading activities increase the total reserve amount (reserve_hp), lp share(delta_shares_hp) will be decreased, resulting in less share minted for user than expected.

//HydraDX-node/math/src/omnipool/math.rs
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);
        //@audit-info note: when total asset reserve(reserve_hp) increase due to a swap tx, delta_shares_hp decreases, and resulting in less lp shares minted for user.
|>	let delta_shares_hp = shares_hp
		.checked_mul(amount_hp)
		.and_then(|v| v.checked_div(reserve_hp))?;
...

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/omnipool/math.rs#L267)

add_liquidity() doesn't have an input parameter for slippage protection. As a result, users would have to accept any amount LP shares.

//HydraDX-node/pallets/omnipool/src/lib.rs
		pub fn add_liquidity(origin: OriginFor<T>, asset: T::AssetId, amount: Balance) -> DispatchResult {
...

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

(2) remove_liquidity() Users will input an amount of shares to remove, but there is no slippage protection on a minimum amount of the asset token for liquidity providers.

//HydraDX-node/pallets/omnipool/src/lib.rs
		pub fn remove_liquidity(
			origin: OriginFor<T>,
			position_id: T::PositionItemId,
			amount: Balance,
		) -> DispatchResult {

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

The amount of asset token(delta_reserve_hp) is also calculated based on the ratio of user input share amount (delta_share_hp) and total shares of the sub pool (current_share_hp) and current reserve (current_reserve_hp).

There are more challenging scenarios compared to add_liquidity() that change the amount of asset token entitled for liquidity providers: a) Similar to (1), when total asset reserves(current_reserve_hp) changes due to swapping (e.g. swapping out asset tokens), there could be much less reserve tokens to withdraw for LPs;

//HydraDX-node/math/src/omnipool/math.rs
	let delta_reserve_hp = current_reserve_hp
		.checked_mul(delta_shares_hp)
		.and_then(|v| v.checked_div(current_shares_hp))?;

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/omnipool/math.rs#L356)

b) Depending on the asset token price deviation between price when LP's position opened (position price) and current spot price (current price), users might lose an additional amount of asset tokens due to protocol's hub asset imbalance mechanisms.

Specifically, when current_price < position_price, user's share will be further deducted by an amount confiscated by the protocol(delta_b_hp) first, before being calculated into an asset token amount. In this case, user directly loses an amount of share, which is a case a user might want to avoid.

//HydraDX-node/math/src/omnipool/math.rs
	let delta_b_hp = if current_price < position_price {
		let numer = p_x_r
			.checked_sub(current_hub_reserve_hp)?
			.checked_mul(shares_removed_hp)?;
		let denom = p_x_r.checked_add(current_hub_reserve_hp)?;
		numer.checked_div(denom)?.checked_add(U256::one())? // round up
...
        //@audit-info note: in this case, the actual amount of share calculated into asset token is first reduced by `delta_b_hp`
|>	let delta_shares_hp = shares_removed_hp.checked_sub(delta_b_hp)?;

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/omnipool/math.rs#L344)

For comparison, in trading functions such as sell() and buy(), slippage protection is enabled which allows a minimal output amount check to prevent unacceptable losses for users.

Tools Used

Manual

Recommended Mitigation Steps

Enable slippage protection for add/remove liquidity as well.

Assessed type

Other

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

Lines of code

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

Vulnerability details

Impact

Users will be overcharged withdrawal_fee when they are not supposed to pay the withdrawal fee.

Proof of Concept

withdrawal_fee in Omnipool is a dynamic fee and is intended to make spot price manipulation less profitable. Users are only supposed to be charged withdrawal_fee when the subpool is actively tradable (safe_withdrawal == false). And when asset token trading is disabled (safe_withdrawal == true), withdrawal_fee shouldn't be charged because no trading is allowed anymore.

Code comments also echo this point:

HydraDX-node/pallets/omnipool/src/lib.rs
|>		/// Dynamic withdrawal fee is applied if withdrawal is not safe. It is calculated using spot price and external price oracle.
		/// Withdrawal is considered safe when trading is disabled.

The issue is current implementation in remove_liquidity() will still charge user withdrawal_fee even when the asset token trading is disabled and withdrawal is considered safe. Users will be overcharged fee.

(1) First, we see withdrawal_fee is still calculated regardless of safe_withdrawal toggle. withdrawal_fee as a percentage will never be 0 due to range mapping(clamp) in calculate_withdrawal_fee().

//HydraDX-node/pallets/omnipool/src/lib.rs
		pub fn remove_liquidity(
			origin: OriginFor<T>,
			position_id: T::PositionItemId,
			amount: Balance,
		) -> DispatchResult {
... 
                        //@audit `safe_withdrawal` toggle is cached but not used when determine `withdrawal_fee`
|>			let safe_withdrawal = asset_state.tradable.is_safe_withdrawal();
...
                        //@audit `withdrawal_fee`  will be calculated regardless of `safe_withdrawal` toggle
|>			let withdrawal_fee = hydra_dx_math::omnipool::calculate_withdrawal_fee(
				asset_state.price().ok_or(ArithmeticError::DivisionByZero)?,
				FixedU128::checked_from_rational(ext_asset_price.n, ext_asset_price.d)
					.defensive_ok_or(Error::<T>::InvalidOraclePrice)?, //@audit-info note: defensive_ok_or() converts option to result
				T::MinWithdrawalFee::get(),
			);
...

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

//HydraDX-node/math/src/omnipool/math.rs
pub fn calculate_withdrawal_fee(
	spot_price: FixedU128,
	oracle_price: FixedU128,
	min_withdrawal_fee: Permill,
) -> FixedU128 {
...
       //@audit-info note: The resulting calculated fee percentage is always within `[min_fee, 1]`
	price_diff.div(oracle_price).clamp(min_fee, FixedU128::one())

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/omnipool/math.rs#L306)

(2) Second, we see the calculated withdrawal_fee is input in calculate_remove_liquidity_state_changes() to calculate asset token state change.withdrawal_fee is a ratio and will be used to deduct user-entitled reserve token amount (delta_reserve).

//HydraDX-node/math/src/omnipool/math.rs
pub fn calculate_remove_liquidity_state_changes(
	asset_state: &AssetReserveState<Balance>,
	shares_removed: Balance,
	position: &Position<Balance>,
	imbalance: I129<Balance>,
	total_hub_reserve: Balance,
|>	withdrawal_fee: FixedU128,
) -> Option<LiquidityStateChange<Balance>> {
	let fee_complement = FixedU128::one().saturating_sub(withdrawal_fee);
        //@audit withdrawal_fee will always reduce `delta_reserve` in `calculate_remove_liquidity_state_changes`. 
	// Apply withdrawal fee
|>	let delta_reserve = fee_complement.checked_mul_int(delta_reserve)?;
...

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/omnipool/math.rs#L387-L390)

As seen from above, user will receive less in reserve asset (delta_reserve) due to withdrawal fee deduction even when withdrawal is safe.

Tools Used

Manual

Recommended Mitigation Steps

Use safe_withdrawal toggle when calculating withdrawal_fee.

Assessed type

Error

Low-01 Incorrect comment - amount should be asset bought, not asset sold

instances(1) In Omnipool -buy(), code comment for parameter amount is incorrect. buy() allows the user to enter the amount to buy, not amount to sell.

//HydraDX-node/pallets/omnipool/src/lib.rs
		/// Parameters:
		/// - `asset_in`: ID of asset sold to the pool
		/// - `asset_out`: ID of asset bought from the pool
|>		/// - `amount`: Amount of asset sold //@audit this should be Amount of asset bought
		/// - `max_sell_amount`: Maximum amount to be sold.
		///
		/// Emits `BuyExecuted` event when successful.
		///
		#[pallet::call_index(6)]
		#[pallet::weight(<T as Config>::WeightInfo::buy()
			.saturating_add(T::OmnipoolHooks::on_trade_weight())
			.saturating_add(T::OmnipoolHooks::on_liquidity_changed_weight())
		)]
		#[transactional]
		pub fn buy(
			origin: OriginFor<T>,
			asset_out: T::AssetId,
			asset_in: T::AssetId,
			amount: Balance,
			max_sell_amount: Balance,
		) -> DispatchResult {
...

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

Recommendations: Change the comment into Amount of the asset bought.

Low-02 Incorrect comment - Should be Swap hub asset for asset

instances(1) In Omnipool -buy_asset_for_hub_asset(), the code comment says /// Swap asset for Hub Asset, which is incorrect. it should be Swap hub asset for asset.

//HydraDX-node/pallets/omnipool/src/lib.rs
        //@audit incorrect comment - should be Swap hub asset for asset.
|>	/// Swap asset for Hub Asset
	/// Special handling of buy trade where asset in is Hub Asset.
	fn buy_asset_for_hub_asset(
		origin: T::RuntimeOrigin,
		who: &T::AccountId,
		asset_out: T::AssetId,
		amount: Balance,
		limit: Balance,
	) -> DispatchResult {
...

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

Recommendations: Change the comment into Swap hub asset for asset.

Low-03 withdraw_protocol_liquidity() doesn't check whether safe_withdrawal is true but only assume it is true

instances(1) In Omnipool -withdraw_protocol_liquidity(), it assumes the function is only called when safe_withdrawal is true (no trading is allowed), however, it doesn't check safe_withdrawal value.

//HydraDX-node/pallets/omnipool/src/lib.rs
		pub fn withdraw_protocol_liquidity(
			origin: OriginFor<T>,
			asset_id: T::AssetId,
			amount: Balance,
			price: (Balance, Balance),
			dest: T::AccountId,
		) -> DispatchResult {
			T::AuthorityOrigin::ensure_origin(origin.clone())?;
...
			// Callback hook info
//@audit note: AssetInfo is input with hardcoded as `true` which indicates safe_withdrawal is true
|>			let info: AssetInfo<T::AssetId, Balance> =
				AssetInfo::new(asset_id, &asset_state, &new_asset_state, &state_changes.asset, true);
...

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

Due to this function is access-controlled, this is low severity considering AutorityOrigin will not call it when safe_withdrawal is false.

Recommendations: Add a check to ensure safe_withdrawal is true, instead of hardcoding true value.

Low-04 pool.is_valid() will only check the minimum number of assets , not maximum number assets

instances(1) In stableswap - do_create_pool(), pool validity is check in is_valid().

However, is_valid() only ensure there are minimum (2) assets in the pool but no maximum number cap is checked. Based on doc and lib.rs, we know the maximum number cap is (5). (pub const MAX_ASSETS_IN_POOL: u32 = 5;)

//HydraDX-node/pallets/stableswap/src/lib.rs
fn do_create_pool(
...
//@audit pool.is_valid() only check wether there're at least two assets in the pool, but it didn't check whether the max is over 5. 
|>		ensure!(pool.is_valid(), Error::<T>::IncorrectAssets);
...
pub(crate) fn is_valid(&self) -> bool {
|> self.assets.len() >= 2 && has_unique_elements(&mut self.assets.iter())
}

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

Recommendations: Check both minimum number and maximum number.

Low-05 Single constant number of MinTradingLimit/ MinPoolLiquidity might not fit assets with various decimal precision.

instances(1) In stableswap pool, MinTradingLimit/ MinPoolLiquidity are used to check trading and liquidity management. However, both MinTradingLimit and MinPoolLiquidity are hardcoded Balance which might not fit all possible assets decimal precisions stableswap intends to support. It might over constraint or under constraint depending on exact token decimal place, which allow user to deposit or trade dust amount.

		/// Minimum pool liquidity
		#[pallet::constant]
		type MinPoolLiquidity: Get<Balance>;

		/// Minimum trading amount
		#[pallet::constant]
		type MinTradingLimit: Get<Balance>;

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

Recommendations: Consider allowing this to be configurable perhaps by pool or by asset. Or trading limit can be enforced as a ratio as well.

Low-06 Stableswap pool will always have a portion of funds locked inside due to vulnerable fee implementation.

instances(1) The problem is there is going to be a small amount of assets left in the stableswap pool, because the fee implementation in remove_liquidity_one_asset() and withdraw_asset_amount().

//HydraDX-node/pallets/stableswap/src/lib.rs
		pub fn remove_liquidity_one_asset(
			origin: OriginFor<T>,
			pool_id: T::AssetId,
			asset_id: T::AssetId,
			share_amount: Balance,
			min_amount_out: Balance,
		) -> DispatchResult {
...
			//Calculate how much asset user will receive. Note that the fee is already subtracted from the amount.
                        //@audit fee amount will be deducted no matter the current reserve state
|>			let (amount, fee) = hydra_dx_math::stableswap::calculate_withdraw_one_asset::<D_ITERATIONS, Y_ITERATIONS>(
				&initial_reserves,
				share_amount,
				asset_idx,
				share_issuance,
				amplification,
				pool.fee,
			)
...

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

Recommendations: Allowing the fee feature to be disabled by admin.

Low-07 Unnecessary duplicated computation - In Stableswap, when first time deposit, adjusted_d will be the same as d1(updated_d)

In stableswap, when first time deposit, adjusted_d will be the same as d1(updated_d), this is due to adjusted_reserve == updated_reserves.

    //@audit Low: unnecessary duplicated computation - when first time deposit, adjusted_d will be the same as d1(updated_d), due to adjusted_reserve == updated_reserves
	let adjusted_d = calculate_d::<D>(&adjusted_reserves, amplification)?;

Recommendations: Consider bypass when first-time deposit.