Skip to content

fix: F-2026-18803 | [Dual Defense] Gasless MsgVoteChainMeta Can Inflate Unregistered ChainMetas Keys - #333

Merged
0xNilesh merged 2 commits into
audit-fixesfrom
F-2026-18803
Aug 26, 2026
Merged

fix: F-2026-18803 | [Dual Defense] Gasless MsgVoteChainMeta Can Inflate Unregistered ChainMetas Keys#333
0xNilesh merged 2 commits into
audit-fixesfrom
F-2026-18803

Conversation

@0xNilesh

Copy link
Copy Markdown
Member

Issue — nothing in the vote's path ever asks whether the chain exists

stage what it actually checks
CheckTx ValidateBasic signer parses, observed_chain_id != "", price > 0, height > 0 — shape only, no length or CAIP-2 cap
Ante gasless → fee + min-gas-price skipped
msgServer.VoteChainMeta IsBondedUniversalValidator + IsTombstonedwho votes, never what they vote on
Keeper.VoteChainMeta GetChainMeta miss → build entry → SetChainMetathe miss creates the row

chain_meta.go cold-start path (!bootstrapped && len(fresh) < chainMetaMinVotesForFirstWrite, = 3)
calls SetChainMeta and returns. The quorum of 3 gates the EVM oracle write, never the store
write
— votes have to accumulate somewhere to ever reach three.

Both axes are attacker-controlled. ChainMetas is collections.Map[string, types.ChainMeta]
with collections.StringKey, so the map key is the raw observed_chain_id, stored verbatim as the
IAVL key and uncapped. Unbounded row count and unbounded key size (a 100 KB id writes a 100 KB
key; donut max_bytes is 21 MB). The id is stored twice per row — key, plus field 1 of the value.

The real cost is the walk, not the disk. PruneValidatorVotes (gas_price.go:21) does an
unpaginated ChainMetas.Walk deserialising every key and value, and it is called from
AfterValidatorRemoved — EndBlock-class, unmetered. The attacker pays metered gas per row once
at write time; every node pays O(N) during consensus later.

Mitigating factor — stated honestly

gas_price.go:41-46: when the pruned validator is the row's last signer the entry is Removed,
not edited. Every fake row has exactly one signer, so ejecting the malicious UV sweeps the entire
set. The bloat persists only while the attacker stays bonded, and the consensus-time walk is the
only thing that actually bites. Combined with the bonded-UV precondition — an already-trusted role
with considerably worse options available elsewhere — this is genuinely Impact 1 / Likelihood 1.
It is fixed because the gate is one line and the invariant ("chain meta only exists for registered
chains") is worth having explicitly, not because the finding is severe.

Fix — two layers, because ValidateBasic is stateless

1. Registry gate in the keeper — at the very top of Keeper.VoteChainMeta, above GetChainMeta:

if _, err := k.uregistryKeeper.GetChainConfig(ctx, observedChainId); err != nil {
    ...
    return sdkerrors.Wrapf(err, "chain %s is not registered", observedChainId)
}

Position is load-bearing: the cold-start branch reaches SetChainMeta on the very first vote, so the
gate has to precede any state read or write. Mirrors VoteInbound
(msg_vote_inbound.go:32, "Check inbound enabled before any state changes"). The underlying
collections.ErrNotFound is wrapped, not swallowed.

Gate is on registered, deliberately not IsChainInboundEnabled: chain meta also feeds
gas-price quoting for outbounds, so an inbound-enabled check would starve outbound-only chains.

2. Length + CAIP-2 shape cap in ValidateBasic — stateless, so it cannot query the registry;
it gets the cheap CheckTx-time bound instead. MaxObservedChainIdLen = 128: CAIP-2 permits at most
8 + 1 + 32 = 41 characters and our longest real id is
solana:EtWTRABZaYq6iMfeYKouRu166VU2xqa1 (41), so 128 is generous headroom while still bounding the
key. Shape check reuses the existing types.ParseCAIP2 helper rather than adding a parser, and is
the same bar Inbound.ValidateBasic already applies to source_chain.

Layer 1 bounds the row count; layer 2 bounds each row's size. Neither alone suffices.

Declined / skipped Hacken recommendations

# Rec Call
2 Don't Set until bootstrap quorum Declined — incorrect as written. Reaching a quorum of 3 requires votes 1 and 2 to have persisted. Deferring the write makes bootstrap unreachable: the chain could never acquire a gas price at all.
3 Cap ChainMetas cardinality Skipped — redundant once N is registry-bounded (admin-controlled; 5 chains on donut)
4 Bound the EndBlock prune walk Skipped, noted — O(N) over an admin-controlled N is fine; revisit if the registry ever grows large

Tests

New x/uexecutor/keeper/chain_meta_test.go:

  • unregistered chain → error and no ChainMetas row, asserted by Has + a full Walk of the map
  • unregistered 207-char id → no key minted
  • registered chain → row created, signer/price/height/storedAt correct, LastAppliedChainHeight still 0 below quorum
  • registered chain → pre-bootstrap accumulation and in-place re-vote unchanged
  • registering one chain does not implicitly admit its neighbours

New x/uexecutor/types/msg_vote_chain_meta_test.go — 12 ValidateBasic table cases: both real id
formats accepted, boundary at exactly 128 accepted / 129 rejected, 100 KB id rejected, non-CAIP-2 /
empty-namespace / empty-reference rejected, and the four pre-existing checks still enforced.

New subtest in test/integration/uexecutor/vote_chain_meta_test.go driving the full authz →
msgServer → keeper path, asserting the store is unchanged and that the registered chain still votes
fine from the same validator immediately afterwards.

In both the keeper and integration tests the store assertion is placed before the error
assertion
, deliberately: the finding is the row being written, not a missing error, so that is the
assertion the mutation check has to break.

Mutation-verified

With the registry gate removed, the store assertions fail — including a dump of the 207-character
IAVL key the bug actually mints:

--- FAIL: TestVoteChainMeta_UnregisteredChain_RejectedAndStoreUnchanged
        Error:      Should be false
        Messages:   unregistered chain must not create a ChainMetas row
--- FAIL: TestVoteChainMeta_UnregisteredLongChainId_WritesNoKey
        Error:      Should be empty, but was [eip155:999999999999999999999...999]
        Messages:   no ChainMetas key may be minted for an unregistered id
--- FAIL: TestVoteChainMetaIntegration/vote_for_an_unregistered_chain_is_rejected_and_writes_no_ChainMetas_row
        Error:      Should be false
        Messages:   unregistered chain must not create a ChainMetas row

Gate restored; all pass again.

Verification

Full CI invocation on this branch — green, 0 failures:

go test -mod=readonly -tags="ledger test_ledger_mock test" ./x/... ./test/integration/...
ok  github.com/pushchain/push-chain-node/test/integration/ante          6.400s
ok  github.com/pushchain/push-chain-node/test/integration/uexecutor    15.583s
ok  github.com/pushchain/push-chain-node/test/integration/upgrades      9.324s
ok  github.com/pushchain/push-chain-node/test/integration/uregistry     5.251s
ok  github.com/pushchain/push-chain-node/test/integration/utss         11.893s
ok  github.com/pushchain/push-chain-node/test/integration/uvalidator    9.975s
ok  github.com/pushchain/push-chain-node/x/uexecutor/keeper             1.223s
ok  github.com/pushchain/push-chain-node/x/uexecutor/types              1.732s
ok  github.com/pushchain/push-chain-node/x/uregistry/keeper             2.902s
ok  github.com/pushchain/push-chain-node/x/uregistry/migrations/v3      1.288s
ok  github.com/pushchain/push-chain-node/x/uregistry/types              1.791s
ok  github.com/pushchain/push-chain-node/x/utss/keeper                  4.019s
ok  github.com/pushchain/push-chain-node/x/utss/types                   5.058s
ok  github.com/pushchain/push-chain-node/x/uvalidator/keeper            1.457s
ok  github.com/pushchain/push-chain-node/x/uvalidator/types             3.423s

Also green on the client-side packages that construct this message
(universalClient/pushsigner, universalClient/chains/..., utils).

No existing test needed changing: both chain-meta integration fixtures
(setupVoteChainMetaTest, setupValidatorPruningTest) already call
AddChainConfig for eip155:11155111, so every pre-existing vote is for a registered chain.
No CHANGELOG entry — the node repo generates it per release.

Gate Keeper.VoteChainMeta on uregistry before any state read/write, and cap
observed_chain_id length + CAIP-2 shape in ValidateBasic.
Assert no ChainMetas row is written, at keeper and integration level.
@0xNilesh
0xNilesh merged commit 5b4b744 into audit-fixes Aug 26, 2026
7 checks passed
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.

1 participant