diff --git a/src/common.rs b/src/common.rs index 89e3570..3f96234 100644 --- a/src/common.rs +++ b/src/common.rs @@ -214,6 +214,11 @@ impl TupleProof { } } +/// Check that the PolyCommitment is properly signed and has the correct degree polynomial +pub fn check_public_shares(poly_comm: &PolyCommitment, threshold: usize) -> bool { + poly_comm.verify() && poly_comm.poly.len() == threshold +} + /// An implementation of p256k1's MultiMult trait that allows fast checking of DKG private shares /// We convert a set of checked polynomial evaluations into a single giant multimult /// These evaluations take the form of s * G == \Sum{k=0}{T+1}(a_k * x^k) where the a vals are the coeffs of the polys diff --git a/src/state_machine/coordinator/fire.rs b/src/state_machine/coordinator/fire.rs index acf8751..87c0d7e 100644 --- a/src/state_machine/coordinator/fire.rs +++ b/src/state_machine/coordinator/fire.rs @@ -3,7 +3,7 @@ use std::{collections::BTreeMap, time::Instant}; use tracing::{debug, error, info, warn}; use crate::{ - common::{PolyCommitment, PublicNonce, Signature, SignatureShare}, + common::{check_public_shares, PolyCommitment, PublicNonce, Signature, SignatureShare}, compute, curve::{ point::{Point, G}, @@ -568,7 +568,7 @@ impl Coordinator { } let mut dkg_failures = HashMap::new(); - + let threshold: usize = self.config.threshold.try_into().unwrap(); if self.dkg_wait_signer_ids.is_empty() { // if there are any errors, mark signers malicious and retry for (signer_id, dkg_end) in &self.dkg_end_messages { @@ -585,7 +585,7 @@ impl Coordinator { let dkg_public_shares = &self.dkg_public_shares[bad_signer_id]; let mut bad_party_ids = Vec::new(); for (party_id, comm) in &dkg_public_shares.comms { - if !comm.verify() { + if !check_public_shares(comm, threshold) { bad_party_ids.push(party_id); } } @@ -597,6 +597,9 @@ impl Coordinator { } else { warn!("Signer {} reported BadPublicShares from {}, mark {} as malicious", signer_id, bad_signer_id, bad_signer_id); self.malicious_dkg_signer_ids.insert(*bad_signer_id); + + // save legitimate failures to return to caller + dkg_failures.insert(*signer_id, dkg_failure.clone()); } } } @@ -677,12 +680,14 @@ impl Coordinator { } else { warn!("Signer {} reported BadPrivateShare from {}, mark {} as malicious", signer_id, bad_signer_id, bad_signer_id); self.malicious_dkg_signer_ids.insert(*bad_signer_id); + + // save legitimate failures to return to caller + dkg_failures.insert(*signer_id, dkg_failure.clone()); } } } _ => (), } - dkg_failures.insert(*signer_id, dkg_failure.clone()); } } if dkg_failures.is_empty() { @@ -2833,4 +2838,114 @@ pub mod test { fn bad_signature_share_request_v2() { bad_signature_share_request::, v2::Signer>(5, 2); } + + #[test] + fn one_signer_bad_threshold_v1() { + one_signer_bad_threshold::(); + } + + #[test] + fn one_signer_bad_threshold_v2() { + one_signer_bad_threshold::(); + } + + fn one_signer_bad_threshold() { + let mut rng = create_rng(); + let (mut coordinators, mut signers) = + setup::, SignerType>(10, 1); + + // persist one signer, change the threshold, reset polys + let mut state = signers[0].save(); + + state.threshold -= 1; + state.signer.threshold -= 1; + + signers[0] = Signer::::load(&state); + + signers[0].signer.reset_polys(&mut rng); + + // We have started a dkg round + let message = coordinators.first_mut().unwrap().start_dkg_round().unwrap(); + assert!(coordinators + .first_mut() + .unwrap() + .get_aggregate_public_key() + .is_none()); + assert_eq!( + coordinators.first_mut().unwrap().get_state(), + State::DkgPublicGather + ); + + // Send the DKG Begin 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()); + for coordinator in coordinators.iter() { + assert_eq!(coordinator.get_state(), State::DkgPrivateGather); + } + + assert_eq!(outbound_messages.len(), 1); + match &outbound_messages[0].msg { + Message::DkgPrivateBegin(_) => {} + _ => { + panic!("Expected DkgPrivateBegin message"); + } + } + + // Send the DKG Private Begin message to all signers and share their responses with the coordinator and signers + let (outbound_messages, operation_results) = + feedback_messages(&mut coordinators, &mut signers, &outbound_messages); + assert_eq!(operation_results.len(), 0); + assert_eq!(outbound_messages.len(), 1); + match &outbound_messages[0].msg { + Message::DkgEndBegin(_) => {} + _ => { + panic!("Expected DkgEndBegin message"); + } + } + + // Send the DkgEndBegin message to all signers and share their responses with the coordinator and signers + let (outbound_messages, operation_results) = + feedback_messages(&mut coordinators, &mut signers, &outbound_messages); + assert_eq!(outbound_messages.len(), 0); + assert_eq!(operation_results.len(), 1); + match &operation_results[0] { + OperationResult::DkgError(DkgError::DkgEndFailure(failure_map)) => { + for i in 1..10 { + match failure_map.get(&i) { + Some(DkgFailure::BadPublicShares(set)) => { + if set.len() != 1 { + panic!( + "signer {} should have reported a single BadPublicShares", + i + ); + } else if !set.contains(&0) { + panic!( + "signer {} should have reported BadPublicShares from signer 0", + i + ); + } + } + Some(failure) => { + panic!("signer {} should have reported BadPublicShares, instead reported {:?}", i, failure); + } + None => { + panic!("signer {} should have reported BadPublicShares", i); + } + } + } + + match failure_map.get(&0) { + Some(failure) => { + panic!("Coordinator should not have passed along incorrect failure {:?} from signer 0", failure); + } + None => {} + } + } + result => panic!( + "Expected OperationResult::DkgError(DkgError::DkgEndFailure), got {:?}", + &result + ), + } + } } diff --git a/src/state_machine/signer/mod.rs b/src/state_machine/signer/mod.rs index 6b87b91..e45128d 100644 --- a/src/state_machine/signer/mod.rs +++ b/src/state_machine/signer/mod.rs @@ -4,7 +4,7 @@ use std::collections::{BTreeMap, BTreeSet}; use tracing::{debug, info, trace, warn}; use crate::{ - common::{PolyCommitment, PublicNonce, TupleProof}, + common::{check_public_shares, PolyCommitment, PublicNonce, TupleProof}, curve::{ point::{Compressed, Point, G}, scalar::Scalar, @@ -357,7 +357,7 @@ impl Signer { for signer_id in &dkg_end_begin.signer_ids { if let Some(shares) = self.dkg_public_shares.get(signer_id) { for (party_id, comm) in shares.comms.iter() { - if comm.poly.len() != threshold || !comm.verify() { + if !check_public_shares(comm, threshold) { bad_public_shares.insert(*signer_id); } else { self.commitments.insert(*party_id, comm.clone()); diff --git a/src/v1.rs b/src/v1.rs index fa1a66a..0633c45 100644 --- a/src/v1.rs +++ b/src/v1.rs @@ -5,7 +5,10 @@ use rand_core::{CryptoRng, RngCore}; use tracing::warn; use crate::{ - common::{CheckPrivateShares, Nonce, PolyCommitment, PublicNonce, Signature, SignatureShare}, + common::{ + check_public_shares, CheckPrivateShares, Nonce, PolyCommitment, PublicNonce, Signature, + SignatureShare, + }, compute, curve::{ point::{Point, G}, @@ -139,7 +142,7 @@ impl Party { let threshold: usize = self.threshold.try_into()?; let mut bad_ids = Vec::new(); //: Vec = polys for (i, comm) in public_shares.iter() { - if comm.poly.len() != threshold || !comm.verify() { + if !check_public_shares(comm, threshold) { bad_ids.push(*i); } else { self.group_key += comm.poly[0]; @@ -420,7 +423,7 @@ impl traits::Aggregator for Aggregator { let threshold = self.threshold.try_into()?; let mut bad_poly_commitments = Vec::new(); for (_id, comm) in comms { - if comm.poly.len() != threshold || !comm.verify() { + if !check_public_shares(comm, threshold) { bad_poly_commitments.push(comm.id.id); } } diff --git a/src/v2.rs b/src/v2.rs index a24e235..2faf5a2 100644 --- a/src/v2.rs +++ b/src/v2.rs @@ -5,7 +5,7 @@ use rand_core::{CryptoRng, RngCore}; use tracing::warn; use crate::{ - common::{Nonce, PolyCommitment, PublicNonce, Signature, SignatureShare}, + common::{check_public_shares, Nonce, PolyCommitment, PublicNonce, Signature, SignatureShare}, compute, curve::{ point::{Point, G}, @@ -112,7 +112,7 @@ impl Party { let mut bad_ids = Vec::new(); for (i, comm) in public_shares.iter() { - if comm.poly.len() != threshold || !comm.verify() { + if !check_public_shares(comm, threshold) { bad_ids.push(*i); } else { self.group_key += comm.poly[0]; @@ -411,7 +411,7 @@ impl traits::Aggregator for Aggregator { let threshold: usize = self.threshold.try_into()?; let mut bad_poly_commitments = Vec::new(); for (_id, comm) in comms { - if comm.poly.len() != threshold || !comm.verify() { + if !check_public_shares(comm, threshold) { bad_poly_commitments.push(comm.id.id); } }