pallet-assets: make touch other permissionless (#7456)

Fix #7450

Pallet assets don't have the assumption that Admin and Freezer are
trusted by the chain, so I don't see any security concern from this
change.

The only difference I see is that people can create more account with 0
balance for a given asset, whereas in the past only privileged role
could do it (privileged from the point of view of the asset, not the
chain).

That said maybe this is not the actual fix we want, maybe it would be
better to have accounts being able to increase their `MaxConsumers`
value by adding some deposit.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Adrian Catangiu <[email protected]>

Author: 3 people

prdoc/pr_7456.prdoc

@@ -0,0 +1,8 @@
+title: 'pallet-assets: make touch other permissionless'
+doc:
+- audience: Runtime User
+  description: |-
+    In pallet assets, make `touch_other` call permissionless. Before this PR only `Admin` and `Freezer` of an asset could touch an account, now every account can.
+crates:
+- name: pallet-assets
+  bump: minor

substrate/frame/assets/src/functions.rs

@@ -338,23 +338,15 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	}
 
 	/// Creates an account for `who` to hold asset `id` with a zero balance and takes a deposit.
-	///
-	/// When `check_depositor` is set to true, the depositor must be either the asset's Admin or
-	/// Freezer, otherwise the depositor can be any account.
 	pub(super) fn do_touch(
 		id: T::AssetId,
 		who: T::AccountId,
 		depositor: T::AccountId,
-		check_depositor: bool,
 	) -> DispatchResult {
 		ensure!(!Account::<T, I>::contains_key(&id, &who), Error::<T, I>::AlreadyExists);
 		let deposit = T::AssetAccountDeposit::get();
 		let mut details = Asset::<T, I>::get(&id).ok_or(Error::<T, I>::Unknown)?;
 		ensure!(details.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
-		ensure!(
-			!check_depositor || &depositor == &details.admin || &depositor == &details.freezer,
-			Error::<T, I>::NoPermission
-		);
 		let reason = Self::new_account(&who, &mut details, Some((&depositor, deposit)))?;
 		T::Currency::reserve(&depositor, deposit)?;
 		Asset::<T, I>::insert(&id, details);

substrate/frame/assets/src/lib.rs

@@ -94,6 +94,7 @@
 //! * `refund`: Return the deposit (if any) of the caller's asset account or a consumer reference
 //!   (if any) of the caller's account.
 //! * `refund_other`: Return the deposit (if any) of a specified asset account.
+//! * `touch_other`: Create an asset account for specified account. Caller must place a deposit.
 //!
 //! ### Permissioned Functions
 //!
@@ -116,8 +117,6 @@
 //!   Owner.
 //! * `set_metadata`: Set the metadata of an asset class; called by the asset class's Owner.
 //! * `clear_metadata`: Remove the metadata of an asset class; called by the asset class's Owner.
-//! * `touch_other`: Create an asset account for specified account. Caller must place a deposit;
-//!   called by the asset class's Freezer or Admin.
 //! * `block`: Disallows further `transfer`s to and from an account; called by the asset class's
 //!   Freezer.
 //!
@@ -1618,7 +1617,7 @@ pub mod pallet {
 		pub fn touch(origin: OriginFor<T>, id: T::AssetIdParameter) -> DispatchResult {
 			let who = ensure_signed(origin)?;
 			let id: T::AssetId = id.into();
-			Self::do_touch(id, who.clone(), who, false)
+			Self::do_touch(id, who.clone(), who)
 		}
 
 		/// Return the deposit (if any) of an asset account or a consumer reference (if any) of an
@@ -1695,9 +1694,10 @@ pub mod pallet {
 		///
 		/// A deposit will be taken from the signer account.
 		///
-		/// - `origin`: Must be Signed by `Freezer` or `Admin` of the asset `id`; the signer account
-		///   must have sufficient funds for a deposit to be taken.
-		/// - `id`: The identifier of the asset for the account to be created.
+		/// - `origin`: Must be Signed; the signer account must have sufficient funds for a deposit
+		///   to be taken.
+		/// - `id`: The identifier of the asset for the account to be created, the asset status must
+		///   be live.
 		/// - `who`: The account to be created.
 		///
 		/// Emits `Touched` event when successful.
@@ -1711,7 +1711,7 @@ pub mod pallet {
 			let origin = ensure_signed(origin)?;
 			let who = T::Lookup::lookup(who)?;
 			let id: T::AssetId = id.into();
-			Self::do_touch(id, who, origin, true)
+			Self::do_touch(id, who, origin)
 		}
 
 		/// Return the deposit (if any) of a target asset account. Useful if you are the depositor.
@@ -1823,7 +1823,6 @@ pub mod pallet {
 
 	/// Implements [`AccountTouch`] trait.
 	/// Note that a depositor can be any account, without any specific privilege.
-	/// This implementation is supposed to be used only for creation of system accounts.
 	impl<T: Config<I>, I: 'static> AccountTouch<T::AssetId, T::AccountId> for Pallet<T, I> {
 		type Balance = DepositBalanceOf<T, I>;
 
@@ -1846,7 +1845,7 @@ pub mod pallet {
 			who: &T::AccountId,
 			depositor: &T::AccountId,
 		) -> DispatchResult {
-			Self::do_touch(asset, who.clone(), depositor.clone(), false)
+			Self::do_touch(asset, who.clone(), depositor.clone())
 		}
 	}
 

substrate/frame/assets/src/tests.rs

@@ -1004,12 +1004,14 @@ fn touching_and_freezing_account_with_zero_asset_balance_should_work() {
 	});
 }
 
+// In the past only admin and freezer could call `touch_other`.
+// Test that this behavior is still supported.
 #[test]
-fn touch_other_works() {
+fn touch_other_works_legacy() {
 	new_test_ext().execute_with(|| {
 		// 1 will be admin
 		// 2 will be freezer
-		// 4 will be an account attempting to execute `touch_other`
+		// 4 will be an account successfully attempting to execute `touch_other`
 		Balances::make_free_balance_be(&1, 100);
 		Balances::make_free_balance_be(&2, 100);
 		Balances::make_free_balance_be(&4, 100);
@@ -1019,19 +1021,42 @@ fn touch_other_works() {
 		assert_eq!(Assets::balance(0, 1), 100);
 		// account `3` does not exist
 		assert!(!Account::<Test>::contains_key(0, &3));
-		// creation of asset account `3` by account `4` fails
-		assert_noop!(
-			Assets::touch_other(RuntimeOrigin::signed(4), 0, 3),
-			Error::<Test>::NoPermission
-		);
+		// creation of asset account `30` by account `4` works
+		assert_ok!(Assets::touch_other(RuntimeOrigin::signed(4), 0, 30));
 		// creation of asset account `3` by admin `1` works
 		assert!(!Account::<Test>::contains_key(0, &3));
 		assert_ok!(Assets::touch_other(RuntimeOrigin::signed(1), 0, 3));
 		assert!(Account::<Test>::contains_key(0, &3));
 		// creation of asset account `4` by freezer `2` works
 		assert!(!Account::<Test>::contains_key(0, &4));
 		assert_ok!(Assets::touch_other(RuntimeOrigin::signed(2), 0, 4));
+	});
+}
+
+#[test]
+fn touch_other_works() {
+	new_test_ext().execute_with(|| {
+		Balances::make_free_balance_be(&3, 100);
+		assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, false, 1));
+		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 3, 100));
+		assert_eq!(Assets::balance(0, 3), 100);
+
+		// account `4` does not exist
+		assert!(!Account::<Test>::contains_key(0, &4));
+
+		// creation of asset account `4` by funded account `3` works
+		assert_ok!(Assets::touch_other(RuntimeOrigin::signed(3), 0, 4));
 		assert!(Account::<Test>::contains_key(0, &4));
+
+		// account `6` does not exist
+		assert!(!Account::<Test>::contains_key(0, &6));
+
+		// creation of asset account `6` by not funded account `5` fails
+		assert_noop!(
+			Assets::touch_other(RuntimeOrigin::signed(5), 0, 6),
+			BalancesError::<Test>::InsufficientBalance,
+		);
+		assert!(!Account::<Test>::contains_key(0, &6));
 	});
 }