relaxing timepoint constraint for multisig

substrate/frame/multisig/src/benchmarking.rs

@@ -210,5 +210,26 @@ benchmarks! {
 		assert!(!Multisigs::<T>::contains_key(multi_account_id, call_hash));
 	}
 
+	cancel_as_multi_without_timepoint {
+		// Signatories, need at least 2 people
+		let s in 2 .. T::MaxSignatories::get();
+		// Transaction Length, not a component
+		let z = 10_000;
+		let (mut signatories, call) = setup_multi::<T>(s, z)?;
+		let multi_account_id = Multisig::<T>::multi_account_id(&signatories, s.try_into().unwrap());
+		let caller = signatories.pop().ok_or("signatories should have len 2 or more")?;
+		let call_hash = call.using_encoded(blake2_256);
+		// Create the multi
+		let o = RawOrigin::Signed(caller.clone()).into();
+		Multisig::<T>::as_multi(o, s as u16, signatories.clone(), None, call, Weight::zero())?;
+		assert!(Multisigs::<T>::contains_key(&multi_account_id, call_hash));
+		// Whitelist caller account from further DB operations.
+		let caller_key = frame_system::Account::<T>::hashed_key_for(&caller);
+		frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
+	}: _(RawOrigin::Signed(caller), s as u16, signatories, call_hash)
+	verify {
+		assert!(!Multisigs::<T>::contains_key(multi_account_id, call_hash));
+	}
+
 	impl_benchmark_test_suite!(Multisig, crate::tests::new_test_ext(), crate::tests::Test);
 }

substrate/frame/multisig/src/lib.rs

@@ -39,6 +39,19 @@
 //!   number of signed origins.
 //! * `approve_as_multi` - Approve a call from a composite origin.
 //! * `cancel_as_multi` - Cancel a call from a composite origin.
+//! * `cancel_as_multi_without_timepoint` - Cancel a call from a composite origin without providing a timepoint.
+//!
+//! ### Security
+//!
+//! Multisig operations are stored in a DoubleKeyMap, where the keys are `call_hash`
+//! and `id` (derived from threshold and signatories).
+//! `timepoint` acts as an identifier for otherwise indistinguishable multisig operations
+//! (e.g. a recurring payment with, say, threshold 2, signatories A, B, C, and the same `call_hash`),
+//! hence granting an additional layer of security.
+//! However, retrieving and providing the `timepoint` may become an arduous task from the UX perspective.
+//! `timepoint` is made optional to balance the tradeoff between UX and security.
+//! If `threshold`, `signatories` and `call_hash` together will be enough for your use-case to distinguish
+//! multisig operations, then you can opt-out of `timepoint`.
 
 // Ensure we're `no_std` when compiling for Wasm.
 #![cfg_attr(not(feature = "std"), no_std)]
