Merge branch 'grarco/fix-replay-protection' (#1867)

* origin/grarco/fix-replay-protection: Changelog #1867 Fixes raw header hash in compressed signature Removes redundant signature on `Code` and `Data` Client signs the raw transaction header Adds raw header signature in tests Removes useless checks on decrypted tx, error codes and unit tests Adds `raw_header_hash` method for `Tx` Inner tx signer also signs tx header
anoma · Nov 11, 2023 · 2a881ee · 2a881ee
2 parents db10297 + c40cbc1
commit 2a881ee
Show file tree

Hide file tree

Showing 20 changed files with 161 additions and 354 deletions.
diff --git a/.changelog/unreleased/improvements/1867-fix-replay-protection.md b/.changelog/unreleased/improvements/1867-fix-replay-protection.md
@@ -0,0 +1,2 @@
+- Reworked the signature of inner transactions to improve safety and fix replay
+  protection. ([\#1867](https://github.com/anoma/namada/pull/1867))
diff --git a/apps/src/lib/bench_utils.rs b/apps/src/lib/bench_utils.rs
@@ -444,7 +444,7 @@ pub fn generate_tx(
 
     if let Some(signer) = signer {
         tx.add_section(Section::Signature(Signature::new(
-            tx.sechashes(),
+            vec![tx.raw_header_hash()],
             [(0, signer.clone())].into_iter().collect(),
             None,
         )));
@@ -486,7 +486,7 @@ pub fn generate_foreign_key_tx(signer: &SecretKey) -> Tx {
         .serialize_to_vec(),
     ));
     tx.add_section(Section::Signature(Signature::new(
-        tx.sechashes(),
+        vec![tx.raw_header_hash()],
         [(0, signer.clone())].into_iter().collect(),
         None,
     )));

diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs
@@ -930,13 +930,13 @@ where
 
     // Allow to replay a specific wasm transaction. Needs as argument the
     // corresponding wrapper transaction to avoid replay of that in the process
-    fn allow_tx_replay(&mut self, mut wrapper_tx: Tx) {
+    fn allow_tx_replay(&mut self, wrapper_tx: Tx) {
         self.wl_storage
             .write_tx_hash(wrapper_tx.header_hash())
             .expect("Error while deleting tx hash from storage");
 
         self.wl_storage
-            .delete_tx_hash(wrapper_tx.update_header(TxType::Raw).header_hash())
+            .delete_tx_hash(wrapper_tx.raw_header_hash())
             .expect("Error while deleting tx hash from storage");
     }
 }
@@ -2236,12 +2236,10 @@ mod test_finalize_block {
 
         let (wrapper_tx, processed_tx) =
             mk_wrapper_tx(&shell, &crate::wallet::defaults::albert_keypair());
-        let mut decrypted_tx = wrapper_tx;
 
-        decrypted_tx.update_header(TxType::Raw);
         let decrypted_hash_key =
             replay_protection::get_replay_protection_last_key(
-                &decrypted_tx.header_hash(),
+                &wrapper_tx.raw_header_hash(),
             );
 
         // merkle tree root before finalize_block
@@ -2274,7 +2272,7 @@ mod test_finalize_block {
                 .shell
                 .wl_storage
                 .write_log
-                .has_replay_protection_entry(&decrypted_tx.header_hash())
+                .has_replay_protection_entry(&wrapper_tx.raw_header_hash())
                 .unwrap_or_default()
         );
         // Check that the hash is present in the merkle tree
@@ -2331,16 +2329,9 @@ mod test_finalize_block {
 
         decrypted_tx.update_header(TxType::Decrypted(DecryptedTx::Decrypted));
         decrypted_tx_2.update_header(TxType::Decrypted(DecryptedTx::Decrypted));
-        let decrypted_hash =
-            wrapper_tx.clone().update_header(TxType::Raw).header_hash();
-        let decrypted_2_hash = wrapper_tx_2
-            .clone()
-            .update_header(TxType::Raw)
-            .header_hash();
-        let decrypted_3_hash = invalid_wrapper_tx
-            .clone()
-            .update_header(TxType::Raw)
-            .header_hash();
+        let decrypted_hash = wrapper_tx.raw_header_hash();
+        let decrypted_2_hash = wrapper_tx_2.raw_header_hash();
+        let decrypted_3_hash = invalid_wrapper_tx.raw_header_hash();
 
         // Write inner hashes in storage
         for hash in [&decrypted_hash, &decrypted_2_hash] {

diff --git a/apps/src/lib/node/ledger/shell/mod.rs b/apps/src/lib/node/ledger/shell/mod.rs
@@ -148,22 +148,19 @@ impl From<Error> for TxResult {
 #[derive(Debug, Copy, Clone, FromPrimitive, ToPrimitive, PartialEq, Eq)]
 pub enum ErrorCodes {
     Ok = 0,
-    InvalidDecryptedChainId = 1,
-    ExpiredDecryptedTx = 2,
-    DecryptedTxGasLimit = 3,
-    WasmRuntimeError = 4,
-    InvalidTx = 5,
-    InvalidSig = 6,
-    InvalidOrder = 7,
-    ExtraTxs = 8,
-    Undecryptable = 9,
-    AllocationError = 10,
-    ReplayTx = 11,
-    InvalidChainId = 12,
-    ExpiredTx = 13,
-    TxGasLimit = 14,
-    FeeError = 15,
-    InvalidVoteExtension = 16,
+    WasmRuntimeError = 1,
+    InvalidTx = 2,
+    InvalidSig = 3,
+    InvalidOrder = 4,
+    ExtraTxs = 5,
+    Undecryptable = 6,
+    AllocationError = 7,
+    ReplayTx = 8,
+    InvalidChainId = 9,
+    ExpiredTx = 10,
+    TxGasLimit = 11,
+    FeeError = 12,
+    InvalidVoteExtension = 13,
 }
 
 impl ErrorCodes {
@@ -174,11 +171,7 @@ impl ErrorCodes {
         // NOTE: pattern match on all `ErrorCodes` variants, in order
         // to catch potential bugs when adding new codes
         match self {
-            Ok
-            | InvalidDecryptedChainId
-            | ExpiredDecryptedTx
-            | WasmRuntimeError
-            | DecryptedTxGasLimit => true,
+            Ok | WasmRuntimeError => true,
             InvalidTx | InvalidSig | InvalidOrder | ExtraTxs
             | Undecryptable | AllocationError | ReplayTx | InvalidChainId
             | ExpiredTx | TxGasLimit | FeeError | InvalidVoteExtension => false,
@@ -949,13 +942,12 @@ where
             )));
         }
 
-        // Write wrapper hash to tx WAL
+        // Write wrapper hash to WAL
         temp_wl_storage
             .write_tx_hash(wrapper_hash)
             .map_err(|e| Error::ReplayAttempt(e.to_string()))?;
 
-        let inner_tx_hash =
-            wrapper.clone().update_header(TxType::Raw).header_hash();
+        let inner_tx_hash = wrapper.raw_header_hash();
         if temp_wl_storage
             .has_replay_protection_entry(&inner_tx_hash)
             .expect("Error while checking inner tx hash key in storage")
@@ -966,7 +958,7 @@ where
             )));
         }
 
-        // Write inner hash to tx WAL
+        // Write inner hash to WAL
         temp_wl_storage
             .write_tx_hash(inner_tx_hash)
             .map_err(|e| Error::ReplayAttempt(e.to_string()))
@@ -1085,22 +1077,19 @@ where
             }
         };
 
-        let tx_chain_id = tx.header.chain_id.clone();
-        let tx_expiration = tx.header.expiration;
-
         // Tx chain id
-        if tx_chain_id != self.chain_id {
+        if tx.header.chain_id != self.chain_id {
             response.code = ErrorCodes::InvalidChainId.into();
             response.log = format!(
                 "{INVALID_MSG}: Tx carries a wrong chain id: expected {}, \
                  found {}",
-                self.chain_id, tx_chain_id
+                self.chain_id, tx.header.chain_id
             );
             return response;
         }
 
         // Tx expiration
-        if let Some(exp) = tx_expiration {
+        if let Some(exp) = tx.header.expiration {
             let last_block_timestamp = self.get_block_timestamp(None);
 
             if last_block_timestamp > exp {
@@ -1259,13 +1248,11 @@ where
                 }
 
                 // Replay protection check
-                let mut inner_tx = tx;
-                inner_tx.update_header(TxType::Raw);
-                let inner_tx_hash = &inner_tx.header_hash();
+                let inner_tx_hash = tx.raw_header_hash();
                 if self
                     .wl_storage
                     .storage
-                    .has_replay_protection_entry(inner_tx_hash)
+                    .has_replay_protection_entry(&tx.raw_header_hash())
                     .expect("Error while checking inner tx hash key in storage")
                 {
                     response.code = ErrorCodes::ReplayTx.into();
@@ -2537,8 +2524,7 @@ mod shell_tests {
             )
         );
 
-        let inner_tx_hash =
-            wrapper.clone().update_header(TxType::Raw).header_hash();
+        let inner_tx_hash = wrapper.raw_header_hash();
         // Write inner hash in storage
         let inner_hash_key =
             replay_protection::get_replay_protection_last_subkey(

diff --git a/apps/src/lib/node/ledger/shell/prepare_proposal.rs b/apps/src/lib/node/ledger/shell/prepare_proposal.rs
@@ -1278,8 +1278,7 @@ mod test_prepare_proposal {
             [(0, keypair)].into_iter().collect(),
             None,
         )));
-        let inner_unsigned_hash =
-            wrapper.clone().update_header(TxType::Raw).header_hash();
+        let inner_unsigned_hash = wrapper.raw_header_hash();
 
         // Write inner hash to storage
         let hash_key = replay_protection::get_replay_protection_last_key(
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		- Reworked the signature of inner transactions to improve safety and fix replay
		protection. ([\#1867](https://github.com/anoma/namada/pull/1867))