A malicious user can block remove_token function in Omnipool by holding a tiny amount of shares

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

A malicious user can easily block the remove_token() function.

Proof of Concept

In Omnipool we have remove_token() function which is used by the admin to remove a token:

	    #[pallet::call_index(12)]
        #[pallet::weight(<T as Config>::WeightInfo::remove_token())]
        #[transactional]
        pub fn remove_token(origin: OriginFor<T>, asset_id: T::AssetId, beneficiary: T::AccountId) -> DispatchResult {
            T::AuthorityOrigin::ensure_origin(origin)?;
            let asset_state = Self::load_asset_state(asset_id)?;

            // Allow only if no shares owned by LPs and asset is frozen.
            ensure!(asset_state.tradable == Tradability::FROZEN, Error::<T>::AssetNotFrozen);
            ensure!(
                asset_state.shares == asset_state.protocol_shares, 
                Error::<T>::SharesRemaining                        
            );
            // Imbalance update
            let imbalance = <HubAssetImbalance<T>>::get();
            let hub_asset_liquidity = Self::get_hub_asset_balance_of_protocol_account();
            let delta_imbalance = hydra_dx_math::omnipool::calculate_delta_imbalance(
                asset_state.hub_reserve,
                I129 {
                    value: imbalance.value,
                    negative: imbalance.negative,
                },
                hub_asset_liquidity,
            )
            .ok_or(ArithmeticError::Overflow)?;
            Self::update_imbalance(BalanceUpdate::Increase(delta_imbalance))?;

            T::Currency::withdraw(T::HubAssetId::get(), &Self::protocol_account(), asset_state.hub_reserve)?;
            T::Currency::transfer(asset_id, &Self::protocol_account(), &beneficiary, asset_state.reserve)?;
            <Assets<T>>::remove(asset_id);
            Self::deposit_event(Event::TokenRemoved {
                asset_id,
                amount: asset_state.reserve,
                hub_withdrawn: asset_state.hub_reserve,
            }); 
            Ok(())
        }
    }

The asset or token to be removed must have its tradability status set to FROZEN and all remaining shares of the asset must belong to the protocol:

            ensure!(
                asset_state.shares == asset_state.protocol_shares, 
                Error::<T>::SharesRemaining                       
            );

This function can be potentially blocked if a malicious user holds a very small unit value of the shares. Since the function checks for exact equality between asset_state.shares and asset_state.protocol_shares, even a minuscule amount of shares held outside the protocol can prevent the asset's removal. This situation could be exploited by malicious user.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Allow a small tolerance in the shares comparison or introduce a mechanism to buy out or forcibly redeem the remaining shares under specific, strictly governed conditions.

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #180

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-a