diff --git a/libwallet/src/contract/slate.rs b/libwallet/src/contract/slate.rs index aeb0498..f8169f4 100644 --- a/libwallet/src/contract/slate.rs +++ b/libwallet/src/contract/slate.rs @@ -197,6 +197,16 @@ where K: Keychain, { debug!("contract::slate::sign => called"); + // The counterparty controls the participant list, so require it to hold exactly the + // participants the slate declares before we sign over it. Setup has already added our + // own entry by this point. + if slate.participant_data.len() != slate.num_participants as usize { + return Err(Error::GenericError(format!( + "Expected {} participant(s) before signing, found {}", + slate.num_participants, + slate.participant_data.len() + ))); + } let keychain = w.keychain(keychain_mask)?; slate.fill_round_2(&keychain, &context.sec_key, &context.sec_nonce)?; debug!( diff --git a/libwallet/src/contract/utils.rs b/libwallet/src/contract/utils.rs index c0077ec..aa03241 100644 --- a/libwallet/src/contract/utils.rs +++ b/libwallet/src/contract/utils.rs @@ -97,8 +97,16 @@ where )?; let sender_address = OnionV3Address::from_private(&sender_key.0)?; - // We're looking for the OTHER party here, the recipient + // We're looking for the OTHER party here, the recipient. The 'xor 1' pairing + // only holds for a two-party slate, so check that before indexing: a slate + // with a shorter participant list would otherwise panic here. let sender_index = slate.find_index_matching_context(&keychain, context)?; + if slate.participant_data.len() != 2 { + return Err(Error::GenericError(format!( + "Expected 2 participants for a payment proof, found {}", + slate.participant_data.len() + ))); + } let recipient_index = sender_index ^ 1; tx_log_entry.payment_proof = Some(StoredProofInfo { diff --git a/libwallet/src/slate.rs b/libwallet/src/slate.rs index 025e7e2..5e4fbcf 100644 --- a/libwallet/src/slate.rs +++ b/libwallet/src/slate.rs @@ -420,14 +420,15 @@ impl Slate { where K: Keychain, { - for i in 0..self.num_participants() as usize { - let calc_pub_excess = PublicKey::from_secret_key(keychain.secp(), &context.sec_key)?; - let calc_pub_nonce = PublicKey::from_secret_key(keychain.secp(), &context.sec_nonce)?; - + let calc_pub_excess = PublicKey::from_secret_key(keychain.secp(), &context.sec_key)?; + let calc_pub_nonce = PublicKey::from_secret_key(keychain.secp(), &context.sec_nonce)?; + // Iterate over the entries the slate actually carries rather than indexing by + // position up to num_participants, which a counterparty can send short. Both keys + // have to match, as fill_round_2 requires, so an entry sharing only one of them is + // not mistaken for ours. + for (i, p) in self.participant_data.iter().enumerate() { // find my entry - if self.participant_data[i].public_blind_excess == calc_pub_excess - || self.participant_data[i].public_nonce == calc_pub_nonce - { + if p.public_blind_excess == calc_pub_excess && p.public_nonce == calc_pub_nonce { return Ok(i); } } @@ -458,12 +459,11 @@ impl Slate { )?; let pub_excess = PublicKey::from_secret_key(keychain.secp(), &sec_key)?; let pub_nonce = PublicKey::from_secret_key(keychain.secp(), &sec_nonce)?; - for i in 0..self.num_participants() as usize { + // As in find_index_matching_context, iterate the entries we actually have + for p in self.participant_data.iter_mut() { // find my entry - if self.participant_data[i].public_blind_excess == pub_excess - && self.participant_data[i].public_nonce == pub_nonce - { - self.participant_data[i].part_sig = Some(sig_part); + if p.public_blind_excess == pub_excess && p.public_nonce == pub_nonce { + p.part_sig = Some(sig_part); break; } } @@ -1614,3 +1614,71 @@ impl From for OutputFeatures { } } } + +#[cfg(test)] +mod test { + use super::*; + use crate::grin_keychain::ExtKeychain; + + // A counterparty controls the participant list, so it can be shorter than + // num_participants claims, or hold an entry that matches only one of our two keys. + fn context_and_keys() -> (ExtKeychain, Context, PublicKey, PublicKey) { + let keychain = ExtKeychain::from_random_seed(true).unwrap(); + let parent_key_id = ExtKeychain::derive_key_id(1, 0, 0, 0, 0); + let context = Context::new(keychain.secp(), &parent_key_id, true, true); + let our_excess = PublicKey::from_secret_key(keychain.secp(), &context.sec_key).unwrap(); + let our_nonce = PublicKey::from_secret_key(keychain.secp(), &context.sec_nonce).unwrap(); + (keychain, context, our_excess, our_nonce) + } + + #[test] + fn find_index_handles_short_participant_list() { + let (keychain, context, _, _) = context_and_keys(); + // A key that matches neither of ours, so the search runs off the end of the list + let other_id = ExtKeychain::derive_key_id(1, 1, 0, 0, 0); + let other_key = keychain + .derive_key(0, &other_id, SwitchCommitmentType::Regular) + .unwrap(); + let other = PublicKey::from_secret_key(keychain.secp(), &other_key).unwrap(); + + let mut slate = Slate::blank(2, false); + // Claims two participants but carries one, and it is not ours + slate.num_participants = 2; + slate.participant_data.push(ParticipantData { + public_blind_excess: other, + public_nonce: other, + part_sig: None, + }); + assert!(slate + .find_index_matching_context(&keychain, &context) + .is_err()); + } + + #[test] + fn find_index_requires_both_keys_to_match() { + let (keychain, context, our_excess, our_nonce) = context_and_keys(); + let mut slate = Slate::blank(2, false); + // Shares our nonce but not our excess, so it is not our entry + slate.participant_data.push(ParticipantData { + public_blind_excess: our_nonce, + public_nonce: our_nonce, + part_sig: None, + }); + assert!(slate + .find_index_matching_context(&keychain, &context) + .is_err()); + + // Both keys match, so this one is ours + slate.participant_data.push(ParticipantData { + public_blind_excess: our_excess, + public_nonce: our_nonce, + part_sig: None, + }); + assert_eq!( + slate + .find_index_matching_context(&keychain, &context) + .unwrap(), + 1 + ); + } +}