standards: harden multisig controller invariants

Author: HDauven

docs/dusk-contract-standards-multisig-controller-review.md

@@ -32,7 +32,9 @@ Moonlight/Phoenix/contract owners approve controller operations.
   bytes, and salt.
 - A ready operation must be removed from pending proposals and tombstoned before
   the wrapper attempts target execution.
-- Expired proposals are pruned and cannot become ready.
+- Expired proposals cannot become ready. Direct confirmation returns the
+  explicit expired error without mutating the pending operation; later
+  proposal/cancel lifecycle calls prune expired pending operations.
 - Authority changes that remove owners or change threshold clear stale pending
   operations.
 
@@ -60,7 +62,8 @@ Phoenix owners and threshold `2`, then deploy `proxy_counter` with
 The native property test exercises the raw controller state machine across
 random owners, ids, targets, TTLs, tombstone windows, and timings. It checks
 atomicity for non-owner proposals, non-owner confirmations, duplicate
-confirmations, one-of-three pending behavior, expiry pruning, ready-state
+confirmations, same-id/different-target rejection, one-of-three pending
+behavior, expired confirmation rejection without mutation, ready-state
 tombstoning, tombstone replay rejection, and successful re-use only after the
 tombstone has expired.
 
@@ -71,11 +74,14 @@ Phoenix. Phoenix owners approve by Schnorr-signed `AuthorizedAction`.
 Moonlight and contract owners may be accepted through observed runtime context
 when the host exposes it.
 
-The Forge wrapper verifies signatures before consuming nonce state, but only
-consumes after the controller accepts the proposal/confirmation/maintenance
-operation. This avoids burning a valid signature on duplicate confirmations,
-wrong threshold membership, invalid targets, expired proposals, or invalid
-maintenance payloads.
+The Forge wrapper verifies signatures before consuming nonce state. Threshold
+maintenance calls receive a `MultisigQuorum` witness from that verification
+path rather than a caller-supplied list of principals, so composing contracts
+cannot accidentally satisfy owner management by naming owners without proving
+authorization. The wrapper consumes nonce state only after the controller
+accepts the proposal/confirmation/maintenance operation. This avoids burning a
+valid signature on duplicate confirmations, wrong threshold membership,
+invalid targets, expired proposals, or invalid maintenance payloads.
 
 Execution is automatic at threshold. There is no separate `execute` step. This
 is simpler and avoids a ready-operation queue, but systems that require a final
@@ -87,10 +93,10 @@ A target execution failure emits `multisig/operation_executed` with
 same logical call requires a new salt. This avoids accidental replay, but
 clients must surface failed execution clearly.
 
-`propose` can count as a confirmation for an existing operation with the same
-operation id. This is acceptable because the signed payload is the exact
-operation id, but client UX should prefer the explicit `confirm` method after a
-proposal already exists.
+`propose` can count as a confirmation for an existing operation only when the
+supplied target matches the stored target for the operation id. Client UX
+should still prefer the explicit `confirm` method after a proposal already
+exists.
 
 ## Residual Risks
 

docs/dusk-contract-standards-security.md

@@ -87,6 +87,11 @@ Multisig approvals should be bound to the exact operation through
 minting, pausing, and role administration should use separate domains or action
 ids so approvals cannot be replayed across policy surfaces.
 
+Quorum-gated mutators take a `MultisigQuorum` witness rather than raw principal
+arrays. The witness is produced by the multisig verification path and has a
+private signer list, so callers can inspect who signed but cannot forge a
+quorum by manually passing known owner principals.
+
 Use the standalone `MultisigController` when a ported contract wants a single
 owner/admin principal like an Ethereum contract owned by a Safe. The target
 contract should assign ownership or the relevant role to the controller's

docs/dusk-contract-standards.md

@@ -220,9 +220,10 @@ The native test suite covers positive and negative paths for:
 - observed-or-signed owner, role, and upgrade-admin authorization;
 - threshold multisig authorization, duplicate signer rejection, observed
   Moonlight/contract approval, Phoenix signed approval, and threshold-gated
-  owner/threshold maintenance;
+  owner/threshold maintenance through verified quorum witnesses;
 - standalone multisig controller proposal, confirmation, duplicate-confirmation
