Skip to content

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

Merged
dhirajs0 merged 9 commits intomasterfrom
dhiraj/fix-unassign-curator-auth-bypass
Apr 3, 2026
Merged

fix(multi-asset-bounties): enforce authorization in unassign_curator when parent bounty is not Active#11612
dhirajs0 merged 9 commits intomasterfrom
dhiraj/fix-unassign-curator-auth-bypass

Conversation

@dhirajs0
Copy link
Copy Markdown
Contributor

@dhirajs0 dhirajs0 commented Apr 1, 2026

Description

Fix an authorization bypass in pallet-multi-asset-bounties where any signed account could
forcibly unassign an active child bounty's curator when the parent bounty was not in Active state
(e.g., CuratorUnassigned). This also caused the child curator's native balance hold (deposit) to
be permanently leaked — removed from pallet storage but never released or burned on-chain.

Root cause: In unassign_curator, the BountyStatus::Active branch's catch-all Some(sender)
arm used if let Some(parent_curator) = parent_curator { ... } with no else clause. When
parent_curator was None (parent bounty not Active), the block was silently skipped and execution
fell through to the state transition — no BadOrigin error was returned.

Integration

No integration changes required for downstream projects. This is a fix internal to
pallet-multi-asset-bounties with no public API changes. The extrinsic signature and behavior for
authorized callers remain identical.

Review Notes

The fix restructures the BountyStatus::Active arm in unassign_curator with two changes:

1. Authorization before storage mutation

Previously, CuratorDeposit::take() was called unconditionally at the top of the Active arm
(before verifying the caller). Now it is called inside each match maybe_sender arm, only after the
caller is confirmed to be authorized. This prevents the deposit from being removed from storage on
an unauthorized (and reverted) call path.

 BountyStatus::Active { ref curator, .. } => {
-    let maybe_curator_deposit =
-        CuratorDeposit::<T, I>::take(parent_bounty_id, child_bounty_id);
     match maybe_sender {
         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);
             }
         },

2. Explicit rejection when parent_curator is None

The catch-all Some(sender) arm now uses parent_curator.ok_or(BadOrigin)? followed by an
ensure!. When parent_curator is None, the call is immediately
rejected with BadOrigin.

         Some(sender) => {
-            if let Some(parent_curator) = parent_curator {
-                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
+            );
+            if let Some(curator_deposit) =
+                CuratorDeposit::<T, I>::take(parent_bounty_id, child_bounty_id)
+            {
+                T::Consideration::burn(curator_deposit, curator);
             }
         },

Regression test

A comprehensive test (unprivileged_caller_cannot_unassign_active_child_curator_when_parent_not_active)
is added that:

  1. Creates an active child bounty with a separate child curator.
  2. Has the parent curator voluntarily unassign (putting parent into CuratorUnassigned).
  3. Asserts that an unprivileged attacker is rejected with BadOrigin.
  4. Verifies the child bounty remains Active, the curator deposit stays in storage, and the
    balance hold is intact.
  5. Confirms the child curator can still voluntarily unassign themselves and that the deposit is
    properly released.

Checklist

  • My PR includes a detailed description as outlined in the "Description" and its two subsections above.
  • My PR follows the [labeling requirements]
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@dhirajs0 dhirajs0 requested a review from a team as a code owner April 1, 2026 14:55
@dhirajs0 dhirajs0 requested a review from muharem April 1, 2026 15:00
@dhirajs0 dhirajs0 added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Apr 1, 2026
@dhirajs0
Copy link
Copy Markdown
Contributor Author

dhirajs0 commented Apr 1, 2026

/cmd prdoc

@dhirajs0 dhirajs0 requested a review from ggwpez April 1, 2026 16:59
@dhirajs0 dhirajs0 added A4-backport-stable2512 Pull request must be backported to the stable2512 release branch A4-backport-stable2603 Pull request must be backported to the stable2603 release branch labels Apr 2, 2026
@Szegoo Szegoo self-requested a review April 2, 2026 12:16
@dhirajs0 dhirajs0 enabled auto-merge April 2, 2026 17:17
Comment thread substrate/frame/multi-asset-bounties/src/lib.rs
@dhirajs0 dhirajs0 added this pull request to the merge queue Apr 3, 2026
@Szegoo Szegoo removed this pull request from the merge queue due to a manual request Apr 3, 2026
@dhirajs0
Copy link
Copy Markdown
Contributor Author

