From 7105fc32bbb3b0d705fff5a60d45e0429c36084c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Wed, 27 Nov 2024 09:14:49 -0500 Subject: [PATCH] fix(pallet-assets): ensure that an account can die before unreserving any deposit / remove `with_storage_layer`. --- substrate/frame/assets/src/functions.rs | 222 +++++++++++------------- 1 file changed, 103 insertions(+), 119 deletions(-) diff --git a/substrate/frame/assets/src/functions.rs b/substrate/frame/assets/src/functions.rs index 2285d4a254b5e..0247b19c907c7 100644 --- a/substrate/frame/assets/src/functions.rs +++ b/substrate/frame/assets/src/functions.rs @@ -27,7 +27,6 @@ pub(super) enum DeadConsequence { Keep, } -use frame_support::storage::with_storage_layer; use DeadConsequence::*; // The main implementation block for the module. @@ -96,21 +95,23 @@ impl, I: 'static> Pallet { Ok(reason) } + pub(super) fn ensure_account_can_die(id: T::AssetId, who: &T::AccountId) -> DispatchResult { + ensure!( + T::Holder::balance_on_hold(id.clone(), &who).is_none(), + Error::::ContainsHolds + ); + ensure!(T::Freezer::frozen_balance(id, &who).is_none(), Error::::ContainsFreezes); + Ok(()) + } + pub(super) fn dead_account( - id: T::AssetId, who: &T::AccountId, d: &mut AssetDetails>, reason: &ExistenceReasonOf, force: bool, - ) -> Result { + ) -> DeadConsequence { use ExistenceReason::*; - ensure!( - T::Holder::balance_on_hold(id.clone(), &who).is_none(), - Error::::ContainsHolds - ); - ensure!(T::Freezer::frozen_balance(id, &who).is_none(), Error::::ContainsFreezes); - match *reason { Consumer => frame_system::Pallet::::dec_consumers(who), Sufficient => { @@ -118,11 +119,11 @@ impl, I: 'static> Pallet { frame_system::Pallet::::dec_sufficients(who); }, DepositRefunded => {}, - DepositHeld(_) | DepositFrom(..) if !force => return Ok(Keep), + DepositHeld(_) | DepositFrom(..) if !force => return Keep, DepositHeld(_) | DepositFrom(..) => {}, } d.accounts = d.accounts.saturating_sub(1); - Ok(Remove) + Remove } /// Returns `true` when the balance of `account` can be increased by `amount`. @@ -382,29 +383,26 @@ impl, I: 'static> Pallet { let mut details = Asset::::get(&id).ok_or(Error::::Unknown)?; ensure!(matches!(details.status, Live | Frozen), Error::::IncorrectStatus); ensure!(account.balance.is_zero() || allow_burn, Error::::WouldBurn); + Self::ensure_account_can_die(id.clone(), &who)?; - with_storage_layer(|| { - if let Some(deposit) = account.reason.take_deposit() { - T::Currency::unreserve(&who, deposit); - } + if let Some(deposit) = account.reason.take_deposit() { + T::Currency::unreserve(&who, deposit); + } - if let Remove = - Self::dead_account(id.clone(), &who, &mut details, &account.reason, false)? - { - Account::::remove(&id, &who); - } else { - debug_assert!(false, "refund did not result in dead account?!"); - // deposit may have been refunded, need to update `Account` - Account::::insert(id, &who, account); - return Ok(()) - } + if let Remove = Self::dead_account(&who, &mut details, &account.reason, false) { + Account::::remove(&id, &who); + } else { + debug_assert!(false, "refund did not result in dead account?!"); + // deposit may have been refunded, need to update `Account` + Account::::insert(id, &who, account); + return Ok(()) + } - Asset::::insert(&id, details); - // Executing a hook here is safe, since it is not in a `mutate`. - T::Freezer::died(id.clone(), &who); - T::Holder::died(id, &who); - Ok(()) - }) + Asset::::insert(&id, details); + // Executing a hook here is safe, since it is not in a `mutate`. + T::Freezer::died(id.clone(), &who); + T::Holder::died(id, &who); + Ok(()) } /// Refunds the `DepositFrom` of an account only if its balance is zero. @@ -426,26 +424,23 @@ impl, I: 'static> Pallet { ensure!(caller == depositor || caller == details.admin, Error::::NoPermission); } ensure!(account.balance.is_zero(), Error::::WouldBurn); + Self::ensure_account_can_die(id.clone(), &who)?; - with_storage_layer(|| { - T::Currency::unreserve(&depositor, deposit); + T::Currency::unreserve(&depositor, deposit); - if let Remove = - Self::dead_account(id.clone(), &who, &mut details, &account.reason, false)? - { - Account::::remove(&id, &who); - } else { - debug_assert!(false, "refund did not result in dead account?!"); - // deposit may have been refunded, need to update `Account` - Account::::insert(&id, &who, account); - return Ok(()) - } - Asset::::insert(&id, details); - // Executing a hook here is safe, since it is not in a `mutate`. - T::Freezer::died(id.clone(), &who); - T::Holder::died(id, &who); + if let Remove = Self::dead_account(&who, &mut details, &account.reason, false) { + Account::::remove(&id, &who); + } else { + debug_assert!(false, "refund did not result in dead account?!"); + // deposit may have been refunded, need to update `Account` + Account::::insert(&id, &who, account); return Ok(()) - }) + } + Asset::::insert(&id, details); + // Executing a hook here is safe, since it is not in a `mutate`. + T::Freezer::died(id.clone(), &who); + T::Holder::died(id, &who); + return Ok(()) } /// Increases the asset `id` balance of `beneficiary` by `amount`. @@ -598,13 +593,7 @@ impl, I: 'static> Pallet { account.balance = account.balance.saturating_sub(actual); if account.balance < details.min_balance { debug_assert!(account.balance.is_zero(), "checked in prep; qed"); - target_died = Some(Self::dead_account( - id.clone(), - target, - details, - &account.reason, - false, - )?); + target_died = Some(Self::dead_account(target, details, &account.reason, false)); if let Some(Remove) = target_died { return Ok(()) } @@ -687,6 +676,8 @@ impl, I: 'static> Pallet { return Ok(()) } + Self::ensure_account_can_die(id.clone(), &who)?; + // Burn any dust if needed. if let Some(burn) = maybe_burn { // Debit dust from supply; this will not saturate since it's already checked in @@ -699,48 +690,41 @@ impl, I: 'static> Pallet { debug_assert!(source_account.balance >= debit, "checked in prep; qed"); source_account.balance = source_account.balance.saturating_sub(debit); - with_storage_layer(|| { - Account::::try_mutate(&id, &dest, |maybe_account| -> DispatchResult { - match maybe_account { - Some(ref mut account) => { - // Calculate new balance; this will not saturate since it's already - // checked in prep. - debug_assert!( - account.balance.checked_add(&credit).is_some(), - "checked in prep; qed" - ); - account.balance.saturating_accrue(credit); - }, - maybe_account @ None => { - *maybe_account = Some(AssetAccountOf:: { - balance: credit, - status: AccountStatus::Liquid, - reason: Self::new_account(dest, details, None)?, - extra: T::Extra::default(), - }); - }, - } - Ok(()) - })?; - - // 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"); - source_died = Some(Self::dead_account( - id.clone(), - source, - details, - &source_account.reason, - false, - )?); - if let Some(Remove) = source_died { - Account::::remove(&id, &source); - return Ok(()) - } + Account::::try_mutate(&id, &dest, |maybe_account| -> DispatchResult { + match maybe_account { + Some(ref mut account) => { + // Calculate new balance; this will not saturate since it's already + // checked in prep. + debug_assert!( + account.balance.checked_add(&credit).is_some(), + "checked in prep; qed" + ); + account.balance.saturating_accrue(credit); + }, + maybe_account @ None => { + *maybe_account = Some(AssetAccountOf:: { + balance: credit, + status: AccountStatus::Liquid, + reason: Self::new_account(dest, details, None)?, + extra: T::Extra::default(), + }); + }, } - Account::::insert(&id, &source, &source_account); Ok(()) - }) + })?; + + // 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"); + source_died = + Some(Self::dead_account(source, details, &source_account.reason, false)); + if let Some(Remove) = source_died { + Account::::remove(&id, &source); + return Ok(()) + } + } + Account::::insert(&id, &source, &source_account); + Ok(()) })?; Self::deposit_event(Event::Transferred { @@ -831,31 +815,31 @@ impl, I: 'static> Pallet { let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; // Should only destroy accounts while the asset is in a destroying state ensure!(details.status == AssetStatus::Destroying, Error::::IncorrectStatus); - with_storage_layer(|| { - for (i, (who, mut v)) in Account::::iter_prefix(&id).enumerate() { - // unreserve the existence deposit if any - if let Some((depositor, deposit)) = v.reason.take_deposit_from() { - T::Currency::unreserve(&depositor, deposit); - } else if let Some(deposit) = v.reason.take_deposit() { - T::Currency::unreserve(&who, deposit); - } - if let Remove = - Self::dead_account(id.clone(), &who, &mut details, &v.reason, false)? - { - Account::::remove(&id, &who); - dead_accounts.push(who); - } else { - // deposit may have been released, need to update `Account` - Account::::insert(&id, &who, v); - defensive!("destroy did not result in dead account?!"); - } - if i + 1 >= (max_items as usize) { - break - } + + for (i, (who, mut v)) in Account::::iter_prefix(&id).enumerate() { + if Self::ensure_account_can_die(id.clone(), &who).is_err() { + continue + } + // unreserve the existence deposit if any + if let Some((depositor, deposit)) = v.reason.take_deposit_from() { + T::Currency::unreserve(&depositor, deposit); + } else if let Some(deposit) = v.reason.take_deposit() { + T::Currency::unreserve(&who, deposit); + } + if let Remove = Self::dead_account(&who, &mut details, &v.reason, false) { + Account::::remove(&id, &who); + dead_accounts.push(who); + } else { + // deposit may have been released, need to update `Account` + Account::::insert(&id, &who, v); + defensive!("destroy did not result in dead account?!"); } - remaining_accounts = details.accounts; - Ok(()) - }) + if i + 1 >= (max_items as usize) { + break + } + } + remaining_accounts = details.accounts; + Ok(()) })?; for who in &dead_accounts {