diff --git a/prdoc/pr_2811.prdoc b/prdoc/pr_2811.prdoc new file mode 100644 index 000000000000..647fb4c8ccd4 --- /dev/null +++ b/prdoc/pr_2811.prdoc @@ -0,0 +1,13 @@ +title: "Interlacing removes the region on which it is performed." + +doc: + - audience: Runtime User + description: | + The current implementation of the broker pallet does not remove + the region on which the interlacing is performed. This can create + a vulnerability, as the original region owner is still allowed to + assign a task to the region even after transferring an interlaced + part of it. + +crates: + - name: "pallet-broker" diff --git a/substrate/frame/broker/src/dispatchable_impls.rs b/substrate/frame/broker/src/dispatchable_impls.rs index b04e15b169bc..f2451013251f 100644 --- a/substrate/frame/broker/src/dispatchable_impls.rs +++ b/substrate/frame/broker/src/dispatchable_impls.rs @@ -234,6 +234,9 @@ impl Pallet { ensure!(!pivot.is_void(), Error::::VoidPivot); ensure!(pivot != region_id.mask, Error::::CompletePivot); + // The old region should be removed. + Regions::::remove(®ion_id); + let one = RegionId { mask: pivot, ..region_id }; Regions::::insert(&one, ®ion); let other = RegionId { mask: region_id.mask ^ pivot, ..region_id }; diff --git a/substrate/frame/broker/src/tests.rs b/substrate/frame/broker/src/tests.rs index e1b489bbe6e6..6aa9ca84fc41 100644 --- a/substrate/frame/broker/src/tests.rs +++ b/substrate/frame/broker/src/tests.rs @@ -602,6 +602,43 @@ fn interlace_works() { }); } +#[test] +fn cant_assign_unowned_region() { + TestExt::new().endow(1, 1000).execute_with(|| { + assert_ok!(Broker::do_start_sales(100, 1)); + advance_to(2); + let region = Broker::do_purchase(1, u64::max_value()).unwrap(); + let (region1, region2) = + Broker::do_interlace(region, Some(1), CoreMask::from_chunk(0, 30)).unwrap(); + + // Transfer the interlaced region to account 2. + assert_ok!(Broker::do_transfer(region2, Some(1), 2)); + + // The initial owner should not be able to assign the non-interlaced region, since they have + // just transferred an interlaced part of it to account 2. + assert_noop!(Broker::do_assign(region, Some(1), 1001, Final), Error::::UnknownRegion); + + // Account 1 can assign only the interlaced region that they did not transfer. + assert_ok!(Broker::do_assign(region1, Some(1), 1001, Final)); + // Account 2 can assign the region they received. + assert_ok!(Broker::do_assign(region2, Some(2), 1002, Final)); + + advance_to(10); + assert_eq!( + CoretimeTrace::get(), + vec![( + 6, + AssignCore { + core: 0, + begin: 8, + assignment: vec![(Task(1001), 21600), (Task(1002), 36000)], + end_hint: None + } + ),] + ); + }); +} + #[test] fn interlace_then_partition_works() { TestExt::new().endow(1, 1000).execute_with(|| {