From 2f9a43ed2b9ab9fbd28dcf5750be40e530481be8 Mon Sep 17 00:00:00 2001 From: Ian Jungyong Um <31336310+code0xff@users.noreply.github.com> Date: Tue, 29 Oct 2024 15:06:15 +0900 Subject: [PATCH] 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 --- frame/babel/src/tests.rs | 20 ++++++++++++- frame/cosmos/x/auth/Cargo.toml | 2 +- frame/cosmos/x/auth/src/sigverify.rs | 38 ++++++++++++++---------- vendor/composable/cosmwasm/src/crypto.rs | 22 +++++--------- 4 files changed, 51 insertions(+), 31 deletions(-) diff --git a/frame/babel/src/tests.rs b/frame/babel/src/tests.rs index 2ac000eb..172546a1 100644 --- 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::::do_secp256k1_verify( + &message_hash, + &signature, + &public_key + )); + }); +} diff --git a/frame/cosmos/x/auth/Cargo.toml b/frame/cosmos/x/auth/Cargo.toml index 1f1741ff..ec8c5755 100644 --- 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 index 407fa57c..b4f18a4d 100644 --- 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(PhantomData); impl AnteDecorator for SigVerificationDecorator 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 index a60e333b..733c2bee 100644 --- a/vendor/composable/cosmwasm/src/crypto.rs +++ b/vendor/composable/cosmwasm/src/crypto.rs @@ -37,30 +37,24 @@ impl Pallet { .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(