HydraDX - zhaojie'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/stableswap/src/lib.rs#L995 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L1118

Vulnerability details

Impact

Some of the functions in omnipool and stableswap lack slippage protection, and users may not expect the same results when they add liquidity.

For example : omnipool#add_liquidity,hub_asset_liquidity is variable, The share amount available to the user is calculated using calculate_add_liquidity_state_changes, which relies on hub_asset_liquidity, so the user may receive less share amount than expected.

Proof of Concept

omnipool and stableswap's user-facing function,buy sell, have slippage protection added, for example: omnipool's buy function can set max_sell_amount.

slippage protection is also added to add_liquidity_shares in the stablepool, and max_asset_amount can be set:

	#[require_transactional]
	fn do_add_liquidity_shares(
		who: &T::AccountId,
		pool_id: T::AssetId,
		shares: Balance,
		asset_id: T::AssetId,
		max_asset_amount: Balance,
	) -> Result<Balance, DispatchError> {
        .....
        ensure!(amount_in <= max_asset_amount, Error::<T>::SlippageLimit);
    }

The problem is that there are other functions that add liquidity that don't have slippage protection:

1. stableswap#do_add_liquidity do_add_liquidity functions similarly to do_add_liquidity_shares, adding liquidity to stablepool, do_add_liquidity can add liquidity to multiple assets, However, do_add_liquidity does not set SlippageLimit as do_add_liquidity_shares does.

2. omnipool#add_liquidity in omnipool#add_liquidity,hub_asset_liquidity is variable, The share amount available to the user is calculated using calculate_add_liquidity_state_changes, which relies on hub_asset_liquidity, so the user may receive less share amount than expected.

	#[require_transactional]
	fn do_add_liquidity(
		who: &T::AccountId,
		pool_id: T::AssetId,
		assets: &[AssetAmount<T::AssetId>],
	) -> Result<Balance, DispatchError> {

@>		let current_hub_asset_liquidity =
				T::Currency::free_balance(T::HubAssetId::get(), &Self::protocol_account());

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

        ....
        let lp_position = Position::<Balance, T::AssetId> {
            asset_id: asset,
            amount,
@>          shares: *state_changes.asset.delta_shares,
            // Note: position needs price after asset state is updated.
            price: (new_asset_state.hub_reserve, new_asset_state.reserve),
        };
        .....
    }

3. omnipool#remove_liquidity Similar to add_liquidity

Tools Used

vscode, manual

Recommended Mitigation Steps

Add slippage protection to all necessary functions

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

The administrator may not be able to remove tokens from the omnipool due to restrictions in omnipool#remove_token

Proof of Concept

In remove_token, we need to make sure that asset_state.shares == asset_state.protocol_shares, This restriction is to ensure that asset no shares owned by LPs.

    #[transactional]
    pub fn remove_token(origin: OriginFor<T>, asset_id: T::AssetId, beneficiary: T::AccountId) -> DispatchResult {
        .....
        ensure!(
            asset_state.shares == asset_state.protocol_shares,
            Error::<T>::SharesRemaining
        );
        ....
    }

The problem is that this condition may never be met, There are two reasons:

1. When initializing token, shares = amount, protocol_shares= 0, so shares > protocol_shares:

    pub fn add_token(
        origin: OriginFor<T>,
        asset: T::AssetId,
        initial_price: Price,
        weight_cap: Permill,
        position_owner: T::AccountId,
    ) -> DispatchResult {
        ......
        let amount = T::Currency::free_balance(asset, &Self::protocol_account());
        ....
        
        // Initial state of asset
        let state = AssetState::<Balance> {
            hub_reserve,
@>          shares: amount,
@>          protocol_shares: Balance::zero(),
            cap: FixedU128::from(weight_cap).into_inner(),
            tradable: Tradability::default(),
        };
        .....
        <Assets<T>>::insert(asset, state);
    }

2. Due to the rounding error of division in the running process of the protocol, the increased share of import is not equal to the removed share.

Tools Used

vscode, manual

Recommended Mitigation Steps

Use another way to determine whether the LPs owns the asset shares

Assessed type

Other