From c761976863f2e73824b3eaac2a1759c7d797721a Mon Sep 17 00:00:00 2001 From: Vladislav Date: Wed, 20 Sep 2023 20:46:24 -0300 Subject: [PATCH] Validate memo in `transfer` extrinsic (#417) * Validate memo in `transfer` extrinsic Added `ValidateMemo` trait --- Cargo.lock | 1 + contracts/pallet-ibc/src/ics20/mod.rs | 4 ++ contracts/pallet-ibc/src/lib.rs | 22 +++++- contracts/pallet-ibc/src/mock.rs | 43 +++++++++++- contracts/pallet-ibc/src/tests.rs | 62 ++++++++++++++++- hyperspace/core/src/substrate/default.rs | 3 +- utils/parachain-node/runtime/Cargo.toml | 5 +- utils/parachain-node/runtime/src/lib.rs | 63 ++++++++++++++++- .../subxt/generated/src/default/parachain.rs | 68 ++++++++++++------- 9 files changed, 233 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1f1982e6e..0fe7e5e1d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8284,6 +8284,7 @@ dependencies = [ "polkadot-runtime-common", "scale-info", "serde", + "serde_json", "simnode-runtime-apis", "smallvec", "sp-api", diff --git a/contracts/pallet-ibc/src/ics20/mod.rs b/contracts/pallet-ibc/src/ics20/mod.rs index e1d26d175..8933df3fb 100644 --- a/contracts/pallet-ibc/src/ics20/mod.rs +++ b/contracts/pallet-ibc/src/ics20/mod.rs @@ -919,3 +919,7 @@ where ); } } + +pub trait ValidateMemo { + fn validate(&self) -> Result<(), String>; +} diff --git a/contracts/pallet-ibc/src/lib.rs b/contracts/pallet-ibc/src/lib.rs index 2791e8b48..8664944e7 100644 --- a/contracts/pallet-ibc/src/lib.rs +++ b/contracts/pallet-ibc/src/lib.rs @@ -30,6 +30,7 @@ #[macro_use] extern crate alloc; +use crate::ics20::ValidateMemo; use codec::{Decode, Encode}; use core::fmt::Debug; use cumulus_primitives_core::ParaId; @@ -273,7 +274,8 @@ pub mod pallet { + Clone + Eq + TryFrom - + TryInto; + + TryInto + + ValidateMemo; type SubstrateMultihopXcmHandler: SubstrateMultihopXcmHandler; @@ -716,10 +718,15 @@ pub mod pallet { /// Access denied AccessDenied, RateLimiter, - //Fee errors + /// Fee errors FailedSendFeeToAccount, - //Failed to derive origin sender address. + /// Failed to derive origin sender address. OriginAddress, + /// The memo hasn't passed the validation. Potential reasons: + /// - The memo is too long. + /// - The memo is in invalid format + /// - The memo contains unsupported middlewares + InvalidMemo, } #[pallet::hooks] @@ -961,6 +968,15 @@ pub mod pallet { }); }; + memo.as_ref() + .map(|memo| { + memo.validate().map_err(|e| { + log::debug!(target: "pallet_ibc", "[transfer]: memo validation error: {}", e); + Error::::InvalidMemo + }) + }) + .transpose()?; + let msg = MsgTransfer { source_port, source_channel, diff --git a/contracts/pallet-ibc/src/mock.rs b/contracts/pallet-ibc/src/mock.rs index b932d85fd..9a70de0d9 100644 --- a/contracts/pallet-ibc/src/mock.rs +++ b/contracts/pallet-ibc/src/mock.rs @@ -3,6 +3,7 @@ use crate::{ routing::ModuleRouter, }; use cumulus_primitives_core::ParaId; +use derive_more::Display; use frame_support::{ pallet_prelude::ConstU32, parameter_types, @@ -29,6 +30,7 @@ use sp_runtime::{ MultiSignature, Perbill, }; use std::{ + convert::Infallible, sync::Arc, time::{SystemTime, UNIX_EPOCH}, }; @@ -45,7 +47,7 @@ pub type Balance = u128; pub type AccountId = <::Signer as IdentifyAccount>::AccountId; use super::*; use crate::{ - ics20::IbcMemoHandler, + ics20::{IbcMemoHandler, MemoData}, light_clients::{AnyClientMessage, AnyConsensusState}, }; use ibc::mock::{client_state::MockConsensusState, header::MockClientMessage, host::MockHostBlock}; @@ -213,6 +215,43 @@ fn create_alice_key() -> ::AccountIdConversion { IbcAccount(account_id_32) } +#[derive(Clone, Debug, Eq, PartialEq, Default, Display, Encode, Decode, TypeInfo)] +pub struct RawMemo(pub String); + +impl FromStr for RawMemo { + type Err = Infallible; + + fn from_str(s: &str) -> Result { + Ok(Self(s.to_string())) + } +} + +impl TryFrom for RawMemo { + type Error = >::Error; + + fn try_from(value: MemoData) -> Result { + Ok(Self(value.try_into()?)) + } +} + +impl TryFrom for MemoData { + type Error = >::Error; + + fn try_from(value: RawMemo) -> Result { + Ok(value.0.try_into()?) + } +} + +impl ValidateMemo for RawMemo { + fn validate(&self) -> Result<(), String> { + if self.0 == "invalid memo" { + return Err(self.0.clone()) + } else { + Ok(()) + } + } +} + impl Config for Test { type TimeProvider = Timestamp; type RuntimeEvent = RuntimeEvent; @@ -238,7 +277,7 @@ impl Config for Test { type TransferOrigin = EnsureSigned; type RelayerOrigin = EnsureSigned; type HandleMemo = IbcMemoHandler<(), Test>; - type MemoMessage = alloc::string::String; + type MemoMessage = RawMemo; type IsReceiveEnabled = sp_core::ConstBool; type IsSendEnabled = sp_core::ConstBool; type FeeAccount = FeeAccount; diff --git a/contracts/pallet-ibc/src/tests.rs b/contracts/pallet-ibc/src/tests.rs index 2826b4c9c..96eeac755 100644 --- a/contracts/pallet-ibc/src/tests.rs +++ b/contracts/pallet-ibc/src/tests.rs @@ -8,7 +8,7 @@ use crate::{ }; use core::time::Duration; use frame_support::{ - assert_ok, + assert_noop, assert_ok, traits::{ fungibles::{Inspect, Mutate}, Currency, Hooks, Len, @@ -265,6 +265,66 @@ fn send_transfer() { }) } +#[test] +fn send_transfer_with_invalid_memo() { + let mut ext = new_test_ext(); + let balance = 100000 * MILLIS; + ext.execute_with(|| { + let pair = sp_core::sr25519::Pair::from_seed(b"12345678901234567890123456789012"); + let ss58_address = + ibc_primitives::runtime_interface::account_id_to_ss58(pair.public().0, 49); + setup_client_and_consensus_state(PortId::transfer()); + let asset_id = + <::IbcDenomToAssetIdConversion as DenomToAssetId>::from_denom_to_asset_id( + "PICA", + ) + .unwrap(); + let _ = <::NativeCurrency as Currency< + ::AccountId, + >>::deposit_creating(&AccountId32::new([0; 32]), balance); + + let timeout = Timeout::Offset { timestamp: Some(1000), height: Some(5) }; + + let ctx = Context::::default(); + let channel_end = ctx + .channel_end(&(PortId::transfer(), ChannelId::new(0))) + .expect("expect source_channel unwrap"); + let destination_channel = channel_end.counterparty().channel_id.unwrap(); + Ibc::add_channels_to_feeless_channel_list( + RuntimeOrigin::root(), + 0, + destination_channel.sequence(), + ) + .expect("expect add channels to feeless list"); + + let result = Ibc::transfer( + RuntimeOrigin::signed(AccountId32::new([0; 32])), + TransferParams { + to: MultiAddress::Raw(ss58_address.as_bytes().to_vec()), + source_channel: 0, + timeout: timeout.clone(), + }, + asset_id, + balance, + Some(RawMemo("{}".to_string())), + ); + assert_ok!(result); + + let result = Ibc::transfer( + RuntimeOrigin::signed(AccountId32::new([0; 32])), + TransferParams { + to: MultiAddress::Raw(ss58_address.as_bytes().to_vec()), + source_channel: 0, + timeout: timeout.clone(), + }, + asset_id, + balance, + Some(RawMemo("invalid memo".to_string())), + ); + assert_noop!(result, crate::Error::::InvalidMemo); + }); +} + #[test] fn send_transfer_no_fee_feeless_channels() { let mut ext = new_test_ext(); diff --git a/hyperspace/core/src/substrate/default.rs b/hyperspace/core/src/substrate/default.rs index 1b657b936..a0e9f82d6 100644 --- a/hyperspace/core/src/substrate/default.rs +++ b/hyperspace/core/src/substrate/default.rs @@ -5,6 +5,7 @@ use self::parachain_subxt::api::{ frame_system::{extensions::check_nonce::CheckNonce, EventRecord}, pallet_ibc::{events::IbcEvent as MetadataIbcEvent, TransferParams as RawTransferParams}, pallet_ibc_ping::SendPingParams as RawSendPingParams, + parachain_runtime::RawMemo, }, sudo::calls::Sudo, }; @@ -118,7 +119,7 @@ define_runtime_transactions!( TransferParamsWrapper, SendPingParamsWrapper, parachain_subxt::api::runtime_types::pallet_ibc::Any, - String, + RawMemo, |x| parachain_subxt::api::tx().ibc().deliver(x), |x, y, z, w| parachain_subxt::api::tx().ibc().transfer(x, y, z, w), |x| parachain_subxt::api::tx().sudo().sudo(x), diff --git a/utils/parachain-node/runtime/Cargo.toml b/utils/parachain-node/runtime/Cargo.toml index 78cc9d82d..a511938a9 100644 --- a/utils/parachain-node/runtime/Cargo.toml +++ b/utils/parachain-node/runtime/Cargo.toml @@ -22,7 +22,8 @@ codec = { package = "parity-scale-codec", version = "3.0.0", default-features = hex-literal = { version = "0.3.4", optional = true } log = { version = "0.4.17", default-features = false } scale-info = { version = "2.1.1", default-features = false, features = ["derive"] } -serde = { version = "1.0.140", optional = true, features = ["derive"] } +serde = { version = "1.0.140", features = ["derive"], default-features = false } +serde_json = { version = "1", default-features = false } smallvec = "1.9.0" # Local @@ -97,7 +98,6 @@ std = [ "codec/std", "log/std", "scale-info/std", - "serde", "cumulus-pallet-aura-ext/std", "cumulus-pallet-dmp-queue/std", "cumulus-pallet-parachain-system/std", @@ -147,6 +147,7 @@ std = [ "asset-registry/std", "orml-traits/std", "simnode-apis/std", + "serde_json/std", "frame-benchmarking/std", ] testing = [ diff --git a/utils/parachain-node/runtime/src/lib.rs b/utils/parachain-node/runtime/src/lib.rs index 9a2ecc968..ab40d117a 100644 --- a/utils/parachain-node/runtime/src/lib.rs +++ b/utils/parachain-node/runtime/src/lib.rs @@ -24,11 +24,13 @@ extern crate alloc; use alloc::string::String; use asset_registry::{AssetMetadata, DefaultAssetMetadata}; +use core::fmt::{Display, Formatter}; mod weights; pub mod xcm_config; -use codec::Encode; +use alloc::string::ToString; +use codec::{Decode, Encode}; use core::str::FromStr; use cumulus_pallet_parachain_system::RelayNumberStrictlyIncreases; use ibc::core::{ @@ -82,12 +84,18 @@ pub use sp_runtime::BuildStorage; // Polkadot imports use polkadot_runtime_common::{BlockHashCount, SlowAdjustingFeeUpdate}; +use scale_info::TypeInfo; +use serde::Deserialize; +use sp_core::crypto::Infallible; use sp_runtime::traits::AccountIdConversion; use weights::{BlockExecutionWeight, ExtrinsicBaseWeight, RocksDbWeight}; // XCM Imports -use pallet_ibc::routing::ModuleRouter; +use pallet_ibc::{ + ics20::{MemoData, ValidateMemo}, + routing::ModuleRouter, +}; use xcm::latest::prelude::BodyId; use xcm_executor::XcmExecutor; @@ -717,6 +725,55 @@ fn create_alice_key() -> ::AccountIdConversion { IbcAccount(account_id_32) } +#[derive(Deserialize, Debug)] +#[serde(rename_all = "snake_case")] +pub enum MemoMiddlewareNamespaceChain { + Forward { next: Option> }, + Wasm { next: Option> }, +} + +#[derive(Clone, Debug, Eq, PartialEq, Default, Encode, Decode, TypeInfo)] +pub struct RawMemo(pub String); + +impl FromStr for RawMemo { + type Err = Infallible; + + fn from_str(s: &str) -> Result { + Ok(Self(s.to_string())) + } +} + +impl TryFrom for RawMemo { + type Error = >::Error; + + fn try_from(value: MemoData) -> Result { + Ok(Self(value.try_into()?)) + } +} + +impl TryFrom for MemoData { + type Error = >::Error; + + fn try_from(value: RawMemo) -> Result { + Ok(value.0.try_into()?) + } +} + +impl Display for RawMemo { + fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl ValidateMemo for RawMemo { + fn validate(&self) -> Result<(), String> { + // the MiddlewareNamespaceChain type contains all the supported middlewares + serde_json::from_str::(&self.0) + .map(|_| ()) + .map_err(|e| e.to_string()) + } +} + impl pallet_ibc::Config for Runtime { type TimeProvider = Timestamp; type RuntimeEvent = RuntimeEvent; @@ -738,7 +795,7 @@ impl pallet_ibc::Config for Runtime { type SpamProtectionDeposit = SpamProtectionDeposit; type TransferOrigin = EnsureSigned; type RelayerOrigin = EnsureSigned; - type MemoMessage = alloc::string::String; + type MemoMessage = RawMemo; type IsReceiveEnabled = sp_core::ConstBool; type IsSendEnabled = sp_core::ConstBool; type HandleMemo = (); diff --git a/utils/subxt/generated/src/default/parachain.rs b/utils/subxt/generated/src/default/parachain.rs index 07537f373..fa886c60c 100644 --- a/utils/subxt/generated/src/default/parachain.rs +++ b/utils/subxt/generated/src/default/parachain.rs @@ -339,9 +339,9 @@ pub mod api { let runtime_metadata_hash = client.metadata().metadata_hash(&PALLETS); if runtime_metadata_hash != [ - 251u8, 123u8, 152u8, 157u8, 101u8, 130u8, 190u8, 3u8, 247u8, 46u8, 252u8, 87u8, - 60u8, 175u8, 182u8, 235u8, 115u8, 209u8, 82u8, 71u8, 235u8, 37u8, 66u8, 27u8, - 136u8, 231u8, 80u8, 8u8, 28u8, 63u8, 197u8, 234u8, + 69u8, 248u8, 221u8, 243u8, 107u8, 103u8, 34u8, 241u8, 63u8, 37u8, 144u8, 59u8, + 253u8, 107u8, 216u8, 176u8, 55u8, 119u8, 239u8, 149u8, 30u8, 55u8, 104u8, 152u8, + 133u8, 175u8, 136u8, 161u8, 190u8, 49u8, 32u8, 46u8, ] { Err(::subxt::error::MetadataError::IncompatibleMetadata) } else { @@ -964,10 +964,10 @@ pub mod api { "Events", vec![], [ - 12u8, 241u8, 58u8, 94u8, 6u8, 234u8, 177u8, 29u8, 113u8, 148u8, 182u8, - 114u8, 17u8, 143u8, 165u8, 75u8, 60u8, 121u8, 233u8, 109u8, 68u8, - 224u8, 32u8, 125u8, 255u8, 141u8, 102u8, 52u8, 160u8, 37u8, 246u8, - 186u8, + 238u8, 157u8, 248u8, 87u8, 132u8, 135u8, 172u8, 82u8, 189u8, 105u8, + 14u8, 25u8, 174u8, 203u8, 215u8, 140u8, 27u8, 99u8, 26u8, 246u8, 226u8, + 113u8, 243u8, 94u8, 141u8, 211u8, 213u8, 119u8, 203u8, 183u8, 139u8, + 2u8, ], ) } @@ -5776,9 +5776,9 @@ pub mod api { "sudo", Sudo { call: ::std::boxed::Box::new(call) }, [ - 134u8, 145u8, 175u8, 131u8, 36u8, 15u8, 183u8, 43u8, 81u8, 218u8, 78u8, - 146u8, 218u8, 64u8, 155u8, 98u8, 165u8, 102u8, 128u8, 239u8, 215u8, - 169u8, 30u8, 0u8, 53u8, 62u8, 76u8, 194u8, 9u8, 34u8, 8u8, 92u8, + 173u8, 204u8, 175u8, 155u8, 169u8, 89u8, 2u8, 54u8, 49u8, 67u8, 145u8, + 241u8, 117u8, 9u8, 58u8, 200u8, 51u8, 4u8, 210u8, 183u8, 46u8, 72u8, + 32u8, 152u8, 40u8, 23u8, 171u8, 171u8, 120u8, 15u8, 205u8, 166u8, ], ) } @@ -5792,9 +5792,10 @@ pub mod api { "sudo_unchecked_weight", SudoUncheckedWeight { call: ::std::boxed::Box::new(call), weight }, [ - 250u8, 174u8, 48u8, 15u8, 138u8, 144u8, 49u8, 236u8, 101u8, 44u8, 94u8, - 89u8, 219u8, 73u8, 205u8, 150u8, 143u8, 90u8, 40u8, 32u8, 30u8, 103u8, - 185u8, 229u8, 144u8, 100u8, 214u8, 182u8, 12u8, 163u8, 0u8, 27u8, + 90u8, 102u8, 130u8, 185u8, 161u8, 105u8, 97u8, 83u8, 76u8, 106u8, + 201u8, 91u8, 246u8, 242u8, 70u8, 133u8, 64u8, 120u8, 11u8, 85u8, 170u8, + 176u8, 219u8, 162u8, 47u8, 203u8, 9u8, 234u8, 184u8, 171u8, 240u8, + 232u8, ], ) } @@ -5823,9 +5824,9 @@ pub mod api { "sudo_as", SudoAs { who, call: ::std::boxed::Box::new(call) }, [ - 129u8, 194u8, 224u8, 17u8, 145u8, 92u8, 12u8, 252u8, 25u8, 29u8, 69u8, - 231u8, 161u8, 156u8, 130u8, 44u8, 77u8, 126u8, 106u8, 18u8, 82u8, 8u8, - 251u8, 144u8, 217u8, 86u8, 64u8, 93u8, 81u8, 197u8, 138u8, 172u8, + 72u8, 212u8, 5u8, 207u8, 172u8, 90u8, 63u8, 88u8, 63u8, 96u8, 75u8, + 189u8, 110u8, 41u8, 55u8, 219u8, 36u8, 123u8, 142u8, 181u8, 1u8, 228u8, + 29u8, 152u8, 56u8, 111u8, 204u8, 6u8, 35u8, 40u8, 14u8, 41u8, ], ) } @@ -7859,7 +7860,7 @@ pub mod api { pub params: runtime_types::pallet_ibc::TransferParams<::subxt::utils::AccountId32>, pub asset_id: ::core::primitive::u128, pub amount: ::core::primitive::u128, - pub memo: ::core::option::Option<::std::string::String>, + pub memo: ::core::option::Option, } #[derive( :: subxt :: ext :: codec :: Decode, @@ -7973,16 +7974,17 @@ pub mod api { params: runtime_types::pallet_ibc::TransferParams<::subxt::utils::AccountId32>, asset_id: ::core::primitive::u128, amount: ::core::primitive::u128, - memo: ::core::option::Option<::std::string::String>, + memo: ::core::option::Option, ) -> ::subxt::tx::Payload { ::subxt::tx::Payload::new_static( "Ibc", "transfer", Transfer { params, asset_id, amount, memo }, [ - 164u8, 5u8, 38u8, 129u8, 232u8, 229u8, 37u8, 74u8, 69u8, 22u8, 49u8, - 209u8, 83u8, 224u8, 204u8, 50u8, 16u8, 142u8, 39u8, 1u8, 100u8, 35u8, - 150u8, 163u8, 236u8, 6u8, 51u8, 115u8, 11u8, 189u8, 179u8, 72u8, + 183u8, 130u8, 92u8, 37u8, 163u8, 96u8, 215u8, 175u8, 205u8, 184u8, + 82u8, 36u8, 187u8, 217u8, 136u8, 110u8, 227u8, 194u8, 142u8, 107u8, + 69u8, 65u8, 139u8, 141u8, 95u8, 191u8, 171u8, 115u8, 225u8, 188u8, + 163u8, 66u8, ], ) } @@ -8516,7 +8518,7 @@ pub mod api { pub asset_id: ::core::primitive::u128, pub amount: ::core::primitive::u128, pub channel: ::core::primitive::u64, - pub next_memo: ::core::option::Option<::std::string::String>, + pub next_memo: ::core::option::Option, } impl ::subxt::events::StaticEvent for ExecuteMemoIbcTokenTransferSuccess { const PALLET: &'static str = "Ibc"; @@ -8555,7 +8557,7 @@ pub mod api { pub asset_id: ::core::primitive::u128, pub amount: ::core::primitive::u128, pub channel: ::core::primitive::u64, - pub next_memo: ::core::option::Option<::std::string::String>, + pub next_memo: ::core::option::Option, } impl ::subxt::events::StaticEvent for ExecuteMemoIbcTokenTransferFailed { const PALLET: &'static str = "Ibc"; @@ -11644,7 +11646,7 @@ pub mod api { runtime_types::pallet_ibc::TransferParams<::subxt::utils::AccountId32>, asset_id: ::core::primitive::u128, amount: ::core::primitive::u128, - memo: ::core::option::Option<::std::string::String>, + memo: ::core::option::Option, }, #[codec(index = 3)] upgrade_client { params: runtime_types::pallet_ibc::UpgradeParams }, @@ -11766,6 +11768,8 @@ pub mod api { FailedSendFeeToAccount, #[codec(index = 38)] OriginAddress, + #[codec(index = 39)] + InvalidMemo, } #[derive( :: subxt :: ext :: codec :: Decode, @@ -11910,7 +11914,8 @@ pub mod api { asset_id: ::core::primitive::u128, amount: ::core::primitive::u128, channel: ::core::primitive::u64, - next_memo: ::core::option::Option<::std::string::String>, + next_memo: + ::core::option::Option, }, #[codec(index = 22)] ExecuteMemoIbcTokenTransferFailedWithReason { @@ -11925,7 +11930,8 @@ pub mod api { asset_id: ::core::primitive::u128, amount: ::core::primitive::u128, channel: ::core::primitive::u64, - next_memo: ::core::option::Option<::std::string::String>, + next_memo: + ::core::option::Option, }, #[codec(index = 24)] ExecuteMemoXcmSuccess { @@ -12627,6 +12633,16 @@ pub mod api { )] #[decode_as_type(crate_path = ":: subxt :: ext :: scale_decode")] #[encode_as_type(crate_path = ":: subxt :: ext :: scale_encode")] + pub struct RawMemo(pub ::std::string::String); + #[derive( + :: subxt :: ext :: codec :: Decode, + :: subxt :: ext :: codec :: Encode, + :: subxt :: ext :: scale_decode :: DecodeAsType, + :: subxt :: ext :: scale_encode :: EncodeAsType, + Debug, + )] + #[decode_as_type(crate_path = ":: subxt :: ext :: scale_decode")] + #[encode_as_type(crate_path = ":: subxt :: ext :: scale_encode")] pub struct Runtime; #[derive( :: subxt :: ext :: codec :: Decode,