diff --git a/pallets/assets/src/extra_mutator.rs b/pallets/assets/src/extra_mutator.rs index e2f7742c..7de59d54 100644 --- a/pallets/assets/src/extra_mutator.rs +++ b/pallets/assets/src/extra_mutator.rs @@ -34,7 +34,11 @@ pub struct ExtraMutator, I: 'static = ()> { impl, I: 'static> Drop for ExtraMutator { fn drop(&mut self) { - debug_assert!(self.commit().is_ok(), "attempt to write to non-existent asset account"); + // Always commit pending changes, not just in debug builds. + // The commit() call was previously inside debug_assert!, which meant + // release builds would silently discard uncommitted sidecar mutations. + let result = self.commit(); + debug_assert!(result.is_ok(), "attempt to write to non-existent asset account"); } } diff --git a/pallets/assets/src/functions.rs b/pallets/assets/src/functions.rs index 9930b19b..b82b22d6 100644 --- a/pallets/assets/src/functions.rs +++ b/pallets/assets/src/functions.rs @@ -382,6 +382,15 @@ impl, I: 'static> Pallet { T::Currency::unreserve(&who, deposit); } + // If allow_burn is true and account has a non-zero balance, we must decrement + // the total supply to maintain accounting invariants. Otherwise total_supply + // would report phantom issuance that no longer corresponds to any live balances. + let burned = account.balance; + if !burned.is_zero() { + debug_assert!(details.supply >= burned, "account balance exceeds total supply"); + details.supply = details.supply.saturating_sub(burned); + } + if let Remove = Self::dead_account(&who, &mut details, &account.reason, false) { Account::::remove(&id, &who); } else { @@ -392,6 +401,16 @@ impl, I: 'static> Pallet { } Asset::::insert(&id, details); + + // Emit Burned event if we burned a non-zero balance + if !burned.is_zero() { + Self::deposit_event(Event::Burned { + asset_id: id.clone(), + owner: who.clone(), + balance: burned, + }); + } + // Executing a hook here is safe, since it is not in a `mutate`. T::Freezer::died(id.clone(), &who); T::Holder::died(id, &who); @@ -447,6 +466,13 @@ impl, I: 'static> Pallet { amount: T::Balance, maybe_check_issuer: Option, ) -> DispatchResult { + // Early return for zero amounts - don't emit events or bypass permission checks. + // Without this, zero-amount mints would skip the issuer check in increase_balance's + // callback (which returns early for zero) but still emit the Issued event. + if amount.is_zero() { + return Ok(()) + } + Self::increase_balance(id.clone(), beneficiary, amount, |details| -> DispatchResult { if let Some(check_issuer) = maybe_check_issuer { ensure!(check_issuer == details.issuer, Error::::NoPermission); @@ -525,6 +551,13 @@ impl, I: 'static> Pallet { maybe_check_admin: Option, f: DebitFlags, ) -> Result { + // Early return for zero amounts - don't emit events or bypass permission checks. + // Without this, zero-amount burns would skip the admin check in decrease_balance's + // callback (which returns early for zero) but still emit the Burned event. + if amount.is_zero() { + return Ok(amount) + } + let d = Asset::::get(&id).ok_or(Error::::Unknown)?; ensure!( d.status == AssetStatus::Live || d.status == AssetStatus::Frozen, @@ -969,6 +1002,13 @@ impl, I: 'static> Pallet { destination: &T::AccountId, amount: T::Balance, ) -> DispatchResult { + // Early return for zero amounts - don't emit events or consume approvals. + // Without this, zero-amount transfers would emit TransferredApproved even though + // transfer_and_die returns early without moving tokens or emitting Transferred. + if amount.is_zero() { + return Ok(()) + } + let mut owner_died: Option = None; let d = Asset::::get(&id).ok_or(Error::::Unknown)?; @@ -1003,8 +1043,20 @@ impl, I: 'static> Pallet { // Execute hook outside of `mutate`. if let Some(Remove) = owner_died { T::Freezer::died(id.clone(), owner); - T::Holder::died(id, owner); + T::Holder::died(id.clone(), owner); } + + // Emit TransferredApproved to identify the delegate responsible for the spend. + // Note: transfer_and_die already emits Event::Transferred, but that event doesn't + // include the delegate. This event allows indexers/monitoring to track delegated activity. + Self::deposit_event(Event::TransferredApproved { + asset_id: id, + owner: owner.clone(), + delegate: delegate.clone(), + destination: destination.clone(), + amount, + }); + Ok(()) } diff --git a/pallets/assets/src/lib.rs b/pallets/assets/src/lib.rs index b91a54d7..106989ab 100644 --- a/pallets/assets/src/lib.rs +++ b/pallets/assets/src/lib.rs @@ -799,11 +799,7 @@ pub mod pallet { }, ); ensure!(T::CallbackHandle::created(&id, &owner).is_ok(), Error::::CallbackFailed); - Self::deposit_event(Event::Created { - asset_id: id, - creator: owner.clone(), - owner: admin, - }); + Self::deposit_event(Event::Created { asset_id: id, creator: owner.clone(), owner }); Ok(()) } @@ -1706,6 +1702,11 @@ pub mod pallet { let mut details = Asset::::get(&id).ok_or(Error::::Unknown)?; ensure!(origin == details.owner, Error::::NoPermission); + // Reject zero min_balance to maintain the invariant enforced by create/force_create. + // A zero min_balance would allow zero-balance accounts to persist (since the reaping + // check is `balance < min_balance`), enabling consumer reference griefing attacks. + ensure!(!min_balance.is_zero(), Error::::MinBalanceZero); + let old_min_balance = details.min_balance; // If the asset is marked as sufficient it won't be allowed to // change the min_balance. diff --git a/pallets/assets/src/tests.rs b/pallets/assets/src/tests.rs index aba8d19d..439e743c 100644 --- a/pallets/assets/src/tests.rs +++ b/pallets/assets/src/tests.rs @@ -1500,6 +1500,44 @@ fn calling_dead_account_fails_if_freezes_or_balances_on_hold_exist_2() { }) } +/// Regression test: refund with allow_burn=true must decrement total_supply. +/// +/// Previously, do_refund would destroy a non-zero balance account without +/// updating AssetDetails.supply, leaving phantom issuance in total_supply. +#[test] +fn refund_with_allow_burn_decrements_total_supply() { + new_test_ext().execute_with(|| { + // Create asset with admin=1, min_balance=1 + assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, false, 1)); + + // Give account 2 native balance for deposit + Balances::make_free_balance_be(&2, 100); + + // Account 2 touches the asset (creates deposit-held account) + assert_ok!(Assets::touch(RuntimeOrigin::signed(2), 0)); + + // Mint 50 tokens to account 2 + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 2, 50)); + + // Verify initial state + assert_eq!(Assets::total_supply(0), 50); + assert_eq!(Assets::balance(0, 2), 50); + + // Account 2 refunds with allow_burn=true, burning their 50 tokens + assert_ok!(Assets::refund(RuntimeOrigin::signed(2), 0, true)); + + // Key assertion: total_supply must be decremented by the burned amount + assert_eq!( + Assets::total_supply(0), + 0, + "total_supply should be 0 after burning 50 tokens via refund" + ); + + // Account should be gone + assert!(Account::::get(&0, &2).is_none()); + }); +} + /// Destroying an asset calls the `FrozenBalance::died` hooks of all accounts. #[test] fn destroy_accounts_calls_died_hooks() { @@ -1866,6 +1904,29 @@ fn set_min_balance_should_work() { }); } +/// Regression test: set_min_balance must reject zero to prevent consumer reference griefing. +/// +/// A zero min_balance would allow zero-balance accounts to persist (since the reaping check +/// is `balance < min_balance`, which is never true when min_balance is 0). An attacker could +/// then strand consumer references on victim accounts. +#[test] +fn set_min_balance_rejects_zero() { + new_test_ext().execute_with(|| { + let id = 42; + Balances::make_free_balance_be(&1, 10); + assert_ok!(Assets::create(RuntimeOrigin::signed(1), id, 1, 30)); + + // Attempting to set min_balance to zero should fail + assert_noop!( + Assets::set_min_balance(RuntimeOrigin::signed(1), id, 0), + Error::::MinBalanceZero + ); + + // Verify min_balance is unchanged + assert_eq!(Asset::::get(id).unwrap().min_balance, 30); + }); +} + #[test] fn balance_conversion_should_work() { new_test_ext().execute_with(|| { diff --git a/pallets/assets/src/weights.rs b/pallets/assets/src/weights.rs index 13c37ca5..8504d598 100644 --- a/pallets/assets/src/weights.rs +++ b/pallets/assets/src/weights.rs @@ -120,10 +120,12 @@ pub struct SubstrateWeight(PhantomData); impl WeightInfo for SubstrateWeight { /// Storage: `Assets::Asset` (r:1 w:1) /// Proof: `Assets::Asset` (`max_values`: None, `max_size`: Some(210), added: 2685, mode: `MaxEncodedLen`) - /// Storage: `Assets::NextAssetId` (r:1 w:0) + /// Storage: `Assets::NextAssetId` (r:1 w:1) /// Proof: `Assets::NextAssetId` (`max_values`: Some(1), `max_size`: Some(4), added: 499, mode: `MaxEncodedLen`) /// Storage: `System::Account` (r:1 w:1) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) + /// + /// Note: NextAssetId write accounts for AutoIncAssetId callback which increments the ID. fn create() -> Weight { // Proof Size summary in bytes: // Measured: `326` @@ -131,12 +133,14 @@ impl WeightInfo for SubstrateWeight { // Minimum execution time: 30_517_000 picoseconds. Weight::from_parts(31_518_000, 3675) .saturating_add(T::DbWeight::get().reads(3_u64)) - .saturating_add(T::DbWeight::get().writes(2_u64)) + .saturating_add(T::DbWeight::get().writes(3_u64)) } /// Storage: `Assets::Asset` (r:1 w:1) /// Proof: `Assets::Asset` (`max_values`: None, `max_size`: Some(210), added: 2685, mode: `MaxEncodedLen`) - /// Storage: `Assets::NextAssetId` (r:1 w:0) + /// Storage: `Assets::NextAssetId` (r:1 w:1) /// Proof: `Assets::NextAssetId` (`max_values`: Some(1), `max_size`: Some(4), added: 499, mode: `MaxEncodedLen`) + /// + /// Note: NextAssetId write accounts for AutoIncAssetId callback which increments the ID. fn force_create() -> Weight { // Proof Size summary in bytes: // Measured: `186` @@ -144,7 +148,7 @@ impl WeightInfo for SubstrateWeight { // Minimum execution time: 12_469_000 picoseconds. Weight::from_parts(13_002_000, 3675) .saturating_add(T::DbWeight::get().reads(2_u64)) - .saturating_add(T::DbWeight::get().writes(1_u64)) + .saturating_add(T::DbWeight::get().writes(2_u64)) } /// Storage: `Assets::Asset` (r:1 w:1) /// Proof: `Assets::Asset` (`max_values`: None, `max_size`: Some(210), added: 2685, mode: `MaxEncodedLen`) @@ -614,8 +618,8 @@ impl WeightInfo for SubstrateWeight { Weight::from_parts(31_972_000, 3675) // Standard Error: 13_748 .saturating_add(Weight::from_parts(198_975, 0).saturating_mul(n.into())) - .saturating_add(RocksDbWeight::get().reads(1_u64)) - .saturating_add(RocksDbWeight::get().writes(1_u64)) + .saturating_add(T::DbWeight::get().reads(1_u64)) + .saturating_add(T::DbWeight::get().writes(1_u64)) } } @@ -623,10 +627,12 @@ impl WeightInfo for SubstrateWeight { impl WeightInfo for () { /// Storage: `Assets::Asset` (r:1 w:1) /// Proof: `Assets::Asset` (`max_values`: None, `max_size`: Some(210), added: 2685, mode: `MaxEncodedLen`) - /// Storage: `Assets::NextAssetId` (r:1 w:0) + /// Storage: `Assets::NextAssetId` (r:1 w:1) /// Proof: `Assets::NextAssetId` (`max_values`: Some(1), `max_size`: Some(4), added: 499, mode: `MaxEncodedLen`) /// Storage: `System::Account` (r:1 w:1) /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) + /// + /// Note: NextAssetId write accounts for AutoIncAssetId callback which increments the ID. fn create() -> Weight { // Proof Size summary in bytes: // Measured: `326` @@ -634,12 +640,14 @@ impl WeightInfo for () { // Minimum execution time: 30_517_000 picoseconds. Weight::from_parts(31_518_000, 3675) .saturating_add(RocksDbWeight::get().reads(3_u64)) - .saturating_add(RocksDbWeight::get().writes(2_u64)) + .saturating_add(RocksDbWeight::get().writes(3_u64)) } /// Storage: `Assets::Asset` (r:1 w:1) /// Proof: `Assets::Asset` (`max_values`: None, `max_size`: Some(210), added: 2685, mode: `MaxEncodedLen`) - /// Storage: `Assets::NextAssetId` (r:1 w:0) + /// Storage: `Assets::NextAssetId` (r:1 w:1) /// Proof: `Assets::NextAssetId` (`max_values`: Some(1), `max_size`: Some(4), added: 499, mode: `MaxEncodedLen`) + /// + /// Note: NextAssetId write accounts for AutoIncAssetId callback which increments the ID. fn force_create() -> Weight { // Proof Size summary in bytes: // Measured: `186` @@ -647,7 +655,7 @@ impl WeightInfo for () { // Minimum execution time: 12_469_000 picoseconds. Weight::from_parts(13_002_000, 3675) .saturating_add(RocksDbWeight::get().reads(2_u64)) - .saturating_add(RocksDbWeight::get().writes(1_u64)) + .saturating_add(RocksDbWeight::get().writes(2_u64)) } /// Storage: `Assets::Asset` (r:1 w:1) /// Proof: `Assets::Asset` (`max_values`: None, `max_size`: Some(210), added: 2685, mode: `MaxEncodedLen`) diff --git a/pallets/balances/src/lib.rs b/pallets/balances/src/lib.rs index ffc34301..37cbf962 100644 --- a/pallets/balances/src/lib.rs +++ b/pallets/balances/src/lib.rs @@ -858,7 +858,7 @@ pub mod pallet { /// Unlike sending funds to a _burn_ address, which merely makes the funds inaccessible, /// this `burn` operation will reduce total issuance by the amount _burned_. #[pallet::call_index(10)] - #[pallet::weight(if *keep_alive {T::WeightInfo::burn_allow_death() } else {T::WeightInfo::burn_keep_alive()})] + #[pallet::weight(if *keep_alive { T::WeightInfo::burn_keep_alive() } else { T::WeightInfo::burn_allow_death() })] pub fn burn( origin: OriginFor, #[pallet::compact] value: T::Balance,