Multisig: make split_spender_key/split_secret more robust

- Check for duplicate identities to make sure that max_signers is
  calculated correctly and according to expectations
- Check for hash collisions in FROST identifiers
- Removed unnecessary allocations and cloning
- Removed unused error kinds

ironfish-rust-nodejs/src/frost.rs

@@ -203,7 +203,7 @@ pub fn generate_and_split_key(
     let identities = try_deserialize_identities(identities)?;
 
     let packages =
-        split_spender_key(&spending_key, min_signers, identities.clone()).map_err(to_napi_err)?;
+        split_spender_key(&spending_key, min_signers, &identities).map_err(to_napi_err)?;
 
     let mut key_packages = Vec::with_capacity(packages.key_packages.len());
 

ironfish-rust/src/errors.rs

@@ -40,8 +40,6 @@ pub enum IronfishErrorKind {
     InvalidDiversificationPoint,
     InvalidEntropy,
     InvalidFr,
-    InvalidFrostIdentifier,
-    InvalidFrostSignatureShare,
     InvalidLanguageEncoding,
     InvalidMinersFeeTransaction,
     InvalidMintProof,

ironfish-rust/src/frost_utils/split_secret.rs

@@ -12,53 +12,84 @@ use ironfish_frost::{
     keys::PublicKeyPackage,
 };
 use rand::{CryptoRng, RngCore};
-use std::collections::HashMap;
+use std::{
+    collections::{BTreeMap, HashMap, HashSet},
+    hash::Hash,
+};
 
-use crate::errors::IronfishError;
+use crate::errors::{IronfishError, IronfishErrorKind};
 use crate::SaplingKey;
 
-pub struct SecretShareConfig {
-    pub min_signers: u16,
-    pub identities: Vec<Identity>,
-    pub spender_key: SaplingKey,
+/// Checks a sequence for duplicates; if any, the first duplicate found is returned as an error,
+/// else the whole sequence is returned as a `HashSet`
+fn find_dupes<I>(it: I) -> Result<HashSet<I::Item>, I::Item>
+where
+    I: IntoIterator,
+    I::Item: Eq + Hash,
+    I::IntoIter: ExactSizeIterator,
+{
+    let it = it.into_iter();
+    let mut set = HashSet::with_capacity(it.len());
+    for item in it {
+        if let Some(dupe) = set.replace(item) {
+            return Err(dupe);
+        }
+    }
+    Ok(set)
 }
 
 pub(crate) fn split_secret<R: RngCore + CryptoRng>(
-    config: &SecretShareConfig,
+    spender_key: &SaplingKey,
+    identities: &[Identity],
+    min_signers: u16,
     mut rng: R,
 ) -> Result<(HashMap<Identity, KeyPackage>, PublicKeyPackage), IronfishError> {
-    let mut frost_id_map = config
-        .identities
+    // Catch duplicate identities. We could in theory just remove duplicates, but doing so might
+    // give users the impression that the maximum number of signers is `identities.len()`, while
+    // it's actually lower than that.
+    let identities = find_dupes(identities.iter().cloned()).map_err(|dupe| {
+        IronfishError::new_with_source(
+            IronfishErrorKind::InvalidData,
+            format!("duplicate identity: {:?}", dupe),
+        )
+    })?;
+    let num_identities = identities.len();
+
+    let mut frost_id_map = identities
         .iter()
         .cloned()
         .map(|identity| (identity.to_frost_identifier(), identity))
-        .collect::<HashMap<_, _>>();
+        .collect::<BTreeMap<_, _>>();
+    assert_eq!(
+        num_identities,
+        frost_id_map.len(),
+        "frost identitifer collision"
+    );
+
     let frost_ids = frost_id_map.keys().cloned().collect::<Vec<_>>();
     let identifier_list = IdentifierList::Custom(&frost_ids[..]);
 
-    let secret_key = SigningKey::deserialize(config.spender_key.spend_authorizing_key.to_bytes())?;
-    let max_signers: u16 = config.identities.len().try_into()?;
+    let secret_key = SigningKey::deserialize(spender_key.spend_authorizing_key.to_bytes())?;
+    let max_signers: u16 = num_identities.try_into()?;
 
     let (shares, pubkeys) = split(
         &secret_key,
         max_signers,
-        config.min_signers,
+        min_signers,
         identifier_list,
         &mut rng,
     )?;
 
-    let mut key_packages: HashMap<_, _> = HashMap::new();
-
+    let mut key_packages = HashMap::new();
     for (frost_id, secret_share) in shares {
         let identity = frost_id_map
             .remove(&frost_id)
             .expect("frost returned an identifier that was not passed as an input");
-        let key_package = KeyPackage::try_from(secret_share.clone())?;
+        let key_package = KeyPackage::try_from(secret_share)?;
         key_packages.insert(identity, key_package);
     }
 
-    let public_key_package =
-        PublicKeyPackage::from_frost(pubkeys, config.identities.iter().cloned());
+    let public_key_package = PublicKeyPackage::from_frost(pubkeys, identities);
 
     Ok((key_packages, public_key_package))
 }
@@ -68,22 +99,16 @@ mod test {
     use super::*;
     use crate::{keys::SaplingKey, test_util::create_multisig_identities};
     use ironfish_frost::frost::{frost::keys::reconstruct, JubjubBlake2b512};
+    use rand::thread_rng;
 
     #[test]
     fn test_split_secret() {
         let identities = create_multisig_identities(10);
         let identities_length = identities.len();
 
-        let rng = rand::thread_rng();
         let key = SaplingKey::generate_key();
 
-        let config = SecretShareConfig {
-            min_signers: 2,
-            identities,
-            spender_key: key,
-        };
-
-        let (key_packages, _) = split_secret(&config, rng).unwrap();
+        let (key_packages, _) = split_secret(&key, &identities, 2, thread_rng()).unwrap();
         assert_eq!(key_packages.len(), identities_length);
 
         let key_parts: Vec<_> = key_packages.values().cloned().collect();
@@ -93,9 +118,6 @@ mod test {
 
         let scalar = signing_key.to_scalar();
 
-        assert_eq!(
-            scalar.to_bytes(),
-            config.spender_key.spend_authorizing_key.to_bytes()
-        );
+        assert_eq!(scalar.to_bytes(), key.spend_authorizing_key.to_bytes());
     }
 }

ironfish-rust/src/frost_utils/split_spender_key.rs

@@ -15,7 +15,7 @@ use crate::{
     IncomingViewKey, OutgoingViewKey, PublicAddress, SaplingKey, ViewKey,
 };
 
-use super::split_secret::{split_secret, SecretShareConfig};
+use super::split_secret::split_secret;
 
 pub struct TrustedDealerKeyPackages {
     pub public_address: PublicAddress,
@@ -30,19 +30,14 @@ pub struct TrustedDealerKeyPackages {
 pub fn split_spender_key(
     spender_key: &SaplingKey,
     min_signers: u16,
-    identities: Vec<Identity>,
+    identities: &[Identity],
 ) -> Result<TrustedDealerKeyPackages, IronfishError> {
     let group_secret_key = SaplingKey::generate_key();
 
-    let secret_config = SecretShareConfig {
-        min_signers,
-        identities,
-        spender_key: spender_key.clone(),
-    };
-
-    let (key_packages, public_key_package) = split_secret(&secret_config, thread_rng())?;
+    let (key_packages, public_key_package) =
+        split_secret(spender_key, identities, min_signers, thread_rng())?;
 
-    let proof_authorizing_key = secret_config.spender_key.sapling_proof_generation_key().nsk;
+    let proof_authorizing_key = spender_key.sapling_proof_generation_key().nsk;
 
     let authorizing_key = public_key_package.verifying_key().serialize();
     let authorizing_key = Option::from(SubgroupPoint::from_bytes(&authorizing_key))
@@ -55,7 +50,7 @@ pub fn split_spender_key(
         nullifier_deriving_key,
     };
 
-    let incoming_view_key = secret_config.spender_key.incoming_view_key().clone();
+    let incoming_view_key = spender_key.incoming_view_key().clone();
     let outgoing_view_key: OutgoingViewKey = group_secret_key.outgoing_view_key().clone();
 
     let public_address = incoming_view_key.public_address();
@@ -88,7 +83,7 @@ mod test {
         let sapling_key = SaplingKey::generate_key();
 
         let trusted_dealer_key_packages =
-            split_spender_key(&sapling_key, 5, identities).expect("spender key split failed");
+            split_spender_key(&sapling_key, 5, &identities).expect("spender key split failed");
 
         assert_eq!(
             trusted_dealer_key_packages.key_packages.len(),

ironfish-rust/src/transaction/tests.rs

@@ -708,7 +708,7 @@ fn test_aggregate_signature_shares() {
     let identities = create_multisig_identities(10);
 
     // key package generation by trusted dealer
-    let key_packages = split_spender_key(&spender_key, 2, identities.clone())
+    let key_packages = split_spender_key(&spender_key, 2, &identities)
         .expect("should be able to split spender key");
 
     // create raw/proposed transaction