diff --git a/src/state_machine/coordinator/mod.rs b/src/state_machine/coordinator/mod.rs index 8d6bdfb..750c718 100644 --- a/src/state_machine/coordinator/mod.rs +++ b/src/state_machine/coordinator/mod.rs @@ -1117,19 +1117,58 @@ pub mod test { } } - // add NonceResponses so there's more than the number of signers + let messages = outbound_messages.clone(); + let result = feedback_messages_with_errors(&mut coordinators, &mut signers, &messages); + assert!(result.is_ok()); + + // test request with no NonceResponses 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()); - } + request.nonce_responses.clear(); } else { panic!("failed to match message"); } - // Send the SignatureShareRequest message to all signers and share their responses with the coordinator and signers + // 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:?}"); + } + + // test request with a duplicate NonceResponse + let mut packet = outbound_messages[0].clone(); + if let Message::SignatureShareRequest(ref mut request) = packet.msg { + request + .nonce_responses + .push(request.nonce_responses[0].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:?}"); + } + + // test request with an out of range signer_id + let mut packet = outbound_messages[0].clone(); + if let Message::SignatureShareRequest(ref mut request) = packet.msg { + request.nonce_responses[0].signer_id = num_signers; + } 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, diff --git a/src/state_machine/signer/mod.rs b/src/state_machine/signer/mod.rs index e45128d..91aac7d 100644 --- a/src/state_machine/signer/mod.rs +++ b/src/state_machine/signer/mod.rs @@ -563,91 +563,75 @@ impl Signer { sign_request: &SignatureShareRequest, rng: &mut R, ) -> Result, Error> { - let mut msgs = vec![]; - - let signer_ids = sign_request - .nonce_responses - .iter() - .map(|nr| nr.signer_id) - .collect::>(); - let signer_id_set = sign_request .nonce_responses .iter() .map(|nr| nr.signer_id) .collect::>(); - if signer_ids.len() != signer_id_set.len() + // The expected usage is that Signer IDs start at zero and + // increment by one until self.total_signers - 1. So the checks + // here should be sufficient for catching empty signer ID sets, + // duplicate signer IDs, or unknown signer IDs. + let is_invalid_request = sign_request.nonce_responses.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" - ); + || signer_id_set.last() >= Some(&self.total_signers); + + if is_invalid_request { + warn!("received an invalid SignatureShareRequest"); return Err(Error::InvalidNonceResponse); - } else { - debug!(?signer_ids, "Got SignatureShareRequest"); } - for signer_id in &signer_ids { - if *signer_id == self.signer_id { - let key_ids: Vec = sign_request - .nonce_responses - .iter() - .flat_map(|nr| nr.key_ids.iter().copied()) - .collect::>(); - let nonces = sign_request - .nonce_responses - .iter() - .flat_map(|nr| nr.nonces.clone()) - .collect::>(); - let signature_shares = match sign_request.signature_type { - SignatureType::Taproot(m) => self.signer.sign_taproot( - &sign_request.message, - &signer_ids, - &key_ids, - &nonces, - m, - ), - SignatureType::Schnorr => self.signer.sign_schnorr( - &sign_request.message, - &signer_ids, - &key_ids, - &nonces, - ), - SignatureType::Frost => { - self.signer - .sign(&sign_request.message, &signer_ids, &key_ids, &nonces) - } - }; + debug!(signer_id = %self.signer_id, "received a valid SignatureShareRequest"); + + if signer_id_set.contains(&self.signer_id) { + let key_ids: Vec = sign_request + .nonce_responses + .iter() + .flat_map(|nr| nr.key_ids.iter().copied()) + .collect::>(); + let nonces = sign_request + .nonce_responses + .iter() + .flat_map(|nr| nr.nonces.clone()) + .collect::>(); + + let signer_ids = signer_id_set.into_iter().collect::>(); + let msg = &sign_request.message; + let signature_shares = match sign_request.signature_type { + SignatureType::Taproot(merkle_root) => { + self.signer + .sign_taproot(msg, &signer_ids, &key_ids, &nonces, merkle_root) + } + SignatureType::Schnorr => { + self.signer + .sign_schnorr(msg, &signer_ids, &key_ids, &nonces) + } + SignatureType::Frost => self.signer.sign(msg, &signer_ids, &key_ids, &nonces), + }; - self.signer.gen_nonces(rng); + self.signer.gen_nonces(rng); - let response = SignatureShareResponse { - dkg_id: sign_request.dkg_id, - sign_id: sign_request.sign_id, - sign_iter_id: sign_request.sign_iter_id, - signer_id: *signer_id, - signature_shares, - }; - info!( - %signer_id, - dkg_id = %sign_request.dkg_id, - sign_id = %sign_request.sign_id, - sign_iter_id = %sign_request.sign_iter_id, - "sending SignatureShareResponse" - ); - - let response = Message::SignatureShareResponse(response); - - msgs.push(response); - } else { - debug!("SignatureShareRequest for {} dropped.", signer_id); - } + let response = SignatureShareResponse { + dkg_id: sign_request.dkg_id, + sign_id: sign_request.sign_id, + sign_iter_id: sign_request.sign_iter_id, + signer_id: self.signer_id, + signature_shares, + }; + info!( + signer_id = %self.signer_id, + dkg_id = %sign_request.dkg_id, + sign_id = %sign_request.sign_id, + sign_iter_id = %sign_request.sign_iter_id, + "sending SignatureShareResponse" + ); + + Ok(vec![Message::SignatureShareResponse(response)]) + } else { + debug!(signer_id = %self.signer_id, "signer not included in SignatureShareRequest"); + Ok(Vec::new()) } - Ok(msgs) } fn dkg_begin(