precompile: price dynamic-gas calldata decoding by supplied gas#3737
precompile: price dynamic-gas calldata decoding by supplied gas#3737codchen wants to merge 8 commits into
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3737 +/- ##
==========================================
- Coverage 60.12% 59.20% -0.93%
==========================================
Files 2303 2212 -91
Lines 192032 182307 -9725
==========================================
- Hits 115466 107937 -7529
+ Misses 66248 64814 -1434
+ Partials 10318 9556 -762
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
codchen
changed the title
PR SummaryHigh Risk Overview
Frozen Reviewed by Cursor Bugbot for commit f5dc250. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Prices dynamic-gas precompile calldata decoding out of the supplied gas before decoding, closing a free-parse DoS; the cost model faithfully mirrors go-ethereum's unpack traversal and is well-tested. No blockers — a few clarifying notes, and I disagree with Codex's sole finding.
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex's High finding (frozen historical replay is affected because legacy versions share
precompiles/common) is a false positive: each legacy precompile imports its own frozenprecompiles/common/legacy/vNNN(e.g. distribution/legacy/v605 -> common/legacy/v605), which is untouched. The changed v6.0.x expectations in evmrpc/tests/regression_test.go are re-baselines of the CURRENT code path — the pacific regression harness sets no upgrade done-heights, so precompiles resolve tolatestUpgradeand those tests always exercised current code, not the v6.0.x snapshots. The author's Compatibility note is correct. - Cursor produced no review output (file empty).
- This is a consensus/app-hash-breaking change to the latest precompile version (correctly labeled). Ensure it ships coordinated with an upgrade and that the pre-change
precompiles/commonis snapshotted as a frozen legacy version at release-cut time so post-upgrade historical replay of pre-upgrade blocks stays deterministic. - Cost-model edge: the
ok=falsefallback in DecodeGasCost charges only the linear base. This is safe in practice (hitting maxDecodeWalkOps requires ~32MB of gas-prohibitive calldata, and other failure paths correspond to inputs the real decoder rejects with bounded work), but it does rely on that invariant — worth a brief comment noting that no ABI shape used by current precompiles can make the real decoder copy super-linearly while the walk bails to base. - Consider a targeted unit test for the fixed-size-array-of-dynamic-element path (ArrayTy with dynamic elem) in decode_cost_test.go; current tests cover string, bytes, string[] (SliceTy), and tuple[], but not the fixed-array branch of walk().
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // metering the calldata decode below — into a reverted precompile call, | ||
| // mirroring how the individual executor methods recover. This keeps a | ||
| // single precompile call's failure from aborting the enclosing EVM frame. | ||
| if r := recover(); r != nil { |
There was a problem hiding this comment.
[suggestion] This new top-level recover() is the right way to convert the OOG panic from the ConsumeGas(DecodeGasCost(...)) call below into a reverted precompile call. Note it also now catches any panic escaping d.executor.Execute (previously none were recovered at this level, though the individual executors recover internally). That's consistent with the existing executor recover pattern and safer for the enclosing EVM frame, but it does broaden what gets silently turned into ErrExecutionReverted — confirm that's intended and won't mask a genuinely non-deterministic panic during execution.
| gasLimit := d.executor.EVMKeeper().GetCosmosGasLimitFromEVMGas(ctx.WithGasMeter(sdk.NewInfiniteGasMeterWithMultiplier(ctx)), suppliedGas) | ||
| ctx = ctx.WithGasMeter(sdk.NewGasMeterWithMultiplier(ctx, gasLimit)) | ||
| operation = method.Name | ||
| ctx.GasMeter().ConsumeGas(DecodeGasCost(method.Inputs, input), fmt.Sprintf("%s precompile calldata decode", d.name)) |
There was a problem hiding this comment.
[nit] The decode charge is metered against the Cosmos gas meter (via the supplied-EVM-gas-derived limit) rather than charged from the EVM gas pool up front like the static path's RequiredGas. This flows through to remainingGas correctly (as the updated test expectations confirm) and is consistent with how execution gas is already metered here, so no change needed — just flagging that the model differs from the static-precompile charge point for reviewers cross-checking gas accounting.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR correctly closes a DoS gap by pricing ABI calldata decoding against supplied gas before decoding on the dynamic-gas precompile path; the cost estimator is conservative and well-tested, and the gas gate is ordered correctly. No blocking issues found — the one external "High" finding does not hold up on investigation.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Disagree with Codex's High finding (regression_test.go historical-replay leak). The evmrpc/tests
testTxharness never registers upgrade done-heights (nomockUpgrade/SetDonecall), soGetCustomPrecompilesVersionshits thenoForkHistorybranch and resolves every precompile tolatestUpgrade— the current framework this PR modifies — regardless of the"v6.0.x"version string. Production tracing of those historical txns resolves to the frozenprecompiles/*/legacy/vNNNsnapshots (which embed their own frozenprecompiles/common/legacy/vNNNcopy of RunAndCalculateGas, untouched here), so there is no production historical-replay change. The updated expectations are a test-harness artifact, not a regression. - Confirm the release process: this is (correctly) labeled app-hash-breaking and applies to the current precompile version with no in-code upgrade-height gate. Ensure the release that ships this snapshots the pre-change
commonframework into a newlegacy/vNNNand registers the upgrade at a height, so blocks produced under the current version before the upgrade boundary continue to replay with the old gas (per evmrpc/AGENTS.md: historical RPC responses must not change across upgrades). - REVIEW_GUIDELINES.md and cursor-review.md are both empty (no repo-specific guidelines and no Cursor second-opinion output to merge).
- The estimator diverges from go-ethereum only on the fixed-array-of-dynamic-element path (reads the offset from the low 8 bytes of the head word rather than the full 32-byte word). This is safe in practice: when the high 24 bytes are non-zero the real decoder rejects the input, so whenever both succeed they agree — no undercharge. Worth a brief comment noting this is intentionally narrower than the general ABI grammar precompiles actually use.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| // metering the calldata decode below — into a reverted precompile call, | ||
| // mirroring how the individual executor methods recover. This keeps a | ||
| // single precompile call's failure from aborting the enclosing EVM frame. | ||
| if r := recover(); r != nil { |
There was a problem hiding this comment.
[nit] This recover() now converts any panic in the decode/execute path into a reverted call, not just the intended out-of-gas panic from ConsumeGas. That is the right behavior for the OOG gate, but it also silently swallows genuine executor bugs (nil derefs, invariant violations) that previously propagated. Consider distinguishing the OOG/gas panic (e.g. sdk.ErrorOutOfGas) from other panics — re-panicking or at least logging unexpected panic types — so real defects aren't masked as ordinary reverts. Non-blocking.
| } | ||
| // DecodeGasCost already includes scanCost; charge only the remaining | ||
| // (string-copy) portion so the decode is priced exactly once. | ||
| ctx.GasMeter().ConsumeGas(decodeCost-scanCost, fmt.Sprintf("%s precompile calldata decode", d.name)) |
There was a problem hiding this comment.
[nit] Ordering and arithmetic here are correct: scanCost == DefaultGasCost(input,false) is charged first, and decodeCost is satAdd(base, ...) with base == scanCost, so decodeCost - scanCost can never underflow and the decode is priced exactly once. Good.
| "0x362af85584493ed225bf592606c989796ba5f0484f5c80b989b8573f85517ec1", | ||
| "v6.0.3", | ||
| "0x8214a", | ||
| "0x82932", |
There was a problem hiding this comment.
[nit] These expectation changes are expected, not a historical-replay regression (addressing the external review's flag). testTx doesn't call mockUpgrade/SetDone, so GetCustomPrecompilesVersions takes the noForkHistory branch and runs the latest precompile framework — the code this PR changes — regardless of the "v6.0.3" argument. Production tracing of these txns resolves to the frozen legacy/vNNN snapshots, which are untouched. No change needed; noting for the record.
There was a problem hiding this comment.
The decode-gas charge is added to the shared common.DynamicGasPrecompile.RunAndCalculateGas with no upgrade-height gate, so it also alters gas for the frozen v6.0.1/v6.0.3 precompile snapshots (bank/addr/oracle/ibc/wasmd/json import the current common), breaking historical replay/app-hash for those mainnet heights; the changed v6.0.0–v6.0.3 regression fixtures are evidence of this, not just a current-version reprice. The decode-cost estimator itself is well-implemented and well-tested.
Findings: 4 blocking | 4 non-blocking | 2 posted inline
Blockers
- Historical-replay / consensus break: the new calldata-decode charge is applied unconditionally in the shared
precompiles/common/precompiles.goRunAndCalculateGas. Thev601andv603legacy precompile snapshots (precompiles/{bank,addr,oracle,ibc,wasmd,json}/legacy/v60{1,3}) import the CURRENTprecompiles/common(verified:grepshowspcommon "github.com/sei-protocol/sei-chain/precompiles/common"), whereas v5.5.x and v6.0.5+ snapshots import frozenprecompiles/common/legacy/vNNN. So the charge changes gas for the frozen v6.0.0–v6.0.3 precompile versions too. Re-executing/state-syncing mainnet blocks in that height range would now compute a different app hash than the network committed. The PR description's claim that “historical replay runs against frozen legacy snapshots which are unchanged, so no upgrade-height guard is required” is therefore incorrect. Fix per the repo’s own pattern: snapshot the currentcommoninto a newcommon/legacy/v6XX, repoint the v601/v603 precompile packages to it, and apply the decode charge only to current/latest — or gate the charge behindctx.ClosestUpgradeName(). - The changed v6.0.0–v6.0.3 entries in
evmrpc/tests/regression_test.goare the direct symptom of the break above: those tests route to the v601/v603 snapshots that share the currentcommon, so their gasUsed shifted, while all other versions (frozen common) are unchanged. Updating these fixtures masks the divergence rather than resolving it — they should return to their original values once the change is correctly scoped to the current version (matches Codex’s finding). - 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor produced no review output (
cursor-review.mdwas empty); this synthesis is based on the diff, the repository code, and the Codex pass only. - The regression suite cannot independently guard against this class of leak: it did surface the v6.0.0–v6.0.3 shift, but only because those snapshots happen to import current
common. Consider an explicit test/assertion that frozen precompile versions do not change gas when the shared framework changes. decode_cost.gois well-constructed — it faithfully mirrors go-ethereum’stoGoType/forEachUnpack/lengthPrefixPointsTobounds and offset handling, prunes non-string subtrees, stays linear via the pre-scan +maxDecodeWalkOpscap, and is covered by the aliased/tuple/malformed unit tests (including a cross-check against the real decoder). No correctness issue found in the estimator itself; the only concern is where the resulting charge is applied.- The pre-existing
fmt.Printf("precompile %s encountered error...")debug line is retained in the modified deferred block; unrelated to this PR but worth replacing with structured logging at some point.
| // Charge a length-proportional amount up front, before the structural scan | ||
| // inside DecodeGasCost runs, so the scan itself is bounded by the supplied gas | ||
| // and can never be performed for free even if it later becomes costlier. | ||
| scanCost := DefaultGasCost(input, false) |
There was a problem hiding this comment.
[blocker] This decode charge is applied unconditionally in the shared framework, with no ctx.ClosestUpgradeName() gate. The v601/v603 legacy precompile snapshots (bank/addr/oracle/ibc/wasmd/json) import this current precompiles/common, so the charge also changes gas for those frozen historical versions — not just the current one. That diverges from the gas the network already committed for v6.0.0–v6.0.3 mainnet blocks (re-execution/state-sync would produce a different app hash). Scope this to current/latest: freeze the current common into a common/legacy/v6XX snapshot and repoint the v601/v603 packages to it, or gate the charge by upgrade height. The PR’s “legacy snapshots are unchanged” claim does not hold for v601/v603, which share this file.
| "0x362af85584493ed225bf592606c989796ba5f0484f5c80b989b8573f85517ec1", | ||
| "v6.0.3", | ||
| "0x8214a", | ||
| "0x82932", |
There was a problem hiding this comment.
[blocker] Every regression fixture that changed in this PR is a v6.0.0–v6.0.3 transaction, and those versions route to the v601/v603 precompile snapshots that import the current precompiles/common (v5.5.x and v6.0.5+ route to frozen common/legacy/* and are unchanged). So this gasUsed change reflects an alteration of already-committed historical precompile behavior, not a current-version-only reprice. Once the decode charge is correctly confined to the current version, these expected values should revert to their originals.
There was a problem hiding this comment.
An organization admin can view or raise the cap at claude.ai/admin-settings/claude-code. The cap resets at the start of the next billing period.
Once the cap resets or is raised, push a new commit or reopen this pull request to trigger a review.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| w.walkElems(output[offset:], t.Size, *t.Elem) | ||
| } else { | ||
| w.walkElems(output[index:], t.Size, *t.Elem) | ||
| } |
There was a problem hiding this comment.
Fixed dynamic array walk wrong
Low Severity
In decodeStringCopyBytes, the ArrayTy branch treats fixed-size arrays whose elements are dynamic (e.g. string[N]) like a single tail offset and walks from one offset. ABI encoding places N consecutive 32-byte head words in the head region, matching go-ethereum’s inline unpack. The estimator can undercount string-copy volume or mark valid encodings invalid.
Reviewed by Cursor Bugbot for commit a31cb2b. Configure here.
There was a problem hiding this comment.
Solid, well-tested fix that prices ABI calldata decoding under the supplied-gas meter for dynamic-gas precompiles, closing a DoS where near-zero-gas calls forced free parse/allocation. The cost estimator faithfully mirrors go-ethereum's unpack traversal and the legacy precompiles are correctly re-pinned to the frozen common snapshot so historical replay is unaffected. No blockers; a few points worth confirming.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- app-hash-breaking: this changes gas cost for every current dynamic-gas precompile (distribution/staking/bank/wasmd/oracle/ibc/pointer/addr/solo). Confirm the new charge only becomes active at the coordinated release/version boundary so upgraded and un-upgraded nodes cannot diverge mid-block. Legacy vNNN snapshots are correctly frozen (verified v66 == pre-PR live common, and no legacy precompile still imports live
precompiles/common), so replay is unaffected — the concern is purely the live-version rollout, which the label acknowledges. - The op-cap fallback (
maxDecodeWalkOps) makesDecodeGasCostreturn not-ok for structurally-valid but extremely nested/aliased calldata that the real decoder would accept, causing such calls to be rejected as "invalid encoding". This is a safe/defensive tradeoff for the fixed, shallow precompile ABIs, but it means a legitimately-crafted (non-attack) deeply-nested input at the extreme would be rejected rather than decoded. Documented in the code; noting for awareness. - Second-opinion passes: OpenAI Codex reported no material issues; the Cursor review file was empty (no output produced).
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // (a call that could not afford the decode), keeping the failure from | ||
| // aborting the enclosing EVM frame. Re-panic anything else so genuine | ||
| // bugs are not masked as reverts. | ||
| if r := recover(); r != nil { |
There was a problem hiding this comment.
[suggestion] The recover now spans the entire function, including d.executor.Execute(...), so an ErrorOutOfGas/ErrorGasOverflow panic originating inside execution (not just the decode charges) is now converted to ErrExecutionReverted here rather than propagating as before. This appears gas-equivalent (on the recovered path remainingGas stays 0, matching all-gas-consumed OOG semantics), but the comment frames the recover as only covering "the calldata-decode gas charges below" — the scope is actually wider. Worth confirming execution-path OOG panics were previously turned into reverts downstream too, so behavior is unchanged.
There was a problem hiding this comment.
@codchen I recommend addressing this one but not a blocker
Superseded: latest AI review found no blocking issues.
Dynamic-gas precompiles decoded the ABI argument payload in Prepare() before converting the supplied EVM gas into a Cosmos gas meter. The static precompile path checks that the supplied gas covers the work up front (vm.RunPrecompiledContract), but that check does not run on the dynamic-gas path. Align the two paths in RunAndCalculateGas: resolve the target method from the 4-byte selector, derive the Cosmos gas limit from the supplied gas, and require it to cover the calldata read cost before unpacking the arguments. Gas accounting for adequately funded calls is unchanged, so existing gas results for normal usage stay the same. Add a regression test and update the existing dynamic-gas precompile test to supply gas. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Charge for ABI-decoding a dynamic-gas precompile call before it runs. The decoder's cost is dominated by copying string payloads, and because one string can be referenced by many array/tuple slots the copied volume can grow faster than the input length. Charge ReadCostFlat + ReadCostPerByte*(len(input) + string-copy volume), where the string-copy volume is computed by a structural pre-scan that follows offsets and reads length prefixes without copying, so it stays linear in len(input). The charge is consumed from the supplied-gas meter before Unpack, so a call that cannot afford the decode is rejected before the parse/allocation work runs. Only the current precompile version is affected; historical replay uses the frozen legacy snapshots. Add unit tests for the cost estimator and update the affected precompile gas expectations. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…gas fixtures Annotate the bounds-checked int/uint64 conversions in decode_cost.go with //nolint:gosec (each value is range-checked immediately before the conversion). Refresh the evmrpc/tests replay gas fixtures that shift under the decode charge. The mock replay harness has no upgrade fork history, so precompile version selection falls back to the latest (current) version; these traces therefore exercise the current precompile and reflect its new gas. Production tracer replay resolves to the frozen legacy snapshots by upgrade height and is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…data early Charge a length-proportional amount before running the calldata scan, so the scan is itself bounded by the supplied gas and can never run for free even if it later becomes costlier; the remaining (string-copy) portion is charged after, so the total is unchanged. DecodeGasCost now reports ok=false for structurally invalid calldata, and the caller rejects it before Unpack rather than charging a fallback and letting Unpack fail. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Only convert sdk.ErrorOutOfGas / sdk.ErrorGasOverflow panics (from the calldata decode gas charges) into a reverted call; re-panic anything else so genuine bugs are not masked as reverts. Add a regression test asserting a non-gas panic propagates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The v601/v603 snapshots of addr, bank, ibc, json, oracle and wasmd imported the current precompiles/common, so changes to the current common leaked into their replay path. Point them at the frozen precompiles/common/legacy/v66 snapshot (byte-identical to the current common prior to the recent decode-cost change), so historical replay is insulated from current-common changes. This restores the pre-change replay gas for the affected traces, so the evmrpc/tests replay fixtures revert to their original values. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
a31cb2b to
c286d5f
Compare
There was a problem hiding this comment.
No new issues found by this run's bug hunt; deferring to a human given this PR's scope and an open question from my earlier review.
What was reviewed: the decode-gas charge ordering/arithmetic in DynamicGasPrecompile.RunAndCalculateGas (scan cost charged before the string-copy cost, no double-charge); the panic-recovery scoping (only gas-meter panics converted to reverts); and the v601/v603 legacy-snapshot gas divergence flagged earlier, which is now fixed by freezing a precompiles/common/legacy/v66 snapshot for those legacy precompiles.
One point from my earlier review remains open: the fix only covers the dynamic-gas precompile path. gov's static-gas precompile charges a flat RequiredGas (30000) for voteWeighted, whose ABI (WeightedVoteOption[], a tuple array containing a string) has the same aliased-string amplification shape this PR fixes elsewhere. This run's automated pass re-examined that concern and ruled it out, but I still think it is a real, unaddressed gap in this PR's stated safety rationale (that the static path is already protected by RequiredGas) rather than a false positive, so it is worth a human judgment call on whether it should block this PR or be tracked separately.
Extended reasoning...
This PR reprices ABI-decode gas for the shared dynamic-gas precompile framework to close a DoS where attacker-controlled calldata (aliased array/tuple string references) could force validators to copy far more data than the flat gas charge accounted for. It is labeled app-hash-breaking and touches gas accounting shared by bank, distribution, ibc, wasmd, oracle, addr, and pointer precompiles, plus new frozen legacy/v66 snapshots for v601/v603 precompiles.
I independently verified the two points raised earlier in the thread: (1) the seidroid-flagged blocker that the decode charge would alter already-committed v6.0.0-v6.0.3 gas via the v601/v603 packages importing the live common package has been fixed — those packages now import a generated, frozen precompiles/common/legacy/v66 snapshot that does not contain the decode-cost changes; (2) my own earlier finding that gov.voteWeighted (a static-gas precompile with a flat RequiredGas) has the same aliased-string decode-cost amplification shape this PR fixes for dynamic-gas precompiles is still unaddressed in the diff — precompiles/gov/gov.go is unchanged and still returns a flat 30000 gas for voteWeighted regardless of input size. This run's bug hunt re-examined that exact concern and concluded it was not a real issue; based on my own reading of the current gov.go, I do not think that conclusion is safe to rely on, since the same amplification mechanics this PR's own tests demonstrate for the dynamic path apply structurally to voteWeighted's ABI shape.
Given the PR is app-hash-breaking, touches gas metering shared across most EVM precompiles, and there is a genuine disagreement between this run's automated verification and my own analysis on an open security-relevant question, I believe this warrants human judgment rather than a shadow approval.
| // as with every current static precompile) add nothing. | ||
| if strBytes, ok := decodeStringCopyBytes(method.Inputs, input[4:]); ok && strBytes > uint64(len(input)) { | ||
| required = satAdd(required, satMul(storetypes.KVGasConfig().ReadCostPerByte, strBytes-uint64(len(input)))) | ||
| } |
There was a problem hiding this comment.
Static gas surcharge skips malformed input
Medium Severity
The new static-path decode surcharge only applies when decodeStringCopyBytes returns ok. On failure it returns 0, false and discards any string-copy volume already counted, so RequiredGas adds nothing. Run then still calls Unpack, which can materialize the valid aliased prefix before failing. The dynamic path rejects on !ok; this gate does not.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 87077ee. Configure here.
There was a problem hiding this comment.
A careful, well-tested security fix that prices ABI calldata decoding under the supplied gas meter for dynamic-gas precompiles (and surcharges the super-linear string-copy case on the static path). The cost estimator faithfully mirrors go-ethereum's decoder traversal, the gas-panic recover is correctly scoped and typed, and versioned replay behavior is preserved by freezing legacy precompiles to the common/legacy/v66 snapshot. No blocking issues found; one minor logging nit.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion coverage: Codex reported "No material issues found"; the Cursor review file (cursor-review.md) was effectively empty (no output produced). Neither surfaced additional findings to merge.
- Cost-model asymmetry (informational, not a bug): the dynamic path charges base(len(input)) + ReadCostPerByte*fullStringBytes, while the static RequiredGas path only surcharges the excess (strBytes - len(input)) above a linear pass. Both guarantee decode work is paid for, but the two paths price the same aliased input differently; a short comment noting this intentional difference would help future readers.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| HandlePrecompileError(err, evm, operation) | ||
| if err != nil { | ||
| fmt.Printf("precompile %s encountered error: %v\n", d.name, err) |
There was a problem hiding this comment.
[nit] This fmt.Printf to stdout fires on every dynamic-precompile error, which is an attacker-reachable revert path (e.g. structurally-invalid calldata, insufficient gas for the decode), so it can be triggered cheaply and repeatedly to spam stdout. It also bypasses the structured logger used elsewhere in this package. Consider dropping it or routing it through the keeper's logger at debug level. (Note: it mirrors the already-shipped common/legacy/v66 snapshot, so this is consistency-preserving — hence a nit, not a blocker.)
There was a problem hiding this comment.
No new bugs found this run. Since my last review, the two previously-flagged issues both look resolved: the static-precompile path (e.g. gov.voteWeighted) now gets a length-proportional decode surcharge in Precompile.RequiredGas itself, and the v601/v603 legacy snapshots now import the frozen common/legacy/v66 package instead of the live one. Given this touches consensus-critical, app-hash-breaking gas metering shared across every precompile, I'd still like a human to take a final pass before merge.
Extended reasoning...
This run's bug hunter found nothing new. I reviewed the diff against the prior findings on this PR: (1) my earlier inline comment noted that Precompile.Prepare (static path, e.g. gov.voteWeighted) had no size-proportional gas charge for ABI decoding, unlike the new dynamic-gas path — the latest commit (87077ee) closes this by adding a surcharge to Precompile.RequiredGas whenever the estimated string-copy volume exceeds a linear pass over the input, which applies to every static precompile including gov, not just the ones named in the PR description; a new test (TestPrecompileRequiredGasDecodeSurcharge) exercises this. (2) seidroid's blocker about v601/v603 importing the live, changing precompiles/common (which would have altered already-committed v6.0.0-v6.0.3 gas) is resolved by pinning those legacy packages to precompiles/common/legacy/v66. (3) I checked Cursor bugbot's low-severity claim about the ArrayTy walk in decode_cost.go against go-ethereum's actual toGoType/forEachUnpack unpack source, and the single-offset-then-walk-elements logic for fixed arrays of dynamic elements matches go-ethereum's real behavior, so that finding looks like a false positive rather than something left unfixed. (4) The remaining open items (the recover() scope broadening flagged by seidroid and acknowledged by masih as non-blocking) are explicitly marked non-blocking by a maintainer. Despite the diff itself looking correct on this pass, the change is large, security-sensitive (closes a real DoS vector in EVM precompile calldata decoding) and consensus/app-hash-breaking, so I think it still warrants a human's final sign-off rather than an automated approval.
Reverts the decode-cost walk that was added to Precompile.RequiredGas. That function is evaluated before the supplied-gas gate in vm.RunPrecompiledContract, so its per-call big.Int allocations ran ahead of the gate — repeated low-gas calls could impose that work without paying for it. The static precompiles do not need it: json, pointerview and p256 all decode linearly and their existing RequiredGas already covers it. Document the invariant instead (RequiredGas must stay cheap and cover the decode; no super-linearly-decoding args without pricing them). The dynamic-gas path is unchanged — its decode charge sits behind the up-front gas gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
87077ee to
5f9feed
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5f9feed. Configure here.
There was a problem hiding this comment.
I didn't find any new bugs this round, but this PR changes gas-metering behavior across the entire EVM precompile framework (app-hash-breaking) and has already gone through several rounds of real fixes (aliased-string decode-cost pricing, legacy-snapshot gas divergence for v601/v603, panic-recovery scope), so I think it's worth a final human pass before merge.
What was reviewed: the new DecodeGasCost/decodeStringCopyBytes walk in precompiles/common/decode_cost.go, the charge-before-decode ordering in DynamicGasPrecompile.RunAndCalculateGas, and the legacy/v66 snapshot repointing for v601/v603 precompiles. Also re-checked whether gov.voteWeighted's flat 30000 RequiredGas is exploitable via the same string-aliasing trick this PR prices for dynamic-gas precompiles (its WeightedVoteOption[] argument contains a string) — that pre-existing static-path gap was investigated again this round and not confirmed as a new issue introduced by this PR.
Extended reasoning...
This PR reprices ABI-decode gas for every dynamic-gas precompile (bank, distribution, ibc, wasmd, oracle, addr, pointer) by charging a calldata-decode cost before Unpack runs, closing a DoS where an attacker could force expensive string-copy amplification in go-ethereum's ABI decoder for near-zero forwarded gas. It also repoints the frozen v601/v603 legacy precompile snapshots to a new common/legacy/v66 package so their historical (already-committed) gas behavior stays frozen while the current precompile framework changes. The PR is labeled app-hash-breaking and carries an approved-but-still-evolving history: cursor bugbot caught two real defects in an intermediate static-path decode-cost surcharge (skipped-on-malformed-input, truncated 64-bit offset read), and the latest commit responded by removing that static-path walk entirely rather than patching it, on the reasoning that RequiredGas runs before the supplied-gas gate in vm.RunPrecompiledContract and so must stay allocation-free.
Security risk here is centered on gas-accounting correctness rather than injection/auth/data-exposure: an under-priced decode reopens the DoS this PR exists to close, while an over-priced or incorrectly-ordered charge could cause legitimate calls to revert or produce a different app hash than intended. The dynamic-gas path's charge-then-decode ordering, malformed-input rejection, and scoped panic-recovery (only sdk.ErrorOutOfGas/sdk.ErrorGasOverflow are converted to reverts, everything else re-panics) all look correct on inspection and are covered by new targeted tests (TestDynamicGasPrecompileGasGate, TestDynamicGasPrecompileRepanicsNonGas, the aliased-string cross-check against the real decoder in decode_cost_test.go).
Given the app-hash-breaking label, the breadth of precompiles touched, and the fact that this exact file has already needed multiple correction rounds from automated reviewers (including two real bugs from cursor bugbot that required removing rather than patching code), I believe this warrants a human maintainer's final sign-off rather than an automated approval, even with zero new findings this round.
The PR author has been responsive: the panic-recovery scoping a maintainer (masih) flagged as non-blocking was addressed in a follow-up commit, and the static-path decode-cost regression cursor bugbot found was resolved by removing the offending code and documenting the invariant instead of leaving a partial fix in place. Test coverage for the new cost model is reasonably thorough, including a direct comparison against the real ABI decoder for the aliased-string worst case.
Back out this PR's static-path changes; the static -> dynamic-gas conversion will land in a separate PR. RequiredGas and Prepare now match main exactly, and the resolveMethod helper is removed — its selector resolution is inlined into RunAndCalculateGas, the only remaining caller. The dynamic-gas decode charge and the legacy-common pinning are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
The PR correctly closes a real DoS by charging supplied EVM gas for ABI-decoding attacker-controlled calldata before decoding, and the decode-cost estimator is carefully built and cross-checked against go-ethereum's decoder. However, the new deferred recover() also spans d.executor.Execute, silently changing out-of-gas behavior during executor execution from "whole-tx abort" to "reverted call" for every dynamic precompile — an undocumented, app-hash-affecting semantic change that is not covered by any test.
Findings: 3 blocking | 4 non-blocking | 2 posted inline
Blockers
- Recover scope broadens out-of-gas semantics for all dynamic precompiles (staking, distribution, bank, wasmd, oracle, ibc, pointer, addr, solo), not just the decode charges. The executor runs under the finite per-call meter installed just above it, so an executor that exhausts its gas panics sdk.ErrorOutOfGas; previously that propagated (msg_server re-panics at msg_server.go:89, baseapp's OOG middleware converts it) and failed the whole EVM tx as out-of-gas, whereas now the deferred recover turns it into vm.ErrExecutionReverted (a normal reverted sub-call the caller can continue past). This is a consensus/app-hash change beyond the PR's stated decode-pricing scope. Please confirm it is intended; if so, document it in the PR/commit and add a test that an executor exhausting gas mid-execution now reverts the call (rather than aborting the tx). If unintended, narrow the recover to wrap only the decode gas charges (e.g. DecodeGasCost + the two ConsumeGas calls) so executor panics keep their prior propagation semantics. Note: the go-ethereum-fork half of this trace (no recover in core/vm's precompile call path) is inferred from go-ethereum's known structure since the fork sources were outside the sandbox; the sei-chain-side path was verified directly — worth a quick maintainer confirmation.
- Missing test coverage for the executor-out-of-gas-mid-execution path introduced by the new recover. Existing new tests cover the decode-charge gate (TestDynamicGasPrecompileGasGate) and non-gas re-panic (TestDynamicGasPrecompileRepanicsNonGas), but none exercise an executor that runs out of gas after the decode charges, which is exactly the path whose semantics changed.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- cursor-review.md is empty — the Cursor second-opinion pass produced no output. codex-review.md reports no material issues but notes it could not run focused tests because the sandbox Go module cache was read-only; treat the Codex pass as inconclusive on test results.
- Minor: decodeStringCopyBytes runs after only scanCost (length-proportional to len(input)) has been charged, yet its op count can reach maxDecodeWalkOps (2^20) for pathologically nested/aliased small inputs, so the scan work is not strictly bounded by the charged gas. This is bounded (~1M O(1) ops) and documented, so it is acceptable, but worth being aware of.
- Consider adding a benchmark or comment quantifying the worst-case wall-clock of a 2^20-op walk to document that the maxDecodeWalkOps backstop keeps the estimator itself cheap.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // (a call that could not afford the decode), keeping the failure from | ||
| // aborting the enclosing EVM frame. Re-panic anything else so genuine | ||
| // bugs are not masked as reverts. | ||
| if r := recover(); r != nil { |
There was a problem hiding this comment.
[blocker] This recover() spans the entire method, including d.executor.Execute below (which runs under the finite per-call meter installed at line 206). So an executor that exhausts its gas panics sdk.ErrorOutOfGas, and this recover now converts that to vm.ErrExecutionReverted — a per-call revert. Previously there was no recover here, so such a panic propagated out of the EVM, was re-panicked in msg_server.go (~line 89), and baseapp's out-of-gas recovery middleware failed the whole tx as out-of-gas.
That is an app-hash-affecting behavior change for every dynamic-gas precompile, beyond this PR's stated decode-pricing scope, and it is untested. If intended (it is arguably the more correct EVM semantics), please document it and add a test for an executor that runs out of gas mid-execution. If unintended, narrow the recover to wrap only the decode gas charges (DecodeGasCost + the two ConsumeGas calls) so executor panics retain their prior propagation semantics.
| } | ||
| // DecodeGasCost already includes scanCost; charge only the remaining | ||
| // (string-copy) portion so the decode is priced exactly once. | ||
| ctx.GasMeter().ConsumeGas(decodeCost-scanCost, fmt.Sprintf("%s precompile calldata decode", d.name)) |
There was a problem hiding this comment.
[nit] decodeCost - scanCost is safe from underflow (DecodeGasCost's base == scanCost == DefaultGasCost(input,false), and satAdd only grows it), so this is correct. Minor nit: charging in two ConsumeGas calls emits two gas-tracking entries ("...calldata scan" and "...calldata decode"); intentional and fine, just noting for anyone auditing gas traces.
3 participants
Summary
Dynamic-gas precompiles decoded their ABI argument payload in
Prepare()before the supplied EVM gas was turned into a Cosmos gas meter, so the cost of
parsing/allocating the calldata was not accounted for. The static precompile
path charges
RequiredGasup front invm.RunPrecompiledContract; that step isskipped on the dynamic-gas path.
This prices the decode and charges it before
Unpack, insideDynamicGasPrecompile.RunAndCalculateGas:resolveMethod),without decoding the argument payload.
decoding, so a call that cannot afford the decode is rejected before the
parse/allocation work runs.
Cost model
The Go ABI decoder's cost is dominated by copying
stringpayloads: itmaterializes each string via
string(output[begin:end]), and because onestring can be referenced by many array/tuple slots, the copied volume can grow
faster than
len(input).bytesvalues are excluded because the decoderreslices them without copying.
The charge is
ReadCostFlat + ReadCostPerByte * (len(input) + stringCopyVolume).stringCopyVolumeis computed bydecodeStringCopyBytes, a structural pre-scanthat mirrors go-ethereum's unpack traversal but only follows offsets and reads
length prefixes — it never copies payloads, so it stays linear in
len(input)even when the decode it prices is not. A defensive op-cap and a conservative
fallback cover structurally-inconsistent or unusually-nested input.
Because this lives in the shared framework, it covers every current dynamic-gas
precompile (distribution, staking, bank, wasmd, oracle, ibc, pointer, addr,
solo).
Compatibility
Only the current precompile version is affected. Historical replay (e.g. from
tracer endpoints) runs against the frozen
legacy/vNNNprecompile snapshots,which are unchanged, so no upgrade-height guard is required here.
Tests
decode_cost_test.go: unit tests for the cost estimator across string, bytes(zero-cost),
string[], tuple, a repeated-reference encoding, and malformedinput — the repeated-reference case is cross-checked against the real decoder.
TestDynamicGasPrecompileGasGate: a call that cannot afford the decode isrejected before the executor runs.
charge (distribution, bank, ibc, pointer).
All
precompiles/...package tests pass;gofmt/goimportsclean.🤖 Generated with Claude Code