diff --git a/src/state_machine/coordinator/fire.rs b/src/state_machine/coordinator/fire.rs index c427c89c..acf87512 100644 --- a/src/state_machine/coordinator/fire.rs +++ b/src/state_machine/coordinator/fire.rs @@ -1281,9 +1281,10 @@ pub mod test { coordinator::{ fire::Coordinator as FireCoordinator, test::{ - check_signature_shares, coordinator_state_machine, equal_after_save_load, - feedback_messages, feedback_mutated_messages, gen_nonces, new_coordinator, - run_dkg_sign, setup, setup_with_timeouts, start_dkg_round, + bad_signature_share_request, check_signature_shares, coordinator_state_machine, + equal_after_save_load, feedback_messages, feedback_mutated_messages, + gen_nonces, new_coordinator, run_dkg_sign, setup, setup_with_timeouts, + start_dkg_round, }, Config, Coordinator as CoordinatorTrait, State, }, @@ -2822,4 +2823,14 @@ pub mod test { fn gen_nonces_v2() { gen_nonces::, v2::Signer>(5, 1); } + + #[test] + fn bad_signature_share_request_v1() { + bad_signature_share_request::, v1::Signer>(5, 2); + } + + #[test] + fn bad_signature_share_request_v2() { + bad_signature_share_request::, v2::Signer>(5, 2); + } } diff --git a/src/state_machine/coordinator/mod.rs b/src/state_machine/coordinator/mod.rs index 11f1b8d4..8d6bdfb6 100644 --- a/src/state_machine/coordinator/mod.rs +++ b/src/state_machine/coordinator/mod.rs @@ -314,8 +314,8 @@ pub mod test { net::{Message, Packet, SignatureShareResponse, SignatureType}, state_machine::{ coordinator::{Config, Coordinator as CoordinatorTrait, Error, State}, - signer::Signer, - OperationResult, PublicKeys, SignError, StateMachine, + signer::{Error as SignerError, Signer}, + Error as StateMachineError, OperationResult, PublicKeys, SignError, StateMachine, }, traits::Signer as SignerTrait, util::create_rng, @@ -540,6 +540,7 @@ pub mod test { ) -> (Vec, Vec) { feedback_mutated_messages(coordinators, signers, messages, |_signer, msgs| msgs) } + /// Helper function for feeding mutated messages back from the processor into the signing rounds and coordinators pub fn feedback_mutated_messages< Coordinator: CoordinatorTrait, @@ -551,38 +552,60 @@ pub mod test { messages: &[Packet], signer_mutator: F, ) -> (Vec, Vec) { + feedback_mutated_messages_with_errors(coordinators, signers, messages, signer_mutator) + .unwrap() + } + + /// Helper function for feeding mutated messages back from the processor into the signing rounds and coordinators + pub fn feedback_messages_with_errors( + coordinators: &mut [Coordinator], + signers: &mut [Signer], + messages: &[Packet], + ) -> Result<(Vec, Vec), StateMachineError> { + feedback_mutated_messages_with_errors(coordinators, signers, messages, |_signer, msgs| msgs) + } + + /// Helper function for feeding mutated messages back from the processor into the signing rounds and coordinators + pub fn feedback_mutated_messages_with_errors< + Coordinator: CoordinatorTrait, + SignerType: SignerTrait, + F: Fn(&Signer, Vec) -> Vec, + >( + coordinators: &mut [Coordinator], + signers: &mut [Signer], + messages: &[Packet], + signer_mutator: F, + ) -> Result<(Vec, Vec), StateMachineError> { let mut inbound_messages = vec![]; let mut feedback_messages = vec![]; let mut rng = create_rng(); for signer in signers.iter_mut() { - let outbound_messages = signer.process_inbound_messages(messages, &mut rng).unwrap(); + let outbound_messages = signer.process_inbound_messages(messages, &mut rng)?; let outbound_messages = signer_mutator(signer, outbound_messages); feedback_messages.extend_from_slice(outbound_messages.as_slice()); inbound_messages.extend(outbound_messages); } for signer in signers.iter_mut() { - let outbound_messages = signer - .process_inbound_messages(&feedback_messages, &mut rng) - .unwrap(); + let outbound_messages = + signer.process_inbound_messages(&feedback_messages, &mut rng)?; inbound_messages.extend(outbound_messages); } for coordinator in coordinators.iter_mut() { // Process all coordinator messages, but don't bother with propogating these results - let _ = coordinator.process_inbound_messages(messages).unwrap(); + let _ = coordinator.process_inbound_messages(messages)?; } let mut results = vec![]; let mut messages = vec![]; for (i, coordinator) in coordinators.iter_mut().enumerate() { - let (outbound_messages, outbound_results) = coordinator - .process_inbound_messages(&inbound_messages) - .unwrap(); + let (outbound_messages, outbound_results) = + coordinator.process_inbound_messages(&inbound_messages)?; // Only propogate a single coordinator's messages and results if i == 0 { messages.extend(outbound_messages); results.extend(outbound_results); } } - (messages, results) + Ok((messages, results)) } pub fn run_dkg( @@ -1053,4 +1076,66 @@ pub mod test { assert_ne!(share1.z_i, share2.z_i); } } + + pub fn bad_signature_share_request( + num_signers: u32, + keys_per_signer: u32, + ) { + let (mut coordinators, mut signers) = + run_dkg::(num_signers, keys_per_signer); + + let msg = "It was many and many a year ago, in a kingdom by the sea" + .as_bytes() + .to_vec(); + + // Start a signing round + let signature_type = SignatureType::Frost; + let message = coordinators + .first_mut() + .unwrap() + .start_signing_round(&msg, signature_type) + .unwrap(); + assert_eq!( + coordinators.first_mut().unwrap().get_state(), + State::NonceGather(signature_type) + ); + + // Send the message to all signers and gather responses by sharing with all other signers and coordinator + let (outbound_messages, operation_results) = + feedback_messages(&mut coordinators, &mut signers, &[message]); + assert!(operation_results.is_empty()); + assert_eq!( + coordinators.first_mut().unwrap().get_state(), + State::SigShareGather(signature_type) + ); + + assert_eq!(outbound_messages.len(), 1); + match &outbound_messages[0].msg { + Message::SignatureShareRequest(_) => {} + _ => { + panic!("Expected SignatureShareRequest message"); + } + } + + // add NonceResponses so there's more than the number of signers + let mut packet = outbound_messages[0].clone(); + if let Message::SignatureShareRequest(ref mut request) = packet.msg { + for _ in 0..1000 { + request + .nonce_responses + .push(request.nonce_responses.first().unwrap().clone()); + } + } else { + panic!("failed to match message"); + } + + // Send the SignatureShareRequest message to all signers and share their responses with the coordinator and signers + let result = feedback_messages_with_errors(&mut coordinators, &mut signers, &[packet]); + if !matches!( + result, + Err(StateMachineError::Signer(SignerError::InvalidNonceResponse)) + ) { + panic!("Should have received signer invalid nonce response error, got {result:?}"); + } + } } diff --git a/src/state_machine/mod.rs b/src/state_machine/mod.rs index 2bc266be..866eaa80 100644 --- a/src/state_machine/mod.rs +++ b/src/state_machine/mod.rs @@ -1,7 +1,7 @@ use std::collections::BTreeMap; use hashbrown::HashMap; -use thiserror::Error; +use thiserror::Error as ThisError; use crate::{ common::Signature, @@ -9,6 +9,7 @@ use crate::{ errors::AggregatorError, net::DkgFailure, state_machine::coordinator::Error as CoordinatorError, + state_machine::signer::Error as SignerError, taproot::SchnorrProof, }; @@ -20,8 +21,20 @@ pub trait StateMachine { fn can_move_to(&self, state: &S) -> Result<(), E>; } +/// All possible state machine errors +#[derive(ThisError, Debug, Clone)] +#[allow(clippy::large_enum_variant)] +pub enum Error { + /// signer error + #[error("signer error {0:?}")] + Signer(#[from] SignerError), + /// coordinator error + #[error("coordinator error {0:?}")] + Coordinator(#[from] CoordinatorError), +} + /// DKG errors -#[derive(Error, Debug, Clone)] +#[derive(ThisError, Debug, Clone)] pub enum DkgError { /// DKG public timeout #[error("DKG public timeout, waiting for {0:?}")] @@ -38,7 +51,7 @@ pub enum DkgError { } /// Sign errors -#[derive(Error, Debug, Clone)] +#[derive(ThisError, Debug, Clone)] #[allow(clippy::large_enum_variant)] pub enum SignError { /// Nonce timeout diff --git a/src/state_machine/signer/mod.rs b/src/state_machine/signer/mod.rs index 214d45e2..6b87b915 100644 --- a/src/state_machine/signer/mod.rs +++ b/src/state_machine/signer/mod.rs @@ -1,6 +1,6 @@ use hashbrown::{HashMap, HashSet}; use rand_core::{CryptoRng, RngCore}; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use tracing::{debug, info, trace, warn}; use crate::{ @@ -571,7 +571,25 @@ impl Signer { .map(|nr| nr.signer_id) .collect::>(); - debug!("Got SignatureShareRequest for signer_ids {:?}", signer_ids); + let signer_id_set = sign_request + .nonce_responses + .iter() + .map(|nr| nr.signer_id) + .collect::>(); + + if signer_ids.len() != signer_id_set.len() + || signer_id_set.is_empty() + || signer_id_set.len() > self.total_signers.try_into().unwrap() + || signer_id_set.last().unwrap() > &self.total_signers + { + warn!( + ?signer_ids, + "Got SignatureShareRequest with invalid NonceResponse" + ); + return Err(Error::InvalidNonceResponse); + } else { + debug!(?signer_ids, "Got SignatureShareRequest"); + } for signer_id in &signer_ids { if *signer_id == self.signer_id {