diff --git a/.changelog/unreleased/improvements/3669-fix-masp-ident.md b/.changelog/unreleased/improvements/3669-fix-masp-ident.md new file mode 100644 index 00000000000..72ecea91a34 --- /dev/null +++ b/.changelog/unreleased/improvements/3669-fix-masp-ident.md @@ -0,0 +1,2 @@ +- Improved the consistency and safety of MASP events construction in protocol. + ([\#3669](https://github.com/anoma/namada/pull/3669)) \ No newline at end of file diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 92c0d33169e..f7140293d12 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -7108,8 +7108,7 @@ pub mod args { ))) .arg(FEE_TOKEN.def().help(wrap!("The token for paying the gas"))) .arg(GAS_LIMIT.def().help(wrap!( - "The multiplier of the gas limit resolution defining the \ - maximum amount of gas needed to run transaction." + "The maximum amount of gas the transaction can use." ))) .arg(WALLET_ALIAS_FORCE.def().help(wrap!( "Override the alias without confirmation if it already exists." diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 46a3e693075..0759b5a01b7 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -391,52 +391,56 @@ where }); } res => { - let is_accepted = - matches!(&res, Ok(result) if result.is_accepted()); + let batched_tx_result = match &res { + Ok(batched_tx_result) => Some(batched_tx_result.to_owned()), + Err(_) => None, + }; extended_tx_result.tx_result.insert_inner_tx_result( wrapper_hash, either::Right(cmt), res, ); - if is_accepted { - // If the transaction was a masp one append the - // transaction refs for the events - let actions = - state.read_actions().map_err(Error::StateError)?; - if let Some(masp_section_ref) = - action::get_masp_section_ref(&actions).map_err( - |msg| { - Error::StateError(state::Error::Temporary { - error: msg.to_string(), - }) - }, - )? - { - extended_tx_result - .masp_tx_refs - .0 - .push(masp_section_ref); - } - if action::is_ibc_shielding_transfer(&*state) - .map_err(Error::StateError)? - { - extended_tx_result - .ibc_tx_data_refs - .0 - .push(*cmt.data_sechash()) - } - state.write_log_mut().commit_tx_to_batch(); - } else { - state.write_log_mut().drop_tx(); - if tx.header.atomic { - // Stop the execution of an atomic batch at the - // first failed transaction - return Err(DispatchError { - error: Error::FailingAtomicBatch(cmt.get_hash()), - tx_result: Some(extended_tx_result), - }); + match batched_tx_result { + Some(ref res) if res.is_accepted() => { + // If the transaction was a masp one append the + // transaction refs for the events. + if let Some(masp_ref) = get_optional_masp_ref( + state, + cmt, + Either::Right(res), + )? { + match masp_ref { + Either::Left(masp_section_ref) => { + extended_tx_result + .masp_tx_refs + .0 + .push(masp_section_ref); + } + Either::Right(data_sechash) => { + extended_tx_result + .ibc_tx_data_refs + .0 + .push(data_sechash) + } + } + } + state.write_log_mut().commit_tx_to_batch(); + } + _ => { + state.write_log_mut().drop_tx(); + + if tx.header.atomic { + // Stop the execution of an atomic batch at the + // first failed transaction + return Err(DispatchError { + error: Error::FailingAtomicBatch( + cmt.get_hash(), + ), + tx_result: Some(extended_tx_result), + }); + } } } } @@ -702,6 +706,9 @@ where H: 'static + StorageHasher + Sync, CA: 'static + WasmCacheAccess + Sync, { + const MASP_FEE_PAYMENT_ERROR: &str = + "The first transaction in the batch failed to pay fees via the MASP."; + // The fee payment is subject to a gas limit imposed by a protocol // parameter. Here we instantiate a custom gas meter for this step and // initialize it with the already consumed gas. The gas limit should @@ -745,54 +752,44 @@ where // cause we might need to discard the effects of this valid // unshield (e.g. if it unshield an amount which is not enough // to pay the fees) - if !result.is_accepted() { - state.write_log_mut().drop_tx(); - tracing::error!( - "The first transaction in the batch failed to pay \ - fees via the MASP, some VPs rejected it: {:#?}", - result.vps_result.rejected_vps - ); - } - - let actions = - state.read_actions().map_err(Error::StateError)?; + let is_masp_transfer = is_masp_transfer(&result.changed_keys); // Ensure that the transaction is actually a masp one, otherwise // reject - if is_masp_transfer(&result.changed_keys) - && result.is_accepted() - { - if let Some(masp_tx_id) = action::get_masp_section_ref( - &actions, - ) - .map_err(|msg| { - Error::StateError(state::Error::Temporary { - error: msg.to_string(), - }) - })? { - Some(MaspTxResult { + if is_masp_transfer && result.is_accepted() { + get_optional_masp_ref( + *state, + first_tx.cmt, + Either::Left(true), + )? + .map(|masp_section_ref| { + MaspTxResult { tx_result: result, - masp_section_ref: Either::Left(masp_tx_id), - }) - } else { - action::is_ibc_shielding_transfer(*state) - .map_err(Error::StateError)? - .then_some(MaspTxResult { - tx_result: result, - masp_section_ref: Either::Right( - *first_tx.cmt.data_sechash(), - ), - }) - } + masp_section_ref, + } + }) } else { + state.write_log_mut().drop_tx(); + + let mut error_msg = MASP_FEE_PAYMENT_ERROR.to_string(); + if !is_masp_transfer { + error_msg.push_str(" Not a MASP transaction."); + } + if !result.is_accepted() { + error_msg = format!( + "{error_msg} Some VPs rejected it: {:#?}", + result.vps_result.rejected_vps + ); + } + tracing::error!(error_msg); + None } } Err(e) => { state.write_log_mut().drop_tx(); tracing::error!( - "The first transaction in the batch failed to pay fees \ - via the MASP, wasm run failed: {}", + "{MASP_FEE_PAYMENT_ERROR} Wasm run failed: {}", e ); if let Error::GasError(_) = e { @@ -813,6 +810,46 @@ where Ok(valid_batched_tx_result) } +// Check that the transaction was a MASP one and extract the MASP tx reference +// (if any) in the same order that the MASP VP follows (IBC first, Actions +// second). The order is important to prevent malicious transactions from +// messing up with indexers/clients. Also a transaction can only be of one of +// the two types, not both at the same time (the MASP VP accepts a single +// Transaction) +fn get_optional_masp_ref>( + state: &S, + cmt: &TxCommitments, + is_masp_tx: Either, +) -> Result>> { + // Always check that the transaction was indeed a MASP one by looking at the + // changed keys. A malicious tx could push a MASP Action without touching + // any storage keys associated with the shielded pool + let is_masp_tx = match is_masp_tx { + Either::Left(res) => res, + Either::Right(tx_result) => is_masp_transfer(&tx_result.changed_keys), + }; + if !is_masp_tx { + return Ok(None); + } + + let masp_ref = if action::is_ibc_shielding_transfer(state) + .map_err(Error::StateError)? + { + Some(Either::Right(cmt.data_sechash().to_owned())) + } else { + let actions = state.read_actions().map_err(Error::StateError)?; + action::get_masp_section_ref(&actions) + .map_err(|msg| { + Error::StateError(state::Error::Temporary { + error: msg.to_string(), + }) + })? + .map(Either::Left) + }; + + Ok(masp_ref) +} + // Manage the token transfer for the fee payment. If an error is detected the // write log is dropped to prevent committing an inconsistent state. Propagates // the result to the caller diff --git a/crates/shielded_token/src/vp.rs b/crates/shielded_token/src/vp.rs index 87be0be0ffc..099e3c56f13 100644 --- a/crates/shielded_token/src/vp.rs +++ b/crates/shielded_token/src/vp.rs @@ -410,12 +410,13 @@ where .data(batched_tx.cmt) .ok_or_err_msg("No transaction data")?; let actions = self.ctx.read_actions()?; + // Try to get the Transaction object from the tx first (IBC) and from + // the actions afterwards let shielded_tx = if let Some(tx) = Ibc::try_extract_masp_tx_from_envelope::(&tx_data)? { tx } else { - // Get the Transaction object from the actions let masp_section_ref = namada_tx::action::get_masp_section_ref(&actions) .map_err(native_vp::Error::new_const)? @@ -424,6 +425,7 @@ where "Missing MASP section reference in action", ) })?; + batched_tx .tx .get_masp_section(&masp_section_ref) diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index b9f6854647c..e2f6d668485 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -2178,7 +2178,8 @@ fn dynamic_assets() -> Result<()> { // Test fee payment in masp: // // 1. Masp fee payment runs out of gas -// 2. Valid fee payment (also check that the first tx in the batch is executed +// 2. Attempt fee payment with a non-MASP transaction +// 3. Valid fee payment (also check that the first tx in the batch is executed // only once) #[test] fn masp_fee_payment() -> Result<()> { @@ -2232,6 +2233,8 @@ fn masp_fee_payment() -> Result<()> { NAM, "--amount", "500000", + "--gas-payer", + CHRISTEL_KEY, "--ledger-address", validator_one_rpc, ], @@ -2314,7 +2317,93 @@ fn masp_fee_payment() -> Result<()> { assert!(captured.result.is_ok()); assert!(captured.contains("nam: 500000")); - // 2. Valid masp fee payment + // 2. Attempt fee payment with non-MASP transfer + // Drain balance of Albert implicit + run( + &node, + Bin::Client, + vec![ + "transparent-transfer", + "--source", + ALBERT_KEY, + "--target", + BERTHA_KEY, + "--token", + NAM, + "--amount", + "1500000", + "--gas-payer", + CHRISTEL_KEY, + "--ledger-address", + validator_one_rpc, + ], + )?; + node.assert_success(); + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + ALBERT_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 0")); + + // Gas payer is Albert implicit, whose balance is 0. Let's try to + // transparently send some tokens (enough to pay fees) to him and check that + // this is not allowed + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "transparent-transfer", + "--source", + BERTHA_KEY, + "--target", + ALBERT_KEY, + "--token", + NAM, + "--amount", + "200000", + "--gas-payer", + ALBERT_KEY, + "--ledger-address", + validator_one_rpc, + // Force to skip check in client + "--force", + ], + ) + }); + assert!(captured.result.is_err()); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec![ + "balance", + "--owner", + ALBERT_KEY, + "--token", + NAM, + "--node", + validator_one_rpc, + ], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 0")); + + // 3. Valid masp fee payment run( &node, Bin::Client, @@ -2496,7 +2585,7 @@ fn masp_fee_payment_gas_limit() -> Result<()> { "--amount", "1", "--gas-limit", - "100000", + "300000", "--gas-price", "1", "--disposable-gas-payer",