Contracts: reject an inconsistent participant list and slate state

add_keys matched a participant on the public excess alone, so an entry
carrying our excess with a nonce that is not ours counted as ours and the
full-slate guard was skipped. add_participant_info only replaces an entry
when both keys match, so that entry was kept and ours appended, leaving a
two party slate with three participants. pub_nonce_sum and pub_blind_sum
fold in every entry, so the partial signature is then made over an
aggregate that includes the spurious one. Match on the excess and nonce
pair, the same test add_participant_info and fill_round_2 already apply,
and reject a list carrying our excess with a foreign nonce rather than
treating that entry as ours.

transition_state turned every other state, including Unknown and an
already completed slate, into a successful Standard3. That happens after
signing and before the wallet state is persisted, so accept only the four
transitions the contract flows use and report anything else.

libwallet/src/contract/slate.rs

@@ -171,9 +171,14 @@ pub fn transition_state(slate: &mut Slate) -> Result<(), Error> {
 		SlateState::Invoice2 => SlateState::Invoice3,
 		SlateState::Standard1 => SlateState::Standard2,
 		SlateState::Standard2 => SlateState::Standard3,
-		_ => {
-			debug!("Slate.state: {}", slate.state);
-			SlateState::Standard3
+		// Unknown, or a slate that has already reached its final state, is not something
+		// we can advance. We have signed by this point and the wallet state is persisted
+		// next, so report it rather than reporting a Standard3 that never happened.
+		ref s => {
+			return Err(Error::GenericError(format!(
+				"Cannot advance a contract slate in state {}",
+				s
+			)))
 		}
 	};
 	slate.state = new_state;
@@ -258,13 +263,27 @@ where
 	K: Keychain,
 {
 	debug!("contract::slate::add_keys => called");
+	let our_pub_key = PublicKey::from_secret_key(keychain.secp(), &context.sec_key)?;
+	let our_pub_nonce = PublicKey::from_secret_key(keychain.secp(), &context.sec_nonce)?;
+	// An entry is ours only when both keys match, which is what add_participant_info and
+	// fill_round_2 use. Our excess paired with a nonce that is not ours cannot have come
+	// from us, so reject it rather than treating the entry as ours: we would otherwise
+	// append a further participant and sign nothing.
+	if slate
+		.participant_data
+		.iter()
+		.any(|p| p.public_blind_excess == our_pub_key && p.public_nonce != our_pub_nonce)
+	{
+		return Err(Error::GenericError(
+			"Slate carries our public excess with a different nonce".to_string(),
+		));
+	}
 	// Guard against a tampered slate that already carries the full participant set.
 	// Re-adding our own info is idempotent, so only block when we are not already in it.
-	let our_pub_key = PublicKey::from_secret_key(keychain.secp(), &context.sec_key)?;
 	let already_ours = slate
 		.participant_data
 		.iter()
-		.any(|p| p.public_blind_excess == our_pub_key);
+		.any(|p| p.public_blind_excess == our_pub_key && p.public_nonce == our_pub_nonce);
 	if !already_ours && slate.participant_data.len() >= slate.num_participants as usize {
 		return Err(Error::GenericError(format!(
 			"Slate already has the expected {} participant(s)",
@@ -273,3 +292,66 @@ where
 	}
 	slate.add_participant_info(keychain, context, None)
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use crate::grin_keychain::{ExtKeychain, Keychain as _, SwitchCommitmentType};
+	use crate::slate::ParticipantData;
+
+	fn keychain_and_context() -> (ExtKeychain, Context) {
+		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);
+		(keychain, context)
+	}
+
+	#[test]
+	fn add_keys_rejects_our_excess_with_a_foreign_nonce() {
+		let (keychain, mut context) = keychain_and_context();
+		let our_excess = PublicKey::from_secret_key(keychain.secp(), &context.sec_key).unwrap();
+		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);
+		slate.participant_data.push(ParticipantData {
+			public_blind_excess: our_excess,
+			public_nonce: other,
+			part_sig: None,
+		});
+		assert!(add_keys(&mut slate, &keychain, &mut context).is_err());
+	}
+
+	#[test]
+	fn transition_state_rejects_states_it_cannot_advance() {
+		for state in [
+			SlateState::Unknown,
+			SlateState::Standard3,
+			SlateState::Invoice3,
+		] {
+			let mut slate = Slate::blank(2, false);
+			slate.state = state.clone();
+			assert!(
+				transition_state(&mut slate).is_err(),
+				"expected {} to be rejected",
+				state
+			);
+		}
+
+		// The transitions the contract flows actually use are still accepted
+		for (from, to) in [
+			(SlateState::Standard1, SlateState::Standard2),
+			(SlateState::Standard2, SlateState::Standard3),
+			(SlateState::Invoice1, SlateState::Invoice2),
+			(SlateState::Invoice2, SlateState::Invoice3),
+		] {
+			let mut slate = Slate::blank(2, false);
+			slate.state = from;
+			transition_state(&mut slate).unwrap();
+			assert_eq!(slate.state, to);
+		}
+	}
+}