Unify and correct the `Spend` allowance event across `transferFrom`, `burnFrom` and `forcedTransfer`

[rya-sge]

Unify the Spend allowance event between transferFrom and burnFrom

Labels: enhancement, breaking-change, events
Affected contracts: ERC20BaseModule, ERC20CrossChainModule, IERC20Allowance
Origin: Nethermind AuditAgent v3.3.0-rc2 — finding NM-24 (Best Practices → Informational).

Scope note: the documentation half of NM-24 (corrected IERC20Allowance.Spend NatSpec,
doc/technical/allowance-spend-event.md) and the forcedTransfer allowance-reduction event have already been
shipped. This issue tracks only the remaining breaking change: making the transferFrom and burnFrom emit
sites consistent with each other.

Summary

CMTAT emits a custom IERC20Allowance.Spend(account, spender, value) event when a spender consumes an owner's
allowance. The two remaining emit sites — transferFrom and burnFrom — are not consistent: they differ in event
ordering, and both emit even when the allowance is infinite (and therefore not reduced). An integrator that
reconstructs allowances from events instead of reading allowance() over-reports the reduction after an
infinite-approval spend.

This does not affect funds or transfer outcomes — it is an events / off-chain-accounting concern only. The unified
fix is a breaking change deferred to a major version.

Current behaviour

// ERC20BaseModule.transferFrom — emits AFTER the transfer, order: Transfer -> Spend
bool result = ERC20Upgradeable.transferFrom(from, to, value);
if (result) {
    emit Spend(from, _msgSender(), value);
}

// ERC20CrossChainModule._burnFrom — emits BETWEEN spend and burn, order: Spend -> Transfer -> BurnFrom
ERC20Upgradeable._spendAllowance(account, sender, value);
emit IERC20Allowance.Spend(account, sender, value);
_burnFromOperator(sender, account, value);

Path	Allowance reduced?	Spend emitted?	Receipt order	Net effect for event-only accounting
transferFrom, finite	Yes	Yes	Transfer → Spend	correct
transferFrom, infinite	No	Yes	Transfer → Spend	over-reports the reduction
burnFrom, finite	Yes	Yes	Spend → Transfer → BurnFrom	correct
burnFrom, infinite	No	Yes	Spend → Transfer → BurnFrom	over-reports the reduction
burn(uint256) self-burn	n/a	No	Transfer → BurnFrom	correct (no allowance)

The problems

1. Ordering is inconsistent. transferFrom emits Spend after Transfer; burnFrom emits it before.
   An indexer that pairs a Spend with the following Transfer mis-associates them on one of the two paths.
2. Spend fires on infinite approvals. When the allowance is type(uint256).max, OpenZeppelin does not
   reduce it, but both paths still emit Spend. Subtracting value from a cached allowance therefore drifts too
   low.

(The IERC20Allowance.Spend NatSpec, which previously claimed no event is emitted for infinite allowances, has
already been corrected to match the current code, and the durable guidance — reconstruct allowances from
allowance(), never by summing Spend/Approval — is already documented. This issue is about consistency,
not correctness.)

Proposed change — unify via a single _spendAllowance override

Both paths already route their allowance spend through OpenZeppelin's ERC20Upgradeable._spendAllowance(...),
which reduces the allowance only when finite. Emit Spend there, once, and drop the two explicit emits:

function _spendAllowance(address owner, address spender, uint256 value) internal virtual override {
    if (allowance(owner, spender) != type(uint256).max) {
        emit IERC20Allowance.Spend(owner, spender, value);
    }
    super._spendAllowance(owner, spender, value);
}

Result: one emit site, one ordering (Spend → Transfer on both paths), and Spend no longer fires for infinite
approvals — so both problems close, and the event finally means what an integrator would expect.

Drawbacks / why this is not a patch

• Observable behaviour change. Spend stops firing for infinite-allowance spends, and the transferFrom
  receipt order flips from Transfer → Spend to Spend → Transfer. Any existing indexer keyed on either breaks.
  → major version only.
• Gas on the hot path. The allowance(...) read inside the override is an extra SLOAD on every
  transferFrom, unless _spendAllowance is fully re-implemented to reuse the value OpenZeppelin already loads —
  which means maintaining a fork of the audited body.
• Contract size. Several deployment variants (HolderList, ERC1363, DebtEngine) are within a few hundred
  bytes of the EIP-170 24 KiB limit. The net bytecode delta (one override added, two emits removed) must be
  measured per variant before merging; it may not fit everywhere.

Cheaper alternative (ordering only)

Move burnFrom's emit Spend to after _burnFromOperator, giving Transfer → BurnFrom → Spend so Spend is
last on both paths. Fixes problem 1 with no gas cost, but leaves problem 2 untouched, so it does not remove the
substantive inconsistency and is not worth a breaking change on its own.

Recommendation

• Now: nothing further — the shipped documentation already resolves the integrator-facing risk.
• Next major version, if consistency is wanted: the _spendAllowance unification above, gated on a per-variant
  contract-size check and a measurement of the extra SLOAD on transferFrom.

Acceptance criteria (if implemented)

• Spend emitted from a single site for both transferFrom and burnFrom, identical ordering
• Spend not emitted when the allowance is infinite
• Deployed size re-measured for HolderList / ERC1363 / DebtEngine against the 24 576-byte limit
• Gas delta on transferFrom measured and accepted
• IERC20Allowance.Spend NatSpec and doc/technical/allowance-spend-event.md updated to the new behaviour
• Tests: event presence/absence and ordering for finite transferFrom, infinite transferFrom, finite
  burnFrom, infinite burnFrom, and self-burn
• CHANGELOG notes the breaking event change