Fix: Limitations of Transfer Allowlist (#1693)

* feat: allow to add restriction for all currencies in one go, allow block native * fix: remove native currency blocker * fix: review suggestion * feat: migration * fix: unit-tests * fix: integration tests
centrifuge · Jan 24, 2024 · 2f19a3c · 2f19a3c
1 parent 0426f76
commit 2f19a3c
Show file tree

Hide file tree

Showing 21 changed files with 673 additions and 152 deletions.
diff --git a/libs/traits/src/lib.rs b/libs/traits/src/lib.rs
@@ -492,7 +492,7 @@ pub trait TransferAllowance<AccountId> {
 		send: AccountId,
 		receive: Self::Location,
 		currency: Self::CurrencyId,
-	) -> DispatchResult;
+	) -> Result<Option<Self::Location>, DispatchError>;
 }
 
 /// Trait to retrieve information about currencies.

diff --git a/libs/types/src/locations.rs b/libs/types/src/locations.rs
@@ -10,22 +10,20 @@
 // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 // GNU General Public License for more details.
 
+use cfg_primitives::AccountId;
 use frame_support::RuntimeDebugNoBound;
 use parity_scale_codec::{Decode, Encode, MaxEncodedLen};
 use scale_info::TypeInfo;
-use sp_core::H256;
-use sp_runtime::{
-	traits::{BlakeTwo256, Hash},
-	AccountId32,
-};
+use sp_core::{crypto::AccountId32, H256};
+use sp_runtime::traits::{BlakeTwo256, Hash};
 use xcm::{v3::MultiLocation, VersionedMultiLocation};
 
 use crate::domain_address::DomainAddress;
 /// Location types for destinations that can receive restricted transfers
 #[derive(Clone, RuntimeDebugNoBound, Encode, Decode, Eq, PartialEq, MaxEncodedLen, TypeInfo)]
 pub enum Location {
 	/// Local chain account sending destination.
-	Local(AccountId32),
+	Local(AccountId),
 	/// XCM MultiLocation sending destinations.
 	/// Using hash value here as Multilocation is large -- v1 is 512 bytes, but
 	/// next largest is only 40 bytes other values aren't hashed as we have
@@ -36,8 +34,8 @@ pub enum Location {
 }
 
 impl From<AccountId32> for Location {
-	fn from(a: AccountId32) -> Self {
-		Self::Local(a)
+	fn from(value: AccountId32) -> Self {
+		Self::Local(value)
 	}
 }
 

diff --git a/libs/types/src/tokens.rs b/libs/types/src/tokens.rs
@@ -321,6 +321,20 @@ impl From<LiquidityPoolsWrappedToken> for DomainAddress {
 	}
 }
 
+#[derive(
+	Clone, Copy, PartialOrd, Ord, PartialEq, Eq, Debug, Encode, Decode, TypeInfo, MaxEncodedLen,
+)]
+pub enum FilterCurrency {
+	All,
+	Specific(CurrencyId),
+}
+
+impl From<CurrencyId> for FilterCurrency {
+	fn from(value: CurrencyId) -> Self {
+		Self::Specific(value)
+	}
+}
+
 pub mod before {
 	use cfg_primitives::{PoolId, TrancheId};
 	use parity_scale_codec::{Decode, Encode, MaxEncodedLen};

diff --git a/pallets/transfer-allowlist/src/benchmarking.rs b/pallets/transfer-allowlist/src/benchmarking.rs
@@ -12,7 +12,7 @@
 
 #![cfg(feature = "runtime-benchmarks")]
 
-use cfg_types::tokens::CurrencyId;
+use cfg_types::tokens::{CurrencyId, FilterCurrency};
 use frame_benchmarking::*;
 use frame_support::{
 	pallet_prelude::Get,
@@ -28,15 +28,15 @@ use sp_runtime::{
 
 use super::*;
 
-const BENCHMARK_CURRENCY_ID: CurrencyId = CurrencyId::ForeignAsset(1);
+const BENCHMARK_CURRENCY_ID: FilterCurrency = FilterCurrency::Specific(CurrencyId::ForeignAsset(1));
 
 benchmarks! {
 	where_clause {
 		where
 		<T as frame_system::Config>::AccountId: Into<<T as pallet::Config>::Location>,
 		<T as pallet::Config>::Location: From<<T as frame_system::Config>::AccountId> + EncodeLike<<T as pallet::Config>::Location>,
 			<T as pallet::Config>::ReserveCurrency: Currency<<T as frame_system::Config>::AccountId> + ReservableCurrency<<T as frame_system::Config>::AccountId>,
-		T: pallet::Config<CurrencyId = CurrencyId>,
+		T: pallet::Config<CurrencyId = FilterCurrency>,
 		<T as frame_system::Config>::BlockNumber: AtLeast32BitUnsigned + Bounded + TypeInfo,
 		<<T as pallet::Config>::ReserveCurrency as frame_support::traits::fungible::Inspect<<T as frame_system::Config>::AccountId,>>::Balance: From<u64>
 	}

diff --git a/pallets/transfer-allowlist/src/lib.rs b/pallets/transfer-allowlist/src/lib.rs
@@ -73,7 +73,7 @@ pub mod pallet {
 	>>::Reason;
 
 	/// The current storage version.
-	const STORAGE_VERSION: StorageVersion = StorageVersion::new(0);
+	pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
 
 	#[pallet::pallet]
 	#[pallet::storage_version(STORAGE_VERSION)]
@@ -84,13 +84,7 @@ pub mod pallet {
 	pub trait Config: frame_system::Config {
 		type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
 
-		type CurrencyId: AssetId + Parameter + Member + Copy + Default;
-
-		// We need this to block restrictions on the native chain currency as
-		// currently it is impossible to place transfer restrictions
-		// on native currencies as we simply have no way of
-		// restricting pallet-balances...
-		type NativeCurrency: Get<Self::CurrencyId>;
+		type CurrencyId: AssetId + Parameter + Member + Copy;
 
 		/// Currency for holding/unholding with allowlist adding/removal,
 		/// given that the allowlist will be in storage
@@ -256,8 +250,6 @@ pub mod pallet {
 		/// Transfer from sending account and currency not allowed to
 		/// destination
 		NoAllowanceForDestination,
-		/// Native currency can not be restricted with allowances
-		NativeCurrencyNotRestrictable,
 	}
 
 	#[pallet::event]
@@ -332,11 +324,6 @@ pub mod pallet {
 		) -> DispatchResult {
 			let account_id = ensure_signed(origin)?;
 
-			ensure!(
-				currency_id != T::NativeCurrency::get(),
-				Error::<T>::NativeCurrencyNotRestrictable
-			);
-
 			let allowance_details = match Self::get_account_currency_restriction_count_delay(
 				&account_id,
 				currency_id,
@@ -766,24 +753,28 @@ pub mod pallet {
 			send: T::AccountId,
 			receive: Self::Location,
 			currency: T::CurrencyId,
-		) -> DispatchResult {
+		) -> Result<Option<Self::Location>, DispatchError> {
 			match Self::get_account_currency_restriction_count_delay(&send, currency) {
 				Some(AllowanceMetadata {
 					allowance_count: count,
 					..
 				}) if count > 0 => {
 					let current_block = <frame_system::Pallet<T>>::block_number();
-					match <AccountCurrencyTransferAllowance<T>>::get((&send, &currency, receive)) {
+					match <AccountCurrencyTransferAllowance<T>>::get((
+						&send,
+						&currency,
+						receive.clone(),
+					)) {
 						Some(AllowanceDetails {
 							allowed_at,
 							blocked_at,
-						}) if current_block >= allowed_at && current_block < blocked_at => Ok(()),
+						}) if current_block >= allowed_at && current_block < blocked_at => Ok(Some(receive)),
 						_ => Err(DispatchError::from(Error::<T>::NoAllowanceForDestination)),
 					}
 				}
 				// In this case no allowances are set for the sending account & currency,
 				// therefore no restrictions should be in place.
-				_ => Ok(()),
+				_ => Ok(None),
 			}
 		}
 	}

diff --git a/pallets/transfer-allowlist/src/mock.rs b/pallets/transfer-allowlist/src/mock.rs
@@ -10,7 +10,7 @@
 // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 // GNU General Public License for more details.
 
-use cfg_types::tokens::CurrencyId;
+use cfg_types::tokens::FilterCurrency;
 use frame_support::{
 	parameter_types,
 	traits::{ConstU32, ConstU64},
@@ -122,16 +122,14 @@ impl pallet_balances::Config for Runtime {
 }
 
 parameter_types! {
-	pub const NativeCurrency: CurrencyId = CurrencyId::Native;
 	pub const HoldId: () = ();
 }
 
 impl transfer_allowlist::Config for Runtime {
-	type CurrencyId = CurrencyId;
+	type CurrencyId = FilterCurrency;
 	type Deposit = ConstU64<10>;
 	type HoldId = HoldId;
 	type Location = Location;
-	type NativeCurrency = NativeCurrency;
 	type ReserveCurrency = Balances;
 	type RuntimeEvent = RuntimeEvent;
 	type WeightInfo = ();

diff --git a/pallets/transfer-allowlist/src/tests.rs b/pallets/transfer-allowlist/src/tests.rs
@@ -1,41 +1,11 @@
-use cfg_types::tokens::CurrencyId;
+use cfg_types::tokens::{CurrencyId, FilterCurrency};
 use frame_support::{assert_err, assert_noop, assert_ok};
 
 use super::*;
 use crate::mock::*;
 
-const TEST_CURRENCY_ID: CurrencyId = CurrencyId::ForeignAsset(1);
+const TEST_CURRENCY_ID: FilterCurrency = FilterCurrency::Specific(CurrencyId::ForeignAsset(1));
 
-#[test]
-fn add_transfer_fails_for_native() {
-	new_test_ext().execute_with(|| {
-		assert_noop!(
-			TransferAllowList::add_transfer_allowance(
-				RuntimeOrigin::signed(SENDER),
-				<Runtime as Config>::NativeCurrency::get(),
-				ACCOUNT_RECEIVER.into(),
-			),
-			Error::<Runtime>::NativeCurrencyNotRestrictable
-		);
-		assert_eq!(
-			TransferAllowList::get_account_currency_transfer_allowance((
-				SENDER,
-				TEST_CURRENCY_ID,
-				<Runtime as Config>::Location::from(ACCOUNT_RECEIVER)
-			)),
-			None,
-		);
-		assert_eq!(
-			TransferAllowList::get_account_currency_restriction_count_delay(
-				SENDER,
-				TEST_CURRENCY_ID
-			),
-			None,
-		);
-
-		assert_eq!(Balances::reserved_balance(&SENDER), 0);
-	})
-}
 #[test]
 fn add_transfer_allowance_works() {
 	new_test_ext().execute_with(|| {
@@ -162,7 +132,7 @@ fn transfer_allowance_allows_correctly_with_allowance_set() {
 		));
 		assert_eq!(
 			TransferAllowList::allowance(SENDER.into(), ACCOUNT_RECEIVER.into(), TEST_CURRENCY_ID),
-			Ok(())
+			Ok(Some(ACCOUNT_RECEIVER.into()))
 		)
 	})
 }
@@ -213,7 +183,7 @@ fn transfer_allowance_blocks_correctly_when_after_blocked_at_block() {
 		));
 		assert_eq!(
 			TransferAllowList::allowance(SENDER.into(), ACCOUNT_RECEIVER.into(), TEST_CURRENCY_ID),
-			Ok(())
+			Ok(Some(ACCOUNT_RECEIVER.into()))
 		)
 	})
 }

diff --git a/runtime/altair/src/lib.rs b/runtime/altair/src/lib.rs
@@ -81,7 +81,7 @@ use runtime_common::{
 	oracle::{Feeder, OracleConverterBridge},
 	permissions::PoolAdminCheck,
 	xcm::AccountIdToMultiLocation,
-	xcm_transactor, AllowanceDeposit, CurrencyED, HoldId, NativeCurrency,
+	xcm_transactor, AllowanceDeposit, CurrencyED, HoldId,
 };
 use scale_info::TypeInfo;
 use sp_api::impl_runtime_apis;
@@ -1742,11 +1742,10 @@ impl pallet_order_book::Config for Runtime {
 }
 
 impl pallet_transfer_allowlist::Config for Runtime {
-	type CurrencyId = CurrencyId;
+	type CurrencyId = FilterCurrency;
 	type Deposit = AllowanceDeposit<Fees>;
 	type HoldId = HoldId;
 	type Location = Location;
-	type NativeCurrency = NativeCurrency;
 	type ReserveCurrency = Balances;
 	type RuntimeEvent = RuntimeEvent;
 	type WeightInfo = weights::pallet_transfer_allowlist::WeightInfo<Runtime>;
@@ -1866,6 +1865,7 @@ pub type SignedExtra = (
 	frame_system::CheckNonce<Runtime>,
 	frame_system::CheckWeight<Runtime>,
 	pallet_transaction_payment::ChargeTransactionPayment<Runtime>,
+	runtime_common::transfer_filter::PreBalanceTransferExtension<Runtime>,
 );
 
 /// Unchecked extrinsic type as expected by this runtime.
@@ -1987,7 +1987,7 @@ mod __runtime_api_use {
 
 #[cfg(not(feature = "disable-runtime-api"))]
 use __runtime_api_use::*;
-use cfg_types::locations::Location;
+use cfg_types::{locations::Location, tokens::FilterCurrency};
 use runtime_common::transfer_filter::PreNativeTransfer;
 
 #[cfg(not(feature = "disable-runtime-api"))]

diff --git a/runtime/altair/src/migrations.rs b/runtime/altair/src/migrations.rs
@@ -53,6 +53,8 @@ pub type UpgradeAltair1034 = (
 	runtime_common::migrations::precompile_account_codes::Migration<crate::Runtime>,
 	// Migrates EpochExecution V1 to V2
 	runtime_common::migrations::epoch_execution::Migration<crate::Runtime>,
+	// Probably not needed, as storage is likely not populated. Mirates currency used in allowlist
+	runtime_common::migrations::transfer_allowlist_currency::Migration<crate::Runtime>,
 );
 
 mod asset_registry {

diff --git a/runtime/centrifuge/src/lib.rs b/runtime/centrifuge/src/lib.rs
@@ -85,7 +85,7 @@ use runtime_common::{
 	oracle::{Feeder, OracleConverterBridge},
 	permissions::PoolAdminCheck,
 	xcm::AccountIdToMultiLocation,
-	xcm_transactor, AllowanceDeposit, CurrencyED, HoldId, NativeCurrency,
+	xcm_transactor, AllowanceDeposit, CurrencyED, HoldId,
 };
 use scale_info::TypeInfo;
 use sp_api::impl_runtime_apis;
@@ -1853,11 +1853,10 @@ impl pallet_order_book::Config for Runtime {
 }
 
 impl pallet_transfer_allowlist::Config for Runtime {
-	type CurrencyId = CurrencyId;
+	type CurrencyId = FilterCurrency;
 	type Deposit = AllowanceDeposit<Fees>;
 	type HoldId = HoldId;
 	type Location = Location;
-	type NativeCurrency = NativeCurrency;
 	type ReserveCurrency = Balances;
 	type RuntimeEvent = RuntimeEvent;
 	type WeightInfo = weights::pallet_transfer_allowlist::WeightInfo<Runtime>;
@@ -1996,6 +1995,7 @@ pub type SignedExtra = (
 	frame_system::CheckNonce<Runtime>,
 	frame_system::CheckWeight<Runtime>,
 	pallet_transaction_payment::ChargeTransactionPayment<Runtime>,
+	runtime_common::transfer_filter::PreBalanceTransferExtension<Runtime>,
 );
 /// Unchecked extrinsic type as expected by this runtime.
 pub type UncheckedExtrinsic =
@@ -2045,7 +2045,7 @@ mod __runtime_api_use {
 
 #[cfg(not(feature = "disable-runtime-api"))]
 use __runtime_api_use::*;
-use cfg_types::locations::Location;
+use cfg_types::{locations::Location, tokens::FilterCurrency};
 use runtime_common::transfer_filter::PreNativeTransfer;
 
 #[cfg(not(feature = "disable-runtime-api"))]

diff --git a/runtime/centrifuge/src/migrations.rs b/runtime/centrifuge/src/migrations.rs
@@ -11,8 +11,13 @@
 // GNU General Public License for more details.
 
 pub type UpgradeCentrifuge1025 = (
+	// Burns tokens from other domains that are falsly not burned when they were transferred back
+	// to their domain
 	burn_unburned::Migration<super::Runtime>,
 	runtime_common::migrations::epoch_execution::Migration<super::Runtime>,
+	// Migrates the currency used in `pallet-transfer-allowlist` from our global currency to a
+	// special filter currency enum
+	runtime_common::migrations::transfer_allowlist_currency::Migration<crate::Runtime>,
 );
 
 // Copyright 2021 Centrifuge Foundation (centrifuge.io).

diff --git a/runtime/common/src/migrations/mod.rs b/runtime/common/src/migrations/mod.rs
@@ -17,3 +17,5 @@ pub mod epoch_execution;
 pub mod nuke;
 pub mod orml_tokens;
 pub mod precompile_account_codes;
+
+pub mod transfer_allowlist_currency;