From ae14e6da6a3b2a729365f9581211d07033719c1a 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.
---
 prdoc/pr_2811.prdoc                           | 13 +++++++
 .../frame/broker/src/dispatchable_impls.rs    |  3 ++
 substrate/frame/broker/src/tests.rs           | 37 +++++++++++++++++++
 3 files changed, 53 insertions(+)
 create mode 100644 prdoc/pr_2811.prdoc

diff --git a/prdoc/pr_2811.prdoc b/prdoc/pr_2811.prdoc
new file mode 100644
index 0000000000..647fb4c8cc
--- /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 b04e15b169..f245101325 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 e1b489bbe6..6aa9ca84fc 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(|| {