Merge branch 'yuji/fix-ibc-shielding-transfer' (#3444)

* origin/yuji/fix-ibc-shielding-transfer:
  remove --refund
  extract_memo_from_packet
  fix refund source
  update Hermes
  IbcShieldingData
  add changelog
  add IbcShielding action
  workaround wasm compilation error
  fix gen_masp_tx in test
  fix e2e tests
  add CLI ibc-gen-shielding
  extract masp_tx in MASP VP
  memo for masp tx
  fix the port and channel for is_receiving_success
  fix tests
  fix convert_to_address
  add ibc trace file
  reduce assumptions for IBC VP
  refactoring: add trace.rs
  fix for apply_recv_msg
  support NftTransfer

Author: brentstone

.changelog/unreleased/bug-fixes/3438-fix-ibc-shielding-transfer.md

@@ -0,0 +1,2 @@
+- Fix IBC shielding transfer for the receiver not to be replaced by a malicious
+  relayer ([\#3438](https://github.com/anoma/namada/issues/3438))

.github/workflows/scripts/hermes.txt

@@ -1 +1 @@
-1.8.2-namada-beta12-rc6
+1.9.0-namada-beta13-rc2

Cargo.lock

@@ -298,6 +298,7 @@ pub mod cmds {
                 // Actions
                 .subcommand(SignTx::def().display_order(6))
                 .subcommand(ShieldedSync::def().display_order(6))
+                .subcommand(GenIbcShieldingTransfer::def().display_order(6))
                 // Utils
                 .subcommand(ClientUtils::def().display_order(7))
         }
@@ -386,6 +387,8 @@ pub mod cmds {
                 Self::parse_with_ctx(matches, AddToEthBridgePool);
             let sign_tx = Self::parse_with_ctx(matches, SignTx);
             let shielded_sync = Self::parse_with_ctx(matches, ShieldedSync);
+            let gen_ibc_shielding =
+                Self::parse_with_ctx(matches, GenIbcShieldingTransfer);
             let utils = SubCmd::parse(matches).map(Self::WithoutContext);
             tx_custom
                 .or(tx_transparent_transfer)
@@ -440,6 +443,7 @@ pub mod cmds {
                 .or(query_account)
                 .or(sign_tx)
                 .or(shielded_sync)
+                .or(gen_ibc_shielding)
                 .or(utils)
         }
     }
@@ -530,6 +534,7 @@ pub mod cmds {
         QueryRewards(QueryRewards),
         SignTx(SignTx),
         ShieldedSync(ShieldedSync),
+        GenIbcShieldingTransfer(GenIbcShieldingTransfer),
     }
 
     #[allow(clippy::large_enum_variant)]
@@ -2310,6 +2315,29 @@ pub mod cmds {
         }
     }
 
+    #[derive(Clone, Debug)]
+    pub struct GenIbcShieldingTransfer(
+        pub args::GenIbcShieldingTransfer<args::CliTypes>,
+    );
+
+    impl SubCmd for GenIbcShieldingTransfer {
+        const CMD: &'static str = "ibc-gen-shielding";
+
+        fn parse(matches: &ArgMatches) -> Option<Self> {
+            matches.subcommand_matches(Self::CMD).map(|matches| {
+                GenIbcShieldingTransfer(args::GenIbcShieldingTransfer::parse(
+                    matches,
+                ))
+            })
+        }
+
+        fn def() -> App {
+            App::new(Self::CMD)
+                .about("Generate shielding transfer for IBC.")
+                .add_args::<args::GenIbcShieldingTransfer<args::CliTypes>>()
+        }
+    }
+
     #[derive(Clone, Debug)]
     pub struct EpochSleep(pub args::Query<args::CliTypes>);
 
@@ -3373,7 +3401,6 @@ pub mod args {
     pub const RAW_PUBLIC_KEY_HASH_OPT: ArgOpt<String> =
         RAW_PUBLIC_KEY_HASH.opt();
     pub const RECEIVER: Arg<String> = arg("receiver");
-    pub const REFUND: ArgFlag = flag("refund");
     pub const REFUND_TARGET: ArgOpt<WalletTransferTarget> =
         arg_opt("refund-target");
     pub const RELAYER: Arg<Address> = arg("relayer");
@@ -6616,32 +6643,31 @@ pub mod args {
         }
     }
 
-    impl CliToSdk<GenIbcShieldedTransfer<SdkTypes>>
-        for GenIbcShieldedTransfer<CliTypes>
+    impl CliToSdk<GenIbcShieldingTransfer<SdkTypes>>
+        for GenIbcShieldingTransfer<CliTypes>
     {
         type Error = std::convert::Infallible;
 
         fn to_sdk(
             self,
             ctx: &mut Context,
-        ) -> Result<GenIbcShieldedTransfer<SdkTypes>, Self::Error> {
+        ) -> Result<GenIbcShieldingTransfer<SdkTypes>, Self::Error> {
             let query = self.query.to_sdk(ctx)?;
             let chain_ctx = ctx.borrow_chain_or_exit();
 
-            Ok(GenIbcShieldedTransfer::<SdkTypes> {
+            Ok(GenIbcShieldingTransfer::<SdkTypes> {
                 query,
                 output_folder: self.output_folder,
                 target: chain_ctx.get(&self.target),
                 token: self.token,
                 amount: self.amount,
                 port_id: self.port_id,
                 channel_id: self.channel_id,
-                refund: self.refund,
             })
         }
     }
 
-    impl Args for GenIbcShieldedTransfer<CliTypes> {
+    impl Args for GenIbcShieldingTransfer<CliTypes> {
         fn parse(matches: &ArgMatches) -> Self {
             let query = Query::parse(matches);
             let output_folder = OUTPUT_FOLDER_PATH.parse(matches);
@@ -6650,7 +6676,6 @@ pub mod args {
             let amount = InputAmount::Unvalidated(AMOUNT.parse(matches));
             let port_id = PORT_ID.parse(matches);
             let channel_id = CHANNEL_ID.parse(matches);
-            let refund = REFUND.parse(matches);
             Self {
                 query,
                 output_folder,
@@ -6659,7 +6684,6 @@ pub mod args {
                 amount,
                 port_id,
                 channel_id,
-                refund,
             }
         }
 
@@ -6681,9 +6705,6 @@ pub mod args {
                 .arg(CHANNEL_ID.def().help(wrap!(
                     "The channel ID via which the token is received."
                 )))
-                .arg(REFUND.def().help(wrap!(
-                    "Generate the shielded transfer for refunding."
-                )))
         }
     }
 

crates/apps_lib/src/cli.rs

@@ -370,6 +370,20 @@ impl CliApi {
                         )
                         .await?;
                     }
+                    Sub::GenIbcShieldingTransfer(GenIbcShieldingTransfer(
+                        args,
+                    )) => {
+                        let chain_ctx = ctx.borrow_mut_chain_or_exit();
+                        let ledger_address =
+                            chain_ctx.get(&args.query.ledger_address);
+                        let client = client.unwrap_or_else(|| {
+                            C::from_tendermint_address(&ledger_address)
+                        });
+                        client.wait_until_node_is_synced(&io).await?;
+                        let args = args.to_sdk(&mut ctx)?;
+                        let namada = ctx.to_sdk(client, io);
+                        tx::gen_ibc_shielding_transfer(&namada, args).await?;
+                    }
                     #[cfg(feature = "namada-eth-bridge")]
                     Sub::AddToEthBridgePool(args) => {
                         let args = args.0;

crates/apps_lib/src/cli/client.rs

@@ -11,9 +11,8 @@ use namada::core::chain::ChainId;
 use namada::core::ethereum_events::EthAddress;
 use namada::core::key::*;
 use namada::core::masp::*;
-use namada::ibc::{is_ibc_denom, is_nft_trace};
+use namada::ibc::trace::{ibc_token, is_ibc_denom, is_nft_trace};
 use namada::io::Io;
-use namada::ledger::ibc::storage::ibc_token;
 use namada_sdk::masp::fs::FsShieldedUtils;
 use namada_sdk::masp::ShieldedContext;
 use namada_sdk::wallet::Wallet;

crates/apps_lib/src/cli/context.rs

@@ -1,4 +1,5 @@
 use std::fs::File;
+use std::io::Write;
 
 use borsh::BorshDeserialize;
 use borsh_ext::BorshSerializeExt;
@@ -11,6 +12,7 @@ use namada::core::key::*;
 use namada::governance::cli::onchain::{
     DefaultProposal, PgfFundingProposal, PgfStewardProposal,
 };
+use namada::ibc::convert_masp_tx_to_ibc_memo;
 use namada::io::Io;
 use namada::state::EPOCH_SWITCH_BLOCKS_DELAY;
 use namada::tx::data::compute_inner_tx_hash;
@@ -1395,3 +1397,32 @@ pub async fn submit_tx(
 ) -> Result<TxResponse, error::Error> {
     tx::submit_tx(namada, to_broadcast).await
 }
+
+/// Generate MASP transaction and output it
+pub async fn gen_ibc_shielding_transfer(
+    context: &impl Namada,
+    args: args::GenIbcShieldingTransfer,
+) -> Result<(), error::Error> {
+    if let Some(masp_tx) =
+        tx::gen_ibc_shielding_transfer(context, args.clone()).await?
+    {
+        let tx_id = masp_tx.txid().to_string();
+        let filename = format!("ibc_masp_tx_{}.memo", tx_id);
+        let output_path = match &args.output_folder {
+            Some(path) => path.join(filename),
+            None => filename.into(),
+        };
+        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);
+        out.write_all(bytes.as_bytes())
+            .expect("Writing IBC MASP transaction file failed.");
+        println!(
+            "Output IBC shielding transfer for {tx_id} to {}",
+            output_path.to_string_lossy()
+        );
+    } else {
+        eprintln!("No shielded transfer for this IBC transfer.")
+    }
+    Ok(())
+}

crates/apps_lib/src/client/tx.rs

@@ -32,6 +32,7 @@ namada_storage = { path = "../storage" }
 namada_token = { path = "../token" }
 
 borsh.workspace = true
+data-encoding.workspace = true
 konst.workspace = true
 linkme = {workspace = true, optional = true}
 ibc.workspace = true
@@ -45,6 +46,7 @@ prost.workspace = true
 serde.workspace = true
 serde_json.workspace = true
 sha2.workspace = true
+smooth-operator.workspace = true
 thiserror.workspace = true
 tracing.workspace = true
 

crates/ibc/Cargo.toml

@@ -9,7 +9,6 @@ use ibc::core::channel::types::commitment::{
 };
 use ibc::core::channel::types::error::{ChannelError, PacketError};
 use ibc::core::channel::types::packet::Receipt;
-use ibc::core::channel::types::timeout::TimeoutHeight;
 use ibc::core::client::types::error::ClientError;
 use ibc::core::client::types::Height;
 use ibc::core::connection::types::error::ConnectionError;
@@ -23,14 +22,14 @@ use ibc::primitives::Timestamp;
 use namada_core::address::Address;
 use namada_core::storage::{BlockHeight, Key};
 use namada_core::tendermint::Time as TmTime;
+use namada_storage::{Error as StorageError, StorageRead};
 use namada_token::storage_key::balance_key;
 use namada_token::Amount;
 use prost::Message;
-use sha2::Digest;
 
 use super::client::{AnyClientState, AnyConsensusState};
 use super::storage::IbcStorageContext;
-use crate::{storage, NftClass, NftMetadata};
+use crate::{storage, trace, NftClass, NftMetadata};
 
 /// Result of IBC common function call
 pub type Result<T> = std::result::Result<T, ContextError>;
@@ -408,7 +407,7 @@ pub trait IbcCommonContext: IbcStorageContext {
         channel_id: &ChannelId,
     ) -> Result<Sequence> {
         let key = storage::next_sequence_send_key(port_id, channel_id);
-        self.read_sequence(&key)
+        read_sequence(self, &key).map_err(ContextError::from)
     }
 
     /// Store the NextSequenceSend
@@ -429,7 +428,7 @@ pub trait IbcCommonContext: IbcStorageContext {
         channel_id: &ChannelId,
     ) -> Result<Sequence> {
         let key = storage::next_sequence_recv_key(port_id, channel_id);
-        self.read_sequence(&key)
+        read_sequence(self, &key).map_err(ContextError::from)
     }
 
     /// Store the NextSequenceRecv
@@ -450,7 +449,7 @@ pub trait IbcCommonContext: IbcStorageContext {
         channel_id: &ChannelId,
     ) -> Result<Sequence> {
         let key = storage::next_sequence_ack_key(port_id, channel_id);
-        self.read_sequence(&key)
+        read_sequence(self, &key).map_err(ContextError::from)
     }
 
     /// Store the NextSequenceAck
@@ -464,57 +463,12 @@ pub trait IbcCommonContext: IbcStorageContext {
         self.store_sequence(&key, seq)
     }
 
-    /// Read a sequence
-    fn read_sequence(&self, key: &Key) -> Result<Sequence> {
-        match self.read_bytes(key)? {
-            Some(value) => {
-                let value: [u8; 8] =
-                    value.try_into().map_err(|_| ChannelError::Other {
-                        description: format!(
-                            "The sequence value wasn't u64: Key {key}",
-                        ),
-                    })?;
-                Ok(u64::from_be_bytes(value).into())
-            }
-            // when the sequence has never been used, returns the initial value
-            None => Ok(1.into()),
-        }
-    }
-
     /// Store the sequence
     fn store_sequence(&mut self, key: &Key, sequence: Sequence) -> Result<()> {
         let bytes = u64::from(sequence).to_be_bytes().to_vec();
         self.write_bytes(key, bytes).map_err(ContextError::from)
     }
 
-    /// Calculate the hash
-    fn hash(value: &[u8]) -> Vec<u8> {
-        sha2::Sha256::digest(value).to_vec()
-    }
-
-    /// Calculate the packet commitment
-    fn compute_packet_commitment(
-        packet_data: &[u8],
-        timeout_height: &TimeoutHeight,
-        timeout_timestamp: &Timestamp,
-    ) -> PacketCommitment {
-        let mut hash_input =
-            timeout_timestamp.nanoseconds().to_be_bytes().to_vec();
-
-        let revision_number =
-            timeout_height.commitment_revision_number().to_be_bytes();
-        hash_input.append(&mut revision_number.to_vec());
-
-        let revision_height =
-            timeout_height.commitment_revision_height().to_be_bytes();
-        hash_input.append(&mut revision_height.to_vec());
-
-        let packet_data_hash = Self::hash(packet_data);
-        hash_input.append(&mut packet_data_hash.to_vec());
-
-        Self::hash(&hash_input).into()
-    }
-
     /// Get the packet commitment
     fn packet_commitment(
         &self,
@@ -703,7 +657,7 @@ pub trait IbcCommonContext: IbcStorageContext {
         token_id: &TokenId,
         owner: &Address,
     ) -> Result<bool> {
-        let ibc_token = storage::ibc_token_for_nft(class_id, token_id);
+        let ibc_token = trace::ibc_token_for_nft(class_id, token_id);
         let balance_key = balance_key(&ibc_token, owner);
         let amount = self.read::<Amount>(&balance_key)?;
         Ok(amount == Some(Amount::from_u64(1)))
@@ -753,3 +707,22 @@ pub trait IbcCommonContext: IbcStorageContext {
         self.write(&key, amount).map_err(ContextError::from)
     }
 }
+
+/// Read and decode the IBC sequence
+pub fn read_sequence<S: StorageRead + ?Sized>(
+    storage: &S,
+    key: &Key,
+) -> std::result::Result<Sequence, StorageError> {
+    match storage.read_bytes(key)? {
+        Some(value) => {
+            let value: [u8; 8] = value.try_into().map_err(|_| {
+                StorageError::new_alloc(format!(
+                    "The sequence value wasn't u64: Key {key}",
+                ))
+            })?;
+            Ok(u64::from_be_bytes(value).into())
+        }
+        // when the sequence has never been used, returns the initial value
+        None => Ok(1.into()),
+    }
+}