Harden FRAME primitives against audit findings

frame/executive/Cargo.toml

@@ -21,6 +21,7 @@ frame-support.workspace = true
 frame-system.workspace = true
 frame-try-runtime = { optional = true, workspace = true }
 log.workspace = true
+qp-header.workspace = true
 scale-info = { features = ["derive"], workspace = true }
 sp-core.workspace = true
 sp-io.workspace = true
@@ -31,7 +32,6 @@ sp-tracing.workspace = true
 array-bytes = { workspace = true, default-features = true }
 pallet-balances = { workspace = true, default-features = true }
 pallet-transaction-payment = { workspace = true, default-features = true }
-qp-header = { workspace = true, default-features = true }
 sp-inherents = { workspace = true, default-features = true }
 sp-version = { workspace = true, default-features = true }
 
@@ -44,6 +44,7 @@ std = [
 	"frame-system/std",
 	"frame-try-runtime/std",
 	"log/std",
+	"qp-header/std",
 	"scale-info/std",
 	"sp-core/std",
 	"sp-io/std",

frame/executive/src/lib.rs

@@ -131,6 +131,7 @@ use frame_support::{
 	MAX_EXTRINSIC_DEPTH,
 };
 use frame_system::pallet_prelude::BlockNumberFor;
+use qp_header::ZkTreeRootProvider;
 use sp_runtime::{
 	generic::Digest,
 	traits::{
@@ -396,6 +397,15 @@ where
 					header.state_root() == storage_root,
 					"Storage root must match that calculated."
 				);
+
+				// The ZK tree root is derived from storage, so it is only expected to match
+				// when the state itself is expected to match.
+				let zk_tree_root = new_header.zk_tree_root();
+				header.zk_tree_root().check_equal(zk_tree_root);
+				assert!(
+					header.zk_tree_root() == zk_tree_root,
+					"ZK tree root must match that calculated."
+				);
 			}
 
 			assert!(
@@ -936,6 +946,13 @@ where
 			header.extrinsics_root() == new_header.extrinsics_root(),
 			"Transaction trie root must be valid.",
 		);
+
+		// check ZK tree root. This field is part of the block-hash preimage, so it must be
+		// re-derived from state on import; otherwise a block author could commit an arbitrary
+		// root into the header.
+		let zk_tree_root = new_header.zk_tree_root();
+		header.zk_tree_root().check_equal(zk_tree_root);
+		assert!(header.zk_tree_root() == zk_tree_root, "ZK tree root must match that calculated.",);
 	}
 
 	/// Check a given signed transaction for validity. This doesn't execute any

frame/executive/src/tests.rs

@@ -708,6 +708,28 @@ fn block_import_of_bad_state_root_fails() {
 	});
 }
 
