fix(precompiles): fail closed on native coin burn supply underflow

[crazywriter1]

Summary

• Mirror mint: replace saturating_sub on total supply with checked_sub + ERR_OVERFLOW so burn fails closed if supply ever underflows.
• Under the current Arc invariant (total_supply == sum(balances) at genesis, preserved by mint/burn/transfer and fee accounting), that underflow is unreachable after a successful balance_decr — so this is fail-closed hardening, not a live consensus bug, and is intentionally not hardfork-gated.
• The only known balance/supply skew in-tree (self-destructed beneficiary fee credit) drives supply above balances, which makes underflow less likely, not more.

Test plan

• cargo test -p arc-precompiles burn_reverts_when_total_supply_underflows
• cargo test -p arc-precompiles native_coin_authority_precompile_outputs

[kutluhaneth46]

Looks solid — mirroring mint's checked_add with checked_sub + fail-closed is the right invariant when supply can lag balances.

One small note for reviewers: reuse of ERR_OVERFLOW ("Arithmetic overflow") for the underflow path matches mint's error surface, so it's intentional rather than a misnamed constant. The test comment about not pinning gas_used (because balance_decr already ran) is a good catch.

No code change requested from me.

[osr21]

Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer. I have no write access to this repository, so any review state I set (approval or change request) carries no merge authority and is advisory only. Please treat this as one contributor's technical assessment, and defer to Circle maintainers for the binding review.

---

Reviewed the diff and traced the reachability question. The change is correct and I'd merge it — but as defence-in-depth, not as a bug fix. The failure mode in the description isn't reachable on any current Arc network, and I think that's worth establishing explicitly, because it decides whether this needs a hardfork gate.

The supply invariant is seeded exactly at genesis

TOTAL_SUPPLY_STORAGE_KEY is slot 2 on NATIVE_COIN_AUTHORITY_ADDRESS. Both shipped genesis files set it to precisely the sum of allocated balances:

network	slot 2	sum of alloc balances	match
mainnet	0x21e19e0c9bab2400000 (1e22)	1e22 across 1 funded account	exact
testnet	0x54b40b1f852bda00000 (2.5e22)	2.5e22 across 25 accounts	exact

So total_supply == sum(balances) holds at block 0 by construction, not by accident.

Every balance-mutating path preserves it

I grepped for all native-balance mutation sites across crates/evm and crates/precompiles:

• mint — supply += amt then balance_incr(amt). Preserved.
• burn — balance_decr(amt) then supply -= amt. Preserved.
• transfer — balance_decr + balance_incr, explicitly net-zero (native_coin_authority.rs:332). Supply untouched, correctly.
• Fees are a pure transfer, not a burn. reward_beneficiary (handler.rs:85-118) credits basefee + priority to the beneficiary, with the comment "This overrides the default EIP-1559 behavior which burns the base fee." The caller is debited by the standard revm path, so the fee is a move, not a destruction. Preserved.
• system_accounting.rs and native_coin_control.rs — zero balance mutations. Neither touches native balances at all.

The one path that does break the invariant breaks it the safe way

handler.rs:103-111 documents it: crediting a self-destructed beneficiary "silently burns the fee at commit." Zero8 rejects that condition, but pre-Zero8 it can happen — balances drop while supply stays put.

That drives total_supply above sum(balances). Underflow needs the opposite drift. So the only known invariant-breaking mechanism in the tree makes checked_sub strictly less likely to fail, never more.

Which makes the burn precondition airtight

balance_decr has already succeeded when the subtraction runs, so:

amount <= balance(from) <= sum(balances) <= total_supply

checked_sub cannot return None. The deleted comment's conclusion was right — though its stated reason ("due to the balance check") was incomplete. The balance check alone only gives amount <= balance(from); you need the global invariant for the second step. If you keep a comment here, that's the version worth writing down, since it's the part a future reader can't re-derive locally.

The part I'd want a maintainer to rule on: gating

This changes a state-transition outcome — a burn that previously succeeded with supply saturating to 0 now reverts and rolls back. That's consensus-observable if it can ever trigger, and the PR adds it ungated.

Ungated is the right call only because it's unreachable. That's a real constraint, not a formality — this codebase gates every observable behavior change in these precompiles:

• native_coin_control.rs:132,146,250,264 — Zero8
• helpers.rs:574,608 — Zero8
• system_accounting.rs:660,668,670 — Zero5/Zero6
• handler.rs:105 — the selfdestructed-beneficiary guard above, Zero8

So the PR can't have it both ways: if the invariant genuinely can lag (the premise in the description), this is a consensus change and needs a ArcHardfork gate like its neighbours. If it can't lag, the change is inert and safe ungated — but then "burn could silently zero supply" overstates it, and the PR risks being triaged as a security fix when it's hardening.

I'd suggest rewording the description to something like "unreachable under the total_supply == sum(balances) invariant; added as fail-closed hardening, therefore not hardfork-gated" — so a reviewer doesn't have to re-derive the reachability argument to approve it.

On the ERR_OVERFLOW reuse

@kutluhaneth46's point that this matches mint's error surface is right, but I checked the other uses and there's no existing precedent for it on an underflow path — both are genuine overflows:

• helpers.rs:444 — TransferError::OverflowPayment
• helpers.rs:486 — checked_add overflow in balance_incr

helpers.rs already exports ERR_INSUFFICIENT_FUNDS, which is closer to what a supply shortfall actually is. Not worth blocking over — consistency with mint is a defensible tiebreak — but it's a new semantic for the constant rather than an established one.

Minor while you're here: ERR_OVERFLOW is defined twice with identical text — helpers.rs:51 (pub) and native_coin_authority.rs:57 (local, shadowing the shared one). The local could just be dropped in favour of the import.

Test

Good instinct putting this in crates/ as a Rust unit test — that actually executes in Public CI under cargo nextest. Worth knowing that the tests/**/*.test.ts hardhat suites are invoked only by make test-unit-hardhat and no workflow calls them, so a regression test placed there would never have run.

The note about not pinning gas_used is correct. One addition worth making: the test asserts the revert reason but not the rollback. Since "fail closed" is the actual claim, asserting that total_supply is still 0 and ADDRESS_A's balance is unchanged afterwards would pin the behaviour that matters, rather than just the error string.

---

Not requesting changes — the code is right and the test is real. The reachability framing and the gating question are what I'd want resolved in the description before merge. Usual caveat: no cargo or rustc available here, so this is source review plus arithmetic on the committed genesis JSON, not a test run.