Skip to content

fix: ics02 and ics20 gas calculation#1164

Merged
vladjdk merged 9 commits into
mainfrom
vlad/fix-ics02-gas
May 5, 2026
Merged

fix: ics02 and ics20 gas calculation#1164
vladjdk merged 9 commits into
mainfrom
vlad/fix-ics02-gas

Conversation

@vladjdk
Copy link
Copy Markdown
Member

@vladjdk vladjdk commented May 5, 2026

Replaces zero-valued gas configs with instantiated values in ICS02, and charges for gas in ICS20.

@vladjdk vladjdk requested a review from a team as a code owner May 5, 2026 18:59
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 5, 2026

Greptile Summary

This PR fixes zero-gas DoS vulnerabilities in the ICS02 and ICS20 precompiles by ensuring KV store operations are properly metered. The ICS02 constructor now uses storetypes.KVGasConfig() and storetypes.TransientGasConfig() instead of zero-valued structs, and the ICS20 transferWithStateDB no longer overrides the context's gas configs with empty ones mid-call.

  • ICS02: Zero-valued GasConfig{} in NewPrecompile caused RequiredGas to return 0 for every call; replaced with the proper SDK defaults so callers are charged based on calldata size.
  • ICS20: A block that saved/zeroed/restored KV gas configs inside transferWithStateDB has been removed, meaning IBC transfer execution now charges for KV store reads/writes at the standard rate; the defer-restore was already a no-op (operating on a by-value ctx copy) so no state-restore behavior is lost.
  • Tests: A new unit test pins the RequiredGas formula against the SDK KV-cost constants for all ICS02 methods, and the UpdateClient integration test gas limit is bumped to 500,000 to reflect the real cost now charged.

Confidence Score: 5/5

Safe to merge — both changes are straightforward removals of zero-gas overrides, backed by a new unit test and a passing integration test with an updated gas budget.

The ICS02 constructor change is a one-line swap to a well-known SDK helper, and the ICS20 removal eliminates code whose defer-restore was already a no-op. The new unit test explicitly pins the cost formula, and the integration test confirms the real-world gas budget. No new code paths are introduced that could regress unrelated behavior.

No files require special attention.

Important Files Changed

Filename Overview
precompiles/ics02/ics02.go Replaces zero-valued GasConfig{} with storetypes.KVGasConfig() and storetypes.TransientGasConfig(), closing a zero-gas DoS bypass where every call was free regardless of calldata size.
precompiles/ics20/tx.go Removes the block that temporarily zeroed KV gas configs inside transferWithStateDB; KV store operations during ICS20 transfers now charge gas at the standard rate set in the constructor.
precompiles/ics02/ics02_test.go New unit test guards against regression of the zero-gas DoS bypass; verifies RequiredGas returns non-zero values and matches the SDK KV-cost formula for all ICS02 methods.
evmd/tests/ibc/ics02_precompile_client_test.go Integration test gas limit for UpdateClient bumped from 200,000 to 500,000 to accommodate the real KV-store gas now charged after the fix.

Sequence Diagram

sequenceDiagram
    participant Caller as EVM Caller
    participant EVM as EVM Engine
    participant PC as Precompile (ICS02/ICS20)
    participant RNA as runNativeAction
    participant KV as KV Store

    Caller->>EVM: call precompile (calldata)
    EVM->>PC: RequiredGas(input)
    Note over PC: KVGasConfig().WriteCostFlat<br/>+ WriteCostPerByte*len(input)<br/>(was 0 before fix)
    PC-->>EVM: base gas charged up front

    EVM->>PC: Run(evm, contract, readonly)
    PC->>RNA: RunNativeAction(action)
    Note over RNA: ctx = ctx.WithKVGasConfig(p.KvGasConfig)<br/>.WithTransientKVGasConfig(p.TransientKVGasConfig)
    RNA->>PC: action(ctx)

    alt ICS02 UpdateClient / VerifyMembership
        PC->>KV: read/write client state
        Note over KV: gas metered at KVGasConfig() rate
    else ICS20 Transfer (after fix)
        PC->>KV: read/write during transfer
        Note over KV: gas metered normally (zero-override block removed)
    end

    KV-->>PC: result
    PC-->>RNA: (bz, err)
    Note over RNA: cost = GasConsumed - initialGas<br/>contract.UseGas(cost)
    RNA-->>EVM: bz
    EVM-->>Caller: result
Loading

Reviews (3): Last reviewed commit: "fix tests" | Re-trigger Greptile

Comment thread precompiles/ics02/ics02_test.go
@vladjdk
Copy link
Copy Markdown
Member Author

vladjdk commented May 5, 2026

@greptile review

@vladjdk
Copy link
Copy Markdown
Member Author

vladjdk commented May 5, 2026

@greptile review

@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.20%. Comparing base (64e859b) to head (472b830).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1164      +/-   ##
==========================================
- Coverage   64.31%   64.20%   -0.12%     
==========================================
  Files         336      336              
  Lines       24054    24063       +9     
==========================================
- Hits        15471    15450      -21     
- Misses       6973     6991      +18     
- Partials     1610     1622      +12     
Files with missing lines Coverage Δ
precompiles/ics02/ics02.go 84.61% <100.00%> (+3.84%) ⬆️
precompiles/ics20/tx.go 49.62% <ø> (-22.16%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@mattac21 mattac21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should add a changelog entry for this. also do you think we need to note this in the upgrade guide?

@vladjdk vladjdk enabled auto-merge May 5, 2026 20:13
@vladjdk vladjdk added this pull request to the merge queue May 5, 2026
Merged via the queue into main with commit 727f3b9 May 5, 2026
19 of 20 checks passed
@vladjdk vladjdk deleted the vlad/fix-ics02-gas branch May 5, 2026 20:40
@mattac21
Copy link
Copy Markdown
Contributor

mattac21 commented May 5, 2026

@mergify backport to release/v0.7.x

@mergify
Copy link
Copy Markdown

mergify Bot commented May 5, 2026

backport to release/v0.7.x

❌ No backport have been created

Details
  • Backport to branch to failed

GitHub error: Branch not found

mattac21 pushed a commit that referenced this pull request May 5, 2026
* fix ics02 gas

* lints

* fix greptile comments

* Revert "fix greptile comments"

This reverts commit beb4078.

* fix greptile 02

* revert viewonly

* fix tests

* changelog

(cherry picked from commit 727f3b9)

Co-authored-by: Vlad J <vladjdk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants