diff --git a/contracts/governance/src/errors.rs b/contracts/governance/src/errors.rs index bc4f13fad..bf69820b4 100644 --- a/contracts/governance/src/errors.rs +++ b/contracts/governance/src/errors.rs @@ -16,6 +16,8 @@ pub enum Error { MaxProposals, NotASigner, ProposalExpired, + /// Signer roster changes are blocked while proposals are actively voting. + SignerChangesLocked, } impl core::fmt::Display for Error { @@ -34,6 +36,9 @@ impl core::fmt::Display for Error { Error::MaxProposals => write!(f, "Maximum active proposals reached"), Error::NotASigner => write!(f, "Caller is not a signer"), Error::ProposalExpired => write!(f, "Proposal has expired"), + Error::SignerChangesLocked => { + write!(f, "Signer roster is locked while proposals are actively voting") + } } } } @@ -54,6 +59,7 @@ impl ContractError for Error { Error::MaxProposals => governance_codes::GOVERNANCE_MAX_PROPOSALS, Error::NotASigner => governance_codes::GOVERNANCE_NOT_A_SIGNER, Error::ProposalExpired => governance_codes::GOVERNANCE_PROPOSAL_EXPIRED, + Error::SignerChangesLocked => governance_codes::GOVERNANCE_SIGNER_CHANGES_LOCKED, } } @@ -72,6 +78,9 @@ impl ContractError for Error { Error::MaxProposals => "Cannot create proposal: active limit reached", Error::NotASigner => "Only signers can perform this action", Error::ProposalExpired => "The proposal voting period has expired", + Error::SignerChangesLocked => { + "Signer roster is locked while proposals are actively voting" + } } } diff --git a/contracts/governance/src/lib.rs b/contracts/governance/src/lib.rs index 4308065f9..6e4ab1da8 100644 --- a/contracts/governance/src/lib.rs +++ b/contracts/governance/src/lib.rs @@ -761,10 +761,25 @@ pub mod governance { } /// Adds a new signer. Only admin may call. + /// + /// # Roster freeze policy (Issue #1024) + /// + /// The signer roster is frozen while any proposal is `Active`. `vote` + /// and `get_proposal_participation` read the **live** signer count, so + /// changing the roster mid-vote would silently alter a proposal's + /// quorum math (a removed signer can shrink `total_signers` and force + /// an early rejection; an added signer can keep a doomed proposal + /// alive). Admin roster changes are therefore rejected until every + /// active proposal has closed; use `emergency_override` for urgent + /// cases. #[ink(message)] pub fn add_signer(&mut self, new_signer: AccountId) -> Result<(), Error> { self.ensure_admin()?; + if self.active_proposal_count > 0 { + return Err(Error::SignerChangesLocked); + } + if self.signers.contains(&new_signer) { return Err(Error::SignerExists); } @@ -784,10 +799,20 @@ pub mod governance { } /// Removes a signer. Only admin may call. + /// + /// # Roster freeze policy (Issue #1024) + /// + /// Same freeze as `add_signer`: the roster cannot change while any + /// proposal is `Active`, so a proposal's pass/reject outcome never + /// depends on when the admin happened to edit the signer set. #[ink(message)] pub fn remove_signer(&mut self, signer: AccountId) -> Result<(), Error> { self.ensure_admin()?; + if self.active_proposal_count > 0 { + return Err(Error::SignerChangesLocked); + } + if self.signers.len() as u32 <= constants::GOVERNANCE_MIN_SIGNERS { return Err(Error::MinSigners); } diff --git a/contracts/governance/src/tests.rs b/contracts/governance/src/tests.rs index e586d465d..4e9eff94c 100644 --- a/contracts/governance/src/tests.rs +++ b/contracts/governance/src/tests.rs @@ -146,6 +146,49 @@ mod tests { assert_eq!(gov.get_signers().len(), 3); } + #[ink::test] + fn signer_changes_locked_while_proposal_active() { + let mut gov = create_governance(); + let accounts = default_accounts(); + set_caller(accounts.alice); + + gov.create_proposal(dummy_hash(), GovernanceAction::ModifyProperty, None) + .unwrap(); + assert_eq!(gov.get_active_proposal_count(), 1); + + // The roster is frozen while a proposal is actively voting, so a + // mid-vote roster change cannot alter the proposal's quorum math. + assert_eq!(gov.add_signer(accounts.django), Err(Error::SignerChangesLocked)); + assert_eq!(gov.remove_signer(accounts.charlie), Err(Error::SignerChangesLocked)); + assert_eq!(gov.get_signers().len(), 3); + } + + #[ink::test] + fn signer_changes_allowed_after_proposal_closes() { + let mut gov = create_governance(); + let accounts = default_accounts(); + set_caller(accounts.alice); + + gov.create_proposal(dummy_hash(), GovernanceAction::ModifyProperty, None) + .unwrap(); + + // Reject the proposal: alice and bob vote no, leaving no path to the + // threshold of 2 with 3 signers. + gov.vote(0, false).unwrap(); + set_caller(accounts.bob); + gov.vote(0, false).unwrap(); + assert_eq!(gov.get_proposal(0).unwrap().status, ProposalStatus::Rejected); + assert_eq!(gov.get_active_proposal_count(), 0); + + // With no active proposals the roster can be edited again. + set_caller(accounts.alice); + gov.add_signer(accounts.django).unwrap(); + gov.remove_signer(accounts.charlie).unwrap(); + assert_eq!(gov.get_signers().len(), 3); + assert!(gov.get_signers().contains(&accounts.django)); + assert!(!gov.get_signers().contains(&accounts.charlie)); + } + #[ink::test] fn cannot_remove_below_min_signers() { let accounts = default_accounts(); diff --git a/contracts/traits/src/errors.rs b/contracts/traits/src/errors.rs index 1abf667a4..501cc10e9 100644 --- a/contracts/traits/src/errors.rs +++ b/contracts/traits/src/errors.rs @@ -430,6 +430,8 @@ pub mod governance_codes { pub const GOVERNANCE_MAX_PROPOSALS: u32 = 8011; pub const GOVERNANCE_NOT_A_SIGNER: u32 = 8012; pub const GOVERNANCE_PROPOSAL_EXPIRED: u32 = 8013; + /// Signer roster changes are blocked while proposals are actively voting. + pub const GOVERNANCE_SIGNER_CHANGES_LOCKED: u32 = 8014; } /// Staking error codes (9000-9999)