fix(multi-asset-bounties): enforce authorization in unassign_curator when parent bounty is not Active

prdoc/pr_11612.prdoc

@@ -0,0 +1,23 @@
+title: "fix(multi-asset-bounties): enforce authorization in unassign_curator when parent bounty is not Active"
+doc:
+- audience: Runtime Dev
+  description: |-
+    Fix an authorization bypass in `unassign_curator` where any signed account could forcibly
+    unassign an active child bounty's curator when the parent bounty was not in `Active` state.
+    The child curator's native balance hold (deposit) was also permanently leaked in this path.
+
+    The `BountyStatus::Active` catch-all arm now uses `parent_curator.ok_or(BadOrigin)?` to
+    explicitly reject callers when no parent curator is available, matching the defensive pattern
+    already used in the `Funded` arm. `CuratorDeposit::take()` is also moved after authorization
+    to prevent storage mutation on unauthorized calls.
+
+    A regression test is added covering the full attack scenario.
+- audience: Runtime User
+  description: |-
+    Previously, any account could unassign a child bounty curator without authorization when the
+    parent bounty's curator had been unassigned. This is now correctly rejected. If you were
+    affected by a permanently locked curator deposit from this bug, a migration or manual
+    intervention may be needed to release the held balance.
+crates:
+- name: pallet-multi-asset-bounties
+  bump: patch

substrate/frame/multi-asset-bounties/src/lib.rs

@@ -911,36 +911,39 @@ pub mod pallet {
 					);
 				},
 				BountyStatus::Active { ref curator, .. } => {
-					let maybe_curator_deposit =
-						CuratorDeposit::<T, I>::take(parent_bounty_id, child_bounty_id);
 					// The child-/bounty is active.
 					match maybe_sender {
 						// If the `RejectOrigin` is calling this function, burn the curator deposit.
 						None => {
-							if let Some(curator_deposit) = maybe_curator_deposit {
+							if let Some(curator_deposit) =
+								CuratorDeposit::<T, I>::take(parent_bounty_id, child_bounty_id)
+							{
 								T::Consideration::burn(curator_deposit, curator);
 							}
 							// Continue to change bounty status below...
 						},
 						Some(sender) if sender == *curator => {
-							if let Some(curator_deposit) = maybe_curator_deposit {
+							if let Some(curator_deposit) =
+								CuratorDeposit::<T, I>::take(parent_bounty_id, child_bounty_id)
+							{
 								// This is the curator, willingly giving up their role. Free their
 								// deposit.
 								T::Consideration::drop(curator_deposit, curator)?;
 							}
 							// Continue to change bounty status below...
 						},
 						Some(sender) => {
-							if let Some(parent_curator) = parent_curator {
-								// If the parent curator is unassigning a child curator, that is not
-								// itself, burn the child curator deposit.
-								if sender == parent_curator && *curator != parent_curator {
-									if let Some(curator_deposit) = maybe_curator_deposit {
-										T::Consideration::burn(curator_deposit, curator);
-									}
-								} else {
-									return Err(BadOrigin.into());
-								}
+							let parent_curator = parent_curator.ok_or(BadOrigin)?;
+							ensure!(
+								sender == parent_curator && *curator != parent_curator,
+								BadOrigin
+							);
+							// Parent curator is unassigning the child curator. Burn the curator
+							// deposit.
+							if let Some(curator_deposit) =
+								CuratorDeposit::<T, I>::take(parent_bounty_id, child_bounty_id)
+							{
+								T::Consideration::burn(curator_deposit, curator);
 							}
 						},
 					}

substrate/frame/multi-asset-bounties/src/tests.rs

@@ -2518,6 +2518,94 @@ fn fund_and_award_child_bounty_without_curator_works() {
 	})
 }
 
+#[test]
+fn unprivileged_caller_cannot_unassign_active_child_curator_when_parent_not_active() {
+	ExtBuilder::default().build_and_execute(|| {
+		let s = create_active_child_bounty();
+		let attacker: u128 = 99;
+
+		// Verify preconditions: child bounty is Active with curator deposit held.
+		assert_eq!(Balances::reserved_balance(&s.child_curator), s.child_curator_deposit);
+		assert!(pallet_bounties::CuratorDeposit::<Test>::get(
+			s.parent_bounty_id,
+			Some(s.child_bounty_id)
+		)
+		.is_some());
+
+		// Step 1: Parent curator voluntarily unassigns from parent bounty.
+		assert_ok!(Bounties::unassign_curator(
+			RuntimeOrigin::signed(s.curator),
+			s.parent_bounty_id,
+			None,
+		));
+
+		// Parent is now CuratorUnassigned; child is still Active.
+		let (_, _, _, parent_status, _) =
+			Bounties::get_bounty_details(s.parent_bounty_id, None).expect("parent bounty exists");
+		assert_eq!(parent_status, BountyStatus::CuratorUnassigned);
+
+		let (_, _, _, child_status, parent_curator) =
+			Bounties::get_bounty_details(s.parent_bounty_id, Some(s.child_bounty_id))
+				.expect("child bounty exists");
+		assert!(matches!(child_status, BountyStatus::Active { .. }));
+		assert!(
+			parent_curator.is_none(),
+			"parent_curator should be None since parent is not Active"
+		);
+
+		// Step 2: An unprivileged attacker tries to unassign the active child bounty curator.
+		// This must be rejected with BadOrigin.
+		assert_noop!(
+			Bounties::unassign_curator(
+				RuntimeOrigin::signed(attacker),
+				s.parent_bounty_id,
+				Some(s.child_bounty_id),
+			),
+			BadOrigin
+		);
+
+		// Child bounty remains Active the unprivileged caller had no effect.
+		let (_, _, _, child_status, _) =
+			Bounties::get_bounty_details(s.parent_bounty_id, Some(s.child_bounty_id))
+				.expect("child bounty exists");
+		assert!(matches!(child_status, BountyStatus::Active { .. }));
+
+		// Curator deposit is still in storage and the hold is intact.
+		assert!(
+			pallet_bounties::CuratorDeposit::<Test>::get(
+				s.parent_bounty_id,
+				Some(s.child_bounty_id)
+			)
+			.is_some(),
+			"curator deposit must remain in storage"
+		);
+		assert_eq!(
+			Balances::reserved_balance(&s.child_curator),
+			s.child_curator_deposit,
+			"curator deposit hold must remain intact"
+		);
+
+		// Step 3: Verify that the child curator can still voluntarily unassign themselves.
+		assert_ok!(Bounties::unassign_curator(
+			RuntimeOrigin::signed(s.child_curator),
+			s.parent_bounty_id,
+			Some(s.child_bounty_id),
+		));
+
+		let (_, _, _, child_status, _) =
+			Bounties::get_bounty_details(s.parent_bounty_id, Some(s.child_bounty_id))
+				.expect("child bounty exists");
+		assert_eq!(child_status, BountyStatus::CuratorUnassigned);
+
+		// Curator's deposit was properly released.
+		assert_eq!(
+			Balances::reserved_balance(&s.child_curator),
+			0,
+			"curator deposit hold must be released after voluntary unassign"
+		);
+	});
+}
+
 #[test]
 fn integrity_test() {
 	ExtBuilder::default().build_and_execute(|| {