Fix reentrancy of FrozenBalance::died hook (#10473)

* assets: execute `died` hook outside of mutate

Signed-off-by: Oliver Tale-Yazdi <oliver@tasty.limo>

* assets: extend tests for `died` hook

Signed-off-by: Oliver Tale-Yazdi <oliver@tasty.limo>

* assets: update doc of FrozenBalance::died

Signed-off-by: Oliver Tale-Yazdi <oliver@tasty.limo>

* assets: review fixes

- fix cases where `died` should not have been called
- use `Option<DeadConsequence>` instead of `DeadConsequence`

Signed-off-by: Oliver Tale-Yazdi <oliver@tasty.limo>

* assets: update comment in mock.rs

Signed-off-by: Oliver Tale-Yazdi <oliver@tasty.limo>

* assets: return `Remove` in dead_account

The return value is ignored in the only case that it is produced
by a call, but having it this way makes it more understandable.

Signed-off-by: Oliver Tale-Yazdi <oliver@tasty.limo>

substrate/frame/assets/src/functions.rs

@@ -84,13 +84,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	}
 
 	pub(super) fn dead_account(
-		what: T::AssetId,
 		who: &T::AccountId,
 		d: &mut AssetDetails<T::Balance, T::AccountId, DepositBalanceOf<T, I>>,
 		reason: &ExistenceReason<DepositBalanceOf<T, I>>,
 		force: bool,
 	) -> DeadConsequence {
-		let mut result = Remove;
 		match *reason {
 			ExistenceReason::Consumer => frame_system::Pallet::<T>::dec_consumers(who),
 			ExistenceReason::Sufficient => {
@@ -99,11 +97,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			},
 			ExistenceReason::DepositRefunded => {},
 			ExistenceReason::DepositHeld(_) if !force => return Keep,
-			ExistenceReason::DepositHeld(_) => result = Keep,
+			ExistenceReason::DepositHeld(_) => {},
 		}
 		d.accounts = d.accounts.saturating_sub(1);
-		T::Freezer::died(what, who);
-		result
+		Remove
 	}
 
 	pub(super) fn can_increase(
@@ -323,12 +320,14 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 		T::Currency::unreserve(&who, deposit);
 
-		if let Remove = Self::dead_account(id, &who, &mut details, &account.reason, false) {
+		if let Remove = Self::dead_account(&who, &mut details, &account.reason, false) {
 			Account::<T, I>::remove(id, &who);
 		} else {
 			debug_assert!(false, "refund did not result in dead account?!");
 		}
 		Asset::<T, I>::insert(&id, details);
+		// Executing a hook here is safe, since it is not in a `mutate`.
+		T::Freezer::died(id, &who);
 		Ok(())
 	}
 
@@ -461,6 +460,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		}
 
 		let actual = Self::prep_debit(id, target, amount, f)?;
+		let mut target_died: Option<DeadConsequence> = None;
 
 		Asset::<T, I>::try_mutate(id, |maybe_details| -> DispatchResult {
 			let mut details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
@@ -475,9 +475,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 				account.balance = account.balance.saturating_sub(actual);
 				if account.balance < details.min_balance {
 					debug_assert!(account.balance.is_zero(), "checked in prep; qed");
-					if let Remove =
-						Self::dead_account(id, target, &mut details, &account.reason, false)
-					{
+					target_died =
+						Some(Self::dead_account(target, &mut details, &account.reason, false));
+					if let Some(Remove) = target_died {
 						return Ok(())
 					}
 				};
@@ -488,6 +488,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			Ok(())
 		})?;
 
+		// Execute hook outside of `mutate`.
+		if let Some(Remove) = target_died {
+			T::Freezer::died(id, target);
+		}
 		Ok(actual)
 	}
 