dhirajs0 commented Apr 3, 2026

/cmd fmt

@dhirajs0 dhirajs0 enabled auto-merge April 3, 2026 17:00
@dhirajs0 dhirajs0 added this pull request to the merge queue Apr 3, 2026
Merged via the queue into master with commit ecada34 Apr 3, 2026
322 of 332 checks passed
@dhirajs0 dhirajs0 deleted the dhiraj/fix-unassign-curator-auth-bypass branch April 3, 2026 18:35
@paritytech-release-backport-bot
Copy link
Copy Markdown

Created backport PR for stable2512:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-11612-to-stable2512
git worktree add --checkout .worktree/backport-11612-to-stable2512 backport-11612-to-stable2512
cd .worktree/backport-11612-to-stable2512
git reset --hard HEAD^
git cherry-pick -x ecada3402a70d906e10c6d33b0f42b6174fea119
git push --force-with-lease

@paritytech-release-backport-bot
Copy link
Copy Markdown

Created backport PR for stable2603:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-11612-to-stable2603
git worktree add --checkout .worktree/backport-11612-to-stable2603 backport-11612-to-stable2603
cd .worktree/backport-11612-to-stable2603
git reset --hard HEAD^
git cherry-pick -x ecada3402a70d906e10c6d33b0f42b6174fea119
git push --force-with-lease

