bug(precompiles): saturating_sub in burn silently corrupts total_supply on underflow #481

Activity

1. github-actions

   added

   need-triage

   on Oct 5, 2026
2. forumevi commented on Oct 5, 2026

   

   forumevi

   on Oct 5, 2026

   Author

   More actions

   I would like to work on this issue. I already have a fix ready with two regression tests.
3. forumevi commented on Oct 5, 2026

   

   forumevi

   on Oct 5, 2026

   Author

   More actions

   @ancazsanfir @romac @ZhiyuCircle @sergio-mena I would like to be assigned to this issue. A fix with two regression tests is ready.
4. forumevi commented on Oct 23, 2026

   

   forumevi

   on Oct 23, 2026

   Author

   More actions

   Hi @ancazsanfir @romac @ZhiyuCircle @sergio-mena — following up on our earlier request for assignment.

   We have analysed the fix in depth and confirmed the scenario:

   Root cause (confirmed by reading helpers.rs):
   balance_decr → check_can_decr_account uses checked_sub on the account balance, which correctly reverts if amount > account_balance. However the total-supply update on line 261 of native_coin_authority.rs uses saturating_sub on the storage slot, which is a separate value. If these two ever diverge (e.g. due to a genesis state initialisation error or a future migration bug), saturating_sub floors silently to 0 instead of reverting — permanently corrupting the invariant total_supply == sum(all balances) with no on-chain signal.

   The existing burn_side_effects test only covers the happy path where total_supply >= burn_amount. The desync scenario (total_supply < account_balance < burn_amount is impossible, but total_supply < burn_amount <= account_balance is theoretically reachable) has no test coverage.

   Fix (one line, native_coin_authority.rs:261):

   // before
   &current_total_supply.saturating_sub(args.amount).to_be_bytes_vec(),
   
   // after
   &current_total_supply.checked_sub(args.amount)
       .ok_or_else(|| new_reverted_with_early_penalty(gas_counter, reservoir, ERR_INSUFFICIENT_FUNDS))?
       .to_be_bytes_vec(),

   Regression tests (ready):

   1. Normal burn path — passes unchanged ✓
   2. Burn with total_supply < amount → now reverts instead of silently flooring ✓

   We are ready to open a PR immediately upon assignment. Could you please assign this issue to us?
5. ZhiyuCircle

   added

   tracked internallyThis issue is already tracked internally by the Arc team.This issue is already tracked internally by the Arc team.

   and removed

   need-triage

   on Oct 23, 2026
6. ZhiyuCircle commented on Oct 23, 2026

   

   ZhiyuCircle

   on Oct 23, 2026

   Contributor

   More actions

   Thanks @forumevi for reporting this, this is tracked internally
7. forumevi commented on Oct 23, 2026

   

   forumevi

   on Oct 23, 2026

   Author

   More actions

   Thanks for the update @ZhiyuCircle! Understood, happy to defer to the internal team on this one. If there's ever a way to contribute (tests, docs, or otherwise), we'd be glad to help.
8. osr21 commented on Oct 23, 2026

   

   osr21

   on Oct 23, 2026

   More actions

   Thanks for tracking this internally. A few implementation/test details that may help the fix, based on source inspection at 6e764023:

   1. Keep supply-underflow distinct from insufficient account funds. The account-balance check and the supply-slot check protect different values. Using the existing ERR_OVERFLOW with checked_sub would match mint’s arithmetic-error convention; ERR_INSUFFICIENT_FUNDS would misleadingly describe a case where the account has enough funds. An in-place replacement could follow mint’s existing pattern:

   let new_total_supply = match current_total_supply.checked_sub(args.amount) {
       Some(supply) => supply,
       None => return Err(new_reverted_with_early_penalty(
           gas_counter, reservoir, ERR_OVERFLOW,
       )),
   };
   // Pass new_total_supply.to_be_bytes_vec() to the existing write call.

   2. Make the regression reach the intended branch. For example, seed supply = 100, account balance = 200, burn = 150. The remaining account balance is nonzero, so the pre-Zero8 empty-account restriction will not mask the supply-underflow case. Also cover supply == amount (valid subtraction to zero) and ordinary insufficient account balance. For an exact-drain test before Zero8, give the account a nonzero nonce/code, or test the relevant fork modes explicitly; the helper rejects emptying a nonce-zero, code-empty account before Zero8.

   3. Test rollback at the right layer. The burn currently decrements the balance before checking/updating supply. The local test helper calls provider.run directly, without establishing/reverting a call-frame checkpoint itself. Use a real EVM call-frame test (or explicitly manage the journal checkpoint) to assert a rejected burn leaves balance and supply unchanged and commits no burn Transfer log. A direct-provider assertion alone is not proof of transaction-level rollback.

   Moving the supply check before the debit is another option, but it changes operation order, gas charging, and potentially error precedence; that should be reviewed for consensus/replay compatibility rather than treated as a purely cosmetic rearrangement.

   This is defensive invariant enforcement, not evidence that an ordinary authorized burn can create the initial supply/balance divergence. These suggestions are source-reviewed; I have not run a patched Arc Node test suite. No assignment or duplicate PR requested.
9. kutluhaneth46 commented on Oct 26, 2026

   

   kutluhaneth46

   on Oct 26, 2026

   Contributor

   More actions

   I’d like to take this if a maintainer can assign it.

   Plan: replace saturating_sub with checked_sub + revert on underflow (mirror mint’s checked_add / ERR_OVERFLOW), plus two regression tests for the underflow path. Won’t open a PR until assigned per the contribution policy. Thanks!