diff --git a/.changelog/unreleased/bug-fixes/3804-fix-transfer-dest-vp-trigger.md b/.changelog/unreleased/bug-fixes/3804-fix-transfer-dest-vp-trigger.md new file mode 100644 index 0000000000..262d4934bd --- /dev/null +++ b/.changelog/unreleased/bug-fixes/3804-fix-transfer-dest-vp-trigger.md @@ -0,0 +1,3 @@ +- The multitoken vp now checks that the involved parties + validate the transaction. Improved tests and transfer code. + ([\#3804](https://github.com/anoma/namada/pull/3804)) \ No newline at end of file diff --git a/Cargo.lock b/Cargo.lock index db39370be6..ffd306a156 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5377,6 +5377,8 @@ dependencies = [ "namada_core", "namada_events", "namada_gas", + "namada_governance", + "namada_ibc", "namada_io", "namada_macros", "namada_migrations", @@ -5385,6 +5387,8 @@ dependencies = [ "namada_systems", "namada_trans_token", "namada_tx", + "namada_vm", + "namada_vp", "namada_vp_env", "namada_wallet", "proptest", diff --git a/clippy.toml b/clippy.toml index 4ae91bc7ca..2fc50cebd1 100644 --- a/clippy.toml +++ b/clippy.toml @@ -8,6 +8,7 @@ disallowed-methods = [ { path = "chrono::Utc::now", reason = "Do not use current date/time in code that must be deterministic" }, { path = "namada_core::time::DateTimeUtc::now", reason = "Do not use current date/time in code that must be deterministic" }, { path = "wasmtimer::std::Instant", reason = "Do not use current date/time in code that must be deterministic" }, + { path = "std::result::Result::unwrap_or_default", reason = "Do not silently ignore an error that could be caused by gas" }, ] allow-dbg-in-tests = true allow-print-in-tests = true diff --git a/crates/apps_lib/src/client/utils.rs b/crates/apps_lib/src/client/utils.rs index 6a227ea362..8e8c7c6fc5 100644 --- a/crates/apps_lib/src/client/utils.rs +++ b/crates/apps_lib/src/client/utils.rs @@ -991,6 +991,7 @@ pub async fn sign_genesis_tx( if let Some(output_path) = output.as_ref() { // If the output path contains existing signed txs, we append // the newly signed txs to the file + #[allow(clippy::disallowed_methods)] let mut prev_txs = genesis::templates::read_transactions(output_path) .unwrap_or_default(); diff --git a/crates/governance/src/finalize_block.rs b/crates/governance/src/finalize_block.rs index 6eea729bc9..6dc6b8d012 100644 --- a/crates/governance/src/finalize_block.rs +++ b/crates/governance/src/finalize_block.rs @@ -324,6 +324,7 @@ where if vote.is_validator() { let vote_data = vote.data.clone(); + #[allow(clippy::disallowed_methods)] let validator_stake = PoS::read_validator_stake::>( storage, validator, epoch, ) diff --git a/crates/governance/src/utils.rs b/crates/governance/src/utils.rs index 9612a166be..dd677b6e17 100644 --- a/crates/governance/src/utils.rs +++ b/crates/governance/src/utils.rs @@ -221,6 +221,7 @@ impl ProposalResult { /// Return true if at least 2/3 of the total voting power voted and at least /// two third of the non-abstained voting power voted nay. /// Returns `false` if any arithmetic fails. + #[allow(clippy::disallowed_methods)] pub fn two_thirds_nay_over_two_thirds_total(&self) -> bool { (|| { let two_thirds_power = diff --git a/crates/governance/src/vp/pgf.rs b/crates/governance/src/vp/pgf.rs index 6f2c656652..c11bedc428 100644 --- a/crates/governance/src/vp/pgf.rs +++ b/crates/governance/src/vp/pgf.rs @@ -119,15 +119,10 @@ where match key_type { KeyType::Stewards(steward_address) => { let stewards_have_increased = { - // TODO(namada#3238): we should check errors here, which - // could be out-of-gas related - let total_stewards_pre = pgf_storage::stewards_handle() - .len(&ctx.pre()) - .unwrap_or_default(); + let total_stewards_pre = + pgf_storage::stewards_handle().len(&ctx.pre())?; let total_stewards_post = - pgf_storage::stewards_handle() - .len(&ctx.post()) - .unwrap_or_default(); + pgf_storage::stewards_handle().len(&ctx.post())?; total_stewards_pre < total_stewards_post }; diff --git a/crates/ibc/src/lib.rs b/crates/ibc/src/lib.rs index b77fef8da4..ff5d8d34f4 100644 --- a/crates/ibc/src/lib.rs +++ b/crates/ibc/src/lib.rs @@ -32,6 +32,7 @@ use std::collections::BTreeSet; use std::fmt::Debug; use std::marker::PhantomData; use std::rc::Rc; +use std::str::FromStr; pub use actions::transfer_over_ibc; use apps::transfer::types::packet::PacketData; @@ -54,13 +55,16 @@ use ibc::apps::nft_transfer::types::msgs::transfer::MsgTransfer as IbcMsgNftTran use ibc::apps::nft_transfer::types::{ ack_success_b64, is_receiver_chain_source as is_nft_receiver_chain_source, PrefixedClassId, TokenId, TracePrefix as NftTracePrefix, + PORT_ID_STR as NFT_PORT_ID_STR, }; use ibc::apps::transfer::handler::{ send_transfer_execute, send_transfer_validate, }; use ibc::apps::transfer::types::error::TokenTransferError; use ibc::apps::transfer::types::msgs::transfer::MsgTransfer as IbcMsgTransfer; -use ibc::apps::transfer::types::{is_receiver_chain_source, TracePrefix}; +use ibc::apps::transfer::types::{ + is_receiver_chain_source, TracePrefix, PORT_ID_STR as FT_PORT_ID_STR, +}; use ibc::core::channel::types::acknowledgement::AcknowledgementStatus; use ibc::core::channel::types::commitment::compute_ack_commitment; use ibc::core::channel::types::msgs::{ @@ -139,6 +143,8 @@ pub enum Error { ChainId(IdentifierError), #[error("Verifier insertion error: {0}")] Verifier(StorageError), + #[error("IBC error: {0}")] + Other(String), } impl From for StorageError { @@ -612,6 +618,18 @@ where self.ctx.inner.clone(), self.verifiers.clone(), ); + // Add the source to the set of verifiers + self.verifiers.borrow_mut().insert( + Address::from_str(msg.message.packet_data.sender.as_ref()) + .map_err(|_| { + Error::TokenTransfer(TokenTransferError::Other( + format!( + "Cannot convert the sender address {}", + msg.message.packet_data.sender + ), + )) + })?, + ); self.insert_verifiers()?; if msg.transfer.is_some() { token_transfer_ctx.enable_shielded_transfer(); @@ -630,6 +648,19 @@ where if msg.transfer.is_some() { nft_transfer_ctx.enable_shielded_transfer(); } + // Add the source to the set of verifiers + self.verifiers.borrow_mut().insert( + Address::from_str(msg.message.packet_data.sender.as_ref()) + .map_err(|_| { + Error::NftTransfer(NftTransferError::Other( + format!( + "Cannot convert the sender address {}", + msg.message.packet_data.sender + ), + )) + })?, + ); + self.insert_verifiers()?; send_nft_transfer_execute( &mut self.ctx, &mut nft_transfer_ctx, @@ -639,8 +670,21 @@ where Ok((msg.transfer, None)) } IbcMessage::Envelope(envelope) => { + if let Some(verifier) = get_envelope_verifier(envelope.as_ref()) + { + self.verifiers.borrow_mut().insert( + Address::from_str(verifier.as_ref()).map_err(|_| { + Error::Other(format!( + "Cannot convert the address {}", + verifier, + )) + })?, + ); + self.insert_verifiers()?; + } execute(&mut self.ctx, &mut self.router, *envelope.clone()) .map_err(|e| Error::Context(Box::new(e)))?; + // Extract MASP tx from the memo in the packet if needed let masp_tx = match &*envelope { MsgEnvelope::Packet(PacketMsg::Recv(msg)) @@ -734,6 +778,70 @@ where } } +// Extract the involved namada address from the packet (either sender or +// receiver) to trigger its vp. Returns None if an address could not be found +fn get_envelope_verifier( + envelope: &MsgEnvelope, +) -> Option { + match envelope { + MsgEnvelope::Packet(PacketMsg::Recv(msg)) => { + match msg.packet.port_id_on_b.as_str() { + FT_PORT_ID_STR => { + serde_json::from_slice::(&msg.packet.data) + .ok() + .map(|packet_data| packet_data.receiver) + } + NFT_PORT_ID_STR => { + serde_json::from_slice::(&msg.packet.data) + .ok() + .map(|packet_data| packet_data.receiver) + } + _ => None, + } + } + MsgEnvelope::Packet(PacketMsg::Ack(msg)) => serde_json::from_slice::< + AcknowledgementStatus, + >( + msg.acknowledgement.as_ref(), + ) + .map_or(None, |ack| { + if ack.is_successful() { + None + } else { + match msg.packet.port_id_on_a.as_str() { + FT_PORT_ID_STR => { + serde_json::from_slice::(&msg.packet.data) + .ok() + .map(|packet_data| packet_data.sender) + } + NFT_PORT_ID_STR => serde_json::from_slice::( + &msg.packet.data, + ) + .ok() + .map(|packet_data| packet_data.sender), + _ => None, + } + } + }), + MsgEnvelope::Packet(PacketMsg::Timeout(msg)) => { + match msg.packet.port_id_on_a.as_str() { + FT_PORT_ID_STR => { + serde_json::from_slice::(&msg.packet.data) + .ok() + .map(|packet_data| packet_data.sender) + } + NFT_PORT_ID_STR => { + serde_json::from_slice::(&msg.packet.data) + .ok() + .map(|packet_data| packet_data.sender) + } + _ => None, + } + } + _ => None, + } +} + /// Tries to decode transaction data to an `IbcMessage` pub fn decode_message( tx_data: &[u8], diff --git a/crates/node/src/shell/mod.rs b/crates/node/src/shell/mod.rs index 4911a4c6f8..ea27241312 100644 --- a/crates/node/src/shell/mod.rs +++ b/crates/node/src/shell/mod.rs @@ -633,6 +633,7 @@ where }; if let Some(schedule_migration) = scheduled_migration.as_ref() { + #[allow(clippy::disallowed_methods)] let current = state.get_block_height().unwrap_or_default(); if schedule_migration.height < current { panic!( diff --git a/crates/node/src/shell/snapshots.rs b/crates/node/src/shell/snapshots.rs index 40572677f6..227fdd5daa 100644 --- a/crates/node/src/shell/snapshots.rs +++ b/crates/node/src/shell/snapshots.rs @@ -112,6 +112,7 @@ impl Shell { } match self.syncing.as_ref() { None => { + #[allow(clippy::disallowed_methods)] if self.state.get_block_height().unwrap_or_default().0 < u64::from(req.snapshot.height) { diff --git a/crates/node/src/shell/testing/node.rs b/crates/node/src/shell/testing/node.rs index abebd5cdd6..4a213c6b05 100644 --- a/crates/node/src/shell/testing/node.rs +++ b/crates/node/src/shell/testing/node.rs @@ -361,6 +361,7 @@ impl MockNode { } pub fn block_height(&self) -> BlockHeight { + #[allow(clippy::disallowed_methods)] self.shell .lock() .unwrap() diff --git a/crates/node/src/shims/abcipp_shim_types.rs b/crates/node/src/shims/abcipp_shim_types.rs index c4b7dad9b4..3e266d60b5 100644 --- a/crates/node/src/shims/abcipp_shim_types.rs +++ b/crates/node/src/shims/abcipp_shim_types.rs @@ -223,6 +223,7 @@ pub mod shim { let header = req.header; FinalizeBlock { header: BlockHeader { + #[allow(clippy::disallowed_methods)] hash: Hash::try_from(header.app_hash.as_bytes()) .unwrap_or_default(), time: DateTimeUtc::try_from(header.time).unwrap(), diff --git a/crates/proof_of_stake/src/rewards.rs b/crates/proof_of_stake/src/rewards.rs index 00a77f27c3..441a73d6d4 100644 --- a/crates/proof_of_stake/src/rewards.rs +++ b/crates/proof_of_stake/src/rewards.rs @@ -261,12 +261,14 @@ where // Ensure TM stake updates properly with a debug_assert if cfg!(debug_assertions) { + #[allow(clippy::disallowed_methods)] + let validator_vp = i64::try_from(validator_vp).unwrap_or_default(); debug_assert_eq!( into_tm_voting_power( params.tm_votes_per_token, stake_from_deltas, ), - i64::try_from(validator_vp).unwrap_or_default(), + validator_vp ); } diff --git a/crates/proof_of_stake/src/tests/test_pos.rs b/crates/proof_of_stake/src/tests/test_pos.rs index 2e1816fab1..aa5fee2cfd 100644 --- a/crates/proof_of_stake/src/tests/test_pos.rs +++ b/crates/proof_of_stake/src/tests/test_pos.rs @@ -1159,6 +1159,7 @@ fn test_unslashed_bond_amount_aux(validators: Vec) { Epoch(0), current_epoch + params.pipeline_len, ) { + #[allow(clippy::disallowed_methods)] let amount = bond_amount( &storage, &BondId { diff --git a/crates/proof_of_stake/src/tests/test_slash_and_redel.rs b/crates/proof_of_stake/src/tests/test_slash_and_redel.rs index ab404cad4d..f0aad33032 100644 --- a/crates/proof_of_stake/src/tests/test_slash_and_redel.rs +++ b/crates/proof_of_stake/src/tests/test_slash_and_redel.rs @@ -1638,6 +1638,7 @@ fn test_slashed_bond_amount_aux(validators: Vec) { let pipeline_epoch = current_epoch + params.pipeline_len; + #[allow(clippy::disallowed_methods)] let del_bond_amount = bond_amount( &storage, &BondId { @@ -1648,6 +1649,7 @@ fn test_slashed_bond_amount_aux(validators: Vec) { ) .unwrap_or_default(); + #[allow(clippy::disallowed_methods)] let self_bond_amount = bond_amount( &storage, &BondId { diff --git a/crates/sdk/src/lib.rs b/crates/sdk/src/lib.rs index 773d94017f..f3f0e2284b 100644 --- a/crates/sdk/src/lib.rs +++ b/crates/sdk/src/lib.rs @@ -82,7 +82,7 @@ pub use {namada_io as io, namada_wallet as wallet}; use crate::masp::ShieldedContext; /// Default gas-limit -pub const DEFAULT_GAS_LIMIT: u64 = 200_000; +pub const DEFAULT_GAS_LIMIT: u64 = 250_000; /// An interface for high-level interaction with the Namada SDK #[cfg_attr(feature = "async-send", async_trait::async_trait)] diff --git a/crates/sdk/src/queries/vp/token.rs b/crates/sdk/src/queries/vp/token.rs index caea8392f8..9d232696cb 100644 --- a/crates/sdk/src/queries/vp/token.rs +++ b/crates/sdk/src/queries/vp/token.rs @@ -101,6 +101,7 @@ pub mod client_only_methods { let balance = if response.data.is_empty() { token::Amount::zero() } else { + #[allow(clippy::disallowed_methods)] token::Amount::try_from_slice(&response.data) .unwrap_or_default() }; @@ -124,6 +125,7 @@ pub mod client_only_methods { let tokens = if response.data.is_empty() { token::Amount::zero() } else { + #[allow(clippy::disallowed_methods)] token::Amount::try_from_slice(&response.data) .unwrap_or_default() }; diff --git a/crates/sdk/src/rpc.rs b/crates/sdk/src/rpc.rs index e1e0fd7589..8958778c35 100644 --- a/crates/sdk/src/rpc.rs +++ b/crates/sdk/src/rpc.rs @@ -994,10 +994,12 @@ pub async fn query_proposal_result( let is_author_pgf_steward = is_steward(client, &proposal.author).await; + #[allow(clippy::disallowed_methods)] let votes = query_proposal_votes(client, proposal_id) .await .unwrap_or_default(); let tally_type = proposal.get_tally_type(is_author_pgf_steward); + #[allow(clippy::disallowed_methods)] let total_active_voting_power = get_total_active_voting_power(client, tally_epoch) .await @@ -1008,6 +1010,7 @@ pub async fn query_proposal_result( for vote in votes { match vote.is_validator() { true => { + #[allow(clippy::disallowed_methods)] let voting_power = get_validator_stake( client, tally_epoch, @@ -1023,6 +1026,7 @@ pub async fn query_proposal_result( ); } false => { + #[allow(clippy::disallowed_methods)] let voting_power = get_bond_amount_at( client, &vote.delegator, diff --git a/crates/sdk/src/signing.rs b/crates/sdk/src/signing.rs index f8c70ab659..8d1c058489 100644 --- a/crates/sdk/src/signing.rs +++ b/crates/sdk/src/signing.rs @@ -483,6 +483,7 @@ pub async fn validate_transparent_fee( let fee_payer_address = Address::from(fee_payer); let balance_key = balance_key(&args.fee_token, &fee_payer_address); + #[allow(clippy::disallowed_methods)] let balance = rpc::query_storage_value::<_, token::Amount>( context.client(), &balance_key, diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 116ed815e7..9e74fd3cc5 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -3151,6 +3151,7 @@ async fn get_masp_fee_payment_amount( ) -> Result> { let fee_payer_address = Address::from(fee_payer); let balance_key = balance_key(&args.fee_token, &fee_payer_address); + #[allow(clippy::disallowed_methods)] let balance = rpc::query_storage_value::<_, token::Amount>( context.client(), &balance_key, @@ -3492,6 +3493,7 @@ async fn construct_shielded_parts( // Get the decoded asset types used in the transaction to give offline // wallet users more information + #[allow(clippy::disallowed_methods)] let asset_types = used_asset_types(context, &shielded_parts.builder) .await .unwrap_or_default(); diff --git a/crates/shielded_token/Cargo.toml b/crates/shielded_token/Cargo.toml index 4243999b25..126c543462 100644 --- a/crates/shielded_token/Cargo.toml +++ b/crates/shielded_token/Cargo.toml @@ -78,9 +78,13 @@ xorf.workspace = true [dev-dependencies] namada_gas = { path = "../gas" } +namada_governance = { path = "../governance", features = ["testing"] } +namada_ibc = { path = "../ibc", features = ["testing"] } namada_parameters = { path = "../parameters", features = ["testing"] } namada_state = { path = "../state", features = ["testing"] } namada_trans_token = { path = "../trans_token" } +namada_vm = { path = "../vm", features = ["testing"] } +namada_vp = { path = "../vp" } lazy_static.workspace = true masp_primitives = { workspace = true, features = ["test-dependencies"] } diff --git a/crates/shielded_token/src/masp.rs b/crates/shielded_token/src/masp.rs index d2cc14a60f..cc1fc79fbb 100644 --- a/crates/shielded_token/src/masp.rs +++ b/crates/shielded_token/src/masp.rs @@ -259,6 +259,7 @@ pub fn is_amount_required( for (asset_type, value) in gap.components() { if *value > 0 && normed_delta[asset_type] > 0 { + #[allow(clippy::disallowed_methods)] let signed_change_amt = checked!(normed_delta[asset_type] - *value).unwrap_or_default(); let unsigned_change_amt = if signed_change_amt > 0 { diff --git a/crates/shielded_token/src/masp/shielded_sync/dispatcher.rs b/crates/shielded_token/src/masp/shielded_sync/dispatcher.rs index 3f5a3ec416..27675a3838 100644 --- a/crates/shielded_token/src/masp/shielded_sync/dispatcher.rs +++ b/crates/shielded_token/src/masp/shielded_sync/dispatcher.rs @@ -291,6 +291,7 @@ where panic_flag: PanicFlag::default(), }; + #[allow(clippy::disallowed_methods)] let cache = ctx.utils.cache_load().await.unwrap_or_default(); Dispatcher { diff --git a/crates/shielded_token/src/storage_key.rs b/crates/shielded_token/src/storage_key.rs index e7a76a71e1..f79d5865f0 100644 --- a/crates/shielded_token/src/storage_key.rs +++ b/crates/shielded_token/src/storage_key.rs @@ -7,6 +7,8 @@ use namada_core::hash::Hash; use namada_core::storage::{self, DbKeySeg, KeySeg}; use namada_systems::trans_token; +// Key segment for a balance key +const BALANCE_STORAGE_KEY: &str = "balance"; /// Key segment prefix for the nullifiers pub const MASP_NULLIFIERS_KEY: &str = "nullifiers"; /// Key segment prefix for the note commitment merkle tree @@ -83,12 +85,27 @@ pub fn masp_last_inflation_key( .with_segment(MASP_LAST_INFLATION_KEY.to_owned()) } +/// Check if the given storage key is MASP transparent balance key +pub fn is_masp_balance_key(key: &storage::Key) -> bool { + matches!( + &key.segments[..], + [DbKeySeg::AddressSeg(addr), DbKeySeg::AddressSeg(_token), DbKeySeg::StringSeg(balance), DbKeySeg::AddressSeg(owner)] + if *addr + == Address::Internal(address::InternalAddress::Multitoken) + && balance == BALANCE_STORAGE_KEY + && *owner == address::MASP + ) +} + /// Check if the given storage key is a masp key pub fn is_masp_key(key: &storage::Key) -> bool { - matches!(&key.segments[..], - [DbKeySeg::AddressSeg(addr), ..] if *addr == address::MASP - ) + match &key.segments[..] { + [DbKeySeg::AddressSeg(addr), ..] if *addr == address::MASP => true, + // The balance key of the MASP is also considered a MASP key + _ => is_masp_balance_key(key), + } } + /// Check if the given storage key is allowed to be touched by a masp transfer pub fn is_masp_transfer_key(key: &storage::Key) -> bool { match &key.segments[..] { @@ -98,13 +115,12 @@ pub fn is_masp_transfer_key(key: &storage::Key) -> bool { { true } - [ DbKeySeg::AddressSeg(addr), DbKeySeg::StringSeg(key), DbKeySeg::StringSeg(_nullifier), ] if *addr == address::MASP && key == MASP_NULLIFIERS_KEY => true, - _ => false, + _ => is_masp_balance_key(key), } } diff --git a/crates/shielded_token/src/utils.rs b/crates/shielded_token/src/utils.rs index 11c0fe9fc7..1cdd51b18e 100644 --- a/crates/shielded_token/src/utils.rs +++ b/crates/shielded_token/src/utils.rs @@ -73,11 +73,11 @@ pub fn handle_masp_tx( Ok(()) } -/// Check if a transaction was a MASP transaction. +/// Check if a transaction is a MASP transfer transaction. /// -/// This means that at least one key owned by MASP was changed. We cannot simply -/// check that the MASP VP was triggered, as this can be manually requested to -/// be triggered by users. +/// We do that by looking at the changed keys. We cannot simply check that the +/// MASP VP was triggered, as this can be manually requested to be triggered by +/// users. pub fn is_masp_transfer(changed_keys: &BTreeSet) -> bool { changed_keys.iter().any(is_masp_transfer_key) } diff --git a/crates/shielded_token/src/vp.rs b/crates/shielded_token/src/vp.rs index 1851cd6fcb..ef8cdc2781 100644 --- a/crates/shielded_token/src/vp.rs +++ b/crates/shielded_token/src/vp.rs @@ -878,3 +878,104 @@ fn verify_sapling_balancing_value( Err(error) } } + +#[cfg(test)] +mod shielded_token_tests { + use std::cell::RefCell; + use std::collections::BTreeSet; + + use namada_core::address::testing::nam; + use namada_core::address::MASP; + use namada_core::borsh::BorshSerializeExt; + use namada_gas::{TxGasMeter, VpGasMeter}; + use namada_state::testing::TestState; + use namada_state::{StateRead, TxIndex}; + use namada_trans_token::storage_key::balance_key; + use namada_trans_token::Amount; + use namada_tx::{BatchedTx, Tx}; + use namada_vm::wasm::compilation_cache::common::testing::vp_cache; + use namada_vm::wasm::run::VpEvalWasm; + use namada_vm::wasm::VpCache; + use namada_vm::WasmCacheRwAccess; + use namada_vp::native_vp::{self, CtxPostStorageRead, CtxPreStorageRead}; + + type CA = WasmCacheRwAccess; + type Eval = VpEvalWasm<::D, ::H, CA>; + type Ctx<'ctx, S> = native_vp::Ctx<'ctx, S, VpCache, Eval>; + type MaspVp<'ctx, S> = super::MaspVp< + 'ctx, + Ctx<'ctx, S>, + namada_parameters::Store< + CtxPreStorageRead<'ctx, 'ctx, S, VpCache, Eval>, + >, + namada_governance::Store< + CtxPreStorageRead<'ctx, 'ctx, S, VpCache, Eval>, + >, + namada_ibc::Store< + CtxPostStorageRead<'ctx, 'ctx, S, VpCache, Eval>, + >, + namada_trans_token::Store< + CtxPreStorageRead<'ctx, 'ctx, S, VpCache, Eval>, + >, + (), + >; + + // Changing only the balance key of the MASP alone is invalid + #[test] + fn test_balance_change() { + let mut state = TestState::default(); + namada_parameters::init_test_storage(&mut state).unwrap(); + let src_key = balance_key(&nam(), &MASP); + let amount = Amount::native_whole(100); + let keys_changed = BTreeSet::from([src_key.clone()]); + let verifiers = Default::default(); + + // Initialize MASP balance + state + .db_write(&src_key, Amount::native_whole(100).serialize_to_vec()) + .unwrap(); + + state.db_write(&src_key, amount.serialize_to_vec()).unwrap(); + + let tx_index = TxIndex::default(); + let mut tx = Tx::from_type(namada_tx::data::TxType::Raw); + tx.push_default_inner_tx(); + let BatchedTx { tx, cmt } = tx.batch_first_tx(); + + // Test both credit and debit + for new_amount in [150, 1] { + // Update the balance key + let new_amount = Amount::native_whole(new_amount); + let _ = state + .write_log_mut() + .write(&src_key, new_amount.serialize_to_vec()) + .unwrap(); + + let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( + &TxGasMeter::new(u64::MAX), + )); + let (vp_vp_cache, _vp_cache_dir) = vp_cache(); + let ctx = Ctx::new( + &MASP, + &state, + &tx, + &cmt, + &tx_index, + &gas_meter, + &keys_changed, + &verifiers, + vp_vp_cache, + ); + + assert!( + MaspVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ) + .is_err() + ); + } + } +} diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index 275c971794..cf0f4c8747 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -73,6 +73,9 @@ const UPGRADED_CHAIN_ID: &str = "upgraded-chain"; /// - When unshielding transfer failure, /// - Mint the IBC token for the refund /// - Unescrow the token for the refund +/// 6. Malformed shielded actions +/// - Missing memo +/// - Wrong memo #[test] fn ibc_transfers() -> Result<()> { let update_genesis = @@ -384,6 +387,56 @@ fn ibc_transfers() -> Result<()> { check_balance(&test, AA_VIEWING_KEY, APFEL, 0)?; check_balance(&test, IBC_REFUND_TARGET_ALIAS, APFEL, 1)?; + // 6. Malformed shielded actions + + // Check initial balance + check_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 40)?; + check_gaia_balance(&test_gaia, GAIA_USER, GAIA_COIN, 810)?; + + // Missing memo + transfer_from_gaia( + &test_gaia, + GAIA_USER, + AA_PAYMENT_ADDRESS, + GAIA_COIN, + 100, + &port_id_gaia, + &channel_id_gaia, + None, + // MASP VP shall reject it, make it timeout + Some(Duration::new(10, 0)), + )?; + wait_for_packet_relay(&port_id_namada, &channel_id_namada, &test)?; + // Check the balance didn't change + check_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 40)?; + check_gaia_balance(&test_gaia, GAIA_USER, GAIA_COIN, 810)?; + + // Wrong memo (different amount) + let shielding_data_path = gen_ibc_shielding_data( + &test, + AA_PAYMENT_ADDRESS, + GAIA_COIN, + 100, + &port_id_namada, + &channel_id_namada, + )?; + transfer_from_gaia( + &test_gaia, + GAIA_USER, + AA_PAYMENT_ADDRESS, + GAIA_COIN, + 101, + &port_id_gaia, + &channel_id_gaia, + Some(shielding_data_path), + // MASP VP shall reject it, make it timeout + Some(Duration::new(10, 0)), + )?; + wait_for_packet_relay(&port_id_gaia, &channel_id_gaia, &test_gaia)?; + // Check the balances didn't change + check_balance(&test, AA_VIEWING_KEY, &ibc_denom_on_namada, 40)?; + check_gaia_balance(&test_gaia, GAIA_USER, GAIA_COIN, 810)?; + Ok(()) } @@ -508,7 +561,9 @@ fn ibc_token_inflation() -> Result<()> { let delegated = epoch + PIPELINE_LEN; while epoch < delegated { sleep(10); - epoch = get_epoch(&test, &rpc).unwrap_or_default(); + #[allow(clippy::disallowed_methods)] + let new_epoch = get_epoch(&test, &rpc).unwrap_or_default(); + epoch = new_epoch; } // inflation proposal on Namada let start_epoch = propose_inflation(&test)?; @@ -516,7 +571,9 @@ fn ibc_token_inflation() -> Result<()> { // Vote while epoch < start_epoch { sleep(10); - epoch = get_epoch(&test, &rpc).unwrap_or_default(); + #[allow(clippy::disallowed_methods)] + let new_epoch = get_epoch(&test, &rpc).unwrap_or_default(); + epoch = new_epoch; } submit_votes(&test)?; @@ -566,7 +623,9 @@ fn ibc_token_inflation() -> Result<()> { let new_epoch = epoch + MASP_EPOCH_MULTIPLIER; while epoch < new_epoch { sleep(10); - epoch = get_epoch(&test, &rpc).unwrap_or_default(); + #[allow(clippy::disallowed_methods)] + let new_epoch = get_epoch(&test, &rpc).unwrap_or_default(); + epoch = new_epoch; } // Check balances diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 71b287cfe5..4bc8292bc1 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -687,7 +687,7 @@ fn masp_incentives() -> Result<()> { "--token", NAM, "--gas-limit", - "250000", + "300000", "--amount", "1.451732", "--signing-keys", @@ -2582,8 +2582,6 @@ fn masp_fee_payment() -> Result<()> { NAM, "--amount", "10000", - "--gas-limit", - "200000", "--gas-price", "1", "--disposable-gas-payer", @@ -2617,7 +2615,7 @@ fn masp_fee_payment() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 290000")); + assert!(captured.contains("nam: 240000")); let captured = CapturedOutput::of(|| { run( &node, @@ -2750,7 +2748,7 @@ fn masp_fee_payment_gas_limit() -> Result<()> { "--amount", "1", "--gas-limit", - "300000", + "500000", "--gas-price", "1", "--disposable-gas-payer", @@ -2920,7 +2918,7 @@ fn masp_fee_payment_with_non_disposable() -> Result<()> { "--amount", "1", "--gas-limit", - "200000", + "300000", "--gas-price", "1", "--gas-payer", @@ -2959,7 +2957,7 @@ fn masp_fee_payment_with_non_disposable() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 1799999")); + assert!(captured.contains("nam: 1699999")); let captured = CapturedOutput::of(|| { run( @@ -3067,7 +3065,7 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> { "--token", NAM, "--amount", - "250000", + "300000", "--ledger-address", validator_one_rpc, ], @@ -3117,7 +3115,7 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("nam: 250000")); + assert!(captured.contains("nam: 300000")); // Masp fee payment with custom gas payer let captured = CapturedOutput::of(|| { @@ -3135,7 +3133,7 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> { "--amount", "9000", "--gas-limit", - "250000", + "300000", "--gas-price", "1", "--gas-spending-key", @@ -3314,7 +3312,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> { "--token", BTC, "--amount", - "250000", + "300000", "--gas-payer", ALBERT_KEY, "--ledger-address", @@ -3383,7 +3381,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("btc: 250000")); + assert!(captured.contains("btc: 300000")); // Masp fee payment with custom token and gas payer let captured = CapturedOutput::of(|| { @@ -3403,7 +3401,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> { "--gas-token", BTC, "--gas-limit", - "250000", + "300000", "--gas-price", "1", "--gas-spending-key", diff --git a/crates/tests/src/vm_host_env/mod.rs b/crates/tests/src/vm_host_env/mod.rs index 4e6aa6fd64..0a7a3c858a 100644 --- a/crates/tests/src/vm_host_env/mod.rs +++ b/crates/tests/src/vm_host_env/mod.rs @@ -1344,8 +1344,10 @@ mod tests { // Check let mut env = tx_host_env::take(); - // The token must be part of the verifier set (checked by MultitokenVp) + // The token and sender must be part of the verifier set (checked by + // MultitokenVp) env.verifiers.insert(ibc_token); + env.verifiers.insert(sender); let result = ibc::validate_ibc_vp_from_tx( &env, &tx.batch_ref_first_tx().unwrap(), @@ -1434,10 +1436,12 @@ mod tests { ); assert!(result.is_ok()); // Check if the token was minted - // The token must be part of the verifier set (checked by MultitokenVp) + // The token and recevier must be part of the verifier set (checked by + // MultitokenVp) let denom = format!("{}/{}/{}", port_id, channel_id, token); let ibc_token = ibc::ibc_token(&denom); env.verifiers.insert(ibc_token.clone()); + env.verifiers.insert(receiver.clone()); let result = ibc::validate_ibc_vp_from_tx( &env, &tx.batch_ref_first_tx().unwrap(), diff --git a/crates/token/src/lib.rs b/crates/token/src/lib.rs index 69460ff73e..dff43909bf 100644 --- a/crates/token/src/lib.rs +++ b/crates/token/src/lib.rs @@ -474,6 +474,9 @@ pub mod testing { #[cfg(test)] mod test_token_transfer_actions { use namada_core::address::testing::{established_address_1, nam}; + use namada_core::address::{self}; + use namada_core::storage::DbKeySeg; + use namada_shielded_token::storage_key::is_masp_balance_key; use super::*; @@ -599,4 +602,37 @@ mod test_token_transfer_actions { transfer2(BTreeMap::from([(account.clone(), amount_80)])), ); } + + /// Check that the MASP balance key is a transparent balance key. + #[test] + fn test_masp_trans_balance_key() { + let token = nam(); + let key = namada_trans_token::storage_key::balance_key( + &token, + &address::MASP, + ); + assert!(is_masp_balance_key(&key)); + + // Replace the token address and check that it still matches + let mut another_token_key = key.clone(); + another_token_key.segments[1] = + DbKeySeg::AddressSeg(address::testing::gen_established_address()); + assert!(is_masp_balance_key(&another_token_key)); + + // Replace one of the non-token segments with some random string or + // address and check that it no longer matches. + // Skip index 1 which is the token address. + for segment_num in [0, 2, 3] { + let mut key = key.clone(); + key.segments[segment_num] = match &key.segments[segment_num] { + DbKeySeg::AddressSeg(_) => DbKeySeg::AddressSeg( + address::testing::gen_established_address(), + ), + DbKeySeg::StringSeg(_) => { + DbKeySeg::StringSeg("Dangus".to_string()) + } + }; + assert!(!is_masp_balance_key(&key)); + } + } } diff --git a/crates/trans_token/src/vp.rs b/crates/trans_token/src/vp.rs index 471ea24ae3..64b7aae644 100644 --- a/crates/trans_token/src/vp.rs +++ b/crates/trans_token/src/vp.rs @@ -98,6 +98,13 @@ where let mut dec_mints: HashMap = HashMap::new(); for key in keys_changed { if let Some([token, owner]) = is_any_token_balance_key(key) { + if !verifiers.contains(owner) { + return Err(Error::new_alloc(format!( + "The vp of the address {} has not been triggered", + owner + ))); + } + let pre: Amount = ctx.read_pre(key)?.unwrap_or_default(); let post: Amount = ctx.read_post(key)?.unwrap_or_default(); match post.checked_sub(pre) { @@ -448,6 +455,7 @@ mod tests { let (vp_vp_cache, _vp_cache_dir) = vp_cache(); let mut verifiers = BTreeSet::new(); verifiers.insert(src); + verifiers.insert(dest); let ctx = Ctx::new( &ADDRESS, &state, @@ -559,8 +567,10 @@ mod tests { let mut verifiers = BTreeSet::new(); // for the minter verifiers.insert(minter); - // The token must be part of the verifier set (checked by MultitokenVp) + // The token and minter must be part of the verifier set (checked by + // MultitokenVp) verifiers.insert(token); + verifiers.insert(target); let ctx = Ctx::new( &ADDRESS, &state, @@ -932,6 +942,7 @@ mod tests { let (vp_vp_cache, _vp_cache_dir) = vp_cache(); let mut verifiers = BTreeSet::new(); verifiers.insert(src); + verifiers.insert(dest); let ctx = Ctx::new( &ADDRESS, &state, @@ -1026,6 +1037,7 @@ mod tests { let (vp_vp_cache, _vp_cache_dir) = vp_cache(); let mut verifiers = BTreeSet::new(); verifiers.insert(src); + verifiers.insert(dest); let ctx = Ctx::new( &ADDRESS, &state, @@ -1078,6 +1090,7 @@ mod tests { let (vp_vp_cache, _vp_cache_dir) = vp_cache(); let mut verifiers = BTreeSet::new(); verifiers.insert(src); + verifiers.insert(dest); let ctx = Ctx::new( &ADDRESS, &state, @@ -1100,4 +1113,85 @@ mod tests { Ok(_) ); } + + // The multitoken vps ensures that all the involved parties have their vp + // triggered + #[test] + fn test_verifiers() { + let mut state = init_state(); + let src1 = established_address_1(); + let dest1 = namada_core::address::MASP; + let src2 = namada_core::address::IBC; + let dest2 = established_address_2(); + let mut keys_changed = transfer(&mut state, &src1, &dest1); + keys_changed.append(&mut transfer(&mut state, &src2, &dest2)); + + let tx_index = TxIndex::default(); + let BatchedTx { tx, cmt } = dummy_tx(&state); + let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( + &TxGasMeter::new(u64::MAX), + )); + let (vp_vp_cache, _vp_cache_dir) = vp_cache(); + + let parties = BTreeSet::from([src1, dest1, src2, dest2]); + + // One at a time remove one of the expected verifiers of this + // transaction and check that the multitoken vp rejects the tx + for verifier in &parties { + let mut verifiers = parties.clone(); + verifiers.remove(verifier); + + let ctx = Ctx::new( + &ADDRESS, + &state, + &tx, + &cmt, + &tx_index, + &gas_meter, + &keys_changed, + &verifiers, + vp_vp_cache.clone(), + ); + + let err_msg = format!( + "The vp of the address {} has not been triggered", + verifier + ); + + match MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers, + ) + .unwrap_err() + { + Error::AllocMessage(msg) if msg == err_msg => (), + _ => panic!("Test failed with an unexpected error"), + } + } + + // Fnally run the validation with all the required verifiers + let ctx = Ctx::new( + &ADDRESS, + &state, + &tx, + &cmt, + &tx_index, + &gas_meter, + &keys_changed, + &parties, + vp_vp_cache, + ); + + assert!( + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &parties + ) + .is_ok() + ); + } } diff --git a/crates/tx_prelude/src/token.rs b/crates/tx_prelude/src/token.rs index 6718ebdfd3..a1b753e39e 100644 --- a/crates/tx_prelude/src/token.rs +++ b/crates/tx_prelude/src/token.rs @@ -24,8 +24,9 @@ pub fn transfer( token: &Address, amount: Amount, ) -> TxResult { - // The tx must be authorized by the source address + // The tx must be authorized by the source and destination addresses ctx.insert_verifier(src)?; + ctx.insert_verifier(dest)?; if token.is_internal() { // Established address tokens do not have VPs themselves, their // validation is handled by the `Multitoken` internal address, but @@ -65,7 +66,7 @@ pub fn multi_transfer( let mut post_balances = BTreeMap::new(); for ((src, token), amount) in sources { - // The tx must be authorized by the source address + // The tx must be authorized by the involved address ctx.insert_verifier(src)?; if token.is_internal() { // Established address tokens do not have VPs themselves, their @@ -84,6 +85,8 @@ pub fn multi_transfer( } for ((dest, token), amount) in dests { + // The tx must be authorized by the involved address + ctx.insert_verifier(dest)?; if token.is_internal() { // Established address tokens do not have VPs themselves, their // validation is handled by the `Multitoken` internal address, but diff --git a/genesis/hardware/parameters.toml b/genesis/hardware/parameters.toml index 584a4b5258..cda15160aa 100644 --- a/genesis/hardware/parameters.toml +++ b/genesis/hardware/parameters.toml @@ -19,9 +19,9 @@ epochs_per_year = 31_536_000 # The multiplier for masp epochs masp_epoch_multiplier = 2 # Max gas for block -max_block_gas = 20_000_000 +max_block_gas = 25_000_000 # Masp fee payment gas limit -masp_fee_payment_gas_limit = 200_000 +masp_fee_payment_gas_limit = 250_000 # Gas scale gas_scale = 10_000 diff --git a/genesis/localnet/parameters.toml b/genesis/localnet/parameters.toml index 60f780cc1b..161e4da0ab 100644 --- a/genesis/localnet/parameters.toml +++ b/genesis/localnet/parameters.toml @@ -19,9 +19,9 @@ epochs_per_year = 31_536_000 # The multiplier for masp epochs masp_epoch_multiplier = 2 # Max gas for block -max_block_gas = 20_000_000 +max_block_gas = 25_000_000 # Masp fee payment gas limit -masp_fee_payment_gas_limit = 200_000 +masp_fee_payment_gas_limit = 250_000 # Gas scale gas_scale = 10_000 diff --git a/genesis/starter/parameters.toml b/genesis/starter/parameters.toml index ad8f91eaf4..79b53b6b1c 100644 --- a/genesis/starter/parameters.toml +++ b/genesis/starter/parameters.toml @@ -19,9 +19,9 @@ epochs_per_year = 31_536_000 # The multiplier for masp epochs masp_epoch_multiplier = 2 # Max gas for block -max_block_gas = 20_000_000 +max_block_gas = 25_000_000 # Masp fee payment gas limit -masp_fee_payment_gas_limit = 200_000 +masp_fee_payment_gas_limit = 250_000 # Gas scale gas_scale = 10_000