check for invalid NonceResponse in SignatureShareRequest

clippy prefers is_empty rather than checking for zero length

add skeleton test for bad signature share request

add feedback funcs that return general state machine errors; finish bad sig share request test and verify the state machine return invalid nonce response

state machine error now has a large variant

remove repeat tokens on log lines

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::<FireCoordinator<v2::Aggregator>, v2::Signer>(5, 1);
     }
+
+    #[test]
+    fn bad_signature_share_request_v1() {
+        bad_signature_share_request::<FireCoordinator<v1::Aggregator>, v1::Signer>(5, 2);
+    }
+
+    #[test]
+    fn bad_signature_share_request_v2() {
+        bad_signature_share_request::<FireCoordinator<v2::Aggregator>, v2::Signer>(5, 2);
+    }
 }

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<Packet>, Vec<OperationResult>) {
         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<Packet>, Vec<OperationResult>) {
+        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<Coordinator: CoordinatorTrait, SignerType: SignerTrait>(
+        coordinators: &mut [Coordinator],
+        signers: &mut [Signer<SignerType>],
+        messages: &[Packet],
+    ) -> Result<(Vec<Packet>, Vec<OperationResult>), 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<SignerType>, Vec<Packet>) -> Vec<Packet>,
+    >(
+        coordinators: &mut [Coordinator],
+        signers: &mut [Signer<SignerType>],
+        messages: &[Packet],
+        signer_mutator: F,
+    ) -> Result<(Vec<Packet>, Vec<OperationResult>), 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<Coordinator: CoordinatorTrait, SignerType: SignerTrait>(
@@ -1053,4 +1076,66 @@ pub mod test {
             assert_ne!(share1.z_i, share2.z_i);
         }
     }
+
+    pub fn bad_signature_share_request<Coordinator: CoordinatorTrait, SignerType: SignerTrait>(
+        num_signers: u32,
+        keys_per_signer: u32,
+    ) {
+        let (mut coordinators, mut signers) =
+            run_dkg::<Coordinator, SignerType>(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
+        match feedback_messages_with_errors(&mut coordinators, &mut signers, &[packet]) {
+            Err(StateMachineError::Signer(SignerError::InvalidNonceResponse)) => {}
+            error => panic!(
+                "Should have received signer invalid nonce response error, got {:?}",
+                error
+            ),
+        };
+    }
 }

src/state_machine/mod.rs

@@ -1,14 +1,15 @@
 use std::collections::BTreeMap;
 
 use hashbrown::HashMap;
-use thiserror::Error;
+use thiserror::Error as ThisError;
 
 use crate::{
     common::Signature,
     curve::{ecdsa, point::Point},
     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<S, E> {
     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

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<SignerType: SignerTrait> Signer<SignerType> {
             .map(|nr| nr.signer_id)
             .collect::<Vec<u32>>();
 
-        debug!("Got SignatureShareRequest for signer_ids {:?}", signer_ids);
+        let signer_id_set = sign_request
+            .nonce_responses
+            .iter()
+            .map(|nr| nr.signer_id)
+            .collect::<BTreeSet<u32>>();
+
+        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 {