From 2c132d2bfde6688229de13b92697feaa8f7e2f23 Mon Sep 17 00:00:00 2001 From: grumbach Date: Thu, 30 Nov 2023 16:57:51 +0900 Subject: [PATCH] fix: protect against amounts tampering and incomplete spends attack --- sn_node/src/put_validation.rs | 2 +- sn_transfers/src/cashnotes/builder.rs | 4 +- sn_transfers/src/cashnotes/signed_spend.rs | 30 ++++++++++-- sn_transfers/src/cashnotes/transaction.rs | 57 ++++++++++------------ sn_transfers/src/error.rs | 6 ++- sn_transfers/src/wallet/local_store.rs | 2 +- 6 files changed, 62 insertions(+), 39 deletions(-) diff --git a/sn_node/src/put_validation.rs b/sn_node/src/put_validation.rs index 2ce2b7393d..d99f812021 100644 --- a/sn_node/src/put_validation.rs +++ b/sn_node/src/put_validation.rs @@ -766,7 +766,7 @@ impl Node { ); for parent_input in &signed_spend.spend.parent_tx.inputs { let parent_cash_note_address = - SpendAddress::from_unique_pubkey(&parent_input.unique_pubkey()); + SpendAddress::from_unique_pubkey(parent_input.unique_pubkey()); trace!( "Checking parent input at {:?} - {parent_cash_note_address:?}", parent_input.unique_pubkey(), diff --git a/sn_transfers/src/cashnotes/builder.rs b/sn_transfers/src/cashnotes/builder.rs index 0f15e9adbf..3497261b31 100644 --- a/sn_transfers/src/cashnotes/builder.rs +++ b/sn_transfers/src/cashnotes/builder.rs @@ -38,7 +38,7 @@ impl TransactionBuilder { input_src_tx: InputSrcTx, ) -> Self { self.input_details - .insert(input.unique_pubkey(), (derived_key, input_src_tx)); + .insert(*input.unique_pubkey(), (derived_key, input_src_tx)); self.inputs.push(input); self } @@ -93,7 +93,7 @@ impl TransactionBuilder { if let Some((derived_key, input_src_tx)) = self.input_details.get(&input.unique_pubkey) { let spend = Spend { - unique_pubkey: input.unique_pubkey(), + unique_pubkey: *input.unique_pubkey(), spent_tx: spent_tx.clone(), reason, token: input.amount, diff --git a/sn_transfers/src/cashnotes/signed_spend.rs b/sn_transfers/src/cashnotes/signed_spend.rs index 87bfe38a3f..d3763815d7 100644 --- a/sn_transfers/src/cashnotes/signed_spend.rs +++ b/sn_transfers/src/cashnotes/signed_spend.rs @@ -64,9 +64,11 @@ impl SignedSpend { /// Verify this SignedSpend /// - /// Checks that the spend was indeed spent for the given Tx hash, and that it was - /// signed by the DerivedSecretKey. Also verifies that that signature is - /// valid for this SignedSpend. + /// Checks that + /// - the spend was indeed spent for the given Tx + /// - it was signed by the DerivedSecretKey that owns the CashNote for this Spend + /// - the signature is valid + /// - its value didn't change between the two transactions it is involved in (creation and spending) pub fn verify(&self, spent_tx_hash: Hash) -> Result<()> { // verify that input spent_tx_hash matches self.spent_tx_hash if spent_tx_hash != self.spent_tx_hash() { @@ -76,6 +78,28 @@ impl SignedSpend { )); } + // check that the value of the spend wasn't tampered with + let claimed_value = self.spend.token; + let creation_value = self + .spend + .parent_tx + .outputs + .iter() + .find(|o| o.unique_pubkey == self.spend.unique_pubkey) + .map(|o| o.amount) + .unwrap_or(NanoTokens::zero()); + let spent_value = self + .spend + .spent_tx + .inputs + .iter() + .find(|i| i.unique_pubkey == self.spend.unique_pubkey) + .map(|i| i.amount) + .unwrap_or(NanoTokens::zero()); + if claimed_value != creation_value || creation_value != spent_value { + return Err(Error::InvalidSpendValue(*self.unique_pubkey())); + } + // check signature // the spend is signed by the DerivedSecretKey // corresponding to the UniquePubkey of the CashNote being spent. diff --git a/sn_transfers/src/cashnotes/transaction.rs b/sn_transfers/src/cashnotes/transaction.rs index 9fce8c50b8..0b89484e47 100644 --- a/sn_transfers/src/cashnotes/transaction.rs +++ b/sn_transfers/src/cashnotes/transaction.rs @@ -34,8 +34,8 @@ impl Input { v } - pub fn unique_pubkey(&self) -> UniquePubkey { - self.unique_pubkey + pub fn unique_pubkey(&self) -> &UniquePubkey { + &self.unique_pubkey } } @@ -144,44 +144,50 @@ impl Transaction { })?; if input_sum != output_sum { - Err(Error::InconsistentTransaction) + Err(Error::UnbalancedTransaction) } else { Ok(()) } } - /// Verifies a transaction including signed spends. + /// Verifies a transaction with Network held signed spends. /// - /// This function relies/assumes that the caller (wallet/client) obtains - /// the Transaction (held by every input spend's close group) in a - /// trustless/verified way. I.e., the caller should not simply obtain a - /// spend from a single peer, but must get the same spend from all in the close group. + /// This function assumes that the signed spends where previously fetched from the Network and where not double spent. + /// This function will verify that: + /// - the transaction is balanced (sum inputs = sum outputs) + /// - the inputs and outputs are unique + /// - the inputs and outputs are different + /// - the inputs have a corresponding signed spend + /// - those signed spends are valid and refer to this transaction pub fn verify_against_inputs_spent(&self, signed_spends: &BTreeSet) -> Result<()> { // verify that the tx has at least one input if self.inputs.is_empty() { return Err(Error::MissingTxInputs); } - // check if we have spends for all inputs - if signed_spends.is_empty() { - return Err(Error::MissingTxInputs)?; - } - if signed_spends.len() != self.inputs.len() { - return Err(Error::SignedSpendInputLenMismatch { - got: signed_spends.len(), - expected: self.inputs.len(), - }); + // check spends match the inputs + let input_keys = self + .inputs + .iter() + .map(|i| i.unique_pubkey()) + .collect::>(); + let signed_spend_keys = signed_spends + .iter() + .map(|s| s.unique_pubkey()) + .collect::>(); + if input_keys != signed_spend_keys { + return Err(Error::SpendsDoNotMatchInputs); } // Verify that each output is unique - let output_pks: BTreeSet = - self.outputs.iter().map(|o| (*o.unique_pubkey())).collect(); + let output_pks: BTreeSet<&UniquePubkey> = + self.outputs.iter().map(|o| (o.unique_pubkey())).collect(); if output_pks.len() != self.outputs.len() { return Err(Error::UniquePubkeyNotUniqueInTx); } // Verify that each input is unique - let input_pks: BTreeSet = + let input_pks: BTreeSet<&UniquePubkey> = self.inputs.iter().map(|i| (i.unique_pubkey())).collect(); if input_pks.len() != self.inputs.len() { return Err(Error::UniquePubkeyNotUniqueInTx); @@ -192,17 +198,6 @@ impl Transaction { return Err(Error::UniquePubkeyNotUniqueInTx); } - // Verify that each input has a corresponding signed spend. - for signed_spend in signed_spends.iter() { - if !self - .inputs - .iter() - .any(|m| m.unique_pubkey == *signed_spend.unique_pubkey()) - { - return Err(Error::SignedSpendInputIdMismatch); - } - } - // Verify that each signed spend is valid let spent_tx_hash = self.hash(); for signed_spend in signed_spends.iter() { diff --git a/sn_transfers/src/error.rs b/sn_transfers/src/error.rs index ea72350627..066ee6d382 100644 --- a/sn_transfers/src/error.rs +++ b/sn_transfers/src/error.rs @@ -27,6 +27,8 @@ pub enum Error { #[error("Failed to parse: {0}")] FailedToParseNanoToken(String), + #[error("Invalid Spend: value was tampered with {0:?}")] + InvalidSpendValue(UniquePubkey), #[error("Invalid Spend Signature for {0:?}")] InvalidSpendSignature(UniquePubkey), #[error("Transaction hash is different from the hash in the the Spend: {0:?} != {1:?}")] @@ -54,9 +56,11 @@ pub enum Error { #[error("Could not serialize CashNote to hex: {0}")] HexSerializationFailed(String), #[error("The input and output amounts of the tx do not match.")] - InconsistentTransaction, + UnbalancedTransaction, #[error("The CashNote tx must have at least one input.")] MissingTxInputs, + #[error("The spends don't match the inputs of the Transaction.")] + SpendsDoNotMatchInputs, #[error("Overflow occurred while adding values")] NumericOverflow, diff --git a/sn_transfers/src/wallet/local_store.rs b/sn_transfers/src/wallet/local_store.rs index d791a963cb..53c78de556 100644 --- a/sn_transfers/src/wallet/local_store.rs +++ b/sn_transfers/src/wallet/local_store.rs @@ -405,7 +405,7 @@ impl LocalWallet { .available_cash_notes .retain(|k, _| !spent_unique_pubkeys.contains(k)); for spent in spent_unique_pubkeys { - self.wallet.spent_cash_notes.insert(spent); + self.wallet.spent_cash_notes.insert(*spent); } if let Some(cash_note) = transfer.change_cash_note {