refactor: Refactor signature verification logic (#91)

* refactor: Refactor signature verification logic * test: Replace hex to const-hex * fix: Fix pallet-cosmwasm ecdsa signature verification bug * refactor: Refactor variable names for improved clarity
NoirHQ · Oct 29, 2024 · 2f9a43e · 2f9a43e
1 parent 271620b
commit 2f9a43e
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 31 deletions.
diff --git a/frame/babel/src/tests.rs b/frame/babel/src/tests.rs
@@ -19,7 +19,7 @@
 use crate::{mock::*, *};
 use frame_support::{assert_ok, traits::fungible::Inspect};
 use np_babel::EthereumAddress;
-use sp_core::ecdsa;
+use sp_core::{ecdsa, sha2_256};
 use sp_runtime::traits::AccountIdConversion;
 
 fn dev_public() -> ecdsa::Public {
@@ -79,3 +79,21 @@ fn transfer_to_nostr_address_works() {
 		assert_eq!(Balances::balance(&account), 100);
 	});
 }
+
+#[test]
+fn ecdsa_verify_prehashed() {
+	let signature = const_hex::decode("f7e0d198c62821cc5817c8e935f523308301e29819f5d882f3249b9e173a614f38000ddbff446c0abfa7c7d019dbb17072b28933fc8187c973fbf03d0459f76e").unwrap();
+	let message = const_hex::decode("0a93010a90010a1c2f636f736d6f732e62616e6b2e763162657461312e4d736753656e6412700a2d636f736d6f7331716436396e75776a393567746134616b6a677978746a39756a6d7a34773865646d7179737177122d636f736d6f7331676d6a32657861673033747467616670726b6463337438383067726d61396e776566636432771a100a057561746f6d12073130303030303012710a4e0a460a1f2f636f736d6f732e63727970746f2e736563703235366b312e5075624b657912230a21020a1091341fe5664bfa1782d5e04779689068c916b04cb365ec3153755684d9a112040a020801121f0a150a057561746f6d120c3838363838303030303030301080c0f1c59495141a1174686574612d746573746e65742d30303120ad8a2e").unwrap();
+	let message_hash = sha2_256(&message);
+	let public_key =
+		const_hex::decode("020a1091341fe5664bfa1782d5e04779689068c916b04cb365ec3153755684d9a1")
+			.unwrap();
+
+	new_test_ext().execute_with(|| {
+		assert!(pallet_cosmwasm::Pallet::<Test>::do_secp256k1_verify(
+			&message_hash,
+			&signature,
+			&public_key
+		));
+	});
+}
diff --git a/frame/cosmos/x/auth/Cargo.toml b/frame/cosmos/x/auth/Cargo.toml
@@ -21,7 +21,7 @@ sp-io = { git = "https://github.com/paritytech/polkadot-sdk", branch = "stable24
 sp-runtime = { git = "https://github.com/paritytech/polkadot-sdk", branch = "stable2409", default-features = false }
 
 [dev-dependencies]
-hex = "0.4"
+const-hex = "1.13"
 
 [features]
 default = ["std"]

diff --git a/frame/cosmos/x/auth/src/sigverify.rs b/frame/cosmos/x/auth/src/sigverify.rs
@@ -42,6 +42,8 @@ use ripemd::Digest;
 use sp_core::{ecdsa, sha2_256, ByteArray, Get, H160};
 use sp_runtime::SaturatedConversion;
 
+const ECDSA_SIGNATURE_LEN: usize = 65;
+
 pub struct SigVerificationDecorator<T>(PhantomData<T>);
 impl<T> AnteDecorator for SigVerificationDecorator<T>
 where
@@ -119,8 +121,9 @@ where
 
 					let sign_bytes = T::SignModeHandler::get_sign_bytes(sign_mode, signer_data, tx)
 						.map_err(|_| RootError::Unauthorized)?;
+					let message_hash = sha2_256(&sign_bytes);
 
-					if !ecdsa_verify_prehashed(signature, &sign_bytes, &public_key.key) {
+					if !ecdsa_verify_prehashed(signature, &message_hash, &public_key.key) {
 						return Err(RootError::Unauthorized.into());
 					}
 
@@ -132,23 +135,27 @@ where
 	}
 }
 
