execution, cl, common/math: fix unchecked integer overflows on untrusted input - #23192
execution, cl, common/math: fix unchecked integer overflows on untrusted input#23192AskAlexSharov wants to merge 5 commits into
Conversation
d7071e0 to
6cea37c
Compare
There was a problem hiding this comment.
Pull request overview
Fixes two integer-overflow issues in untrusted-input parsing paths: RIP-7560 (AA) gas-limit summation and Caplin bitlist sentinel handling, preventing wrapped-small totals and malformed bitlists from producing incorrect behavior (or panics).
Changes:
- Add
AccountAbstractionTransaction.TotalGasLimitwith overflow detection, and makeGetGasLimit()saturate toMaxUint64on overflow (fail-closed for interface callers). - Reject RIP-7560 gas-limit overflows in AA charging/refunding/execution paths by returning
ErrGasLimitReached. - Make
parseBitlisttreat “no sentinel” shapes (including empty input) as length 0, and add regression tests for both fixes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| execution/types/transaction_test.go | Adds regression coverage for AA total gas-limit overflow behavior. |
| execution/types/aa_transaction.go | Introduces overflow-checked gas-limit summation and saturating GetGasLimit(). |
| execution/protocol/aa/aa_gas.go | Switches gas precharge/refund totals to overflow-checked summation and fails closed on overflow. |
| execution/protocol/aa/aa_exec.go | Uses overflow-checked total gas limit when returning unused gas to the pool. |
| cl/merkle_tree/merkle_root_test.go | Adds regression test for malformed bitlists missing the sentinel bit. |
| cl/merkle_tree/list.go | Guards parseBitlist against empty/zero-terminator inputs to avoid underflow and bad length mixing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
da119d9 to
0f5326f
Compare
…rflow uint64 The four RIP-7560 gas limits arrive unvalidated and were summed with no overflow check in four places. A wrapped total made the balance precharge in chargeGas small enough to pass the insufficient-funds check, and made refundGas compute a refund from a preCharge below the actual cost. TotalGasLimit reports the overflow so both can reject. GetGasLimit cannot return an error, so it saturates and lets downstream gas checks fail closed.
A bitlist encoding always ends in a sentinel bit, so the last byte is never zero. Reading msb from a zero byte underflowed to 255 and inflated the length mixed into the hash tree root; an empty buffer indexed out of range.
f83a5fe to
2d9714d
Compare
geth carries SafeAdd, SafeSub and SafeMul; erigon's copy dropped SafeSub. Subtraction is the most common unchecked case, so restore it. Two call sites computed the result first and detected the wrap afterwards. That is correct Go but relies on the wraparound it is trying to reject, and arithmetic instrumentation flags it as an overflow.
3bc4ab8 to
07eeeb9
Compare
AskAlexSharov
changed the title
finishProgressAfter can trail finishProgressBefore. The subtraction then wrapped, min clamped it to 1024, and notifyFrom was computed from a block span that never ran.
domiwei
left a comment
domiwei
left a comment
There was a problem hiding this comment.
Two overflow-hardening gaps remain:
- The inline comment below covers malformed SSZ bitlists being normalized into the canonical empty root.
node/privateapi/ethbackend.go:541still computespreTxCost + ValidationGasLimit + PaymasterValidationGasLimit + GasLimit + PostOpGasLimitwith uncheckeduint64arithmetic at the protobuf AA-validation ingress. The downstreamchargeGascheck currently fails closed, but arithmetic instrumentation can still panic at this earlier sum and correctness relies on the duplicated later check. Please useaaTxn.TotalGasLimit(preTxCost)at ingress and add a protobuf/public-path regression test.
There are also no production-entry regression tests proving overflow is rejected before AA balance/gas-pool mutation, nor a focused test for the new finishProgressAfter < finishProgressBefore notification branch.
| func parseBitlist(dst, buf []byte) ([]byte, uint64) { | ||
| // A bitlist without its sentinel bit has no recoverable length. | ||
| if len(buf) == 0 || buf[len(buf)-1] == 0 { | ||
| return dst, 0 |
There was a problem hiding this comment.
Malformed non-empty bitlists should be rejected here rather than normalized to length zero. For example, {0x00} and {0xff, 0x00} now return success and hash to the same root as the logical empty value, so hashing no longer proves the representation was valid. SSZ requires a terminating 1 bit; please return an error for len(buf) > 0 && buf[len(buf)-1] == 0. If the internal zero-length representation must remain supported, handle len(buf) == 0 separately and keep {0x01} as the canonical serialized-empty control in the tests.
|
I found two related unchecked AA arithmetic sites outside the changed hunks:
|
|
|
||
| func parseBitlist(dst, buf []byte) ([]byte, uint64) { | ||
| // A bitlist without its sentinel bit has no recoverable length. | ||
| if len(buf) == 0 || buf[len(buf)-1] == 0 { |
There was a problem hiding this comment.
Could we reject non-empty sentinel-less encodings here instead of hashing them as empty? This makes {0x00} and {0xff, 0x00} produce the same root as canonical empty {0x01}, even though solid.BitList.DecodeSSZ correctly rejects both malformed forms. The exported BitlistFromBytes(...).HashSSZ() path bypasses that decoder, so this creates a decode/hash representation mismatch. If len(buf) == 0 must remain supported as Erigon internal empty state, please at least return an error for len(buf) > 0 && buf[len(buf)-1] == 0 and change the regression test to assert rejection.
| effectiveGasPrice := new(uint256.Int).Add(baseFee, &effectiveGasTip) | ||
|
|
||
| totalGasLimit := preTxCost + tx.ValidationGasLimit + tx.PaymasterValidationGasLimit + tx.GasLimit + tx.PostOpGasLimit | ||
| totalGasLimit, ok := tx.TotalGasLimit(preTxCost) |
There was a problem hiding this comment.
RIP-7560 defines maxPossibleGasCost using AA_BASE_GAS_COST plus the four declared limits. preTxCost also includes dynamic calldata/access-list/authorization charges, and those charges are already deducted from ValidationGasLimit by the validation-frame budgeting. Passing preTxCost here therefore precharges spec maximum + (preTxCost - TxAAGas), while refundGas and the gas-pool restoration use TxAAGas; a transaction whose balance exactly meets the spec maximum is rejected, and with a larger balance the extra amount is never refunded. Could this use tx.TotalGasLimit(params.TxAAGas) and add a test with non-empty calldata that checks payer and gas-pool conservation?
Two unchecked integer overflows in code that parses untrusted input, found by compiling the unit suite with gosentry's arithmetic instrumentation (
go test -short ./...surfaced 116 panics across 18 sites; the rest were intentional wraparound in SWAR, hashing and crypto code).RIP-7560 gas limits sum without an overflow check.
ValidationGasLimit,PaymasterValidationGasLimit,GasLimitandPostOpGasLimitcome off the wire unvalidated and were added in four places.chargeGasturns the wrapped total intopreChargeand compares it against the payer's balance, so a wrapped-small total passes the insufficient-funds check;refundGasthen computespreCharge - actualGasCostfrom the same value.A bitlist with no sentinel bit corrupts its hash tree root.
parseBitlistreadsbits.Len8(last) - 1; when the last byte is zero that underflows to 255 and inflates the length mixed into the root. An empty buffer indexed out of range.GetGasLimit()withValidationGasLimit = MaxUint6415000(base cost alone)MaxUint64, checks fail closedchargeGas/refundGason an overflowing sumErrGasLimitReachedBitlistRootWithLimit([]byte{0x00})BitlistRootWithLimit([]byte{})index out of range [-1]Both regression tests were confirmed to fail on unfixed code before the fix landed.
SafeSubrestored. geth'scommon/mathcarriesSafeAdd,SafeSubandSafeMul; erigon's copy droppedSafeSub, even though subtraction is the most common unchecked case. Two call sites computed the result first and detected the wrap afterwards — correct Go, but it relies on the wraparound it is trying to reject:backward_beacon_downloader.goslot - count + 1, thenif start > slotmath.SafeSub(slot, count-1)txn_executor.gostNonce+1 < stNoncemath.SafeAdd(stNonce, 1)Sites that already guard before subtracting (
eon_tracker.go,polygon/sync,txpool/pool.go) are left alone — they are correct and converting them would be churn.Notes for reviewers
GetGasLimit()is on theTransactioninterface and cannot return an error, so it saturates toMaxUint64. That is the fail-closed direction: gas-pool and block-gas-limit checks then reject, where a wrapped-small value would pass. If the AA owners prefer validation to happen strictly earlier, the helperTotalGasLimitis there to build on.cl/merkle_treechange is deliberately conservative: a bitlist whose last byte is non-zero — every well-formed one — hashes bit-identically to before. Only the malformed shapes change, from garbage/panic to a defined length of 0. Rejecting malformed bitlists outright may be more spec-correct than computing a root for them; that is a call for the Caplin owners, andBitlistRootWithLimitalready returns an error if they want it.TotalGasLimituses the existingcommon/math.SafeAdd, which carries out viabits.Add64rather than adding and testing for a wrap. A wrap-based guard is correct Go but is itself an overflow, so it trips the same instrumentation on every future sweep../cl/merkle_tree/...,./execution/typesand./execution/protocol/...— 9 packages, 0 failures.stage_senders.godebug-log underflow and the Caplin epoch-0 wrap, both of which are wrapped-then-discarded and change no behavior.