Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions prdoc/pr_11441.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
title: '[pallet-assets] fix: decrement supply when refund burns balance'
doc:
- audience: Runtime Dev
description: |-
When a user calls `refund` with `allow_burn = true`, their token balance is destroyed, but the asset's total supply was never updated. This caused `total_issuance()` to overcount. The fix decrements supply and emits a `Burned` event, consistent with how every other burn path works.

In production, burning path is rarely triggered. The fungibles trait interface always passes `allow_burn = false`, so only users manually submitting the refund extrinsic with the burn flag would hit it.

Follow-up issue for migrating the discrepancy (observed on Westend): https://github.com/paritytech/polkadot-sdk/issues/11443.

Fixes #10412
crates:
- name: pallet-assets
bump: patch
12 changes: 12 additions & 0 deletions substrate/frame/assets/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,10 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
}

if let Remove = Self::dead_account(&who, &mut details, &account.reason, false) {
if !account.balance.is_zero() {
debug_assert!(details.supply >= account.balance, "supply < balance; qed");
details.supply = details.supply.saturating_sub(account.balance);
}
Account::<T, I>::remove(&id, &who);
} else {
debug_assert!(false, "refund did not result in dead account?!");
Expand All @@ -392,6 +396,14 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
return Ok(());
}

if !account.balance.is_zero() {
Self::deposit_event(Event::Burned {
asset_id: id.clone(),
owner: who.clone(),
balance: account.balance,
});
}

Asset::<T, I>::insert(&id, details);
// Executing a hook here is safe, since it is not in a `mutate`.
T::Freezer::died(id.clone(), &who);
Expand Down
9 changes: 5 additions & 4 deletions substrate/frame/assets/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2024,10 +2024,11 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
}
}

// Using >= instead of == because the provided `do_refund` implementation destroys
// the account balance without decrementing the asset supply. This causes the
// tracked supply to permanently exceed the actual sum of balances + holds
// whenever a refund occurs.
// Using >= instead of == because the provided `do_refund` implementation
// historically destroyed the account balance without decrementing the asset
// supply. Although this has been fixed, existing on-chain state may still
// contain overcounted supply from prior refunds.
// TODO: add a migration to recalculate supply, then tighten this to `==`.
ensure!(details.supply >= calculated_supply, "Asset supply mismatch");
ensure!(details.accounts == calculated_accounts, "Asset account count mismatch");
ensure!(
Expand Down
6 changes: 5 additions & 1 deletion substrate/frame/assets/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,11 @@ pub(crate) fn set_balance_on_hold(asset: u32, who: u64, amount: u64) {

pub(crate) fn clear_balance_on_hold(asset: u32, who: u64) {
OnHold::mutate(|v| {
v.remove(&(asset, who));
if let Some(amount) = v.remove(&(asset, who)) {
if amount > 0 {
assert_ok!(Assets::increase_balance(asset, &who, amount, |_| Ok(())));
}
}
});
}
pub struct TestFreezer;
Expand Down
26 changes: 26 additions & 0 deletions substrate/frame/assets/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,35 @@ fn refunding_asset_deposit_with_burn_should_work() {
Balances::make_free_balance_be(&1, 100);
assert_ok!(Assets::touch(RuntimeOrigin::signed(1), 0));
assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
assert_eq!(Assets::total_supply(0), 100);
assert_ok!(Assets::refund(RuntimeOrigin::signed(1), 0, true));
assert_eq!(Balances::reserved_balance(&1), 0);
assert_eq!(Assets::balance(1, 0), 0);
assert_eq!(Assets::total_supply(0), 0);
System::assert_last_event(RuntimeEvent::Assets(crate::Event::Burned {
asset_id: 0,
owner: 1,
balance: 100,
}));
});
}

#[test]
fn refund_with_zero_balance_does_not_emit_burned() {
build_and_execute(|| {
assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, false, 1));
Balances::make_free_balance_be(&1, 100);
assert_ok!(Assets::touch(RuntimeOrigin::signed(1), 0));
assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
assert_ok!(Assets::burn(RuntimeOrigin::signed(1), 0, 1, 100));
assert_eq!(Assets::total_supply(0), 0);

System::reset_events();
assert_ok!(Assets::refund(RuntimeOrigin::signed(1), 0, false));
assert_eq!(Assets::total_supply(0), 0);
assert!(System::events()
.iter()
.all(|e| !matches!(e.event, RuntimeEvent::Assets(crate::Event::Burned { .. }))));
});
}

Expand Down
Loading