Skip to content

fix(core+injected): replay-risk typed confirm + core stability hardening#5

Merged
Lykhoyda merged 2 commits intomainfrom
fix/chainid-0-typed-confirm
Apr 20, 2026
Merged

fix(core+injected): replay-risk typed confirm + core stability hardening#5
Lykhoyda merged 2 commits intomainfrom
fix/chainid-0-typed-confirm

Conversation

@Lykhoyda
Copy link
Copy Markdown
Owner

Summary

Two related hardening passes bundled because they share the safety surface.

G1 — EIP-7702 chainId=0 replay requires typed confirmation

Previously chainId=0 delegations were flagged CRITICAL but users could still proceed via a plain "Proceed Anyway" button. Because the signature can be replayed on every EVM chain, this now requires the same friction as eth_sign (typed "I ACCEPT THE RISK").

  • New replay-risk WarningContext wired through intent-builder.ts, decoder/index.ts, ModalButtons.tsx, warningVM.ts, and injected.tsx
  • Unit tests: buildReplayRiskIntent covered (3 tests)

Core package stability (audit gaps #1#3 + #6)

# Fix File
1 InvalidBytecodeError on non-hex / odd-length input parser.ts
2 Instruction.truncated flag + detectUnlimitedApproval now requires data.length === 32 parser.ts, detectors.ts, types.ts
3 Typed BytecodeFetchError (preserves cause); analyzer classifies errors into bytecode_fetch_failed / invalid_bytecode fetcher.ts, analyzer.ts
6 Proxy-chain cycle guard — _visitedAddresses set threaded through recursion; A → B → A returns proxy_cycle_detected analyzer.ts

Fail-open behavior preserved per CLAUDE.md — core analyzes, extension decides (ADR-006 / ADR-013).

Test plan

  • yarn workspace @testudo/extension run test366 pass (3 new replay-risk)
  • yarn workspace @testudo/core run test395 pass (22 new: parser edges, truncated PUSH32, fetcher mocked errors, cycle detection, error classification)
  • yarn workspace @testudo/extension run build — clean
  • yarn workspace @testudo/core run build — clean
  • yarn biome check packages/{core,extension}/{src,tests} — clean
  • Manual QA: craft a chainId=0 EIP-7702 typed data in mock-dapp; verify modal forces typed confirmation
  • E2E on CI

🤖 Generated with Claude Code

Lykhoyda and others added 2 commits April 16, 2026 21:17
…ploy)

Captures the client-side work needed to match testudo-api PR #5, which
moves threat/safe/encounter routes under /api/v1/chains/:chainId/...
Default to mainnet (chainId=1) when chainId is not yet resolved from
eth_chainId, so pingApi and first-paint analysis don't block on RPC.

Prerequisite for the larger 2.1 Multichain EVM Support work.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two related hardening passes bundled because they share the safety surface:

## Extension — G1: EIP-7702 chainId=0 replay requires typed confirmation
Previously chainId=0 delegations were flagged CRITICAL but users could still
proceed via a plain "Proceed Anyway" button. Because the signature can be
replayed on every EVM chain, this now requires the same friction as eth_sign.

- types.ts: add replay-risk to WarningContext union
- intent-builder.ts: add buildReplayRiskIntent (headline + all-networks impact)
- decoder/index.ts: dispatch replay-risk context to new builder
- ModalButtons.tsx: render EthSignConfirm for eth-sign-danger OR replay-risk
- warningVM.ts: accept optional context on updateAnalysis for context switch
- injected.tsx: set context=replay-risk when chainId=0, adjust error message
- intent-builder.test.ts: 3 new tests covering replay-risk intent

## Core — stability + edge cases
Found during codebase audit; closes gaps #1-#3 and #6.

Parser input validation (parser.ts, types.ts):
- Throw InvalidBytecodeError on non-hex chars or odd-length input
- Accept 0X prefix
- Add Instruction.truncated flag when PUSH data is cut short by EOF

Detector hardening (detectors.ts):
- detectUnlimitedApproval requires data.length === 32 (no more false
  positive on truncated PUSH32 of 0xFFs via .every() on short slice)

Fetcher error typing (fetcher.ts):
- Wrap getCode in try/catch, throw typed BytecodeFetchError with cause
- Treat undefined RPC response same as 0x

Analyzer hardening (analyzer.ts):
- Classify errors into bytecode_fetch_failed / invalid_bytecode /
  Analysis failed threat tags
- Proxy-chain cycle guard: pass _visitedAddresses through recursion;
  A to B to A returns proxy_cycle_detected instead of looping

Public API exports added: BytecodeFetchError, InvalidBytecodeError.

Fail-open behavior preserved per CLAUDE.md — core analyzes, extension decides.

Tests:
- Extension: 366 pass (3 new for replay-risk)
- Core:     395 pass (22 new: parser edges, truncated PUSH32, fetcher
            mocked errors, cycle detection, error classification)
@Lykhoyda Lykhoyda merged commit 68ffa09 into main Apr 20, 2026
6 checks passed
Lykhoyda added a commit that referenced this pull request Apr 20, 2026
testudo-api PR #5 moves client-facing threat routes under
`/api/v1/chains/:chainId/...`. Without this change, deploying the API
produces 404s on every threat lookup. Ship order: extension lands in
Chrome Web Store -> then API deploys.

## api-client.ts
- `ApiClientOptions.chainId?: number`; `checkAddressThreat()` and
  `pingApi()` build URLs as `/api/v1/chains/${chainId}/threats/address/...`
- `resolveChainId()` falls back to `1` (mainnet) for missing/non-finite
  /non-positive input — matches api-server's current behavior for
  missing chainId and keeps cold-start protection alive
- `ThreatResponse` adds optional `matchedChainId` + `crossChainMatch`
  (ignored in UI for now, reserved for future cross-chain surfacing)
- `checkDomainThreat()` unchanged — domain route stays global

## analysis.ts
- `AnalysisDeps.checkAddressThreat` accepts `{chainId?}`
- `analyzeWithCache()` / `addressCheckWithCache()` accept optional `chainId`
- Threaded through `performThreeLayerAnalysis` + `performAddressOnlyCheck`
- Cache key is now `${chainId}:${address}` — same address on different
  chains caches independently. `pendingFullAnalysis` /
  `pendingAddressCheck` use the same key for coalescing

## injected.tsx
- Lazy `getCurrentChainId()` via `eth_chainId` RPC, cached in-module,
  invalidated on wallet's `chainChanged` event
- `parseChainIdHex()` accepts number or 0x-hex; rejects 0/NaN/invalid
- Permit + typed-data flows prefer the typed domain's chainId,
  fall back to active chain
- EIP-7702 authorization uses the auth's chainId; when that is `0`
  (replay risk) the threat lookup falls back to the active chain so
  we still key into a real chain's registry instead of a sentinel

## background.ts + content.ts + services/messaging.ts
- chainId is forwarded end-to-end through the channel and runtime
  message bridge. Handlers treat non-number values as `undefined`
  so the api-client default applies at the URL boundary

## Tests (9 new, 375 total)
- api-client: URL has chains segment, custom chainId routed, default
  to 1 on missing / negative / NaN, pingApi routing, pingApi default
- analysis: chainId forwarded to checkAddressThreat for both flows,
  per-chain cache partitioning, default cache key, cross-chain cache
  isolation
- Existing tests updated to assert the new cache-key shape

Build + lint clean. No behavior change when chainId is omitted —
every path defaults to mainnet (1).
Lykhoyda added a commit that referenced this pull request Apr 20, 2026
testudo-api PR #5 moves client-facing threat routes under
`/api/v1/chains/:chainId/...`. Without this change, deploying the API
produces 404s on every threat lookup. Ship order: extension lands in
Chrome Web Store -> then API deploys.

## api-client.ts
- `ApiClientOptions.chainId?: number`; `checkAddressThreat()` and
  `pingApi()` build URLs as `/api/v1/chains/${chainId}/threats/address/...`
- `resolveChainId()` falls back to `1` (mainnet) for missing/non-finite
  /non-positive input — matches api-server's current behavior for
  missing chainId and keeps cold-start protection alive
- `ThreatResponse` adds optional `matchedChainId` + `crossChainMatch`
  (ignored in UI for now, reserved for future cross-chain surfacing)
- `checkDomainThreat()` unchanged — domain route stays global

## analysis.ts
- `AnalysisDeps.checkAddressThreat` accepts `{chainId?}`
- `analyzeWithCache()` / `addressCheckWithCache()` accept optional `chainId`
- Threaded through `performThreeLayerAnalysis` + `performAddressOnlyCheck`
- Cache key is now `${chainId}:${address}` — same address on different
  chains caches independently. `pendingFullAnalysis` /
  `pendingAddressCheck` use the same key for coalescing

## injected.tsx
- Lazy `getCurrentChainId()` via `eth_chainId` RPC, cached in-module,
  invalidated on wallet's `chainChanged` event
- `parseChainIdHex()` accepts number or 0x-hex; rejects 0/NaN/invalid
- Permit + typed-data flows prefer the typed domain's chainId,
  fall back to active chain
- EIP-7702 authorization uses the auth's chainId; when that is `0`
  (replay risk) the threat lookup falls back to the active chain so
  we still key into a real chain's registry instead of a sentinel

## background.ts + content.ts + services/messaging.ts
- chainId is forwarded end-to-end through the channel and runtime
  message bridge. Handlers treat non-number values as `undefined`
  so the api-client default applies at the URL boundary

## Tests (9 new, 375 total)
- api-client: URL has chains segment, custom chainId routed, default
  to 1 on missing / negative / NaN, pingApi routing, pingApi default
- analysis: chainId forwarded to checkAddressThreat for both flows,
  per-chain cache partitioning, default cache key, cross-chain cache
  isolation
- Existing tests updated to assert the new cache-key shape

Build + lint clean. No behavior change when chainId is omitted —
every path defaults to mainnet (1).
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