c4-bot-9 commented 6 months ago

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:

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);
    }

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

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as insufficient quality report

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as duplicate of #180

c4-judge commented 6 months ago

OpenCoreCH changed the severity to QA (Quality Assurance)