Bug: DCA terminate false ScheduleNotFound locks funds

[cmoxiu]

Details

Report ID
62795
Target
https://github.com/galacticcouncil/HydraDX-node/blob/master/pallets/dca/src/lib.rs
Blockchain/DLT
Impact(s)

• Blocking users from accessing their funds

Description

Brief/Intro
This is a deterministic mismatch of “storage does not promise ordering vs logic depends on ordering”: ScheduleIdsPerBlock can naturally become unsorted, and terminate using binary_search can fail to find an existing schedule; under #[transactional] semantics this can keep reserves locked long-term and block users from accessing their funds.

Vulnerability Details

1.When terminate removes a schedule_id from ScheduleIdsPerBlock[next_block], it uses:

• schedule_ids.binary_search(&schedule_id)
• If the search fails, it is directly mapped to Error::::ScheduleNotFound and returned (termination fails).

2.However, the write path for ScheduleIdsPerBlock does not guarantee sorting:

• plan_schedule_for_block only appends via try_push(schedule_id) to the end of the vec; there is no sorting or ordered insertion.

3.The same schedule_id is repeatedly “rescheduled” into future blocks during execution:

• paths such as replan_or_complete / retry_schedule / prepare_schedule call plan_schedule_for_block(..., schedule_id, ...) again, so an old id can be appended to the end of a future block’s queue.

4.schedule_id is allocated monotonically (newly created ids are larger). Therefore, a very realistic situation is:

• block X already contains a newly created plan id=100 (it entered X first)
• an old plan id=10 runs and is replanned into the same X, calling try_push(10) again
• the queue becomes [100, 10] (unsorted)
• terminate performing binary_search(10) on an unsorted vec is not guaranteed to find it → false ScheduleNotFound.

Impact Details

reserved funds remain locked while terminate fails; unlock_reserves is blocked while schedules are active.

Fix

remove the implicit “sorted” precondition in terminate and switch to linear search removal:

let idx = schedule_ids
    .iter()
    .position(|x| *x == schedule_id)
    .ok_or(Error::<T>::ScheduleNotFound)?;
schedule_ids.remove(idx);

References

