Payments bugs v12

[illuzen]

Fix V12 Audit Items: Assets Pallet Security Fixes

Summary

This PR addresses several security and correctness issues in the assets pallet identified during the V12 audit review.

Changes

1. Fix ExtraMutator Drop Not Committing in Release Builds

File: pallets/assets/src/extra_mutator.rs

The Drop implementation for ExtraMutator placed the self.commit() call inside debug_assert!, which meant release builds would silently discard pending sidecar mutations. The fix separates the commit from the assertion:

// Before (buggy)
fn drop(&mut self) {
    debug_assert!(self.commit().is_ok(), "...");
}

// After (fixed)
fn drop(&mut self) {
    let result = self.commit();
    debug_assert!(result.is_ok(), "...");
}

2. Prevent Zero-Amount Mint/Burn Event Emission Bypass

File: pallets/assets/src/functions.rs

Zero-amount mint and burn requests bypassed role checks (issuer/admin) because the low-level increase_balance/decrease_balance functions return early for zero amounts, but do_mint/do_burn still emitted privileged events afterward.

Added early returns in do_mint and do_burn for zero amounts to prevent:

• Emitting Issued/Burned events without permission checks
• Forging privileged lifecycle events for non-existent assets

3. Emit Missing TransferredApproved Event

File: pallets/assets/src/functions.rs

The transfer_approved dispatchable documented emission of TransferredApproved, but only the generic Transferred event was emitted (which lacks the delegate field). Added emission of TransferredApproved in do_transfer_approved to allow off-chain systems to identify delegated transfers.

4. Fix Incorrect owner Field in Created Event

File: pallets/assets/src/lib.rs

The Created event incorrectly used the admin parameter as the owner field instead of the actual owner from CreateOrigin. This allowed attackers to cause event consumers to observe arbitrary accounts as owners.

// Before (buggy)
Self::deposit_event(Event::Created {
    asset_id: id,
    creator: owner.clone(),
    owner: admin,  // Wrong!
});

// After (fixed)
Self::deposit_event(Event::Created {
    asset_id: id,
    creator: owner.clone(),
    owner,  // Correct
});

5. Fix Hard-Coded RocksDbWeight in set_reserves Weight

File: pallets/assets/src/weights.rs

SubstrateWeight<T>::set_reserves was the only weight function using hard-coded RocksDbWeight instead of T::DbWeight. This caused incorrect weight calculations for runtimes with different database backends.

Testing

All existing tests pass:

cargo test -p pallet-assets
cargo test -p pallet-assets-holder

Impact

Issue	Severity	Impact
ExtraMutator Drop	Medium	Sidecar state corruption in release builds
Zero-amount events	Low	Event forgery for indexers/monitoring
Missing TransferredApproved	Low	Delegated transfers indistinguishable from owner transfers
Wrong Created.owner	Low	Misleading ownership data for off-chain consumers
RocksDbWeight	Low	Fee undercollection on non-RocksDB runtimes

---

Note

Medium Risk
Touches core asset supply/refund/mint paths and transaction fee validation; incorrect behavior could affect issuance accounting or tx acceptance, but changes are targeted fixes with tests.

Overview
Addresses V12 audit findings across assets, transaction-payment, and minor balances formatting.

Assets — correctness & security: ExtraMutator Drop now always runs commit() in release builds (was only inside debug_assert!). do_refund with allow_burn decrements supply, emits Burned, and keeps totals aligned. do_mint / do_burn return early on zero amount so issuer/admin checks are not skipped while events still fire. set_min_balance rejects zero (MinBalanceZero) like create paths. Created event owner is the create-origin owner, not the admin param. do_transfer_approved emits TransferredApproved (delegate visible to indexers). Weight docs/fixes: NextAssetId counted as written on create/force_create; set_reserves uses T::DbWeight instead of hard-coded RocksDB.

Transaction payment: Fee validation mirrors keep-alive rules so pool checks do not pass when prepare would kill the account below ED.

Regression tests cover refund+burn supply and zero min_balance.

^{Reviewed by Cursor Bugbot for commit 151abd0. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 151abd0. Configure here.}

[cursor]

Zero transfer emits approval event

Low Severity

do_transfer_approved always deposits TransferredApproved after success, but transfer_and_die returns immediately when amount is zero without moving tokens or emitting Transferred. Indexers can record a delegated transfer that never happened, inconsistent with the new zero-amount guards on do_mint and do_burn.

Additional Locations (1)

• pallets/assets/src/functions.rs#L997-L1019

 

^{Reviewed by Cursor Bugbot for commit 151abd0. Configure here.}

[n13]

Review verdict: Request changes

Reviewed at 151abd0. I checked out the branch and ran it locally: cargo test -p pallet-assets passes all 92 tests (including the two new regression tests), and pallet-balances + pallet-transaction-payment compile clean.

