prevent nested DelayedOrigin (#845)

* prevent nested DelayedOrigin * fix deps * Update authority/src/tests.rs Co-authored-by: zjb0807 <[email protected]> * allow nested DelayedOrigin * update docstring * typo fix Co-authored-by: zjb0807 <[email protected]>
open-web3-stack · Nov 22, 2022 · e885d8d · e885d8d
1 parent 7f95607
commit e885d8d
Show file tree

Hide file tree

Showing 3 changed files with 76 additions and 9 deletions.
diff --git a/authority/Cargo.toml b/authority/Cargo.toml
@@ -12,15 +12,15 @@ scale-info = { version = "2.1.2", default-features = false, features = ["derive"
 serde = { version = "1.0.145", optional = true }
 codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false }
 
-sp-runtime = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" }
+sp-core = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" }
 sp-io = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" }
+sp-runtime = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" }
 sp-std = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" }
 frame-support = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" }
 frame-system = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.31" }
 
 [dev-dependencies]
 sp-io = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.31" }
-sp-core = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.31" }
 pallet-scheduler = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.31" }
 pallet-preimage = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.31" }
 

diff --git a/authority/src/lib.rs b/authority/src/lib.rs
@@ -11,6 +11,13 @@
 //! Two functionalities are provided by this module:
 //! - schedule a dispatchable
 //! - dispatch method with on behalf of other origins
+//!
+//! NOTE:
+//!
+//! In order to derive a feasible max encoded len for `DelayedOrigin`, it is
+//! assumed that there are no nested `DelayedOrigin` in `OriginCaller`.
+//! In practice, this means there should not be nested `schedule_dispatch`.
+//! Otherwise the proof size estimation may not be accurate.
 
 #![cfg_attr(not(feature = "std"), no_std)]
 // Disable the following three lints since they originate from an external macro
@@ -32,6 +39,7 @@ use frame_support::{
 };
 use frame_system::{pallet_prelude::*, EnsureRoot, EnsureSigned};
 use scale_info::TypeInfo;
+use sp_core::defer;
 use sp_runtime::{
 	traits::{CheckedSub, Dispatchable, Hash, Saturating},
 	ArithmeticError, DispatchError, DispatchResult, Either, RuntimeDebug,
@@ -45,12 +53,64 @@ mod weights;
 pub use weights::WeightInfo;
 
 /// A delayed origin. Can only be dispatched via `dispatch_as` with a delay.
-#[derive(PartialEq, Eq, Clone, RuntimeDebug, Encode, Decode, TypeInfo, MaxEncodedLen)]
+#[derive(PartialEq, Eq, Clone, RuntimeDebug, Encode, Decode, TypeInfo)]
 pub struct DelayedOrigin<BlockNumber, PalletsOrigin> {
 	/// Number of blocks that this call have been delayed.
-	pub delay: BlockNumber,
+	pub(crate) delay: BlockNumber,
 	/// The initial origin.
-	pub origin: Box<PalletsOrigin>,
+	pub(crate) origin: Box<PalletsOrigin>,
+}
+
+#[cfg(feature = "std")]
+mod helper {
+	use std::cell::RefCell;
+
+	thread_local! {
+		static NESTED_MAX_ENCODED_LEN: RefCell<bool> = RefCell::new(false);
+	}
+
+	pub fn set_nested_max_encoded_len(val: bool) {
+		NESTED_MAX_ENCODED_LEN.with(|v| *v.borrow_mut() = val);
+	}
+
+	pub fn nested_max_encoded_len() -> bool {
+		NESTED_MAX_ENCODED_LEN.with(|v| *v.borrow())
+	}
+}
+
+#[cfg(not(feature = "std"))]
+mod helper {
+	static mut NESTED_MAX_ENCODED_LEN: bool = false;
+
+	pub fn set_nested_max_encoded_len(val: bool) {
+		unsafe {
+			NESTED_MAX_ENCODED_LEN = val;
+		}
+	}
+
+	pub fn nested_max_encoded_len() -> bool {
+		unsafe { NESTED_MAX_ENCODED_LEN }
+	}
+}
+
+// Manual implementation to break recursive calls of `MaxEncodedLen` as the
+// implementation of `PalletsOrigin::max_encoded_len` will also call
+// `MaxEncodedLen` on `DelayedOrigin`. This is only safe if there are no nested
+// `DelayedOrigin`. It is only possible to construct a `DelayedOrigin` via
+// `schedule_dispatch` which is a protected call only accessible via governance.
+impl<BlockNumber: MaxEncodedLen, PalletsOrigin: MaxEncodedLen> MaxEncodedLen
+	for DelayedOrigin<BlockNumber, PalletsOrigin>
+{
+	fn max_encoded_len() -> usize {
+		if helper::nested_max_encoded_len() {
+			return 0;
+		}
+
+		helper::set_nested_max_encoded_len(true);
+		defer!(helper::set_nested_max_encoded_len(false));
+
+		BlockNumber::max_encoded_len() + PalletsOrigin::max_encoded_len()
+	}
 }
 
 /// Ensure the origin have a minimum amount of delay.
@@ -99,10 +159,10 @@ pub trait AuthorityConfig<Origin, PalletsOrigin, BlockNumber> {
 		new_delay: BlockNumber,
 	) -> DispatchResult;
 	/// Check if the `origin` is allow to delay a scheduled task that
-	/// initially created by `inital_origin`.
+	/// initially created by `initial_origin`.
 	fn check_delay_schedule(origin: Origin, initial_origin: &PalletsOrigin) -> DispatchResult;
 	/// Check if the `origin` is allow to cancel a scheduled task that
-	/// initially created by `inital_origin`.
+	/// initially created by `initial_origin`.
 	fn check_cancel_schedule(origin: Origin, initial_origin: &PalletsOrigin) -> DispatchResult;
 }
 
@@ -396,12 +456,12 @@ pub mod module {
 
 		#[pallet::weight(T::WeightInfo::remove_authorized_call())]
 		pub fn remove_authorized_call(origin: OriginFor<T>, hash: T::Hash) -> DispatchResult {
-			let root_or_sigend =
+			let root_or_signed =
 				EitherOfDiverse::<EnsureRoot<T::AccountId>, EnsureSigned<T::AccountId>>::ensure_origin(origin)?;
 
 			SavedCalls::<T>::try_mutate_exists(hash, |maybe_call| {
 				let (_, maybe_caller) = maybe_call.take().ok_or(Error::<T>::CallNotAuthorized)?;
-				match root_or_sigend {
+				match root_or_signed {
 					Either::Left(_) => {} // root, do nothing
 					Either::Right(who) => {
 						// signed, ensure it's the caller

diff --git a/authority/src/tests.rs b/authority/src/tests.rs
@@ -3,6 +3,7 @@
 #![cfg(test)]
 
 use super::*;
+use codec::MaxEncodedLen;
 use frame_support::{
 	assert_noop, assert_ok,
 	dispatch::DispatchErrorWithPostInfo,
@@ -691,3 +692,9 @@ fn trigger_old_call_should_be_free_and_operational() {
 		);
 	});
 }
+
+#[test]
+fn origin_max_encoded_len_works() {
+	assert_eq!(DelayedOrigin::<u32, OriginCaller>::max_encoded_len(), 22);
+	assert_eq!(OriginCaller::max_encoded_len(), 27);
+}