Merge branch 'yuji/validate-sent-coin' (#4733)

* origin/yuji/validate-sent-coin:
  add test
  changelog
  validate the base denom in MsgTransfer

Author: tzemanovic

.changelog/unreleased/bug-fixes/4733-validate-sent-coin.md

@@ -0,0 +1,2 @@
+- Validate the base denom in MsgTransfer
+  ([\#4733](https://github.com/anoma/namada/issues/4733))

crates/ibc/src/context/token_transfer.rs

@@ -55,6 +55,22 @@ where
         self.is_shielded = true;
     }
 
+    fn validate_sent_coin(&self, coin: &PrefixedCoin) -> Result<(), HostError> {
+        // The base denom should not be an IBC token address because an IBC
+        // token address has been already encoded and other chains can't extract
+        // the trace paths
+        match Address::decode(coin.denom.base_denom.as_str()) {
+            Ok(Address::Internal(InternalAddress::IbcToken(_))) => {
+                Err(HostError::Other {
+                    description: "The base denom should not be an IBC token \
+                                  address"
+                        .to_string(),
+                })
+            }
+            _ => Ok(()),
+        }
+    }
+
     /// Get the token address and the amount from PrefixedCoin. If the base
     /// denom is not an address, it returns `IbcToken`
     fn get_token_amount(
@@ -201,9 +217,11 @@ where
         _from_account: &Self::AccountId,
         _port_id: &PortId,
         _channel_id: &ChannelId,
-        _coin: &PrefixedCoin,
+        coin: &PrefixedCoin,
         _memo: &Memo,
     ) -> Result<(), HostError> {
+        self.validate_sent_coin(coin)?;
+
         // validated by Multitoken VP
         Ok(())
     }
@@ -231,9 +249,11 @@ where
     fn burn_coins_validate(
         &self,
         _account: &Self::AccountId,
-        _coin: &PrefixedCoin,
+        coin: &PrefixedCoin,
         _memo: &Memo,
     ) -> Result<(), HostError> {
+        self.validate_sent_coin(coin)?;
+
         // validated by Multitoken VP
         Ok(())
     }

crates/ibc/src/vp/mod.rs

@@ -476,7 +476,9 @@ mod tests {
     use std::str::FromStr;
 
     use assert_matches::assert_matches;
+    use ibc::apps::transfer::types::error::TokenTransferError;
     use ibc::core::channel::types::timeout::TimeoutTimestamp;
+    use ibc::core::host::types::error::HostError;
     use ibc::primitives::IntoTimestamp;
     use ibc_testkit::testapp::ibc::clients::mock::client_state::{
         MOCK_CLIENT_TYPE, MockClientState, client_type,
@@ -485,7 +487,7 @@ mod tests {
     use ibc_testkit::testapp::ibc::clients::mock::header::MockHeader;
     use namada_core::address::InternalAddress;
     use namada_core::address::testing::{
-        established_address_1, established_address_2, nam,
+        apfel, established_address_1, established_address_2, nam,
     };
     use namada_core::borsh::{BorshDeserialize, BorshSerializeExt};
     use namada_core::chain::testing::get_dummy_header;
@@ -3507,4 +3509,177 @@ mod tests {
             Ok(_)
         );
     }
+
+    #[test]
+    fn test_send_invalid_base_denom() {
+        let mut keys_changed = BTreeSet::new();
+        let mut state = init_storage();
+        insert_init_client(&mut state);
+
+        // insert an open connection
+        let conn_key = connection_key(&get_connection_id());
+        let conn = get_connection(ConnState::Open);
+        let bytes = conn.encode_vec();
+        let _ = state
+            .write_log_mut()
+            .write(&conn_key, bytes)
+            .expect("write failed");
+        // insert an Open channel
+        let channel_key = channel_key(&get_port_id(), &get_channel_id());
+        let channel = get_channel(ChanState::Open, Order::Unordered);
+        let bytes = channel.encode_vec();
+        let _ = state
+            .write_log_mut()
+            .write(&channel_key, bytes)
+            .expect("write failed");
+        // init balance
+        let sender = established_address_1();
+        let ibc_token = ibc_token(format!(
+            "{}/{}/{}",
+            get_port_id(),
+            get_channel_id(),
+            apfel()
+        ));
+        let balance_key = balance_key(&ibc_token, &sender);
+        let amount = Amount::from_u64(100);
+        let _ = state
+            .write_log_mut()
+            .write(&balance_key, amount.serialize_to_vec())
+            .expect("write failed");
+        state.write_log_mut().commit_batch_and_current_tx();
+        state.commit_block().expect("commit failed");
+        // for next block
+        state
+            .in_mem_mut()
+            .set_header(get_dummy_header())
+            .expect("Setting a dummy header shouldn't fail");
+        state.in_mem_mut().begin_block(BlockHeight(2)).unwrap();
+
+        // prepare data with an invalid message
+        let msg = IbcMsgTransfer {
+            port_id_on_a: get_port_id(),
+            chan_id_on_a: get_channel_id(),
+            packet_data: PacketData {
+                token: PrefixedCoin {
+                    // Invalid denom with IbcToken address
+                    denom: ibc_token.to_string().parse().unwrap(),
+                    amount: amount.into(),
+                },
+                sender: sender.to_string().into(),
+                receiver: "receiver".to_string().into(),
+                memo: "memo".to_string().into(),
+            },
+            timeout_height_on_b: TimeoutHeight::At(Height::new(0, 10).unwrap()),
+            timeout_timestamp_on_b: TimeoutTimestamp::Never,
+        };
+
+        // the sequence send
+        let seq_key = next_sequence_send_key(&get_port_id(), &get_channel_id());
+        let sequence = get_next_seq(&state, &seq_key);
+        let _ = state
+            .write_log_mut()
+            .write(&seq_key, (u64::from(sequence) + 1).to_be_bytes().to_vec())
+            .expect("write failed");
+        keys_changed.insert(seq_key);
+        // packet commitment
+        let packet =
+            packet_from_message(&msg, sequence, &get_channel_counterparty());
+        let commitment_key =
+            commitment_key(&msg.port_id_on_a, &msg.chan_id_on_a, sequence);
+        let commitment = commitment(&packet);
+        let bytes = commitment.into_vec();
+        let _ = state
+            .write_log_mut()
+            .write(&commitment_key, bytes)
+            .expect("write failed");
+        keys_changed.insert(commitment_key);
+        // withdraw
+        let withdraw_key = withdraw_key(&ibc_token);
+        let bytes = amount.serialize_to_vec();
+        let _ = state
+            .write_log_mut()
+            .write(&withdraw_key, bytes)
+            .expect("write failed");
+        keys_changed.insert(withdraw_key);
+        // event
+        let transfer_event = TransferEvent {
+            sender: msg.packet_data.sender.clone(),
+            receiver: msg.packet_data.receiver.clone(),
+            amount: msg.packet_data.token.amount,
+            denom: msg.packet_data.token.denom.clone(),
+            memo: msg.packet_data.memo.clone(),
+        };
+        let event = RawIbcEvent::Module(ModuleEvent::from(transfer_event));
+        state
+            .write_log_mut()
+            .emit_event::<IbcEvent>(event.try_into().unwrap());
+        let event = RawIbcEvent::SendPacket(SendPacket::new(
+            packet,
+            Order::Unordered,
+            get_connection_id(),
+        ));
+        let message_event = RawIbcEvent::Message(MessageEvent::Channel);
+        state
+            .write_log_mut()
+            .emit_event::<IbcEvent>(message_event.try_into().unwrap());
+        state
+            .write_log_mut()
+            .emit_event::<IbcEvent>(event.try_into().unwrap());
+        let message_event =
+            RawIbcEvent::Message(MessageEvent::Module("transfer".to_owned()));
+        state
+            .write_log_mut()
+            .emit_event::<IbcEvent>(message_event.try_into().unwrap());
+
+        let tx_index = TxIndex::default();
+        let tx_code = vec![];
+        let tx_data = MsgTransfer::<Transfer> {
+            message: msg,
+            transfer: None,
+        }
+        .serialize_to_vec();
+
+        let mut tx = Tx::new(state.in_mem().chain_id.clone(), None);
+        tx.add_code(tx_code, None)
+            .add_serialized_data(tx_data)
+            .sign_wrapper(keypair_1());
+
+        let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter(
+            &TxGasMeter::new(TX_GAS_LIMIT, GAS_SCALE),
+        ));
+        let (vp_wasm_cache, _vp_cache_dir) =
+            wasm::compilation_cache::common::testing::vp_cache();
+
+        let verifiers = BTreeSet::new();
+        let batched_tx = tx.batch_ref_first_tx().unwrap();
+        let ctx = Ctx::new(
+            &ADDRESS,
+            &state,
+            batched_tx.tx,
+            batched_tx.cmt,
+            &tx_index,
+            &gas_meter,
+            &keys_changed,
+            &verifiers,
+            vp_wasm_cache,
+            GasMeterKind::MutGlobal,
+        );
+        let ibc = Ibc::new(ctx);
+        // this should fail because of the invalid message
+        let result = ibc
+            .validate_tx(&batched_tx, &keys_changed, &verifiers)
+            .unwrap_err();
+        println!("DEBUG: {result:?}");
+        let error = result.downcast_ref::<ActionError>().unwrap();
+
+        // check the error is related to "base denom"
+        match error {
+            ActionError::TokenTransfer(TokenTransferError::Host(
+                HostError::Other { description: e },
+            )) => {
+                assert!(e.contains("base denom"));
+            }
+            _ => panic!("Unexpected error: {error}"),
+        }
+    }
 }