diff --git a/prdoc/pr_11612.prdoc b/prdoc/pr_11612.prdoc new file mode 100644 index 0000000000000..6fb14e231d16d --- /dev/null +++ b/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 diff --git a/substrate/frame/multi-asset-bounties/src/lib.rs b/substrate/frame/multi-asset-bounties/src/lib.rs index 495eadff05d74..b5e7133d2175d 100644 --- a/substrate/frame/multi-asset-bounties/src/lib.rs +++ b/substrate/frame/multi-asset-bounties/src/lib.rs @@ -916,36 +916,40 @@ pub mod pallet { ); }, BountyStatus::Active { ref curator, .. } => { - let maybe_curator_deposit = - CuratorDeposit::::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::::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::::get(parent_bounty_id, child_bounty_id) + { // This is the curator, willingly giving up their role. Free their // deposit. T::Consideration::drop(curator_deposit, curator)?; + CuratorDeposit::::remove(parent_bounty_id, child_bounty_id); } // 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::::take(parent_bounty_id, child_bounty_id) + { + T::Consideration::burn(curator_deposit, curator); } }, } diff --git a/substrate/frame/multi-asset-bounties/src/tests.rs b/substrate/frame/multi-asset-bounties/src/tests.rs index 40a733a350440..e099486406e47 100644 --- a/substrate/frame/multi-asset-bounties/src/tests.rs +++ b/substrate/frame/multi-asset-bounties/src/tests.rs @@ -2527,6 +2527,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::::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::::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 multi_asset_bounty_accounts_differ_from_legacy_bounty_accounts() { ExtBuilder::default().build_and_execute(|| {