@@ -507,6 +511,24 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		maybe_need_admin: Option<T::AccountId>,
 		f: TransferFlags,
 	) -> Result<T::Balance, DispatchError> {
+		let (balance, died) =
+			Self::transfer_and_die(id, source, dest, amount, maybe_need_admin, f)?;
+		if let Some(Remove) = died {
+			T::Freezer::died(id, source);
+		}
+		Ok(balance)
+	}
+
+	/// Same as `do_transfer` but it does not execute the `FrozenBalance::died` hook and
+	/// instead returns whether and how the `source` account died in this operation.
+	fn transfer_and_die(
+		id: T::AssetId,
+		source: &T::AccountId,
+		dest: &T::AccountId,
+		amount: T::Balance,
+		maybe_need_admin: Option<T::AccountId>,
+		f: TransferFlags,
+	) -> Result<(T::Balance, Option<DeadConsequence>), DispatchError> {
 		// Early exist if no-op.
 		if amount.is_zero() {
 			Self::deposit_event(Event::Transferred {
@@ -515,7 +537,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 				to: dest.clone(),
 				amount,
 			});
-			return Ok(amount)
+			return Ok((amount, None))
 		}
 
 		// Figure out the debit and credit, together with side-effects.
@@ -524,6 +546,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 		let mut source_account =
 			Account::<T, I>::get(id, &source).ok_or(Error::<T, I>::NoAccount)?;
+		let mut source_died: Option<DeadConsequence> = None;
 
 		Asset::<T, I>::try_mutate(id, |maybe_details| -> DispatchResult {
 			let details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
@@ -576,9 +599,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			// Remove source account if it's now dead.
 			if source_account.balance < details.min_balance {
 				debug_assert!(source_account.balance.is_zero(), "checked in prep; qed");
-				if let Remove =
-					Self::dead_account(id, &source, details, &source_account.reason, false)
-				{
+				source_died =
+					Some(Self::dead_account(&source, details, &source_account.reason, false));
+				if let Some(Remove) = source_died {
 					Account::<T, I>::remove(id, &source);
 					return Ok(())
 				}
@@ -593,7 +616,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			to: dest.clone(),
 			amount: credit,
 		});
-		Ok(credit)
+		Ok((credit, source_died))
 	}
 
 	/// Create a new asset without taking a deposit.
@@ -646,41 +669,53 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		witness: DestroyWitness,
 		maybe_check_owner: Option<T::AccountId>,
 	) -> Result<DestroyWitness, DispatchError> {
-		Asset::<T, I>::try_mutate_exists(id, |maybe_details| {
-			let mut details = maybe_details.take().ok_or(Error::<T, I>::Unknown)?;
-			if let Some(check_owner) = maybe_check_owner {
-				ensure!(details.owner == check_owner, Error::<T, I>::NoPermission);
-			}
-			ensure!(details.accounts <= witness.accounts, Error::<T, I>::BadWitness);
-			ensure!(details.sufficients <= witness.sufficients, Error::<T, I>::BadWitness);
-			ensure!(details.approvals <= witness.approvals, Error::<T, I>::BadWitness);
+		let mut dead_accounts: Vec<T::AccountId> = vec![];
 
-			for (who, v) in Account::<T, I>::drain_prefix(id) {
-				// We have to force this as it's destroying the entire asset class.
-				// This could mean that some accounts now have irreversibly reserved
-				// funds.
-				let _ = Self::dead_account(id, &who, &mut details, &v.reason, true);
-			}
-			debug_assert_eq!(details.accounts, 0);
-			debug_assert_eq!(details.sufficients, 0);
+		let result_witness: DestroyWitness = Asset::<T, I>::try_mutate_exists(
+			id,
+			|maybe_details| -> Result<DestroyWitness, DispatchError> {
+				let mut details = maybe_details.take().ok_or(Error::<T, I>::Unknown)?;
+				if let Some(check_owner) = maybe_check_owner {
+					ensure!(details.owner == check_owner, Error::<T, I>::NoPermission);
+				}
+				ensure!(details.accounts <= witness.accounts, Error::<T, I>::BadWitness);
+				ensure!(details.sufficients <= witness.sufficients, Error::<T, I>::BadWitness);
+				ensure!(details.approvals <= witness.approvals, Error::<T, I>::BadWitness);
 
-			let metadata = Metadata::<T, I>::take(&id);
-			T::Currency::unreserve(
-				&details.owner,
-				details.deposit.saturating_add(metadata.deposit),
-			);
+				for (who, v) in Account::<T, I>::drain_prefix(id) {
+					// We have to force this as it's destroying the entire asset class.
+					// This could mean that some accounts now have irreversibly reserved
+					// funds.
+					let _ = Self::dead_account(&who, &mut details, &v.reason, true);
+					dead_accounts.push(who);
+				}
+				debug_assert_eq!(details.accounts, 0);
+				debug_assert_eq!(details.sufficients, 0);
 
-			for ((owner, _), approval) in Approvals::<T, I>::drain_prefix((&id,)) {
-				T::Currency::unreserve(&owner, approval.deposit);
-			}
-			Self::deposit_event(Event::Destroyed { asset_id: id });
+				let metadata = Metadata::<T, I>::take(&id);
+				T::Currency::unreserve(
+					&details.owner,
+					details.deposit.saturating_add(metadata.deposit),
+				);
 
-			Ok(DestroyWitness {
-				accounts: details.accounts,
-				sufficients: details.sufficients,
-				approvals: details.approvals,
-			})
-		})
+				for ((owner, _), approval) in Approvals::<T, I>::drain_prefix((&id,)) {
+					T::Currency::unreserve(&owner, approval.deposit);
+				}
+				Self::deposit_event(Event::Destroyed { asset_id: id });
+
+				Ok(DestroyWitness {
+					accounts: details.accounts,
+					sufficients: details.sufficients,
+					approvals: details.approvals,
+				})
+			},
+		)?;
+
+		// Execute hooks outside of `mutate`.
+		for who in dead_accounts {
+			T::Freezer::died(id, &who);
+		}
+		Ok(result_witness)
 	}
 
 	/// Creates an approval from `owner` to spend `amount` of asset `id` tokens by 'delegate'
@@ -742,6 +777,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		destination: &T::AccountId,
 		amount: T::Balance,
 	) -> DispatchResult {
+		let mut owner_died: Option<DeadConsequence> = None;
+
 		Approvals::<T, I>::try_mutate_exists(
 			(id, &owner, delegate),
 			|maybe_approved| -> DispatchResult {
@@ -750,7 +787,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 					approved.amount.checked_sub(&amount).ok_or(Error::<T, I>::Unapproved)?;
 
 				let f = TransferFlags { keep_alive: false, best_effort: false, burn_dust: false };
-				Self::do_transfer(id, &owner, &destination, amount, None, f)?;
+				owner_died = Self::transfer_and_die(id, &owner, &destination, amount, None, f)?.1;
 
 				if remaining.is_zero() {
 					T::Currency::unreserve(&owner, approved.deposit);
@@ -766,6 +803,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 				Ok(())
 			},
 		)?;
+
+		// Execute hook outside of `mutate`.
+		if let Some(Remove) = owner_died {
+			T::Freezer::died(id, owner);
+		}
 		Ok(())
 	}