1.Trigger point: terminate uses binary_search to delete (requires the vec to be sorted) [https://github.com/galacticcouncil/hydration-node/blob/master/pallets/dca/src/lib.rs#L626]

let index = schedule_ids
    .binary_search(&schedule_id)
    .map_err(|_| Error::<T>::ScheduleNotFound)?;
schedule_ids.remove(index);

2.Root cause: the queue write path only try_pushes (appends) and does not guarantee ordering [https://github.com/galacticcouncil/hydration-node/blob/master/pallets/dca/src/lib.rs#L1123]

ScheduleIdsPerBlock::<T>::try_mutate(next_free_block, |schedule_ids| -> DispatchResult {
    schedule_ids
        .try_push(schedule_id)
        .map_err(|_| Error::<T>::InvalidState)?;
    Ok(())
})?;

this only appends schedule_id to the end, with no sorting/ordered insertion; when an old id is rescheduled and appended behind newer ids already planned for the same block, the queue can become unsorted (e.g., [big, small]), which then triggers the false-negative binary_search behavior above.

Proof of Concept

add hydration-node/pallets/dca/src/tests/poc.rs

use crate::{assert_that_schedule_has_been_removed_from_storages, Error};
use crate::tests::mock::*;
use crate::tests::*;
use frame_support::traits::Hooks;
use frame_support::{assert_noop, assert_ok};
use hydradx_traits::router::{PoolType, Trade};
use orml_traits::{MultiCurrency, NamedMultiReservableCurrency};
use pretty_assertions::assert_eq;

#[test]
fn terminate_can_fail_and_lock_reserves_when_schedule_ids_per_block_is_unsorted() {
	ExtBuilder::default()
		.with_endowed_accounts(vec![(ALICE, HDX, 10000 * ONE), (BOB, HDX, 10000 * ONE)])
		.build()
		.execute_with(|| {
			let (block_600, schedule_id_0, schedule_id_1) =
				arrange_unsorted_schedule_ids_per_block_600();

			let reserved_before =
				Currencies::reserved_balance_named(&NamedReserveId::get(), HDX, &ALICE);
			assert!(reserved_before > 0);

			assert_noop!(
				DCA::terminate(RuntimeOrigin::signed(ALICE), schedule_id_1, None),
				Error::<Test>::ScheduleNotFound
			);

			assert!(DCA::schedules(schedule_id_1).is_some());
			assert_eq!(DCA::schedule_execution_block(schedule_id_1), Some(block_600));

			let reserved_after_failed_terminate =
				Currencies::reserved_balance_named(&NamedReserveId::get(), HDX, &ALICE);
			assert_eq!(reserved_after_failed_terminate, reserved_before);

			assert_noop!(
				DCA::unlock_reserves(RuntimeOrigin::signed(ALICE), ALICE, HDX),
				Error::<Test>::HasActiveSchedules
			);

			let reserved_after_failed_unlock =
				Currencies::reserved_balance_named(&NamedReserveId::get(), HDX, &ALICE);
			assert_eq!(reserved_after_failed_unlock, reserved_before);

			assert_eq!(
				DCA::schedule_ids_per_block(block_600).to_vec(),
				vec![schedule_id_1, schedule_id_0]
			);

			let events_before = System::events().len();
			let alice_btc_before = Currencies::free_balance(BTC, &ALICE);
			set_block_number(block_600);
			let alice_btc_after = Currencies::free_balance(BTC, &ALICE);
			assert!(alice_btc_after > alice_btc_before);

			let trade_executed = System::events()
				.iter()
				.skip(events_before)
				.any(|record| {
					matches!(
						record.event,
						RuntimeEvent::DCA(crate::Event::<Test>::TradeExecuted {
							id,
							who,
							amount_out,
							..
						}) if id == schedule_id_1 && who == ALICE && amount_out > 0
					)
				});
			assert!(trade_executed);
		});
}

#[test]
#[ignore = "Expected to fail until fixed. Run with `--ignored --nocapture`."]
fn terminate_should_not_depend_on_schedule_ids_per_block_ordering() {
	ExtBuilder::default()
		.with_endowed_accounts(vec![(ALICE, HDX, 10000 * ONE), (BOB, HDX, 10000 * ONE)])
		.build()
		.execute_with(|| {
			let (block_600, schedule_id_0, schedule_id_1) =
				arrange_unsorted_schedule_ids_per_block_600();

			let reserved_before =
				Currencies::reserved_balance_named(&NamedReserveId::get(), HDX, &ALICE);
			assert!(reserved_before > 0);

			let terminate_result =
				DCA::terminate(RuntimeOrigin::signed(ALICE), schedule_id_1, None);

			println!(
				"ScheduleIdsPerBlock[{}] = {:?}",
				block_600,
				DCA::schedule_ids_per_block(block_600).to_vec()
			);
			println!(
				"terminate(ALICE, {}) = {:?}",
				schedule_id_1, terminate_result
			);
			println!(
				"ALICE reserved_balance_named before/after = {}/{}",
				reserved_before,
				Currencies::reserved_balance_named(&NamedReserveId::get(), HDX, &ALICE)
			);

			assert_ok!(terminate_result);

			assert_that_schedule_has_been_removed_from_storages!(ALICE, schedule_id_1);
			assert_eq!(
				DCA::schedule_ids_per_block(block_600).to_vec(),
				vec![schedule_id_0]
			);
			assert_eq!(
				Currencies::reserved_balance_named(&NamedReserveId::get(), HDX, &ALICE),
				0
			);
			assert_noop!(
				DCA::unlock_reserves(RuntimeOrigin::signed(ALICE), ALICE, HDX),
				Error::<Test>::NoReservesLocked
			);
		});
}

fn arrange_unsorted_schedule_ids_per_block_600() -> (u64, ScheduleId, ScheduleId) {
	// Builds `ScheduleIdsPerBlock[600] == [1, 0]` via normal flow.
	let block_502 = 502u64;
	let block_600 = 600u64;
	let schedule_id_0: ScheduleId = 0;
	let schedule_id_1: ScheduleId = 1;

	set_block_number(500);

	let schedule_replans_into_block_600 = ScheduleBuilder::new()
		.with_owner(BOB)
		.with_period(98)
		.with_order(Order::Sell {
			asset_in: HDX,
			asset_out: BTC,
			amount_in: ONE,
			min_amount_out: 0,
			route: create_bounded_vec(vec![Trade {
				pool: PoolType::Omnipool,
				asset_in: HDX,
				asset_out: BTC,
			}]),
		})
		.build();
	assert_ok!(DCA::schedule(
		RuntimeOrigin::signed(BOB),
		schedule_replans_into_block_600,
		None
	));
	assert!(DCA::schedules(schedule_id_0).is_some());

	let schedule_planned_for_block_600 = ScheduleBuilder::new()
		.with_total_amount(0)
		.with_order(Order::Sell {
			asset_in: HDX,
			asset_out: BTC,
			amount_in: ONE,
			min_amount_out: 0,
			route: create_bounded_vec(vec![Trade {
				pool: PoolType::Omnipool,
				asset_in: HDX,
				asset_out: BTC,
			}]),
		})
		.build();
	assert_ok!(DCA::schedule(
		RuntimeOrigin::signed(ALICE),
		schedule_planned_for_block_600,
		Some(block_600)
	));
	assert!(DCA::schedules(schedule_id_1).is_some());

	set_block_number(block_502);

	assert!(DCA::schedules(schedule_id_1).is_some());
	assert_eq!(DCA::schedule_execution_block(schedule_id_1), Some(block_600));
	assert_eq!(
		DCA::schedule_ids_per_block(block_600).to_vec(),
		vec![schedule_id_1, schedule_id_0]
	);
	assert!(
		DCA::schedule_ids_per_block(block_600)
			.iter()
			.any(|id| *id == schedule_id_1)
	);

	(block_600, schedule_id_0, schedule_id_1)
}

fn set_block_number(to: u64) {
	System::set_block_number(to);
	DCA::on_initialize(to);
}

Run

cargo test -p pallet-dca tests::poc -- --include-ignored --nocapture

step-by-step

1.Prepare two accounts: BOB and ALICE (both funded with HDX).

2.BOB creates a DCA schedule that will execute at block N and then be rescheduled into a future block X:

• Pick a period such that the first execution happens at N and the next planned execution lands on X (e.g., period=98 with N=502 and X=600).

3.ALICE creates a newer schedule explicitly planned for the same future block X (schedule(..., Some(X))), ensuring ALICE ends up with a non-zero named reserve locked.

4.Advance to block N and let it execute once (triggering BOB’s schedule execution and rescheduling into X):

• This appends BOB’s old schedule_id to the end of ScheduleIdsPerBlock[X] via try_push, naturally producing an unsorted queue like [ALICE_new_id, BOB_old_id].
• Query ScheduleIdsPerBlock[X] and confirm it is unsorted and contains ALICE’s id.

5.Before block X, have ALICE call terminate(ALICE_id, None): it can return ScheduleNotFound (false negative).

6.Verify the impact:

• ALICE’s reserved_balance_named is unchanged (funds remain reserved/locked).
• unlock_reserves returns HasActiveSchedules (cannot self-unlock).

[cmoxiu]

@andresilva @mikespook @martinfridrich
Hi,

The Scope page explicitly lists pallet-dca as an in-scope asset. The Information page states that GC-developed pallets that are in the mainnet runtime and live are in-scope, and the official documentation confirms the DCA pallet schedules trades and that open orders can be terminated. Therefore, I believe this report is in scope. Could you please specify the exact basis for marking it out of scope?