-  rejection, non-owner rejection, expiry cleanup, cancellation, tombstoning,
+  rejection, non-owner rejection, same-id/different-target rejection, explicit
+  expired-confirmation rejection, expiry cleanup, cancellation, tombstoning,
   authority updates, 2-of-3 property coverage, and proxy-as-owner VM execution;
 - timelock scheduling, cancellation, execution, and invalid states;
 - role-gated timelock controller flows, including self-governed delay updates;

standards/dusk-contract-standards/src/governance/multisig.rs

@@ -137,7 +137,7 @@ impl ThresholdMultisig {
         approvals: &[SignedAuthorization],
         envelope: ActionEnvelope,
         now: u64,
-    ) -> Vec<Principal> {
+    ) -> MultisigQuorum {
         let (quorum, verified) = self.verify_action_with_witnesses(
             authorizations,
             context,
@@ -148,7 +148,7 @@ impl ThresholdMultisig {
         for approval in verified {
             authorizations.consume_verified(approval);
         }
-        quorum.signers
+        quorum
     }
 
     /// Verifies that the observed caller plus supplied signed approvals satisfy

standards/dusk-contract-standards/src/governance/multisig_controller.rs

@@ -353,7 +353,7 @@ impl MultisigController {
         approvals: &[SignedAuthorization],
         envelope: ActionEnvelope,
         now: u64,
-    ) -> Vec<Principal> {
+    ) -> MultisigQuorum {
         self.policy.authorize_action(
             authorizations,
             context,
@@ -457,7 +457,7 @@ impl MultisigController {
     ) -> MultisigControllerOutcome {
         self.assert_initialized();
         self.assert_owner(authorizer);
-        self.prune(now);
+        self.prune_tombstones(now);
         self.assert_not_tombstoned(id);
         self.confirm_pending(id, authorizer, now)
     }
@@ -557,6 +557,9 @@ impl MultisigController {
                 .proposals
                 .get_mut(&id)
                 .unwrap_or_else(|| panic!("{}", error::OPERATION_UNKNOWN));
+            if now > operation.deadline {
+                panic!("{}", error::EXPIRED);
+            }
             if operation.confirmed_by(authorizer) {
                 panic!("{}", error::UNAUTHORIZED);
             }
@@ -637,6 +640,10 @@ impl MultisigController {
     fn prune(&mut self, now: u64) {
         self.proposals
             .retain(|_, operation| now <= operation.deadline);
+        self.prune_tombstones(now);
+    }
+
+    fn prune_tombstones(&mut self, now: u64) {
         self.tombstones.retain(|_, expiry| now <= *expiry);
     }
 

standards/dusk-contract-standards/tests/access_multisig.rs

@@ -713,16 +713,16 @@ fn threshold_multisig_consumes_approvals_only_after_quorum_success() {
     });
     assert_eq!(authorizations.nonce(outsider, DOMAIN), 0);
 
-    let signers = multisig.authorize_action(
+    let quorum = multisig.authorize_action(
         &mut authorizations,
         CallContext::from_principal(owner_a),
         &[signed_b.clone()],
         envelope(),
         100,
     );
-    assert_eq!(signers.len(), 2);
-    assert!(signers.contains(&owner_a));
-    assert!(signers.contains(&owner_b));
+    assert_eq!(quorum.signers().len(), 2);
+    assert!(quorum.signers().contains(&owner_a));
+    assert!(quorum.signers().contains(&owner_b));
     assert_eq!(authorizations.nonce(owner_b, DOMAIN), 1);
     assert_panics(|| {
         multisig.authorize_action(
@@ -809,7 +809,7 @@ fn multisig_controller_lifecycle_replay_and_authority_changes_are_pinned() {
     let expired_id = [36u8; 32];
     controller.propose(expired_id, target(36), owner_a, 3);
     assert_panics(|| controller.confirm(expired_id, owner_b, 14));
-    assert!(controller.proposal(expired_id).is_none());
+    assert!(controller.proposal(expired_id).is_some());
     let replacement_id = [37u8; 32];
     controller.propose(replacement_id, target(37), owner_a, 14);
     assert!(controller.proposal(expired_id).is_none());