Merge pull request #3960 from anoma/grarco/memo-validation-in-vp

Memo integrity check in validity predicates

.changelog/unreleased/bug-fixes/3960-memo-validation-in-vp.md

@@ -0,0 +1,2 @@
+- Added validation of the transaction's memo in the validity predicates to catch
+  any possible tamperings. ([\#3960](https://github.com/anoma/namada/pull/3960))

crates/core/src/hash.rs

@@ -175,7 +175,7 @@ impl Hash {
     }
 
     /// Return zeros
-    pub fn zero() -> Self {
+    pub const fn zero() -> Self {
         Self([0u8; HASH_LENGTH])
     }
 

crates/node/src/protocol.rs

@@ -1669,45 +1669,51 @@ mod tests {
         // Test that the strategy produces valid txs first
         let result =
             runner.run(&arb_valid_signed_inner_tx(signing_key.clone()), |tx| {
+                for cmt in tx.commitments() {
+                    let batched_tx = BatchedTxRef { tx: &tx, cmt };
+                    assert!(
+                        wasm::run::vp(
+                            code_hash,
+                            &batched_tx,
+                            &TxIndex::default(),
+                            &addr,
+                            &state,
+                            &RefCell::new(VpGasMeter::new_from_tx_meter(
+                                &TxGasMeter::new(u64::MAX),
+                            )),
+                            &Default::default(),
+                            &Default::default(),
+                            vp_cache.clone(),
+                        )
+                        .is_ok()
+                    );
+                }
+                Ok(())
+            });
+        assert!(result.is_ok());
+
+        // Then test tampered txs
+        let mut runner = TestRunner::new(Config::default());
+        let result = runner.run(&arb_tampered_inner_tx(signing_key), |tx| {
+            for cmt in tx.commitments() {
+                let batched_tx = BatchedTxRef { tx: &tx, cmt };
                 assert!(
                     wasm::run::vp(
                         code_hash,
-                        &tx.batch_ref_first_tx().unwrap(),
+                        &batched_tx,
                         &TxIndex::default(),
                         &addr,
                         &state,
                         &RefCell::new(VpGasMeter::new_from_tx_meter(
-                            &TxGasMeter::new(u64::MAX),
+                            &TxGasMeter::new(u64::MAX,)
                         )),
                         &Default::default(),
                         &Default::default(),
                         vp_cache.clone(),
                     )
-                    .is_ok()
+                    .is_err()
                 );
-                Ok(())
-            });
-        assert!(result.is_ok());
-
-        // Then test tampered txs
-        let mut runner = TestRunner::new(Config::default());
-        let result = runner.run(&arb_tampered_inner_tx(signing_key), |tx| {
-            assert!(
-                wasm::run::vp(
-                    code_hash,
-                    &tx.batch_ref_first_tx().unwrap(),
-                    &TxIndex::default(),
-                    &addr,
-                    &state,
-                    &RefCell::new(VpGasMeter::new_from_tx_meter(
-                        &TxGasMeter::new(u64::MAX,)
-                    )),
-                    &Default::default(),
-                    &Default::default(),
-                    vp_cache.clone(),
-                )
-                .is_err()
-            );
+            }
             Ok(())
         });
         assert!(result.is_ok());

crates/sdk/src/lib.rs

@@ -1245,11 +1245,18 @@ pub mod testing {
     }
 
     prop_compose! {
-        /// Generate an arbitrary transaction with maybe a memo
+        /// Generate an arbitrary transaction with maybe a valid memo
         pub fn arb_memoed_tx()(
             (mut tx, tx_data) in arb_tx(),
             memo in option::of(arb_utf8_code()),
         ) -> (Tx, TxData) {
+            // Clean up any previous memo commitments
+            let mut batch: Vec<_> = tx.header.batch.iter().cloned().collect();
+            for inner_tx in batch.iter_mut() {
+                inner_tx.memo_hash = Default::default();
+            }
+            tx.header.batch = batch.into_iter().collect();
+
             if let Some(memo) = memo {
                 let sechash = tx
                     .add_section(Section::ExtraData(memo))

crates/tests/src/integration/masp.rs

@@ -2145,6 +2145,8 @@ fn masp_incentives() -> Result<()> {
                 NAM,
                 "--amount",
                 "1.451732",
+                "--gas-limit",
+                "60000",
                 "--signing-keys",
                 BERTHA_KEY,
                 "--node",
@@ -5166,13 +5168,7 @@ fn identical_output_descriptions() -> Result<()> {
     let tx: namada_sdk::tx::Tx = serde_json::from_slice(&tx_bytes).unwrap();
     // Inject some randomness in the cloned tx to change the hash
     let mut tx_clone = tx.clone();
-    let mut cmt = tx_clone.header.batch.first().unwrap().to_owned();
-    let random_hash: Vec<_> = (0..namada_sdk::hash::HASH_LENGTH)
-        .map(|_| rand::random::<u8>())
-        .collect();
-    cmt.memo_hash = namada_sdk::hash::Hash(random_hash.try_into().unwrap());
-    tx_clone.header.batch.clear();
-    tx_clone.header.batch.insert(cmt);
+    tx_clone.add_memo(&[1, 2, 3]);
 
     let signing_data = SigningTxData {
         owner: None,

crates/vp_prelude/src/lib.rs

@@ -52,7 +52,7 @@ use namada_vm_env::vp::*;
 use namada_vm_env::{read_from_buffer, read_key_val_bytes_from_buffer};
 pub use namada_vp_env::{collection_validation, VpEnv};
 pub use sha2::{Digest, Sha256, Sha384, Sha512};
-use tx::BatchedTxRef;
+use tx::{BatchedTxRef, TxCommitments};
 pub use {
     namada_account as account, namada_parameters as parameters,
     namada_proof_of_stake as proof_of_stake, namada_token as token,
@@ -131,9 +131,22 @@ impl VerifySigGadget {
         &mut self,
         ctx: &Ctx,
         tx_data: &Tx,
+        cmt: &TxCommitments,
         owner: &Address,
     ) -> VpResult {
         if !self.has_validated_sig {
+            // First check that the memo section of this inner tx has not been
+            // tampered with
+            if cmt.memo_hash != namada_core::hash::Hash::zero() {
+                tx_data.get_section(&cmt.memo_hash).ok_or_else(|| {
+                    VpError::Erased(format!(
+                        "Memo section with hash {} is missing",
+                        cmt.memo_hash
+                    ))
+                })?;
+            }
+
+            // Then check the signature
             verify_signatures(ctx, tx_data, owner)?;
             self.has_validated_sig = true;
         }
@@ -149,10 +162,11 @@ impl VerifySigGadget {
         predicate: F,
         ctx: &Ctx,
         tx_data: &Tx,
+        cmt: &TxCommitments,
         owner: &Address,
     ) -> VpResult {
         if predicate() {
-            self.verify_signatures(ctx, tx_data, owner)?;
+            self.verify_signatures(ctx, tx_data, cmt, owner)?;
         }
         Ok(())
     }