Removal of Tokens can be blocked at the cost of 1$

[c4-bot-10]

Lines of code

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

Vulnerability details

Bug Description

The HydraDX protocol includes the Omnipool AMM. It allows for the registration of single tokens through the Authority. When a token becomes compromised or worthless, an authority can also remove a token from the Omnipool using remove_token().

To be able to remove a token, the token has to fulfill 2 requirements.

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

First, the token has to be in the FROZEN state. This can easily be achieved by the Authority by setting the asset's tradable state using the function set_asset_tradable_state().

The second requirement is that all remaining shares of the asset have to be held by the protocol. Here is where the problem occurs. If only a single user does not withdraw all his shares, the remove_token() will always fail. This requires some loss on the user's behalf, as the market will probably stay frozen and his shares won't earn any interest and will be stuck until he stops the DOS. This might seem to deter attackers from abusing this, but in reality, an attacker would only need to keep 1 share inside the pool. For example, for the USDC used in the test, this would only cost him 1$ total, to keep an endless DOS on the remove_token() functionality.

Impact

The issue allows a malicious user to DOS the Omnipool's removeToken() functionality indefinitely at a cost of ~1$.

Proof of Concept

The issue can be seen in the following testcase, where a user blocks the removeToken() functionality at the cost of ~1$

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

            //Withdraw all but 1 share of liquidity
            assert_ok!(Omnipool::remove_liquidity(RuntimeOrigin::signed(LP1), current_position_id, 399 * ONE));

            assert_ok!(Omnipool::set_asset_tradable_state(
                RuntimeOrigin::root(),
                1_000,
                Tradability::FROZEN
            ));

            assert_noop!(
                Omnipool::remove_token(RuntimeOrigin::root(), 1000, LP1),
                Error::<Test>::SharesRemaining
            );
        });
}

The test can be added to the pallets/omnipool/src/tests/remove_token.rs file.

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by separating the token removal and liquidity withdrawals. A simple way to facilitate this would be to implement an escrow mechanism. This way when a token gets removed, all remaining liquidity would be transferred to an escrow where the holders can withdraw it.

Assessed type

DoS

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #180

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[J4X-98]

Hi @OpenCoreCH,

thank you very much for reviewing this contest. While I understand your reasoning in the primary issue #180 I would like to add why in my opinion this issue fulfills a medium severity according to C4 judging criteria.

1. C4 severity guidelines for a Medium severity issue state "Assets not at direct risk, but the function of the protocol or its availability could be impacted". Tokens being removable is intentional, otherwise the function would not have been added at all. As the DOS is always possible and very low cost, anyone can easily do it for the next 100 years and block the removal of any token that ever gets added to the protocol. So in this case the intended function of the protocol is clearly impacted.
2. Your reasoning for this being a correct security measure against malicious governance is correct. That's why I did not recommend removing this check. Instead, my recommendation was to decouple the liquidity withdrawal and removal process. This way a token can be frozen for trading and removed, with user shares still existing and the users being able to withdraw later.

Also, I'd like to add that of the duplicates of #180 only this issue, #144 and #26 describe the same and correct issue. #161 mentions some invalid rounding issue.

[OpenCoreCH]

While I understand your reasoning, the fact that the protocol does not work as intended alone is not a guarantee for a valid medium (see https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023). For this issue, while it would definitely be annoying that this call fails, the issue does not show how this would impact users, their interaction with the protocol, or what the benefit of an attacker after performing this attack would be.