From a1f6e2ab18ac761de5f7336feb1affb4c231daad Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 29 Oct 2024 10:18:19 +0100 Subject: [PATCH 1/8] Vp sig gadget also verifies the memo sections --- crates/vp_prelude/src/lib.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/crates/vp_prelude/src/lib.rs b/crates/vp_prelude/src/lib.rs index cc698fdbcd..f30e3a02b7 100644 --- a/crates/vp_prelude/src/lib.rs +++ b/crates/vp_prelude/src/lib.rs @@ -134,6 +134,20 @@ impl VerifySigGadget { owner: &Address, ) -> VpResult { if !self.has_validated_sig { + //FIXME: change the api and do the check for this inner tx only + // First check that none of the memo sections of this batch have been tampered with + for cmt in &tx_data.header.batch { + if cmt.memo_hash != namada_core::hash::Hash::zero() { + tx_data.get_section(&cmt.memo_hash).ok_or_else(|| { + VpError::Erased(format!( + "Memo section with hash {} is missing", + cmt.memo_hash + )) + })?; + } + } + + // Then check the signature verify_signatures(ctx, tx_data, owner)?; self.has_validated_sig = true; } From 2d1de9cb80bf1ee061557aa7cd205de3b1704c2f Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 29 Oct 2024 11:10:07 +0100 Subject: [PATCH 2/8] Vps verify the integrity of the memo for just the current inner tx --- crates/vp_prelude/src/lib.rs | 26 ++--- wasm/vp_implicit/src/lib.rs | 187 +++++++++++++++++++++++++++++++++- wasm/vp_user/src/lib.rs | 188 ++++++++++++++++++++++++++++++++++- 3 files changed, 384 insertions(+), 17 deletions(-) diff --git a/crates/vp_prelude/src/lib.rs b/crates/vp_prelude/src/lib.rs index f30e3a02b7..7f6057c06c 100644 --- a/crates/vp_prelude/src/lib.rs +++ b/crates/vp_prelude/src/lib.rs @@ -52,7 +52,7 @@ use namada_vm_env::vp::*; use namada_vm_env::{read_from_buffer, read_key_val_bytes_from_buffer}; pub use namada_vp_env::{collection_validation, VpEnv}; pub use sha2::{Digest, Sha256, Sha384, Sha512}; -use tx::BatchedTxRef; +use tx::{BatchedTxRef, TxCommitments}; pub use { namada_account as account, namada_parameters as parameters, namada_proof_of_stake as proof_of_stake, namada_token as token, @@ -131,20 +131,19 @@ impl VerifySigGadget { &mut self, ctx: &Ctx, tx_data: &Tx, + cmt: &TxCommitments, owner: &Address, ) -> VpResult { if !self.has_validated_sig { - //FIXME: change the api and do the check for this inner tx only - // First check that none of the memo sections of this batch have been tampered with - for cmt in &tx_data.header.batch { - if cmt.memo_hash != namada_core::hash::Hash::zero() { - tx_data.get_section(&cmt.memo_hash).ok_or_else(|| { - VpError::Erased(format!( - "Memo section with hash {} is missing", - cmt.memo_hash - )) - })?; - } + // First check that the memo section of this inner tx has not been + // tampered with + if cmt.memo_hash != namada_core::hash::Hash::zero() { + tx_data.get_section(&cmt.memo_hash).ok_or_else(|| { + VpError::Erased(format!( + "Memo section with hash {} is missing", + cmt.memo_hash + )) + })?; } // Then check the signature @@ -163,10 +162,11 @@ impl VerifySigGadget { predicate: F, ctx: &Ctx, tx_data: &Tx, + cmt: &TxCommitments, owner: &Address, ) -> VpResult { if predicate() { - self.verify_signatures(ctx, tx_data, owner)?; + self.verify_signatures(ctx, tx_data, cmt, owner)?; } Ok(()) } diff --git a/wasm/vp_implicit/src/lib.rs b/wasm/vp_implicit/src/lib.rs index cffc14fe06..e15f9cb02f 100644 --- a/wasm/vp_implicit/src/lib.rs +++ b/wasm/vp_implicit/src/lib.rs @@ -69,6 +69,7 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )?, PosAction::Bond(Bond { @@ -85,6 +86,7 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )? } @@ -100,10 +102,17 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )?, Action::Masp(MaspAction::MaspAuthorizer(source)) => gadget - .verify_signatures_when(|| source == addr, ctx, &tx, &addr)?, + .verify_signatures_when( + || source == addr, + ctx, + &tx, + cmt, + &addr, + )?, Action::Masp(MaspAction::MaspSectionRef(_)) => (), Action::IbcShielding => (), } @@ -164,6 +173,7 @@ fn validate_tx( || change.is_negative(), ctx, &tx, + cmt, &addr, )?; let sign = if change.non_negative() { "" } else { "-" }; @@ -193,12 +203,13 @@ fn validate_tx( || minter_addr == &addr, ctx, &tx, + cmt, &addr, ), KeyType::Masp | KeyType::Ibc => Ok(()), KeyType::Unknown => { // Unknown changes require a valid signature - gadget.verify_signatures(ctx, &tx, &addr) + gadget.verify_signatures(ctx, &tx, cmt, &addr) } }; validate_change().inspect_err(|reason| { @@ -1039,4 +1050,176 @@ mod tests { .contains("InvalidSectionSignature") ); } + + // Test malleability attacks on memo sections + #[test] + fn test_tampered_memo_section() { + // Initialize a tx environment + let mut tx_env = TestTxEnv::default(); + + let secret_key = key::testing::keypair_1(); + let public_key = secret_key.ref_to(); + let vp_owner: Address = (&public_key).into(); + let target = address::testing::established_address_2(); + let token = address::testing::nam(); + let amount = token::Amount::from_uint(10_098_123, 0).unwrap(); + + tx_env.init_parameters(None, None, None); + + // Spawn the accounts to be able to modify their storage + tx_env.spawn_accounts([&vp_owner, &target, &token]); + tx_env.init_account_storage(&vp_owner, vec![public_key.clone()], 1); + + // Credit the tokens to the VP owner before running the transaction to + // be able to transfer from it + tx_env.credit_tokens(&vp_owner, &token, amount); + // write the denomination of NAM into storage + token::write_denom( + &mut tx_env.state, + &token, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ) + .unwrap(); + + let amount = token::DenominatedAmount::new( + amount, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ); + // Initialize VP environment from a transaction + vp_host_env::init_from_tx(vp_owner.clone(), tx_env, |address| { + // Apply transfer in a transaction + tx_host_env::token::transfer( + tx::ctx(), + address, + &target, + &token, + amount.amount(), + ) + .unwrap(); + }); + let mut vp_env = vp_host_env::take(); + let pks_map = AccountPublicKeysMap::from_iter(vec![public_key]); + + let mut tx = vp_env.batched_tx.tx.clone(); + tx.set_data(Data::new(vec![])); + tx.set_code(Code::new(vec![], None)); + let (_, memo_hash) = tx.add_memo(&[1, 2, 3]); + tx.push_default_inner_tx(); + tx.set_data(Data::new(vec![])); + tx.set_code(Code::new(vec![], None)); + tx.add_memo(&[1, 2, 3]); + tx.add_section(Section::Authorization(Authorization::new( + vec![tx.raw_header_hash()], + pks_map.index_secret_keys(vec![secret_key]), + None, + ))); + + // Tamper with the first memo section + let memo_section = tx + .sections + .iter_mut() + .find(|sec| sec.get_hash() == memo_hash) + .unwrap(); + let mut bytes = memo_section.extra_data().unwrap(); + *bytes.last_mut().unwrap() = 4; + *memo_section = Section::ExtraData(Code::new(bytes, None)); + + let signed_tx = tx.clone().batch_first_tx(); + vp_env.batched_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); + + // 1. Tampared memo section of the current inner tx causes tx rejection + // when validating the signature + let err = validate_tx( + &CTX, + signed_tx.clone(), + vp_owner.clone(), + keys_changed, + verifiers, + ) + .unwrap_err(); + if let VpError::Erased(msg) = err { + assert_eq!( + msg, + format!("Memo section with hash {} is missing", memo_hash) + ); + } else { + panic!("Got wrong error message"); + } + + // 2. If signature is not checked, a tampered memo section should not + // cause rejection + assert!( + validate_tx( + &CTX, + signed_tx, + vp_owner.clone(), + Default::default(), + Default::default(), + ) + .is_ok() + ); + + // 3. If the memo section of another inner tx has been tampered with the + // current inner tx does not get rejected + let mut tx_env = TestTxEnv::default(); + let vp_owner = address::testing::established_address_1(); + let keypair = key::testing::keypair_1(); + let public_key = keypair.ref_to(); + let target = address::testing::established_address_2(); + let token = address::testing::nam(); + let amount = token::Amount::from_uint(10_098_123, 0).unwrap(); + + // Spawn the accounts to be able to modify their storage + tx_env.spawn_accounts([&vp_owner, &target, &token]); + tx_env.init_account_storage(&vp_owner, vec![public_key.clone()], 1); + + // Credit the tokens to the VP owner before running the transaction to + // be able to transfer from it + tx_env.credit_tokens(&vp_owner, &token, amount); + // write the denomination of NAM into storage + token::write_denom( + &mut tx_env.state, + &token, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ) + .unwrap(); + + let amount = token::DenominatedAmount::new( + amount, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ); + + // Initialize VP environment from a transaction that requires a + // signature check + vp_host_env::init_from_tx(vp_owner.clone(), tx_env, |address| { + // Apply transfer in a transaction + tx_host_env::token::transfer( + tx::ctx(), + address, + &target, + &token, + amount.amount(), + ) + .unwrap(); + }); + let mut vp_env = vp_host_env::take(); + let second_inner_tx = tx.header.batch.get_index(1).unwrap().clone(); + let signed_tx = tx.batch_tx(second_inner_tx); + vp_env.batched_tx = signed_tx.clone(); + vp_host_env::set(vp_env); + assert!( + validate_tx( + &CTX, + signed_tx, + vp_owner, + Default::default(), + Default::default(), + ) + .is_ok() + ); + } } diff --git a/wasm/vp_user/src/lib.rs b/wasm/vp_user/src/lib.rs index 78544afca6..12d0d603bb 100644 --- a/wasm/vp_user/src/lib.rs +++ b/wasm/vp_user/src/lib.rs @@ -68,6 +68,7 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )?, PosAction::Bond(Bond { @@ -84,6 +85,7 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )? } @@ -99,10 +101,17 @@ fn validate_tx( || source == addr, ctx, &tx, + cmt, &addr, )?, Action::Masp(MaspAction::MaspAuthorizer(source)) => gadget - .verify_signatures_when(|| source == addr, ctx, &tx, &addr)?, + .verify_signatures_when( + || source == addr, + ctx, + &tx, + cmt, + &addr, + )?, Action::Masp(MaspAction::MaspSectionRef(_)) => (), Action::IbcShielding => (), } @@ -124,6 +133,7 @@ fn validate_tx( || change.is_negative(), ctx, &tx, + cmt, &addr, )?; let sign = if change.non_negative() { "" } else { "-" }; @@ -153,6 +163,7 @@ fn validate_tx( || minter_addr == &addr, ctx, &tx, + cmt, &addr, ), KeyType::Vp(owner) => { @@ -162,13 +173,14 @@ fn validate_tx( || owner == &addr && vp_overwritten, ctx, &tx, + cmt, &addr, ) } KeyType::Masp | KeyType::Ibc => Ok(()), KeyType::Unknown => { // Unknown changes require a valid signature - gadget.verify_signatures(ctx, &tx, &addr) + gadget.verify_signatures(ctx, &tx, cmt, &addr) } }; validate_change().inspect_err(|reason| { @@ -1533,4 +1545,176 @@ mod tests { .is_ok() ); } + + // Test malleability attacks on memo sections + #[test] + fn test_tampered_memo_section() { + // Initialize a tx environment + let mut tx_env = TestTxEnv::default(); + + let vp_owner = address::testing::established_address_1(); + let keypair = key::testing::keypair_1(); + let public_key = keypair.ref_to(); + let target = address::testing::established_address_2(); + let token = address::testing::nam(); + let amount = token::Amount::from_uint(10_098_123, 0).unwrap(); + + // Spawn the accounts to be able to modify their storage + tx_env.spawn_accounts([&vp_owner, &target, &token]); + tx_env.init_account_storage(&vp_owner, vec![public_key.clone()], 1); + + // Credit the tokens to the VP owner before running the transaction to + // be able to transfer from it + tx_env.credit_tokens(&vp_owner, &token, amount); + // write the denomination of NAM into storage + token::write_denom( + &mut tx_env.state, + &token, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ) + .unwrap(); + + let amount = token::DenominatedAmount::new( + amount, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ); + + // Initialize VP environment from a transaction that requires a + // signature check + vp_host_env::init_from_tx(vp_owner.clone(), tx_env, |address| { + // Apply transfer in a transaction + tx_host_env::token::transfer( + tx::ctx(), + address, + &target, + &token, + amount.amount(), + ) + .unwrap(); + }); + let mut vp_env = vp_host_env::take(); + let pks_map = AccountPublicKeysMap::from_iter(vec![public_key]); + + let mut tx = vp_env.batched_tx.tx.clone(); + tx.set_data(Data::new(vec![])); + tx.set_code(Code::new(vec![], None)); + let (_, memo_hash) = tx.add_memo(&[1, 2, 3]); + tx.push_default_inner_tx(); + tx.set_data(Data::new(vec![])); + tx.set_code(Code::new(vec![], None)); + tx.add_memo(&[1, 2, 3]); + tx.add_section(Section::Authorization(Authorization::new( + vec![tx.raw_header_hash()], + pks_map.index_secret_keys(vec![keypair]), + None, + ))); + + // Tamper with the first memo section + let memo_section = tx + .sections + .iter_mut() + .find(|sec| sec.get_hash() == memo_hash) + .unwrap(); + let mut bytes = memo_section.extra_data().unwrap(); + *bytes.last_mut().unwrap() = 4; + *memo_section = Section::ExtraData(Code::new(bytes, None)); + + let signed_tx = tx.clone().batch_first_tx(); + vp_env.batched_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); + + // 1. Tampared memo section of the current inner tx causes tx rejection + // when validating the signature + let err = validate_tx( + &CTX, + signed_tx.clone(), + vp_owner.clone(), + keys_changed, + verifiers, + ) + .unwrap_err(); + if let VpError::Erased(msg) = err { + assert_eq!( + msg, + format!("Memo section with hash {} is missing", memo_hash) + ); + } else { + panic!("Got wrong error message"); + } + + // 2. If signature is not checked, a tampered memo section should not + // cause rejection + assert!( + validate_tx( + &CTX, + signed_tx, + vp_owner.clone(), + Default::default(), + Default::default(), + ) + .is_ok() + ); + + // 3. If the memo section of another inner tx has been tampered with the + // current inner tx does not get rejected + let mut tx_env = TestTxEnv::default(); + let vp_owner = address::testing::established_address_1(); + let keypair = key::testing::keypair_1(); + let public_key = keypair.ref_to(); + let target = address::testing::established_address_2(); + let token = address::testing::nam(); + let amount = token::Amount::from_uint(10_098_123, 0).unwrap(); + + // Spawn the accounts to be able to modify their storage + tx_env.spawn_accounts([&vp_owner, &target, &token]); + tx_env.init_account_storage(&vp_owner, vec![public_key.clone()], 1); + + // Credit the tokens to the VP owner before running the transaction to + // be able to transfer from it + tx_env.credit_tokens(&vp_owner, &token, amount); + // write the denomination of NAM into storage + token::write_denom( + &mut tx_env.state, + &token, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ) + .unwrap(); + + let amount = token::DenominatedAmount::new( + amount, + token::NATIVE_MAX_DECIMAL_PLACES.into(), + ); + + // Initialize VP environment from a transaction that requires a + // signature check + vp_host_env::init_from_tx(vp_owner.clone(), tx_env, |address| { + // Apply transfer in a transaction + tx_host_env::token::transfer( + tx::ctx(), + address, + &target, + &token, + amount.amount(), + ) + .unwrap(); + }); + let mut vp_env = vp_host_env::take(); + let second_inner_tx = tx.header.batch.get_index(1).unwrap().clone(); + let signed_tx = tx.batch_tx(second_inner_tx); + vp_env.batched_tx = signed_tx.clone(); + vp_host_env::set(vp_env); + assert!( + validate_tx( + &CTX, + signed_tx, + vp_owner, + Default::default(), + Default::default(), + ) + .is_ok() + ); + } } From 3f4839b6cb0b74b5ebc4cb4d836ff833cf32fbbb Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 29 Oct 2024 13:02:11 +0100 Subject: [PATCH 3/8] Const zero hash function --- crates/core/src/hash.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/core/src/hash.rs b/crates/core/src/hash.rs index bfddb087bd..b5c42777ee 100644 --- a/crates/core/src/hash.rs +++ b/crates/core/src/hash.rs @@ -175,7 +175,7 @@ impl Hash { } /// Return zeros - pub fn zero() -> Self { + pub const fn zero() -> Self { Self([0u8; HASH_LENGTH]) } From 481d95a3f8df54adb94d4b39880292e77a1168a7 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 29 Oct 2024 18:44:26 +0100 Subject: [PATCH 4/8] Fixes broken integration test --- crates/tests/src/integration/masp.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index f88f08ecdd..132fb572f4 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -5166,13 +5166,7 @@ fn identical_output_descriptions() -> Result<()> { let tx: namada_sdk::tx::Tx = serde_json::from_slice(&tx_bytes).unwrap(); // Inject some randomness in the cloned tx to change the hash let mut tx_clone = tx.clone(); - let mut cmt = tx_clone.header.batch.first().unwrap().to_owned(); - let random_hash: Vec<_> = (0..namada_sdk::hash::HASH_LENGTH) - .map(|_| rand::random::()) - .collect(); - cmt.memo_hash = namada_sdk::hash::Hash(random_hash.try_into().unwrap()); - tx_clone.header.batch.clear(); - tx_clone.header.batch.insert(cmt); + tx_clone.add_memo(&[1, 2, 3]); let signing_data = SigningTxData { owner: None, From 94caa75dab602c4337cacb5fd7ef8faae11d6aa9 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 21 Nov 2024 10:51:59 +0100 Subject: [PATCH 5/8] Changelog #3960 --- .changelog/unreleased/bug-fixes/3960-memo-validation-in-vp.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/3960-memo-validation-in-vp.md diff --git a/.changelog/unreleased/bug-fixes/3960-memo-validation-in-vp.md b/.changelog/unreleased/bug-fixes/3960-memo-validation-in-vp.md new file mode 100644 index 0000000000..3c47481c45 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/3960-memo-validation-in-vp.md @@ -0,0 +1,2 @@ +- Added validation of the transaction's memo in the validity predicates to catch + any possible tamperings. ([\#3960](https://github.com/anoma/namada/pull/3960)) \ No newline at end of file From 73898d61025eac7a123edfc9a01017827cf3bbf3 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 21 Nov 2024 12:56:19 +0100 Subject: [PATCH 6/8] Fixes rebase conflicts --- wasm_for_tests/tx_proposal_token_gas/src/lib.rs | 6 +++--- wasm_for_tests/vp_verify_signature/src/lib.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/wasm_for_tests/tx_proposal_token_gas/src/lib.rs b/wasm_for_tests/tx_proposal_token_gas/src/lib.rs index bbf087da34..18a7f2d45a 100644 --- a/wasm_for_tests/tx_proposal_token_gas/src/lib.rs +++ b/wasm_for_tests/tx_proposal_token_gas/src/lib.rs @@ -1,8 +1,8 @@ use std::collections::BTreeMap; -use namada_tx_prelude::{ - parameters_storage::get_gas_cost_key, token::Amount, *, -}; +use namada_tx_prelude::parameters_storage::get_gas_cost_key; +use namada_tx_prelude::token::Amount; +use namada_tx_prelude::*; #[transaction] fn apply_tx(ctx: &mut Ctx, _tx_data: BatchedTx) -> TxResult { diff --git a/wasm_for_tests/vp_verify_signature/src/lib.rs b/wasm_for_tests/vp_verify_signature/src/lib.rs index f31d1e4f05..714494fed8 100644 --- a/wasm_for_tests/vp_verify_signature/src/lib.rs +++ b/wasm_for_tests/vp_verify_signature/src/lib.rs @@ -9,5 +9,5 @@ fn validate_tx( _verifiers: BTreeSet
, ) -> VpResult { let mut gadget = VerifySigGadget::new(); - gadget.verify_signatures(ctx, &tx_data.tx, &addr) + gadget.verify_signatures(ctx, &tx_data.tx, &tx_data.cmt, &addr) } From 6302c87b4cb648e4705e218dbb3e22787775746c Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 21 Nov 2024 15:15:42 +0100 Subject: [PATCH 7/8] Fixes arbitrary memos in proptests --- crates/node/src/protocol.rs | 58 ++++++++++++++++++++----------------- crates/sdk/src/lib.rs | 9 +++++- 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 826f06f625..c2a42ba707 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -1669,45 +1669,51 @@ mod tests { // Test that the strategy produces valid txs first let result = runner.run(&arb_valid_signed_inner_tx(signing_key.clone()), |tx| { + for cmt in tx.commitments() { + let batched_tx = BatchedTxRef { tx: &tx, cmt }; + assert!( + wasm::run::vp( + code_hash, + &batched_tx, + &TxIndex::default(), + &addr, + &state, + &RefCell::new(VpGasMeter::new_from_tx_meter( + &TxGasMeter::new(u64::MAX), + )), + &Default::default(), + &Default::default(), + vp_cache.clone(), + ) + .is_ok() + ); + } + Ok(()) + }); + assert!(result.is_ok()); + + // Then test tampered txs + let mut runner = TestRunner::new(Config::default()); + let result = runner.run(&arb_tampered_inner_tx(signing_key), |tx| { + for cmt in tx.commitments() { + let batched_tx = BatchedTxRef { tx: &tx, cmt }; assert!( wasm::run::vp( code_hash, - &tx.batch_ref_first_tx().unwrap(), + &batched_tx, &TxIndex::default(), &addr, &state, &RefCell::new(VpGasMeter::new_from_tx_meter( - &TxGasMeter::new(u64::MAX), + &TxGasMeter::new(u64::MAX,) )), &Default::default(), &Default::default(), vp_cache.clone(), ) - .is_ok() + .is_err() ); - Ok(()) - }); - assert!(result.is_ok()); - - // Then test tampered txs - let mut runner = TestRunner::new(Config::default()); - let result = runner.run(&arb_tampered_inner_tx(signing_key), |tx| { - assert!( - wasm::run::vp( - code_hash, - &tx.batch_ref_first_tx().unwrap(), - &TxIndex::default(), - &addr, - &state, - &RefCell::new(VpGasMeter::new_from_tx_meter( - &TxGasMeter::new(u64::MAX,) - )), - &Default::default(), - &Default::default(), - vp_cache.clone(), - ) - .is_err() - ); + } Ok(()) }); assert!(result.is_ok()); diff --git a/crates/sdk/src/lib.rs b/crates/sdk/src/lib.rs index fd186d1346..65c3e457e6 100644 --- a/crates/sdk/src/lib.rs +++ b/crates/sdk/src/lib.rs @@ -1245,11 +1245,18 @@ pub mod testing { } prop_compose! { - /// Generate an arbitrary transaction with maybe a memo + /// Generate an arbitrary transaction with maybe a valid memo pub fn arb_memoed_tx()( (mut tx, tx_data) in arb_tx(), memo in option::of(arb_utf8_code()), ) -> (Tx, TxData) { + // Clean up any previous memo commitments + let mut batch: Vec<_> = tx.header.batch.iter().cloned().collect(); + for inner_tx in batch.iter_mut() { + inner_tx.memo_hash = Default::default(); + } + tx.header.batch = batch.into_iter().collect(); + if let Some(memo) = memo { let sechash = tx .add_section(Section::ExtraData(memo)) From 9171b37e7bd592f24b94a655218e9fcb33a024e8 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 21 Nov 2024 15:31:53 +0100 Subject: [PATCH 8/8] Fixes insufficient gas in integration test --- crates/tests/src/integration/masp.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 132fb572f4..c447c0ba24 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -2145,6 +2145,8 @@ fn masp_incentives() -> Result<()> { NAM, "--amount", "1.451732", + "--gas-limit", + "60000", "--signing-keys", BERTHA_KEY, "--node",