+#[test]
+#[should_panic(expected = "ZK tree root must match that calculated.")]
+fn block_import_of_bad_zk_tree_root_fails() {
+	new_test_ext(1).execute_with(|| {
+		let mut header = Header::new(
+			1,
+			array_bytes::hex_n_into_unchecked(
+				"03170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c111314",
+			),
+			array_bytes::hex_n_into_unchecked(
+				"d6b465f5a50c9f8d5a6edc0f01d285a6b19030f097d3aaf1649b7be81649f118",
+			),
+			[69u8; 32].into(),
+			Digest { logs: vec![] },
+		);
+		// Forge a ZK tree root that does not match on-chain state (which is the default zero
+		// root). Before the `final_checks` fix this block would import successfully.
+		header.set_zk_tree_root([42u8; 32].into());
+		Executive::execute_block(TestBlock { header, extrinsics: vec![] }.into());
+	});
+}
+
 #[test]
 #[should_panic]
 fn block_import_of_bad_extrinsic_root_fails() {

frame/support-procedural/src/dynamic_params.rs

@@ -149,6 +149,9 @@ fn ensure_codec_index(attrs: &Vec<syn::Attribute>, span: Span) -> Result<()> {
 ///
 /// This allows the outer `#[dynamic_params(..)]` attribute to specify some arguments that don't
 /// need to be repeated every time.
+///
+/// Arguments already present on the inner attribute take precedence and are left untouched;
+/// the outer arguments are only injected when the inner attribute has none.
 struct MacroInjectArgs {
 	runtime_params: syn::Ident,
 	params_pallet: syn::Type,
@@ -159,20 +162,24 @@ impl VisitMut for MacroInjectArgs {
 		let attr = item.attrs.iter_mut().find(|attr| attr.path().is_ident("dynamic_pallet_params"));
 
 		if let Some(attr) = attr {
-			match &attr.meta {
-				syn::Meta::Path(path) =>
-					assert_eq!(path.to_token_stream().to_string(), "dynamic_pallet_params"),
-				_ => (),
+			// Only inject the outer arguments if the inner attribute does not specify its own:
+			// either a bare `#[dynamic_pallet_params]` or an empty `#[dynamic_pallet_params()]`.
+			let has_own_args = match &attr.meta {
+				syn::Meta::Path(_) => false,
+				syn::Meta::List(list) => !list.tokens.is_empty(),
+				syn::Meta::NameValue(_) => true,
+			};
+
+			if !has_own_args {
+				let runtime_params = &self.runtime_params;
+				let params_pallet = &self.params_pallet;
+
+				attr.meta = syn::parse2::<syn::Meta>(quote! {
+					dynamic_pallet_params(#runtime_params, #params_pallet)
+				})
+				.unwrap()
+				.into();
 			}
-
-			let runtime_params = &self.runtime_params;
-			let params_pallet = &self.params_pallet;
-
-			attr.meta = syn::parse2::<syn::Meta>(quote! {
-				dynamic_pallet_params(#runtime_params, #params_pallet)
-			})
-			.unwrap()
-			.into();
 		}
 
 		visit_mut::visit_item_mod_mut(self, item);
@@ -570,3 +577,94 @@ impl ToTokens for DynamicParamAggregatedEnum {
 fn crate_access() -> core::result::Result<syn::Path, TokenStream> {
 	generate_access_from_frame_or_crate("frame-support").map_err(|e| e.to_compile_error())
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	fn inject(module: TokenStream) -> syn::ItemMod {
+		let mut params_mod: syn::ItemMod = parse2(module).unwrap();
+		MacroInjectArgs {
+			runtime_params: syn::Ident::new("RuntimeParameters", Span::call_site()),
+			params_pallet: parse2(quote!(dynamic_params::pallet_parameters::Parameters::<Runtime>))
+				.unwrap(),
+		}
+		.visit_item_mod_mut(&mut params_mod);
+		params_mod
+	}
+
+	fn inner_attr(params_mod: &syn::ItemMod) -> &syn::Attribute {
+		let (_, items) = params_mod.content.as_ref().unwrap();
+		items
+			.iter()
+			.find_map(|i| match i {
+				syn::Item::Mod(m) =>
+					m.attrs.iter().find(|attr| attr.path().is_ident("dynamic_pallet_params")),
+				_ => None,
+			})
+			.unwrap()
+	}
+
+	#[test]
+	fn outer_args_are_injected_into_bare_inner_attribute() {
+		let params_mod = inject(quote! {
+			pub mod dynamic_params {
+				#[dynamic_pallet_params]
+				#[codec(index = 0)]
+				pub mod inner {}
+			}
+		});
+
+		let attr = inner_attr(&params_mod);
+		assert_eq!(
+			attr.meta.to_token_stream().to_string(),
+			quote!(dynamic_pallet_params(
+				RuntimeParameters,
+				dynamic_params::pallet_parameters::Parameters::<Runtime>
+			))
+			.to_string(),
+		);
+	}
+
+	#[test]
+	fn outer_args_are_injected_into_empty_inner_attribute() {
+		let params_mod = inject(quote! {
+			pub mod dynamic_params {
+				#[dynamic_pallet_params()]
+				#[codec(index = 0)]
+				pub mod inner {}
+			}
+		});
+
+		let attr = inner_attr(&params_mod);
+		assert_eq!(
+			attr.meta.to_token_stream().to_string(),
+			quote!(dynamic_pallet_params(
+				RuntimeParameters,
+				dynamic_params::pallet_parameters::Parameters::<Runtime>
+			))
+			.to_string(),
+		);
+	}
+
+	#[test]
+	fn inner_attribute_args_take_precedence_over_outer() {
+		let params_mod = inject(quote! {
+			pub mod dynamic_params {
+				#[dynamic_pallet_params(InnerRuntimeParameters, pallet_other::Parameters::<Runtime>)]
+				#[codec(index = 0)]
+				pub mod inner {}
+			}
+		});
+
+		let attr = inner_attr(&params_mod);
+		assert_eq!(
+			attr.meta.to_token_stream().to_string(),
+			quote!(dynamic_pallet_params(
+				InnerRuntimeParameters,
+				pallet_other::Parameters::<Runtime>
+			))
+			.to_string(),
+		);
+	}
+}