Skip to content

fix: use checked arithmetic for cumulative gas accounting in payload builder#24

Open
Himess wants to merge 2 commits intocirclefin:mainfrom
Himess:fix/checked-gas-accumulation
Open

fix: use checked arithmetic for cumulative gas accounting in payload builder#24
Himess wants to merge 2 commits intocirclefin:mainfrom
Himess:fix/checked-gas-accumulation

Conversation

@Himess
Copy link
Copy Markdown

@Himess Himess commented Apr 11, 2026

Summary

Use checked arithmetic for cumulative gas accounting in the payload builder to prevent potential overflow on u64 addition.

Two instances in arc_ethereum_payload:

  • Line 579: cumulative_gas_used + pool_tx.gas_limit() — replaced with checked_add + is_none_or to reject the transaction if the addition overflows
  • Line 660: cumulative_gas_used += gas_used — replaced with saturating_add to prevent wrapping

This follows the same defensive arithmetic pattern applied to reward_beneficiary in #21, where u128 multiplication was replaced with U256 arithmetic to prevent overflow.

…builder

Use checked_add and saturating_add for cumulative gas tracking to
prevent potential u64 overflow, consistent with the defensive arithmetic
pattern applied to reward_beneficiary in circlefin#21.
@Himess Himess force-pushed the fix/checked-gas-accumulation branch from 3d93e31 to eae0f2a Compare April 11, 2026 13:14
@atiwari-circle
Copy link
Copy Markdown
Contributor

Overflow here is not practically reachable -- cumulative_gas_used and pool_tx.gas_limit() are both bounded by block_gas_limit (~30M), nowhere near u64::MAX (~18.4 exagas). The checked_add at line 579 is fine if you want to keep it as defensive hardening, but saturating_add at line 660 is the wrong choice -- it would silently clamp at u64::MAX instead of failing the block build. Use checked_add with error propagation there, or leave it as plain addition.

…review

Per reviewer feedback, saturating_add silently clamps at u64::MAX which
would corrupt cumulative_gas_used rather than fail the block build cleanly.
Switch to checked_add with PayloadBuilderError propagation, matching the
defensive pattern used at the capacity check on line 579.
@Himess
Copy link
Copy Markdown
Author

Himess commented Apr 13, 2026

Thanks for the review @atiwari-circle. Agreed, replaced saturating_add with checked_add + PayloadBuilderError propagation in 4755edd, so an overflow (still unreachable in practice) would now fail the build cleanly instead of silently clamping. Kept checked_add at line 579 as defensive hardening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants