diff --git a/pallets/common/src/traits/multisig.rs b/pallets/common/src/traits/multisig.rs index 69bc4de65..eff080119 100644 --- a/pallets/common/src/traits/multisig.rs +++ b/pallets/common/src/traits/multisig.rs @@ -40,6 +40,7 @@ pub trait WeightInfo { fn create_join_identity() -> Weight; fn approve_join_identity() -> Weight; fn join_identity() -> Weight; + fn remove_admin() -> Weight; fn default_max_weight(max_weight: &Option) -> Weight { max_weight.unwrap_or_else(|| { diff --git a/pallets/multisig/src/benchmarking.rs b/pallets/multisig/src/benchmarking.rs index f03e94d77..8150fa716 100644 --- a/pallets/multisig/src/benchmarking.rs +++ b/pallets/multisig/src/benchmarking.rs @@ -369,4 +369,9 @@ benchmarks! { MultiSig::::approve_join_identity(users[0].origin().into(), multisig.clone(), auth_id).unwrap(); // The second approval call just approves. }: approve_join_identity(users[1].origin(), multisig, auth_id) + + remove_admin { + let (alice, multisig, _, _, multisig_origin) = generate_multisig_for_alice::(2, 2).unwrap(); + init_admin(&multisig, &alice); + }: _(multisig_origin) } diff --git a/pallets/multisig/src/lib.rs b/pallets/multisig/src/lib.rs index 4d3941992..4cfef8116 100644 --- a/pallets/multisig/src/lib.rs +++ b/pallets/multisig/src/lib.rs @@ -533,6 +533,21 @@ pub mod pallet { pallet_identity::Module::::join_identity(origin, auth_id)?; Ok(().into()) } + + /// Removes the admin identity from the `multisig`. This must be called by the multisig itself. + #[pallet::call_index(17)] + #[pallet::weight(::WeightInfo::remove_admin())] + pub fn remove_admin(origin: OriginFor) -> DispatchResultWithPostInfo { + let multisig = ensure_signed(origin)?; + let caller_did = Self::ensure_ms_has_did(&multisig)?; + let admin_did = AdminDid::::take(&multisig).ok_or(Error::::AdminNotFound)?; + Self::deposit_event(Event::MultiSigRemovedAdmin { + caller_did, + multisig, + admin_did, + }); + Ok(().into()) + } } #[pallet::event] @@ -677,6 +692,12 @@ pub mod pallet { TooManySigners, /// Multisig doesn't have a paying DID. NoPayingDid, + /// Expiry must be in the future. + InvalidExpiryDate, + /// The proposal has been invalidated after a multisg update. + InvalidatedProposal, + /// Multisig has no admin. + AdminNotFound, } /// Nonce to ensure unique MultiSig addresses are generated; starts from 1. @@ -783,6 +804,14 @@ pub mod pallet { #[pallet::getter(fn transaction_version)] pub(super) type TransactionVersion = StorageValue<_, u32, ValueQuery>; + /// The last proposal id before the multisig changed signers or signatures required. + /// + /// multisig => Option + #[pallet::storage] + #[pallet::getter(fn last_invalid_proposal)] + pub type LastInvalidProposal = + StorageMap<_, Identity, T::AccountId, u64, OptionQuery>; + /// Storage version. #[pallet::storage] #[pallet::getter(fn storage_version)] @@ -876,8 +905,12 @@ impl Pallet { Some(ProposalState::ExecutionSuccessful | ProposalState::ExecutionFailed) => { Err(Error::::ProposalAlreadyExecuted.into()) } - Some(ProposalState::Active { until: None }) => Ok(()), + Some(ProposalState::Active { until: None }) => { + Self::ensure_valid_proposal(multisig, proposal_id)?; + Ok(()) + } Some(ProposalState::Active { until: Some(until) }) => { + Self::ensure_valid_proposal(multisig, proposal_id)?; // Ensure proposal is not expired ensure!( until > pallet_timestamp::Pallet::::get(), @@ -949,6 +982,7 @@ impl Pallet { } NumberOfSigners::::insert(&multisig, pending_num_of_signers); + Self::set_invalid_proposals(&multisig); Self::deposit_event(Event::MultiSigSignersRemoved { caller_did: caller_did.unwrap_or(ms_did), multisig: multisig.clone(), @@ -1004,6 +1038,7 @@ impl Pallet { let max_weight = proposal.get_dispatch_info().weight; let caller_did = Self::ensure_ms_get_did(multisig)?; let proposal_id = Self::next_proposal_id(multisig); + Self::ensure_valid_expiry(&expiry)?; Proposals::::insert(multisig, proposal_id, &*proposal); ProposalVoteCounts::::insert(multisig, proposal_id, ProposalVoteCount::default()); @@ -1211,6 +1246,8 @@ impl Pallet { IdentityPallet::::ensure_auth_by(ms_identity, auth_by)?; + Self::set_invalid_proposals(&multisig); + // Update number of signers for this multisig. let pending_num_of_signers = Self::ensure_max_signers(&multisig, 1)?; NumberOfSigners::::insert(&multisig, pending_num_of_signers); @@ -1280,6 +1317,7 @@ impl Pallet { Error::::ChangeNotAllowed ); MultiSigSignsRequired::::insert(multisig, &signatures_required); + Self::set_invalid_proposals(&multisig); Self::deposit_event(Event::MultiSigSignersRequiredChanged { caller_did: caller_did.or(ms_did), multisig: multisig.clone(), @@ -1287,6 +1325,40 @@ impl Pallet { }); Ok(()) } + + /// Returns `Ok` if `expiry` is in the future. Otherwise, returns [`Error::InvalidExpiryDate`]. + fn ensure_valid_expiry(expiry: &Option) -> DispatchResult { + if let Some(expiry) = expiry { + ensure!( + expiry > &pallet_timestamp::Pallet::::get(), + Error::::InvalidExpiryDate + ); + } + Ok(()) + } + + /// Returns `Ok` if `proposal_id` is valid. Otherwise, returns [`Error::InvalidatedProposal`]. + fn ensure_valid_proposal(multisig: &T::AccountId, proposal_id: u64) -> DispatchResult { + if let Some(last_invalid_proposal) = Self::last_invalid_proposal(multisig) { + ensure!( + proposal_id > last_invalid_proposal, + Error::::InvalidatedProposal + ); + } + Ok(()) + } + + /// Sets [`LastInvalidProposal`] with the proposal id of the last proposal. + fn set_invalid_proposals(multisig: &T::AccountId) { + let next_proposal_id = Self::next_proposal_id(multisig); + + // There are no proposals for the multisig + if next_proposal_id == 0 { + return; + } + + LastInvalidProposal::::insert(multisig, next_proposal_id.saturating_sub(1)); + } } impl MultiSigSubTrait for Pallet { diff --git a/pallets/runtime/tests/src/multisig.rs b/pallets/runtime/tests/src/multisig.rs index 19bdb7fa1..a9bd77af6 100644 --- a/pallets/runtime/tests/src/multisig.rs +++ b/pallets/runtime/tests/src/multisig.rs @@ -3,7 +3,9 @@ use frame_support::{ dispatch::DispatchResult, BoundedVec, }; -use pallet_multisig::{self as multisig, AdminDid, ProposalStates, ProposalVoteCounts, Votes}; +use pallet_multisig::{ + self as multisig, AdminDid, LastInvalidProposal, ProposalStates, ProposalVoteCounts, Votes, +}; use polymesh_common_utilities::constants::currency::POLY; use polymesh_primitives::multisig::ProposalState; use polymesh_primitives::{AccountId, AuthorizationData, Permissions, SecondaryKey, Signatory}; @@ -1253,6 +1255,51 @@ fn proposal_owner_rejection_denied() { }); } +#[test] +fn remove_admin_successfully() { + ExtBuilder::default().build().execute_with(|| { + // Multisig creator + let alice = User::new(AccountKeyring::Alice); + // Multisig signers + let ferdie_signer = AccountKeyring::Ferdie.to_account_id(); + let bob_signer = AccountKeyring::Bob.to_account_id(); + let charlie_signer = AccountKeyring::Charlie.to_account_id(); + + let ms_address = create_multisig_default_perms( + alice.acc(), + create_signers(vec![ferdie_signer, bob_signer.clone(), charlie_signer]), + 2, + ); + assert!(AdminDid::::get(&ms_address).is_some()); + + assert_ok!(MultiSig::remove_admin(Origin::signed(ms_address.clone()))); + assert!(AdminDid::::get(ms_address).is_none()) + }); +} + +#[test] +fn remove_admin_not_multsig() { + ExtBuilder::default().build().execute_with(|| { + // Multisig creator + let alice = User::new(AccountKeyring::Alice); + // Multisig signers + let ferdie_signer = AccountKeyring::Ferdie.to_account_id(); + let bob_signer = AccountKeyring::Bob.to_account_id(); + let charlie_signer = AccountKeyring::Charlie.to_account_id(); + + create_multisig_default_perms( + alice.acc(), + create_signers(vec![ferdie_signer, bob_signer.clone(), charlie_signer]), + 2, + ); + + assert_noop!( + MultiSig::remove_admin(alice.origin()), + Error::NoSuchMultisig + ); + }); +} + fn expired_proposals() { ExtBuilder::default().build().execute_with(|| { let alice = User::new(AccountKeyring::Alice); @@ -1395,6 +1442,270 @@ fn multisig_nesting_not_allowed() { }); } +#[test] +fn create_expired_proposal() { + ExtBuilder::default().build().execute_with(|| { + let alice = User::new(AccountKeyring::Alice); + let bob_key = AccountKeyring::Bob.to_account_id(); + let ferdie_key = AccountKeyring::Ferdie.to_account_id(); + let ferdie = Origin::signed(ferdie_key.clone()); + let charlie_key = AccountKeyring::Charlie.to_account_id(); + + let ms_address = setup_multisig( + alice.acc(), + 3, + create_signers(vec![ferdie_key, bob_key, charlie_key]), + ); + + let expires_at = 100u64; + let call = Box::new(RuntimeCall::MultiSig( + multisig::Call::change_sigs_required { sigs_required: 2 }, + )); + + set_timestamp(expires_at); + + assert_eq!( + MultiSig::create_proposal(ferdie.clone(), ms_address.clone(), call, Some(100u64),) + .unwrap_err() + .error, + Error::InvalidExpiryDate.into() + ) + }); +} + +#[test] +fn invalidate_proposals_change_sigs_required() { + ExtBuilder::default().build().execute_with(|| { + let alice = User::new(AccountKeyring::Alice); + let bob_key = AccountKeyring::Bob.to_account_id(); + let ferdie_key = AccountKeyring::Ferdie.to_account_id(); + let ferdie = Origin::signed(ferdie_key.clone()); + let charlie_key = AccountKeyring::Charlie.to_account_id(); + + let ms_address = setup_multisig( + alice.acc(), + 3, + create_signers(vec![ferdie_key, bob_key, charlie_key]), + ); + + let call = Box::new(RuntimeCall::MultiSig( + multisig::Call::change_sigs_required { sigs_required: 2 }, + )); + + assert_ok!(MultiSig::create_proposal( + ferdie.clone(), + ms_address.clone(), + call, + None + )); + + assert_ok!(MultiSig::change_sigs_required_via_admin( + alice.origin(), + ms_address.clone(), + 2 + )); + + assert_eq!( + LastInvalidProposal::::get(&ms_address), + Some(0) + ); + + assert_eq!( + MultiSig::approve(ferdie.clone(), ms_address.clone(), 0, None) + .unwrap_err() + .error, + Error::InvalidatedProposal.into() + ); + assert_eq!( + MultiSig::reject(ferdie, ms_address, 0).unwrap_err().error, + Error::InvalidatedProposal.into() + ); + }); +} + +#[test] +fn invalidate_proposals_add_signer() { + ExtBuilder::default().build().execute_with(|| { + let alice = User::new(AccountKeyring::Alice); + let bob_key = AccountKeyring::Bob.to_account_id(); + let ferdie_key = AccountKeyring::Ferdie.to_account_id(); + let ferdie = Origin::signed(ferdie_key.clone()); + let charlie = Origin::signed(AccountKeyring::Charlie.to_account_id()); + let charlie_key = AccountKeyring::Charlie.to_account_id(); + + let ms_address = setup_multisig(alice.acc(), 2, create_signers(vec![ferdie_key, bob_key])); + + let call = Box::new(RuntimeCall::MultiSig( + multisig::Call::change_sigs_required { sigs_required: 2 }, + )); + + assert_ok!(MultiSig::create_proposal( + ferdie.clone(), + ms_address.clone(), + call, + None + )); + + assert_ok!(MultiSig::add_multisig_signers_via_admin( + alice.origin(), + ms_address.clone(), + create_signers(vec![charlie_key.clone()]) + )); + + assert_eq!(LastInvalidProposal::::get(&ms_address), None); + + let charlie_auth_id = get_last_auth_id(&charlie_key); + assert_ok!(MultiSig::accept_multisig_signer( + charlie.clone(), + charlie_auth_id + )); + + assert_eq!( + LastInvalidProposal::::get(&ms_address), + Some(0) + ); + + assert_eq!( + MultiSig::approve(ferdie.clone(), ms_address.clone(), 0, None) + .unwrap_err() + .error, + Error::InvalidatedProposal.into() + ); + assert_eq!( + MultiSig::reject(ferdie, ms_address, 0).unwrap_err().error, + Error::InvalidatedProposal.into() + ); + }); +} + +#[test] +fn invalidate_proposals_remove_signer() { + ExtBuilder::default().build().execute_with(|| { + let alice = User::new(AccountKeyring::Alice); + let bob_key = AccountKeyring::Bob.to_account_id(); + let ferdie_key = AccountKeyring::Ferdie.to_account_id(); + let ferdie = Origin::signed(ferdie_key.clone()); + let charlie_key = AccountKeyring::Charlie.to_account_id(); + + let ms_address = setup_multisig( + alice.acc(), + 2, + create_signers(vec![ferdie_key, bob_key, charlie_key.clone()]), + ); + + let call = Box::new(RuntimeCall::MultiSig( + multisig::Call::change_sigs_required { sigs_required: 2 }, + )); + + assert_ok!(MultiSig::create_proposal( + ferdie.clone(), + ms_address.clone(), + call, + None + )); + + assert_ok!(MultiSig::remove_multisig_signers_via_admin( + alice.origin(), + ms_address.clone(), + create_signers(vec![charlie_key]) + )); + + assert_eq!( + LastInvalidProposal::::get(&ms_address), + Some(0) + ); + + assert_eq!( + MultiSig::approve(ferdie.clone(), ms_address.clone(), 0, None) + .unwrap_err() + .error, + Error::InvalidatedProposal.into() + ); + assert_eq!( + MultiSig::reject(ferdie, ms_address, 0).unwrap_err().error, + Error::InvalidatedProposal.into() + ); + }); +} + +#[test] +fn invalidate_proposals_via_executed_proposal() { + ExtBuilder::default().build().execute_with(|| { + let alice = User::new(AccountKeyring::Alice); + let bob = Origin::signed(AccountKeyring::Bob.to_account_id()); + let bob_signer = AccountKeyring::Bob.to_account_id(); + let charlie = Origin::signed(AccountKeyring::Charlie.to_account_id()); + let charlie_signer = AccountKeyring::Charlie.to_account_id(); + + let ms_address = create_multisig_default_perms( + alice.acc(), + create_signers(vec![charlie_signer.clone(), bob_signer.clone()]), + 2, + ); + + let charlie_auth_id = get_last_auth_id(&charlie_signer); + assert_ok!(MultiSig::accept_multisig_signer( + charlie.clone(), + charlie_auth_id + )); + + let bob_auth_id = get_last_auth_id(&bob_signer); + assert_ok!(MultiSig::accept_multisig_signer(bob.clone(), bob_auth_id)); + + let call = Box::new(RuntimeCall::MultiSig( + multisig::Call::change_sigs_required { sigs_required: 1 }, + )); + assert_ok!(MultiSig::create_proposal( + bob.clone(), + ms_address.clone(), + call.clone(), + None, + )); + + assert_ok!(MultiSig::create_proposal( + bob.clone(), + ms_address.clone(), + call.clone(), + None, + )); + + assert_ok!(MultiSig::create_proposal( + bob.clone(), + ms_address.clone(), + call, + None, + )); + + assert_ok!(MultiSig::approve( + charlie.clone(), + ms_address.clone(), + 0, + None + )); + + // At this point the proposal of id 0 executes + next_block(); + + // All other proposals must have been invalidated + assert_eq!( + LastInvalidProposal::::get(&ms_address), + Some(2) + ); + assert_eq!( + MultiSig::approve(charlie.clone(), ms_address.clone(), 1, None) + .unwrap_err() + .error, + Error::InvalidatedProposal.into() + ); + assert_eq!( + MultiSig::reject(charlie.clone(), ms_address, 2) + .unwrap_err() + .error, + Error::InvalidatedProposal.into() + ); + }); +} + fn setup_multisig( creator: AccountId, sigs_required: u64, diff --git a/pallets/weights/src/pallet_multisig.rs b/pallets/weights/src/pallet_multisig.rs index 6e8cdd8e2..32d45bda2 100644 --- a/pallets/weights/src/pallet_multisig.rs +++ b/pallets/weights/src/pallet_multisig.rs @@ -471,4 +471,18 @@ impl pallet_multisig::WeightInfo for SubstrateWeight { .saturating_add(DbWeight::get().reads(8)) .saturating_add(DbWeight::get().writes(2)) } + // Storage: MultiSig MultiSigSignsRequired (r:1 w:0) + // Proof Skipped: MultiSig MultiSigSignsRequired (max_values: None, max_size: None, mode: Measured) + // Storage: Identity KeyRecords (r:1 w:0) + // Proof Skipped: Identity KeyRecords (max_values: None, max_size: None, mode: Measured) + // Storage: Identity IsDidFrozen (r:1 w:0) + // Proof Skipped: Identity IsDidFrozen (max_values: None, max_size: None, mode: Measured) + // Storage: MultiSig AdminDid (r:1 w:1) + // Proof Skipped: MultiSig AdminDid (max_values: None, max_size: None, mode: Measured) + fn remove_admin() -> Weight { + // Minimum execution time: 29_555 nanoseconds. + Weight::from_ref_time(29_694_000) + .saturating_add(DbWeight::get().reads(4)) + .saturating_add(DbWeight::get().writes(1)) + } }