@@ -206,8 +219,6 @@ pub mod pallet {
 		NotFound,
 		/// Only the account that originally created the multisig is able to cancel it.
 		NotOwner,
-		/// No timepoint was given, yet the multisig operation is already underway.
-		NoTimepoint,
 		/// A different timepoint was given to the multisig operation that is underway.
 		WrongTimepoint,
 		/// A timepoint was given, yet no multisig operation is underway.
@@ -226,22 +237,22 @@ pub mod pallet {
 		/// A multisig operation has been approved by someone.
 		MultisigApproval {
 			approving: T::AccountId,
-			timepoint: Timepoint<BlockNumberFor<T>>,
+			timepoint: Option<Timepoint<BlockNumberFor<T>>>,
 			multisig: T::AccountId,
 			call_hash: CallHash,
 		},
 		/// A multisig operation has been executed.
 		MultisigExecuted {
 			approving: T::AccountId,
-			timepoint: Timepoint<BlockNumberFor<T>>,
+			timepoint: Option<Timepoint<BlockNumberFor<T>>>,
 			multisig: T::AccountId,
 			call_hash: CallHash,
 			result: DispatchResult,
 		},
 		/// A multisig operation has been cancelled.
 		MultisigCancelled {
 			cancelling: T::AccountId,
-			timepoint: Timepoint<BlockNumberFor<T>>,
+			timepoint: Option<Timepoint<BlockNumberFor<T>>>,
 			multisig: T::AccountId,
 			call_hash: CallHash,
 		},
@@ -328,8 +339,9 @@ pub mod pallet {
 		/// - `other_signatories`: The accounts (other than the sender) who can approve this
 		/// dispatch. May not be empty.
 		/// - `maybe_timepoint`: If this is the first approval, then this must be `None`. If it is
-		/// not the first approval, then it must be `Some`, with the timepoint (block number and
-		/// transaction index) of the first approval transaction.
+		/// not the first approval, then it can be `Some`, with the timepoint (block number and
+		/// transaction index) of the first approval transaction. When provided, timepoint will serve
+		/// as an additional layer of security.
 		/// - `call`: The call to be executed.
 		///
 		/// NOTE: Unless this is the final approval, you will generally want to use
@@ -394,8 +406,9 @@ pub mod pallet {
 		/// - `other_signatories`: The accounts (other than the sender) who can approve this
 		/// dispatch. May not be empty.
 		/// - `maybe_timepoint`: If this is the first approval, then this must be `None`. If it is
-		/// not the first approval, then it must be `Some`, with the timepoint (block number and
-		/// transaction index) of the first approval transaction.
+		/// not the first approval, then it can be `Some`, with the timepoint (block number and
+		/// transaction index) of the first approval transaction. When provided, timepoint will serve
+		/// as an additional layer of security.
 		/// - `call_hash`: The hash of the call to be executed.
 		///
 		/// NOTE: If this is the final approval, you will want to use `as_multi` instead.
@@ -446,8 +459,11 @@ pub mod pallet {
 		/// - `threshold`: The total number of approvals for this dispatch before it is executed.
 		/// - `other_signatories`: The accounts (other than the sender) who can approve this
 		/// dispatch. May not be empty.
-		/// - `timepoint`: The timepoint (block number and transaction index) of the first approval
-		/// transaction for this dispatch.
+		/// - `timepoint`: The timepoint (block number and transaction index) of the first approval.
+		/// Timepoint serves as an additional layer of security. If you do not wish to provide timepoint,
+		/// use `cancel_as_multi_without_timepoint`
+		/// (`timepoint` parameter is not an `Option`, but instead there is another function
+		/// `cancel_as_multi_without_timepoint` due to backwards compatibility).
 		/// - `call_hash`: The hash of the call to be executed.
 		///
 		/// ## Complexity
@@ -469,29 +485,23 @@ pub mod pallet {
 			call_hash: [u8; 32],
 		) -> DispatchResult {
 			let who = ensure_signed(origin)?;
-			ensure!(threshold >= 2, Error::<T>::MinimumThreshold);
-			let max_sigs = T::MaxSignatories::get() as usize;
-			ensure!(!other_signatories.is_empty(), Error::<T>::TooFewSignatories);
-			ensure!(other_signatories.len() < max_sigs, Error::<T>::TooManySignatories);
-			let signatories = Self::ensure_sorted_and_insert(other_signatories, who.clone())?;
-
-			let id = Self::multi_account_id(&signatories, threshold);
-
-			let m = <Multisigs<T>>::get(&id, call_hash).ok_or(Error::<T>::NotFound)?;
-			ensure!(m.when == timepoint, Error::<T>::WrongTimepoint);
-			ensure!(m.depositor == who, Error::<T>::NotOwner);
-
-			let err_amount = T::Currency::unreserve(&m.depositor, m.deposit);
-			debug_assert!(err_amount.is_zero());
-			<Multisigs<T>>::remove(&id, &call_hash);
+			Self::cancel(who, threshold, other_signatories, Some(timepoint), call_hash)
+		}
 
-			Self::deposit_event(Event::MultisigCancelled {
-				cancelling: who,
-				timepoint,
-				multisig: id,
-				call_hash,
-			});
-			Ok(())
+		/// Same as [`Pallet::cancel_as_multi`], but without a timepoint.
+		///
+		/// If `threshold`, `signatories` and `call_hash` together will be enough for your use-case
+		/// to distinguish multisig operations, use this function instead of `cancel_as_multi`
+		#[pallet::call_index(4)]
+		#[pallet::weight(T::WeightInfo::cancel_as_multi(other_signatories.len() as u32))]
+		pub fn cancel_as_multi_without_timepoint(
+			origin: OriginFor<T>,
+			threshold: u16,
+			other_signatories: Vec<T::AccountId>,
+			call_hash: [u8; 32],
+		) -> DispatchResult {
+			let who = ensure_signed(origin)?;
+			Self::cancel(who, threshold, other_signatories, None, call_hash)
 		}
 	}
 }
@@ -507,6 +517,41 @@ impl<T: Config> Pallet<T> {
 			.expect("infinite length input; no invalid inputs for type; qed")
 	}
 
+	fn cancel(
+		who: T::AccountId,
+		threshold: u16,
+		other_signatories: Vec<T::AccountId>,
+		maybe_timepoint: Option<Timepoint<BlockNumberFor<T>>>,
+		call_hash: [u8; 32],
+	) -> DispatchResult {
+		ensure!(threshold >= 2, Error::<T>::MinimumThreshold);
+		let max_sigs = T::MaxSignatories::get() as usize;
+		ensure!(!other_signatories.is_empty(), Error::<T>::TooFewSignatories);
+		ensure!(other_signatories.len() < max_sigs, Error::<T>::TooManySignatories);
+		let signatories = Self::ensure_sorted_and_insert(other_signatories, who.clone())?;
+
+		let id = Self::multi_account_id(&signatories, threshold);
+
+		let m = <Multisigs<T>>::get(&id, call_hash).ok_or(Error::<T>::NotFound)?;
+		if let Some(timepoint) = maybe_timepoint.as_ref() {
+			// if the optional timepoint is supplied, it should be the correct one
+			ensure!(m.when == *timepoint, Error::<T>::WrongTimepoint);
+		}
+		ensure!(m.depositor == who, Error::<T>::NotOwner);
+
+		let err_amount = T::Currency::unreserve(&m.depositor, m.deposit);
+		debug_assert!(err_amount.is_zero());
+		<Multisigs<T>>::remove(&id, &call_hash);
+
+		Self::deposit_event(Event::MultisigCancelled {
+			cancelling: who,
+			timepoint: maybe_timepoint,
+			multisig: id,
+			call_hash,
+		});
+		Ok(())
+	}
+
 	fn operate(
 		who: T::AccountId,
 		threshold: u16,
@@ -535,9 +580,10 @@ impl<T: Config> Pallet<T> {
 
 		// Branch on whether the operation has already started or not.
 		if let Some(mut m) = <Multisigs<T>>::get(&id, call_hash) {
-			// Yes; ensure that the timepoint exists and agrees.
-			let timepoint = maybe_timepoint.ok_or(Error::<T>::NoTimepoint)?;
-			ensure!(m.when == timepoint, Error::<T>::WrongTimepoint);
+			if let Some(timepoint) = maybe_timepoint.as_ref() {
+				// if the optional timepoint is supplied, it should be the correct one
+				ensure!(m.when == *timepoint, Error::<T>::WrongTimepoint);
+			}
 
 			// Ensure that either we have not yet signed or that it is at threshold.
 			let mut approvals = m.approvals.len() as u16;
@@ -564,7 +610,7 @@ impl<T: Config> Pallet<T> {
 				let result = call.dispatch(RawOrigin::Signed(id.clone()).into());
 				Self::deposit_event(Event::MultisigExecuted {
 					approving: who,
-					timepoint,
+					timepoint: maybe_timepoint,
 					multisig: id,
 					call_hash,
 					result: result.map(|_| ()).map_err(|e| e.error),
@@ -590,7 +636,7 @@ impl<T: Config> Pallet<T> {
 					<Multisigs<T>>::insert(&id, call_hash, m);
 					Self::deposit_event(Event::MultisigApproval {
 						approving: who,
-						timepoint,
+						timepoint: maybe_timepoint,
 						multisig: id,
 						call_hash,
 					});

substrate/frame/multisig/src/tests.rs

@@ -168,6 +168,7 @@ fn timepoint_checking_works() {
 		assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(3), multi, 5));
 
 		let call = call_transfer(6, 15);
+		let call_weight = call.get_dispatch_info().weight;
 		let hash = blake2_256(&call.encode());
 
 		assert_noop!(
@@ -191,29 +192,29 @@ fn timepoint_checking_works() {
 			Weight::zero()
 		));
 
-		assert_noop!(
-			Multisig::as_multi(
-				RuntimeOrigin::signed(2),
-				2,
-				vec![1, 3],
-				None,
-				call.clone(),
-				Weight::zero()
-			),
-			Error::<Test>::NoTimepoint,
-		);
 		let later = Timepoint { index: 1, ..now() };
 		assert_noop!(
 			Multisig::as_multi(
 				RuntimeOrigin::signed(2),
 				2,
 				vec![1, 3],
 				Some(later),
-				call,
+				call.clone(),
 				Weight::zero()
 			),
 			Error::<Test>::WrongTimepoint,
 		);
+
+		assert_ok!(Multisig::as_multi(
+			RuntimeOrigin::signed(2),
+			2,
+			vec![1, 3],
+			None,
+			call,
+			call_weight
+		));
+
+		assert_eq!(Balances::free_balance(6), 15);
 	});
 }
 
@@ -250,6 +251,47 @@ fn multisig_2_of_3_works() {
 	});
 }
 
+#[test]
+fn multisig_3_of_3_without_timepoint_works() {
+	new_test_ext().execute_with(|| {
+		let multi = Multisig::multi_account_id(&[1, 2, 3][..], 3);
+		assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5));
+		assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5));
+		assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(3), multi, 5));
+
+		let call = call_transfer(6, 15);
+		let call_weight = call.get_dispatch_info().weight;
+		let hash = blake2_256(&call.encode());
+		assert_ok!(Multisig::approve_as_multi(
+			RuntimeOrigin::signed(1),
+			3,
+			vec![2, 3],
+			None,
+			hash,
+			Weight::zero()
+		));
+		assert_ok!(Multisig::approve_as_multi(
+			RuntimeOrigin::signed(3),
+			3,
+			vec![1, 2],
+			None,
+			hash,
+			Weight::zero()
+		));
+		assert_eq!(Balances::free_balance(6), 0);
+
+		assert_ok!(Multisig::as_multi(
+			RuntimeOrigin::signed(2),
+			3,
+			vec![1, 3],
+			None,
+			call,
+			call_weight
+		));
+		assert_eq!(Balances::free_balance(6), 15);
+	});
+}
+
 #[test]
 fn multisig_3_of_3_works() {
 	new_test_ext().execute_with(|| {
@@ -316,7 +358,46 @@ fn cancel_multisig_works() {
 			Multisig::cancel_as_multi(RuntimeOrigin::signed(2), 3, vec![1, 3], now(), hash),
 			Error::<Test>::NotOwner,
 		);
-		assert_ok!(Multisig::cancel_as_multi(RuntimeOrigin::signed(1), 3, vec![2, 3], now(), hash),);
+		assert_ok!(Multisig::cancel_as_multi(RuntimeOrigin::signed(1), 3, vec![2, 3], now(), hash));
+	});
+}
+
+#[test]
+fn cancel_multisig_without_timepoint_works() {
+	new_test_ext().execute_with(|| {
+		let call = call_transfer(6, 15).encode();
+		let hash = blake2_256(&call);
+		assert_ok!(Multisig::approve_as_multi(
+			RuntimeOrigin::signed(1),
+			3,
+			vec![2, 3],
+			None,
+			hash,
+			Weight::zero()
+		));
+		assert_ok!(Multisig::approve_as_multi(
+			RuntimeOrigin::signed(2),
+			3,
+			vec![1, 3],
+			None,
+			hash,
+			Weight::zero()
+		));
+		assert_noop!(
+			Multisig::cancel_as_multi_without_timepoint(
+				RuntimeOrigin::signed(2),
+				3,
+				vec![1, 3],
+				hash
+			),
+			Error::<Test>::NotOwner,
+		);
+		assert_ok!(Multisig::cancel_as_multi_without_timepoint(
+			RuntimeOrigin::signed(1),
+			3,
+			vec![2, 3],
+			hash
+		));
 	});
 }
 
@@ -452,7 +533,7 @@ fn multisig_2_of_3_cannot_reissue_same_call() {
 		System::assert_last_event(
 			pallet_multisig::Event::MultisigExecuted {
 				approving: 3,
-				timepoint: now(),
+				timepoint: Some(now()),
 				multisig: multi,
 				call_hash: hash,
 				result: Err(TokenError::FundsUnavailable.into()),