From fb7fdf356f6f8bf508353c882945ca7167fb8d0b Mon Sep 17 00:00:00 2001 From: Karlo <88337245+Rqnsom@users.noreply.github.com> Date: Tue, 26 Mar 2024 12:36:50 +0100 Subject: [PATCH] refactor: optimise solo signer in regards to multisig - Optimization included. - Block calculation updated. (for cleaning up the multisig storage) - Included new tests. - Updated existing tests with additional checks. - Fixed one tiny bug that allowed users to fill out the multisig storage when the wrong user tried to sign the single signer script. --- pallet/src/balance.rs | 1 - pallet/src/lib.rs | 88 ++++++++++++++++++---------- pallet/src/mock.rs | 2 +- pallet/src/signer.rs | 117 ++++++++++++++++++++----------------- pallet/src/tests/signer.rs | 99 ++++++++++++++++++++++++++++++- 5 files changed, 218 insertions(+), 89 deletions(-) diff --git a/pallet/src/balance.rs b/pallet/src/balance.rs index e5c3e38..1bb3a5f 100644 --- a/pallet/src/balance.rs +++ b/pallet/src/balance.rs @@ -14,7 +14,6 @@ use move_vm_backend::balance::BalanceHandler; use sp_runtime::traits::Zero; use sp_std::{ cell::{Ref, RefCell}, - default::Default, rc::Rc, vec::Vec, }; diff --git a/pallet/src/lib.rs b/pallet/src/lib.rs index 1bcb051..e7b5415 100644 --- a/pallet/src/lib.rs +++ b/pallet/src/lib.rs @@ -45,6 +45,7 @@ pub mod pallet { traits::{ tokens::currency::LockIdentifier, Currency, Get, LockableCurrency, ReservableCurrency, }, + BoundedBTreeSet, }; use frame_system::pallet_prelude::*; pub use move_core_types::language_storage::TypeTag; @@ -86,7 +87,7 @@ pub mod pallet { /// Storage for multi-signature/signer requests. #[pallet::storage] - pub type MultisigStorage = StorageMap<_, Blake2_128Concat, CallHash, MultisigOf>; + pub type MultisigStorage = StorageMap<_, Blake2_128Concat, CallHash, SigDataOf>; #[pallet::storage] pub type ChoreOnIdleStorage = StorageMap< @@ -252,45 +253,61 @@ pub mod pallet { // Make sure the scripts are not maliciously trying to use forged signatures. let signer_count = verify_script_integrity_and_check_signers(&bytecode).map_err(Error::::from)?; - let accounts = Self::extract_account_ids_from_args(&args, signer_count)?; - let block_height = >::block_number(); - - let (mut signature_handler, call_hash) = if signer_count > 1 { - // Generate the call hash to identify this multi-sign call request. - let call_hash = Self::transaction_bc_call_hash(&transaction_bc[..]); + let unique_signers = Self::extract_account_ids_from_args(&args, signer_count)?; + + // Based on the number of unique signers, decide the following: + let (is_signature_required, contains_multisig) = match unique_signers.len() { + 0 => (false, None), + 1 => (true, None), + _ => ( + true, + // Generate the unique script tx hash to identify this multi-sign call request. + Some(Self::transaction_bc_call_hash(&transaction_bc[..])), + ), + }; - let multisig = MultisigStorage::::get(call_hash).unwrap_or( - MultisigOf::::new(accounts, block_height).map_err(Into::>::into)?, + let mut signature_handler = if let Some(script_hash) = contains_multisig { + let multisig_data = MultisigStorage::::get(script_hash).unwrap_or( + SigDataOf::::new(unique_signers).map_err(Into::>::into)?, ); - (ScriptSignatureHandler::::from(multisig), call_hash) + ScriptSignatureHandler::::from(multisig_data) } else { - ( - ScriptSignatureHandler::::new(accounts, block_height)?, - [0u8; 32], - ) + ScriptSignatureHandler::::new(unique_signers)? }; - if signer_count > 0 { + + if is_signature_required { let lock_id = Self::multi_signer_lock_id(&who, &transaction_bc[..], &cheque_limit); signature_handler.sign_script(&who, &cheque_limit, lock_id)?; } - // If the script is signed correctly, we can execute it in MoveVM and update the - // blockchain storage or the balance sheet. - // If we have only one signer, it will skip this; if not, we have to wait for more signers, so we store it as a multi-signer request. + // The script needs to be signed by all signers before we can execute it in MoveVM and + // update the blockchain storage or the balance sheet. if !signature_handler.all_signers_approved() { - // Use the initial block height always when the first signer initiates the - // multi-signer execution request. This will prevent the lifetime of such requests - // from being extended with each signing. - let multisig = signature_handler.into_inner(); - Self::new_multi_sign_request_chore(*multisig.block_number(), call_hash)?; - MultisigStorage::::insert(call_hash, multisig); + // We can enter this block only in multisig scenario, so unwrap can't fail here. + let script_hash = contains_multisig.expect("multisig script hash not found"); + let mut sig_data = signature_handler.into_inner(); + + // The deadline for collecting all signatures is set by the first signer in the + // multisig scenario. There's no way to extend the initally set deadline. + if sig_data.stored_block_height().is_none() { + let block_height = >::block_number(); + + sig_data.set_block_height(block_height); + Self::new_multi_sign_request_chore(block_height, script_hash)?; + } + + MultisigStorage::::insert(script_hash, sig_data); + Self::deposit_event(Event::SignedMultisigScript { who }); return result::execute_only_signing(); } + // If we have multiple signers and they all have signed, we have to remove the multi-signer request from the MultisigStorage. - if signer_count > 1 { - MultisigStorage::::remove(call_hash); + if let Some(script_hash) = contains_multisig { + // TODO: Consider remove the entry from the ChoreOnIdleStorage in the future. Not + // cleaning it shall cause no harm. + MultisigStorage::::remove(script_hash); } // We need to provide MoveVM read only access to balance sheet - MoveVM is allowed to @@ -308,7 +325,11 @@ pub mod pallet { let result = result::from_vm_result::(vm_result)?; // Emit an event. - let signers = signature_handler.into_signer_accounts()?; + let mut signers = signature_handler.into_signer_accounts()?; + if signers.is_empty() { + // Signer list can be empty in zero-signer scripts, so append here the user at least. + signers.push(who); + } Self::deposit_event(Event::ExecuteCalled { who: signers }); Ok(result) @@ -518,17 +539,20 @@ pub mod pallet { pub(crate) fn extract_account_ids_from_args( script_args: &[&[u8]], signer_count: usize, - ) -> Result, Error> { + ) -> Result, Error> { if signer_count > script_args.len() { return Err(Error::::ScriptSignatureFailure); } - let mut accounts = Vec::::new(); + let mut accounts = BoundedBTreeSet::::new(); for signer in &script_args[..signer_count] { let account_address = bcs::from_bytes(signer).map_err(|_| Error::::UnableToDeserializeAccount)?; let account = Self::to_native_account(&account_address)?; - accounts.push(account); + + accounts + .try_insert(account) + .map_err(|_| Error::::NumberOfSignerArgumentsMismatch)?; } Ok(accounts) @@ -570,7 +594,7 @@ pub mod pallet { let Ok(hashes_ready_for_cleanup) = ChoreOnIdleStorage::::try_get(block) else { return T::DbWeight::get().reads(1); }; - // We don't need this entry in storage anymore, remove it! + // We don't need this entry in storage anymore, remove it. ChoreOnIdleStorage::::remove(block); // Remove all that entries from MultisigStorage. @@ -622,6 +646,8 @@ pub mod pallet { InsufficientBalance, /// Script signature failure. ScriptSignatureFailure, + /// If the script expects a list of signers, only users from that list can sign the transaction. + UnexpectedUserSignature, /// Script transaction cannot be deserialized. InvalidScriptTransaction, /// User tried to publish module in a protected memory area. diff --git a/pallet/src/mock.rs b/pallet/src/mock.rs index a4605e3..f3c0829 100644 --- a/pallet/src/mock.rs +++ b/pallet/src/mock.rs @@ -172,7 +172,7 @@ pub(crate) fn addrs_from_ss58(ss58: &str) -> Result<(AccountId32, AccountAddress Ok((addr_32, addr_mv)) } -/// Rolls forward in history to the given block height. +/// Rolls forward in future to the given block height. pub(crate) fn roll_to(n: BlockNumberFor) { let weight = Weight::from_parts(100_000_000_000, 1); while System::block_number() < n { diff --git a/pallet/src/signer.rs b/pallet/src/signer.rs index 2ba4d42..c506981 100644 --- a/pallet/src/signer.rs +++ b/pallet/src/signer.rs @@ -10,7 +10,7 @@ use frame_support::{ }, Get, }, - BoundedBTreeMap, Parameter, + BoundedBTreeMap, BoundedBTreeSet, Parameter, }; use frame_system::{pallet_prelude::BlockNumberFor, Config as SysConfig}; use scale_info::TypeInfo; @@ -24,7 +24,7 @@ use crate::{ // Some alias definition to make life easier. pub type MaxSignersOf = ::MaxScriptSigners; -pub type MultisigOf = Multisig, BalanceOf, BlockNumberFor, MaxSignersOf>; +pub type SigDataOf = SigData, BalanceOf, BlockNumberFor, MaxSignersOf>; /// This definition stores the hash value of a script transaction. pub type CallHash = [u8; 32]; @@ -40,20 +40,20 @@ pub enum Signature { } #[derive(Clone, Eq, PartialEq)] -pub enum MultisigError { +pub enum SigDataError { MaxSignersExceeded, ScriptSignatureFailure, UnableToDeserializeAccount, UserHasAlreadySigned, } -impl From for Error { - fn from(err: MultisigError) -> Self { +impl From for Error { + fn from(err: SigDataError) -> Self { match err { - MultisigError::MaxSignersExceeded => Error::::MaxSignersExceeded, - MultisigError::ScriptSignatureFailure => Error::::ScriptSignatureFailure, - MultisigError::UnableToDeserializeAccount => Error::::UnableToDeserializeAccount, - MultisigError::UserHasAlreadySigned => Error::::UserHasAlreadySigned, + SigDataError::MaxSignersExceeded => Error::::MaxSignersExceeded, + SigDataError::ScriptSignatureFailure => Error::::ScriptSignatureFailure, + SigDataError::UnableToDeserializeAccount => Error::::UnableToDeserializeAccount, + SigDataError::UserHasAlreadySigned => Error::::UserHasAlreadySigned, } } } @@ -71,7 +71,7 @@ pub struct SignerData { /// Storage struct definition for a multi-signer request. #[derive(Clone, Eq, PartialEq, Encode, Decode, RuntimeDebug, TypeInfo, MaxEncodedLen)] #[scale_info(skip_type_params(Size))] -pub struct Multisig +pub struct SigData where AccountId: Parameter + Ord + MaybeSerializeDeserialize, Balance: From + Into + Default, @@ -80,43 +80,50 @@ where { /// The signers of a script transaction. signers: BoundedBTreeMap, Size>, - /// The block number when this `Multisig` was created and stored. - block_height: BlockNumber, + + /// The block height at which this `SigData` was initally stored. + /// + /// Used only for multisig purposes. Is set to `None` otherwise (non-multisig scenarios). + stored_block_height: Option, } -impl Multisig +impl SigData where AccountId: Parameter + Ord + MaybeSerializeDeserialize, Balance: From + Into + Default, BlockNumber: Parameter + Ord + MaybeSerializeDeserialize + Default, Size: Get, { - /// Creates a new [`Multisig`] with all blank signatures for the provided script. - pub fn new(signers: Vec, block_height: BlockNumber) -> Result { + /// Creates a new [`SigData`] with all blank signatures for the provided script. + pub fn new(signers: BoundedBTreeSet) -> Result { if signers.len() > (Size::get() as usize) { - return Err(MultisigError::MaxSignersExceeded); + return Err(SigDataError::MaxSignersExceeded); } - let mut multisig_info = Multisig:: { - block_height, + let mut sig_info = SigData:: { + stored_block_height: None, ..Default::default() }; for account in signers.iter() { - multisig_info + sig_info .try_insert(account.clone(), SignerData::default()) - .map_err(|_| MultisigError::UnableToDeserializeAccount)?; + .map_err(|_| SigDataError::UnableToDeserializeAccount)?; } - Ok(multisig_info) + Ok(sig_info) + } + + pub fn stored_block_height(&self) -> Option<&BlockNumber> { + self.stored_block_height.as_ref() } - pub fn block_number(&self) -> &BlockNumber { - &self.block_height + pub fn set_block_height(&mut self, block_height: BlockNumber) { + self.stored_block_height = Some(block_height); } } impl core::ops::Deref - for Multisig + for SigData where AccountId: Parameter + Ord + MaybeSerializeDeserialize, Balance: From + Into + Default, @@ -131,7 +138,7 @@ where } impl core::ops::DerefMut - for Multisig + for SigData where AccountId: Parameter + Ord + MaybeSerializeDeserialize, Balance: From + Into + Default, @@ -145,7 +152,7 @@ where // Because in substrate_move::AccountAddress Default impl is missing. impl Default - for Multisig + for SigData where AccountId: Parameter + Ord + MaybeSerializeDeserialize, Balance: From + Into + Default, @@ -153,9 +160,9 @@ where Size: Get, { fn default() -> Self { - Multisig { + SigData { signers: BoundedBTreeMap::, Size>::new(), - block_height: BlockNumber::default(), + stored_block_height: None, } } } @@ -163,20 +170,18 @@ where /// Script signature handler tracks required signatures for the provided script. pub(crate) struct ScriptSignatureHandler { _pd_config: PhantomData, - /// All required multisig_info. - multisig_info: MultisigOf, + /// All required script signature info. + sig_info: SigDataOf, } impl ScriptSignatureHandler { /// Creates a new [`ScriptSignatureHandler`] with all blank signatures for the provided script. pub(crate) fn new( - accounts: Vec, - block_height: BlockNumberFor, + accounts: BoundedBTreeSet, ) -> Result> { Ok(Self { _pd_config: PhantomData, - multisig_info: MultisigOf::::new(accounts, block_height) - .map_err(Into::>::into)?, + sig_info: SigDataOf::::new(accounts).map_err(Into::>::into)?, }) } @@ -193,24 +198,26 @@ impl ScriptSignatureHandler { cheque_limit: &BalanceOf, lock_id: LockIdentifier, ) -> Result<(), Error> { - if let Some(ms_data) = self.multisig_info.get_mut(account) { - if matches!(ms_data.signature, Signature::Approved) { - Err(Error::::UserHasAlreadySigned) - } else { - ms_data.signature = Signature::Approved; - ms_data.cheque_limit = *cheque_limit; - ms_data.lock_id = lock_id; - T::Currency::set_lock(lock_id, account, *cheque_limit, WithdrawReasons::all()); - Ok(()) - } - } else { - Err(Error::::ScriptSignatureFailure) + // Only users that are on the signer list can sign this script. + let Some(ms_data) = self.sig_info.get_mut(account) else { + return Err(Error::::UnexpectedUserSignature); + }; + + // User can sign only once. + if matches!(ms_data.signature, Signature::Approved) { + return Err(Error::::UserHasAlreadySigned); } + + ms_data.signature = Signature::Approved; + ms_data.cheque_limit = *cheque_limit; + ms_data.lock_id = lock_id; + T::Currency::set_lock(lock_id, account, *cheque_limit, WithdrawReasons::all()); + Ok(()) } /// Check whether the script has been approved by all required signers. pub(crate) fn all_signers_approved(&self) -> bool { - self.multisig_info + self.sig_info .values() .all(|signer| signer.signature == Signature::Approved) } @@ -219,7 +226,7 @@ impl ScriptSignatureHandler { /// Function returns an error if not all signers have signed. pub(crate) fn write_cheques(&self) -> Result, Error> { let mut balances = BalanceAdapter::::new(); - for (account, ms_data) in self.multisig_info.iter() { + for (account, ms_data) in self.sig_info.iter() { T::Currency::remove_lock(ms_data.lock_id, account); balances .write_cheque(account, &ms_data.cheque_limit) @@ -229,23 +236,23 @@ impl ScriptSignatureHandler { Ok(balances) } - /// Consumes [`ScriptSignatureHandler`] and returns innner `Multisig`. - pub(crate) fn into_inner(self) -> MultisigOf { - self.multisig_info + /// Consumes [`ScriptSignatureHandler`] and returns innner `SigData`. + pub(crate) fn into_inner(self) -> SigDataOf { + self.sig_info } /// Consumes [`ScriptSignatureHandler`] and returns accounts of all signers as vector. pub(crate) fn into_signer_accounts(self) -> Result, Error> { - let accounts: Vec = self.multisig_info.keys().cloned().collect(); + let accounts: Vec = self.sig_info.keys().cloned().collect(); Ok(accounts) } } -impl From> for ScriptSignatureHandler { - fn from(multisig_info: MultisigOf) -> Self { +impl From> for ScriptSignatureHandler { + fn from(sig_info: SigDataOf) -> Self { Self { _pd_config: PhantomData, - multisig_info, + sig_info, } } } diff --git a/pallet/src/tests/signer.rs b/pallet/src/tests/signer.rs index 40aadbd..72d6792 100644 --- a/pallet/src/tests/signer.rs +++ b/pallet/src/tests/signer.rs @@ -109,6 +109,9 @@ impl ParamGenerator { #[test] fn general_script_no_params_works() { ExtBuilder::default().build().execute_with(|| { + // Roll to first block in case of block based event checkings and processes. + roll_to(1); + let (bob_addr_32, _) = addrs_from_ss58(BOB_ADDR).unwrap(); // no_param_at_all() @@ -116,6 +119,12 @@ fn general_script_no_params_works() { let type_args: Vec = vec![]; let params: Vec<&[u8]> = vec![]; assert_ok!(execute_script(&bob_addr_32, script, params, type_args)); + assert_eq!( + last_event(), + RuntimeEvent::MoveModule(Event::::ExecuteCalled { + who: vec![bob_addr_32] + }) + ); }) } @@ -123,6 +132,9 @@ fn general_script_no_params_works() { #[test] fn general_script_no_signers_param_at_all_works() { ExtBuilder::default().build().execute_with(|| { + // Roll to first block in case of block based event checkings and processes. + roll_to(1); + let mut pg = ParamGenerator::new(); let (bob_addr_32, _) = addrs_from_ss58(BOB_ADDR).unwrap(); @@ -140,6 +152,54 @@ fn general_script_no_signers_param_at_all_works() { let params: Vec<&[u8]> = vec![&iter, &a, &b, &c, &d, &e, &f]; assert_ok!(execute_script(&bob_addr_32, script, params, type_args)); + assert_eq!( + last_event(), + RuntimeEvent::MoveModule(Event::::ExecuteCalled { + who: vec![bob_addr_32] + }) + ); + }) +} + +/// Script with a single signer fails if executed by the wrong user. +#[test] +fn script_with_single_signer_fails_if_executed_by_the_wrong_user() { + let (bob_addr_32, bob_addr_mv) = addrs_from_ss58(BOB_ADDR).unwrap(); + let (eve_addr_32, _) = addrs_from_ss58(EVE_ADDR).unwrap(); + const BALANCE_UNUSED: Balance = 0; + + ExtBuilder::default().build().execute_with(|| { + // Roll to first block in case of block based event checkings and processes. + roll_to(1); + + // We are using this script below here, but any script with a single signer could have been used here. + // trying_with_signer_reference(_ref: &signer) + let script = + assets::read_script_from_project("signer-scripts", "trying_with_signer_reference"); + let transaction_bc = script_transaction!(script, no_type_args!(), &bob_addr_mv); + + // Eve cannot execute a script which requires signers, when Eve is not in the signer list. + let res = MoveModule::execute( + RuntimeOrigin::signed(eve_addr_32.clone()), + transaction_bc.clone(), + MAX_GAS_AMOUNT, + BALANCE_UNUSED, + ); + assert!(verify_module_error_with_msg(res, "UnexpectedUserSignature").unwrap()); + + // Only Bob can execute this script and generate the `ExecuteCalled` event. + assert_ok!(MoveModule::execute( + RuntimeOrigin::signed(bob_addr_32.clone()), + transaction_bc.clone(), + MAX_GAS_AMOUNT, + BALANCE_UNUSED, + )); + assert_eq!( + last_event(), + RuntimeEvent::MoveModule(Event::::ExecuteCalled { + who: vec![bob_addr_32] + }) + ); }) } @@ -184,6 +244,9 @@ fn general_script_eight_normal_signers_works() { #[test] fn eve_cant_execute_multisig_script_without_other_signers_works() { ExtBuilder::default().build().execute_with(|| { + // Roll to first block in case of block based event checkings and processes. + roll_to(1); + let mut pg = ParamGenerator::new(); // Eve is basically Bob here, but since Bob is pretending to be bad, we'll rename him. let (eve_addr_32, eve_addr_mv) = addrs_from_ss58(BOB_ADDR).unwrap(); @@ -199,12 +262,21 @@ fn eve_cant_execute_multisig_script_without_other_signers_works() { let extra = pg.rand::(); let params: Vec<&[u8]> = vec![&eve, &eve, &alice, &eve, &eve, &eve, &eve, &eve, &extra]; + // We don't expect `ExecuteCalled` event here. Only `SignedMultisigScript` event from Eve. assert_ok!(execute_script( &eve_addr_32, script.clone(), params.clone(), type_args.clone() )); + assert_eq!( + last_event(), + RuntimeEvent::MoveModule(Event::::SignedMultisigScript { + who: eve_addr_32.clone() + }) + ); + + // Executing twice will result in an error. let result = execute_script( &eve_addr_32, script.clone(), @@ -212,12 +284,20 @@ fn eve_cant_execute_multisig_script_without_other_signers_works() { type_args.clone(), ); assert_err!(result, Error::::UserHasAlreadySigned); + + // With both signatures, we can expect the `ExecuteCalled` event. assert_ok!(execute_script( &alice_addr_32, script.clone(), params.clone(), type_args.clone() )); + assert_eq!( + last_event(), + RuntimeEvent::MoveModule(Event::::ExecuteCalled { + who: vec![eve_addr_32, alice_addr_32] + }) + ); }) } @@ -225,6 +305,9 @@ fn eve_cant_execute_multisig_script_without_other_signers_works() { #[test] fn signer_before_all_possible_vectors_works() { ExtBuilder::default().build().execute_with(|| { + // Roll to first block in case of block based event checkings and processes. + roll_to(1); + let mut pg = ParamGenerator::new(); let (bob_addr_32, bob_addr_mv) = addrs_from_ss58(BOB_ADDR).unwrap(); @@ -251,6 +334,12 @@ fn signer_before_all_possible_vectors_works() { ]; assert_ok!(execute_script(&bob_addr_32, script, params, type_args)); + assert_eq!( + last_event(), + RuntimeEvent::MoveModule(Event::::ExecuteCalled { + who: vec![bob_addr_32] + }) + ); }) } @@ -324,8 +413,9 @@ fn multiple_signers_in_multisig_script_works() { ]) .build() .execute_with(|| { + let block_no_1 = 1; // Roll to first block in case of block based event checkings and processes. - roll_to(1); + roll_to(block_no_1); // Initialisation & Setup by developer Bob. let module = assets::read_module_from_project("multiple-signers", "Dorm"); @@ -450,6 +540,11 @@ fn multiple_signers_in_multisig_script_works() { assert_ok!(ensure_can_withdraw(&alice_addr_32, CHANGE)); assert_ok!(ensure_can_withdraw(&dave_addr_32, CHANGE)); assert_ok!(ensure_can_withdraw(&eve_addr_32, CHANGE)); + + // Make sure the cleanup chore won't complain for trying to delete the sigdata for + // an already executed multisig script in ChoreOnIdleStorage - related to TODO in the + // lib.rs. + roll_to(block_no_1 + 100000); // Using an arbitrary big number here. }) } @@ -606,6 +701,7 @@ fn insufficient_cheque_limit_aborts_the_multisig_script_works() { MAX_GAS_AMOUNT, BALANCE, )); + // One of the signers will set his cheque-limit too low to rent the apartment. The // script "rent_apartment" expects every of the signers to pay the same equal amount. let res = MoveModule::execute( @@ -614,6 +710,7 @@ fn insufficient_cheque_limit_aborts_the_multisig_script_works() { MAX_GAS_AMOUNT, BALANCE / 2, ); + // Verify that the execution will be aborted since on of the signers has a too low // cheque-limit to pay his part of the bill. assert!(verify_module_error_with_msg(res, "Aborted").unwrap());