Merge branch 'grarco/fix-shielded-actions' (#2428)

* origin/grarco/fix-shielded-actions:
  Changelog #2428
  Check balance key change in masp vp
  Adds shielded commit to transfer in `gen_ibc_shielded_transfer`

.changelog/unreleased/bug-fixes/2428-fix-shielded-actions.md

@@ -0,0 +1,2 @@
+- Validates changes to the balance key in masp vp.
+  ([\#2428](https://github.com/anoma/namada/pull/2428))

crates/namada/src/ledger/native_vp/masp.rs

@@ -23,10 +23,11 @@ use ripemd::Digest as RipemdDigest;
 use sha2::Digest as Sha2Digest;
 use thiserror::Error;
 use token::storage_key::{
-    is_masp_allowed_key, is_masp_key, is_masp_nullifier_key,
+    balance_key, is_masp_allowed_key, is_masp_key, is_masp_nullifier_key,
     masp_commitment_anchor_key, masp_commitment_tree_key,
     masp_convert_anchor_key, masp_nullifier_key, masp_pin_tx_key,
 };
+use token::Amount;
 
 use crate::ledger::native_vp;
 use crate::ledger::native_vp::{Ctx, NativeVp};
@@ -234,7 +235,8 @@ where
     fn valid_state(
         &self,
         masp_keys_changed: &[&Key],
-        pin_key: Option<&str>,
+        transfer: &token::Transfer,
+        transfer_amount: Amount,
     ) -> Result<bool> {
         // Check that the transaction didn't write unallowed masp keys
         if masp_keys_changed
@@ -244,8 +246,55 @@ where
             return Ok(false);
         }
 
+        // Check that the declared amount in the transaction matches the actual
+        // change in storage
+        let pre_masp_balance: Amount = self
+            .ctx
+            .read_pre(&balance_key(&transfer.token, &Address::Internal(Masp)))?
+            .unwrap_or_default();
+        let post_masp_balance: Amount = self
+            .ctx
+            .read_post(&balance_key(&transfer.token, &Address::Internal(Masp)))?
+            .unwrap_or_default();
+
+        match (&transfer.source, &transfer.target) {
+            (&Address::Internal(Masp), &Address::Internal(Masp)) => {
+                // For shileded operations the amount must be redacted to 0
+                if post_masp_balance != pre_masp_balance {
+                    return Ok(false);
+                }
+            }
+            (&Address::Internal(Masp), _) => {
+                // Unshielding
+                if pre_masp_balance.checked_sub(post_masp_balance).ok_or_else(
+                    || {
+                        Error::NativeVpError(native_vp::Error::SimpleMessage(
+                            "Underflowed in balance check",
+                        ))
+                    },
+                )? != transfer_amount
+                {
+                    return Ok(false);
+                }
+            }
+            (_, &Address::Internal(Masp)) => {
+                // Shielding
+                if post_masp_balance.checked_sub(pre_masp_balance).ok_or_else(
+                    || {
+                        Error::NativeVpError(native_vp::Error::SimpleMessage(
+                            "Underflowed in balance check",
+                        ))
+                    },
+                )? != transfer_amount
+                {
+                    return Ok(false);
+                }
+            }
+            _ => return Ok(false),
+        }
+
         // Validate pin key
-        if let Some(key) = pin_key {
+        if let Some(ref key) = transfer.key {
             match self.ctx.read_post::<IndexedTx>(&masp_pin_tx_key(key))? {
                 Some(IndexedTx { height, index })
                     if height == self.ctx.get_block_height()?
@@ -316,7 +365,8 @@ where
             keys_changed.iter().filter(|key| is_masp_key(key)).collect();
         if !self.valid_state(
             masp_keys_changed.as_slice(),
-            transfer.key.as_deref(),
+            &transfer,
+            transfer_amount,
         )? {
             return Ok(false);
         }

crates/sdk/src/tx.rs

@@ -2337,8 +2337,7 @@ pub async fn build_transfer<N: Namada>(
 
     let masp_addr = MASP;
 
-    // For MASP sources, use a special sentinel key recognized by VPs as default
-    // signer. Also, if the transaction is shielded, redact the amount and token
+    // If the transaction is shielded, redact the amount and token
     // types by setting the transparent value to 0 and token type to a constant.
     // This has no side-effect because transaction is to self.
     let (transparent_amount, transparent_token) =
@@ -2711,7 +2710,7 @@ pub async fn gen_ibc_shielded_transfer<N: Namada>(
         .await
         .map_err(|err| TxSubmitError::MaspError(err.to_string()))?;
 
-    let transfer = token::Transfer {
+    let mut transfer = token::Transfer {
         source: source.clone(),
         target: MASP,
         token: token.clone(),
@@ -2720,6 +2719,8 @@ pub async fn gen_ibc_shielded_transfer<N: Namada>(
         shielded: None,
     };
     if let Some(shielded_transfer) = shielded_transfer {
+        transfer.shielded =
+            Some(Section::MaspTx(shielded_transfer.masp_tx.clone()).get_hash());
         Ok(Some(IbcShieldedTransfer {
             transfer,
             masp_tx: shielded_transfer.masp_tx,