fix: metadata extrinsic front-run attack, rework benchmarks (#927)

Author: AmbientTea

demo/runtime/src/lib.rs

@@ -509,26 +509,35 @@ impl pallet_block_production_log::benchmarking::BenchmarkHelper<BlockAuthor>
 pub struct PalletBlockProducerMetadataBenchmarkHelper;
 
 #[cfg(feature = "runtime-benchmarks")]
-impl pallet_block_producer_metadata::benchmarking::BenchmarkHelper<BlockProducerMetadataType>
-	for PalletBlockProducerMetadataBenchmarkHelper
+impl
+	pallet_block_producer_metadata::benchmarking::BenchmarkHelper<
+		BlockProducerMetadataType,
+		AccountId,
+	> for PalletBlockProducerMetadataBenchmarkHelper
 {
+	fn genesis_utxo() -> UtxoId {
+		Sidechain::genesis_utxo()
+	}
+
 	fn metadata() -> BlockProducerMetadataType {
 		BlockProducerMetadataType {
 			url: "https://cool.stuff/spo.json".try_into().unwrap(),
 			hash: SizedByteString::from([0; 32]),
 		}
 	}
+
 	fn cross_chain_pub_key() -> sidechain_domain::CrossChainPublicKey {
 		sidechain_domain::CrossChainPublicKey(
 			hex_literal::hex!("020a1091341fe5664bfa1782d5e04779689068c916b04cb365ec3153755684d9a1")
 				.to_vec(),
 		)
 	}
-	fn upsert_cross_chain_signature() -> sidechain_domain::CrossChainSignature {
-		sidechain_domain::CrossChainSignature(hex_literal::hex!("0892ab398baf72fb90c7e90147caea9b5aa642f082e8573877aba34aeb618c7e3c4a904ef5c12bdb6560d665a71584ea266279ba686ff7e4f886ca75e357fdff").to_vec())
-	}
-	fn delete_cross_chain_signature() -> sidechain_domain::CrossChainSignature {
-		sidechain_domain::CrossChainSignature(hex_literal::hex!("e891b42327fc5202f258b080d0a3f33d9a292693840dee5fa5e46033fe0b059b682597be65ac5678c182a65b46b621aaadcfb0811155f54e8d99c9e4394a1fe9").to_vec())
+
+	fn cross_chain_sign_key() -> pallet_block_producer_metadata::benchmarking::SecretKey {
+		pallet_block_producer_metadata::benchmarking::SecretKey::from_slice(&hex_literal::hex!(
+			"cb6df9de1efca7a3998a8ead4e02159d5fa99c3e0d4fd6432667390bb4726854"
+		))
+		.unwrap()
 	}
 
 	fn upsert_valid_before() -> u64 {

e2e-tests/src/partner_chains_node/node.py

@@ -41,13 +41,13 @@ def sign_address_association(self, genesis_utxo: str, partner_chain_address, sta
             logging.error(f"Could not parse response of sign-address-association cmd: {result}")
             raise e
 
-    def sign_block_producer_metadata_upsert(self, genesis_utxo, metadata_file, cross_chain_signing_key):
-        return self.sign_block_producer_metadata_operation(genesis_utxo, metadata_file, cross_chain_signing_key)
+    def sign_block_producer_metadata_upsert(self, genesis_utxo, metadata_file, cross_chain_signing_key, partner_chain_account):
+        return self.sign_block_producer_metadata_operation(genesis_utxo, metadata_file, cross_chain_signing_key, partner_chain_account)
 
-    def sign_block_producer_metadata_delete(self, genesis_utxo, cross_chain_signing_key):
-        return self.sign_block_producer_metadata_operation(genesis_utxo, None, cross_chain_signing_key)
+    def sign_block_producer_metadata_delete(self, genesis_utxo, cross_chain_signing_key, partner_chain_account):
+        return self.sign_block_producer_metadata_operation(genesis_utxo, None, cross_chain_signing_key, partner_chain_account)
 
-    def sign_block_producer_metadata_operation(self, genesis_utxo, metadata_file, cross_chain_signing_key):
+    def sign_block_producer_metadata_operation(self, genesis_utxo, metadata_file, cross_chain_signing_key, partner_chain_account):
         cross_chain_signing_key = cross_chain_signing_key.to_string().hex()
 
         sign_block_producer_metadata_cmd = " ".join([
@@ -56,7 +56,8 @@ def sign_block_producer_metadata_operation(self, genesis_utxo, metadata_file, cr
                 (f"upsert" if metadata_file is not None else "delete"),
                 f"--genesis-utxo {genesis_utxo}",
                 (f"--metadata-file {metadata_file}" if metadata_file is not None else ""),
-                f"--cross-chain-signing-key {cross_chain_signing_key}"
+                f"--cross-chain-signing-key {cross_chain_signing_key}",
+                f"--partner-chain-account {partner_chain_account}"
              ])
 
         result = self.run_command.exec(sign_block_producer_metadata_cmd)

e2e-tests/src/substrate_api.py

@@ -685,14 +685,14 @@ def _effective_in_mc_epoch(self):
     def sign_address_association(self, genesis_utxo, address, stake_signing_key):
         return self.partner_chains_node.sign_address_association(genesis_utxo, address, stake_signing_key)
 
-    def sign_block_producer_metadata_upsert(self, genesis_utxo, metadata_file, cross_chain_signing_key):
+    def sign_block_producer_metadata_upsert(self, genesis_utxo, metadata_file, cross_chain_signing_key, partner_chain_account):
         return self.partner_chains_node.sign_block_producer_metadata_upsert(
-            genesis_utxo, metadata_file, cross_chain_signing_key
+            genesis_utxo, metadata_file, cross_chain_signing_key, partner_chain_account
         )
 
-    def sign_block_producer_metadata_delete(self, genesis_utxo, cross_chain_signing_key):
+    def sign_block_producer_metadata_delete(self, genesis_utxo, cross_chain_signing_key, partner_chain_account):
         return self.partner_chains_node.sign_block_producer_metadata_delete(
-            genesis_utxo, cross_chain_signing_key
+            genesis_utxo, cross_chain_signing_key, partner_chain_account
         )
 
     @long_running_function

e2e-tests/tests/committee/test_blocks.py

@@ -18,7 +18,7 @@ def test_block_producer_can_update_their_metadata(genesis_utxo, api: BlockchainA
     }
     metadata_filepath = write_file(api.partner_chains_node.run_command, metadata)
 
-    signature = api.sign_block_producer_metadata_upsert(genesis_utxo, metadata_filepath, skey)
+    signature = api.sign_block_producer_metadata_upsert(genesis_utxo, metadata_filepath, skey, get_wallet.address)
     assert signature.signature, "Signature is empty"
     assert signature.cross_chain_pub_key == f"0x{vkey_hex}"
 
@@ -41,7 +41,7 @@ def test_block_producer_can_update_their_metadata(genesis_utxo, api: BlockchainA
         "hash": "0x0000000000000000000000000000000000000000000000000000000000000002",
     }
     metadata_filepath = write_file(api.partner_chains_node.run_command, metadata)
-    signature = api.sign_block_producer_metadata_upsert(genesis_utxo, metadata_filepath, skey)
+    signature = api.sign_block_producer_metadata_upsert(genesis_utxo, metadata_filepath, skey, get_wallet.address)
     assert signature.signature, "Signature is empty"
     assert signature.cross_chain_pub_key == f"0x{vkey_hex}"
 
@@ -72,7 +72,7 @@ def test_block_producer_can_delete_their_metadata(genesis_utxo, api: BlockchainA
     }
     metadata_filepath = write_file(api.partner_chains_node.run_command, metadata)
 
-    signature = api.sign_block_producer_metadata_upsert(genesis_utxo, metadata_filepath, skey)
+    signature = api.sign_block_producer_metadata_upsert(genesis_utxo, metadata_filepath, skey, get_wallet.address)
     assert signature.signature, "Signature is empty"
     assert signature.cross_chain_pub_key == f"0x{vkey_hex}"
 
@@ -90,7 +90,7 @@ def test_block_producer_can_delete_their_metadata(genesis_utxo, api: BlockchainA
     assert rpc_metadata == metadata, "RPC did not return block producer metadata or it is incorrect"
 
     logger.info("Starting delete")
-    signature = api.sign_block_producer_metadata_delete(genesis_utxo, skey)
+    signature = api.sign_block_producer_metadata_delete(genesis_utxo, skey, get_wallet.address)
     assert signature.signature, "Signature is empty"
     assert signature.cross_chain_pub_key == f"0x{vkey_hex}"
 

toolkit/block-producer-metadata/pallet/Cargo.toml

@@ -24,6 +24,7 @@ sp-core = { workspace = true, optional = true }
 sp-block-producer-metadata = { workspace = true }
 sp-runtime = { workspace = true, optional = true }
 sp-io = { workspace = true, optional = true }
+k256 = { workspace = true, optional = true }
 
 [dev-dependencies]
 sp-core = { workspace = true }
@@ -33,6 +34,7 @@ hex-literal = { workspace = true }
 k256 = { workspace = true }
 pallet-balances = { workspace = true }
 pretty_assertions = { workspace = true }
+sp-block-producer-metadata = { workspace = true, features = ["std"] }
 
 [features]
 default = ["std"]
@@ -57,4 +59,5 @@ runtime-benchmarks = [
     "hex-literal",
     "sp-core",
     "sp-io",
+    "k256",
 ]

toolkit/block-producer-metadata/pallet/src/benchmarking.rs

@@ -7,18 +7,24 @@
 //!
 //! ```rust
 //! use sidechain_domain::byte_string::*;
-//! use sidechain_domain::{ CrossChainSignature, CrossChainPublicKey };
-//! use sp_core::ConstU32;
+//! use sidechain_domain::{ CrossChainSignature, CrossChainPublicKey, UtxoId };
+//! use sp_core::{ ConstU32, Encode };
 //! use hex_literal::hex;
+//! use sp_runtime::AccountId32;
 //!
+//! #[derive(Encode)]
 //! struct BlockProducerMetadata {
 //!     pub url: BoundedString<ConstU32<512>>,
 //!     pub hash: SizedByteString<32>,
 //! }
 //!
 //! struct ExampleBenchmarkHelper;
 //!
-//! impl pallet_block_producer_metadata::benchmarking::BenchmarkHelper<BlockProducerMetadata> for ExampleBenchmarkHelper {
+//! impl pallet_block_producer_metadata::benchmarking::BenchmarkHelper<BlockProducerMetadata, AccountId32> for ExampleBenchmarkHelper {
+//!     fn genesis_utxo() -> UtxoId {
+//!         UtxoId::new([1;32], 0)
+//!     }
+//!
 //!     fn metadata() -> BlockProducerMetadata {
 //!         BlockProducerMetadata {
 //!        	   url: BoundedString::try_from("https://cool.stuff/spo.json").unwrap(),
@@ -28,11 +34,8 @@
 //!     fn cross_chain_pub_key() -> CrossChainPublicKey {
 //!         CrossChainPublicKey(hex!("020a1091341fe5664bfa1782d5e04779689068c916b04cb365ec3153755684d9a1").to_vec())
 //!     }
-//!     fn upsert_cross_chain_signature() -> sidechain_domain::CrossChainSignature {
-//!         CrossChainSignature(hex!("810854f5bd1d06dc8583ebd58ff4877dddb1646511edb10afd021f716bf51a8e617353b6c5d5f92a2005e2c3c24b782a6f74132d6b54251854cce186c981862c").to_vec())
-//!     }
-//!     fn delete_cross_chain_signature() -> sidechain_domain::CrossChainSignature {
-//!         CrossChainSignature(hex!("5c1a701c8adffdf53a371409a24cc6c2d778a4c65c2c105c5fccfc5eeb69e3fa59bd723e7c10893f53fcfdfff8c02954f2230953cb9596119c11d4a9a29564c5").to_vec())
+//!     fn cross_chain_sign_key() -> k256::SecretKey {
+//!         k256::SecretKey::from_slice(&[2;32]).unwrap()
 //!     }
 //!     fn upsert_valid_before() -> u64 {
 //!     	100_000_000
@@ -65,26 +68,49 @@
 use super::*;
 use frame_benchmarking::v2::*;
 use frame_system::RawOrigin;
+pub use k256::SecretKey;
 use sidechain_domain::*;
 
 /// Helper trait for injecting mock values for use in benchmarks
-pub trait BenchmarkHelper<BlockProducerMetadata> {
+pub trait BenchmarkHelper<BlockProducerMetadata: Encode, AccountId: Encode> {
+	/// Should return the chain's genesis utxo
+	fn genesis_utxo() -> UtxoId;
 	/// Should return mock metadata
 	fn metadata() -> BlockProducerMetadata;
 	/// Should return mock cross-chain pubkey
 	fn cross_chain_pub_key() -> CrossChainPublicKey;
+	/// Should return mock cross-chain signing key
+	fn cross_chain_sign_key() -> SecretKey;
 	/// Should return mock cross-chain signature for upsert operation
 	///
 	/// This signature must match the cross-chain pubkey returned by `cross_chain_pub_key` and be a valid
 	/// signature of [MetadataSignedMessage] created using values returned by `metadata` and `cross_chain_pub_key`
 	/// and the genesis UTXO used for benchmarks.
-	fn upsert_cross_chain_signature() -> CrossChainSignature;
+	fn upsert_cross_chain_signature(owner: AccountId) -> CrossChainSignature {
+		MetadataSignedMessage {
+			cross_chain_pub_key: Self::cross_chain_pub_key(),
+			metadata: Some(Self::metadata()),
+			genesis_utxo: Self::genesis_utxo(),
+			valid_before: Self::upsert_valid_before(),
+			owner,
+		}
+		.sign_with_key(&Self::cross_chain_sign_key())
+	}
 
 	/// Should return mock cross-chain signature for delete operation
 	///
 	/// This signature must match the cross-chain pubkey returned by `cross_chain_pub_key` and be a valid
 	/// for the genesis UTXO used for benchmarks.
-	fn delete_cross_chain_signature() -> CrossChainSignature;
+	fn delete_cross_chain_signature(owner: AccountId) -> CrossChainSignature {
+		MetadataSignedMessage {
+			cross_chain_pub_key: Self::cross_chain_pub_key(),
+			metadata: None::<BlockProducerMetadata>,
+			genesis_utxo: Self::genesis_utxo(),
+			valid_before: Self::delete_valid_before(),
+			owner,
+		}
+		.sign_with_key(&Self::cross_chain_sign_key())
+	}
 
 	/// Should return the valid-before value for the upsert
 	fn upsert_valid_before() -> u64;
@@ -103,13 +129,15 @@ mod benchmarks {
 	fn upsert_metadata() {
 		let metadata = T::BenchmarkHelper::metadata();
 		let cross_chain_pub_key = T::BenchmarkHelper::cross_chain_pub_key();
-		let cross_chain_signature = T::BenchmarkHelper::upsert_cross_chain_signature();
 		let valid_before = T::BenchmarkHelper::upsert_valid_before();
 
 		// Create an account and fund it with sufficient balance
 		let caller: T::AccountId = account("caller", 0, 0);
 		let _ = T::Currency::mint_into(&caller, T::HoldAmount::get() * 2u32.into());
 
+		let cross_chain_signature =
+			T::BenchmarkHelper::upsert_cross_chain_signature(caller.clone());
+
 		#[extrinsic_call]
 		_(
 			RawOrigin::Signed(caller),
@@ -124,13 +152,15 @@ mod benchmarks {
 	fn delete_metadata() {
 		let metadata = T::BenchmarkHelper::metadata();
 		let cross_chain_pub_key = T::BenchmarkHelper::cross_chain_pub_key();
-		let cross_chain_signature = T::BenchmarkHelper::delete_cross_chain_signature();
 		let valid_before = T::BenchmarkHelper::delete_valid_before();
 
 		let caller: T::AccountId = account("caller", 0, 0);
 		let _ =
 			T::Currency::hold(&HoldReason::MetadataDeposit.into(), &caller, T::HoldAmount::get());
 
+		let cross_chain_signature =
+			T::BenchmarkHelper::delete_cross_chain_signature(caller.clone());
+
 		BlockProducerMetadataStorage::<T>::insert(
 			cross_chain_pub_key.hash(),
 			(metadata, caller.clone(), T::HoldAmount::get()),

toolkit/block-producer-metadata/pallet/src/lib.rs

@@ -183,7 +183,7 @@ pub mod pallet {
 
 		/// Helper providing mock values for use in benchmarks
 		#[cfg(feature = "runtime-benchmarks")]
-		type BenchmarkHelper: benchmarking::BenchmarkHelper<Self::BlockProducerMetadata>;
+		type BenchmarkHelper: benchmarking::BenchmarkHelper<Self::BlockProducerMetadata, Self::AccountId>;
 	}
 
 	/// Storage mapping from block producers to their metadata, owner account and deposit amount
@@ -248,6 +248,7 @@ pub mod pallet {
 				metadata: Some(metadata.clone()),
 				genesis_utxo,
 				valid_before,
+				owner: origin_account.clone(),
 			};
 
 			let is_valid_signature =
@@ -297,11 +298,12 @@ pub mod pallet {
 			let origin_account = ensure_signed(origin)?;
 
 			let genesis_utxo = T::genesis_utxo();
-			let metadata_message = MetadataSignedMessage::<T::BlockProducerMetadata> {
+			let metadata_message = MetadataSignedMessage::<T::BlockProducerMetadata, T::AccountId> {
 				cross_chain_pub_key: cross_chain_pub_key.clone(),
 				metadata: None,
 				genesis_utxo,
 				valid_before,
+				owner: origin_account.clone(),
 			};
 			let cross_chain_key_hash = cross_chain_pub_key.hash();
 			let is_valid_signature =