fix: Fix for proposal execution (#56)

bridge/src/lib.rs

@@ -28,10 +28,7 @@ pub mod pallet {
 	use frame_system::pallet_prelude::*;
 	use primitive_types::U256;
 	use scale_info::TypeInfo;
-	use sp_io::{
-		crypto::secp256k1_ecdsa_recover,
-		hashing::{blake2_256, keccak_256},
-	};
+	use sp_io::{crypto::secp256k1_ecdsa_recover, hashing::keccak_256};
 	use sp_runtime::{
 		traits::{AccountIdConversion, Clear},
 		RuntimeDebug,
@@ -180,6 +177,8 @@ pub mod pallet {
 		AssetNotBound,
 		/// Proposal has either failed or succeeded
 		ProposalAlreadyComplete,
+		/// Proposal list empty
+		EmptyProposalList,
 		/// Transactor operation failed
 		TransactorFailed,
 		/// Deposit data not correct
@@ -523,9 +522,14 @@ pub mod pallet {
 			// Check MPC address and bridge status
 			ensure!(!MpcAddr::<T>::get().is_clear(), Error::<T>::MissingMpcAddress);
 
+			ensure!(!proposals.is_empty(), Error::<T>::EmptyProposalList);
+
+			// parse proposals and construct signing message to meet EIP712 typed data
+			let final_message = Self::construct_ecdsa_signing_proposals_data(&proposals);
+
 			// Verify MPC signature
 			ensure!(
-				Self::verify_by_mpc_address(&proposals, signature),
+				Self::verify_by_mpc_address(final_message, signature),
 				Error::<T>::BadMpcSignature
 			);
 
@@ -574,25 +578,18 @@ pub mod pallet {
 	where
 		<T as frame_system::Config>::AccountId: From<[u8; 32]> + Into<[u8; 32]>,
 	{
-		/// Verifies that proposal data is signed by MPC address.
+		/// Verifies that EIP712 typed proposal data is signed by MPC address
 		#[allow(dead_code)]
-		fn verify_by_mpc_address(proposals: &Vec<Proposal>, signature: Vec<u8>) -> bool {
+		fn verify_by_mpc_address(signing_message: [u8; 32], signature: Vec<u8>) -> bool {
 			let sig = match signature.try_into() {
 				Ok(_sig) => _sig,
 				Err(error) => return false,
 			};
 
-			if proposals.is_empty() {
-				return false
-			}
-
-			// parse proposals and construct signing message
-			let final_message = Self::construct_ecdsa_signing_proposals_data(proposals);
-
 			// recover the signing address
 			if let Ok(pubkey) =
 				// recover the uncompressed pubkey
-				secp256k1_ecdsa_recover(&sig, &blake2_256(&final_message))
+				secp256k1_ecdsa_recover(&sig, &signing_message)
 			{
 				let address = Self::public_key_to_address(&pubkey);
 
@@ -611,6 +608,10 @@ pub mod pallet {
 
 		/// Parse proposals and construct the original signing message
 		pub fn construct_ecdsa_signing_proposals_data(proposals: &Vec<Proposal>) -> [u8; 32] {
+			let proposals_typehash = keccak_256(
+				"Proposals(Proposal[] proposals)Proposal(uint8 originDomainID,uint64 depositNonce,bytes32 resourceID,bytes data)"
+					.as_bytes(),
+			);
 			let proposal_typehash = keccak_256(
 				"Proposal(uint8 originDomainID,uint64 depositNonce,bytes32 resourceID,bytes data)"
 					.as_bytes(),
@@ -649,7 +650,7 @@ pub mod pallet {
 			let hashed_keccak_data = keccak_256(bytes.as_slice());
 
 			let struct_hash = keccak_256(&abi_encode(&[
-				Token::FixedBytes(proposal_typehash.to_vec()),
+				Token::FixedBytes(proposals_typehash.to_vec()),
 				Token::FixedBytes(hashed_keccak_data.to_vec()),
 			]));
 
@@ -969,8 +970,10 @@ pub mod pallet {
 				};
 				let proposals = vec![p1, p2];
 
+				let final_message = SygmaBridge::construct_ecdsa_signing_proposals_data(&proposals);
+
 				// should be false
-				assert!(!SygmaBridge::verify_by_mpc_address(&proposals, signature.encode()));
+				assert!(!SygmaBridge::verify_by_mpc_address(final_message, signature.encode()));
 			})
 		}
 
@@ -1002,8 +1005,10 @@ pub mod pallet {
 				};
 				let proposals = vec![p1, p2];
 
+				let final_message = SygmaBridge::construct_ecdsa_signing_proposals_data(&proposals);
+
 				// verify non matched signature against proposal list, should be false
-				assert!(!SygmaBridge::verify_by_mpc_address(&proposals, signature.encode()));
+				assert!(!SygmaBridge::verify_by_mpc_address(final_message, signature.encode()));
 			})
 		}
 
@@ -1036,10 +1041,10 @@ pub mod pallet {
 				let final_message = SygmaBridge::construct_ecdsa_signing_proposals_data(&proposals);
 
 				// sign final message using generated prikey
-				let signature = pair.sign(&final_message[..]);
+				let signature = pair.sign_prehashed(&final_message);
 
 				// verify signature, should be false because the signing address != mpc address
-				assert!(!SygmaBridge::verify_by_mpc_address(&proposals, signature.encode()));
+				assert!(!SygmaBridge::verify_by_mpc_address(final_message, signature.encode()));
 			})
 		}
 
@@ -1072,10 +1077,12 @@ pub mod pallet {
 				let final_message = SygmaBridge::construct_ecdsa_signing_proposals_data(&proposals);
 
 				// sign final message using generated mpc prikey
-				let signature = pair.sign(&final_message[..]);
+				// `pari.sign` will hash the final message into blake2_256 then sign it, so use
+				// sign_prehashed here
+				let signature = pair.sign_prehashed(&final_message);
 
 				// verify signature, should be true
-				assert!(SygmaBridge::verify_by_mpc_address(&proposals, signature.encode()));
+				assert!(SygmaBridge::verify_by_mpc_address(final_message, signature.encode()));
 			})
 		}
 
@@ -1659,10 +1666,9 @@ pub mod pallet {
 					invalid_recipient_proposal,
 				];
 
-				let proposals_with_valid_signature =
-					pair.sign(&SygmaBridge::construct_ecdsa_signing_proposals_data(&proposals));
-				let proposals_with_bad_signature = evil_pair
-					.sign(&SygmaBridge::construct_ecdsa_signing_proposals_data(&proposals));
+				let final_message = SygmaBridge::construct_ecdsa_signing_proposals_data(&proposals);
+				let proposals_with_valid_signature = pair.sign_prehashed(&final_message);
+				let proposals_with_bad_signature = evil_pair.sign_prehashed(&final_message);
 
 				// Should failed if dest domain 1 bridge paused
 				assert_ok!(SygmaBridge::pause_bridge(Origin::root(), DEST_DOMAIN_ID));
@@ -1693,7 +1699,7 @@ pub mod pallet {
 				assert_eq!(Balances::free_balance(&BOB), ENDOWED_BALANCE);
 				assert_eq!(Assets::balance(UsdcAssetId::get(), &BOB), 0);
 				assert!(SygmaBridge::verify_by_mpc_address(
-					&proposals,
+					final_message,
 					proposals_with_valid_signature.encode()
 				));
 				assert_ok!(SygmaBridge::execute_proposal(

substrate-node/runtime/src/lib.rs

@@ -15,6 +15,7 @@ use pallet_grandpa::{
 	fg_primitives, AuthorityId as GrandpaId, AuthorityList as GrandpaAuthorityList,
 };
 use polkadot_parachain::primitives::Sibling;
+use primitive_types::U256;
 use sp_api::impl_runtime_apis;
 use sp_consensus_aura::sr25519::AuthorityId as AuraId;
 use sp_core::{crypto::KeyTypeId, OpaqueMetadata};
@@ -382,7 +383,7 @@ parameter_types! {
 	// BridgeAccount address: 5EMepC39b7E2zfM9g6CkPp8KCAxGTh7D4w4T2tFjmjpd4tPw
 	pub BridgeAccount: AccountId32 = AccountId32::new([101u8; 32]);
 	// EIP712ChainID is the chainID that pallet is assigned with, used in EIP712 typed data domain
-	pub EIP712ChainID: ChainID = primitive_types::U256([1u64; 4]);
+	pub EIP712ChainID: ChainID = U256::from(5);
 	// DestVerifyingContractAddress is a H160 address that is used in proposal signature verification, specifically EIP712 typed data
 	// When relayers signing, this address will be included in the EIP712Domain
 	// As long as the relayer and pallet configured with the same address, EIP712Domain should be recognized properly.