Most of this is solid, audit-grade work. There's one confirmed (low) bug and one fix that doesn't do what the description claims — details below.

Correct and good as-is

• extra_mutator.rs Drop — moving commit() out of debug_assert! so release builds actually persist sidecar mutations. Correct.
• do_refund supply / Burned — verified dead_account() does not touch details.supply (only accounts/sufficients), so decrementing supply by account.balance here is correct with no double-count. Good catch on the phantom-issuance bug.
• do_mint / do_burn zero guards — return values stay consistent (Ok(()) / Ok(0), matching decrease_balance). Fixes the event-emission-without-permission-check.
• Created.owner — now the create-origin owner instead of admin. Correct.
• set_min_balance rejects zero — consistent with the existing create/force_create invariant (pallets/assets/src/lib.rs:775).
• balances::burn weight — the original mapping was inverted; if keep_alive { burn_keep_alive } else { burn_allow_death } is the correct mapping.
• weights.rs::set_reserves — SubstrateWeight<T> now uses T::DbWeight while impl WeightInfo for () correctly keeps RocksDbWeight. Correct.

1. Transaction-payment fix is on dead code — no runtime effect (please address)

The keep-alive mirror was added to CurrencyAdapter::can_withdraw_fee, but:

• The runtime wires OnChargeTransaction = FungibleAdapter<...> (runtime/src/configs/mod.rs:448-449).
• CurrencyAdapter is #[deprecated] and is referenced nowhere in this repo except its own definition — not in the runtime, tests, mocks, or benchmarks.
• FungibleAdapter::can_withdraw_fee already rejects the kill case: it only accepts WithdrawConsequence::Success (pallets/transaction-payment/src/payment.rs:163-166), which already excludes accounts that would drop below ED — consistent with withdraw_fee's Preservation::Preserve (payment.rs:140).

So this hunk has zero effect on the chain, and the description's "pool checks do not pass when prepare would kill the account" doesn't apply to the live path. Recommendation: drop the change (and note that FungibleAdapter is already correct), or if you believe there is a real gap, demonstrate it against FungibleAdapter and fix there. Minor aside: even within CurrencyAdapter the mirror compares only free_balance, whereas pallet-balances' withdraw keep-alive uses total balance (free + reserved), so it could over-reject.

2. Zero-amount do_transfer_approved emits TransferredApproved (confirms Bugbot — Low)

pallets/assets/src/functions.rs:1045-1051 deposits TransferredApproved unconditionally, but transfer_and_die returns early for amount == 0 (functions.rs:646-648) without moving funds or emitting Transferred. This is exactly the inconsistency you guarded against in do_mint/do_burn. Add the same guard — e.g. early-return at the top of do_transfer_approved when amount.is_zero() (a zero approved-spend is a no-op), or gate the event on !amount.is_zero().

Minor / notes

• create/force_create weights are hand-edited (NextAssetId w:0 → w:1, +1 write). Direction is right — AutoIncAssetId::created writes NextAssetId (pallets/assets/src/lib.rs:245) — but that write only happens when auto-increment is enabled (NextAssetId present), so this is the conservative worst-case value. Prefer regenerating via the benchmark if practical.
• The new zero-amount guards in do_mint/do_burn short-circuit before the asset existence/status checks, so a zero-amount op on an unknown/non-live asset now returns Ok instead of Unknown/IncorrectStatus. Benign (no-op), just flagging the behavior change.

Happy to re-review once #1 and #2 are addressed.

[n13]

Review verdict: Approve

Re-reviewed 151abd0 → 6a1d2035. Both requested changes are resolved, and cargo test -p pallet-assets still passes all 92 tests.

1. Dead-code transaction-payment fix — resolved. CurrencyAdapter::can_withdraw_fee is fully reverted; pallets/transaction-payment/src/payment.rs now has zero net diff vs main. The live FungibleAdapter path was already correct, so there's no behavior change on the runtime — which is the right outcome.
2. Zero-amount do_transfer_approved — resolved. Clean if amount.is_zero() { return Ok(()) } guard at the top of do_transfer_approved (pallets/assets/src/functions.rs:1005-1011), matching the do_mint/do_burn/transfer_and_die pattern. A zero-value delegated transfer is now a complete no-op (no approval consumed, no TransferredApproved event) — consistent with the rest of the PR and benign.

The remaining changes (refund supply/Burned accounting, mint/burn zero guards, Created.owner, set_min_balance, extra_mutator Drop, balances::burn weight, set_reserves/NextAssetId weights) were verified correct in the prior pass and are unchanged here.

LGTM. One non-blocking note: the branch is behind main (#607/#608 not present), so a merge/rebase will be needed before merging.