dhirajs0 added a commit that referenced this pull request Apr 8, 2026
…when parent bounty is not Active (#11612)

Fix an authorization bypass in `pallet-multi-asset-bounties` where any
signed account could
forcibly unassign an active child bounty's curator when the parent
bounty was not in `Active` state
(e.g., `CuratorUnassigned`). This also caused the child curator's native
balance hold (deposit) to
be permanently leaked — removed from pallet storage but never released
or burned on-chain.

**Root cause:** In `unassign_curator`, the `BountyStatus::Active`
branch's catch-all `Some(sender)`
arm used `if let Some(parent_curator) = parent_curator { ... }` with no
`else` clause. When
`parent_curator` was `None` (parent bounty not Active), the block was
silently skipped and execution
fell through to the state transition — no `BadOrigin` error was
returned.

No integration changes required for downstream projects. This is a fix
internal to
`pallet-multi-asset-bounties` with no public API changes. The extrinsic
signature and behavior for
authorized callers remain identical.

The fix restructures the `BountyStatus::Active` arm in
`unassign_curator` with two changes:

Previously, `CuratorDeposit::take()` was called unconditionally at the
top of the `Active` arm
(before verifying the caller). Now it is called inside each `match
maybe_sender` arm, only after the
caller is confirmed to be authorized. This prevents the deposit from
being removed from storage on
an unauthorized (and reverted) call path.

```diff
 BountyStatus::Active { ref curator, .. } => {
-    let maybe_curator_deposit =
-        CuratorDeposit::<T, I>::take(parent_bounty_id, child_bounty_id);
     match maybe_sender {
         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);
             }
         },
```

The catch-all `Some(sender)` arm now uses
`parent_curator.ok_or(BadOrigin)?` followed by an
`ensure!`. When `parent_curator` is `None`, the call is immediately
rejected with `BadOrigin`.

```diff
         Some(sender) => {
-            if let Some(parent_curator) = parent_curator {
-                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
+            );
+            if let Some(curator_deposit) =
+                CuratorDeposit::<T, I>::take(parent_bounty_id, child_bounty_id)
+            {
+                T::Consideration::burn(curator_deposit, curator);
             }
         },
```

A comprehensive test
(`unprivileged_caller_cannot_unassign_active_child_curator_when_parent_not_active`)
is added that:

1. Creates an active child bounty with a separate child curator.
2. Has the parent curator voluntarily unassign (putting parent into
`CuratorUnassigned`).
3. Asserts that an unprivileged attacker is rejected with `BadOrigin`.
4. Verifies the child bounty remains `Active`, the curator deposit stays
in storage, and the
   balance hold is intact.
5. Confirms the child curator can still voluntarily unassign themselves
and that the deposit is
   properly released.

* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [x] My PR follows the [labeling requirements]
* [x] I have made corresponding changes to the documentation (if
applicable)
* [x] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
(cherry picked from commit ecada34)
dhirajs0 added a commit that referenced this pull request Apr 8, 2026
…when parent bounty is not Active (#11612)

Fix an authorization bypass in `pallet-multi-asset-bounties` where any
signed account could
forcibly unassign an active child bounty's curator when the parent
bounty was not in `Active` state
(e.g., `CuratorUnassigned`). This also caused the child curator's native
balance hold (deposit) to
be permanently leaked — removed from pallet storage but never released
or burned on-chain.

**Root cause:** In `unassign_curator`, the `BountyStatus::Active`
branch's catch-all `Some(sender)`
arm used `if let Some(parent_curator) = parent_curator { ... }` with no
`else` clause. When
`parent_curator` was `None` (parent bounty not Active), the block was
silently skipped and execution
fell through to the state transition — no `BadOrigin` error was
returned.

No integration changes required for downstream projects. This is a fix
internal to
`pallet-multi-asset-bounties` with no public API changes. The extrinsic
signature and behavior for
authorized callers remain identical.

The fix restructures the `BountyStatus::Active` arm in
`unassign_curator` with two changes:

Previously, `CuratorDeposit::take()` was called unconditionally at the
top of the `Active` arm
(before verifying the caller). Now it is called inside each `match
maybe_sender` arm, only after the
caller is confirmed to be authorized. This prevents the deposit from
being removed from storage on
an unauthorized (and reverted) call path.

```diff
 BountyStatus::Active { ref curator, .. } => {
-    let maybe_curator_deposit =
-        CuratorDeposit::<T, I>::take(parent_bounty_id, child_bounty_id);
     match maybe_sender {
         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);
             }
         },
```

The catch-all `Some(sender)` arm now uses
`parent_curator.ok_or(BadOrigin)?` followed by an
`ensure!`. When `parent_curator` is `None`, the call is immediately
rejected with `BadOrigin`.

```diff
         Some(sender) => {
-            if let Some(parent_curator) = parent_curator {
-                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
+            );
+            if let Some(curator_deposit) =
+                CuratorDeposit::<T, I>::take(parent_bounty_id, child_bounty_id)
+            {
+                T::Consideration::burn(curator_deposit, curator);
             }
         },
```

A comprehensive test
(`unprivileged_caller_cannot_unassign_active_child_curator_when_parent_not_active`)
is added that:

1. Creates an active child bounty with a separate child curator.
2. Has the parent curator voluntarily unassign (putting parent into
`CuratorUnassigned`).
3. Asserts that an unprivileged attacker is rejected with `BadOrigin`.
4. Verifies the child bounty remains `Active`, the curator deposit stays
in storage, and the
   balance hold is intact.
5. Confirms the child curator can still voluntarily unassign themselves
and that the deposit is
   properly released.

* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [x] My PR follows the [labeling requirements]
* [x] I have made corresponding changes to the documentation (if
applicable)
* [x] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
(cherry picked from commit ecada34)
EgorPopelyaev pushed a commit that referenced this pull request Apr 10, 2026
Backport #11612 into `stable2512` from dhirajs0.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

Co-authored-by: Dhiraj Sah <dhiraj@parity.io>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
EgorPopelyaev pushed a commit that referenced this pull request Apr 10, 2026
Backport #11612 into `stable2603` from dhirajs0.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

Co-authored-by: Dhiraj Sah <dhiraj@parity.io>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A4-backport-stable2512 Pull request must be backported to the stable2512 release branch A4-backport-stable2603 Pull request must be backported to the stable2603 release branch T1-FRAME This PR/Issue is related to core FRAME, the framework.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants