Merge pull request #3925 from anoma/grarco/test-malleability-attacks

Test malleability attacks

.changelog/unreleased/testing/3925-test-malleability-attacks.md

@@ -0,0 +1,2 @@
+- Added property testing for malleability attacks on transactions.
+  ([\#3925](https://github.com/anoma/namada/pull/3925))

crates/apps_lib/src/client/tx.rs

@@ -1444,8 +1444,7 @@ pub async fn broadcast_tx(
 ///
 /// Checks that
 /// 1. The tx has been successfully included into the mempool of a validator
-/// 2. The tx with encrypted payload has been included on the blockchain
-/// 3. The decrypted payload of the tx has been included on the blockchain.
+/// 2. The tx has been included on the blockchain
 ///
 /// In the case of errors in any of those stages, an error message is returned
 pub async fn submit_tx(

crates/light_sdk/src/writing/asynchronous/mod.rs

@@ -12,8 +12,7 @@ use tendermint_rpc::HttpClient;
 ///
 /// Checks that
 /// 1. The tx has been successfully included into the mempool of a validator
-/// 2. The tx with encrypted payload has been included on the blockchain
-/// 3. The decrypted payload of the tx has been included on the blockchain.
+/// 2. The tx has been included on the blockchain
 ///
 /// In the case of errors in any of those stages, an error message is returned
 pub async fn broadcast_tx(

crates/light_sdk/src/writing/blocking/mod.rs

@@ -14,8 +14,7 @@ use tokio::runtime::Runtime;
 ///
 /// Checks that
 /// 1. The tx has been successfully included into the mempool of a validator
-/// 2. The tx with encrypted payload has been included on the blockchain
-/// 3. The decrypted payload of the tx has been included on the blockchain.
+/// 2. The tx has been included on the blockchain
 ///
 /// In the case of errors in any of those stages, an error message is returned
 pub fn broadcast_tx(tendermint_addr: &str, tx: Tx) -> Result<Response, Error> {
@@ -31,7 +30,7 @@ pub fn broadcast_tx(tendermint_addr: &str, tx: Tx) -> Result<Response, Error> {
     let rt = Runtime::new().unwrap();
 
     let wrapper_tx_hash = tx.header_hash().to_string();
-    // We use this to determine when the decrypted inner tx makes it
+    // We use this to determine when the inner tx makes it
     // on-chain
     let decrypted_tx_hash = tx.raw_header_hash().to_string();
 

crates/node/src/protocol.rs

@@ -1401,6 +1401,7 @@ fn merge_vp_results(
 #[cfg(test)]
 mod tests {
     use eyre::Result;
+    use namada_sdk::account::pks_handle;
     use namada_sdk::chain::BlockHeight;
     use namada_sdk::collections::HashMap;
     use namada_sdk::eth_bridge::protocol::transactions::votes::{
@@ -1413,11 +1414,18 @@ mod tests {
     use namada_sdk::ethereum_events::testing::DAI_ERC20_ETH_ADDRESS;
     use namada_sdk::ethereum_events::{EthereumEvent, TransferToNamada};
     use namada_sdk::keccak::keccak_hash;
+    use namada_sdk::key::RefTo;
+    use namada_sdk::testing::{
+        arb_tampered_inner_tx, arb_valid_signed_inner_tx,
+    };
     use namada_sdk::tx::{SignableEthMessage, Signed};
     use namada_sdk::voting_power::FractionalVotingPower;
     use namada_sdk::{address, key};
+    use namada_test_utils::TestWasms;
     use namada_vote_ext::bridge_pool_roots::BridgePoolRootVext;
     use namada_vote_ext::ethereum_events::EthereumEventsVext;
+    use namada_vp::state::StorageWrite;
+    use proptest::test_runner::{Config, TestRunner};
 
     use super::*;
 
@@ -1630,4 +1638,78 @@ mod tests {
         );
         assert!(matches!(result.unwrap_err(), Error::GasError(_)));
     }
+
+    // Test that the host function for signature verification we expose allows
+    // the vps to detect a tx that has been tampered with
+    #[test]
+    fn test_tampered_inner_tx_rejected() {
+        let (mut state, _validators) = test_utils::setup_default_storage();
+        let signing_key = key::testing::keypair_1();
+        let pk = signing_key.ref_to();
+        let addr = Address::from(&pk);
+
+        // Reveal the pk
+        pks_handle(&addr)
+            .insert(&mut state, 0_u8, pk.clone())
+            .unwrap();
+
+        // Allowlist the vp for the signature verification
+        let vp_code = TestWasms::VpVerifySignature.read_bytes();
+        // store the wasm code
+        let code_hash = Hash::sha256(&vp_code);
+        let key = namada_sdk::storage::Key::wasm_code(&code_hash);
+        let len_key = namada_sdk::storage::Key::wasm_code_len(&code_hash);
+        let code_len = vp_code.len() as u64;
+        state.write(&key, vp_code).unwrap();
+        state.write(&len_key, code_len).unwrap();
+
+        let (vp_cache, _) = wasm::compilation_cache::common::testing::cache();
+
+        let mut runner = TestRunner::new(Config::default());
+        // Test that the strategy produces valid txs first
+        let result =
+            runner.run(&arb_valid_signed_inner_tx(signing_key.clone()), |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_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| {
+            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/node/src/shell/block_alloc.rs

@@ -390,7 +390,7 @@ mod tests {
         // reserve block space for protocol txs
         let mut alloc = BsaInitialProtocolTxs::init(BLOCK_SIZE, BLOCK_GAS);
 
-        // allocate ~1/2 of the block space to encrypted txs
+        // allocate ~1/2 of the block space to wrapper txs
         assert!(alloc.try_alloc(&[0; 29]).is_ok());
 
         // reserve block space for normal txs
@@ -504,7 +504,7 @@ mod tests {
         // Fill the entire gas bin
         bins.normal_txs.gas.occupied = bins.normal_txs.gas.allotted;
 
-        // Make sure we can't dump any new wncrypted txs in the bin
+        // Make sure we can't dump any new wrapper txs in the bin
         assert_matches!(
             bins.try_alloc(BlockResources::new(b"arbitrary tx bytes", 1)),
             Err(AllocFailure::Rejected { .. })

crates/node/src/shell/block_alloc/states.rs

@@ -38,7 +38,7 @@ pub struct BuildingProtocolTxBatch<Mode> {
 /// [`crate::shell::block_alloc::states`].
 pub enum WithNormalTxs {}
 
-/// Allow block proposals to include encrypted txs.
+/// Allow block proposals to include wrapper txs.
 ///
 /// For more info, read the module docs of
 /// [`crate::shell::block_alloc::states`].

crates/node/src/shell/finalize_block.rs

@@ -1357,9 +1357,8 @@ mod test_finalize_block {
                 WRAPPER_GAS_LIMIT.into(),
             ))));
         wrapper_tx.header.chain_id = shell.chain_id.clone();
-        wrapper_tx.set_data(Data::new(
-            "Encrypted transaction data".as_bytes().to_owned(),
-        ));
+        wrapper_tx
+            .set_data(Data::new("transaction data".as_bytes().to_owned()));
         wrapper_tx.set_code(Code::new(tx_code, None));
         wrapper_tx.add_section(Section::Authorization(Authorization::new(
             wrapper_tx.sechashes(),
@@ -3478,9 +3477,8 @@ mod test_finalize_block {
                     0.into(),
                 ))));
             wrapper_tx.header.chain_id = shell.chain_id.clone();
-            wrapper_tx.set_data(Data::new(
-                "Encrypted transaction data".as_bytes().to_owned(),
-            ));
+            wrapper_tx
+                .set_data(Data::new("transaction data".as_bytes().to_owned()));
             wrapper_tx.add_code(TestWasms::TxNoOp.read_bytes(), None);
             wrapper_tx.sign_wrapper(keypair.clone());
             wrapper_tx
@@ -3514,7 +3512,7 @@ mod test_finalize_block {
 
         failing_wrapper
             .add_code(TestWasms::TxFail.read_bytes(), None)
-            .add_data("Encrypted transaction data");
+            .add_data("transaction data");
 
         let mut wrong_commitment_wrapper = failing_wrapper.clone();
         let tx_code = TestWasms::TxInvalidData.read_bytes();
@@ -3691,9 +3689,7 @@ mod test_finalize_block {
             ))));
         wrapper.header.chain_id = shell.chain_id.clone();
         wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned(), None));
-        wrapper.set_data(Data::new(
-            "Encrypted transaction data".as_bytes().to_owned(),
-        ));
+        wrapper.set_data(Data::new("transaction data".as_bytes().to_owned()));
         wrapper.add_section(Section::Authorization(Authorization::new(
             wrapper.sechashes(),
             [(0, keypair)].into_iter().collect(),

crates/node/src/shell/mod.rs

@@ -2491,7 +2491,7 @@ mod shell_tests {
         )
     }
 
-    /// Mempool validation must reject already applied wrapper and decrypted
+    /// Mempool validation must reject already applied wrapper and inner
     /// transactions
     #[test]
     fn test_replay_attack() {

crates/node/src/shell/prepare_proposal.rs

@@ -56,7 +56,7 @@ where
             let (alloc, mut txs) =
                 self.build_protocol_tx_with_normal_txs(alloc, &mut req.txs);
 
-            // add encrypted txs
+            // add wrapper txs
             let tm_raw_hash_string =
                 tm_raw_hash_to_string(req.proposer_address);
             let block_proposer =
@@ -100,7 +100,7 @@ where
         self.state.read_only().into()
     }
 
-    /// Builds a batch of encrypted transactions, retrieved from
+    /// Builds a batch of wrapper  transactions, retrieved from
     /// CometBFT's mempool.
     fn build_normal_txs(
         &self,
@@ -763,7 +763,7 @@ mod test_prepare_proposal {
         assert_eq!(signed_eth_ev_vote_extension, rsp_ext.0);
     }
 
-    /// Test that if the unsigned wrapper tx hash is known (replay attack), the
+    /// Test that if the wrapper tx hash is known (replay attack), the
     /// transaction is not included in the block
     #[test]
     fn test_wrapper_tx_hash() {
@@ -830,7 +830,7 @@ mod test_prepare_proposal {
         assert_eq!(received_txs.len(), 1);
     }
 
-    /// Test that if the unsigned inner tx hash is known (replay attack), the
+    /// Test that if the inner tx hash is known (replay attack), the
     /// transaction is not included in the block
     #[test]
     fn test_inner_tx_hash() {
@@ -870,7 +870,7 @@ mod test_prepare_proposal {
         assert_eq!(received_txs.len(), 0);
     }
 
-    /// Test that if two identical decrypted txs are proposed for this block,
+    /// Test that if two identical inner txs are proposed for this block,
     /// both get accepted
     #[test]
     fn test_inner_tx_hash_same_block() {