Validate flag ciphertexts in MASP VP

Author: sug0

crates/apps_lib/src/client/tx.rs

@@ -1935,7 +1935,8 @@ pub async fn gen_ibc_shielding_transfer(
 ) -> Result<(), error::Error> {
     let output_folder = args.output_folder.clone();
 
-    if let Some(masp_tx) = tx::gen_ibc_shielding_transfer(context, args).await?
+    if let Some((masp_tx, fmd_flags)) =
+        tx::gen_ibc_shielding_transfer(context, args).await?
     {
         let tx_id = masp_tx.txid().to_string();
         let filename = format!("ibc_masp_tx_{}.memo", tx_id);
@@ -1945,11 +1946,7 @@ pub async fn gen_ibc_shielding_transfer(
         };
         let mut out = File::create(&output_path)
             .expect("Creating a new file for IBC MASP transaction failed.");
-        let bytes = convert_masp_tx_to_ibc_memo(
-            masp_tx,
-            // TODO: add actual flag ciphertext
-            Default::default(),
-        );
+        let bytes = convert_masp_tx_to_ibc_memo(masp_tx, fmd_flags);
         out.write_all(bytes.as_bytes())
             .expect("Writing IBC MASP transaction file failed.");
         println!(

crates/core/src/masp.rs

@@ -1017,6 +1017,22 @@ pub enum UnifiedPaymentAddress {
 }
 
 impl UnifiedPaymentAddress {
+    /// Generate a [`FlagCiphertext`] from this payment address.
+    ///
+    /// Returns [`None`] if the FMD public key in the
+    /// [`UnifiedPaymentAddress::V1`] variant is invalid, and a random flag
+    /// ciphertext for the [`UnifiedPaymentAddress::V0`] variant.
+    #[cfg(feature = "rand")]
+    pub fn flag<R>(&self, rng: &mut R) -> Option<FlagCiphertext>
+    where
+        R: rand_core::CryptoRng + rand_core::RngCore,
+    {
+        match self {
+            Self::V0(_) => Some(FlagCiphertext::random(rng)),
+            Self::V1(pa) => pa.to_fmd_public_key().map(|pk| pk.flag(rng)),
+        }
+    }
+
     /// Create a v0 payment address.
     pub fn v0_from_zip32(
         vk: ExtendedViewingKey,

crates/ibc/src/lib.rs

@@ -92,7 +92,7 @@ use namada_core::ibc::core::channel::types::commitment::{
     AcknowledgementCommitment, PacketCommitment, compute_packet_commitment,
 };
 pub use namada_core::ibc::*;
-use namada_core::masp::{TAddrData, addr_taddr, ibc_taddr};
+use namada_core::masp::{FlagCiphertext, TAddrData, addr_taddr, ibc_taddr};
 use namada_core::masp_primitives::transaction::components::ValueSum;
 use namada_core::token::Amount;
 use namada_events::EmitEvents;
@@ -237,18 +237,19 @@ impl<S> namada_systems::ibc::Read<S> for Store<S>
 where
     S: StorageRead,
 {
-    fn try_extract_masp_tx_from_envelope<Transfer: BorshDeserialize>(
+    fn try_extract_shielding_data_from_envelope<Transfer: BorshDeserialize>(
         tx_data: &[u8],
-    ) -> StorageResult<Option<masp_primitives::transaction::Transaction>> {
+    ) -> StorageResult<Option<(MaspTransaction, Vec<FlagCiphertext>)>> {
         let msg = decode_message::<Transfer>(tx_data)
             .into_storage_result()
             .ok();
         let tx = if let Some(IbcMessage::Envelope(ref envelope)) = msg {
-            Some(extract_masp_tx_from_envelope(envelope).ok_or_else(|| {
-                StorageError::new_const(
-                    "Missing MASP transaction in IBC message",
-                )
-            })?)
+            let shielding_data = extract_shielding_data_from_envelope(envelope)
+                .ok_or(StorageError::new_const(
+                    "Missing MASP shielding data in IBC message",
+                ))?;
+
+            Some((shielding_data.masp_tx, shielding_data.flag_ciphertexts))
         } else {
             None
         };
@@ -578,7 +579,7 @@ pub struct InternalData<Transfer> {
     /// The transparent transfer that happens in parallel to IBC processes
     pub transparent: Option<Transfer>,
     /// The shielded transaction that happens in parallel to IBC processes
-    pub shielded: Option<MaspTransaction>,
+    pub shielded: Option<IbcShieldingData>,
     /// IBC tokens that are credited/debited to internal accounts
     pub ibc_tokens: BTreeSet<Address>,
 }
@@ -760,13 +761,16 @@ where
                             .map_err(Error::Storage)?;
                             tokens.insert(token);
                         }
-                        (extract_masp_tx_from_packet(&msg.packet), tokens)
+                        (
+                            extract_shielding_data_from_packet(&msg.packet),
+                            tokens,
+                        )
                     }
                     #[cfg(is_apple_silicon)]
                     MsgEnvelope::Packet(PacketMsg::Ack(msg)) => {
                         // NOTE: This is unneeded but wasm compilation error
                         // happened if deleted on macOS with Apple Silicon
-                        let _ = extract_masp_tx_from_packet(&msg.packet);
+                        let _ = extract_shielding_data_from_packet(&msg.packet);
                         (None, BTreeSet::new())
                     }
                     _ => (None, BTreeSet::new()),

crates/ibc/src/msg.rs

@@ -242,8 +242,8 @@ impl<Transfer: BorshSchema> BorshSchema for MsgNftTransfer<Transfer> {
 pub struct IbcShieldingData {
     /// MASP transaction forwarded over IBC.
     pub masp_tx: MaspTransaction,
-    /// Flag ciphertext to signal the owner of the new note(s).
-    pub flag_ciphertext: FlagCiphertext,
+    /// Flag ciphertexts to signal the owner(s) of the new note(s).
+    pub flag_ciphertexts: Vec<FlagCiphertext>,
 }
 
 impl From<&IbcShieldingData> for String {
@@ -279,9 +279,17 @@ impl FromStr for IbcShieldingData {
 pub fn extract_masp_tx_from_envelope(
     envelope: &MsgEnvelope,
 ) -> Option<MaspTransaction> {
+    extract_shielding_data_from_envelope(envelope)
+        .map(|IbcShieldingData { masp_tx, .. }| masp_tx)
+}
+
+/// Extract IBC shielding data from IBC envelope
+pub fn extract_shielding_data_from_envelope(
+    envelope: &MsgEnvelope,
+) -> Option<IbcShieldingData> {
     match envelope {
         MsgEnvelope::Packet(PacketMsg::Recv(msg)) => {
-            extract_masp_tx_from_packet(&msg.packet)
+            extract_shielding_data_from_packet(&msg.packet)
         }
         _ => None,
     }
@@ -306,10 +314,17 @@ pub fn decode_ibc_shielding_data(
 
 /// Extract MASP transaction from IBC packet memo
 pub fn extract_masp_tx_from_packet(packet: &Packet) -> Option<MaspTransaction> {
+    extract_shielding_data_from_packet(packet)
+        .map(|IbcShieldingData { masp_tx, .. }| masp_tx)
+}
+
+/// Extract IBC shielding data from IBC packet memo
+pub fn extract_shielding_data_from_packet(
+    packet: &Packet,
+) -> Option<IbcShieldingData> {
     let memo = extract_memo_from_packet(packet, &packet.port_id_on_b)?;
 
     decode_ibc_shielding_data(memo)
-        .map(|IbcShieldingData { masp_tx, .. }| masp_tx)
 }
 
 fn extract_memo_from_packet(
@@ -376,11 +391,11 @@ pub fn extract_traces_from_recv_msg(
 /// Get IBC memo string from MASP transaction for receiving
 pub fn convert_masp_tx_to_ibc_memo(
     masp_tx: MaspTransaction,
-    flag_ciphertext: FlagCiphertext,
+    flag_ciphertexts: Vec<FlagCiphertext>,
 ) -> String {
     IbcShieldingData {
         masp_tx,
-        flag_ciphertext,
+        flag_ciphertexts,
     }
     .into()
 }

crates/sdk/src/args.rs

@@ -718,7 +718,7 @@ impl TxOsmosisSwap<SdkTypes> {
                     ),
                 };
 
-                let shielding_tx = tx::gen_ibc_shielding_transfer(
+                let (shielding_tx, fmd_flags) = tx::gen_ibc_shielding_transfer(
                     ctx,
                     GenIbcShieldingTransfer {
                         query: Query {
@@ -754,8 +754,7 @@ impl TxOsmosisSwap<SdkTypes> {
                             shielding_data: StringEncoded::new(
                                 IbcShieldingData {
                                     masp_tx: shielding_tx,
-                                    // TODO: add actual flag ciphertext here
-                                    flag_ciphertext: Default::default(),
+                                    flag_ciphertexts: fmd_flags,
                                 },
                             ),
                             shielded_amount: amount_to_shield,

crates/sdk/src/tx.rs

@@ -3981,7 +3981,7 @@ pub async fn build_custom(
 pub async fn gen_ibc_shielding_transfer<N: Namada>(
     context: &N,
     args: args::GenIbcShieldingTransfer,
-) -> Result<Option<MaspTransaction>> {
+) -> Result<Option<(MaspTransaction, Vec<FlagCiphertext>)>> {
     let source = IBC;
 
     let token = match args.asset {
@@ -4043,7 +4043,7 @@ pub async fn gen_ibc_shielding_transfer<N: Namada>(
             .map_err(|err| TxSubmitError::MaspError(err.to_string()))?
     };
 
-    Ok(shielded_transfer.map(|st| st.masp_tx))
+    Ok(shielded_transfer.map(|st| (st.masp_tx, st.fmd_flags)))
 }
 
 pub(crate) async fn get_ibc_src_port_channel(

crates/shielded_token/src/masp/shielded_wallet.rs

@@ -32,7 +32,7 @@ use namada_core::collections::{HashMap, HashSet};
 use namada_core::control_flow;
 use namada_core::masp::{
     AssetData, FlagCiphertext, FmdSecretKey, MaspEpoch, TransferSource,
-    TransferTarget, UnifiedPaymentAddress, encode_asset_type,
+    TransferTarget, encode_asset_type,
 };
 use namada_core::task_env::TaskEnvironment;
 use namada_core::time::{DateTimeUtc, DurationSecs};
@@ -1775,17 +1775,9 @@ pub trait ShieldedApi<U: ShieldedUtils + MaybeSend + MaybeSync>:
                             error: builder::Error::SaplingBuild(e),
                         })?;
 
-                    fmd_flags.push({
-                        match pa {
-                            UnifiedPaymentAddress::V0(_) => {
-                                FlagCiphertext::random(rng)
-                            }
-                            UnifiedPaymentAddress::V1(pa) => pa
-                                .to_fmd_public_key()
-                                .ok_or(TransferErr::InvalidFmdPublicKey)?
-                                .flag(rng),
-                        }
-                    });
+                    fmd_flags.push(
+                        pa.flag(rng).ok_or(TransferErr::InvalidFmdPublicKey)?,
+                    );
                 } else if let Some(t_addr_data) = target.t_addr_data() {
                     // If there is a transparent output
                     builder

crates/shielded_token/src/vp.rs

@@ -17,7 +17,9 @@ use namada_core::address::{self, Address};
 use namada_core::arith::{CheckedAdd, CheckedSub, checked};
 use namada_core::booleans::BoolResultUnitExt;
 use namada_core::collections::HashSet;
-use namada_core::masp::{MaspEpoch, TAddrData, addr_taddr, encode_asset_type};
+use namada_core::masp::{
+    FlagCiphertext, MaspEpoch, TAddrData, addr_taddr, encode_asset_type,
+};
 use namada_core::storage::Key;
 use namada_core::token;
 use namada_core::token::{Amount, MaspDigitPos};
@@ -435,12 +437,13 @@ where
             .data(batched_tx.cmt)
             .ok_or_err_msg("No transaction data")?;
         let actions = 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)?
+
+        // Try to get the Transaction object and FMD flag ciphertexts
+        // from the tx first (IBC) and from the actions afterwards
+        let (shielded_tx, fmd_flags) = if let Some(shielding_data) =
+            Ibc::try_extract_shielding_data_from_envelope::<Transfer>(&tx_data)?
         {
-            tx
+            shielding_data
         } else {
             let masp_section_ref =
                 namada_tx::action::get_masp_section_ref(&actions)
@@ -450,14 +453,33 @@ where
                             "Missing MASP section reference in action",
                         )
                     })?;
+            let flag_ciphertexts_ref =
+                namada_tx::action::get_fmd_flag_ciphertexts_ref(&actions)
+                    .map_err(Error::new_const)?
+                    .ok_or_else(|| {
+                        Error::new_const(
+                            "Missing FMD flag ciphertexts reference in action",
+                        )
+                    })?;
 
-            batched_tx
+            let masp_tx = batched_tx
                 .tx
                 .get_masp_section(&masp_section_ref)
                 .cloned()
                 .ok_or_else(|| {
                     Error::new_const("Missing MASP section in transaction")
-                })?
+                })?;
+            let fmd_flags = batched_tx
+                .tx
+                .get_fmd_flag_ciphertexts(&flag_ciphertexts_ref)
+                .map_err(Error::new)?
+                .ok_or_else(|| {
+                    Error::new_const(
+                        "Missing FMD flag ciphertexts in transaction",
+                    )
+                })?;
+
+            (masp_tx, fmd_flags)
         };
 
         if u64::from(ctx.get_block_height()?)
@@ -468,6 +490,8 @@ where
             return Err(error);
         }
 
+        validate_flag_ciphertexts(&shielded_tx, fmd_flags)?;
+
         // Check the validity of the keys and get the transfer data
         let changed_balances = Self::validate_state_and_get_transfer_data(
             ctx,
@@ -949,6 +973,39 @@ fn verify_sapling_balancing_value(
     }
 }
 
+/// Check if the flag ciphertexts included in the tx are valid.
+fn validate_flag_ciphertexts(
+    masp_tx: &Transaction,
+    fmd_flags: Vec<FlagCiphertext>,
+) -> Result<()> {
+    let shielded_outputs_len = masp_tx
+        .sapling_bundle()
+        .map_or(0, |bundle| bundle.shielded_outputs.len());
+
+    if shielded_outputs_len != fmd_flags.len() {
+        let error = Error::new(format!(
+            "The number of shielded outputs in the MASP tx ({}) does not \
+             match the number of FMD flag ciphertexts ({})",
+            shielded_outputs_len,
+            fmd_flags.len()
+        ));
+        tracing::debug!("{error}");
+        return Err(error);
+    }
+
+    fmd_flags
+        .iter()
+        .all(FlagCiphertext::is_valid)
+        .ok_or_else(|| {
+            let error = Error::new_const(
+                "Not all FMD flag ciphertexts in the MASP tx were considered \
+                 valid, either because of invalid gamma or tampered bits",
+            );
+            tracing::debug!("{error}");
+            error
+        })
+}
+
 #[cfg(test)]
 mod shielded_token_tests {
     use std::cell::RefCell;

crates/systems/src/ibc.rs

@@ -6,16 +6,21 @@ use masp_primitives::transaction::TransparentAddress;
 use masp_primitives::transaction::components::ValueSum;
 use namada_core::address::Address;
 use namada_core::borsh::BorshDeserialize;
-use namada_core::masp::TAddrData;
+use namada_core::masp::{FlagCiphertext, TAddrData};
 use namada_core::{masp_primitives, storage, token};
 pub use namada_storage::Result;
 
 /// Abstract IBC storage read interface
 pub trait Read<S> {
-    /// Extract MASP transaction from IBC envelope
-    fn try_extract_masp_tx_from_envelope<Transfer: BorshDeserialize>(
+    /// Extract shielding data from IBC envelope
+    fn try_extract_shielding_data_from_envelope<Transfer: BorshDeserialize>(
         tx_data: &[u8],
-    ) -> Result<Option<masp_primitives::transaction::Transaction>>;
+    ) -> Result<
+        Option<(
+            masp_primitives::transaction::Transaction,
+            Vec<FlagCiphertext>,
+        )>,
+    >;
 
     /// Apply relevant IBC packets to the changed balances structure
     fn apply_ibc_packet<Transfer: BorshDeserialize>(