allow identifying culprit in error returned by dkg::part3() (#728)

frost-core/CHANGELOG.md

@@ -20,6 +20,9 @@ Entries are listed in reverse chronological order.
   individual signature shares. This is not required for regular FROST usage but
   might useful in certain situations where it is desired to verify each
   individual signature share before aggregating the signature.
+* It is now possible to identify the culprit in `frost_core::keys::dkg::part3()`
+  if an invalid secret share was sent by one of the participants (by calling
+  frost_core::Error<C>::culprit()`).
 
 ## 2.0.0-rc.0
 

frost-core/src/error.rs

@@ -74,7 +74,11 @@ pub enum Error<C: Ciphersuite> {
     },
     /// Secret share verification failed.
     #[error("Invalid secret share.")]
-    InvalidSecretShare,
+    InvalidSecretShare {
+        /// The identifier of the signer whose secret share validation failed,
+        /// if possible to identify.
+        culprit: Option<Identifier<C>>,
+    },
     /// Round 1 package not found for Round 2 participant.
     #[error("Round 1 package not found for Round 2 participant.")]
     PackageNotFound,
@@ -132,8 +136,10 @@ where
             | Error::InvalidProofOfKnowledge {
                 culprit: identifier,
             } => Some(*identifier),
-            Error::InvalidSecretShare
-            | Error::InvalidMinSigners
+            Error::InvalidSecretShare {
+                culprit: identifier,
+            } => *identifier,
+            Error::InvalidMinSigners
             | Error::InvalidMaxSigners
             | Error::InvalidCoefficients
             | Error::MalformedIdentifier

frost-core/src/keys.rs

@@ -423,7 +423,18 @@ where
         let result = evaluate_vss(self.identifier, &self.commitment);
 
         if !(f_result == result) {
-            return Err(Error::InvalidSecretShare);
+            // The culprit needs to be identified by the caller if needed,
+            // because this function is called in two different contexts:
+            // - after trusted dealer key generation, by the participant who
+            //   receives the SecretShare. In that case it does not make sense
+            //   to identify themselves as the culprit, since the issue was with
+            //   the Coordinator or in the communication.
+            // - during DKG, where a "fake" SecretShare is built just to reuse
+            //   the verification logic and it does make sense to identify the
+            //   culprit. Note that in this case, self.identifier is the caller's
+            //   identifier and not the culprit's, so we couldn't identify
+            //   the culprit inside this function anyway.
+            return Err(Error::InvalidSecretShare { culprit: None });
         }
 
         Ok((

frost-core/src/keys/dkg.rs

@@ -525,7 +525,16 @@ pub fn part3<C: Ciphersuite>(
         };
 
         // Verify the share. We don't need the result.
-        let _ = secret_share.verify()?;
+        // Identify the culprit if an InvalidSecretShare error is returned.
+        let _ = secret_share.verify().map_err(|e| {
+            if let Error::InvalidSecretShare { .. } = e {
+                Error::InvalidSecretShare {
+                    culprit: Some(*sender_identifier),
+                }
+            } else {
+                e
+            }
+        })?;
 
         // Round 2, Step 3
         //

frost-core/src/tests/ciphersuite_generic.rs

@@ -4,6 +4,7 @@
 use alloc::collections::BTreeMap;
 
 use crate as frost;
+use crate::keys::SigningShare;
 use crate::round2::SignatureShare;
 use crate::{
     keys::PublicKeyPackage, Error, Field, Group, Identifier, Signature, SigningKey, SigningPackage,
@@ -499,7 +500,7 @@ where
     // for each signature before being aggregated.
     let mut pubkey_packages_by_participant = BTreeMap::new();
 
-    check_part3_different_participants(
+    check_part3_errors(
         max_signers,
         round2_secret_packages.clone(),
         received_round1_packages.clone(),
@@ -538,6 +539,33 @@ where
     check_sign(min_signers, key_packages, rng, pubkeys).unwrap()
 }
 
+/// Check for error cases related to DKG part3.
+fn check_part3_errors<C: Ciphersuite>(
+    max_signers: u16,
+    round2_secret_packages: BTreeMap<Identifier<C>, frost::keys::dkg::round2::SecretPackage<C>>,
+    received_round1_packages: BTreeMap<
+        Identifier<C>,
+        BTreeMap<Identifier<C>, frost::keys::dkg::round1::Package<C>>,
+    >,
+    received_round2_packages: BTreeMap<
+        Identifier<C>,
+        BTreeMap<Identifier<C>, frost::keys::dkg::round2::Package<C>>,
+    >,
+) {
+    check_part3_different_participants(
+        max_signers,
+        round2_secret_packages.clone(),
+        received_round1_packages.clone(),
+        received_round2_packages.clone(),
+    );
+    check_part3_corrupted_share(
+        max_signers,
+        round2_secret_packages,
+        received_round1_packages,
+        received_round2_packages,
+    );
+}
+
 /// Check that calling dkg::part3() with distinct sets of participants fail.
 fn check_part3_different_participants<C: Ciphersuite>(
     max_signers: u16,
@@ -575,6 +603,49 @@ fn check_part3_different_participants<C: Ciphersuite>(
     }
 }
 
+/// Check that calling dkg::part3() with a corrupted share fail, and the
+/// culprit is correctly identified.
+fn check_part3_corrupted_share<C: Ciphersuite>(
+    max_signers: u16,
+    round2_secret_packages: BTreeMap<Identifier<C>, frost::keys::dkg::round2::SecretPackage<C>>,
+    received_round1_packages: BTreeMap<
+        Identifier<C>,
+        BTreeMap<Identifier<C>, frost::keys::dkg::round1::Package<C>>,
+    >,
+    received_round2_packages: BTreeMap<
+        Identifier<C>,
+        BTreeMap<Identifier<C>, frost::keys::dkg::round2::Package<C>>,
+    >,
+) {
+    // For each participant, perform the third part of the DKG protocol.
+    // In practice, each participant will perform this on their own environments.
+    for participant_index in 1..=max_signers {
+        let participant_identifier = participant_index.try_into().expect("should be nonzero");
+
+        // Remove the first package from the map, and reinsert it with an unrelated
+        // Do the same for Round 2 packages
+        let mut received_round2_packages =
+            received_round2_packages[&participant_identifier].clone();
+        let culprit = *received_round2_packages.keys().next().unwrap();
+        let package = received_round2_packages.get_mut(&culprit).unwrap();
+        let one = <<C as Ciphersuite>::Group as Group>::Field::one();
+        package.signing_share = SigningShare::new(package.signing_share().to_scalar() + one);
+
+        let r = frost::keys::dkg::part3(
+            &round2_secret_packages[&participant_identifier],
+            &received_round1_packages[&participant_identifier],
+            &received_round2_packages,
+        )
+        .expect_err("Should have failed due to corrupted share");
+        assert_eq!(
+            r,
+            Error::InvalidSecretShare {
+                culprit: Some(culprit)
+            }
+        )
+    }
+}
+
 /// Test FROST signing with trusted dealer with a Ciphersuite, using specified
 /// Identifiers.
 pub fn check_sign_with_dealer_and_identifiers<C: Ciphersuite, R: RngCore + CryptoRng>(
@@ -648,10 +719,12 @@ pub fn check_sign_with_dealer_and_identifiers<C: Ciphersuite, R: RngCore + Crypt
     check_sign(min_signers, key_packages, rng, pubkeys).unwrap()
 }
 
+// Check for error cases in DKG part 2.
 fn check_part2_error<C: Ciphersuite>(
     round1_secret_package: frost::keys::dkg::round1::SecretPackage<C>,
     mut round1_packages: BTreeMap<frost::Identifier<C>, frost::keys::dkg::round1::Package<C>>,
 ) {
+    // Check if a corrupted proof of knowledge results in failure.
     let one = <<C as Ciphersuite>::Group as Group>::Field::one();
     // Corrupt a PoK
     let id = *round1_packages.keys().next().unwrap();