From 00b7ae5124f6c9231e3d2a5983125104b94f76e8 Mon Sep 17 00:00:00 2001 From: Hansie Odendaal Date: Thu, 25 May 2023 14:56:53 +0200 Subject: [PATCH 1/2] Add metadata signature check Added metadata signature check when verifying the revealed value proofs, just to be sure that the basis of the balance calculation check will have integrity. --- .../transaction_output.rs | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/base_layer/core/src/transactions/transaction_components/transaction_output.rs b/base_layer/core/src/transactions/transaction_components/transaction_output.rs index e02f674c68..79f0fea163 100644 --- a/base_layer/core/src/transactions/transaction_components/transaction_output.rs +++ b/base_layer/core/src/transactions/transaction_components/transaction_output.rs @@ -261,6 +261,19 @@ impl TransactionOutput { &self.encrypted_data, self.minimum_value_promise, ); + // Let's first verify that the metadata signature is valid + if !self.metadata_signature.verify_challenge( + &self.commitment, + &self.sender_offset_public_key, + &e_bytes, + &CommitmentFactory::default(), + &mut OsRng, + ) { + return Err(RangeProofError::InvalidRangeProof( + "Metadata signature not valid!".to_string(), + )); + } + // Now we can perform the balance proof let e = PrivateKey::from_bytes(&e_bytes).unwrap(); let value_as_private_key = PrivateKey::from(self.minimum_value_promise.as_u64()); let commit_nonce_a = PrivateKey::default(); // This is the deterministic nonce `r_a` of zero @@ -674,6 +687,8 @@ pub fn batch_verify_range_proofs( #[cfg(test)] mod test { + use tari_crypto::errors::RangeProofError; + use super::{batch_verify_range_proofs, TransactionOutput}; use crate::transactions::{ tari_amount::MicroTari, @@ -825,6 +840,32 @@ mod test { } } + #[test] + fn revealed_value_proofs_only_succeed_with_valid_metadata_signatures() { + let factories = CryptoFactories::default(); + let test_params = TestParams::new(); + let mut output = create_output( + &test_params, + &factories, + MicroTari(20), + MicroTari(20), + RangeProofType::RevealedValue, + ) + .unwrap(); + assert!(output.verify_metadata_signature().is_ok()); + assert!(output.revealed_value_range_proof_check().is_ok()); + + output.features.maturity += 1; + assert!(output.verify_metadata_signature().is_err()); + match output.revealed_value_range_proof_check() { + Ok(_) => panic!("Should not have passed check"), + Err(e) => assert_eq!( + e, + RangeProofError::InvalidRangeProof("Metadata signature not valid!".to_string()) + ), + } + } + #[test] fn it_does_not_batch_verify_incorrect_minimum_values() { let factories = CryptoFactories::default(); From eb3ae1f53a49ded19dab3eda06ddaa8b57848466 Mon Sep 17 00:00:00 2001 From: Hansie Odendaal Date: Thu, 25 May 2023 16:43:06 +0200 Subject: [PATCH 2/2] review comments --- .../transaction_output.rs | 45 ++++++++----------- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/base_layer/core/src/transactions/transaction_components/transaction_output.rs b/base_layer/core/src/transactions/transaction_components/transaction_output.rs index 79f0fea163..9fcc46fbdc 100644 --- a/base_layer/core/src/transactions/transaction_components/transaction_output.rs +++ b/base_layer/core/src/transactions/transaction_components/transaction_output.rs @@ -249,30 +249,16 @@ impl TransactionOutput { self.commitment.to_hex() ))); } - let e_bytes = TransactionOutput::build_metadata_signature_challenge( - self.version, - &self.script, - &self.features, - &self.sender_offset_public_key, - self.metadata_signature.ephemeral_commitment(), - self.metadata_signature.ephemeral_pubkey(), - &self.commitment, - &self.covenant, - &self.encrypted_data, - self.minimum_value_promise, - ); - // Let's first verify that the metadata signature is valid - if !self.metadata_signature.verify_challenge( - &self.commitment, - &self.sender_offset_public_key, - &e_bytes, - &CommitmentFactory::default(), - &mut OsRng, - ) { - return Err(RangeProofError::InvalidRangeProof( - "Metadata signature not valid!".to_string(), - )); - } + // Let's first verify that the metadata signature is valid. + // Note: If normal code paths are followed, this is checked elsewhere already, but it is theoretically possible + // to meddle with the metadata signature after it has been verified and before it is used here, so we + // check it again. It is also a very cheap test in comparison to a range proof verification + let e_bytes = match self.verify_metadata_signature_internal() { + Ok(val) => val, + Err(e) => { + return Err(RangeProofError::InvalidRangeProof(format!("{}", e))); + }, + }; // Now we can perform the balance proof let e = PrivateKey::from_bytes(&e_bytes).unwrap(); let value_as_private_key = PrivateKey::from(self.minimum_value_promise.as_u64()); @@ -287,8 +273,7 @@ impl TransactionOutput { } } - /// Verify that the metadata signature is valid - pub fn verify_metadata_signature(&self) -> Result<(), TransactionError> { + fn verify_metadata_signature_internal(&self) -> Result<[u8; 32], TransactionError> { let challenge = TransactionOutput::build_metadata_signature_challenge( self.version, &self.script, @@ -312,6 +297,12 @@ impl TransactionOutput { "Metadata signature not valid!".to_string(), )); } + Ok(challenge) + } + + /// Verify that the metadata signature is valid + pub fn verify_metadata_signature(&self) -> Result<(), TransactionError> { + let _challenge = self.verify_metadata_signature_internal()?; Ok(()) } @@ -861,7 +852,7 @@ mod test { Ok(_) => panic!("Should not have passed check"), Err(e) => assert_eq!( e, - RangeProofError::InvalidRangeProof("Metadata signature not valid!".to_string()) + RangeProofError::InvalidRangeProof("Signature is invalid: Metadata signature not valid!".to_string()) ), } }