Skip to content

Commit

Permalink
Fix reentrancy of FrozenBalance::died hook (paritytech#10473)
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
ggwpez authored and ark0f committed Feb 27, 2023
1 parent c692eef commit 859604f
Show file tree
Hide file tree
Showing 4 changed files with 148 additions and 53 deletions.
136 changes: 89 additions & 47 deletions frame/assets/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand All @@ -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(
Expand Down Expand Up @@ -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(())
}

Expand Down Expand Up @@ -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)?;
Expand All @@ -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(())
}
};
Expand All @@ -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)
}

Expand All @@ -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 {
Expand All @@ -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.
Expand All @@ -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)?;
Expand Down Expand Up @@ -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(())
}
Expand All @@ -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.
Expand Down Expand Up @@ -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);

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 mut dead_accounts: Vec<T::AccountId> = vec![];

let metadata = Metadata::<T, I>::take(&id);
T::Currency::unreserve(
&details.owner,
details.deposit.saturating_add(metadata.deposit),
);
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);

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'
Expand Down Expand Up @@ -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 {
Expand All @@ -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);
Expand All @@ -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(())
}

Expand Down
10 changes: 10 additions & 0 deletions frame/assets/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,19 +120,27 @@ impl FrozenBalance<u32, u64, u64> for TestFreezer {

fn died(asset: u32, who: &u64) {
HOOKS.with(|h| h.borrow_mut().push(Hook::Died(asset, who.clone())));
// Sanity check: dead accounts have no balance.
assert!(Assets::balance(asset, *who).is_zero());
}
}

pub(crate) fn set_frozen_balance(asset: u32, who: u64, amount: u64) {
FROZEN.with(|f| f.borrow_mut().insert((asset, who), amount));
}

pub(crate) fn clear_frozen_balance(asset: u32, who: u64) {
FROZEN.with(|f| f.borrow_mut().remove(&(asset, who)));
}

pub(crate) fn hooks() -> Vec<Hook> {
HOOKS.with(|h| h.borrow().clone())
}

pub(crate) fn take_hooks() -> Vec<Hook> {
HOOKS.with(|h| h.take())
}

pub(crate) fn new_test_ext() -> sp_io::TestExternalities {
let mut storage = frame_system::GenesisConfig::default().build_storage::<Test>().unwrap();

Expand All @@ -154,6 +162,8 @@ pub(crate) fn new_test_ext() -> sp_io::TestExternalities {
config.assimilate_storage(&mut storage).unwrap();

let mut ext: sp_io::TestExternalities = storage.into();
// Clear thread local vars for https://github.com/paritytech/substrate/issues/10479.
ext.execute_with(|| take_hooks());
ext.execute_with(|| System::set_block_number(1));
ext
}
47 changes: 47 additions & 0 deletions frame/assets/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,21 @@ fn refunding_asset_deposit_without_burn_should_work() {
});
}

/// Refunding reaps an account and calls the `FrozenBalance::died` hook.
#[test]
fn refunding_calls_died_hook() {
new_test_ext().execute_with(|| {
assert_ok!(Assets::force_create(Origin::root(), 0, 1, false, 1));
Balances::make_free_balance_be(&1, 100);
assert_ok!(Assets::touch(Origin::signed(1), 0));
assert_ok!(Assets::mint(Origin::signed(1), 0, 1, 100));
assert_ok!(Assets::refund(Origin::signed(1), 0, true));

assert_eq!(Asset::<Test>::get(0).unwrap().accounts, 0);
assert_eq!(hooks(), vec![Hook::Died(0, 1)]);
});
}

#[test]
fn approval_lifecycle_works() {
new_test_ext().execute_with(|| {
Expand Down Expand Up @@ -389,19 +404,32 @@ fn min_balance_should_work() {
);

// When deducting from an account to below minimum, it should be reaped.
// Death by `transfer`.
assert_ok!(Assets::transfer(Origin::signed(1), 0, 2, 91));
assert!(Assets::maybe_balance(0, 1).is_none());
assert_eq!(Assets::balance(0, 2), 100);
assert_eq!(Asset::<Test>::get(0).unwrap().accounts, 1);
assert_eq!(take_hooks(), vec![Hook::Died(0, 1)]);

// Death by `force_transfer`.
assert_ok!(Assets::force_transfer(Origin::signed(1), 0, 2, 1, 91));
assert!(Assets::maybe_balance(0, 2).is_none());
assert_eq!(Assets::balance(0, 1), 100);
assert_eq!(Asset::<Test>::get(0).unwrap().accounts, 1);
assert_eq!(take_hooks(), vec![Hook::Died(0, 2)]);

// Death by `burn`.
assert_ok!(Assets::burn(Origin::signed(1), 0, 1, 91));
assert!(Assets::maybe_balance(0, 1).is_none());
assert_eq!(Asset::<Test>::get(0).unwrap().accounts, 0);
assert_eq!(take_hooks(), vec![Hook::Died(0, 1)]);

// Death by `transfer_approved`.
assert_ok!(Assets::mint(Origin::signed(1), 0, 1, 100));
Balances::make_free_balance_be(&1, 1);
assert_ok!(Assets::approve_transfer(Origin::signed(1), 0, 2, 100));
assert_ok!(Assets::transfer_approved(Origin::signed(2), 0, 1, 3, 91));
assert_eq!(take_hooks(), vec![Hook::Died(0, 1)]);
});
}

Expand Down Expand Up @@ -448,6 +476,7 @@ fn transferring_enough_to_kill_source_when_keep_alive_should_fail() {
assert_ok!(Assets::transfer_keep_alive(Origin::signed(1), 0, 2, 90));
assert_eq!(Assets::balance(0, 1), 10);
assert_eq!(Assets::balance(0, 2), 90);
assert!(hooks().is_empty());
});
}

Expand Down Expand Up @@ -684,6 +713,24 @@ fn set_metadata_should_work() {
});
}

/// Destroying an asset calls the `FrozenBalance::died` hooks of all accounts.
#[test]
fn destroy_calls_died_hooks() {
new_test_ext().execute_with(|| {
assert_ok!(Assets::force_create(Origin::root(), 0, 1, true, 50));
// Create account 1 and 2.
assert_ok!(Assets::mint(Origin::signed(1), 0, 1, 100));
assert_ok!(Assets::mint(Origin::signed(1), 0, 2, 100));
// Destroy the asset.
let w = Asset::<Test>::get(0).unwrap().destroy_witness();
assert_ok!(Assets::destroy(Origin::signed(1), 0, w));

// Asset is gone and accounts 1 and 2 died.
assert!(Asset::<Test>::get(0).is_none());
assert_eq!(hooks(), vec![Hook::Died(0, 1), Hook::Died(0, 2)]);
})
}

#[test]
fn freezer_should_work() {
new_test_ext().execute_with(|| {
Expand Down
8 changes: 2 additions & 6 deletions frame/assets/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,13 +172,9 @@ pub trait FrozenBalance<AssetId, AccountId, Balance> {
/// If `None` is returned, then nothing special is enforced.
fn frozen_balance(asset: AssetId, who: &AccountId) -> Option<Balance>;

/// Called when an account has been removed.
/// Called after an account has been removed.
///
/// # Warning
///
/// This function must never access storage of pallet asset. This function is called while some
/// change are pending. Calling into the pallet asset in this function can result in unexpected
/// state.
/// NOTE: It is possible that the asset does no longer exist when this hook is called.
fn died(asset: AssetId, who: &AccountId);
}

Expand Down

0 comments on commit 859604f

Please sign in to comment.