Payments bugs v12

pallets/assets/src/extra_mutator.rs

@@ -34,7 +34,11 @@ pub struct ExtraMutator<T: Config<I>, I: 'static = ()> {
 
 impl<T: Config<I>, I: 'static> Drop for ExtraMutator<T, I> {
 	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");
 	}
 }
 

pallets/assets/src/functions.rs

@@ -382,6 +382,15 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			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::<T, I>::remove(&id, &who);
 		} else {
@@ -392,6 +401,16 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		}
 
 		Asset::<T, I>::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<T: Config<I>, I: 'static> Pallet<T, I> {
 		amount: T::Balance,
 		maybe_check_issuer: Option<T::AccountId>,
 	) -> 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::<T, I>::NoPermission);
@@ -525,6 +551,13 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		maybe_check_admin: Option<T::AccountId>,
 		f: DebitFlags,
 	) -> Result<T::Balance, DispatchError> {
+		// 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::<T, I>::get(&id).ok_or(Error::<T, I>::Unknown)?;
 		ensure!(
 			d.status == AssetStatus::Live || d.status == AssetStatus::Frozen,
@@ -969,6 +1002,13 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		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<DeadConsequence> = None;
 
 		let d = Asset::<T, I>::get(&id).ok_or(Error::<T, I>::Unknown)?;
@@ -1003,8 +1043,20 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		// 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(())
 	}
 

pallets/assets/src/lib.rs

@@ -799,11 +799,7 @@ pub mod pallet {
 				},
 			);
 			ensure!(T::CallbackHandle::created(&id, &owner).is_ok(), Error::<T, I>::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::<T, I>::get(&id).ok_or(Error::<T, I>::Unknown)?;
 			ensure!(origin == details.owner, Error::<T, I>::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::<T, I>::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.

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::<Test>::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::<Test>::MinBalanceZero
+		);
+
+		// Verify min_balance is unchanged
+		assert_eq!(Asset::<Test>::get(id).unwrap().min_balance, 30);
+	});
+}
+
 #[test]
 fn balance_conversion_should_work() {
 	new_test_ext().execute_with(|| {

pallets/assets/src/weights.rs

@@ -120,31 +120,35 @@ pub struct SubstrateWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	/// 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`
 		//  Estimated: `3675`
 		// 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`
 		//  Estimated: `3675`
 		// 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,40 +618,44 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 		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))
 	}
 }
 
 // For backwards compatibility and tests.
 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`
 		//  Estimated: `3675`
 		// 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`
 		//  Estimated: `3675`
 		// 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`)

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<T>,
 			#[pallet::compact] value: T::Balance,