diff --git a/pallet/src/balance.rs b/pallet/src/balance.rs index e5c3e38..e5461af 100644 --- a/pallet/src/balance.rs +++ b/pallet/src/balance.rs @@ -14,9 +14,7 @@ use move_vm_backend::balance::BalanceHandler; use sp_runtime::traits::Zero; use sp_std::{ cell::{Ref, RefCell}, - default::Default, rc::Rc, - vec::Vec, }; use crate::{Config, Error, Pallet}; diff --git a/pallet/src/lib.rs b/pallet/src/lib.rs index 1bcb051..ee9ebc4 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; @@ -252,45 +253,65 @@ 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( + MultisigOf::::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); + // Everything below this block, applies to multisig scenario only. + let Some(script_hash) = contains_multisig else { + // THIS PART SHOULD NOT BE REACHABLE - WE CAN RECONSIDER IT + // If the solely required signer hasn't signed the script, reject the script entirely. + return Err(Error::::ScriptSignatureMissing.into()); + }; + + let mut multisig = 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 multisig.stored_block_height().is_none() { + let block_height = >::block_number(); + + multisig.set_block_height(block_height); + Self::new_multi_sign_request_chore(block_height, script_hash)?; + } + + MultisigStorage::::insert(script_hash, multisig); + 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: Clear the ChoreOnIdleStorage as well here maybe? + MultisigStorage::::remove(script_hash); } // We need to provide MoveVM read only access to balance sheet - MoveVM is allowed to @@ -308,7 +329,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 +543,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 +598,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 +650,10 @@ 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, + /// Proper signature missing. + ScriptSignatureMissing, /// 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..321f43f 100644 --- a/pallet/src/signer.rs +++ b/pallet/src/signer.rs @@ -10,12 +10,11 @@ use frame_support::{ }, Get, }, - BoundedBTreeMap, Parameter, + BoundedBTreeMap, BoundedBTreeSet, Parameter, }; use frame_system::{pallet_prelude::BlockNumberFor, Config as SysConfig}; use scale_info::TypeInfo; use sp_runtime::traits::MaybeSerializeDeserialize; -use sp_std::vec::Vec; use crate::{ balance::{AccountIdOf, BalanceAdapter, BalanceOf}, @@ -80,8 +79,11 @@ 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 `Multisig` was initally stored. + /// + /// Used only for multisigure purposes. + stored_block_height: Option, } impl Multisig @@ -92,13 +94,13 @@ where Size: Get, { /// Creates a new [`Multisig`] with all blank signatures for the provided script. - pub fn new(signers: Vec, block_height: BlockNumber) -> Result { + pub fn new(signers: BoundedBTreeSet) -> Result { if signers.len() > (Size::get() as usize) { return Err(MultisigError::MaxSignersExceeded); } let mut multisig_info = Multisig:: { - block_height, + stored_block_height: None, ..Default::default() }; for account in signers.iter() { @@ -110,8 +112,12 @@ where Ok(multisig_info) } - pub fn block_number(&self) -> &BlockNumber { - &self.block_height + pub fn stored_block_height(&self) -> Option<&BlockNumber> { + self.stored_block_height.as_ref() + } + + pub fn set_block_height(&mut self, block_height: BlockNumber) { + self.stored_block_height = Some(block_height); } } @@ -155,7 +161,7 @@ where fn default() -> Self { Multisig { signers: BoundedBTreeMap::, Size>::new(), - block_height: BlockNumber::default(), + stored_block_height: None, } } } @@ -170,13 +176,11 @@ pub(crate) struct ScriptSignatureHandler { 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)?, + multisig_info: MultisigOf::::new(accounts).map_err(Into::>::into)?, }) } @@ -193,19 +197,21 @@ 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.multisig_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. diff --git a/pallet/src/tests/signer.rs b/pallet/src/tests/signer.rs index 40aadbd..bd12821 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] + }) + ); }) } @@ -606,6 +695,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 +704,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());