From eebe5fd9ba409a07f123b59f5fa6a571759c4f86 Mon Sep 17 00:00:00 2001 From: Jamie Ford Date: Wed, 11 Jan 2023 13:17:17 +1100 Subject: [PATCH] Stricter types for multisig payloads (#2658) --- .../src/multisig/client/ceremony_manager.rs | 18 +++-- .../multisig/client/ceremony_manager/tests.rs | 5 +- .../multisig/client/ceremony_runner/tests.rs | 7 +- engine/src/multisig/client/helpers.rs | 24 ++++--- engine/src/multisig/client/keygen/tests.rs | 5 +- engine/src/multisig/client/mod.rs | 10 +-- .../multisig/client/multisig_client_tests.rs | 4 +- engine/src/multisig/client/signing.rs | 8 +-- .../multisig/client/signing/signing_detail.rs | 65 +++++++++++++----- .../multisig/client/signing/signing_stages.rs | 17 ++--- engine/src/multisig/client/signing/tests.rs | 3 +- engine/src/multisig/crypto.rs | 23 ++++--- engine/src/multisig/crypto/eth.rs | 61 +++++++++++------ engine/src/multisig/crypto/polkadot.rs | 68 +++++++++++++------ engine/src/multisig/mod.rs | 12 ---- engine/src/multisig/tests/fixtures.rs | 8 --- engine/src/multisig/tests/mod.rs | 1 - .../state_chain_observer/sc_observer/mod.rs | 9 +-- .../state_chain_observer/sc_observer/tests.rs | 2 +- 19 files changed, 204 insertions(+), 146 deletions(-) delete mode 100644 engine/src/multisig/tests/fixtures.rs delete mode 100644 engine/src/multisig/tests/mod.rs diff --git a/engine/src/multisig/client/ceremony_manager.rs b/engine/src/multisig/client/ceremony_manager.rs index 977718b771..b0b8f1a0ef 100644 --- a/engine/src/multisig/client/ceremony_manager.rs +++ b/engine/src/multisig/client/ceremony_manager.rs @@ -38,8 +38,6 @@ use client::common::{ broadcast::BroadcastStage, CeremonyCommon, CeremonyFailureReason, KeygenResultInfo, }; -use crate::multisig::SigningPayload; - use super::{ common::{CeremonyStage, KeygenStageName, PreProcessStageDataCheck, SigningStageName}, keygen::{HashCommitments1, HashContext, KeygenData}, @@ -93,11 +91,11 @@ pub struct SigningCeremony { _phantom: PhantomData, } -impl CeremonyTrait for SigningCeremony { - type Crypto = Crypto; - type Data = SigningData<::Point>; - type Request = CeremonyRequest; - type Output = ::Signature; +impl CeremonyTrait for SigningCeremony { + type Crypto = C; + type Data = SigningData<::Point>; + type Request = CeremonyRequest; + type Output = ::Signature; type FailureReason = SigningFailureReason; type CeremonyStageName = SigningStageName; } @@ -128,7 +126,7 @@ pub fn prepare_signing_request( own_account_id: &AccountId, signers: BTreeSet, key_info: KeygenResultInfo, - payload: SigningPayload, + payload: Crypto::SigningPayload, outgoing_p2p_message_sender: &UnboundedSender, rng: Rng, logger: &slog::Logger, @@ -398,7 +396,7 @@ impl CeremonyManager { &mut self, ceremony_id: CeremonyId, signers: BTreeSet, - payload: SigningPayload, + payload: C::SigningPayload, key_info: KeygenResultInfo, rng: Rng, result_sender: CeremonyResultSender>, @@ -501,7 +499,7 @@ impl CeremonyManager { fn single_party_signing( &self, - payload: SigningPayload, + payload: C::SigningPayload, keygen_result_info: KeygenResultInfo, mut rng: Rng, ) -> C::Signature { diff --git a/engine/src/multisig/client/ceremony_manager/tests.rs b/engine/src/multisig/client/ceremony_manager/tests.rs index 4203cc73b1..9636e242ef 100644 --- a/engine/src/multisig/client/ceremony_manager/tests.rs +++ b/engine/src/multisig/client/ceremony_manager/tests.rs @@ -22,7 +22,6 @@ use crate::{ }, crypto::{CryptoScheme, Rng}, eth::{EthSchnorrSignature, EthSigning}, - tests::fixtures::SIGNING_PAYLOAD, }, p2p::OutgoingMultisigStageMessages, task_scope::task_scope, @@ -50,7 +49,7 @@ async fn run_on_request_to_sign( ceremony_manager.on_request_to_sign( ceremony_id, participants, - SIGNING_PAYLOAD.clone(), + C::signing_payload_for_test(), get_key_data_for_test::(BTreeSet::from_iter(ACCOUNT_IDS.iter().cloned())), Rng::from_seed(DEFAULT_SIGNING_SEED), result_sender, @@ -93,7 +92,7 @@ fn send_signing_request( ceremony_id, details: Some(CeremonyRequestDetails::Sign(SigningRequestDetails:: { participants, - payload: SIGNING_PAYLOAD.clone(), + payload: EthSigning::signing_payload_for_test(), keygen_result_info: get_key_data_for_test::(BTreeSet::from_iter( ACCOUNT_IDS.iter().cloned(), )), diff --git a/engine/src/multisig/client/ceremony_runner/tests.rs b/engine/src/multisig/client/ceremony_runner/tests.rs index 1578c8f685..3d83333e54 100644 --- a/engine/src/multisig/client/ceremony_runner/tests.rs +++ b/engine/src/multisig/client/ceremony_runner/tests.rs @@ -17,7 +17,6 @@ use crate::{ }, crypto::CryptoScheme, eth::{EthSigning, Point}, - tests::fixtures::SIGNING_PAYLOAD, Rng, }, p2p::OutgoingMultisigStageMessages, @@ -135,7 +134,7 @@ async fn should_delay_stage_1_message_while_unauthorised() { &our_account_id.clone(), participants.clone(), get_key_data_for_test::(participants), - SIGNING_PAYLOAD.clone(), + EthSigning::signing_payload_for_test(), &outgoing_p2p_sender, Rng::from_seed(DEFAULT_SIGNING_SEED), &new_test_logger(), @@ -224,7 +223,7 @@ async fn gen_stage_1_signing_state( &our_account_id.clone(), BTreeSet::from_iter(participants.clone()), get_key_data_for_test::(BTreeSet::from_iter(participants)), - SIGNING_PAYLOAD.clone(), + EthSigning::signing_payload_for_test(), &outgoing_p2p_sender, Rng::from_seed(DEFAULT_SIGNING_SEED), &logger, @@ -335,7 +334,7 @@ async fn should_timeout_authorised_ceremony() { &ACCOUNT_IDS[0], BTreeSet::from_iter(ACCOUNT_IDS.iter().cloned()), get_key_data_for_test::(BTreeSet::from_iter(ACCOUNT_IDS.iter().cloned())), - SIGNING_PAYLOAD.clone(), + EthSigning::signing_payload_for_test(), &outgoing_p2p_sender, Rng::from_seed(DEFAULT_SIGNING_SEED), &new_test_logger(), diff --git a/engine/src/multisig/client/helpers.rs b/engine/src/multisig/client/helpers.rs index a373fe4d53..66baad5bba 100644 --- a/engine/src/multisig/client/helpers.rs +++ b/engine/src/multisig/client/helpers.rs @@ -35,8 +35,8 @@ use crate::{ keygen::{generate_key_data, HashComm1, HashContext, VerifyHashCommitmentsBroadcast2}, signing, KeygenResultInfo, PartyIdxMapping, ThresholdParameters, }, - crypto::{ECPoint, Rng, Verifiable}, - CryptoScheme, KeyId, SigningPayload, + crypto::{ECPoint, Rng}, + CryptoScheme, KeyId, }, p2p::OutgoingMultisigStageMessages, }; @@ -51,7 +51,6 @@ use crate::{ // This determines which crypto scheme will be used in tests // (we make arbitrary choice to use eth) crypto::eth::{EthSigning, Point}, - tests::fixtures::SIGNING_PAYLOAD, }, testing::expect_recv_with_timeout, }; @@ -117,7 +116,7 @@ pub struct SigningCeremonyDetails { pub rng: Rng, pub ceremony_id: CeremonyId, pub signers: BTreeSet, - pub payload: SigningPayload, + pub payload: C::SigningPayload, pub keygen_result_info: KeygenResultInfo, } @@ -623,7 +622,7 @@ impl KeygenCeremonyRunner { pub struct SigningCeremonyRunnerData { pub key_id: KeyId, pub key_data: HashMap>, - pub payload: SigningPayload, + pub payload: C::SigningPayload, } pub type SigningCeremonyRunner = CeremonyTestRunner, SigningCeremony>; @@ -641,9 +640,12 @@ impl CeremonyRunnerStrategy for SigningCeremonyRunner { let signature = all_same(outputs.into_iter().map(|(_, signature)| signature)) .expect("Signatures don't match"); - signature - .verify(&self.ceremony_runner_data.key_id, &SIGNING_PAYLOAD) - .expect("Should be valid signature"); + C::verify_signature( + &signature, + &self.ceremony_runner_data.key_id, + &C::signing_payload_for_test(), + ) + .expect("Should be valid signature"); signature } @@ -665,7 +667,7 @@ impl SigningCeremonyRunner { ceremony_id: CeremonyId, key_id: KeyId, key_data: HashMap>, - payload: SigningPayload, + payload: C::SigningPayload, rng: Rng, ) -> Self { Self::inner_new( @@ -681,7 +683,7 @@ impl SigningCeremonyRunner { ceremony_id: CeremonyId, key_id: KeyId, key_data: HashMap>, - payload: SigningPayload, + payload: C::SigningPayload, rng: Rng, ) -> (Self, HashMap>>) { let nodes_len = nodes.len(); @@ -726,7 +728,7 @@ pub async fn new_signing_ceremony( DEFAULT_SIGNING_CEREMONY_ID, key_id, key_data, - SIGNING_PAYLOAD.clone(), + C::signing_payload_for_test(), Rng::from_seed(DEFAULT_SIGNING_SEED), ) } diff --git a/engine/src/multisig/client/keygen/tests.rs b/engine/src/multisig/client/keygen/tests.rs index d97f4d99a1..43fb208f10 100644 --- a/engine/src/multisig/client/keygen/tests.rs +++ b/engine/src/multisig/client/keygen/tests.rs @@ -18,6 +18,7 @@ use crate::multisig::{ utils::PartyIdxMapping, }, crypto::Rng, + CryptoScheme, }; use crate::multisig::crypto::eth::Point; @@ -886,7 +887,7 @@ mod timeout { #[tokio::test] async fn genesis_keys_can_sign() { - use crate::multisig::{crypto::eth::EthSigning, tests::fixtures::SIGNING_PAYLOAD}; + use crate::multisig::crypto::eth::EthSigning; let account_ids: BTreeSet<_> = [1, 2, 3, 4].iter().map(|i| AccountId::new([*i; 32])).collect(); @@ -900,7 +901,7 @@ async fn genesis_keys_can_sign() { DEFAULT_SIGNING_CEREMONY_ID, key_id.clone(), key_data.clone(), - SIGNING_PAYLOAD.clone(), + EthSigning::signing_payload_for_test(), Rng::from_entropy(), ); standard_signing(&mut signing_ceremony).await; diff --git a/engine/src/multisig/client/mod.rs b/engine/src/multisig/client/mod.rs index ae5cfc6744..997b7f4efa 100644 --- a/engine/src/multisig/client/mod.rs +++ b/engine/src/multisig/client/mod.rs @@ -58,7 +58,7 @@ use self::{ use super::{ crypto::{CryptoScheme, ECPoint}, - Rng, SigningPayload, + Rng, }; #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] @@ -123,7 +123,7 @@ pub trait MultisigClientApi { ceremony_id: CeremonyId, key_id: KeyId, signers: BTreeSet, - payload: SigningPayload, + payload: C::SigningPayload, ) -> BoxFuture<'_, Result, SigningFailureReason)>>; fn update_latest_ceremony_id(&self, ceremony_id: CeremonyId); @@ -154,7 +154,7 @@ where C: CryptoScheme, { pub participants: BTreeSet, - pub payload: SigningPayload, + pub payload: C::SigningPayload, pub keygen_result_info: KeygenResultInfo<::Point>, pub rng: Rng, pub result_sender: CeremonyResultSender>, @@ -252,14 +252,14 @@ impl MultisigClientApi for MultisigClient { ceremony_id: CeremonyId, key_id: KeyId, signers: BTreeSet, - payload: SigningPayload, + payload: C::SigningPayload, ) -> BoxFuture<'_, Result, SigningFailureReason)>> { assert!(signers.contains(&self.my_account_id)); slog::debug!( self.logger, "Received a request to sign"; - "message_hash" => payload.to_string(), + "payload" => payload.to_string(), "signers" => format_iterator(&signers).to_string(), CEREMONY_ID_KEY => ceremony_id ); diff --git a/engine/src/multisig/client/multisig_client_tests.rs b/engine/src/multisig/client/multisig_client_tests.rs index aac8f53f83..e82109ba2e 100644 --- a/engine/src/multisig/client/multisig_client_tests.rs +++ b/engine/src/multisig/client/multisig_client_tests.rs @@ -17,7 +17,7 @@ use crate::{ CeremonyRequestDetails, }, eth::EthSigning, - KeyId, PersistentKeyDB, SigningPayload, + KeyId, PersistentKeyDB, }, testing::{assert_future_can_complete, new_temp_directory_with_nonexistent_file}, }; @@ -51,7 +51,7 @@ async fn should_ignore_rts_for_unknown_key() { DEFAULT_SIGNING_CEREMONY_ID, key_id, BTreeSet::from_iter(ACCOUNT_IDS.iter().cloned()), - SigningPayload(vec![0; 32]), + EthSigning::signing_payload_for_test(), ); // Check sign request fails immediately with "unknown key" error diff --git a/engine/src/multisig/client/signing.rs b/engine/src/multisig/client/signing.rs index 04b9e7ad18..47f3484144 100644 --- a/engine/src/multisig/client/signing.rs +++ b/engine/src/multisig/client/signing.rs @@ -7,7 +7,7 @@ mod tests; use std::sync::Arc; -use crate::multisig::{crypto::ECPoint, SigningPayload}; +use crate::multisig::CryptoScheme; use super::common::KeygenResult; @@ -27,7 +27,7 @@ pub use signing_detail::get_lagrange_coeff; /// Data common for signing stages #[derive(Clone)] -pub struct SigningStateCommonInfo { - pub payload: SigningPayload, - pub key: Arc>, +pub struct SigningStateCommonInfo { + pub payload: C::SigningPayload, + pub key: Arc>, } diff --git a/engine/src/multisig/client/signing/signing_detail.rs b/engine/src/multisig/client/signing/signing_detail.rs index bb10e67142..9c082f4c7c 100644 --- a/engine/src/multisig/client/signing/signing_detail.rs +++ b/engine/src/multisig/client/signing/signing_detail.rs @@ -8,10 +8,7 @@ use cf_primitives::AuthorityCount; use zeroize::Zeroize; -use crate::multisig::{ - crypto::{CryptoScheme, ECPoint, ECScalar, KeyShare, Rng}, - SigningPayload, -}; +use crate::multisig::crypto::{CryptoScheme, ECPoint, ECScalar, KeyShare, Rng}; use sha2::{Digest, Sha256}; @@ -117,27 +114,27 @@ fn gen_rho_i( type SigningResponse

= LocalSig3

; /// Generate binding values for each party given their previously broadcast commitments -fn generate_bindings( - payload: &SigningPayload, - commitments: &BTreeMap>, +fn generate_bindings( + payload: &C::SigningPayload, + commitments: &BTreeMap>, all_idxs: &BTreeSet, -) -> BTreeMap { +) -> BTreeMap::Scalar> { all_idxs .iter() - .map(|idx| (*idx, gen_rho_i(*idx, &payload.0, commitments, all_idxs))) + .map(|idx| (*idx, gen_rho_i(*idx, payload.as_ref(), commitments, all_idxs))) .collect() } /// Generate local signature/response (shard). See step 5 in Figure 3 (page 15). pub fn generate_local_sig( - payload: &SigningPayload, + payload: &C::SigningPayload, key: &KeyShare, nonces: &SecretNoncePair, commitments: &BTreeMap>, own_idx: AuthorityCount, all_idxs: &BTreeSet, ) -> SigningResponse { - let bindings = generate_bindings(payload, commitments, all_idxs); + let bindings = generate_bindings::(payload, commitments, all_idxs); // This is `R` in a Schnorr signature let group_commitment = gen_group_commitment(commitments, &bindings); @@ -163,7 +160,7 @@ pub fn generate_schnorr_response( pubkey: C::Point, nonce_commitment: C::Point, nonce: ::Scalar, - payload: &SigningPayload, + payload: &C::SigningPayload, ) -> ::Scalar { let challenge = C::build_challenge(pubkey, nonce_commitment, payload); @@ -174,14 +171,14 @@ pub fn generate_schnorr_response( /// (aggregate) signature given that no party misbehaved. Otherwise /// return the misbehaving parties. pub fn aggregate_signature( - payload: &SigningPayload, + payload: &C::SigningPayload, signer_idxs: &BTreeSet, agg_pubkey: C::Point, pubkeys: &BTreeMap, commitments: &BTreeMap>, responses: &BTreeMap>, ) -> Result> { - let bindings = generate_bindings(payload, commitments, signer_idxs); + let bindings = generate_bindings::(payload, commitments, signer_idxs); let group_commitment = gen_group_commitment(commitments, &bindings); @@ -228,7 +225,10 @@ mod tests { use super::*; - use crate::multisig::crypto::eth::{EthSigning, Point, Scalar}; + use crate::multisig::{ + crypto::eth::{EthSigning, Point, Scalar}, + eth::EthSigningPayload, + }; const SECRET_KEY: &str = "fbcb47bc85b881e0dfb31c872d4e06848f80530ccbd18fc016a27c4a744d0eba"; const NONCE_KEY: &str = "d51e13c68bf56155a83e50fd9bc840e2a1847fb9b49cd206a577ecd1cd15e285"; @@ -243,7 +243,7 @@ mod tests { // Given the signing key, nonce and message hash, check that // sigma (signature response) is correct and matches the expected // (by the KeyManager contract) value - let payload = SigningPayload(hex::decode(MESSAGE_HASH).unwrap().to_vec()); + let payload = EthSigningPayload(hex::decode(MESSAGE_HASH).unwrap().try_into().unwrap()); let nonce = Scalar::from_hex(NONCE_KEY); let commitment = Point::from_scalar(&nonce); @@ -276,4 +276,37 @@ mod tests { &response, )); } + + #[test] + fn bindings_are_backwards_compatible() { + use rand_legacy::SeedableRng; + // The seed must not change or the test will break. + let mut rng = Rng::from_seed([0; 32]); + + let payload = EthSigningPayload(hex::decode(MESSAGE_HASH).unwrap().try_into().unwrap()); + let idxs = BTreeSet::from_iter(vec![1u32, 2, 3]); + let commitments: BTreeMap> = idxs + .iter() + .map(|id| { + (*id, SigningCommitment { d: Point::random(&mut rng), e: Point::random(&mut rng) }) + }) + .collect(); + + let bindings = generate_bindings::(&payload, &commitments, &idxs); + + // Compare the generated bindings with existing bindings to confirm that the hashing in + // `gen_rho_i` has not changed. + assert_eq!( + hex::encode(bindings.get(&1u32).unwrap().as_bytes()), + "d21e9745014dedea06fc653b93845b17c20737ef9fe1bac189c70ffb2794250a" + ); + assert_eq!( + hex::encode(bindings.get(&2u32).unwrap().as_bytes()), + "87c25a1056df0e55a359468f76822a7244232e8a339700d24293d7ea3547aad9" + ); + assert_eq!( + hex::encode(bindings.get(&3u32).unwrap().as_bytes()), + "d74a3892851b2f4114fb58cd0a7813dec65a7b5c1bfe6c512091e627a92f512d" + ); + } } diff --git a/engine/src/multisig/client/signing/signing_stages.rs b/engine/src/multisig/client/signing/signing_stages.rs index a6a72debef..b3f8ee08ad 100644 --- a/engine/src/multisig/client/signing/signing_stages.rs +++ b/engine/src/multisig/client/signing/signing_stages.rs @@ -31,15 +31,12 @@ type SigningStageResult = StageResult>; /// and collect those from all other parties pub struct AwaitCommitments1 { common: CeremonyCommon, - signing_common: SigningStateCommonInfo, + signing_common: SigningStateCommonInfo, nonces: Box>, } impl AwaitCommitments1 { - pub fn new( - mut common: CeremonyCommon, - signing_common: SigningStateCommonInfo, - ) -> Self { + pub fn new(mut common: CeremonyCommon, signing_common: SigningStateCommonInfo) -> Self { let nonces = SecretNoncePair::sample_random(&mut common.rng); AwaitCommitments1 { common, signing_common, nonces } @@ -67,7 +64,7 @@ impl BroadcastStageProcessor> let processor = VerifyCommitmentsBroadcast2:: { common: self.common.clone(), - signing_common: self.signing_common.clone(), + signing_common: self.signing_common, nonces: self.nonces, commitments: messages, }; @@ -83,7 +80,7 @@ impl BroadcastStageProcessor> /// Stage 2: Verifying data broadcast during stage 1 struct VerifyCommitmentsBroadcast2 { common: CeremonyCommon, - signing_common: SigningStateCommonInfo, + signing_common: SigningStateCommonInfo, // Our nonce pair generated in the previous stage nonces: Box>, // Public nonce commitments collected in the previous stage @@ -139,7 +136,7 @@ impl BroadcastStageProcessor> /// Stage 3: Generating and broadcasting signature response shares struct LocalSigStage3 { common: CeremonyCommon, - signing_common: SigningStateCommonInfo, + signing_common: SigningStateCommonInfo, // Our nonce pair generated in the previous stage nonces: Box>, // Public nonce commitments (verified) @@ -184,7 +181,7 @@ impl BroadcastStageProcessor> ) -> SigningStageResult { let processor = VerifyLocalSigsBroadcastStage4:: { common: self.common.clone(), - signing_common: self.signing_common.clone(), + signing_common: self.signing_common, commitments: self.commitments, local_sigs: messages, }; @@ -198,7 +195,7 @@ impl BroadcastStageProcessor> /// Stage 4: Verifying the broadcasting of signature shares struct VerifyLocalSigsBroadcastStage4 { common: CeremonyCommon, - signing_common: SigningStateCommonInfo, + signing_common: SigningStateCommonInfo, /// Nonce commitments from all parties (verified to be correctly broadcast) commitments: BTreeMap>, /// Signature shares sent to us (NOT verified to be correctly broadcast) diff --git a/engine/src/multisig/client/signing/tests.rs b/engine/src/multisig/client/signing/tests.rs index 67822f7cc4..51fac7f6d3 100644 --- a/engine/src/multisig/client/signing/tests.rs +++ b/engine/src/multisig/client/signing/tests.rs @@ -15,7 +15,6 @@ use crate::multisig::{ }, crypto::polkadot::PolkadotSigning, eth::EthSigning, - tests::fixtures::SIGNING_PAYLOAD, CryptoScheme, Rng, }; @@ -111,7 +110,7 @@ async fn should_sign_with_all_parties() { DEFAULT_SIGNING_CEREMONY_ID, key_id, key_data, - SIGNING_PAYLOAD.clone(), + C::signing_payload_for_test(), Rng::from_seed(DEFAULT_SIGNING_SEED), ); diff --git a/engine/src/multisig/crypto.rs b/engine/src/multisig/crypto.rs index 13bcba4725..bd5e0b7c40 100644 --- a/engine/src/multisig/crypto.rs +++ b/engine/src/multisig/crypto.rs @@ -10,12 +10,12 @@ use generic_array::{typenum::Unsigned, ArrayLength}; use num_derive::FromPrimitive; use zeroize::{DefaultIsZeroes, ZeroizeOnDrop}; -use std::fmt::Debug; +use std::fmt::{Debug, Display}; use generic_array::GenericArray; use serde::{Deserialize, Serialize}; -use super::{KeyId, SigningPayload}; +use super::KeyId; /// The db uses a static length prefix, that must include the keygen data prefix and the chain tag pub const CHAIN_TAG_SIZE: usize = std::mem::size_of::(); @@ -81,15 +81,10 @@ pub trait ECPoint: } } -pub trait Verifiable { - fn verify(&self, key_id: &KeyId, payload: &SigningPayload) -> anyhow::Result<()>; -} - pub trait CryptoScheme: 'static { type Point: ECPoint; - type Signature: Verifiable - + Debug + type Signature: Debug + Clone + PartialEq + serde::Serialize @@ -98,6 +93,7 @@ pub trait CryptoScheme: 'static { + Send; type AggKey; + type SigningPayload: Display + Debug + Sync + Send + Clone + PartialEq + Eq + AsRef<[u8]>; /// Friendly name of the scheme used for logging const NAME: &'static str; @@ -114,7 +110,7 @@ pub trait CryptoScheme: 'static { fn build_challenge( pubkey: Self::Point, nonce_commitment: Self::Point, - payload: &SigningPayload, + payload: &Self::SigningPayload, ) -> ::Scalar; /// Build challenge response using our key share @@ -135,6 +131,12 @@ pub trait CryptoScheme: 'static { signature_response: &::Scalar, ) -> bool; + fn verify_signature( + signature: &Self::Signature, + key_id: &KeyId, + payload: &Self::SigningPayload, + ) -> anyhow::Result<()>; + fn agg_key(pubkey: &Self::Point) -> Self::AggKey; // Only relevant for ETH contract keys, which is the only @@ -142,6 +144,9 @@ pub trait CryptoScheme: 'static { fn is_pubkey_compatible(_pubkey: &Self::Point) -> bool { true } + + #[cfg(test)] + fn signing_payload_for_test() -> Self::SigningPayload; } pub trait ECScalar: diff --git a/engine/src/multisig/crypto/eth.rs b/engine/src/multisig/crypto/eth.rs index e00c4bbe27..b6427ab6bd 100644 --- a/engine/src/multisig/crypto/eth.rs +++ b/engine/src/multisig/crypto/eth.rs @@ -1,6 +1,6 @@ -use crate::multisig::{crypto::ECScalar, SigningPayload}; +use crate::multisig::crypto::ECScalar; -use super::{ChainTag, CryptoScheme, ECPoint, Verifiable}; +use super::{ChainTag, CryptoScheme, ECPoint}; // NOTE: for now, we re-export these to make it // clear that these a the primitives used by ethereum. @@ -27,32 +27,29 @@ impl From for cf_chains::eth::SchnorrVerificationComponents } } -impl Verifiable for EthSchnorrSignature { - fn verify( - &self, - key_id: &crate::multisig::KeyId, - payload: &SigningPayload, - ) -> anyhow::Result<()> { - // Get the aggkey - let pk_ser: &[u8; 33] = key_id.0[..].try_into().unwrap(); - let agg_key = cf_chains::eth::AggKey::from_pubkey_compressed(*pk_ser); +/// Ethereum crypto scheme (as defined by the Key Manager contract) +pub struct EthSigning {} - // Verify the signature with the aggkey - agg_key - .verify(payload.0.as_slice().try_into().unwrap(), &self.clone().into()) - .map_err(|e| anyhow::anyhow!("Failed to verify signature: {:?}", e))?; +#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Hash, Eq)] +pub struct EthSigningPayload(pub [u8; 32]); - Ok(()) +impl std::fmt::Display for EthSigningPayload { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", hex::encode(&self.0)) } } -/// Ethereum crypto scheme (as defined by the Key Manager contract) -pub struct EthSigning {} +impl AsRef<[u8]> for EthSigningPayload { + fn as_ref(&self) -> &[u8] { + &self.0 + } +} impl CryptoScheme for EthSigning { type Point = Point; type Signature = EthSchnorrSignature; type AggKey = cf_chains::eth::AggKey; + type SigningPayload = EthSigningPayload; const NAME: &'static str = "Ethereum"; const CHAIN_TAG: ChainTag = ChainTag::Ethereum; @@ -65,15 +62,13 @@ impl CryptoScheme for EthSigning { fn build_challenge( pubkey: Self::Point, nonce_commitment: Self::Point, - payload: &SigningPayload, + payload: &Self::SigningPayload, ) -> Scalar { use crate::eth::utils::pubkey_to_eth_addr; use cf_chains::eth::AggKey; - let msg_hash: &[u8; 32] = payload.0.as_slice().try_into().unwrap(); - let e = AggKey::from_pubkey_compressed(pubkey.get_element().serialize()) - .message_challenge(msg_hash, &pubkey_to_eth_addr(nonce_commitment.get_element())); + .message_challenge(&payload.0, &pubkey_to_eth_addr(nonce_commitment.get_element())); Scalar::from_bytes_mod_order(&e) } @@ -96,6 +91,23 @@ impl CryptoScheme for EthSigning { Point::from_scalar(signature_response) == *commitment - (*y_i) * challenge * lambda_i } + fn verify_signature( + signature: &Self::Signature, + key_id: &crate::multisig::KeyId, + payload: &Self::SigningPayload, + ) -> anyhow::Result<()> { + // Get the aggkey + let pk_ser: &[u8; 33] = key_id.0[..].try_into().unwrap(); + let agg_key = cf_chains::eth::AggKey::from_pubkey_compressed(*pk_ser); + + // Verify the signature with the aggkey + agg_key + .verify(&payload.0, &signature.clone().into()) + .map_err(|e| anyhow::anyhow!("Failed to verify signature: {:?}", e))?; + + Ok(()) + } + fn agg_key(pubkey: &Self::Point) -> Self::AggKey { let pk = pubkey.get_element(); cf_chains::eth::AggKey::from_pubkey_compressed(pk.serialize()) @@ -111,4 +123,9 @@ impl CryptoScheme for EthSigning { x < half_order } + + #[cfg(test)] + fn signing_payload_for_test() -> Self::SigningPayload { + EthSigningPayload("Chainflip:Chainflip:Chainflip:01".as_bytes().try_into().unwrap()) + } } diff --git a/engine/src/multisig/crypto/polkadot.rs b/engine/src/multisig/crypto/polkadot.rs index 15725947a6..353188791d 100644 --- a/engine/src/multisig/crypto/polkadot.rs +++ b/engine/src/multisig/crypto/polkadot.rs @@ -1,6 +1,6 @@ -use crate::multisig::SigningPayload; +use anyhow::Result; -use super::{curve25519_ristretto::Point, ChainTag, CryptoScheme, ECPoint, Verifiable}; +use super::{curve25519_ristretto::Point, ChainTag, CryptoScheme, ECPoint}; use cf_chains::dot::PolkadotPublicKey; use schnorrkel::context::{SigningContext, SigningTranscript}; use serde::{Deserialize, Serialize}; @@ -13,6 +13,30 @@ const SIGNING_CTX: &[u8] = b"substrate"; #[derive(Debug, Clone, PartialEq, Eq)] pub struct PolkadotSignature(schnorrkel::Signature); +#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Hash, Eq)] +pub struct PolkadotSigningPayload(Vec); + +impl std::fmt::Display for PolkadotSigningPayload { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", hex::encode(&self.0)) + } +} + +impl AsRef<[u8]> for PolkadotSigningPayload { + fn as_ref(&self) -> &[u8] { + self.0.as_ref() + } +} + +impl PolkadotSigningPayload { + pub fn new(payload: Vec) -> Result { + if payload.is_empty() || payload.len() > 256 { + anyhow::bail!("Invalid payload size"); + } + Ok(PolkadotSigningPayload(payload)) + } +} + impl From for cf_chains::dot::PolkadotSignature { fn from(cfe_sig: PolkadotSignature) -> Self { sp_core::sr25519::Signature(cfe_sig.0.to_bytes()) @@ -41,26 +65,11 @@ impl<'de> Deserialize<'de> for PolkadotSignature { } } -impl Verifiable for PolkadotSignature { - fn verify( - &self, - key_id: &crate::multisig::KeyId, - payload: &SigningPayload, - ) -> anyhow::Result<()> { - let public_key = schnorrkel::PublicKey::from_bytes(&key_id.0).expect("invalid public key"); - - let context = schnorrkel::signing_context(SIGNING_CTX); - - public_key - .verify(context.bytes(payload.0.as_slice()), &self.0) - .map_err(anyhow::Error::msg) - } -} - impl CryptoScheme for PolkadotSigning { type Point = Point; type Signature = PolkadotSignature; type AggKey = cf_chains::dot::PolkadotPublicKey; + type SigningPayload = PolkadotSigningPayload; const NAME: &'static str = "Polkadot"; const CHAIN_TAG: ChainTag = ChainTag::Polkadot; @@ -84,7 +93,7 @@ impl CryptoScheme for PolkadotSigning { fn build_challenge( pubkey: Self::Point, nonce_commitment: Self::Point, - payload: &SigningPayload, + payload: &Self::SigningPayload, ) -> ::Scalar { // NOTE: This computation is copied from schnorrkel's // source code (since it is the "source of truth") @@ -111,6 +120,20 @@ impl CryptoScheme for PolkadotSigning { Point::from_scalar(signature_response) == *commitment + (*y_i) * challenge * lambda_i } + fn verify_signature( + signature: &Self::Signature, + key_id: &crate::multisig::KeyId, + payload: &Self::SigningPayload, + ) -> anyhow::Result<()> { + let public_key = schnorrkel::PublicKey::from_bytes(&key_id.0).expect("invalid public key"); + + let context = schnorrkel::signing_context(SIGNING_CTX); + + public_key + .verify(context.bytes(payload.0.as_slice()), &signature.0) + .map_err(anyhow::Error::msg) + } + fn agg_key(pubkey: &Self::Point) -> Self::AggKey { PolkadotPublicKey(sp_core::sr25519::Public::from_raw( pubkey.get_element().compress().to_bytes(), @@ -125,6 +148,11 @@ impl CryptoScheme for PolkadotSigning { // "Response" is computed as done in schnorrkel challenge * private_key + nonce } + + #[cfg(test)] + fn signing_payload_for_test() -> Self::SigningPayload { + PolkadotSigningPayload::new(vec![1_u8; 256]).unwrap() + } } // Check that our signature generation results in @@ -143,7 +171,7 @@ fn signature_should_be_valid() { let public_key = Point::from_scalar(&secret_key); // Message to sign - let payload = SigningPayload(vec![b't'; 256]); + let payload = PolkadotSigning::signing_payload_for_test(); let signature = { // Pick random nonce and commit to it diff --git a/engine/src/multisig/mod.rs b/engine/src/multisig/mod.rs index e73daef731..23be84fde1 100644 --- a/engine/src/multisig/mod.rs +++ b/engine/src/multisig/mod.rs @@ -9,9 +9,6 @@ pub mod db; pub use crypto::{eth, polkadot, ChainTag, CryptoScheme, Rng}; -#[cfg(test)] -mod tests; - use anyhow::Result; use cf_primitives::CeremonyId; @@ -30,15 +27,6 @@ pub use db::PersistentKeyDB; use self::client::key_store::KeyStore; -#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Hash, Eq)] -pub struct SigningPayload(pub Vec); - -impl std::fmt::Display for SigningPayload { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", hex::encode(&self.0)) - } -} - /// Public key compressed (33 bytes = 32 bytes + a y parity byte) #[derive(Serialize, Deserialize, PartialEq, Eq, Hash, Debug, Clone)] pub struct KeyId(pub Vec); // TODO: Use [u8; 33] not a Vec diff --git a/engine/src/multisig/tests/fixtures.rs b/engine/src/multisig/tests/fixtures.rs deleted file mode 100644 index 075c003d9e..0000000000 --- a/engine/src/multisig/tests/fixtures.rs +++ /dev/null @@ -1,8 +0,0 @@ -use lazy_static::lazy_static; - -use crate::multisig::SigningPayload; - -lazy_static! { - pub static ref MESSAGE: Vec = "Chainflip:Chainflip:Chainflip:01".as_bytes().to_vec(); - pub static ref SIGNING_PAYLOAD: SigningPayload = SigningPayload(MESSAGE.clone()); -} diff --git a/engine/src/multisig/tests/mod.rs b/engine/src/multisig/tests/mod.rs deleted file mode 100644 index 1d7e992f42..0000000000 --- a/engine/src/multisig/tests/mod.rs +++ /dev/null @@ -1 +0,0 @@ -pub(crate) mod fixtures; diff --git a/engine/src/state_chain_observer/sc_observer/mod.rs b/engine/src/state_chain_observer/sc_observer/mod.rs index 40bafb50eb..e1c2207661 100644 --- a/engine/src/state_chain_observer/sc_observer/mod.rs +++ b/engine/src/state_chain_observer/sc_observer/mod.rs @@ -33,7 +33,7 @@ use crate::{ client::{KeygenFailureReason, MultisigClientApi}, eth::EthSigning, polkadot::PolkadotSigning, - CryptoScheme, KeyId, SigningPayload, + CryptoScheme, KeyId, }, p2p::{PeerInfo, PeerUpdate}, state_chain_observer::client::{extrinsic_api::ExtrinsicApi, storage_api::StorageApi}, @@ -103,7 +103,7 @@ async fn handle_signing_request<'a, StateChainClient, MultisigClient, C, I>( ceremony_id: CeremonyId, key_id: KeyId, signers: BTreeSet, - payload: SigningPayload, + payload: C::SigningPayload, logger: slog::Logger, ) where MultisigClient: MultisigClientApi, @@ -442,7 +442,7 @@ where ceremony_id, KeyId(key_id), signatories, - SigningPayload(payload.0.to_vec()), + crate::multisig::eth::EthSigningPayload(payload.0), logger.clone(), ).await; } @@ -466,7 +466,8 @@ where ceremony_id, KeyId(key_id), signatories, - SigningPayload(payload.0.to_vec()), + crate::multisig::polkadot::PolkadotSigningPayload::new(payload.0) + .expect("Payload should be correct size"), logger.clone(), ).await; } diff --git a/engine/src/state_chain_observer/sc_observer/tests.rs b/engine/src/state_chain_observer/sc_observer/tests.rs index d372e4442d..9fc85a9dd7 100644 --- a/engine/src/state_chain_observer/sc_observer/tests.rs +++ b/engine/src/state_chain_observer/sc_observer/tests.rs @@ -1369,7 +1369,7 @@ where let logger = new_test_logger(); let first_ceremony_id = 1; let key_id = crate::multisig::KeyId(vec![0u8; 32]); - let payload = crate::multisig::SigningPayload(vec![0u8; 32]); + let payload = C::signing_payload_for_test(); let our_account_id = AccountId32::new([0; 32]); let not_our_account_id = AccountId32::new([1u8; 32]); assert_ne!(our_account_id, not_our_account_id);