From 4f45b8139ffff29704a9a5ed102b074a168e5cfa Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 28 Aug 2023 16:43:37 +0200 Subject: [PATCH 1/8] Inner tx signer also signs tx header --- core/src/proto/types.rs | 9 ++++++++- shared/src/vm/host_env.rs | 2 +- vp_prelude/src/lib.rs | 9 ++++++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/core/src/proto/types.rs b/core/src/proto/types.rs index a6082fbbab..5b03c6c7ef 100644 --- a/core/src/proto/types.rs +++ b/core/src/proto/types.rs @@ -1764,8 +1764,15 @@ impl Tx { account_public_keys_map: AccountPublicKeysMap, signer: Option
, ) -> &mut Self { + // The inner tx signer signs the Raw version of the Header + let mut header = self.header(); + header.tx_type = TxType::Raw; + + let mut hashes = vec![Section::Header(header).get_hash()]; self.protocol_filter(); - let hashes = self.inner_section_targets(); + let sections_hashes = self.inner_section_targets(); + hashes.extend(sections_hashes); + self.add_section(Section::Signature(Signature::new( hashes, account_public_keys_map.index_secret_keys(keypairs), diff --git a/shared/src/vm/host_env.rs b/shared/src/vm/host_env.rs index 7806d1abef..3e3b78e31b 100644 --- a/shared/src/vm/host_env.rs +++ b/shared/src/vm/host_env.rs @@ -1809,7 +1809,7 @@ where let gas_meter = unsafe { env.ctx.gas_meter.get() }; vp_host_fns::add_gas(gas_meter, gas)?; - let hashes = <[Hash; 2]>::try_from_slice(&hash_list) + let hashes = <[Hash; 3]>::try_from_slice(&hash_list) .map_err(vp_host_fns::RuntimeError::EncodingError)?; let (public_keys_map, gas) = env diff --git a/vp_prelude/src/lib.rs b/vp_prelude/src/lib.rs index 0962628363..162d26b9dd 100644 --- a/vp_prelude/src/lib.rs +++ b/vp_prelude/src/lib.rs @@ -30,6 +30,7 @@ use namada_core::types::internal::HostEnvResult; use namada_core::types::storage::{ BlockHash, BlockHeight, Epoch, Header, TxIndex, BLOCK_HASH_LENGTH, }; +use namada_core::types::transaction::TxType; pub use namada_core::types::*; pub use namada_macros::validity_predicate; pub use namada_proof_of_stake::storage as proof_of_stake; @@ -88,7 +89,13 @@ pub fn verify_signatures(ctx: &Ctx, tx: &Tx, owner: &Address) -> VpResult { let threshold = storage_api::account::threshold(&ctx.pre(), owner)?.unwrap_or(1); - let targets = [*tx.data_sechash(), *tx.code_sechash()]; + let mut header = tx.header(); + header.tx_type = TxType::Raw; + let targets = [ + Section::Header(header).get_hash(), + *tx.data_sechash(), + *tx.code_sechash(), + ]; // Serialize parameters let max_signatures = max_signatures_per_transaction.try_to_vec().unwrap(); From ee31f471d6b9da98d61f33bb6aebebf6f0f0b04c Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 28 Aug 2023 18:44:32 +0200 Subject: [PATCH 2/8] Adds `raw_header_hash` method for `Tx` --- .../lib/node/ledger/shell/finalize_block.rs | 159 +++++++----------- apps/src/lib/node/ledger/shell/mod.rs | 23 +-- .../lib/node/ledger/shell/prepare_proposal.rs | 3 +- .../lib/node/ledger/shell/process_proposal.rs | 44 ++--- core/src/proto/types.rs | 13 +- shared/src/ledger/protocol/mod.rs | 4 +- vp_prelude/src/lib.rs | 9 +- 7 files changed, 103 insertions(+), 152 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 53252065f1..14c24addce 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -213,9 +213,7 @@ where .pop() .expect("Missing wrapper tx in queue") .tx - .clone() - .update_header(TxType::Raw) - .header_hash(); + .raw_header_hash(); let tx_hash_key = replay_protection::get_replay_protection_key(&tx_hash); self.wl_storage @@ -276,7 +274,7 @@ where continue; } - let (mut tx_event, tx_unsigned_hash, mut tx_gas_meter, wrapper) = + let (mut tx_event, tx_header_hash, mut tx_gas_meter, wrapper) = match &tx_header.tx_type { TxType::Wrapper(wrapper) => { stats.increment_wrapper_txs(); @@ -286,7 +284,7 @@ where } TxType::Decrypted(inner) => { // We remove the corresponding wrapper tx from the queue - let mut tx_in_queue = self + let tx_in_queue = self .wl_storage .storage .tx_queue @@ -323,12 +321,7 @@ where ( event, - Some( - tx_in_queue - .tx - .update_header(TxType::Raw) - .header_hash(), - ), + Some(tx_in_queue.tx.raw_header_hash()), TxGasMeter::new_from_sub_limit(tx_in_queue.gas), None, ) @@ -511,7 +504,7 @@ where // If transaction type is Decrypted and failed because of // out of gas, remove its hash from storage to allow // rewrapping it - if let Some(hash) = tx_unsigned_hash { + if let Some(hash) = tx_header_hash { if let Error::TxApply(protocol::Error::GasError(_)) = msg { @@ -2081,11 +2074,9 @@ mod test_finalize_block { // won't receive votes from TM since we receive votes at a 1-block // delay, so votes will be empty here next_block_for_inflation(&mut shell, pkh1.clone(), vec![], None); - assert!( - rewards_accumulator_handle() - .is_empty(&shell.wl_storage) - .unwrap() - ); + assert!(rewards_accumulator_handle() + .is_empty(&shell.wl_storage) + .unwrap()); // FINALIZE BLOCK 2. Tell Namada that val1 is the block proposer. // Include votes that correspond to block 1. Make val2 the next block's @@ -2095,11 +2086,9 @@ mod test_finalize_block { assert!(rewards_prod_2.is_empty(&shell.wl_storage).unwrap()); assert!(rewards_prod_3.is_empty(&shell.wl_storage).unwrap()); assert!(rewards_prod_4.is_empty(&shell.wl_storage).unwrap()); - assert!( - !rewards_accumulator_handle() - .is_empty(&shell.wl_storage) - .unwrap() - ); + assert!(!rewards_accumulator_handle() + .is_empty(&shell.wl_storage) + .unwrap()); // Val1 was the proposer, so its reward should be larger than all // others, which should themselves all be equal let acc_sum = get_rewards_sum(&shell.wl_storage); @@ -2213,11 +2202,9 @@ mod test_finalize_block { None, ); } - assert!( - rewards_accumulator_handle() - .is_empty(&shell.wl_storage) - .unwrap() - ); + assert!(rewards_accumulator_handle() + .is_empty(&shell.wl_storage) + .unwrap()); let rp1 = rewards_prod_1 .get(&shell.wl_storage, &Epoch::default()) .unwrap() @@ -2307,26 +2294,22 @@ mod test_finalize_block { assert!(shell.shell.wl_storage.has_key(&wrapper_hash_key).unwrap()); assert!(shell.shell.wl_storage.has_key(&decrypted_hash_key).unwrap()); // Check that non of the hashes is present in the merkle tree - assert!( - !shell - .shell - .wl_storage - .storage - .block - .tree - .has_key(&wrapper_hash_key) - .unwrap() - ); - assert!( - !shell - .shell - .wl_storage - .storage - .block - .tree - .has_key(&decrypted_hash_key) - .unwrap() - ); + assert!(!shell + .shell + .wl_storage + .storage + .block + .tree + .has_key(&wrapper_hash_key) + .unwrap()); + assert!(!shell + .shell + .wl_storage + .storage + .block + .tree + .has_key(&decrypted_hash_key) + .unwrap()); } /// Test that if a decrypted transaction fails because of out-of-gas, its @@ -2362,7 +2345,7 @@ mod test_finalize_block { // Write inner hash in storage let inner_hash_key = replay_protection::get_replay_protection_key( - &wrapper_tx.clone().update_header(TxType::Raw).header_hash(), + &wrapper_tx.raw_header_hash(), ); shell .wl_storage @@ -2397,12 +2380,10 @@ mod test_finalize_block { let code = event.attributes.get("code").expect("Testfailed").as_str(); assert_eq!(code, String::from(ErrorCodes::WasmRuntimeError).as_str()); - assert!( - !shell - .wl_storage - .has_key(&inner_hash_key) - .expect("Test failed") - ) + assert!(!shell + .wl_storage + .has_key(&inner_hash_key) + .expect("Test failed")) } #[test] @@ -2439,7 +2420,7 @@ mod test_finalize_block { &wrapper.header_hash(), ); let inner_hash_key = replay_protection::get_replay_protection_key( - &wrapper.clone().update_header(TxType::Raw).header_hash(), + &wrapper.raw_header_hash(), ); let processed_tx = ProcessedTx { @@ -2463,18 +2444,14 @@ mod test_finalize_block { let code = event.attributes.get("code").expect("Testfailed").as_str(); assert_eq!(code, String::from(ErrorCodes::InvalidTx).as_str()); - assert!( - shell - .wl_storage - .has_key(&wrapper_hash_key) - .expect("Test failed") - ); - assert!( - !shell - .wl_storage - .has_key(&inner_hash_key) - .expect("Test failed") - ) + assert!(shell + .wl_storage + .has_key(&wrapper_hash_key) + .expect("Test failed")); + assert!(!shell + .wl_storage + .has_key(&inner_hash_key) + .expect("Test failed")) } // Test that if the fee payer doesn't have enough funds for fee payment the @@ -2761,11 +2738,9 @@ mod test_finalize_block { .unwrap(), Some(ValidatorState::Consensus) ); - assert!( - enqueued_slashes_handle() - .at(&Epoch::default()) - .is_empty(&shell.wl_storage)? - ); + assert!(enqueued_slashes_handle() + .at(&Epoch::default()) + .is_empty(&shell.wl_storage)?); assert_eq!( get_num_consensus_validators(&shell.wl_storage, Epoch::default()) .unwrap(), @@ -2784,21 +2759,17 @@ mod test_finalize_block { .unwrap(), Some(ValidatorState::Jailed) ); - assert!( - enqueued_slashes_handle() - .at(&epoch) - .is_empty(&shell.wl_storage)? - ); + assert!(enqueued_slashes_handle() + .at(&epoch) + .is_empty(&shell.wl_storage)?); assert_eq!( get_num_consensus_validators(&shell.wl_storage, epoch).unwrap(), 5_u64 ); } - assert!( - !enqueued_slashes_handle() - .at(&processing_epoch) - .is_empty(&shell.wl_storage)? - ); + assert!(!enqueued_slashes_handle() + .at(&processing_epoch) + .is_empty(&shell.wl_storage)?); // Advance to the processing epoch loop { @@ -2821,11 +2792,9 @@ mod test_finalize_block { // println!("Reached processing epoch"); break; } else { - assert!( - enqueued_slashes_handle() - .at(&shell.wl_storage.storage.block.epoch) - .is_empty(&shell.wl_storage)? - ); + assert!(enqueued_slashes_handle() + .at(&shell.wl_storage.storage.block.epoch) + .is_empty(&shell.wl_storage)?); let stake1 = read_validator_stake( &shell.wl_storage, ¶ms, @@ -3371,15 +3340,13 @@ mod test_finalize_block { ) .unwrap(); assert_eq!(last_slash, Some(Epoch(4))); - assert!( - namada_proof_of_stake::is_validator_frozen( - &shell.wl_storage, - &val1.address, - current_epoch, - ¶ms - ) - .unwrap() - ); + assert!(namada_proof_of_stake::is_validator_frozen( + &shell.wl_storage, + &val1.address, + current_epoch, + ¶ms + ) + .unwrap()); assert!( namada_proof_of_stake::validator_slashes_handle(&val1.address) .is_empty(&shell.wl_storage) diff --git a/apps/src/lib/node/ledger/shell/mod.rs b/apps/src/lib/node/ledger/shell/mod.rs index a1c17fe450..bd58e06af8 100644 --- a/apps/src/lib/node/ledger/shell/mod.rs +++ b/apps/src/lib/node/ledger/shell/mod.rs @@ -932,8 +932,7 @@ where tx_bytes: &[u8], temp_wl_storage: &mut TempWlStorage, ) -> Result<()> { - let inner_tx_hash = - wrapper.clone().update_header(TxType::Raw).header_hash(); + let inner_tx_hash = wrapper.raw_header_hash(); let inner_hash_key = replay_protection::get_replay_protection_key(&inner_tx_hash); if temp_wl_storage @@ -1089,22 +1088,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 { @@ -1263,11 +1259,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(); let inner_hash_key = - replay_protection::get_replay_protection_key(inner_tx_hash); + replay_protection::get_replay_protection_key( + &inner_tx_hash, + ); if self .wl_storage .storage @@ -2502,8 +2498,7 @@ mod 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_key(&inner_tx_hash); diff --git a/apps/src/lib/node/ledger/shell/prepare_proposal.rs b/apps/src/lib/node/ledger/shell/prepare_proposal.rs index 3687a6d39b..10f9cbc6c1 100644 --- a/apps/src/lib/node/ledger/shell/prepare_proposal.rs +++ b/apps/src/lib/node/ledger/shell/prepare_proposal.rs @@ -1279,8 +1279,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 = diff --git a/apps/src/lib/node/ledger/shell/process_proposal.rs b/apps/src/lib/node/ledger/shell/process_proposal.rs index ab544de3f8..281bd04399 100644 --- a/apps/src/lib/node/ledger/shell/process_proposal.rs +++ b/apps/src/lib/node/ledger/shell/process_proposal.rs @@ -721,14 +721,7 @@ where metadata.has_decrypted_txs = true; match tx_queue_iter.next() { Some(wrapper) => { - let mut inner_tx = tx.clone(); - inner_tx.update_header(TxType::Raw); - if wrapper - .tx - .clone() - .update_header(TxType::Raw) - .header_hash() - != inner_tx.header_hash() + if wrapper.tx.raw_header_hash() != tx.raw_header_hash() { TxResult { code: ErrorCodes::InvalidOrder.into(), @@ -742,7 +735,9 @@ where wrapper.tx.clone(), privkey, ) { - // Tx chain id + // FIXME: remove these first 2 checks (also from + // prepare proposal if they are there and finalize + // block?) Tx chain id if wrapper.tx.header.chain_id != self.chain_id { return TxResult { code: ErrorCodes::InvalidDecryptedChainId @@ -1124,13 +1119,11 @@ mod test_process_proposal { shell.chain_id.clone(), ) .to_bytes(); - assert!( - shell - .process_proposal(ProcessProposal { - txs: vec![tx.clone(), tx] - }) - .is_err() - ); + assert!(shell + .process_proposal(ProcessProposal { + txs: vec![tx.clone(), tx] + }) + .is_err()); } #[cfg(feature = "abcipp")] @@ -1287,11 +1280,9 @@ mod test_process_proposal { sig, } .sign(shell.mode.get_protocol_key().expect("Test failed")); - let mut txs = vec![ - EthereumTxData::BridgePool(vote_ext.into()) - .sign(protocol_key, shell.chain_id.clone()) - .to_bytes(), - ]; + let mut txs = vec![EthereumTxData::BridgePool(vote_ext.into()) + .sign(protocol_key, shell.chain_id.clone()) + .to_bytes()]; let event = EthereumEvent::TransfersToNamada { nonce: 0u64.into(), @@ -2226,10 +2217,7 @@ mod test_process_proposal { format!( "Transaction replay attempt: Inner transaction hash \ {} already in storage", - wrapper - .clone() - .update_header(TxType::Raw) - .header_hash(), + wrapper.raw_header_hash() ) ); } @@ -2263,10 +2251,9 @@ mod test_process_proposal { [(0, keypair)].into_iter().collect(), None, ))); - let inner_unsigned_hash = - wrapper.clone().update_header(TxType::Raw).header_hash(); // Write inner hash to storage + let inner_unsigned_hash = wrapper.raw_header_hash(); let hash_key = replay_protection::get_replay_protection_key(&inner_unsigned_hash); shell @@ -2327,8 +2314,7 @@ mod test_process_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(); new_wrapper.update_header(TxType::Wrapper(Box::new(WrapperTx::new( Fee { diff --git a/core/src/proto/types.rs b/core/src/proto/types.rs index 5b03c6c7ef..b5935e504b 100644 --- a/core/src/proto/types.rs +++ b/core/src/proto/types.rs @@ -1250,6 +1250,14 @@ impl Tx { Section::Header(self.header.clone()).get_hash() } + /// Gets the hash of the raw transaction's header + pub fn raw_header_hash(&self) -> crate::types::hash::Hash { + let mut raw_header = self.header(); + raw_header.tx_type = TxType::Raw; + + Section::Header(raw_header).get_hash() + } + /// Get hashes of all the sections in this transaction pub fn sechashes(&self) -> Vec { let mut hashes = vec![self.header_hash()]; @@ -1765,10 +1773,7 @@ impl Tx { signer: Option
, ) -> &mut Self { // The inner tx signer signs the Raw version of the Header - let mut header = self.header(); - header.tx_type = TxType::Raw; - - let mut hashes = vec![Section::Header(header).get_hash()]; + let mut hashes = vec![self.raw_header_hash()]; self.protocol_filter(); let sections_hashes = self.inner_section_targets(); hashes.extend(sections_hashes); diff --git a/shared/src/ledger/protocol/mod.rs b/shared/src/ledger/protocol/mod.rs index a23b026eea..2f2af49cd7 100644 --- a/shared/src/ledger/protocol/mod.rs +++ b/shared/src/ledger/protocol/mod.rs @@ -231,7 +231,7 @@ where WLS: WriteLogAndStorage, { let mut changed_keys = BTreeSet::default(); - let mut tx: Tx = tx_bytes.try_into().unwrap(); + let tx: Tx = tx_bytes.try_into().unwrap(); // Writes wrapper tx hash to block write log (changes must be persisted even // in case of failure) @@ -258,7 +258,7 @@ where // If wrapper was succesful, write inner tx hash to storage let inner_hash_key = replay_protection::get_replay_protection_key( - &hash::Hash(tx.update_header(TxType::Raw).header_hash().0), + &hash::Hash(tx.raw_header_hash().0), ); shell_params .wl_storage diff --git a/vp_prelude/src/lib.rs b/vp_prelude/src/lib.rs index 162d26b9dd..c42a29864a 100644 --- a/vp_prelude/src/lib.rs +++ b/vp_prelude/src/lib.rs @@ -89,13 +89,12 @@ pub fn verify_signatures(ctx: &Ctx, tx: &Tx, owner: &Address) -> VpResult { let threshold = storage_api::account::threshold(&ctx.pre(), owner)?.unwrap_or(1); + // FIXME: add a test to check the invalid signature in vp of the tx header + // hash FIXME: tryo a replay attack on a local devnet let mut header = tx.header(); header.tx_type = TxType::Raw; - let targets = [ - Section::Header(header).get_hash(), - *tx.data_sechash(), - *tx.code_sechash(), - ]; + let targets = + [tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()]; // Serialize parameters let max_signatures = max_signatures_per_transaction.try_to_vec().unwrap(); From 673c99a1a391f1bfeeeb6fceb97970a953038005 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 28 Aug 2023 19:10:11 +0200 Subject: [PATCH 3/8] Removes useless checks on decrypted tx, error codes and unit tests --- apps/src/lib/node/ledger/shell/mod.rs | 35 ++-- .../lib/node/ledger/shell/process_proposal.rs | 151 ------------------ tests/src/e2e/ledger_tests.rs | 4 +- 3 files changed, 16 insertions(+), 174 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/mod.rs b/apps/src/lib/node/ledger/shell/mod.rs index bd58e06af8..5c393ad85c 100644 --- a/apps/src/lib/node/ledger/shell/mod.rs +++ b/apps/src/lib/node/ledger/shell/mod.rs @@ -146,22 +146,19 @@ impl From 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 { @@ -172,11 +169,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, diff --git a/apps/src/lib/node/ledger/shell/process_proposal.rs b/apps/src/lib/node/ledger/shell/process_proposal.rs index 281bd04399..e79d3dba42 100644 --- a/apps/src/lib/node/ledger/shell/process_proposal.rs +++ b/apps/src/lib/node/ledger/shell/process_proposal.rs @@ -735,37 +735,6 @@ where wrapper.tx.clone(), privkey, ) { - // FIXME: remove these first 2 checks (also from - // prepare proposal if they are there and finalize - // block?) Tx chain id - if wrapper.tx.header.chain_id != self.chain_id { - return TxResult { - code: ErrorCodes::InvalidDecryptedChainId - .into(), - info: format!( - "Decrypted tx carries a wrong chain \ - id: expected {}, found {}", - self.chain_id, - wrapper.tx.header.chain_id - ), - }; - } - - // Tx expiration - if let Some(exp) = wrapper.tx.header.expiration { - if block_time > exp { - return TxResult { - code: ErrorCodes::ExpiredDecryptedTx - .into(), - info: format!( - "Decrypted tx expired at {:#?}, \ - block time: {:#?}", - exp, block_time - ), - }; - } - } - TxResult { code: ErrorCodes::Ok.into(), info: "Process Proposal accepted this \ @@ -2418,70 +2387,6 @@ mod test_process_proposal { } } - /// Test that a decrypted transaction with a mismatching chain id gets - /// rejected without rejecting the entire block - #[test] - fn test_decrypted_wrong_chain_id() { - let (mut shell, _recv, _, _) = test_utils::setup(); - let keypair = crate::wallet::defaults::daewon_keypair(); - - let wrong_chain_id = ChainId("Wrong chain id".to_string()); - let mut wrapper = - Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( - Fee { - amount_per_gas_unit: token::Amount::zero(), - token: shell.wl_storage.storage.native_token.clone(), - }, - keypair.ref_to(), - Epoch(0), - GAS_LIMIT_MULTIPLIER.into(), - None, - )))); - wrapper.header.chain_id = wrong_chain_id.clone(); - wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned())); - wrapper - .set_data(Data::new("new transaction data".as_bytes().to_owned())); - let mut decrypted = wrapper.clone(); - - decrypted.update_header(TxType::Decrypted(DecryptedTx::Decrypted)); - decrypted.add_section(Section::Signature(Signature::new( - decrypted.sechashes(), - [(0, keypair)].into_iter().collect(), - None, - ))); - let gas_limit = Gas::from(wrapper.header.wrapper().unwrap().gas_limit) - .checked_sub(Gas::from(wrapper.to_bytes().len() as u64)) - .unwrap(); - let wrapper_in_queue = TxInQueue { - tx: wrapper, - gas: gas_limit, - }; - shell.wl_storage.storage.tx_queue.push(wrapper_in_queue); - - // Run validation - let request = ProcessProposal { - txs: vec![decrypted.to_bytes()], - }; - - match shell.process_proposal(request) { - Ok(response) => { - assert_eq!( - response[0].result.code, - u32::from(ErrorCodes::InvalidDecryptedChainId) - ); - assert_eq!( - response[0].result.info, - format!( - "Decrypted tx carries a wrong chain id: expected {}, \ - found {}", - shell.chain_id, wrong_chain_id - ) - ) - } - Err(_) => panic!("Test failed"), - } - } - /// Test that an expired wrapper transaction causes a block rejection #[test] fn test_expired_wrapper() { @@ -2524,62 +2429,6 @@ mod test_process_proposal { } } - /// Test that an expired decrypted transaction is correctly marked as so - /// without rejecting the entire block - #[test] - fn test_expired_decrypted() { - let (mut shell, _recv, _, _) = test_utils::setup(); - let keypair = crate::wallet::defaults::daewon_keypair(); - - let mut wrapper = - Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( - Fee { - amount_per_gas_unit: token::Amount::zero(), - token: shell.wl_storage.storage.native_token.clone(), - }, - keypair.ref_to(), - Epoch(0), - GAS_LIMIT_MULTIPLIER.into(), - None, - )))); - wrapper.header.chain_id = shell.chain_id.clone(); - wrapper.header.expiration = Some(DateTimeUtc::default()); - wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned())); - wrapper - .set_data(Data::new("new transaction data".as_bytes().to_owned())); - let mut decrypted = wrapper.clone(); - - decrypted.update_header(TxType::Decrypted(DecryptedTx::Decrypted)); - decrypted.add_section(Section::Signature(Signature::new( - decrypted.sechashes(), - [(0, keypair)].into_iter().collect(), - None, - ))); - let gas_limit = Gas::from(wrapper.header.wrapper().unwrap().gas_limit) - .checked_sub(Gas::from(wrapper.to_bytes().len() as u64)) - .unwrap(); - let wrapper_in_queue = TxInQueue { - tx: wrapper, - gas: gas_limit, - }; - shell.wl_storage.storage.tx_queue.push(wrapper_in_queue); - - // Run validation - let request = ProcessProposal { - txs: vec![decrypted.to_bytes()], - }; - match shell.process_proposal(request) { - Ok(response) => { - assert_eq!(response.len(), 1); - assert_eq!( - response[0].result.code, - u32::from(ErrorCodes::ExpiredDecryptedTx) - ); - } - Err(_) => panic!("Test failed"), - } - } - /// Check that a tx requiring more gas than the block limit causes a block /// rejection #[test] diff --git a/tests/src/e2e/ledger_tests.rs b/tests/src/e2e/ledger_tests.rs index 2f5bbe4ea7..92edce853a 100644 --- a/tests/src/e2e/ledger_tests.rs +++ b/tests/src/e2e/ledger_tests.rs @@ -986,7 +986,7 @@ fn invalid_transactions() -> Result<()> { client.exp_string("Transaction accepted")?; client.exp_string("Transaction applied")?; client.exp_string("Transaction is invalid")?; - client.exp_string(r#""code": "5"#)?; + client.exp_string(r#""code": "2"#)?; client.assert_success(); let mut ledger = bg_ledger.foreground(); @@ -1042,7 +1042,7 @@ fn invalid_transactions() -> Result<()> { client.exp_string("Error trying to apply a transaction")?; - client.exp_string(r#""code": "4"#)?; + client.exp_string(r#""code": "1"#)?; client.assert_success(); Ok(()) From 39c9b43af75bd1433843d60c208ddac057d55ebc Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 29 Aug 2023 12:04:13 +0200 Subject: [PATCH 4/8] Adds raw header signature in tests --- core/src/types/transaction/mod.rs | 2 +- shared/src/ledger/native_vp/ibc/mod.rs | 151 ++++++++++++---------- tests/src/vm_host_env/mod.rs | 2 + wasm/wasm_source/src/vp_implicit.rs | 58 ++++++--- wasm/wasm_source/src/vp_testnet_faucet.rs | 18 ++- wasm/wasm_source/src/vp_user.rs | 100 ++++++++------ wasm/wasm_source/src/vp_validator.rs | 100 ++++++++------ 7 files changed, 261 insertions(+), 170 deletions(-) diff --git a/core/src/types/transaction/mod.rs b/core/src/types/transaction/mod.rs index 8acb9e6c7e..0102a228eb 100644 --- a/core/src/types/transaction/mod.rs +++ b/core/src/types/transaction/mod.rs @@ -234,7 +234,7 @@ mod test_process_tx { .set_data(Data::new("transaction data".as_bytes().to_owned())) .clone(); tx.add_section(Section::Signature(Signature::new( - vec![*tx.code_sechash(), *tx.data_sechash()], + vec![tx.raw_header_hash(), *tx.code_sechash(), *tx.data_sechash()], [(0, gen_keypair())].into_iter().collect(), None, ))); diff --git a/shared/src/ledger/native_vp/ibc/mod.rs b/shared/src/ledger/native_vp/ibc/mod.rs index 3b6521905b..94fca6063e 100644 --- a/shared/src/ledger/native_vp/ibc/mod.rs +++ b/shared/src/ledger/native_vp/ibc/mod.rs @@ -244,8 +244,8 @@ pub fn get_dummy_header() -> crate::types::storage::Header { /// A dummy validator used for testing #[cfg(any(test, feature = "testing"))] -pub fn get_dummy_genesis_validator() --> namada_proof_of_stake::types::GenesisValidator { +pub fn get_dummy_genesis_validator( +) -> namada_proof_of_stake::types::GenesisValidator { use crate::core::types::address::testing::established_address_1; use crate::core::types::dec::Dec; use crate::core::types::key::testing::common_sk_from_simple_seed; @@ -724,7 +724,11 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![*outer_tx.code_sechash(), *outer_tx.data_sechash()], + vec![ + outer_tx.header_hash(), + *outer_tx.code_sechash(), + *outer_tx.data_sechash(), + ], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -742,10 +746,9 @@ mod tests { let ibc = Ibc { ctx }; // this should return true because state has been stored - assert!( - ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -950,10 +953,9 @@ mod tests { ); let ibc = Ibc { ctx }; // this should return true because state has been stored - assert!( - ibc.validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -1037,7 +1039,11 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![*outer_tx.code_sechash(), *outer_tx.data_sechash()], + vec![ + outer_tx.header_hash(), + *outer_tx.code_sechash(), + *outer_tx.data_sechash(), + ], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1061,10 +1067,9 @@ mod tests { ); let ibc = Ibc { ctx }; // this should return true because state has been stored - assert!( - ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -1284,10 +1289,9 @@ mod tests { ); let ibc = Ibc { ctx }; // this should return true because state has been stored - assert!( - ibc.validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -1371,7 +1375,11 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![*outer_tx.code_sechash(), *outer_tx.data_sechash()], + vec![ + outer_tx.header_hash(), + *outer_tx.code_sechash(), + *outer_tx.data_sechash(), + ], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1394,10 +1402,9 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!( - ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -1459,7 +1466,11 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![*outer_tx.code_sechash(), *outer_tx.data_sechash()], + vec![ + outer_tx.header_hash(), + *outer_tx.code_sechash(), + *outer_tx.data_sechash(), + ], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1482,10 +1493,9 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!( - ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -1584,7 +1594,11 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![*outer_tx.code_sechash(), *outer_tx.data_sechash()], + vec![ + outer_tx.header_hash(), + *outer_tx.code_sechash(), + *outer_tx.data_sechash(), + ], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1607,10 +1621,9 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!( - ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -1708,7 +1721,11 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![*outer_tx.code_sechash(), *outer_tx.data_sechash()], + vec![ + outer_tx.header_hash(), + *outer_tx.code_sechash(), + *outer_tx.data_sechash(), + ], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1731,10 +1748,9 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!( - ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -1817,7 +1833,11 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![*outer_tx.code_sechash(), *outer_tx.data_sechash()], + vec![ + outer_tx.header_hash(), + *outer_tx.code_sechash(), + *outer_tx.data_sechash(), + ], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1840,10 +1860,9 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!( - ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -1944,10 +1963,9 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!( - ibc.validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed")); } // skip test_close_init_channel() and test_close_confirm_channel() since it @@ -2086,10 +2104,9 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!( - ibc.validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -2274,10 +2291,9 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!( - ibc.validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -2421,10 +2437,9 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!( - ibc.validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -2572,10 +2587,9 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!( - ibc.validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed")); } #[test] @@ -2724,9 +2738,8 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!( - ibc.validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed") - ); + assert!(ibc + .validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed")); } } diff --git a/tests/src/vm_host_env/mod.rs b/tests/src/vm_host_env/mod.rs index 68ebd76dff..a5ef2d54f1 100644 --- a/tests/src/vm_host_env/mod.rs +++ b/tests/src/vm_host_env/mod.rs @@ -476,6 +476,7 @@ mod tests { signed_tx_data .verify_signatures( &[ + signed_tx_data.header_hash(), *signed_tx_data.data_sechash(), *signed_tx_data.code_sechash(), ], @@ -495,6 +496,7 @@ mod tests { signed_tx_data .verify_signatures( &[ + signed_tx_data.header_hash(), *signed_tx_data.data_sechash(), *signed_tx_data.code_sechash(), ], diff --git a/wasm/wasm_source/src/vp_implicit.rs b/wasm/wasm_source/src/vp_implicit.rs index 215dccf421..3c6ec99c26 100644 --- a/wasm/wasm_source/src/vp_implicit.rs +++ b/wasm/wasm_source/src/vp_implicit.rs @@ -536,7 +536,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -547,10 +547,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a debit transfer without a valid signature is rejected. @@ -672,7 +676,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -684,10 +688,14 @@ mod tests { let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a transfer on with accounts other than self is accepted. @@ -756,8 +764,8 @@ mod tests { /// Generates a keypair, derive an implicit address from it and generate /// a storage key inside its storage. - fn arb_account_storage_subspace_key() - -> impl Strategy { + fn arb_account_storage_subspace_key( + ) -> impl Strategy { // Generate a keypair key::testing::arb_common_keypair().prop_flat_map(|sk| { let pk = sk.ref_to(); @@ -943,10 +951,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - !validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(!validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } #[test] @@ -988,7 +1000,7 @@ mod tests { tx.set_code(Code::new(vec![])); tx.set_data(Data::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -998,9 +1010,13 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } } diff --git a/wasm/wasm_source/src/vp_testnet_faucet.rs b/wasm/wasm_source/src/vp_testnet_faucet.rs index 7298c0b126..091af51372 100644 --- a/wasm/wasm_source/src/vp_testnet_faucet.rs +++ b/wasm/wasm_source/src/vp_testnet_faucet.rs @@ -267,7 +267,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -277,10 +277,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } prop_compose! { @@ -400,7 +404,7 @@ mod tests { tx_data.set_data(Data::new(solution_bytes)); tx_data.set_code(Code::new(vec![])); tx_data.add_section(Section::Signature(Signature::new( - vec![*tx_data.data_sechash(), *tx_data.code_sechash()], + vec![tx_data.raw_header_hash(), *tx_data.data_sechash(), *tx_data.code_sechash()], [(0, target_key)].into_iter().collect(), None, ))); @@ -454,7 +458,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); diff --git a/wasm/wasm_source/src/vp_user.rs b/wasm/wasm_source/src/vp_user.rs index a334576b53..5bb0593563 100644 --- a/wasm/wasm_source/src/vp_user.rs +++ b/wasm/wasm_source/src/vp_user.rs @@ -393,7 +393,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -403,10 +403,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a PoS action that must be authorized is rejected without a @@ -562,7 +566,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -572,10 +576,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a transfer on with accounts other than self is accepted. @@ -724,7 +732,7 @@ mod tests { tx.set_code(Code::new(vec![])); tx.set_data(Data::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -811,7 +819,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -821,10 +829,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a validity predicate update is rejected if not whitelisted @@ -866,7 +878,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -876,10 +888,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - !validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(!validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a validity predicate update is accepted if whitelisted @@ -922,7 +938,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -932,10 +948,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a tx is rejected if not whitelisted @@ -978,7 +998,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -988,10 +1008,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - !validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(!validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } #[test] @@ -1034,7 +1058,7 @@ mod tests { tx.set_code(Code::new(vec![])); tx.set_data(Data::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -1044,9 +1068,13 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } } diff --git a/wasm/wasm_source/src/vp_validator.rs b/wasm/wasm_source/src/vp_validator.rs index f929a8a0d1..29e0c6f3d2 100644 --- a/wasm/wasm_source/src/vp_validator.rs +++ b/wasm/wasm_source/src/vp_validator.rs @@ -400,7 +400,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -410,10 +410,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a PoS action that must be authorized is rejected without a @@ -580,7 +584,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -590,10 +594,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a transfer on with accounts other than self is accepted. @@ -742,7 +750,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -828,7 +836,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -838,10 +846,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a validity predicate update is rejected if not whitelisted @@ -883,7 +895,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -893,10 +905,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - !validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(!validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a validity predicate update is accepted if whitelisted @@ -939,7 +955,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -949,10 +965,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } /// Test that a tx is rejected if not whitelisted @@ -995,7 +1015,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -1005,10 +1025,14 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - !validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(!validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } #[test] @@ -1051,7 +1075,7 @@ mod tests { tx.set_code(Code::new(vec![])); tx.set_data(Data::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -1061,9 +1085,13 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!( - validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) - .unwrap() - ); + assert!(validate_tx( + &CTX, + signed_tx, + vp_owner, + keys_changed, + verifiers + ) + .unwrap()); } } From 3dbd59a5056285d2fb676b16d8605b28c8a5ddf9 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 29 Aug 2023 20:57:55 +0200 Subject: [PATCH 5/8] Client signs the raw transaction header --- core/src/proto/types.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/proto/types.rs b/core/src/proto/types.rs index b5935e504b..5bd16d61cb 100644 --- a/core/src/proto/types.rs +++ b/core/src/proto/types.rs @@ -1494,7 +1494,8 @@ impl Tx { public_keys_index_map: &AccountPublicKeysMap, signer: Option
, ) -> Vec { - let targets = self.inner_section_targets(); + let mut targets = vec![self.header_hash()]; + targets.extend(self.inner_section_targets()); let mut signatures = Vec::new(); let section = Signature::new( targets, From 44633e28af359e1b87863b9fa7fb81e7702ec4b0 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 30 Aug 2023 20:09:51 +0200 Subject: [PATCH 6/8] Removes redundant signature on `Code` and `Data` --- .../lib/node/ledger/shell/finalize_block.rs | 143 +++++++------ apps/src/lib/node/ledger/shell/mod.rs | 5 +- .../lib/node/ledger/shell/prepare_proposal.rs | 2 +- .../lib/node/ledger/shell/process_proposal.rs | 28 +-- benches/lib.rs | 4 +- core/src/proto/types.rs | 33 +-- core/src/types/transaction/mod.rs | 2 +- core/src/types/transaction/protocol.rs | 6 +- shared/src/ledger/native_vp/ibc/mod.rs | 151 ++++++-------- shared/src/vm/host_env.rs | 2 +- tests/src/vm_host_env/mod.rs | 12 +- vp_prelude/src/lib.rs | 10 +- wasm/checksums.json | 40 ++-- wasm/wasm_source/src/vp_implicit.rs | 62 ++---- wasm/wasm_source/src/vp_testnet_faucet.rs | 6 +- wasm/wasm_source/src/vp_user.rs | 196 ++++++++---------- wasm/wasm_source/src/vp_validator.rs | 100 ++++----- 17 files changed, 353 insertions(+), 449 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 14c24addce..fa329ae681 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -2074,9 +2074,11 @@ mod test_finalize_block { // won't receive votes from TM since we receive votes at a 1-block // delay, so votes will be empty here next_block_for_inflation(&mut shell, pkh1.clone(), vec![], None); - assert!(rewards_accumulator_handle() - .is_empty(&shell.wl_storage) - .unwrap()); + assert!( + rewards_accumulator_handle() + .is_empty(&shell.wl_storage) + .unwrap() + ); // FINALIZE BLOCK 2. Tell Namada that val1 is the block proposer. // Include votes that correspond to block 1. Make val2 the next block's @@ -2086,9 +2088,11 @@ mod test_finalize_block { assert!(rewards_prod_2.is_empty(&shell.wl_storage).unwrap()); assert!(rewards_prod_3.is_empty(&shell.wl_storage).unwrap()); assert!(rewards_prod_4.is_empty(&shell.wl_storage).unwrap()); - assert!(!rewards_accumulator_handle() - .is_empty(&shell.wl_storage) - .unwrap()); + assert!( + !rewards_accumulator_handle() + .is_empty(&shell.wl_storage) + .unwrap() + ); // Val1 was the proposer, so its reward should be larger than all // others, which should themselves all be equal let acc_sum = get_rewards_sum(&shell.wl_storage); @@ -2202,9 +2206,11 @@ mod test_finalize_block { None, ); } - assert!(rewards_accumulator_handle() - .is_empty(&shell.wl_storage) - .unwrap()); + assert!( + rewards_accumulator_handle() + .is_empty(&shell.wl_storage) + .unwrap() + ); let rp1 = rewards_prod_1 .get(&shell.wl_storage, &Epoch::default()) .unwrap() @@ -2259,11 +2265,8 @@ mod test_finalize_block { let wrapper_hash_key = replay_protection::get_replay_protection_key( &wrapper_tx.header_hash(), ); - let mut decrypted_tx = wrapper_tx; - - decrypted_tx.update_header(TxType::Raw); let decrypted_hash_key = replay_protection::get_replay_protection_key( - &decrypted_tx.header_hash(), + &wrapper_tx.raw_header_hash(), ); // merkle tree root before finalize_block @@ -2294,22 +2297,26 @@ mod test_finalize_block { assert!(shell.shell.wl_storage.has_key(&wrapper_hash_key).unwrap()); assert!(shell.shell.wl_storage.has_key(&decrypted_hash_key).unwrap()); // Check that non of the hashes is present in the merkle tree - assert!(!shell - .shell - .wl_storage - .storage - .block - .tree - .has_key(&wrapper_hash_key) - .unwrap()); - assert!(!shell - .shell - .wl_storage - .storage - .block - .tree - .has_key(&decrypted_hash_key) - .unwrap()); + assert!( + !shell + .shell + .wl_storage + .storage + .block + .tree + .has_key(&wrapper_hash_key) + .unwrap() + ); + assert!( + !shell + .shell + .wl_storage + .storage + .block + .tree + .has_key(&decrypted_hash_key) + .unwrap() + ); } /// Test that if a decrypted transaction fails because of out-of-gas, its @@ -2380,10 +2387,12 @@ mod test_finalize_block { let code = event.attributes.get("code").expect("Testfailed").as_str(); assert_eq!(code, String::from(ErrorCodes::WasmRuntimeError).as_str()); - assert!(!shell - .wl_storage - .has_key(&inner_hash_key) - .expect("Test failed")) + assert!( + !shell + .wl_storage + .has_key(&inner_hash_key) + .expect("Test failed") + ) } #[test] @@ -2444,14 +2453,18 @@ mod test_finalize_block { let code = event.attributes.get("code").expect("Testfailed").as_str(); assert_eq!(code, String::from(ErrorCodes::InvalidTx).as_str()); - assert!(shell - .wl_storage - .has_key(&wrapper_hash_key) - .expect("Test failed")); - assert!(!shell - .wl_storage - .has_key(&inner_hash_key) - .expect("Test failed")) + assert!( + shell + .wl_storage + .has_key(&wrapper_hash_key) + .expect("Test failed") + ); + assert!( + !shell + .wl_storage + .has_key(&inner_hash_key) + .expect("Test failed") + ) } // Test that if the fee payer doesn't have enough funds for fee payment the @@ -2738,9 +2751,11 @@ mod test_finalize_block { .unwrap(), Some(ValidatorState::Consensus) ); - assert!(enqueued_slashes_handle() - .at(&Epoch::default()) - .is_empty(&shell.wl_storage)?); + assert!( + enqueued_slashes_handle() + .at(&Epoch::default()) + .is_empty(&shell.wl_storage)? + ); assert_eq!( get_num_consensus_validators(&shell.wl_storage, Epoch::default()) .unwrap(), @@ -2759,17 +2774,21 @@ mod test_finalize_block { .unwrap(), Some(ValidatorState::Jailed) ); - assert!(enqueued_slashes_handle() - .at(&epoch) - .is_empty(&shell.wl_storage)?); + assert!( + enqueued_slashes_handle() + .at(&epoch) + .is_empty(&shell.wl_storage)? + ); assert_eq!( get_num_consensus_validators(&shell.wl_storage, epoch).unwrap(), 5_u64 ); } - assert!(!enqueued_slashes_handle() - .at(&processing_epoch) - .is_empty(&shell.wl_storage)?); + assert!( + !enqueued_slashes_handle() + .at(&processing_epoch) + .is_empty(&shell.wl_storage)? + ); // Advance to the processing epoch loop { @@ -2792,9 +2811,11 @@ mod test_finalize_block { // println!("Reached processing epoch"); break; } else { - assert!(enqueued_slashes_handle() - .at(&shell.wl_storage.storage.block.epoch) - .is_empty(&shell.wl_storage)?); + assert!( + enqueued_slashes_handle() + .at(&shell.wl_storage.storage.block.epoch) + .is_empty(&shell.wl_storage)? + ); let stake1 = read_validator_stake( &shell.wl_storage, ¶ms, @@ -3340,13 +3361,15 @@ mod test_finalize_block { ) .unwrap(); assert_eq!(last_slash, Some(Epoch(4))); - assert!(namada_proof_of_stake::is_validator_frozen( - &shell.wl_storage, - &val1.address, - current_epoch, - ¶ms - ) - .unwrap()); + assert!( + namada_proof_of_stake::is_validator_frozen( + &shell.wl_storage, + &val1.address, + current_epoch, + ¶ms + ) + .unwrap() + ); assert!( namada_proof_of_stake::validator_slashes_handle(&val1.address) .is_empty(&shell.wl_storage) diff --git a/apps/src/lib/node/ledger/shell/mod.rs b/apps/src/lib/node/ledger/shell/mod.rs index 5c393ad85c..bfaf513530 100644 --- a/apps/src/lib/node/ledger/shell/mod.rs +++ b/apps/src/lib/node/ledger/shell/mod.rs @@ -922,7 +922,6 @@ where pub fn replay_protection_checks( &self, wrapper: &Tx, - tx_bytes: &[u8], temp_wl_storage: &mut TempWlStorage, ) -> Result<()> { let inner_tx_hash = wrapper.raw_header_hash(); @@ -944,9 +943,7 @@ where .write(&inner_hash_key, vec![]) .expect("Couldn't write inner transaction hash to write log"); - let tx = - Tx::try_from(tx_bytes).expect("Deserialization shouldn't fail"); - let wrapper_hash = tx.header_hash(); + let wrapper_hash = wrapper.header_hash(); let wrapper_hash_key = replay_protection::get_replay_protection_key(&wrapper_hash); if temp_wl_storage diff --git a/apps/src/lib/node/ledger/shell/prepare_proposal.rs b/apps/src/lib/node/ledger/shell/prepare_proposal.rs index 10f9cbc6c1..986bf32bbe 100644 --- a/apps/src/lib/node/ledger/shell/prepare_proposal.rs +++ b/apps/src/lib/node/ledger/shell/prepare_proposal.rs @@ -246,7 +246,7 @@ where tx_gas_meter.add_tx_size_gas(tx_bytes).map_err(|_| ())?; // Check replay protection - self.replay_protection_checks(&tx, tx_bytes, temp_wl_storage) + self.replay_protection_checks(&tx, temp_wl_storage) .map_err(|_| ())?; // Check fees diff --git a/apps/src/lib/node/ledger/shell/process_proposal.rs b/apps/src/lib/node/ledger/shell/process_proposal.rs index e79d3dba42..3b75de9948 100644 --- a/apps/src/lib/node/ledger/shell/process_proposal.rs +++ b/apps/src/lib/node/ledger/shell/process_proposal.rs @@ -842,11 +842,9 @@ where } } else { // Replay protection checks - if let Err(e) = self.replay_protection_checks( - &tx, - tx_bytes, - temp_wl_storage, - ) { + if let Err(e) = + self.replay_protection_checks(&tx, temp_wl_storage) + { return TxResult { code: ErrorCodes::ReplayTx.into(), info: e.to_string(), @@ -1088,11 +1086,13 @@ mod test_process_proposal { shell.chain_id.clone(), ) .to_bytes(); - assert!(shell - .process_proposal(ProcessProposal { - txs: vec![tx.clone(), tx] - }) - .is_err()); + assert!( + shell + .process_proposal(ProcessProposal { + txs: vec![tx.clone(), tx] + }) + .is_err() + ); } #[cfg(feature = "abcipp")] @@ -1249,9 +1249,11 @@ mod test_process_proposal { sig, } .sign(shell.mode.get_protocol_key().expect("Test failed")); - let mut txs = vec![EthereumTxData::BridgePool(vote_ext.into()) - .sign(protocol_key, shell.chain_id.clone()) - .to_bytes()]; + let mut txs = vec![ + EthereumTxData::BridgePool(vote_ext.into()) + .sign(protocol_key, shell.chain_id.clone()) + .to_bytes(), + ]; let event = EthereumEvent::TransfersToNamada { nonce: 0u64.into(), diff --git a/benches/lib.rs b/benches/lib.rs index 47645abdf4..91e65ec6ff 100644 --- a/benches/lib.rs +++ b/benches/lib.rs @@ -452,7 +452,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, ))); @@ -495,7 +495,7 @@ pub fn generate_foreign_key_tx(signer: &SecretKey) -> Tx { .unwrap(), )); tx.add_section(Section::Signature(Signature::new( - tx.sechashes(), + vec![tx.raw_header_hash()], [(0, signer.clone())].into_iter().collect(), None, ))); diff --git a/core/src/proto/types.rs b/core/src/proto/types.rs index 5bd16d61cb..98839ccf98 100644 --- a/core/src/proto/types.rs +++ b/core/src/proto/types.rs @@ -1250,7 +1250,7 @@ impl Tx { Section::Header(self.header.clone()).get_hash() } - /// Gets the hash of the raw transaction's header + /// Gets the hash of the decrypted transaction's header pub fn raw_header_hash(&self) -> crate::types::hash::Hash { let mut raw_header = self.header(); raw_header.tx_type = TxType::Raw; @@ -1280,6 +1280,10 @@ impl Tx { ) -> Option> { if self.header_hash() == *hash { return Some(Cow::Owned(Section::Header(self.header.clone()))); + } else if self.raw_header_hash() == *hash { + let mut header = self.header(); + header.tx_type = TxType::Raw; + return Some(Cow::Owned(Section::Header(header))); } for section in &self.sections { if section.get_hash() == *hash { @@ -1366,20 +1370,6 @@ impl Tx { bytes } - /// Get the inner section hashes - pub fn inner_section_targets(&self) -> Vec { - let mut sections_hashes = self - .sections - .iter() - .filter_map(|section| match section { - Section::Data(_) | Section::Code(_) => Some(section.get_hash()), - _ => None, - }) - .collect::>(); - sections_hashes.sort(); - sections_hashes - } - /// Verify that the section with the given hash has been signed by the given /// public key pub fn verify_signatures( @@ -1494,8 +1484,7 @@ impl Tx { public_keys_index_map: &AccountPublicKeysMap, signer: Option
, ) -> Vec { - let mut targets = vec![self.header_hash()]; - targets.extend(self.inner_section_targets()); + let targets = vec![self.raw_header_hash()]; let mut signatures = Vec::new(); let section = Signature::new( targets, @@ -1773,11 +1762,9 @@ impl Tx { account_public_keys_map: AccountPublicKeysMap, signer: Option
, ) -> &mut Self { - // The inner tx signer signs the Raw version of the Header - let mut hashes = vec![self.raw_header_hash()]; + // The inner tx signer signs the Decrypted version of the Header + let hashes = vec![self.raw_header_hash()]; self.protocol_filter(); - let sections_hashes = self.inner_section_targets(); - hashes.extend(sections_hashes); self.add_section(Section::Signature(Signature::new( hashes, @@ -1794,7 +1781,7 @@ impl Tx { ) -> &mut Self { self.protocol_filter(); let mut pk_section = Signature { - targets: self.inner_section_targets(), + targets: vec![self.raw_header_hash()], signatures: BTreeMap::new(), signer: Signer::PubKeys(vec![]), }; @@ -1805,7 +1792,7 @@ impl Tx { // Add the signature under the given multisig address let section = sections.entry(addr.clone()).or_insert_with(|| Signature { - targets: self.inner_section_targets(), + targets: vec![self.raw_header_hash()], signatures: BTreeMap::new(), signer: Signer::Address(addr.clone()), }); diff --git a/core/src/types/transaction/mod.rs b/core/src/types/transaction/mod.rs index 0102a228eb..419611e01e 100644 --- a/core/src/types/transaction/mod.rs +++ b/core/src/types/transaction/mod.rs @@ -234,7 +234,7 @@ mod test_process_tx { .set_data(Data::new("transaction data".as_bytes().to_owned())) .clone(); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.code_sechash(), *tx.data_sechash()], + vec![tx.raw_header_hash()], [(0, gen_keypair())].into_iter().collect(), None, ))); diff --git a/core/src/types/transaction/protocol.rs b/core/src/types/transaction/protocol.rs index 1a51434b29..7770df1fed 100644 --- a/core/src/types/transaction/protocol.rs +++ b/core/src/types/transaction/protocol.rs @@ -335,11 +335,7 @@ mod protocol_txs { .expect("Serializing request should not fail"), )); outer_tx.add_section(Section::Signature(Signature::new( - vec![ - outer_tx.header_hash(), - *outer_tx.code_sechash(), - *outer_tx.data_sechash(), - ], + vec![outer_tx.header_hash()], [(0, signing_key.clone())].into_iter().collect(), None, ))); diff --git a/shared/src/ledger/native_vp/ibc/mod.rs b/shared/src/ledger/native_vp/ibc/mod.rs index 94fca6063e..91b506fc0a 100644 --- a/shared/src/ledger/native_vp/ibc/mod.rs +++ b/shared/src/ledger/native_vp/ibc/mod.rs @@ -244,8 +244,8 @@ pub fn get_dummy_header() -> crate::types::storage::Header { /// A dummy validator used for testing #[cfg(any(test, feature = "testing"))] -pub fn get_dummy_genesis_validator( -) -> namada_proof_of_stake::types::GenesisValidator { +pub fn get_dummy_genesis_validator() +-> namada_proof_of_stake::types::GenesisValidator { use crate::core::types::address::testing::established_address_1; use crate::core::types::dec::Dec; use crate::core::types::key::testing::common_sk_from_simple_seed; @@ -724,11 +724,7 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![ - outer_tx.header_hash(), - *outer_tx.code_sechash(), - *outer_tx.data_sechash(), - ], + vec![outer_tx.header_hash()], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -746,9 +742,10 @@ mod tests { let ibc = Ibc { ctx }; // this should return true because state has been stored - assert!(ibc - .validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -953,9 +950,10 @@ mod tests { ); let ibc = Ibc { ctx }; // this should return true because state has been stored - assert!(ibc - .validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -1039,11 +1037,7 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![ - outer_tx.header_hash(), - *outer_tx.code_sechash(), - *outer_tx.data_sechash(), - ], + vec![outer_tx.header_hash()], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1067,9 +1061,10 @@ mod tests { ); let ibc = Ibc { ctx }; // this should return true because state has been stored - assert!(ibc - .validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -1289,9 +1284,10 @@ mod tests { ); let ibc = Ibc { ctx }; // this should return true because state has been stored - assert!(ibc - .validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -1375,11 +1371,7 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![ - outer_tx.header_hash(), - *outer_tx.code_sechash(), - *outer_tx.data_sechash(), - ], + vec![outer_tx.header_hash()], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1402,9 +1394,10 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!(ibc - .validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -1466,11 +1459,7 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![ - outer_tx.header_hash(), - *outer_tx.code_sechash(), - *outer_tx.data_sechash(), - ], + vec![outer_tx.header_hash()], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1493,9 +1482,10 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!(ibc - .validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -1594,11 +1584,7 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![ - outer_tx.header_hash(), - *outer_tx.code_sechash(), - *outer_tx.data_sechash(), - ], + vec![outer_tx.header_hash()], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1621,9 +1607,10 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!(ibc - .validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -1721,11 +1708,7 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![ - outer_tx.header_hash(), - *outer_tx.code_sechash(), - *outer_tx.data_sechash(), - ], + vec![outer_tx.header_hash()], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1748,9 +1731,10 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!(ibc - .validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -1833,11 +1817,7 @@ mod tests { outer_tx.set_code(Code::new(tx_code)); outer_tx.set_data(Data::new(tx_data)); outer_tx.add_section(Section::Signature(Signature::new( - vec![ - outer_tx.header_hash(), - *outer_tx.code_sechash(), - *outer_tx.data_sechash(), - ], + vec![outer_tx.header_hash()], [(0, keypair_1())].into_iter().collect(), None, ))); @@ -1860,9 +1840,10 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!(ibc - .validate_tx(&outer_tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&outer_tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -1963,9 +1944,10 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!(ibc - .validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } // skip test_close_init_channel() and test_close_confirm_channel() since it @@ -2104,9 +2086,10 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!(ibc - .validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -2291,9 +2274,10 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!(ibc - .validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -2437,9 +2421,10 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!(ibc - .validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -2587,9 +2572,10 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!(ibc - .validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } #[test] @@ -2738,8 +2724,9 @@ mod tests { vp_wasm_cache, ); let ibc = Ibc { ctx }; - assert!(ibc - .validate_tx(&tx, &keys_changed, &verifiers) - .expect("validation failed")); + assert!( + ibc.validate_tx(&tx, &keys_changed, &verifiers) + .expect("validation failed") + ); } } diff --git a/shared/src/vm/host_env.rs b/shared/src/vm/host_env.rs index 3e3b78e31b..338bc74a2f 100644 --- a/shared/src/vm/host_env.rs +++ b/shared/src/vm/host_env.rs @@ -1809,7 +1809,7 @@ where let gas_meter = unsafe { env.ctx.gas_meter.get() }; vp_host_fns::add_gas(gas_meter, gas)?; - let hashes = <[Hash; 3]>::try_from_slice(&hash_list) + let hashes = <[Hash; 1]>::try_from_slice(&hash_list) .map_err(vp_host_fns::RuntimeError::EncodingError)?; let (public_keys_map, gas) = env diff --git a/tests/src/vm_host_env/mod.rs b/tests/src/vm_host_env/mod.rs index a5ef2d54f1..3a2ee2e786 100644 --- a/tests/src/vm_host_env/mod.rs +++ b/tests/src/vm_host_env/mod.rs @@ -475,11 +475,7 @@ mod tests { assert!( signed_tx_data .verify_signatures( - &[ - signed_tx_data.header_hash(), - *signed_tx_data.data_sechash(), - *signed_tx_data.code_sechash(), - ], + &[signed_tx_data.header_hash(),], pks_map, &None, 1, @@ -495,11 +491,7 @@ mod tests { assert!( signed_tx_data .verify_signatures( - &[ - signed_tx_data.header_hash(), - *signed_tx_data.data_sechash(), - *signed_tx_data.code_sechash(), - ], + &[signed_tx_data.header_hash(),], AccountPublicKeysMap::from_iter([ other_keypair.ref_to() ]), diff --git a/vp_prelude/src/lib.rs b/vp_prelude/src/lib.rs index c42a29864a..220dab1daf 100644 --- a/vp_prelude/src/lib.rs +++ b/vp_prelude/src/lib.rs @@ -30,7 +30,6 @@ use namada_core::types::internal::HostEnvResult; use namada_core::types::storage::{ BlockHash, BlockHeight, Epoch, Header, TxIndex, BLOCK_HASH_LENGTH, }; -use namada_core::types::transaction::TxType; pub use namada_core::types::*; pub use namada_macros::validity_predicate; pub use namada_proof_of_stake::storage as proof_of_stake; @@ -89,17 +88,10 @@ pub fn verify_signatures(ctx: &Ctx, tx: &Tx, owner: &Address) -> VpResult { let threshold = storage_api::account::threshold(&ctx.pre(), owner)?.unwrap_or(1); - // FIXME: add a test to check the invalid signature in vp of the tx header - // hash FIXME: tryo a replay attack on a local devnet - let mut header = tx.header(); - header.tx_type = TxType::Raw; - let targets = - [tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()]; - // Serialize parameters let max_signatures = max_signatures_per_transaction.try_to_vec().unwrap(); let public_keys_map = public_keys_index_map.try_to_vec().unwrap(); - let targets = targets.try_to_vec().unwrap(); + let targets = [tx.raw_header_hash()].try_to_vec().unwrap(); let signer = owner.try_to_vec().unwrap(); let valid = unsafe { diff --git a/wasm/checksums.json b/wasm/checksums.json index 614ab78e6b..de46655847 100644 --- a/wasm/checksums.json +++ b/wasm/checksums.json @@ -1,22 +1,22 @@ { - "tx_bond.wasm": "tx_bond.b322054eef9d45e299384b2a363049ce0b0160a0c4781ca357aa59970904726c.wasm", - "tx_bridge_pool.wasm": "tx_bridge_pool.6f6ad3b95e21072af9e854e374fa0d7f691f0743da8cf52a643ed1bdb0e16611.wasm", - "tx_change_validator_commission.wasm": "tx_change_validator_commission.9310e0a0b7c14fc7c2427040da8c91eb4067babfaaea9e3b646edbfdd09c8069.wasm", - "tx_ibc.wasm": "tx_ibc.54313469bcc9bcaabf661177f88cb90ac9008f542edbf686f286a02f8cdbfd41.wasm", - "tx_init_account.wasm": "tx_init_account.10ee01dac5325685360119ba8e4b597d776a018ea4c9ac3534dd876ec377789e.wasm", - "tx_init_proposal.wasm": "tx_init_proposal.04cad5a3a71f833a5867bca3ced54b06d34ad07f3f21877599d38581d362ba10.wasm", - "tx_init_validator.wasm": "tx_init_validator.16d53a09e5df06400849aaa161c35e4e377284692f73a71dcbd4573656da7f64.wasm", - "tx_resign_steward.wasm": "tx_resign_steward.b5d92c1bd196be0d196ef16e2ceed9a9ced7ac61d7b177fdbad208c0e784e172.wasm", - "tx_reveal_pk.wasm": "tx_reveal_pk.32011ddc5316705ae005059d5916b071288a04fb4dee80854af16d61548b5c27.wasm", - "tx_transfer.wasm": "tx_transfer.963ec4c2705377423ddc46b4ff3de63f9b625351467d89290fa771a485710c41.wasm", - "tx_unbond.wasm": "tx_unbond.7f26336db8e8cfebc04d301dc4790138fdd9bc22878fe7542c3da525a09576be.wasm", - "tx_unjail_validator.wasm": "tx_unjail_validator.15a7a399d8fb79f8df959d0ddf4c193020886d1caab1e094cca10ea3aff44a72.wasm", - "tx_update_account.wasm": "tx_update_account.7b4e225a823449d3d8bffde197c439ad24f4f6c95cf754acf62b6373958c4486.wasm", - "tx_update_steward_commission.wasm": "tx_update_steward_commission.0001b21ef3ef4f9b33afb5a5ef75a6a5427fbe221a8350cfbd81781ac18ded6e.wasm", - "tx_vote_proposal.wasm": "tx_vote_proposal.727e36112fcd0753f758370dff981cc93430fe7d6f95ceb570a02a37529a7531.wasm", - "tx_withdraw.wasm": "tx_withdraw.e70485a8b79c5bff17d3b6ea96a7546cb709137c8a64606bdd1e77637157de33.wasm", - "vp_implicit.wasm": "vp_implicit.e0958c2ec06863f7bd48cd9abb67cc7557f956ce9fa6c714deba885db721fa50.wasm", - "vp_masp.wasm": "vp_masp.037671b60b3e9f312c1c5fdc53d040ebfad21a646b9b1e2dac6b3e20fc0d01ec.wasm", - "vp_user.wasm": "vp_user.0203fddde57bc31ef411370b628963486928a7c4d34614980d1a52616e0f617b.wasm", - "vp_validator.wasm": "vp_validator.39c685bc1407ef484f963aff9f7576273d56bbf283dcbded9f01944cf7ff9bf0.wasm" + "tx_bond.wasm": "tx_bond.c9fde5719da6dd63a79e0da4a099717401fbb8b7a618b3cb0778015a0773ef23.wasm", + "tx_bridge_pool.wasm": "tx_bridge_pool.e07070f6127f18e47fd4e43ef60f78344c0724f400ed69e8f30af31ad1bfd3cb.wasm", + "tx_change_validator_commission.wasm": "tx_change_validator_commission.34f0e93ee7e55410e75345c8077299f0cc4aa00aa807b09e0ff380f2709a51c8.wasm", + "tx_ibc.wasm": "tx_ibc.ab49f6c6164e4016b9662405ad8da2ec45d876c1e2d0b756a33a75a9e2f45d38.wasm", + "tx_init_account.wasm": "tx_init_account.926d20201221e325c687a39fe9d338f01fb770f6397efd3882b99493089c2ce4.wasm", + "tx_init_proposal.wasm": "tx_init_proposal.6a95f3f1f6ceeeb57335e122dc2ce6a99cafdaef095f00f3439da63881d73e1a.wasm", + "tx_init_validator.wasm": "tx_init_validator.445646b22db97882b3bbdc82a1b276b2bc8f0e18e04c0ba46c096fd5c729d593.wasm", + "tx_resign_steward.wasm": "tx_resign_steward.7f6810fd9901093b044d1f759a3fec6faef26fac1501d1a0c22f7c36e5b90fb4.wasm", + "tx_reveal_pk.wasm": "tx_reveal_pk.cf7811df8c17d38faee925fa77996e8e58abf89b3d4e196972be8b99007b682a.wasm", + "tx_transfer.wasm": "tx_transfer.041e11a019e88466328a2508c1754ea49c16fe8d009ffa15fa9e4a8190e8e0d5.wasm", + "tx_unbond.wasm": "tx_unbond.0778900cdb631687121ee4fbf1cc95cead67b337f5e5e0a92ad6c22c14c3ebd9.wasm", + "tx_unjail_validator.wasm": "tx_unjail_validator.616b6874bbc91a7dc8751a34cd512050695622b5c4b2eeb82ea533b5184089d6.wasm", + "tx_update_account.wasm": "tx_update_account.c39ff535e6b67fa65e902352795a8573aeae77ea6158e93492cecfefa7f3c475.wasm", + "tx_update_steward_commission.wasm": "tx_update_steward_commission.d3ae5fca19609aa2cfa126bbf7926e6ba42b0e78570d8adb38e5f9ec786153e6.wasm", + "tx_vote_proposal.wasm": "tx_vote_proposal.5d6493da13f1a815fe353f9d46b130b2ff779cd59bb7d89b34bb98cb4e271b3a.wasm", + "tx_withdraw.wasm": "tx_withdraw.df045a91abda536abbf7e68fee98cddea6d7faf0a0f0c8f8ecd6ec35b8dcead8.wasm", + "vp_implicit.wasm": "vp_implicit.fd0c536e007782a3b8d3672e9db119725872607a56611e748f185147ac4b3569.wasm", + "vp_masp.wasm": "vp_masp.856241eb315b01531ec3143eec72720b9608616a6f7bb4109c1f818f42c140dd.wasm", + "vp_user.wasm": "vp_user.131360a9656267034d87eaa391390be5d7007c1ffc3f88626e4e407b233c1d18.wasm", + "vp_validator.wasm": "vp_validator.952dcbb21bb2d0cd285e1d75b08174e29ab749ae927d78b130fb6b91bcdfe200.wasm" } \ No newline at end of file diff --git a/wasm/wasm_source/src/vp_implicit.rs b/wasm/wasm_source/src/vp_implicit.rs index 3c6ec99c26..93dfb82fef 100644 --- a/wasm/wasm_source/src/vp_implicit.rs +++ b/wasm/wasm_source/src/vp_implicit.rs @@ -536,7 +536,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -547,14 +547,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a debit transfer without a valid signature is rejected. @@ -676,7 +672,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -688,14 +684,10 @@ mod tests { let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a transfer on with accounts other than self is accepted. @@ -764,8 +756,8 @@ mod tests { /// Generates a keypair, derive an implicit address from it and generate /// a storage key inside its storage. - fn arb_account_storage_subspace_key( - ) -> impl Strategy { + fn arb_account_storage_subspace_key() + -> impl Strategy { // Generate a keypair key::testing::arb_common_keypair().prop_flat_map(|sk| { let pk = sk.ref_to(); @@ -848,7 +840,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -941,7 +933,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![*tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -951,14 +943,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(!validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + !validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } #[test] @@ -1000,7 +988,7 @@ mod tests { tx.set_code(Code::new(vec![])); tx.set_data(Data::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -1010,13 +998,9 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } } diff --git a/wasm/wasm_source/src/vp_testnet_faucet.rs b/wasm/wasm_source/src/vp_testnet_faucet.rs index 091af51372..598b7a77ef 100644 --- a/wasm/wasm_source/src/vp_testnet_faucet.rs +++ b/wasm/wasm_source/src/vp_testnet_faucet.rs @@ -267,7 +267,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -404,7 +404,7 @@ mod tests { tx_data.set_data(Data::new(solution_bytes)); tx_data.set_code(Code::new(vec![])); tx_data.add_section(Section::Signature(Signature::new( - vec![tx_data.raw_header_hash(), *tx_data.data_sechash(), *tx_data.code_sechash()], + vec![tx_data.raw_header_hash()], [(0, target_key)].into_iter().collect(), None, ))); @@ -458,7 +458,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); diff --git a/wasm/wasm_source/src/vp_user.rs b/wasm/wasm_source/src/vp_user.rs index 5bb0593563..63d87aff10 100644 --- a/wasm/wasm_source/src/vp_user.rs +++ b/wasm/wasm_source/src/vp_user.rs @@ -393,7 +393,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -403,14 +403,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a PoS action that must be authorized is rejected without a @@ -566,7 +562,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -576,14 +572,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a transfer on with accounts other than self is accepted. @@ -695,56 +687,56 @@ mod tests { } proptest! { - /// Test that a signed tx that performs arbitrary storage writes or - /// deletes to the account is accepted. - #[test] - fn test_signed_arb_storage_write( - (vp_owner, storage_key) in arb_account_storage_subspace_key(), - // Generate bytes to write. If `None`, delete from the key instead - storage_value in any::>>(), - ) { - // Initialize a tx environment - let mut tx_env = TestTxEnv::default(); - - let keypair = key::testing::keypair_1(); - let public_key = keypair.ref_to(); - - // Spawn all the accounts in the storage key to be able to modify - // their storage - let storage_key_addresses = storage_key.find_addresses(); - tx_env.spawn_accounts(storage_key_addresses); - tx_env.init_account_storage(&vp_owner, vec![public_key.clone()], 1); - - // Initialize VP environment from a transaction - vp_host_env::init_from_tx(vp_owner.clone(), tx_env, |_address| { - // Write or delete some data in the transaction - if let Some(value) = &storage_value { - tx::ctx().write(&storage_key, value).unwrap(); - } else { - tx::ctx().delete(&storage_key).unwrap(); - } - }); - - let pks_map = AccountPublicKeysMap::from_iter(vec![public_key]); - - let mut vp_env = vp_host_env::take(); - let mut tx = vp_env.tx.clone(); - tx.set_code(Code::new(vec![])); - tx.set_data(Data::new(vec![])); - tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], - pks_map.index_secret_keys(vec![keypair]), - None, - ))); - let signed_tx = tx.clone(); - vp_env.tx = signed_tx.clone(); - let keys_changed: BTreeSet = - vp_env.all_touched_storage_keys(); - let verifiers: BTreeSet
= BTreeSet::default(); - vp_host_env::set(vp_env); - assert!(validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers).unwrap()); + /// Test that a signed tx that performs arbitrary storage writes or + /// deletes to the account is accepted. + #[test] + fn test_signed_arb_storage_write( + (vp_owner, storage_key) in arb_account_storage_subspace_key(), + // Generate bytes to write. If `None`, delete from the key instead + storage_value in any::>>(), + ) { + // Initialize a tx environment + let mut tx_env = TestTxEnv::default(); + + let keypair = key::testing::keypair_1(); + let public_key = keypair.ref_to(); + + // Spawn all the accounts in the storage key to be able to modify + // their storage + let storage_key_addresses = storage_key.find_addresses(); + tx_env.spawn_accounts(storage_key_addresses); + tx_env.init_account_storage(&vp_owner, vec![public_key.clone()], 1); + + // Initialize VP environment from a transaction + vp_host_env::init_from_tx(vp_owner.clone(), tx_env, |_address| { + // Write or delete some data in the transaction + if let Some(value) = &storage_value { + tx::ctx().write(&storage_key, value).unwrap(); + } else { + tx::ctx().delete(&storage_key).unwrap(); + } + }); + + let pks_map = AccountPublicKeysMap::from_iter(vec![public_key]); + + let mut vp_env = vp_host_env::take(); + let mut tx = vp_env.tx.clone(); + tx.set_code(Code::new(vec![])); + tx.set_data(Data::new(vec![])); + tx.add_section(Section::Signature(Signature::new( + vec![ tx.raw_header_hash()], + pks_map.index_secret_keys(vec![keypair]), + None, + ))); + let signed_tx = tx.clone(); + vp_env.tx = signed_tx.clone(); + let keys_changed: BTreeSet = + vp_env.all_touched_storage_keys(); + let verifiers: BTreeSet
= BTreeSet::default(); + vp_host_env::set(vp_env); + assert!(validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers).unwrap()); + } } - } /// Test that a validity predicate update without a valid signature is /// rejected. @@ -819,7 +811,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -829,14 +821,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a validity predicate update is rejected if not whitelisted @@ -878,7 +866,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -888,14 +876,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(!validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + !validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a validity predicate update is accepted if whitelisted @@ -938,7 +922,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -948,14 +932,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a tx is rejected if not whitelisted @@ -998,7 +978,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -1008,14 +988,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(!validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + !validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } #[test] @@ -1058,7 +1034,7 @@ mod tests { tx.set_code(Code::new(vec![])); tx.set_data(Data::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -1068,13 +1044,9 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } } diff --git a/wasm/wasm_source/src/vp_validator.rs b/wasm/wasm_source/src/vp_validator.rs index 29e0c6f3d2..1c306099c1 100644 --- a/wasm/wasm_source/src/vp_validator.rs +++ b/wasm/wasm_source/src/vp_validator.rs @@ -400,7 +400,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -410,14 +410,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a PoS action that must be authorized is rejected without a @@ -584,7 +580,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![secret_key]), None, ))); @@ -594,14 +590,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a transfer on with accounts other than self is accepted. @@ -750,7 +742,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -836,7 +828,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -846,14 +838,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a validity predicate update is rejected if not whitelisted @@ -895,7 +883,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -905,14 +893,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(!validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + !validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a validity predicate update is accepted if whitelisted @@ -955,7 +939,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -965,14 +949,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } /// Test that a tx is rejected if not whitelisted @@ -1015,7 +995,7 @@ mod tests { tx.set_data(Data::new(vec![])); tx.set_code(Code::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -1025,14 +1005,10 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(!validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + !validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } #[test] @@ -1075,7 +1051,7 @@ mod tests { tx.set_code(Code::new(vec![])); tx.set_data(Data::new(vec![])); tx.add_section(Section::Signature(Signature::new( - vec![tx.raw_header_hash(), *tx.data_sechash(), *tx.code_sechash()], + vec![tx.raw_header_hash()], pks_map.index_secret_keys(vec![keypair]), None, ))); @@ -1085,13 +1061,9 @@ mod tests { vp_env.all_touched_storage_keys(); let verifiers: BTreeSet
= BTreeSet::default(); vp_host_env::set(vp_env); - assert!(validate_tx( - &CTX, - signed_tx, - vp_owner, - keys_changed, - verifiers - ) - .unwrap()); + assert!( + validate_tx(&CTX, signed_tx, vp_owner, keys_changed, verifiers) + .unwrap() + ); } } From ca14bfc4b90b32718fb279646a0bcc8cd5c0585a Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 23 Oct 2023 12:02:11 +0200 Subject: [PATCH 7/8] Fixes raw header hash in compressed signature --- core/src/proto/types.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/src/proto/types.rs b/core/src/proto/types.rs index 98839ccf98..75bf5ad5bf 100644 --- a/core/src/proto/types.rs +++ b/core/src/proto/types.rs @@ -628,6 +628,9 @@ impl CompressedSignature { if idx == 0 { // The "zeroth" section is the header targets.push(tx.header_hash()); + } else if idx == 255 { + // The 255th section is the raw header + targets.push(tx.raw_header_hash()); } else { targets.push(tx.sections[idx as usize - 1].get_hash()); } From c40cbc1e2e66c6319179626e7aa5aeb0f2ef3204 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 12 Oct 2023 17:48:04 +0200 Subject: [PATCH 8/8] Changelog #1867 --- .../unreleased/improvements/1867-fix-replay-protection.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/1867-fix-replay-protection.md diff --git a/.changelog/unreleased/improvements/1867-fix-replay-protection.md b/.changelog/unreleased/improvements/1867-fix-replay-protection.md new file mode 100644 index 0000000000..ad22c70c55 --- /dev/null +++ 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)) \ No newline at end of file