Merge pull request #3669 from anoma/grarco/fix-masp-ident

Fix masp events

.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))

crates/apps_lib/src/cli.rs

@@ -7116,8 +7116,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."

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),
+                            });
+                        }
                     }
                 }
             }
@@ -711,6 +715,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
@@ -754,54 +761,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 {
@@ -822,6 +819,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<S: Read<Err = state::Error>>(
+    state: &S,
+    cmt: &TxCommitments,
+    is_masp_tx: Either<bool, &BatchedTxResult>,
+) -> Result<Option<Either<namada_sdk::masp::MaspTxId, Hash>>> {
+    // 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

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::<Transfer>(&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)

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",