From 9c2bf41ec8f649ffac824878256c09598bf52269 Mon Sep 17 00:00:00 2001
From: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com>
Date: Fri, 26 May 2023 09:04:39 +0200
Subject: [PATCH] feat: add metadata signature check (#5411)

Description
---
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.

Motivation and Context
---
See above.

How Has This Been Tested?
---
Added a new unit test.

What process can a PR reviewer use to test or verify this change?
---
Inspection

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
---
 .../transaction_output.rs                     | 60 ++++++++++++++-----
 1 file changed, 46 insertions(+), 14 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 e02f674c68..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,18 +249,17 @@ 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.
+        // 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());
         let commit_nonce_a = PrivateKey::default(); // This is the deterministic nonce `r_a` of zero
@@ -274,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,
@@ -299,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(())
     }
 
@@ -674,6 +678,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 +831,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("Signature is invalid: Metadata signature not valid!".to_string())
+            ),
+        }
+    }
+
     #[test]
     fn it_does_not_batch_verify_incorrect_minimum_values() {
         let factories = CryptoFactories::default();