fix: protect against amounts tampering and incomplete spends attack

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(),

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,

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.

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<SignedSpend>) -> 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::<BTreeSet<_>>();
+        let signed_spend_keys = signed_spends
+            .iter()
+            .map(|s| s.unique_pubkey())
+            .collect::<BTreeSet<_>>();
+        if input_keys != signed_spend_keys {
+            return Err(Error::SpendsDoNotMatchInputs);
         }
 
         // Verify that each output is unique
-        let output_pks: BTreeSet<UniquePubkey> =
-            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<UniquePubkey> =
+        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() {

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,
 

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 {