-pub fn ecdsa_verify_prehashed(signature: &[u8], message: &[u8], public_key: &[u8]) -> bool {
+pub fn ecdsa_verify_prehashed(
+	signature: &[u8],
+	message_hash: &[u8; 32],
+	public_key: &[u8],
+) -> bool {
 	let pub_key = match ecdsa::Public::from_slice(public_key) {
 		Ok(pub_key) => pub_key,
 		Err(_) => return false,
 	};
-	let msg = sha2_256(message);
 
 	match signature.len() {
-		64 => (0..=3).any(|rec_id| {
-			let mut rec_sig = [0u8; 65];
-			rec_sig[..64].copy_from_slice(signature);
-			rec_sig[64] = rec_id;
-			let sig = ecdsa::Signature::from(rec_sig);
-			sp_io::crypto::ecdsa_verify_prehashed(&sig, &msg, &pub_key)
+		64 => (0..=3).any(|recovery_id| {
+			let mut signature_inner = [0u8; ECDSA_SIGNATURE_LEN];
+			signature_inner[..ECDSA_SIGNATURE_LEN - 1].copy_from_slice(signature);
+			signature_inner[ECDSA_SIGNATURE_LEN - 1] = recovery_id;
+			let sig = ecdsa::Signature::from(signature_inner);
+			sp_io::crypto::ecdsa_verify_prehashed(&sig, message_hash, &pub_key)
+		}),
+		65 => ecdsa::Signature::try_from(signature).map_or(false, |sig| {
+			sp_io::crypto::ecdsa_verify_prehashed(&sig, message_hash, &pub_key)
 		}),
-		65 => ecdsa::Signature::try_from(signature)
-			.map_or(false, |sig| sp_io::crypto::ecdsa_verify_prehashed(&sig, &msg, &pub_key)),
 		_ => false,
 	}
 }
@@ -214,12 +221,13 @@ pub mod tests {
 
 	#[test]
 	fn ecdsa_verify_test() {
-		let sig = hex::decode("f7e0d198c62821cc5817c8e935f523308301e29819f5d882f3249b9e173a614f38000ddbff446c0abfa7c7d019dbb17072b28933fc8187c973fbf03d0459f76e").unwrap();
-		let message = hex::decode("0a93010a90010a1c2f636f736d6f732e62616e6b2e763162657461312e4d736753656e6412700a2d636f736d6f7331716436396e75776a393567746134616b6a677978746a39756a6d7a34773865646d7179737177122d636f736d6f7331676d6a32657861673033747467616670726b6463337438383067726d61396e776566636432771a100a057561746f6d12073130303030303012710a4e0a460a1f2f636f736d6f732e63727970746f2e736563703235366b312e5075624b657912230a21020a1091341fe5664bfa1782d5e04779689068c916b04cb365ec3153755684d9a112040a020801121f0a150a057561746f6d120c3838363838303030303030301080c0f1c59495141a1174686574612d746573746e65742d30303120ad8a2e").unwrap();
+		let signature = const_hex::decode("f7e0d198c62821cc5817c8e935f523308301e29819f5d882f3249b9e173a614f38000ddbff446c0abfa7c7d019dbb17072b28933fc8187c973fbf03d0459f76e").unwrap();
+		let message = const_hex::decode("0a93010a90010a1c2f636f736d6f732e62616e6b2e763162657461312e4d736753656e6412700a2d636f736d6f7331716436396e75776a393567746134616b6a677978746a39756a6d7a34773865646d7179737177122d636f736d6f7331676d6a32657861673033747467616670726b6463337438383067726d61396e776566636432771a100a057561746f6d12073130303030303012710a4e0a460a1f2f636f736d6f732e63727970746f2e736563703235366b312e5075624b657912230a21020a1091341fe5664bfa1782d5e04779689068c916b04cb365ec3153755684d9a112040a020801121f0a150a057561746f6d120c3838363838303030303030301080c0f1c59495141a1174686574612d746573746e65742d30303120ad8a2e").unwrap();
+		let message_hash = sha2_256(&message);
 		let public_key =
-			hex::decode("020a1091341fe5664bfa1782d5e04779689068c916b04cb365ec3153755684d9a1")
+			const_hex::decode("020a1091341fe5664bfa1782d5e04779689068c916b04cb365ec3153755684d9a1")
 				.unwrap();
 
-		assert!(ecdsa_verify_prehashed(&sig, &message, &public_key));
+		assert!(ecdsa_verify_prehashed(&signature, &message_hash, &public_key));
 	}
 }
diff --git a/vendor/composable/cosmwasm/src/crypto.rs b/vendor/composable/cosmwasm/src/crypto.rs
@@ -37,30 +37,24 @@ impl<T: Config> Pallet<T> {
 			.map_err(|_| ())
 	}
 
-	pub(crate) fn do_secp256k1_verify(
-		message_hash: &[u8],
-		signature: &[u8],
-		public_key: &[u8],
-	) -> bool {
+	pub fn do_secp256k1_verify(message_hash: &[u8], signature: &[u8], public_key: &[u8]) -> bool {
 		let message_hash = match message_hash.try_into() {
 			Ok(message_hash) => message_hash,
 			Err(_) => return false,
 		};
 
-		// We are expecting 64 bytes long public keys but the substrate function use an
-		// additional byte for recovery id. So we insert a dummy byte.
-		let signature = {
-			let mut signature_inner = [0_u8; SUBSTRATE_ECDSA_SIGNATURE_LEN];
-			signature_inner[..SUBSTRATE_ECDSA_SIGNATURE_LEN - 1].copy_from_slice(signature);
-			ecdsa::Signature::from(signature_inner)
-		};
-
 		let public_key = match libsecp256k1::PublicKey::parse_slice(public_key, None) {
 			Ok(public_key) => ecdsa::Public::from_raw(public_key.serialize_compressed()),
 			Err(_) => return false,
 		};
 
-		sp_io::crypto::ecdsa_verify_prehashed(&signature, &message_hash, &public_key)
+		(0..=3).any(|recovery_id| {
+			let mut signature_inner = [0_u8; SUBSTRATE_ECDSA_SIGNATURE_LEN];
+			signature_inner[..SUBSTRATE_ECDSA_SIGNATURE_LEN - 1].copy_from_slice(signature);
+			signature_inner[SUBSTRATE_ECDSA_SIGNATURE_LEN - 1] = recovery_id;
+			let sig = ecdsa::Signature::from(signature_inner);
+			sp_io::crypto::ecdsa_verify_prehashed(&sig, message_hash, &public_key)
+		})
 	}
 
 	pub(crate) fn do_ed25519_batch_verify(