diff --git a/.changelog/unreleased/bug-fixes/2428-fix-shielded-actions.md b/.changelog/unreleased/bug-fixes/2428-fix-shielded-actions.md new file mode 100644 index 0000000000..362819183c --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/crates/namada/src/ledger/native_vp/masp.rs b/crates/namada/src/ledger/native_vp/masp.rs index 9894f0e5eb..d64b425d39 100644 --- a/crates/namada/src/ledger/native_vp/masp.rs +++ b/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 { // 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::(&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); } diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 7574897f24..b9913c9983 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -2337,8 +2337,7 @@ pub async fn build_transfer( 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( .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( 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,