From 249f9938c3414b4bd39bac0ad8c6fd0f4edfe2dd Mon Sep 17 00:00:00 2001
From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com>
Date: Fri, 29 Dec 2023 08:04:53 +0100
Subject: [PATCH] Broker pallet: fix interlacing (#2811)

With the current code, when a user interlaces their region, the end
result will be three regions in the state:
- the non-interlaced region
- first part of the interlaced region
- second part of the interlaced region

The existing implementation retains the non-interlaced region in the
state, leading to a problematic scenario:

1. User 1 acquires a region from the market.
2. User 1 then interlaces this region.
3. Subsequently, User 1 transfers one part of the interlaced regions to
User 2.
Despite this transfer, User 1 retains the ability to assign the entire
original non-interlaced region, which is inconsistent with the fact that
they no longer own one of the interlaced parts.

This PR resolves the issue by removing the original region, ensuring
that only the two new interlaced regions remain in the state.
---
 .../frame/broker/src/dispatchable_impls.rs    |  3 ++
 substrate/frame/broker/src/tests.rs           | 37 +++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/substrate/frame/broker/src/dispatchable_impls.rs b/substrate/frame/broker/src/dispatchable_impls.rs
index b04e15b169bc5..f2451013251fb 100644
--- a/substrate/frame/broker/src/dispatchable_impls.rs
+++ b/substrate/frame/broker/src/dispatchable_impls.rs
@@ -234,6 +234,9 @@ impl<T: Config> Pallet<T> {
 		ensure!(!pivot.is_void(), Error::<T>::VoidPivot);
 		ensure!(pivot != region_id.mask, Error::<T>::CompletePivot);
 
+		// The old region should be removed.
+		Regions::<T>::remove(&region_id);
+
 		let one = RegionId { mask: pivot, ..region_id };
 		Regions::<T>::insert(&one, &region);
 		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 e1b489bbe6e67..6aa9ca84fc419 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::<Test>::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(|| {