From 3307d9527f3044abaa5e493f57cf01448f4c7f0b Mon Sep 17 00:00:00 2001 From: dhirajs0 Date: Wed, 1 Apr 2026 18:40:34 +0530 Subject: [PATCH 1/6] fix: reject unauthorized child bounty curator unassignment when parent is not Active --- .../frame/multi-asset-bounties/src/lib.rs | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/substrate/frame/multi-asset-bounties/src/lib.rs b/substrate/frame/multi-asset-bounties/src/lib.rs index e1dadcfbd5628..d5310565a427f 100644 --- a/substrate/frame/multi-asset-bounties/src/lib.rs +++ b/substrate/frame/multi-asset-bounties/src/lib.rs @@ -911,19 +911,21 @@ 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::::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)?; @@ -931,16 +933,17 @@ pub mod pallet { // 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); } }, } From d86fbb78370a1dbab11d4298a7a625b843051cdd Mon Sep 17 00:00:00 2001 From: dhirajs0 Date: Wed, 1 Apr 2026 18:42:26 +0530 Subject: [PATCH 2/6] test: add regression test for child bounty curator unassignment auth bypass --- .../frame/multi-asset-bounties/src/tests.rs | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/substrate/frame/multi-asset-bounties/src/tests.rs b/substrate/frame/multi-asset-bounties/src/tests.rs index 265b4037a6e69..c82e344458ccc 100644 --- a/substrate/frame/multi-asset-bounties/src/tests.rs +++ b/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::::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 integrity_test() { ExtBuilder::default().build_and_execute(|| { From 73a59433aa2d59790a395fc90397a78a79cfe979 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 1 Apr 2026 15:05:03 +0000 Subject: [PATCH 3/6] Update from github-actions[bot] running command 'prdoc' --- prdoc/pr_11612.prdoc | 59 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 prdoc/pr_11612.prdoc diff --git a/prdoc/pr_11612.prdoc b/prdoc/pr_11612.prdoc new file mode 100644 index 0000000000000..1f290cda6fe4c --- /dev/null +++ b/prdoc/pr_11612.prdoc @@ -0,0 +1,59 @@ +title: 'fix(multi-asset-bounties): enforce authorization in unassign_curator when + parent bounty is not Active' +doc: +- audience: Todo + description: "# Description\n\nFix an authorization bypass in `pallet-multi-asset-bounties`\ + \ where any signed account could\nforcibly unassign an active child bounty's curator\ + \ when the parent bounty was not in `Active` state\n(e.g., `CuratorUnassigned`).\ + \ This also caused the child curator's native balance hold (deposit) to\nbe permanently\ + \ leaked \u2014 removed from pallet storage but never released or burned on-chain.\n\ + \n**Root cause:** In `unassign_curator`, the `BountyStatus::Active` branch's catch-all\ + \ `Some(sender)`\narm used `if let Some(parent_curator) = parent_curator { ...\ + \ }` with no `else` clause. When\n`parent_curator` was `None` (parent bounty not\ + \ Active), the block was silently skipped and execution\nfell through to the state\ + \ transition \u2014 no `BadOrigin` error was returned.\n\n## Integration\n\nNo\ + \ integration changes required for downstream projects. This is a bug fix internal\ + \ to\n`pallet-multi-asset-bounties` with no public API changes. The extrinsic\ + \ signature and behavior for\nauthorized callers remain identical.\n\n## Review\ + \ Notes\n\nThe fix restructures the `BountyStatus::Active` arm in `unassign_curator`\ + \ with two changes:\n\n### 1. Authorization before storage mutation\n\nPreviously,\ + \ `CuratorDeposit::take()` was called unconditionally at the top of the `Active`\ + \ arm\n(before verifying the caller). Now it is called inside each `match maybe_sender`\ + \ arm, only after the\ncaller is confirmed to be authorized. This prevents the\ + \ deposit from being removed from storage on\nan unauthorized (and reverted) call\ + \ path.\n\n```diff\n BountyStatus::Active { ref curator, .. } => {\n- let maybe_curator_deposit\ + \ =\n- CuratorDeposit::::take(parent_bounty_id, child_bounty_id);\n\ + \ match maybe_sender {\n None => {\n- if let Some(curator_deposit)\ + \ = maybe_curator_deposit {\n+ if let Some(curator_deposit) =\n+ \ + \ CuratorDeposit::::take(parent_bounty_id, child_bounty_id)\n\ + + {\n T::Consideration::burn(curator_deposit, curator);\n\ + \ }\n },\n```\n\n### 2. Explicit rejection when `parent_curator`\ + \ is `None`\n\nThe catch-all `Some(sender)` arm now uses `parent_curator.ok_or(BadOrigin)?`\ + \ followed by an\n`ensure!`. When `parent_curator` is `None`, the call is immediately\n\ + rejected with `BadOrigin`.\n\n```diff\n Some(sender) => {\n- \ + \ if let Some(parent_curator) = parent_curator {\n- if sender\ + \ == parent_curator && *curator != parent_curator {\n- if let\ + \ Some(curator_deposit) = maybe_curator_deposit {\n- T::Consideration::burn(curator_deposit,\ + \ curator);\n- }\n- } else {\n- \ + \ return Err(BadOrigin.into());\n- }\n+ let parent_curator\ + \ = parent_curator.ok_or(BadOrigin)?;\n+ ensure!(\n+ \ + \ sender == parent_curator && *curator != parent_curator,\n+ BadOrigin\n\ + + );\n+ if let Some(curator_deposit) =\n+ \ + \ CuratorDeposit::::take(parent_bounty_id, child_bounty_id)\n+ \ + \ {\n+ T::Consideration::burn(curator_deposit, curator);\n \ + \ }\n },\n```\n\n### Regression test\n\nA comprehensive test\ + \ (`unprivileged_caller_cannot_unassign_active_child_curator_when_parent_not_active`)\n\ + is added that:\n\n1. Creates an active child bounty with a separate child curator.\n\ + 2. Has the parent curator voluntarily unassign (putting parent into `CuratorUnassigned`).\n\ + 3. Asserts that an unprivileged attacker is rejected with `BadOrigin`.\n4. Verifies\ + \ the child bounty remains `Active`, the curator deposit stays in storage, and\ + \ the\n balance hold is intact.\n5. Confirms the child curator can still voluntarily\ + \ unassign themselves and that the deposit is\n properly released.\n\n# Checklist\n\ + \n* [x] My PR includes a detailed description as outlined in the \"Description\"\ + \ and its two subsections above.\n* [x] My PR follows the [labeling requirements]\n\ + * [x] I have made corresponding changes to the documentation (if applicable)\n\ + * [x] I have added tests that prove my fix is effective or that my feature works\ + \ (if applicable)" +crates: +- name: pallet-multi-asset-bounties + bump: major From cfa1f26b3dddf0ec6f3645fdf76cb78cf7f05728 Mon Sep 17 00:00:00 2001 From: dhirajs0 Date: Wed, 1 Apr 2026 20:41:50 +0530 Subject: [PATCH 4/6] prdoc updated --- prdoc/pr_11612.prdoc | 76 ++++++++++++-------------------------------- 1 file changed, 20 insertions(+), 56 deletions(-) diff --git a/prdoc/pr_11612.prdoc b/prdoc/pr_11612.prdoc index 1f290cda6fe4c..6fb14e231d16d 100644 --- a/prdoc/pr_11612.prdoc +++ b/prdoc/pr_11612.prdoc @@ -1,59 +1,23 @@ -title: 'fix(multi-asset-bounties): enforce authorization in unassign_curator when - parent bounty is not Active' +title: "fix(multi-asset-bounties): enforce authorization in unassign_curator when parent bounty is not Active" doc: -- audience: Todo - description: "# Description\n\nFix an authorization bypass in `pallet-multi-asset-bounties`\ - \ where any signed account could\nforcibly unassign an active child bounty's curator\ - \ when the parent bounty was not in `Active` state\n(e.g., `CuratorUnassigned`).\ - \ This also caused the child curator's native balance hold (deposit) to\nbe permanently\ - \ leaked \u2014 removed from pallet storage but never released or burned on-chain.\n\ - \n**Root cause:** In `unassign_curator`, the `BountyStatus::Active` branch's catch-all\ - \ `Some(sender)`\narm used `if let Some(parent_curator) = parent_curator { ...\ - \ }` with no `else` clause. When\n`parent_curator` was `None` (parent bounty not\ - \ Active), the block was silently skipped and execution\nfell through to the state\ - \ transition \u2014 no `BadOrigin` error was returned.\n\n## Integration\n\nNo\ - \ integration changes required for downstream projects. This is a bug fix internal\ - \ to\n`pallet-multi-asset-bounties` with no public API changes. The extrinsic\ - \ signature and behavior for\nauthorized callers remain identical.\n\n## Review\ - \ Notes\n\nThe fix restructures the `BountyStatus::Active` arm in `unassign_curator`\ - \ with two changes:\n\n### 1. Authorization before storage mutation\n\nPreviously,\ - \ `CuratorDeposit::take()` was called unconditionally at the top of the `Active`\ - \ arm\n(before verifying the caller). Now it is called inside each `match maybe_sender`\ - \ arm, only after the\ncaller is confirmed to be authorized. This prevents the\ - \ deposit from being removed from storage on\nan unauthorized (and reverted) call\ - \ path.\n\n```diff\n BountyStatus::Active { ref curator, .. } => {\n- let maybe_curator_deposit\ - \ =\n- CuratorDeposit::::take(parent_bounty_id, child_bounty_id);\n\ - \ match maybe_sender {\n None => {\n- if let Some(curator_deposit)\ - \ = maybe_curator_deposit {\n+ if let Some(curator_deposit) =\n+ \ - \ CuratorDeposit::::take(parent_bounty_id, child_bounty_id)\n\ - + {\n T::Consideration::burn(curator_deposit, curator);\n\ - \ }\n },\n```\n\n### 2. Explicit rejection when `parent_curator`\ - \ is `None`\n\nThe catch-all `Some(sender)` arm now uses `parent_curator.ok_or(BadOrigin)?`\ - \ followed by an\n`ensure!`. When `parent_curator` is `None`, the call is immediately\n\ - rejected with `BadOrigin`.\n\n```diff\n Some(sender) => {\n- \ - \ if let Some(parent_curator) = parent_curator {\n- if sender\ - \ == parent_curator && *curator != parent_curator {\n- if let\ - \ Some(curator_deposit) = maybe_curator_deposit {\n- T::Consideration::burn(curator_deposit,\ - \ curator);\n- }\n- } else {\n- \ - \ return Err(BadOrigin.into());\n- }\n+ let parent_curator\ - \ = parent_curator.ok_or(BadOrigin)?;\n+ ensure!(\n+ \ - \ sender == parent_curator && *curator != parent_curator,\n+ BadOrigin\n\ - + );\n+ if let Some(curator_deposit) =\n+ \ - \ CuratorDeposit::::take(parent_bounty_id, child_bounty_id)\n+ \ - \ {\n+ T::Consideration::burn(curator_deposit, curator);\n \ - \ }\n },\n```\n\n### Regression test\n\nA comprehensive test\ - \ (`unprivileged_caller_cannot_unassign_active_child_curator_when_parent_not_active`)\n\ - is added that:\n\n1. Creates an active child bounty with a separate child curator.\n\ - 2. Has the parent curator voluntarily unassign (putting parent into `CuratorUnassigned`).\n\ - 3. Asserts that an unprivileged attacker is rejected with `BadOrigin`.\n4. Verifies\ - \ the child bounty remains `Active`, the curator deposit stays in storage, and\ - \ the\n balance hold is intact.\n5. Confirms the child curator can still voluntarily\ - \ unassign themselves and that the deposit is\n properly released.\n\n# Checklist\n\ - \n* [x] My PR includes a detailed description as outlined in the \"Description\"\ - \ and its two subsections above.\n* [x] My PR follows the [labeling requirements]\n\ - * [x] I have made corresponding changes to the documentation (if applicable)\n\ - * [x] I have added tests that prove my fix is effective or that my feature works\ - \ (if applicable)" +- 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: major + bump: patch From f094078a30561a7276ddaa6f7df052a569c6384d Mon Sep 17 00:00:00 2001 From: dhirajs0 Date: Fri, 3 Apr 2026 16:04:35 +0530 Subject: [PATCH 5/6] refactor: use get+remove instead of take before fallible Consideration::drop --- .../frame/multi-asset-bounties/src/lib.rs | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/substrate/frame/multi-asset-bounties/src/lib.rs b/substrate/frame/multi-asset-bounties/src/lib.rs index d5310565a427f..1eab1c0f5add7 100644 --- a/substrate/frame/multi-asset-bounties/src/lib.rs +++ b/substrate/frame/multi-asset-bounties/src/lib.rs @@ -922,16 +922,17 @@ pub mod pallet { } // Continue to change bounty status below... }, - Some(sender) if sender == *curator => { - if let Some(curator_deposit) = - CuratorDeposit::::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 sender == *curator => { + 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) => { let parent_curator = parent_curator.ok_or(BadOrigin)?; ensure!( From 2e0145f79a3d81f4bbae9f260e80e79331b1f36c Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 3 Apr 2026 10:53:17 +0000 Subject: [PATCH 6/6] Update from github-actions[bot] running command 'fmt' --- .../frame/multi-asset-bounties/src/lib.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/substrate/frame/multi-asset-bounties/src/lib.rs b/substrate/frame/multi-asset-bounties/src/lib.rs index 1eab1c0f5add7..e6a24f73df433 100644 --- a/substrate/frame/multi-asset-bounties/src/lib.rs +++ b/substrate/frame/multi-asset-bounties/src/lib.rs @@ -922,17 +922,17 @@ pub mod pallet { } // Continue to change bounty status below... }, - Some(sender) if sender == *curator => { - 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 sender == *curator => { + 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) => { let parent_curator = parent_curator.ok_or(BadOrigin)?; ensure!(