diff --git a/.changelog/unreleased/testing/3925-test-malleability-attacks.md b/.changelog/unreleased/testing/3925-test-malleability-attacks.md new file mode 100644 index 00000000000..5a0882a1db7 --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index f28bf942cf9..770f7886b86 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/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( diff --git a/crates/light_sdk/src/writing/asynchronous/mod.rs b/crates/light_sdk/src/writing/asynchronous/mod.rs index a71b747bfbf..4f47cc3cd1e 100644 --- a/crates/light_sdk/src/writing/asynchronous/mod.rs +++ b/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( diff --git a/crates/light_sdk/src/writing/blocking/mod.rs b/crates/light_sdk/src/writing/blocking/mod.rs index ec0cd2a4cae..776e88ba163 100644 --- a/crates/light_sdk/src/writing/blocking/mod.rs +++ b/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 { @@ -31,7 +30,7 @@ pub fn broadcast_tx(tendermint_addr: &str, tx: Tx) -> Result { 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(); diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index d099fb0dbb5..826f06f625c 100644 --- a/crates/node/src/protocol.rs +++ b/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()); + } } diff --git a/crates/node/src/shell/block_alloc.rs b/crates/node/src/shell/block_alloc.rs index cbf7893f1af..a20b05fb4a2 100644 --- a/crates/node/src/shell/block_alloc.rs +++ b/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 { .. }) diff --git a/crates/node/src/shell/block_alloc/states.rs b/crates/node/src/shell/block_alloc/states.rs index 3fcc57a0ad8..eb10d34a2f7 100644 --- a/crates/node/src/shell/block_alloc/states.rs +++ b/crates/node/src/shell/block_alloc/states.rs @@ -38,7 +38,7 @@ pub struct BuildingProtocolTxBatch { /// [`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`]. diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 636ed56d3fc..1d3663ab5b0 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/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(), diff --git a/crates/node/src/shell/mod.rs b/crates/node/src/shell/mod.rs index 3c4acfb4add..ffb176ff1ae 100644 --- a/crates/node/src/shell/mod.rs +++ b/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() { diff --git a/crates/node/src/shell/prepare_proposal.rs b/crates/node/src/shell/prepare_proposal.rs index b0e31a23097..3fc98507f50 100644 --- a/crates/node/src/shell/prepare_proposal.rs +++ b/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() { diff --git a/crates/node/src/shell/process_proposal.rs b/crates/node/src/shell/process_proposal.rs index 3b9069b1c6e..cc34042339c 100644 --- a/crates/node/src/shell/process_proposal.rs +++ b/crates/node/src/shell/process_proposal.rs @@ -171,9 +171,7 @@ where /// Checks if the Tx can be deserialized from bytes. Checks the fees and /// signatures of the fee payer for a transaction if it is a wrapper tx. /// - /// Checks validity of a decrypted tx or that a tx marked un-decryptable - /// is in fact so. Also checks that decrypted txs were submitted in - /// correct order. + /// Checks validity of an inner tx. /// /// Error codes: /// 0: Ok @@ -597,12 +595,15 @@ mod test_process_proposal { }; use namada_sdk::key::*; use namada_sdk::state::StorageWrite; + use namada_sdk::testing::{arb_tampered_wrapper_tx, arb_valid_signed_tx}; use namada_sdk::token::{read_denom, Amount, DenominatedAmount}; use namada_sdk::tx::data::Fee; use namada_sdk::tx::{Code, Data, Signed}; use namada_vote_ext::{ bridge_pool_roots, ethereum_events, validator_set_update, }; + use proptest::test_runner::{Config, TestCaseError, TestRunner}; + use proptest::{prop_assert, prop_assert_eq}; use super::*; use crate::shell::test_utils::{ @@ -943,77 +944,81 @@ mod test_process_proposal { /// Test that a block including a wrapper tx with invalid signature is /// rejected #[test] - fn test_wrapper_bad_signature_rejected() { + fn test_wrapper_bad_signature() { let (shell, _recv, _, _) = test_utils::setup_at_height(3u64); - let keypair = gen_keypair(); - let mut outer_tx = - Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( - Fee { - amount_per_gas_unit: DenominatedAmount::native( - Amount::from_uint(100, 0).expect("Test failed"), - ), - token: shell.state.in_mem().native_token.clone(), - }, - keypair.ref_to(), - GAS_LIMIT.into(), - )))); - outer_tx.header.chain_id = shell.chain_id.clone(); - outer_tx.set_code(Code::new("wasm_code".as_bytes().to_owned(), None)); - outer_tx.set_data(Data::new("transaction data".as_bytes().to_owned())); - outer_tx.sign_wrapper(keypair); - let mut new_tx = outer_tx.clone(); - if let TxType::Wrapper(wrapper) = &mut new_tx.header.tx_type { - // we mount a malleability attack to try and remove the fee - wrapper.fee.amount_per_gas_unit = - DenominatedAmount::native(Default::default()); - } else { - panic!("Test failed") - }; - let request = ProcessProposal { - txs: vec![new_tx.to_bytes()], - }; - match shell.process_proposal(request) { - Ok(_) => panic!("Test failed"), - Err(TestError::RejectProposal(response)) => { - let response = if let [response] = response.as_slice() { - response.clone() - } else { - panic!("Test failed") - }; - let expected_error = "WrapperTx signature verification \ - failed: The wrapper signature is \ - invalid."; - assert_eq!( - response.result.code, - u32::from(ResultCode::InvalidSig) - ); - assert!( - response.result.info.contains(expected_error), - "Result info {} doesn't contain the expected error {}", - response.result.info, - expected_error - ); + let mut runner = TestRunner::new(Config::default()); + // Test that the strategy produces valid txs first + let result = + runner.run(&arb_valid_signed_tx(), |tx| match tx.validate_tx() { + Ok(Some(_)) => Ok(()), + _ => { + Err(TestCaseError::fail("Unexpectedly produced invalid tx")) + } + }); + assert!(result.is_ok()); + + // Then test invalid tx + let mut runner = TestRunner::new(Config::default()); + let result = runner.run(&arb_tampered_wrapper_tx(), |tx| { + let request = ProcessProposal { + txs: vec![tx.to_bytes()], + }; + + match shell.process_proposal(request) { + Ok(_) => Err(TestCaseError::fail( + "Tampered transaction was mistakenly accepted", + )), + Err(TestError::RejectProposal(response)) => { + let response = if let [response] = response.as_slice() { + response.clone() + } else { + return Err(TestCaseError::fail("Missing tx result")); + }; + let expected_error = "WrapperTx signature verification \ + failed: The wrapper signature is \ + invalid."; + prop_assert_eq!( + response.result.code, + u32::from(ResultCode::InvalidSig) + ); + prop_assert!( + response.result.info.contains(expected_error), + "Result info {} doesn't contain the expected error {}", + response.result.info, + expected_error + ); + + Ok(()) + } } - } + }); + assert!(result.is_ok()); } - /// Test that if the account submitting the tx is not known and the fee is - /// non-zero, [`process_proposal`] rejects that block + /// Test that an implicit account does not need to reveal the pk for fee + /// payment #[test] fn test_wrapper_unknown_address() { let (mut shell, _recv, _, _) = test_utils::setup_at_height(3u64); let keypair = gen_keypair(); - // reduce address balance to match the 100 token min fee + let address = Address::from(&keypair.ref_to()); let balance_key = token::storage_key::balance_key( &shell.state.in_mem().native_token, - &Address::from(&keypair.ref_to()), + &address, ); shell .state - .write(&balance_key, Amount::native_whole(99)) + .write(&balance_key, Amount::native_whole(GAS_LIMIT)) .unwrap(); - let keypair = gen_keypair(); + shell.commit(); + // Verify that the public key associated with the fee payer has not been + // revealed + assert!( + namada_sdk::account::public_keys(&shell.state, &address) + .unwrap() + .is_empty() + ); let mut outer_tx = Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { @@ -1028,34 +1033,15 @@ mod test_process_proposal { outer_tx.set_data(Data::new("transaction data".as_bytes().to_owned())); outer_tx.sign_wrapper(keypair); - let response = { - let request = ProcessProposal { - txs: vec![outer_tx.to_bytes()], - }; - if let Err(TestError::RejectProposal(resp)) = - shell.process_proposal(request) - { - if let [resp] = resp.as_slice() { - resp.clone() - } else { - panic!("Test failed") - } - } else { - panic!("Test failed") - } + let request = ProcessProposal { + txs: vec![outer_tx.to_bytes()], }; - assert_eq!(response.result.code, u32::from(ResultCode::FeeError)); - assert!(response.result.info.contains( - "Error trying to apply a transaction: Error while processing \ - transaction's fees: The first transaction in the batch failed to \ - pay fees via the MASP. Wasm run failed: Transaction runner \ - error: Wasm validation error" - )); + let response = shell.process_proposal(request).unwrap(); + assert_eq!(response[0].result.code, u32::from(ResultCode::Ok)); } - /// Test that if the account submitting the tx does - /// not have sufficient balance to pay the fee, - /// [`process_proposal`] rejects the entire block + /// Test that if the account submitting the tx does not have sufficient + /// balance to pay the fee, [`process_proposal`] rejects the entire block #[test] fn test_wrapper_insufficient_balance_address() { let (mut shell, _recv, _, _) = test_utils::setup_at_height(3u64); @@ -1149,7 +1135,7 @@ mod test_process_proposal { ); } - /// Test that if the unsigned wrapper tx hash is known (replay attack), the + /// Test that if the uwrapper tx hash is known (replay attack), the /// block is rejected #[test] fn test_wrapper_tx_hash() { @@ -1261,7 +1247,7 @@ mod test_process_proposal { } } - /// 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 /// block is rejected #[test] fn test_inner_tx_hash() { diff --git a/crates/sdk/src/lib.rs b/crates/sdk/src/lib.rs index b6000d5c6c1..fd186d13466 100644 --- a/crates/sdk/src/lib.rs +++ b/crates/sdk/src/lib.rs @@ -885,6 +885,7 @@ pub mod testing { }; use namada_tx::data::{Fee, TxType, WrapperTx}; use proptest::prelude::{Just, Strategy}; + use proptest::sample::SizeRange; use proptest::{arbitrary, collection, option, prop_compose, prop_oneof}; use token::testing::arb_transparent_transfer; @@ -971,7 +972,7 @@ pub mod testing { } prop_compose! { - /// Generate an arbitrary uttf8 commitment + /// Generate an arbitrary utf8 commitment pub fn arb_utf8_commitment()( commitment in prop_oneof![ arb_hash().prop_map(Commitment::Hash), @@ -1059,15 +1060,36 @@ pub mod testing { } } + prop_compose! { + /// Generate an arbitrary tx commitments + pub fn arb_tx_commitment()( + code_hash in arb_hash(), + data_hash in arb_hash(), + memo_hash in arb_hash(), + ) -> TxCommitments { + TxCommitments { + data_hash, + code_hash, + memo_hash + } + } + } + + /// Generate an arbitrary number of tx commitments + pub fn arb_tx_commitments( + number_of_cmts: impl Into, + ) -> impl Strategy> { + collection::hash_set(arb_tx_commitment(), number_of_cmts) + .prop_map(|s| s.into_iter().collect()) + } + prop_compose! { /// Generate an arbitrary header pub fn arb_header()( chain_id in arb_chain_id(), expiration in option::of(arb_date_time_utc()), timestamp in arb_date_time_utc(), - code_hash in arb_hash(), - data_hash in arb_hash(), - memo_hash in arb_hash(), + batch in arb_tx_commitments(1..10), atomic in proptest::bool::ANY, tx_type in arb_tx_type(), ) -> Header { @@ -1075,12 +1097,7 @@ pub mod testing { chain_id, expiration, timestamp, - //TODO: arbitrary number of commitments - batch: [TxCommitments{ - data_hash, - code_hash, - memo_hash, - }].into(), + batch, atomic, tx_type, } @@ -1656,4 +1673,120 @@ pub mod testing { (tx.0, tx.1) } } + + prop_compose! { + /// Generate an arbitrary tx with a valid wrapper signature + pub fn arb_valid_signed_tx() + ( + (mut tx, _data) in arb_memoed_tx(), + signer in arb_common_keypair(), + ) -> Tx { + // Sign the wrapper tx + let mut wrapper = tx.header.wrapper().unwrap(); + wrapper.pk = signer.to_public(); + tx.update_header(TxType::Wrapper(Box::new(wrapper))); + tx.sign_wrapper(signer); + + tx + } + } + + prop_compose! { + /// Generate an arbitrary tx with a valid raw signature + pub fn arb_valid_signed_inner_tx(signer: common::SecretKey) + ( + (mut tx, _data) in arb_memoed_tx(), + ) -> Tx { + tx.update_header(TxType::Raw); + // Sign the inner tx + tx.sign_raw( + vec![signer.clone()], + vec![signer.ref_to()].into_iter().collect(), + None + ); + + tx + } + } + + // An enumeration representing different ways to tamper with a transaction + #[derive(Debug, Clone)] + enum TamperTx { + RemoveSection, + AddExtraSection, + SwapSection, + SwapHeader, + } + + prop_compose! { + /// Generate an arbitrary signed wrapped tx that has been tampered with. + pub fn arb_tampered_wrapper_tx() + (tx1 in arb_valid_signed_tx())( + tamper in prop_oneof![ + Just(TamperTx::RemoveSection), + Just(TamperTx::AddExtraSection), + Just(TamperTx::SwapSection), + Just(TamperTx::SwapHeader) + ], + tx2 in arb_signed_tx(), + selector in proptest::prelude::any::(), + mut tx in Just(tx1), + ) -> Tx { + match tamper { + TamperTx::RemoveSection => { + let to_remove = selector.select(&tx.sections).to_owned(); + tx.sections.retain(|section| section != &to_remove); + + tx + }, + TamperTx::AddExtraSection => { + let to_add = selector.select(&tx2.0.sections).to_owned(); + tx.sections.push(to_add); + + tx + }, + TamperTx::SwapSection => { + let mut to_remove = selector.select(&tx.sections).to_owned(); + let mut to_add = selector.select(&tx2.0.sections).to_owned(); + + // Try to pick different sections of the same type for the swap if possible + for source in tx.sections.iter() { + if let Some(target) = tx2.0.sections.iter().find(|section| { + std::mem::discriminant(*section) == std::mem::discriminant(&to_remove) && section.get_hash() != source.get_hash() + }) { + to_remove = source.to_owned(); + to_add = target.to_owned(); + break; + } + } + + tx.sections.retain(|section| section != &to_remove); + tx.sections.push(to_add); + + tx + }, + TamperTx::SwapHeader => { + // Maintain the original wrapper signer + let mut new_wrapper = tx2.0.header.wrapper().unwrap(); + new_wrapper.pk = tx.header.wrapper().unwrap().pk; + tx.update_header(TxType::Wrapper(Box::new(new_wrapper))); + + tx + }, + } + } + } + + prop_compose! { + /// Generate an arbitrary signed inner tx that has been tampered with. + pub fn arb_tampered_inner_tx(signer: common::SecretKey) + (tx1 in arb_valid_signed_inner_tx(signer.clone()))( + tx2 in arb_valid_signed_inner_tx(signer.clone()), + mut tx in Just(tx1), + ) -> Tx { + // Tamper with the header only since signature is computed on this alone + tx.header = tx2.header; + tx + } + } } diff --git a/crates/sdk/src/rpc.rs b/crates/sdk/src/rpc.rs index 52dbf06bf37..ba7cfd94a79 100644 --- a/crates/sdk/src/rpc.rs +++ b/crates/sdk/src/rpc.rs @@ -616,12 +616,9 @@ pub async fn dry_run_tx( Ok(result) } -/// Data needed for broadcasting a tx and -/// monitoring its progress on chain +/// Data needed for broadcasting a tx and monitoring its progress on chain. /// -/// Txs may be either a dry run or else -/// they should be encrypted and included -/// in a wrapper. +/// Txs may be either a dry run or else they should be included in a wrapper. #[derive(Debug, Clone)] pub enum TxBroadcastData { /// Dry run broadcast data diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 33d7a6c89b3..be37d4af508 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -265,7 +265,7 @@ pub async fn process_tx( let tx_hash = tx.header_hash().to_string(); let cmts = tx.commitments().clone(); let wrapper_hash = tx.wrapper_hash(); - // 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 to_broadcast = TxBroadcastData::Live { tx, tx_hash }; if args.broadcast_only { @@ -393,8 +393,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( diff --git a/crates/test_utils/src/lib.rs b/crates/test_utils/src/lib.rs index 0f1f17581dc..e2b1e566007 100644 --- a/crates/test_utils/src/lib.rs +++ b/crates/test_utils/src/lib.rs @@ -39,6 +39,7 @@ pub enum TestWasms { VpInfiniteHostGas, VpMemoryLimit, VpReadStorageKey, + VpVerifySignature, } impl TestWasms { @@ -69,6 +70,7 @@ impl TestWasms { TestWasms::VpInfiniteHostGas => "vp_infinite_host_gas.wasm", TestWasms::VpMemoryLimit => "vp_memory_limit.wasm", TestWasms::VpReadStorageKey => "vp_read_storage_key.wasm", + TestWasms::VpVerifySignature => "vp_verify_signature.wasm", }; let cwd = env::current_dir().expect("Couldn't get current working directory"); diff --git a/crates/tx/src/data/mod.rs b/crates/tx/src/data/mod.rs index a299ac9b2e9..b2d6d04b087 100644 --- a/crates/tx/src/data/mod.rs +++ b/crates/tx/src/data/mod.rs @@ -7,7 +7,7 @@ pub mod pgf; pub mod pos; /// transaction protocols made by validators pub mod protocol; -/// wrapper txs with encrypted payloads +/// wrapper txs pub mod wrapper; use std::collections::{BTreeMap, BTreeSet}; diff --git a/crates/tx/src/data/wrapper.rs b/crates/tx/src/data/wrapper.rs index 1f030dd67f1..943c3b2287a 100644 --- a/crates/tx/src/data/wrapper.rs +++ b/crates/tx/src/data/wrapper.rs @@ -137,9 +137,8 @@ impl GasLimit { } } -/// A transaction with an encrypted payload, an optional shielded pool -/// unshielding tx for fee payment and some non-encrypted metadata for -/// inclusion and / or verification purposes +/// A wrapper transaction with some metadata for inclusion and / or verification +/// purposes #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] #[derive( Debug, diff --git a/crates/tx/src/types.rs b/crates/tx/src/types.rs index 01de49e86ad..e800caa5528 100644 --- a/crates/tx/src/types.rs +++ b/crates/tx/src/types.rs @@ -46,8 +46,6 @@ pub enum DecodeError { InvalidJSONDeserialization(String), } -/// Errors relating to decrypting a wrapper tx and its -/// encrypted payload from a Tx type #[allow(missing_docs)] #[derive(thiserror::Error, Debug, PartialEq)] pub enum TxError { @@ -206,7 +204,7 @@ impl Tx { Section::Header(self.header.clone()).get_hash() } - /// Gets the hash of the decrypted transaction's header + /// Gets the hash of the raw transaction's header pub fn raw_header_hash(&self) -> namada_core::hash::Hash { let mut raw_header = self.header(); raw_header.tx_type = TxType::Raw; @@ -544,15 +542,9 @@ impl Tx { /// Determines the type of the input Tx /// - /// If it is a raw Tx, signed or not, the Tx is - /// returned unchanged inside an enum variant stating its type. + /// If it is a raw Tx, signed or not, we return `None`. /// - /// If it is a decrypted tx, signing it adds no security so we - /// extract the signed data without checking the signature (if it - /// is signed) or return as is. Either way, it is returned in - /// an enum variant stating its type. - /// - /// If it is a WrapperTx, we extract the signed data of + /// If it is a WrapperTx or ProtocolTx, we extract the signed data of /// the Tx and verify it is of the appropriate form. This means /// 1. The wrapper tx is indeed signed /// 2. The signature is valid diff --git a/wasm_for_tests/Cargo.lock b/wasm_for_tests/Cargo.lock index 3c92182500b..90a3606b0b6 100644 --- a/wasm_for_tests/Cargo.lock +++ b/wasm_for_tests/Cargo.lock @@ -3973,6 +3973,15 @@ dependencies = [ "rlsf", ] +[[package]] +name = "vp_verify_signature" +version = "0.41.0" +dependencies = [ + "getrandom", + "namada_vp_prelude", + "rlsf", +] + [[package]] name = "wasi" version = "0.11.0+wasi-snapshot-preview1" diff --git a/wasm_for_tests/Cargo.toml b/wasm_for_tests/Cargo.toml index 74655bd3083..85617c3556b 100644 --- a/wasm_for_tests/Cargo.toml +++ b/wasm_for_tests/Cargo.toml @@ -23,6 +23,7 @@ members = [ "vp_infinite_host_gas", "vp_memory_limit", "vp_read_storage_key", + "vp_verify_signature" ] [workspace.package] diff --git a/wasm_for_tests/Makefile b/wasm_for_tests/Makefile index 5e922786984..f54f4ab6ebb 100644 --- a/wasm_for_tests/Makefile +++ b/wasm_for_tests/Makefile @@ -26,6 +26,7 @@ wasms += vp_infinite_guest_gas wasms += vp_infinite_host_gas wasms += vp_memory_limit wasms += vp_read_storage_key +wasms += vp_verify_signature # Build all wasms in release mode diff --git a/wasm_for_tests/vp_verify_signature/Cargo.toml b/wasm_for_tests/vp_verify_signature/Cargo.toml new file mode 100644 index 00000000000..99408d668ed --- /dev/null +++ b/wasm_for_tests/vp_verify_signature/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "vp_verify_signature" +description = "Wasm vp used for testing." +authors.workspace = true +edition.workspace = true +license.workspace = true +version.workspace = true + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] +namada_vp_prelude.workspace = true +rlsf.workspace = true +getrandom.workspace = true + +[lib] +crate-type = ["cdylib"] diff --git a/wasm_for_tests/vp_verify_signature/src/lib.rs b/wasm_for_tests/vp_verify_signature/src/lib.rs new file mode 100644 index 00000000000..f31d1e4f05e --- /dev/null +++ b/wasm_for_tests/vp_verify_signature/src/lib.rs @@ -0,0 +1,13 @@ +use namada_vp_prelude::*; + +#[validity_predicate] +fn validate_tx( + ctx: &Ctx, + tx_data: BatchedTx, + addr: Address, + _keys_changed: BTreeSet, + _verifiers: BTreeSet
, +) -> VpResult { + let mut gadget = VerifySigGadget::new(); + gadget.verify_signatures(ctx, &tx_data.tx, &addr) +}