The vulnerability this closes, which is live on next today
next compares from, to, value and data and nothing else. It does not
look at the transaction type. Measured against origin/next's own src/shared/approvalVerify.js with the repo's ethers 6.16.0:
NEXT TYPE4 parsed.type=4 delegates_to=0xdAC17F958D2ee523a2206206994597C13D831ec7 => PASSED VERIFICATION
The approval is the ordinary dApp eth_sendTransaction shape
({from, to, value, data}, no fee fields, which is the normal case because populateTransaction() fills them). The artifact is an EIP-7702 type 4
transaction with identical to / value / data / chainId / from, plus a
signed authorization delegating the signer's own EOA to an attacker address.
The user approves a plain ETH transfer; the broadcast also installs attacker
code at the user's own account, permanently. Every field on the approval screen
matches. eth_sendRawTransaction accepts type 4 on both supported networks
post-Pectra, so this is live, not theoretical.
The first revision of this PR did not close it: it added per-field comparisons
but left parsed.type unconstrained and never looked at authorizationList.
Same result on that revision. Type 3 had the same shape: blobVersionedHashes
uncompared, maxFeePerBlobGas absent from the fee ceilings, and type 3
explicitly admitted as EIP-1559.
Why this is now an allowlist, and what makes it exhaustive
A field-by-field denylist cannot be correct against a transaction format that
gains fields: every new EIP-2718 type adds consequential content that defaults
to unchecked. The check is now closed in both directions.
1. The type is allowlisted.parsed.type must be 0, 1 or 2 — the only
types this wallet signs, since populateTransaction() produces nothing else.
Anything else is refused before a single field is compared, because the type is
what decides which fields exist at all. Types 3 and 4 are refused by this, not
incidentally by a fee check that happens to fire only when the approval carried
a fee.
2. Fields no allowed type may carry are refused by name. authorizationList, blobs, blobVersionedHashes and maxFeePerBlobGas.
Redundant with the type allowlist by construction — that is the point — and
each has its own test, because nothing reachable would otherwise exercise them.
3. The access list is compared with the approval. It was previously
uncompared on both types that carry it.
4. Verification closes by rebuilding the artifact from the checked fields
and comparing the bytes.SERIALIZED_FIELDS names the serialized fields of
each allowed type; the transaction is rebuilt from exactly those and unsignedSerialized is compared. This is what makes the approach exhaustive
rather than one bug better: any field the artifact carries that this module
does not account for is absent from the rebuild, changes the bytes, and is
refused without having to be anticipated. The final assertion is that the
artifact is the approved transaction, not that it is none of the tampered
shapes someone thought of.
5. The artifact's own bytes are required to be canonical. Both sides of the
byte comparison derive from one Transaction.from(), while what is broadcast
is the artifact string. An artifact re-encoded with a leading zero byte on an
RLP quantity therefore decoded to the approved transaction, passed, and
broadcast different bytes. assertCanonicalBytes() requires the artifact to be
the canonical encoding of its own decode, which is what makes "this is the
approved transaction" true of the bytes that actually go to the node.
6. A tripwire on ethers itself. One test asserts that the set of accessors Transaction.prototype exposes is exactly the set this module accounts for —
checked, refused by name, or derived (from, hash, signature, the
serializations). An ethers upgrade that introduces a transaction field fails
the suite and forces a decision about it, instead of letting it default to
unchecked.
Post-fix, against this branch head:
TYPE4 parsed.type=4 delegates_to=0xdAC17F958D2ee523a2206206994597C13D831ec7
=> REFUSED (approvalMismatch=true): The signed transaction is of a type this
wallet does not sign, so what it would do beyond the approved transfer
cannot be checked.
TYPE3 parsed.type=3
=> REFUSED (approvalMismatch=true): The signed transaction is of a type this
wallet does not sign, so what it would do beyond the approved transfer
cannot be checked.
TYPE2 + unapproved access list
=> REFUSED (approvalMismatch=true): The signed transaction does not carry the
approved access list.
TYPE2 honest artifact
=> PASSED VERIFICATION
The type 4 probe, the type 3 probe and the access list probe are all committed
regression tests, not just evidence.
One approval can no longer send funds twice
A broadcastTransaction() throw previously left the approval pending and retryable: true. The popup's retry does not re-broadcast the artifact it
already produced — it re-runs populateTransaction() and signTransaction(),
minting a different transaction at a freshly fetched pending-tag nonce. A
broadcast that throws after the node accepted the transaction is routine (a
timeout, a dropped response, a node answering "already known"), so tx1 sits in
the mempool, the retry takes nonce N+1, and the approved transfer executes
twice.
Broadcast failure is now terminal: the approval is spent, the error is resolved
to the dApp, and the popup does not offer the button again. The popup-side msg.error path that #174
actually asked about — the wrong-password case — stays retryable, and a
verification mismatch still spends the approval. The three stages are one
exported decision function, describeTxFailure(stage, err), with a test each.
A failed broadcast also gets its own user-facing wording. Telling the user to
"start it again from the site" after a broadcast whose outcome is unknown is
the one instruction that could produce the double spend by hand; it now says
the transaction may still have reached the network and to check the account
first.
The interlock, and the single chokepoint that makes it hold
Keeping the approval alive so a wrong password can be retried costs it its
single use. The handler reads the approval, then verifies and broadcasts
asynchronously, so a second AUTISTMASK_TX_RESPONSE carrying the same id would
start an independent verify and broadcast. Nothing in the ordinary dApp
approval shape fixes a nonce, so two artifacts signed at different nonces both
verify. The approval is therefore claimed synchronously, before the handler's
first await, and released only when an attempt fails in a way the user may
retry.
Surviving the whole verify-and-broadcast window also put the approval within
reach of every other path that retires one, and those did not consult the
claim:
windows.onRemoved — the user closes the approval popup.
broadcastAccountsChanged() — the user switches active address.
a late !msg.approved reject on either response type.
Each resolved the waiting promise 4001 User rejected the request. while the
attempt behind it ran to completion. The attempt's own resolve({txHash}) then
landed on an already-settled promise: the transaction reached the chain and
the page was told the user rejected it. The natural response is to redo the
transfer from the site, which re-signs at a fresh nonce — the double send this
PR exists to prevent, reached with no adversary at all, since src/popup/views/approval.js keeps the popup open across the broadcast and a
user closing an apparently-hung window is enough.
The fix is one chokepoint, not three patched call sites. settleApproval(id, result, {holdsClaim}) is now the only place an
approval is resolved or removed — grep over src/background/index.js finds
exactly one delete pendingApprovals[...] and one approval.resolve(...),
both inside it — and it refuses a claimed approval unless the caller holds the
claim. A retirement path added later inherits the interlock instead of having
to remember it. broadcastAccountsChanged() additionally leaves a claimed
approval's window standing rather than force-closing the window the attempt is
reporting into.
The duplicate refusal on the sign path previously omitted stage, so the popup
told the user to start again from the site while the first attempt might still
succeed. Both in-flight refusals now carry a stage whose wording says the first
attempt is still running and may still succeed.
An approved value that is not a number
normalizeValue() called BigInt(v) bare. A page-controlled value of "cheap", 1.5, "1e18" or {} threw a raw SyntaxError/RangeError with approvalMismatch === undefined, so it was reported retryable, the approval was
never spent and approval.resolve was never called — the exact dead-button
shape #174 exists to remove. value now goes through normalizeQuantity() like every other quantity and
refuses as a mismatch.
an approval naming EIP-1559 fees must not be signed as legacy, and one naming gasPrice must not be signed as EIP-1559
the artifact's own encoding
must be the canonical encoding of its decode
everything else
refused by the closing byte comparison
Why nonce, gas limit and fees are conditional, not blanket equality
The dApp usually fixes none of them: populateTransaction() in the popup fills
in nonce, gas limit, fees and chain id from the provider. There is then no
approved value to compare against, and a blanket equality check would refuse
every legitimate transaction. Those locally populated values are instead held
to two absolute ceilings, chosen so that nothing includable is ever refused:
gas limit <= 100,000,000 — above the block gas limit of both supported
networks (src/shared/networks.js).
each fee per gas <= 100,000 gwei — far above anything mainnet or Sepolia
has produced.
These are a sanity bound, which is what #174 asked for, not a limit
on loss: a bare 21,000-gas transfer at the fee ceiling still hands the
validator 2.1 ETH. The user is protected from the absurd, not from the ruinous.
Tightening them is a separate product decision.
The chain id needs no such carve-out: the background knows the selected network
independently of the artifact, which is what makes a cross-chain replay
impossible.
Known gap, deliberately left alone: a dApp-supplied gas (the JSON-RPC
spelling) is not compared, because ethers' copyRequest drops the key and
the popup estimates instead — so the signed gas limit is not an approved value.
Normalization rules
Addresses: compared through EIP-55 checksumming, so case never matters;
absent on both sides is equal (contract creation), absent on one side is not.
Quantities (chainId, nonce, gasLimit, all fee fields, value):
coerced to BigInt, so hex, decimal string, number and bigint compare equal.
A value that is not a number refuses rather than passes.
value: absent means zero, matching what ethers signs.
Call data: lowercased; absent, "" and 0x are the same thing.
Access list: normalized through ethers' accessListify, then lowercased;
absent and [] are equal. A malformed approved list refuses.
Optional fields (nonce, gasLimit, fees): absent means not approved,
never zero.
Verification
make check on the branch rebased onto next at bd4bdca:
Test Suites: 20 passed, 20 total
Tests: 482 passed, 482 total
All matched files use Prettier code style!
Also run in the container. script/lint is prettier --check on the host, so
the gating run is docker build --no-cache ., which runs make check and make build inside the image. The check layer executed, uncached — no CACHED
on it:
#14 [7/8] RUN make check
#14 9.198 Test Suites: 20 passed, 20 total
#14 9.198 Tests: 482 passed, 482 total
#14 12.71 All matched files use Prettier code style!
#14 16.35 All matched files use Prettier code style!
#14 DONE 16.5s
#15 2.466 verify-build: 4 bundle(s) verified autistmask-build-debug=off
#15 DONE 2.5s
The background wiring is now driven end to end
tests/backgroundApproval.test.js loads src/background/index.js against
stubbed browser and network APIs and drives it through the real message
listener, from a dApp eth_sendTransaction to the broadcast. The approval
verification is the real module. windows.onRemoved is captured, not stubbed
as a no-op — swallowing it is what let the retirement defect through a
previous revision.
Six tests cover the retirement paths. Negative-verified: with the claim check
in settleApproval() disabled, the first four fail with the exact defect
signature and the two controls still pass.
x closing the approval window mid-broadcast still reports the result
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x switching the active address mid-broadcast still reports the result
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject arriving mid-broadcast is refused, not honoured
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject during a sign attempt is refused with the in-flight stage
Expected: ObjectContaining {"retryable": false, "stage": "inflight"}
+ with no attempt running, closing the window still rejects
+ with no attempt running, an active-address switch still rejects and closes
Tests: 4 failed, 2 passed
With the guard in place all six pass, the dApp receives {result: "0xfeed"} in
each mid-broadcast case, and broadcastTransaction is called exactly once. The
two controls are there so the refusals cannot quietly cost a genuine rejection
its meaning.
Alongside them, the duplicate-response tests: a second AUTISTMASK_TX_RESPONSE, the same artifact twice, a response arriving after
the broadcast finished, a cross-type AUTISTMASK_SIGN_RESPONSE, a retryable
failure leaving the approval usable, a mismatch spending it outright, and a
page sender refused.
Mutation matrix
Every comparison in src/shared/approvalVerify.js, disabled one at a time in
the working tree with the suite re-run against it and the file restored from
git afterwards. 25 of 25 mutants killed:
01 signer address KILLED 02 transaction type allowlist KILLED
03 forbidden fields KILLED 04 selected network present KILLED
05 chain id vs selected network KILLED 06 chain id vs approval KILLED
07 recipient KILLED 08 value KILLED
09 call data KILLED 10 access list KILLED
11 fee mechanism KILLED 12 approved nonce KILLED
13 approved gas limit KILLED 14 approved gas price KILLED
15 approved max fee per gas KILLED 16 approved max priority fee KILLED
17 gas limit ceiling KILLED 18 fee per gas ceiling KILLED
19 structural rebuild KILLED 20 value refusal routing KILLED
21 quantity refusal KILLED 22 access list refusal KILLED
23 broadcast failure terminal KILLED 24 verify failure mismatch KILLED
25 broadcast wording KILLED
Mutants 03 and 19 — the two layers that sit behind the type allowlist — first
survived, because nothing reachable through verifySignedTx can trip them
while the allowlist holds. Rather than leave two untested guards, they were
extracted as assertNoForbiddenFields() and assertNothingUnchecked() and
given direct tests, and both now die.
Not verified
The popup paths are not reachable from the unit suite (no DOM environment),
so the retry and stage behaviour is covered at the decision functions the
popup and background call, not by driving the button. make test-e2e does
not yet cover the approval flow.
Pre-existing, unchanged by this diff, and noted rather than fixed here: txParams.from is never compared, so an active-address switch between
approval and signing yields a transaction from an account the approval did
not name. Verification also compares against the dApp's request, never
against what the popup displayed — for every field the dApp omitted, the
number the user read on the approval screen is verified by nothing. The
structural fix is for the background to populate the transaction itself and
hand the complete approved set to the popup, which is a larger change.
script/lint is prettier --check on the host; there is no ESLint in the
check chain yet (#152).
Everything reported above was additionally run inside the container.
Closes [#174](https://git.eeqj.de/sneak/AutistMask/issues/174).
## The vulnerability this closes, which is live on `next` today
`next` compares `from`, `to`, `value` and `data` and nothing else. It does not
look at the transaction type. Measured against `origin/next`'s own
`src/shared/approvalVerify.js` with the repo's ethers 6.16.0:
```
NEXT TYPE4 parsed.type=4 delegates_to=0xdAC17F958D2ee523a2206206994597C13D831ec7 => PASSED VERIFICATION
```
The approval is the ordinary dApp `eth_sendTransaction` shape
(`{from, to, value, data}`, no fee fields, which is the normal case because
`populateTransaction()` fills them). The artifact is an EIP-7702 type 4
transaction with identical `to` / `value` / `data` / `chainId` / `from`, plus a
signed authorization delegating the signer's own EOA to an attacker address.
The user approves a plain ETH transfer; the broadcast also installs attacker
code at the user's own account, permanently. Every field on the approval screen
matches. `eth_sendRawTransaction` accepts type 4 on both supported networks
post-Pectra, so this is live, not theoretical.
The first revision of this PR did not close it: it added per-field comparisons
but left `parsed.type` unconstrained and never looked at `authorizationList`.
Same result on that revision. Type 3 had the same shape: `blobVersionedHashes`
uncompared, `maxFeePerBlobGas` absent from the fee ceilings, and type 3
explicitly admitted as EIP-1559.
## Why this is now an allowlist, and what makes it exhaustive
A field-by-field denylist cannot be correct against a transaction format that
gains fields: every new EIP-2718 type adds consequential content that defaults
to unchecked. The check is now closed in both directions.
**1. The type is allowlisted.** `parsed.type` must be 0, 1 or 2 — the only
types this wallet signs, since `populateTransaction()` produces nothing else.
Anything else is refused before a single field is compared, because the type is
what decides which fields exist at all. Types 3 and 4 are refused by this, not
incidentally by a fee check that happens to fire only when the approval carried
a fee.
**2. Fields no allowed type may carry are refused by name.**
`authorizationList`, `blobs`, `blobVersionedHashes` and `maxFeePerBlobGas`.
Redundant with the type allowlist by construction — that is the point — and
each has its own test, because nothing reachable would otherwise exercise them.
**3. The access list is compared with the approval.** It was previously
uncompared on both types that carry it.
**4. Verification closes by rebuilding the artifact from the checked fields
and comparing the bytes.** `SERIALIZED_FIELDS` names the serialized fields of
each allowed type; the transaction is rebuilt from exactly those and
`unsignedSerialized` is compared. This is what makes the approach exhaustive
rather than one bug better: any field the artifact carries that this module
does not account for is absent from the rebuild, changes the bytes, and is
refused **without having to be anticipated**. The final assertion is that the
artifact *is* the approved transaction, not that it is none of the tampered
shapes someone thought of.
**5. The artifact's own bytes are required to be canonical.** Both sides of the
byte comparison derive from one `Transaction.from()`, while what is broadcast
is the artifact string. An artifact re-encoded with a leading zero byte on an
RLP quantity therefore decoded to the approved transaction, passed, and
broadcast different bytes. `assertCanonicalBytes()` requires the artifact to be
the canonical encoding of its own decode, which is what makes "this *is* the
approved transaction" true of the bytes that actually go to the node.
**6. A tripwire on ethers itself.** One test asserts that the set of accessors
`Transaction.prototype` exposes is exactly the set this module accounts for —
checked, refused by name, or derived (`from`, `hash`, `signature`, the
serializations). An ethers upgrade that introduces a transaction field fails
the suite and forces a decision about it, instead of letting it default to
unchecked.
Post-fix, against this branch head:
```
TYPE4 parsed.type=4 delegates_to=0xdAC17F958D2ee523a2206206994597C13D831ec7
=> REFUSED (approvalMismatch=true): The signed transaction is of a type this
wallet does not sign, so what it would do beyond the approved transfer
cannot be checked.
TYPE3 parsed.type=3
=> REFUSED (approvalMismatch=true): The signed transaction is of a type this
wallet does not sign, so what it would do beyond the approved transfer
cannot be checked.
TYPE2 + unapproved access list
=> REFUSED (approvalMismatch=true): The signed transaction does not carry the
approved access list.
TYPE2 honest artifact
=> PASSED VERIFICATION
```
The type 4 probe, the type 3 probe and the access list probe are all committed
regression tests, not just evidence.
## One approval can no longer send funds twice
A `broadcastTransaction()` throw previously left the approval pending and
`retryable: true`. The popup's retry does not re-broadcast the artifact it
already produced — it re-runs `populateTransaction()` and `signTransaction()`,
minting a different transaction at a freshly fetched pending-tag nonce. A
broadcast that throws *after* the node accepted the transaction is routine (a
timeout, a dropped response, a node answering "already known"), so tx1 sits in
the mempool, the retry takes nonce N+1, and the approved transfer executes
twice.
Broadcast failure is now terminal: the approval is spent, the error is resolved
to the dApp, and the popup does not offer the button again. The popup-side
`msg.error` path that [#174](https://git.eeqj.de/sneak/AutistMask/issues/174)
actually asked about — the wrong-password case — stays retryable, and a
verification mismatch still spends the approval. The three stages are one
exported decision function, `describeTxFailure(stage, err)`, with a test each.
A failed broadcast also gets its own user-facing wording. Telling the user to
"start it again from the site" after a broadcast whose outcome is unknown is
the one instruction that could produce the double spend by hand; it now says
the transaction may still have reached the network and to check the account
first.
### The interlock, and the single chokepoint that makes it hold
Keeping the approval alive so a wrong password can be retried costs it its
single use. The handler reads the approval, then verifies and broadcasts
asynchronously, so a second `AUTISTMASK_TX_RESPONSE` carrying the same id would
start an independent verify and broadcast. Nothing in the ordinary dApp
approval shape fixes a nonce, so two artifacts signed at different nonces both
verify. The approval is therefore *claimed* synchronously, before the handler's
first `await`, and released only when an attempt fails in a way the user may
retry.
Surviving the whole verify-and-broadcast window also put the approval within
reach of every **other** path that retires one, and those did not consult the
claim:
- `windows.onRemoved` — the user closes the approval popup.
- `broadcastAccountsChanged()` — the user switches active address.
- a late `!msg.approved` reject on either response type.
Each resolved the waiting promise `4001 User rejected the request.` while the
attempt behind it ran to completion. The attempt's own `resolve({txHash})` then
landed on an already-settled promise: **the transaction reached the chain and
the page was told the user rejected it.** The natural response is to redo the
transfer from the site, which re-signs at a fresh nonce — the double send this
PR exists to prevent, reached with no adversary at all, since
`src/popup/views/approval.js` keeps the popup open across the broadcast and a
user closing an apparently-hung window is enough.
The fix is one chokepoint, not three patched call sites.
`settleApproval(id, result, {holdsClaim})` is now the **only** place an
approval is resolved or removed — `grep` over `src/background/index.js` finds
exactly one `delete pendingApprovals[...]` and one `approval.resolve(...)`,
both inside it — and it refuses a claimed approval unless the caller holds the
claim. A retirement path added later inherits the interlock instead of having
to remember it. `broadcastAccountsChanged()` additionally leaves a claimed
approval's window standing rather than force-closing the window the attempt is
reporting into.
The duplicate refusal on the sign path previously omitted `stage`, so the popup
told the user to start again from the site while the first attempt might still
succeed. Both in-flight refusals now carry a stage whose wording says the first
attempt is still running and may still succeed.
## An approved `value` that is not a number
`normalizeValue()` called `BigInt(v)` bare. A page-controlled `value` of
`"cheap"`, `1.5`, `"1e18"` or `{}` threw a raw `SyntaxError`/`RangeError` with
`approvalMismatch === undefined`, so it was reported retryable, the approval was
never spent and `approval.resolve` was never called — the exact dead-button
shape [#174](https://git.eeqj.de/sneak/AutistMask/issues/174) exists to remove.
`value` now goes through `normalizeQuantity()` like every other quantity and
refuses as a mismatch.
## Fields compared
| field | rule |
| --- | --- |
| transaction type | must be 0, 1 or 2; anything else refuses |
| `authorizationList`, `blobs`, `blobVersionedHashes`, `maxFeePerBlobGas` | must be absent |
| `chainId` | must equal the **selected network**'s chain id, always; and the approval's `chainId` too when the page fixed one. An unknown selected network refuses. |
| `to`, `value`, `data` | equality |
| `accessList` | equality; absent and `[]` are the same thing |
| `nonce`, `gasLimit`, `gasPrice`, `maxFeePerGas`, `maxPriorityFeePerGas` | equality when the approval carries one |
| fee mechanism | an approval naming EIP-1559 fees must not be signed as legacy, and one naming `gasPrice` must not be signed as EIP-1559 |
| the artifact's own encoding | must be the canonical encoding of its decode |
| everything else | refused by the closing byte comparison |
### Why nonce, gas limit and fees are conditional, not blanket equality
The dApp usually fixes none of them: `populateTransaction()` in the popup fills
in nonce, gas limit, fees and chain id from the provider. There is then no
approved value to compare against, and a blanket equality check would refuse
every legitimate transaction. Those locally populated values are instead held
to two absolute ceilings, chosen so that nothing includable is ever refused:
- gas limit <= 100,000,000 — above the block gas limit of both supported
networks (`src/shared/networks.js`).
- each fee per gas <= 100,000 gwei — far above anything mainnet or Sepolia
has produced.
These are a sanity bound, which is what
[#174](https://git.eeqj.de/sneak/AutistMask/issues/174) asked for, not a limit
on loss: a bare 21,000-gas transfer at the fee ceiling still hands the
validator 2.1 ETH. The user is protected from the absurd, not from the ruinous.
Tightening them is a separate product decision.
The chain id needs no such carve-out: the background knows the selected network
independently of the artifact, which is what makes a cross-chain replay
impossible.
Known gap, deliberately left alone: a dApp-supplied `gas` (the JSON-RPC
spelling) is **not** compared, because ethers' `copyRequest` drops the key and
the popup estimates instead — so the signed gas limit is not an approved value.
### Normalization rules
- **Addresses**: compared through EIP-55 checksumming, so case never matters;
absent on both sides is equal (contract creation), absent on one side is not.
- **Quantities** (`chainId`, `nonce`, `gasLimit`, all fee fields, `value`):
coerced to `BigInt`, so hex, decimal string, number and bigint compare equal.
A value that is not a number refuses rather than passes.
- **`value`**: absent means zero, matching what ethers signs.
- **Call data**: lowercased; absent, `""` and `0x` are the same thing.
- **Access list**: normalized through ethers' `accessListify`, then lowercased;
absent and `[]` are equal. A malformed approved list refuses.
- **Optional fields** (`nonce`, `gasLimit`, fees): absent means *not approved*,
never zero.
## Verification
`make check` on the branch rebased onto `next` at `bd4bdca`:
```
Test Suites: 20 passed, 20 total
Tests: 482 passed, 482 total
All matched files use Prettier code style!
```
Also run in the container. `script/lint` is `prettier --check` on the host, so
the gating run is `docker build --no-cache .`, which runs `make check` and
`make build` inside the image. The check layer executed, uncached — no `CACHED`
on it:
```
#14 [7/8] RUN make check
#14 9.198 Test Suites: 20 passed, 20 total
#14 9.198 Tests: 482 passed, 482 total
#14 12.71 All matched files use Prettier code style!
#14 16.35 All matched files use Prettier code style!
#14 DONE 16.5s
#15 2.466 verify-build: 4 bundle(s) verified autistmask-build-debug=off
#15 DONE 2.5s
```
### The background wiring is now driven end to end
`tests/backgroundApproval.test.js` loads `src/background/index.js` against
stubbed browser and network APIs and drives it through the **real** message
listener, from a dApp `eth_sendTransaction` to the broadcast. The approval
verification is the real module. `windows.onRemoved` is **captured, not stubbed
as a no-op** — swallowing it is what let the retirement defect through a
previous revision.
Six tests cover the retirement paths. Negative-verified: with the claim check
in `settleApproval()` disabled, the first four fail with the exact defect
signature and the two controls still pass.
```
x closing the approval window mid-broadcast still reports the result
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x switching the active address mid-broadcast still reports the result
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject arriving mid-broadcast is refused, not honoured
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject during a sign attempt is refused with the in-flight stage
Expected: ObjectContaining {"retryable": false, "stage": "inflight"}
+ with no attempt running, closing the window still rejects
+ with no attempt running, an active-address switch still rejects and closes
Tests: 4 failed, 2 passed
```
With the guard in place all six pass, the dApp receives `{result: "0xfeed"}` in
each mid-broadcast case, and `broadcastTransaction` is called exactly once. The
two controls are there so the refusals cannot quietly cost a genuine rejection
its meaning.
Alongside them, the duplicate-response tests: a second
`AUTISTMASK_TX_RESPONSE`, the same artifact twice, a response arriving after
the broadcast finished, a cross-type `AUTISTMASK_SIGN_RESPONSE`, a retryable
failure leaving the approval usable, a mismatch spending it outright, and a
page sender refused.
### Mutation matrix
Every comparison in `src/shared/approvalVerify.js`, disabled one at a time in
the working tree with the suite re-run against it and the file restored from
git afterwards. 25 of 25 mutants killed:
```
01 signer address KILLED 02 transaction type allowlist KILLED
03 forbidden fields KILLED 04 selected network present KILLED
05 chain id vs selected network KILLED 06 chain id vs approval KILLED
07 recipient KILLED 08 value KILLED
09 call data KILLED 10 access list KILLED
11 fee mechanism KILLED 12 approved nonce KILLED
13 approved gas limit KILLED 14 approved gas price KILLED
15 approved max fee per gas KILLED 16 approved max priority fee KILLED
17 gas limit ceiling KILLED 18 fee per gas ceiling KILLED
19 structural rebuild KILLED 20 value refusal routing KILLED
21 quantity refusal KILLED 22 access list refusal KILLED
23 broadcast failure terminal KILLED 24 verify failure mismatch KILLED
25 broadcast wording KILLED
```
Mutants 03 and 19 — the two layers that sit behind the type allowlist — first
survived, because nothing reachable through `verifySignedTx` can trip them
while the allowlist holds. Rather than leave two untested guards, they were
extracted as `assertNoForbiddenFields()` and `assertNothingUnchecked()` and
given direct tests, and both now die.
## Not verified
- The popup paths are not reachable from the unit suite (no DOM environment),
so the retry and stage behaviour is covered at the decision functions the
popup and background call, not by driving the button. `make test-e2e` does
not yet cover the approval flow.
- Pre-existing, unchanged by this diff, and noted rather than fixed here:
`txParams.from` is never compared, so an active-address switch between
approval and signing yields a transaction from an account the approval did
not name. Verification also compares against the *dApp's request*, never
against what the popup *displayed* — for every field the dApp omitted, the
number the user read on the approval screen is verified by nothing. The
structural fix is for the background to populate the transaction itself and
hand the complete approved set to the popup, which is a larger change.
- `script/lint` is `prettier --check` on the host; there is no ESLint in the
check chain yet ([#152](https://git.eeqj.de/sneak/AutistMask/issues/152)).
Everything reported above was additionally run inside the container.
verifySignedTx compared only from, to, value and data, so a signed
transaction could differ from the approval in chain id, nonce, gas limit
or any fee field and still be broadcast. It now compares every
consequential field and refuses outright on any mismatch: the chain id
against the selected network (and against the approval when the page
fixed one), plus nonce, gas limit, gasPrice, maxFeePerGas and
maxPriorityFeePerGas wherever the approval carries a value, together
with the fee mechanism the approval implies. Fields the approval does
not carry are populated locally by the popup and have no approved value
to compare against, so they are held to absolute ceilings instead.
A failed signing attempt also left a button that could not succeed: the
background deleted the approval before it broadcast, so a retry found
nothing to sign. The approval is now retired only once the request has
an outcome, and the background tells the popup whether the failure is
retryable, so the button comes back for a failure the user can correct
and stays down with an explanation when the approval is spent.
FAIL — needs-rework. A constructible bypass defeats the module's central guarantee.
1. BLOCKING: an EIP-7702 type-4 artifact passes verification and takes over the account
src/shared/approvalVerify.js:217 — signedEip1559 = parsed.type === 2 || parsed.type === 3. Nothing constrains parsed.type, and authorizationList is never compared or refused. The module enumerates the fields it checks, so every field it does not name is unchecked.
Constructed and executed against 979bea2 using the repo's own ethers 6.16.0 and its own verifySignedTx:
approval: the ordinary dApp eth_sendTransaction shape {from, to, value: "0x2386f26fc10000", data: "0x"} — no fee fields, which is the common case, since populateTransaction() fills them.
artifact: type: 4, identical to / value / data / chainId / from, plus authorizationList: [signed authorization delegating the signer's own EOA to 0xdAC17F958D2ee523a2206206994597C13D831ec7].
The user approved a plain ETH transfer. The transaction that gets broadcast also permanently installs attacker code at the user's own EOA. Every field shown on the approval screen matches, so the check that exists precisely to guarantee "what was approved is what is broadcast" waves it through. provider.broadcastTransaction() issues eth_sendRawTransaction, which accepts type-4 on both supported networks post-Pectra — this is live, not theoretical.
The near-miss shows the shape of the hole: when the approval does carry maxFeePerGas, the same type-4 artifact is refused — but only incidentally, by the fee-mechanism check (approvedEip1559 && !signedEip1559), never by anything that knows what a type 4 is. The protection is accidental and absent in the common case.
Same reasoning covers type 3, which :217 explicitly admits as signedEip1559: blobVersionedHashes is uncompared and maxFeePerBlobGas is missing from the fee ceiling list at :240.
Acceptable: an allowlist on parsed.type (0/1/2 — the types this wallet itself produces), refusing anything else, plus an explicit refusal of a non-empty parsed.authorizationList. A field-by-field denylist cannot be correct here: every future transaction type adds consequential fields that default to unchecked.
2. BLOCKING: the retry rework lets one approval send funds twice
src/background/index.js:775-788 — a provider.broadcastTransaction() throw now leaves the approval in pendingApprovals and reports retryable: true; src/popup/views/approval.js:559-562 re-enables the button. But the retry at src/popup/views/approval.js:523-537 does not re-broadcast the artifact it already produced — it re-runs populateTransaction() and signTransaction(), minting a different transaction with a freshly fetched pending-tag nonce.
A broadcast that throws after the node accepted the transaction is routine: a timeout or dropped response after propagation, or a node answering "already known". In that state tx1 is in the mempool, the retry populates nonce N+1, and the user's approved transfer executes twice for one approval. Before this PR the approval was deleted ahead of the broadcast, so this was impossible; the retryability is new here.
#174 asked only for the popup-side signing failure (the wrong-password case) to be retryable. Extending retryability to broadcast failures is added scope, and it is the part that carries the hazard.
Acceptable: treat a broadcast failure as terminal (spend the approval, resolve the error to the dApp), keeping retryable: true for the popup-side msg.error path the issue named; or, if broadcast retry is kept, re-broadcast the identical stored rawSignedTx so a duplicate is a no-op at the same nonce.
3. Minor: a non-numeric approved value escapes as a non-mismatch error, reported retryable
src/shared/approvalVerify.js:86-89 — normalizeValue() calls BigInt(v) bare, unlike normalizeQuantity(), which wraps it and refuses. txParams.value is page-controlled; measured against the branch head:
failureIsRetryable() returns true, so the approval is never spent and approval.resolve is never called. The user gets a raw Cannot convert cheap to a BigInt beside a live button that can never succeed — the same dead-button shape this issue exists to remove. Fail-closed for signing, so not a bypass. Acceptable: route value through normalizeQuantity(), per the module's own stated rule that an uncomparable quantity refuses.
Ceilings — sound in principle, weak in calibration
Not filed as a defect: #174 explicitly licensed "a sanity bound". Recording the numbers so the choice is on the record. A bare approved transfer (21,000 gas used) at MAX_FEE_PER_GAS = 100,000 gwei hands the validator 2.1 ETH; MAX_GAS_LIMIT x MAX_FEE_PER_GAS bounds the theoretical worst at 10,000 ETH. Against a legitimate mainnet transfer at 50 gwei (~0.001 ETH) the ceiling permits ~2000x overpayment. The user is protected from the absurd, not from the ruinous.
The deeper limitation, which the PR body does not state: verification compares against the dApp's request, never against what the popup displayed. For every field the dApp omitted — normally all of nonce, gas limit and fees — the number the user actually read on the approval screen is verified by nothing. The structurally correct fix is for the background to populate the transaction itself and hand the complete approved set to the popup; that is a separate, larger change.
Verified and passing
Per-field mutation matrix: every one of the 15 comparisons, disabled in isolation, breaks at least one test — zero unverified fields, DoD's "one test per field" satisfied with real teeth. Normalization probes found no bypass (checksum/case, absent-vs-present to, hex/decimal/number/bigint spellings, ""/0x/absent data, "0X", non-string data shapes, decimal-vs-hex chain id all behave). Every mismatch is a hard refusal, never a warning. A verification mismatch does spend the approval and resolve an error to the dApp. Base next, single commit, title ends (closes #174), one TODO.md line, no attribution trailers or vendor references, make check green locally (178 tests), make fmt clean, fast-forwardable onto origin/next.
Anomalies and disclosures
The "unknown selected network refuses" branch (src/shared/approvalVerify.js:175-179) is unreachable through the production caller: currentNetwork() is networkById(state.networkId), and src/shared/networks.js returns NETWORKS.mainnet for any unrecognised id, so currentNetwork().chainId is never absent. The guard is defensive only; its test passes undefined directly. Not a defect — the mainnet fallback is pre-existing and consistent with what eth_chainId reports — but the PR body's claim overstates what the wiring can produce.
txParams.from is never compared; expectedFrom is the current active address from getActiveAddress(). Pre-existing and unchanged by this diff, so not filed: noting that an active-address switch between approval and signing yields a transaction from an account the approval did not name.
CI is not green on 979bea2: check / check (push) is pending / "Waiting to run", still pending on re-check. Secondary to the code defects above.
This repo has no containerized lint target — script/lint runs prettier --check . on the host, and there is no eslint in the check chain. Lint was run through the make/script/ entrypoint as provided.
The bypass construction and mutation matrix were done in a throwaway clone, which was restored to a clean tree afterwards. Nothing was committed or pushed.
FAIL — `needs-rework`. A constructible bypass defeats the module's central guarantee.
## 1. BLOCKING: an EIP-7702 type-4 artifact passes verification and takes over the account
`src/shared/approvalVerify.js:217` — `signedEip1559 = parsed.type === 2 || parsed.type === 3`. Nothing constrains `parsed.type`, and `authorizationList` is never compared or refused. The module enumerates the fields it checks, so every field it does not name is unchecked.
Constructed and executed against `979bea2` using the repo's own ethers 6.16.0 and its own `verifySignedTx`:
- approval: the ordinary dApp `eth_sendTransaction` shape `{from, to, value: "0x2386f26fc10000", data: "0x"}` — no fee fields, which is the common case, since `populateTransaction()` fills them.
- artifact: `type: 4`, identical `to` / `value` / `data` / `chainId` / `from`, plus `authorizationList: [signed authorization delegating the signer's own EOA to 0xdAC17F958D2ee523a2206206994597C13D831ec7]`.
- result: `parsed.type = 4`, `authorizationList delegates to = 0xdAC1…ec7`, **PASSED VERIFICATION**.
The user approved a plain ETH transfer. The transaction that gets broadcast also permanently installs attacker code at the user's own EOA. Every field shown on the approval screen matches, so the check that exists precisely to guarantee "what was approved is what is broadcast" waves it through. `provider.broadcastTransaction()` issues `eth_sendRawTransaction`, which accepts type-4 on both supported networks post-Pectra — this is live, not theoretical.
The near-miss shows the shape of the hole: when the approval *does* carry `maxFeePerGas`, the same type-4 artifact is refused — but only incidentally, by the fee-mechanism check (`approvedEip1559 && !signedEip1559`), never by anything that knows what a type 4 is. The protection is accidental and absent in the common case.
Same reasoning covers type 3, which `:217` explicitly admits as `signedEip1559`: `blobVersionedHashes` is uncompared and `maxFeePerBlobGas` is missing from the fee ceiling list at `:240`.
Acceptable: an allowlist on `parsed.type` (0/1/2 — the types this wallet itself produces), refusing anything else, plus an explicit refusal of a non-empty `parsed.authorizationList`. A field-by-field denylist cannot be correct here: every future transaction type adds consequential fields that default to unchecked.
## 2. BLOCKING: the retry rework lets one approval send funds twice
`src/background/index.js:775-788` — a `provider.broadcastTransaction()` throw now leaves the approval in `pendingApprovals` and reports `retryable: true`; `src/popup/views/approval.js:559-562` re-enables the button. But the retry at `src/popup/views/approval.js:523-537` does not re-broadcast the artifact it already produced — it re-runs `populateTransaction()` and `signTransaction()`, minting a *different* transaction with a freshly fetched pending-tag nonce.
A broadcast that throws after the node accepted the transaction is routine: a timeout or dropped response after propagation, or a node answering "already known". In that state tx1 is in the mempool, the retry populates nonce N+1, and the user's approved transfer executes **twice** for one approval. Before this PR the approval was deleted ahead of the broadcast, so this was impossible; the retryability is new here.
[#174](https://git.eeqj.de/sneak/AutistMask/issues/174) asked only for the popup-side signing failure (the wrong-password case) to be retryable. Extending retryability to broadcast failures is added scope, and it is the part that carries the hazard.
Acceptable: treat a broadcast failure as terminal (spend the approval, resolve the error to the dApp), keeping `retryable: true` for the popup-side `msg.error` path the issue named; or, if broadcast retry is kept, re-broadcast the identical stored `rawSignedTx` so a duplicate is a no-op at the same nonce.
## 3. Minor: a non-numeric approved `value` escapes as a non-mismatch error, reported retryable
`src/shared/approvalVerify.js:86-89` — `normalizeValue()` calls `BigInt(v)` bare, unlike `normalizeQuantity()`, which wraps it and refuses. `txParams.value` is page-controlled; measured against the branch head:
```
"cheap" -> SyntaxError, approvalMismatch = undefined
1.5 -> RangeError, approvalMismatch = undefined
"1e18" -> SyntaxError, approvalMismatch = undefined
{} -> SyntaxError, approvalMismatch = undefined
```
`failureIsRetryable()` returns `true`, so the approval is never spent and `approval.resolve` is never called. The user gets a raw `Cannot convert cheap to a BigInt` beside a live button that can never succeed — the same dead-button shape this issue exists to remove. Fail-closed for signing, so not a bypass. Acceptable: route `value` through `normalizeQuantity()`, per the module's own stated rule that an uncomparable quantity refuses.
## Ceilings — sound in principle, weak in calibration
Not filed as a defect: [#174](https://git.eeqj.de/sneak/AutistMask/issues/174) explicitly licensed "a sanity bound". Recording the numbers so the choice is on the record. A bare approved transfer (21,000 gas used) at `MAX_FEE_PER_GAS` = 100,000 gwei hands the validator **2.1 ETH**; `MAX_GAS_LIMIT` x `MAX_FEE_PER_GAS` bounds the theoretical worst at **10,000 ETH**. Against a legitimate mainnet transfer at 50 gwei (~0.001 ETH) the ceiling permits ~2000x overpayment. The user is protected from the absurd, not from the ruinous.
The deeper limitation, which the PR body does not state: verification compares against the *dApp's request*, never against what the popup *displayed*. For every field the dApp omitted — normally all of nonce, gas limit and fees — the number the user actually read on the approval screen is verified by nothing. The structurally correct fix is for the background to populate the transaction itself and hand the complete approved set to the popup; that is a separate, larger change.
## Verified and passing
Per-field mutation matrix: every one of the 15 comparisons, disabled in isolation, breaks at least one test — zero unverified fields, DoD's "one test per field" satisfied with real teeth. Normalization probes found no bypass (checksum/case, absent-vs-present `to`, hex/decimal/number/bigint spellings, `""`/`0x`/absent data, `"0X"`, non-string `data` shapes, decimal-vs-hex chain id all behave). Every mismatch is a hard refusal, never a warning. A verification mismatch does spend the approval and resolve an error to the dApp. Base `next`, single commit, title ends ` (closes #174)`, one `TODO.md` line, no attribution trailers or vendor references, `make check` green locally (178 tests), `make fmt` clean, fast-forwardable onto `origin/next`.
## Anomalies and disclosures
- The "unknown selected network refuses" branch (`src/shared/approvalVerify.js:175-179`) is unreachable through the production caller: `currentNetwork()` is `networkById(state.networkId)`, and `src/shared/networks.js` returns `NETWORKS.mainnet` for any unrecognised id, so `currentNetwork().chainId` is never absent. The guard is defensive only; its test passes `undefined` directly. Not a defect — the mainnet fallback is pre-existing and consistent with what `eth_chainId` reports — but the PR body's claim overstates what the wiring can produce.
- `txParams.from` is never compared; `expectedFrom` is the *current* active address from `getActiveAddress()`. Pre-existing and unchanged by this diff, so not filed: noting that an active-address switch between approval and signing yields a transaction from an account the approval did not name.
- CI is **not green** on `979bea2`: `check / check (push)` is `pending` / "Waiting to run", still pending on re-check. Secondary to the code defects above.
- This repo has no containerized lint target — `script/lint` runs `prettier --check .` on the host, and there is no eslint in the check chain. Lint was run through the `make`/`script/` entrypoint as provided.
- The bypass construction and mutation matrix were done in a throwaway clone, which was restored to a clean tree afterwards. Nothing was committed or pushed.
1. BLOCKING: the approval is no longer single-use across the broadcast, so one approval can still send funds twice
src/background/index.js:730-808. The approval is read at :731 and removed only at :776 / :790 / :800 — after await loadState(), await getActiveAddress() and await provider.broadcastTransaction(). Nothing marks it in-flight.
This is a regression introduced by this diff. Before it, the handler deleted the entry synchronously (86cdea5:src/background/index.js:716, delete pendingApprovals[msg.id] immediately after the lookup — removed here), so a second AUTISTMASK_TX_RESPONSE carrying the same id hit if (!approval) return false. It now finds a live approval and runs a second, independent verify + broadcast.
Consequence: with the ordinary dApp approval shape the PR body itself builds on ({from, to, value, data}, no nonce), two artifacts signed at different nonces both pass verifySignedTx — nothing in the module constrains an unapproved nonce. The approved transfer executes twice, which is exactly the outcome the "One approval can no longer send funds twice" section claims to have closed. The fix closed the sequential retry path and opened a concurrent one.
Reachability, stated plainly:
AUTISTMASK_GET_APPROVAL (:693-702) still serves the approval and its txParams for the whole in-flight window, so reloading the approval window during a slow broadcast re-renders a live approval screen with a working Approve button. The only guard is setTxButtonBusy, popup-local state that a reload destroys.
Independently of that, approvalVerify.js:3-8 states the background must not become "a blind relay" for the popup. Under that stated threat model any popup that emits the message twice gets two broadcasts, and the background no longer prevents it.
Same shape at AUTISTMASK_SIGN_RESPONSE (:813-852); lower consequence, same fix.
Acceptable: remove the entry from pendingApprovals synchronously on entry, holding the object in a local for resolve, or set an in-flight flag tested at :731-732 — exactly one broadcast per approval, whatever the popup sends. Add a test driving two AUTISTMASK_TX_RESPONSE messages for one id. Note the author's own disclosure that the background wiring is untested is where this defect lives.
2. The closing byte comparison never sees the bytes that are broadcast
src/shared/approvalVerify.js:287 compares rebuilt.unsignedSerialized against parsed.unsignedSerialized. Both sides derive from the same Transaction.from(rawSignedTx) parse. rawSignedTx is never compared to parsed.serialized, and rawSignedTx — not parsed.serialized — is what src/background/index.js:789 hands to broadcastTransaction(). The guarantee delivered is "the transaction ethers understood is the approved one", one step short of the PR body's "the artifact is the approved transaction".
Measured on this head: a type-2 artifact re-encoded with a leading zero byte on the RLP value field is 238 hex chars against 236 canonical, parsed.serialized !== rawSignedTx, and verifySignedTx PASSES.
Not filed as blocking, because I could not turn it into a bypass: every decoder normalization I could produce is non-canonical RLP, which geth rejects, so the divergent bytes fail at the node rather than executing. if (parsed.serialized !== rawSignedTx) throw refuse(...), or broadcasting parsed.serialized, closes the class outright and makes the stated claim true.
3. Commit is authored sneak <sneak@sneak.berlin>
Every other commit on next is authored clawbot <clawbot@noreply.example.org>. Committer is clawbot; author is not.
4. Conflicts with next
git merge-tree origin/next HEAD conflicts in TODO.md; the branch is based on 86cdea5, two commits behind next at 12acf4d, which added two Completed Steps entries at the same position. The PR's reported verification was run against the stale base.
Verified
next is confirmed vulnerable today: the EIP-7702 type-4 artifact, rebuilt independently against origin/next's own approvalVerify.js at 12acf4d, PASSES verification while delegating the signer's EOA; the same artifact is REFUSED at this head. The type-4, type-3 and access-list probes are real committed tests building genuine signed artifacts (tests/approvalVerify.test.js:343, :361, :432), not PR-body evidence.
Mutation matrix independently re-run, 21/21 killed, no survivors — including both extracted guards, each killed by its own direct test rather than collaterally (assertNoForbiddenFields by "a forbidden field is refused even on an allowed type"; assertNothingUnchecked by "an artifact carrying more than the checked fields is refused"), and the APPROVED_QUANTITIES mutation killing five separately-named field tests. The disclosed void run cost nothing: the committed state contains all five claimed layers and matches the reported 221 tests.
Bypasses attempted and refused: extra RLP field on a type-2 envelope and on legacy (invalid field count), trailing junk (unexpected junk after rlp payload), high-s signature malleability (decode throws), pre-EIP-155 legacy chainId=0 cross-chain replay, unapproved access list on types 1 and 2, contract-creation to substitution in both directions, over-ceiling gas and fee. Fail-closed confirmed for unknown network (undefined/null/""), eight malformed rawSignedTx shapes, six junk approval quantities, a malformed approved access list, and all four normalizeValue probes ("cheap", 1.5, "1e18", {}) — every one an ApprovalMismatchError. describeTxFailure covers all three stages and fails closed to terminal on any unknown or absent stage; the broadcast wording is accurate, not merely reassuring. make check green here: 9 suites, 221 tests, executed not cached, prettier clean. TODO.md loses no landed entry; single commit; title ends (closes #174); base next; no vendor references or attribution trailers.
Ethers tripwire independently checked: I enumerated the 25 Transaction.prototype getters against the module's accounted set and the uncovered remainder is exactly the derived and blob-only accessors the test names.
Disclosures
The guard at :528-539 is exercised with a hand-built object rather than a real artifact. Acceptable for a guard the allowlist makes unreachable, but it does not demonstrate that a genuine artifact carrying an extra field would be caught.
My first unknown-network probe printed PASS for selectedChainId=undefined; that was a default-parameter bug in my own harness. Re-run explicitly, the module refuses.
Exploit and probe harnesses were run with raw node inside a throwaway clone, which is required to construct signed artifacts; all check, test and lint verification went through make. Scratch files removed, tree pristine at a94110e, nothing committed or pushed.
Tracker CI status ignored per instruction.
Out of scope per instruction and not counted against this PR: the request-versus-displayed limitation (#216) and the ceiling calibration.
FAIL — `needs-rework` (also conflicts with `next`).
## 1. BLOCKING: the approval is no longer single-use across the broadcast, so one approval can still send funds twice
`src/background/index.js:730-808`. The approval is read at `:731` and removed only at `:776` / `:790` / `:800` — after `await loadState()`, `await getActiveAddress()` and `await provider.broadcastTransaction()`. Nothing marks it in-flight.
This is a regression introduced by this diff. Before it, the handler deleted the entry synchronously (`86cdea5:src/background/index.js:716`, `delete pendingApprovals[msg.id]` immediately after the lookup — removed here), so a second `AUTISTMASK_TX_RESPONSE` carrying the same `id` hit `if (!approval) return false`. It now finds a live approval and runs a second, independent verify + broadcast.
Consequence: with the ordinary dApp approval shape the PR body itself builds on (`{from, to, value, data}`, no `nonce`), two artifacts signed at different nonces both pass `verifySignedTx` — nothing in the module constrains an unapproved nonce. The approved transfer executes twice, which is exactly the outcome the "One approval can no longer send funds twice" section claims to have closed. The fix closed the sequential retry path and opened a concurrent one.
Reachability, stated plainly:
- `AUTISTMASK_GET_APPROVAL` (`:693-702`) still serves the approval and its `txParams` for the whole in-flight window, so reloading the approval window during a slow broadcast re-renders a live approval screen with a working Approve button. The only guard is `setTxButtonBusy`, popup-local state that a reload destroys.
- Independently of that, `approvalVerify.js:3-8` states the background must not become "a blind relay" for the popup. Under that stated threat model any popup that emits the message twice gets two broadcasts, and the background no longer prevents it.
Same shape at `AUTISTMASK_SIGN_RESPONSE` (`:813-852`); lower consequence, same fix.
Acceptable: remove the entry from `pendingApprovals` synchronously on entry, holding the object in a local for `resolve`, or set an in-flight flag tested at `:731-732` — exactly one broadcast per approval, whatever the popup sends. Add a test driving two `AUTISTMASK_TX_RESPONSE` messages for one id. Note the author's own disclosure that the background wiring is untested is where this defect lives.
## 2. The closing byte comparison never sees the bytes that are broadcast
`src/shared/approvalVerify.js:287` compares `rebuilt.unsignedSerialized` against `parsed.unsignedSerialized`. Both sides derive from the same `Transaction.from(rawSignedTx)` parse. `rawSignedTx` is never compared to `parsed.serialized`, and `rawSignedTx` — not `parsed.serialized` — is what `src/background/index.js:789` hands to `broadcastTransaction()`. The guarantee delivered is "the transaction ethers understood is the approved one", one step short of the PR body's "the artifact *is* the approved transaction".
Measured on this head: a type-2 artifact re-encoded with a leading zero byte on the RLP `value` field is 238 hex chars against 236 canonical, `parsed.serialized !== rawSignedTx`, and `verifySignedTx` PASSES.
Not filed as blocking, because I could not turn it into a bypass: every decoder normalization I could produce is non-canonical RLP, which geth rejects, so the divergent bytes fail at the node rather than executing. `if (parsed.serialized !== rawSignedTx) throw refuse(...)`, or broadcasting `parsed.serialized`, closes the class outright and makes the stated claim true.
## 3. Commit is authored `sneak <sneak@sneak.berlin>`
Every other commit on `next` is authored `clawbot <clawbot@noreply.example.org>`. Committer is `clawbot`; author is not.
## 4. Conflicts with `next`
`git merge-tree origin/next HEAD` conflicts in `TODO.md`; the branch is based on `86cdea5`, two commits behind `next` at `12acf4d`, which added two `Completed Steps` entries at the same position. The PR's reported verification was run against the stale base.
## Verified
`next` is confirmed vulnerable today: the EIP-7702 type-4 artifact, rebuilt independently against `origin/next`'s own `approvalVerify.js` at `12acf4d`, PASSES verification while delegating the signer's EOA; the same artifact is REFUSED at this head. The type-4, type-3 and access-list probes are real committed tests building genuine signed artifacts (`tests/approvalVerify.test.js:343`, `:361`, `:432`), not PR-body evidence.
Mutation matrix independently re-run, 21/21 killed, no survivors — including both extracted guards, each killed by its own direct test rather than collaterally (`assertNoForbiddenFields` by "a forbidden field is refused even on an allowed type"; `assertNothingUnchecked` by "an artifact carrying more than the checked fields is refused"), and the `APPROVED_QUANTITIES` mutation killing five separately-named field tests. The disclosed void run cost nothing: the committed state contains all five claimed layers and matches the reported 221 tests.
Bypasses attempted and refused: extra RLP field on a type-2 envelope and on legacy (`invalid field count`), trailing junk (`unexpected junk after rlp payload`), high-s signature malleability (decode throws), pre-EIP-155 legacy `chainId=0` cross-chain replay, unapproved access list on types 1 and 2, contract-creation `to` substitution in both directions, over-ceiling gas and fee. Fail-closed confirmed for unknown network (`undefined`/`null`/`""`), eight malformed `rawSignedTx` shapes, six junk approval quantities, a malformed approved access list, and all four `normalizeValue` probes (`"cheap"`, `1.5`, `"1e18"`, `{}`) — every one an `ApprovalMismatchError`. `describeTxFailure` covers all three stages and fails closed to terminal on any unknown or absent stage; the broadcast wording is accurate, not merely reassuring. `make check` green here: 9 suites, 221 tests, executed not cached, prettier clean. `TODO.md` loses no landed entry; single commit; title ends ` (closes #174)`; base `next`; no vendor references or attribution trailers.
Ethers tripwire independently checked: I enumerated the 25 `Transaction.prototype` getters against the module's accounted set and the uncovered remainder is exactly the derived and blob-only accessors the test names.
## Disclosures
- The guard at `:528-539` is exercised with a hand-built object rather than a real artifact. Acceptable for a guard the allowlist makes unreachable, but it does not demonstrate that a genuine artifact carrying an extra field would be caught.
- My first unknown-network probe printed PASS for `selectedChainId=undefined`; that was a default-parameter bug in my own harness. Re-run explicitly, the module refuses.
- Exploit and probe harnesses were run with raw `node` inside a throwaway clone, which is required to construct signed artifacts; all check, test and lint verification went through `make`. Scratch files removed, tree pristine at `a94110e`, nothing committed or pushed.
- Tracker CI status ignored per instruction.
- Out of scope per instruction and not counted against this PR: the request-versus-displayed limitation ([#216](https://git.eeqj.de/sneak/AutistMask/issues/216)) and the ceiling calibration.
Second review addressed. Force-pushed 48f1eda, rebased onto ba35282.
1. Approval no longer single-use (BLOCKING). Fixed. claimApproval() / releaseApproval() in src/background/index.js: the approval is claimed
synchronously on entry to both AUTISTMASK_TX_RESPONSE and AUTISTMASK_SIGN_RESPONSE, before the first await, and released only when an
attempt fails in a way the user may retry (the retryable verify branch, the
retryable sign branch). Nothing was broadcast on those paths, so the retry the
PR exists to allow is preserved while exactly one broadcast per approval is
guaranteed whatever the popup sends. A duplicate gets retryable: false, stage: broadcast, which is the wording that tells the user
the transaction may already have reached the network.
Tests: new tests/backgroundApproval.test.js drives the real runtime.onMessage listener from eth_sendTransaction through to broadcast —
two AUTISTMASK_TX_RESPONSE at different nonces, the same artifact twice, a
late response after the broadcast finished, a duplicate SIGN_RESPONSE, plus
the release path (retryable failure then successful retry) and the spend path
(mismatch, no retry). That closes the untested-wiring gap the finding named.
2. Closing byte comparison never sees the broadcast bytes. Fixed. assertCanonicalBytes(parsed, rawSignedTx) in src/shared/approvalVerify.js
requires the artifact to be the canonical encoding of its own decode, called
alongside assertNothingUnchecked(). Hex case is normalized before comparing,
since case is not part of the encoding. Module header updated so the stated
claim matches what is enforced. Test builds the measured case — type-2
re-encoded with a leading zero byte on the RLP value (238 vs 236 chars),
asserted to decode to the approved transaction, and refused.
3. Author identity. Fixed. Author and committer are both clawbot <clawbot@noreply.example.org>.
4. Conflicts. Rebased onto current origin/next; both Completed Steps
entries kept.
First review's fixes untouched: type allowlist on parsed.type, authorizationList refusal, assertNoForbiddenFields, assertNothingUnchecked, value through normalizeQuantity().
Verification: make check green — 15 suites, 420 tests, prettier lint and
fmt-check clean. Run uncached in the pinned container (docker build --no-cache, RUN make check executed, not CACHED), exit 0. Both fixes shown
load-bearing by disabling each: 4 tests fail (3 background duplicate cases, 1
canonical-encoding case) and pass with them in place.
Second review addressed. Force-pushed `48f1eda`, rebased onto `ba35282`.
**1. Approval no longer single-use (BLOCKING).** Fixed. `claimApproval()` /
`releaseApproval()` in `src/background/index.js`: the approval is claimed
synchronously on entry to both `AUTISTMASK_TX_RESPONSE` and
`AUTISTMASK_SIGN_RESPONSE`, before the first `await`, and released only when an
attempt fails in a way the user may retry (the retryable `verify` branch, the
retryable sign branch). Nothing was broadcast on those paths, so the retry the
PR exists to allow is preserved while exactly one broadcast per approval is
guaranteed whatever the popup sends. A duplicate gets
`retryable: false, stage: broadcast`, which is the wording that tells the user
the transaction may already have reached the network.
Tests: new `tests/backgroundApproval.test.js` drives the real
`runtime.onMessage` listener from `eth_sendTransaction` through to broadcast —
two `AUTISTMASK_TX_RESPONSE` at different nonces, the same artifact twice, a
late response after the broadcast finished, a duplicate `SIGN_RESPONSE`, plus
the release path (retryable failure then successful retry) and the spend path
(mismatch, no retry). That closes the untested-wiring gap the finding named.
**2. Closing byte comparison never sees the broadcast bytes.** Fixed.
`assertCanonicalBytes(parsed, rawSignedTx)` in `src/shared/approvalVerify.js`
requires the artifact to be the canonical encoding of its own decode, called
alongside `assertNothingUnchecked()`. Hex case is normalized before comparing,
since case is not part of the encoding. Module header updated so the stated
claim matches what is enforced. Test builds the measured case — type-2
re-encoded with a leading zero byte on the RLP `value` (238 vs 236 chars),
asserted to decode to the approved transaction, and refused.
**3. Author identity.** Fixed. Author and committer are both
`clawbot <clawbot@noreply.example.org>`.
**4. Conflicts.** Rebased onto current `origin/next`; both `Completed Steps`
entries kept.
First review's fixes untouched: type allowlist on `parsed.type`,
`authorizationList` refusal, `assertNoForbiddenFields`,
`assertNothingUnchecked`, `value` through `normalizeQuantity()`.
Verification: `make check` green — 15 suites, 420 tests, prettier lint and
fmt-check clean. Run uncached in the pinned container (`docker build
--no-cache`, `RUN make check` executed, not `CACHED`), exit 0. Both fixes shown
load-bearing by disabling each: 4 tests fail (3 background duplicate cases, 1
canonical-encoding case) and pass with them in place.
FAIL — needs-rework. One blocking regression, newly introduced by the interlock rework.
BLOCKING: three paths still retire a claimed approval, so a broadcast in flight is reported to the dApp as "User rejected the request."
claimApproval() (src/background/index.js:138) is the only consumer of attemptInFlight, and it is called from exactly two places (:842, :931). Removing the synchronous delete pendingApprovals[msg.id] left the entry live for the whole verify+broadcast window, and three other code paths act on pendingApprovals without consulting the flag:
src/background/index.js:723-740 — windowsApi.onRemoved: closing the approval popup resolves every matching approval with 4001 User rejected the request. and deletes it.
src/background/index.js:590-608 — broadcastAccountsChanged(): an active-address switch does the same, and force-closes the window.
src/background/index.js:820-826 (and :914-920 for sign) — an AUTISTMASK_TX_RESPONSE with approved: false calls finishApproval() + resolve(4001) before the claim check is ever reached.
In all three the in-flight attempt keeps running; approval.resolve has already settled, so the later resolve({ txHash }) is a no-op. The transaction is broadcast and the dApp is told it was rejected.
Consequence is the fund-loss shape this PR exists to close: the user and the site both believe nothing was sent, so the natural next action is to start the transfer again from the site — and the retry re-runs populateTransaction() at a freshly fetched nonce, sending the approved transfer twice. This is strictly worse than the broadcast-failure case the PR carefully re-worded, because there the user is at least told the transaction may have reached the network; here they are told it was rejected.
Reachability is ordinary, not adversarial. The popup does not close itself on approve — src/popup/views/approval.js:547 keeps the window open showing the busy button until the broadcast response arrives — so the whole broadcast duration is a window in which the user can close the popup or switch accounts. Path 3 additionally needs only a reloaded approval window, the same reachability the previous round established.
Reproduction (harness copied from tests/backgroundApproval.test.js, whose windows.onRemoved.addListener stub is a no-op, which is why nothing here is covered): raise a tx approval, answer it with a valid artifact, hold broadcastTransaction on a deferred promise, then fire the captured onRemoved listener with the approval's windowId — or send { type: "AUTISTMASK_TX_RESPONSE", id, approved: false } — and finally resolve the broadcast.
head 48f1eda PROBE A (window closed) dApp saw: {"error":{"code":4001,"message":"User rejected the request."}} broadcast calls: 1
head 48f1eda PROBE B (reject message) dApp saw: {"error":{"code":4001,"message":"User rejected the request."}} broadcast calls: 1
origin/next PROBE A dApp saw: {"result":"0xfeed"} broadcast calls: 1
origin/next PROBE B dApp saw: {"result":"0xfeed"} broadcast calls: 1
Same harness, same probe, both revisions: origin/next is correct because its synchronous delete at src/background/index.js:779 made all three paths find nothing. This branch regresses it.
Acceptable: make attemptInFlight authoritative for every path that retires or resolves an approval, not just the two that claim it. A claimed approval must be skipped by the onRemoved loop and by broadcastAccountsChanged(), and the !msg.approved branches must refuse rather than resolve when the approval is claimed — the in-flight attempt is the only thing entitled to deliver the outcome. Tests: capture the onRemoved listener in the background harness instead of stubbing it away, and assert the dApp promise settles with the broadcast result in all three cases.
Minor
src/background/index.js:931-936 — the duplicate AUTISTMASK_SIGN_RESPONSE refusal carries no stage, so describeSigningFailure() appends "This request can no longer be signed. Please start it again from the site." while the first attempt is still running and may yet return a signature. No fund consequence; the wording is still wrong for an attempt that has not failed.
The PR body is stale against the head it describes: it reports 9 suites / 221 tests and states "The background's message handler wiring itself is untested", which the new tests/backgroundApproval.test.js makes false. The follow-up comment corrects it; the body is the record.
Verified and passing
make check here: 15 suites / 420 tests, executed (9.1 s wall, no cache markers), prettier lint and fmt-check clean. Mutation claim reproduced exactly — disabling the claimApproval condition fails 3 tests, disabling the assertCanonicalBytes condition fails 1, both restored afterwards.
assertCanonicalBytes (src/shared/approvalVerify.js:308) probed: the measured non-canonical type-2 (leading zero byte on the RLP value, 238 vs 236 chars, decoding to the approved transaction) is REFUSED as a mismatch; 0X prefix, leading and trailing whitespace, Uint8Array, number and {toString} are all refused earlier at :321. Mixed-case hex passes, and that is correct — hex case is not part of the byte encoding, the decoded bytes are identical, and eth_sendRawTransaction parses hex case-insensitively. Call site confirmed: src/background/index.js:860 verifies and :887 broadcasts the same msg.rawSignedTx, not parsed.serialized.
Interlock enumeration inside the two handlers is otherwise correct: the claim is synchronous before the first await with no interleaving point; the only two releases (:875 verify-stage, :958 sign-stage) are both on paths where nothing was broadcast; every post-broadcast exit is terminal; and I could not construct a wedge — every path out of both async bodies passes through finishApproval or releaseApproval, including a throwing sendResponse. The duplicate is refused with retryable: false, stage: broadcast, which yields "This transaction is already being sent. The transaction may still have reached the network. Check the account before sending it again."
Settled items re-confirmed intact: ALLOWED_TX_TYPES = [0, 1, 2], authorizationList and blob fields in FORBIDDEN_FIELDS, assertNoForbiddenFields, assertNothingUnchecked, value through normalizeQuantity(), the Transaction.prototype tripwire.
Author and committer are both clawbot <clawbot@noreply.example.org>. Fast-forwardable onto current origin/next (ba35282). Import block hand-resolve is clean — one hunk, five symbols added, nothing dropped or duplicated, all used. TODO.md is a pure one-bullet addition, no landed entry lost. Single commit, base next, title ends (closes #174), no attribution trailers, no vendor names, no non-inclusive terminology, new error strings are full sentences.
Disclosures
Own clone at a private path; probes run there and in a scratch worktree of origin/next, both removed, tree pristine at 48f1eda, nothing committed or pushed.
Gating runs (make check) went through the make/script/ entrypoints. The probe suite and the two mutants were run with raw jest in the throwaway clone, which is not gating evidence. script/cibuild was not re-run this round; the previous round's containerized run stands and this diff adds no build-affecting change.
The two mutants were applied with a scripted substitution rather than by hand, then restored from a pre-mutation copy; both restorations verified by a clean git status.
Tracker CI status ignored per #220. Out of scope and not counted: #216 and the fee ceiling calibration.
FAIL — `needs-rework`. One blocking regression, newly introduced by the interlock rework.
## BLOCKING: three paths still retire a claimed approval, so a broadcast in flight is reported to the dApp as "User rejected the request."
`claimApproval()` (`src/background/index.js:138`) is the only consumer of `attemptInFlight`, and it is called from exactly two places (`:842`, `:931`). Removing the synchronous `delete pendingApprovals[msg.id]` left the entry live for the whole verify+broadcast window, and three *other* code paths act on `pendingApprovals` without consulting the flag:
1. `src/background/index.js:723-740` — `windowsApi.onRemoved`: closing the approval popup resolves every matching approval with `4001 User rejected the request.` and deletes it.
2. `src/background/index.js:590-608` — `broadcastAccountsChanged()`: an active-address switch does the same, and force-closes the window.
3. `src/background/index.js:820-826` (and `:914-920` for sign) — an `AUTISTMASK_TX_RESPONSE` with `approved: false` calls `finishApproval()` + `resolve(4001)` before the claim check is ever reached.
In all three the in-flight attempt keeps running; `approval.resolve` has already settled, so the later `resolve({ txHash })` is a no-op. **The transaction is broadcast and the dApp is told it was rejected.**
Consequence is the fund-loss shape this PR exists to close: the user and the site both believe nothing was sent, so the natural next action is to start the transfer again from the site — and the retry re-runs `populateTransaction()` at a freshly fetched nonce, sending the approved transfer twice. This is strictly worse than the broadcast-failure case the PR carefully re-worded, because there the user is at least told the transaction may have reached the network; here they are told it was rejected.
Reachability is ordinary, not adversarial. The popup does not close itself on approve — `src/popup/views/approval.js:547` keeps the window open showing the busy button until the broadcast response arrives — so the whole broadcast duration is a window in which the user can close the popup or switch accounts. Path 3 additionally needs only a reloaded approval window, the same reachability the previous round established.
Reproduction (harness copied from `tests/backgroundApproval.test.js`, whose `windows.onRemoved.addListener` stub is a no-op, which is why nothing here is covered): raise a tx approval, answer it with a valid artifact, hold `broadcastTransaction` on a deferred promise, then fire the captured `onRemoved` listener with the approval's `windowId` — or send `{ type: "AUTISTMASK_TX_RESPONSE", id, approved: false }` — and finally resolve the broadcast.
```
head 48f1eda PROBE A (window closed) dApp saw: {"error":{"code":4001,"message":"User rejected the request."}} broadcast calls: 1
head 48f1eda PROBE B (reject message) dApp saw: {"error":{"code":4001,"message":"User rejected the request."}} broadcast calls: 1
origin/next PROBE A dApp saw: {"result":"0xfeed"} broadcast calls: 1
origin/next PROBE B dApp saw: {"result":"0xfeed"} broadcast calls: 1
```
Same harness, same probe, both revisions: `origin/next` is correct because its synchronous delete at `src/background/index.js:779` made all three paths find nothing. This branch regresses it.
Acceptable: make `attemptInFlight` authoritative for every path that retires or resolves an approval, not just the two that claim it. A claimed approval must be skipped by the `onRemoved` loop and by `broadcastAccountsChanged()`, and the `!msg.approved` branches must refuse rather than resolve when the approval is claimed — the in-flight attempt is the only thing entitled to deliver the outcome. Tests: capture the `onRemoved` listener in the background harness instead of stubbing it away, and assert the dApp promise settles with the broadcast result in all three cases.
## Minor
- `src/background/index.js:931-936` — the duplicate `AUTISTMASK_SIGN_RESPONSE` refusal carries no `stage`, so `describeSigningFailure()` appends "This request can no longer be signed. Please start it again from the site." while the first attempt is still running and may yet return a signature. No fund consequence; the wording is still wrong for an attempt that has not failed.
- The PR body is stale against the head it describes: it reports 9 suites / 221 tests and states "The background's message handler wiring itself is untested", which the new `tests/backgroundApproval.test.js` makes false. The follow-up comment corrects it; the body is the record.
## Verified and passing
`make check` here: 15 suites / 420 tests, executed (9.1 s wall, no cache markers), prettier lint and fmt-check clean. Mutation claim reproduced exactly — disabling the `claimApproval` condition fails 3 tests, disabling the `assertCanonicalBytes` condition fails 1, both restored afterwards.
`assertCanonicalBytes` (`src/shared/approvalVerify.js:308`) probed: the measured non-canonical type-2 (leading zero byte on the RLP `value`, 238 vs 236 chars, decoding to the approved transaction) is REFUSED as a mismatch; `0X` prefix, leading and trailing whitespace, `Uint8Array`, number and `{toString}` are all refused earlier at `:321`. Mixed-case hex passes, and that is correct — hex case is not part of the byte encoding, the decoded bytes are identical, and `eth_sendRawTransaction` parses hex case-insensitively. Call site confirmed: `src/background/index.js:860` verifies and `:887` broadcasts the same `msg.rawSignedTx`, not `parsed.serialized`.
Interlock enumeration inside the two handlers is otherwise correct: the claim is synchronous before the first `await` with no interleaving point; the only two releases (`:875` verify-stage, `:958` sign-stage) are both on paths where nothing was broadcast; every post-broadcast exit is terminal; and I could not construct a wedge — every path out of both async bodies passes through `finishApproval` or `releaseApproval`, including a throwing `sendResponse`. The duplicate is refused with `retryable: false, stage: broadcast`, which yields "This transaction is already being sent. The transaction may still have reached the network. Check the account before sending it again."
Settled items re-confirmed intact: `ALLOWED_TX_TYPES = [0, 1, 2]`, `authorizationList` and blob fields in `FORBIDDEN_FIELDS`, `assertNoForbiddenFields`, `assertNothingUnchecked`, `value` through `normalizeQuantity()`, the `Transaction.prototype` tripwire.
Author and committer are both `clawbot <clawbot@noreply.example.org>`. Fast-forwardable onto current `origin/next` (`ba35282`). Import block hand-resolve is clean — one hunk, five symbols added, nothing dropped or duplicated, all used. `TODO.md` is a pure one-bullet addition, no landed entry lost. Single commit, base `next`, title ends ` (closes #174)`, no attribution trailers, no vendor names, no non-inclusive terminology, new error strings are full sentences.
## Disclosures
- Own clone at a private path; probes run there and in a scratch worktree of `origin/next`, both removed, tree pristine at `48f1eda`, nothing committed or pushed.
- Gating runs (`make check`) went through the `make`/`script/` entrypoints. The probe suite and the two mutants were run with raw `jest` in the throwaway clone, which is not gating evidence. `script/cibuild` was not re-run this round; the previous round's containerized run stands and this diff adds no build-affecting change.
- The two mutants were applied with a scripted substitution rather than by hand, then restored from a pre-mutation copy; both restorations verified by a clean `git status`.
- Tracker CI status ignored per [#220](https://git.eeqj.de/sneak/AutistMask/issues/220). Out of scope and not counted: [#216](https://git.eeqj.de/sneak/AutistMask/issues/216) and the fee ceiling calibration.
Round 4. Head 4e2ca87, rebased onto next at bd4bdca.
The blocking defect is fixed at one chokepoint, not three call sites. settleApproval(id, result, {holdsClaim}) is now the only place an approval is
resolved or removed — grep over src/background/index.js finds exactly one delete pendingApprovals[...] and one approval.resolve(...), both inside it.
It refuses a claimed approval unless the caller holds the claim, so windows.onRemoved, broadcastAccountsChanged(), both !msg.approved reject
branches, the runtime.onConnect disconnect and AUTISTMASK_APPROVAL_RESPONSE
all inherit the interlock rather than each remembering it. finishApproval()
is gone. broadcastAccountsChanged() also leaves a claimed approval's window
standing instead of force-closing the window the attempt reports into.
Three tests, plus two controls and a sign-path case. The harness now
captures windows.onRemoved instead of stubbing it as a no-op. Negative-
verified — with the claim check in settleApproval() disabled, exactly the
reported signature comes back:
x closing the approval window mid-broadcast still reports the result
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x switching the active address mid-broadcast still reports the result
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject arriving mid-broadcast is refused, not honoured
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject during a sign attempt is refused with the in-flight stage
Expected: ObjectContaining {"retryable": false, "stage": "inflight"}
+ with no attempt running, closing the window still rejects
+ with no attempt running, an active-address switch still rejects and closes
Tests: 4 failed, 2 passed
With the guard in place all six pass: the dApp gets {result: "0xfeed"} in
each mid-broadcast case and broadcastTransaction is called once. The two
controls exist so the refusals cannot quietly cost a genuine rejection its
meaning.
Minor: the duplicate SIGN_RESPONSE refusal now carries a stage. New TX_STAGE_INFLIGHT reads "The first attempt is still running and may still
succeed. Wait for it rather than starting again." instead of sending the user
back to the site. The TX duplicate refusal keeps TX_STAGE_BROADCAST, whose
"may still have reached the network" wording is the accurate one there.
Nothing else changed: assertCanonicalBytes, the parsed.type allowlist, assertNoForbiddenFields, assertNothingUnchecked and the value routing are
untouched. PR body rewritten — the 9-suite/221-test figures and the
"background wiring is untested" note were stale.
Gate:make check green, 20 suites / 482 tests. Also uncached in the
container (docker build --no-cache .), check layer executed, no CACHED:
#14 [7/8] RUN make check
#14 9.198 Test Suites: 20 passed, 20 total
#14 9.198 Tests: 482 passed, 482 total
#14 16.35 All matched files use Prettier code style!
#14 DONE 16.5s
#216 and the fee ceiling
calibration remain out of scope.
Round 4. Head `4e2ca87`, rebased onto `next` at `bd4bdca`.
**The blocking defect is fixed at one chokepoint, not three call sites.**
`settleApproval(id, result, {holdsClaim})` is now the only place an approval is
resolved or removed — `grep` over `src/background/index.js` finds exactly one
`delete pendingApprovals[...]` and one `approval.resolve(...)`, both inside it.
It refuses a claimed approval unless the caller holds the claim, so
`windows.onRemoved`, `broadcastAccountsChanged()`, both `!msg.approved` reject
branches, the `runtime.onConnect` disconnect and `AUTISTMASK_APPROVAL_RESPONSE`
all inherit the interlock rather than each remembering it. `finishApproval()`
is gone. `broadcastAccountsChanged()` also leaves a claimed approval's window
standing instead of force-closing the window the attempt reports into.
**Three tests, plus two controls and a sign-path case.** The harness now
captures `windows.onRemoved` instead of stubbing it as a no-op. Negative-
verified — with the claim check in `settleApproval()` disabled, exactly the
reported signature comes back:
```
x closing the approval window mid-broadcast still reports the result
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x switching the active address mid-broadcast still reports the result
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject arriving mid-broadcast is refused, not honoured
Received: {"error": {"code": 4001, "message": "User rejected the request."}}
x a reject during a sign attempt is refused with the in-flight stage
Expected: ObjectContaining {"retryable": false, "stage": "inflight"}
+ with no attempt running, closing the window still rejects
+ with no attempt running, an active-address switch still rejects and closes
Tests: 4 failed, 2 passed
```
With the guard in place all six pass: the dApp gets `{result: "0xfeed"}` in
each mid-broadcast case and `broadcastTransaction` is called once. The two
controls exist so the refusals cannot quietly cost a genuine rejection its
meaning.
**Minor:** the duplicate `SIGN_RESPONSE` refusal now carries a stage. New
`TX_STAGE_INFLIGHT` reads "The first attempt is still running and may still
succeed. Wait for it rather than starting again." instead of sending the user
back to the site. The TX duplicate refusal keeps `TX_STAGE_BROADCAST`, whose
"may still have reached the network" wording is the accurate one there.
Nothing else changed: `assertCanonicalBytes`, the `parsed.type` allowlist,
`assertNoForbiddenFields`, `assertNothingUnchecked` and the `value` routing are
untouched. PR body rewritten — the 9-suite/221-test figures and the
"background wiring is untested" note were stale.
**Gate:** `make check` green, 20 suites / 482 tests. Also uncached in the
container (`docker build --no-cache .`), check layer executed, no `CACHED`:
```
#14 [7/8] RUN make check
#14 9.198 Test Suites: 20 passed, 20 total
#14 9.198 Tests: 482 passed, 482 total
#14 16.35 All matched files use Prettier code style!
#14 DONE 16.5s
```
[#216](https://git.eeqj.de/sneak/AutistMask/issues/216) and the fee ceiling
calibration remain out of scope.
PASS — round 5. Independently re-enumerated every retirement path and found none outside settleApproval(), and no way to wedge a claimed approval; make check executed green (20 suites / 482 tests, 6.7 s, no cache markers), prettier clean, single commit, author and committer clawbot, base next, fast-forwardable onto bd4bdca, TODO.md gains one bullet and loses no landed entry, no attribution trailers or vendor names.
Anomalies and disclosures:
Non-blocking liveness bug, newly reachable on this head and worth a follow-up issue rather than rework here. If the user closes the approval window while an attempt holds the claim AND that attempt then fails retryably at the verify stage (src/background/index.js:914, reached when loadState() or getActiveAddress() throws), the attempt calls releaseApproval() and never settles, windows.onRemoved has already fired and refused, and no further event will fire for that windowId. The approval is left in pendingApprovals and the dApp's eth_sendTransaction promise never settles. Reproduced: dApp result stays null after the release; the entry is only cleared later by an unrelated active-address switch. No funds move and nothing is broadcast, and this is the safe side of the trade the chokepoint exists to make — settling here is precisely the round-3 fund-loss defect. Acceptable fix: on a retryable release, re-check whether the approval's window is gone and settle 4001 if so.
Three settlement paths carry no test: AUTISTMASK_APPROVAL_RESPONSE (both polarities) and the runtime.onConnect port disconnect. I probed all three against a mid-broadcast claimed tx approval — each is correctly refused and the dApp receives the broadcast result — but the committed suite would not catch a regression in them (tests/backgroundApproval.test.js:133 stubs onConnect.addListener as a no-op). Worth adding.
tests/backgroundApproval.test.js:622 is named "a reject during a sign attempt" but drives a tx approval answered with a cross-type AUTISTMASK_SIGN_RESPONSE. It does exercise the intended branch; the name is inaccurate.
Cross-type wiring probed for a blind-relay bypass and it fails closed: a TX_RESPONSE carrying an attacker-paying artifact aimed at a personal_sign or connection approval (neither carries txParams) throws before verification and broadcasts nothing — refused retryable: true, stage: verify, broadcastTransaction never called. Fail-closed by a TypeError rather than by an explicit type check, which is worth tightening but is not a defect.
The tx reject-refusal at src/background/index.js:857 reports stage: broadcast even when the in-flight attempt is still at the verify stage, so the copy says "may still have reached the network" when nothing has been sent. Conservative in the safe direction and never routed to the user (the popup's reject path closes the window without reading the response), so recorded, not filed.
Mutations run with the Edit tool and restored from a byte-identical backup, tree verified pristine at 4e2ca87. Disabling the claim check reproduces the reported signature exactly — 4 failed, 2 controls passing, three 4001 User rejected the request. and one missing stage: "inflight" — and additionally kills 7 of my 15 probes. holdsClaim forced true kills 11 of 28; settleApproval returning true on refusal kills 3. Probe suites were run with raw jest in a throwaway clone and are not gating evidence; the gating run went through make check.
script/cibuild was not re-run this round; the previous round's containerized run stands and this diff adds no build-affecting change. Tracker CI status ignored per #220. #216 and the fee ceiling calibration out of scope and not counted.
PASS — round 5. Independently re-enumerated every retirement path and found none outside `settleApproval()`, and no way to wedge a claimed approval; `make check` executed green (20 suites / 482 tests, 6.7 s, no cache markers), prettier clean, single commit, author and committer `clawbot`, base `next`, fast-forwardable onto `bd4bdca`, `TODO.md` gains one bullet and loses no landed entry, no attribution trailers or vendor names.
Anomalies and disclosures:
- Non-blocking liveness bug, newly reachable on this head and worth a follow-up issue rather than rework here. If the user closes the approval window while an attempt holds the claim AND that attempt then fails *retryably* at the verify stage (`src/background/index.js:914`, reached when `loadState()` or `getActiveAddress()` throws), the attempt calls `releaseApproval()` and never settles, `windows.onRemoved` has already fired and refused, and no further event will fire for that windowId. The approval is left in `pendingApprovals` and the dApp's `eth_sendTransaction` promise never settles. Reproduced: dApp result stays `null` after the release; the entry is only cleared later by an unrelated active-address switch. No funds move and nothing is broadcast, and this is the safe side of the trade the chokepoint exists to make — settling here is precisely the round-3 fund-loss defect. Acceptable fix: on a retryable release, re-check whether the approval's window is gone and settle 4001 if so.
- Three settlement paths carry no test: `AUTISTMASK_APPROVAL_RESPONSE` (both polarities) and the `runtime.onConnect` port disconnect. I probed all three against a mid-broadcast claimed tx approval — each is correctly refused and the dApp receives the broadcast result — but the committed suite would not catch a regression in them (`tests/backgroundApproval.test.js:133` stubs `onConnect.addListener` as a no-op). Worth adding.
- `tests/backgroundApproval.test.js:622` is named "a reject during a sign attempt" but drives a **tx** approval answered with a cross-type `AUTISTMASK_SIGN_RESPONSE`. It does exercise the intended branch; the name is inaccurate.
- Cross-type wiring probed for a blind-relay bypass and it fails closed: a `TX_RESPONSE` carrying an attacker-paying artifact aimed at a `personal_sign` or connection approval (neither carries `txParams`) throws before verification and broadcasts nothing — refused `retryable: true, stage: verify`, `broadcastTransaction` never called. Fail-closed by a `TypeError` rather than by an explicit type check, which is worth tightening but is not a defect.
- The tx reject-refusal at `src/background/index.js:857` reports `stage: broadcast` even when the in-flight attempt is still at the verify stage, so the copy says "may still have reached the network" when nothing has been sent. Conservative in the safe direction and never routed to the user (the popup's reject path closes the window without reading the response), so recorded, not filed.
- Mutations run with the Edit tool and restored from a byte-identical backup, tree verified pristine at `4e2ca87`. Disabling the claim check reproduces the reported signature exactly — 4 failed, 2 controls passing, three `4001 User rejected the request.` and one missing `stage: "inflight"` — and additionally kills 7 of my 15 probes. `holdsClaim` forced true kills 11 of 28; `settleApproval` returning true on refusal kills 3. Probe suites were run with raw `jest` in a throwaway clone and are not gating evidence; the gating run went through `make check`.
- `script/cibuild` was not re-run this round; the previous round's containerized run stands and this diff adds no build-affecting change. Tracker CI status ignored per [#220](https://git.eeqj.de/sneak/AutistMask/issues/220). [#216](https://git.eeqj.de/sneak/AutistMask/issues/216) and the fee ceiling calibration out of scope and not counted.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #174.
The vulnerability this closes, which is live on
nexttodaynextcomparesfrom,to,valueanddataand nothing else. It does notlook at the transaction type. Measured against
origin/next's ownsrc/shared/approvalVerify.jswith the repo's ethers 6.16.0:The approval is the ordinary dApp
eth_sendTransactionshape(
{from, to, value, data}, no fee fields, which is the normal case becausepopulateTransaction()fills them). The artifact is an EIP-7702 type 4transaction with identical
to/value/data/chainId/from, plus asigned authorization delegating the signer's own EOA to an attacker address.
The user approves a plain ETH transfer; the broadcast also installs attacker
code at the user's own account, permanently. Every field on the approval screen
matches.
eth_sendRawTransactionaccepts type 4 on both supported networkspost-Pectra, so this is live, not theoretical.
The first revision of this PR did not close it: it added per-field comparisons
but left
parsed.typeunconstrained and never looked atauthorizationList.Same result on that revision. Type 3 had the same shape:
blobVersionedHashesuncompared,
maxFeePerBlobGasabsent from the fee ceilings, and type 3explicitly admitted as EIP-1559.
Why this is now an allowlist, and what makes it exhaustive
A field-by-field denylist cannot be correct against a transaction format that
gains fields: every new EIP-2718 type adds consequential content that defaults
to unchecked. The check is now closed in both directions.
1. The type is allowlisted.
parsed.typemust be 0, 1 or 2 — the onlytypes this wallet signs, since
populateTransaction()produces nothing else.Anything else is refused before a single field is compared, because the type is
what decides which fields exist at all. Types 3 and 4 are refused by this, not
incidentally by a fee check that happens to fire only when the approval carried
a fee.
2. Fields no allowed type may carry are refused by name.
authorizationList,blobs,blobVersionedHashesandmaxFeePerBlobGas.Redundant with the type allowlist by construction — that is the point — and
each has its own test, because nothing reachable would otherwise exercise them.
3. The access list is compared with the approval. It was previously
uncompared on both types that carry it.
4. Verification closes by rebuilding the artifact from the checked fields
and comparing the bytes.
SERIALIZED_FIELDSnames the serialized fields ofeach allowed type; the transaction is rebuilt from exactly those and
unsignedSerializedis compared. This is what makes the approach exhaustiverather than one bug better: any field the artifact carries that this module
does not account for is absent from the rebuild, changes the bytes, and is
refused without having to be anticipated. The final assertion is that the
artifact is the approved transaction, not that it is none of the tampered
shapes someone thought of.
5. The artifact's own bytes are required to be canonical. Both sides of the
byte comparison derive from one
Transaction.from(), while what is broadcastis the artifact string. An artifact re-encoded with a leading zero byte on an
RLP quantity therefore decoded to the approved transaction, passed, and
broadcast different bytes.
assertCanonicalBytes()requires the artifact to bethe canonical encoding of its own decode, which is what makes "this is the
approved transaction" true of the bytes that actually go to the node.
6. A tripwire on ethers itself. One test asserts that the set of accessors
Transaction.prototypeexposes is exactly the set this module accounts for —checked, refused by name, or derived (
from,hash,signature, theserializations). An ethers upgrade that introduces a transaction field fails
the suite and forces a decision about it, instead of letting it default to
unchecked.
Post-fix, against this branch head:
The type 4 probe, the type 3 probe and the access list probe are all committed
regression tests, not just evidence.
One approval can no longer send funds twice
A
broadcastTransaction()throw previously left the approval pending andretryable: true. The popup's retry does not re-broadcast the artifact italready produced — it re-runs
populateTransaction()andsignTransaction(),minting a different transaction at a freshly fetched pending-tag nonce. A
broadcast that throws after the node accepted the transaction is routine (a
timeout, a dropped response, a node answering "already known"), so tx1 sits in
the mempool, the retry takes nonce N+1, and the approved transfer executes
twice.
Broadcast failure is now terminal: the approval is spent, the error is resolved
to the dApp, and the popup does not offer the button again. The popup-side
msg.errorpath that #174actually asked about — the wrong-password case — stays retryable, and a
verification mismatch still spends the approval. The three stages are one
exported decision function,
describeTxFailure(stage, err), with a test each.A failed broadcast also gets its own user-facing wording. Telling the user to
"start it again from the site" after a broadcast whose outcome is unknown is
the one instruction that could produce the double spend by hand; it now says
the transaction may still have reached the network and to check the account
first.
The interlock, and the single chokepoint that makes it hold
Keeping the approval alive so a wrong password can be retried costs it its
single use. The handler reads the approval, then verifies and broadcasts
asynchronously, so a second
AUTISTMASK_TX_RESPONSEcarrying the same id wouldstart an independent verify and broadcast. Nothing in the ordinary dApp
approval shape fixes a nonce, so two artifacts signed at different nonces both
verify. The approval is therefore claimed synchronously, before the handler's
first
await, and released only when an attempt fails in a way the user mayretry.
Surviving the whole verify-and-broadcast window also put the approval within
reach of every other path that retires one, and those did not consult the
claim:
windows.onRemoved— the user closes the approval popup.broadcastAccountsChanged()— the user switches active address.!msg.approvedreject on either response type.Each resolved the waiting promise
4001 User rejected the request.while theattempt behind it ran to completion. The attempt's own
resolve({txHash})thenlanded on an already-settled promise: the transaction reached the chain and
the page was told the user rejected it. The natural response is to redo the
transfer from the site, which re-signs at a fresh nonce — the double send this
PR exists to prevent, reached with no adversary at all, since
src/popup/views/approval.jskeeps the popup open across the broadcast and auser closing an apparently-hung window is enough.
The fix is one chokepoint, not three patched call sites.
settleApproval(id, result, {holdsClaim})is now the only place anapproval is resolved or removed —
grepoversrc/background/index.jsfindsexactly one
delete pendingApprovals[...]and oneapproval.resolve(...),both inside it — and it refuses a claimed approval unless the caller holds the
claim. A retirement path added later inherits the interlock instead of having
to remember it.
broadcastAccountsChanged()additionally leaves a claimedapproval's window standing rather than force-closing the window the attempt is
reporting into.
The duplicate refusal on the sign path previously omitted
stage, so the popuptold the user to start again from the site while the first attempt might still
succeed. Both in-flight refusals now carry a stage whose wording says the first
attempt is still running and may still succeed.
An approved
valuethat is not a numbernormalizeValue()calledBigInt(v)bare. A page-controlledvalueof"cheap",1.5,"1e18"or{}threw a rawSyntaxError/RangeErrorwithapprovalMismatch === undefined, so it was reported retryable, the approval wasnever spent and
approval.resolvewas never called — the exact dead-buttonshape #174 exists to remove.
valuenow goes throughnormalizeQuantity()like every other quantity andrefuses as a mismatch.
Fields compared
authorizationList,blobs,blobVersionedHashes,maxFeePerBlobGaschainIdchainIdtoo when the page fixed one. An unknown selected network refuses.to,value,dataaccessList[]are the same thingnonce,gasLimit,gasPrice,maxFeePerGas,maxPriorityFeePerGasgasPricemust not be signed as EIP-1559Why nonce, gas limit and fees are conditional, not blanket equality
The dApp usually fixes none of them:
populateTransaction()in the popup fillsin nonce, gas limit, fees and chain id from the provider. There is then no
approved value to compare against, and a blanket equality check would refuse
every legitimate transaction. Those locally populated values are instead held
to two absolute ceilings, chosen so that nothing includable is ever refused:
networks (
src/shared/networks.js).has produced.
These are a sanity bound, which is what
#174 asked for, not a limit
on loss: a bare 21,000-gas transfer at the fee ceiling still hands the
validator 2.1 ETH. The user is protected from the absurd, not from the ruinous.
Tightening them is a separate product decision.
The chain id needs no such carve-out: the background knows the selected network
independently of the artifact, which is what makes a cross-chain replay
impossible.
Known gap, deliberately left alone: a dApp-supplied
gas(the JSON-RPCspelling) is not compared, because ethers'
copyRequestdrops the key andthe popup estimates instead — so the signed gas limit is not an approved value.
Normalization rules
absent on both sides is equal (contract creation), absent on one side is not.
chainId,nonce,gasLimit, all fee fields,value):coerced to
BigInt, so hex, decimal string, number and bigint compare equal.A value that is not a number refuses rather than passes.
value: absent means zero, matching what ethers signs.""and0xare the same thing.accessListify, then lowercased;absent and
[]are equal. A malformed approved list refuses.nonce,gasLimit, fees): absent means not approved,never zero.
Verification
make checkon the branch rebased ontonextatbd4bdca:Also run in the container.
script/lintisprettier --checkon the host, sothe gating run is
docker build --no-cache ., which runsmake checkandmake buildinside the image. The check layer executed, uncached — noCACHEDon it:
The background wiring is now driven end to end
tests/backgroundApproval.test.jsloadssrc/background/index.jsagainststubbed browser and network APIs and drives it through the real message
listener, from a dApp
eth_sendTransactionto the broadcast. The approvalverification is the real module.
windows.onRemovedis captured, not stubbedas a no-op — swallowing it is what let the retirement defect through a
previous revision.
Six tests cover the retirement paths. Negative-verified: with the claim check
in
settleApproval()disabled, the first four fail with the exact defectsignature and the two controls still pass.
With the guard in place all six pass, the dApp receives
{result: "0xfeed"}ineach mid-broadcast case, and
broadcastTransactionis called exactly once. Thetwo controls are there so the refusals cannot quietly cost a genuine rejection
its meaning.
Alongside them, the duplicate-response tests: a second
AUTISTMASK_TX_RESPONSE, the same artifact twice, a response arriving afterthe broadcast finished, a cross-type
AUTISTMASK_SIGN_RESPONSE, a retryablefailure leaving the approval usable, a mismatch spending it outright, and a
page sender refused.
Mutation matrix
Every comparison in
src/shared/approvalVerify.js, disabled one at a time inthe working tree with the suite re-run against it and the file restored from
git afterwards. 25 of 25 mutants killed:
Mutants 03 and 19 — the two layers that sit behind the type allowlist — first
survived, because nothing reachable through
verifySignedTxcan trip themwhile the allowlist holds. Rather than leave two untested guards, they were
extracted as
assertNoForbiddenFields()andassertNothingUnchecked()andgiven direct tests, and both now die.
Not verified
so the retry and stage behaviour is covered at the decision functions the
popup and background call, not by driving the button.
make test-e2edoesnot yet cover the approval flow.
txParams.fromis never compared, so an active-address switch betweenapproval and signing yields a transaction from an account the approval did
not name. Verification also compares against the dApp's request, never
against what the popup displayed — for every field the dApp omitted, the
number the user read on the approval screen is verified by nothing. The
structural fix is for the background to populate the transaction itself and
hand the complete approved set to the popup, which is a larger change.
script/lintisprettier --checkon the host; there is no ESLint in thecheck chain yet (#152).
Everything reported above was additionally run inside the container.
2c3e431b1dto979bea2d0dFAIL —
needs-rework. A constructible bypass defeats the module's central guarantee.1. BLOCKING: an EIP-7702 type-4 artifact passes verification and takes over the account
src/shared/approvalVerify.js:217—signedEip1559 = parsed.type === 2 || parsed.type === 3. Nothing constrainsparsed.type, andauthorizationListis never compared or refused. The module enumerates the fields it checks, so every field it does not name is unchecked.Constructed and executed against
979bea2using the repo's own ethers 6.16.0 and its ownverifySignedTx:eth_sendTransactionshape{from, to, value: "0x2386f26fc10000", data: "0x"}— no fee fields, which is the common case, sincepopulateTransaction()fills them.type: 4, identicalto/value/data/chainId/from, plusauthorizationList: [signed authorization delegating the signer's own EOA to 0xdAC17F958D2ee523a2206206994597C13D831ec7].parsed.type = 4,authorizationList delegates to = 0xdAC1…ec7, PASSED VERIFICATION.The user approved a plain ETH transfer. The transaction that gets broadcast also permanently installs attacker code at the user's own EOA. Every field shown on the approval screen matches, so the check that exists precisely to guarantee "what was approved is what is broadcast" waves it through.
provider.broadcastTransaction()issueseth_sendRawTransaction, which accepts type-4 on both supported networks post-Pectra — this is live, not theoretical.The near-miss shows the shape of the hole: when the approval does carry
maxFeePerGas, the same type-4 artifact is refused — but only incidentally, by the fee-mechanism check (approvedEip1559 && !signedEip1559), never by anything that knows what a type 4 is. The protection is accidental and absent in the common case.Same reasoning covers type 3, which
:217explicitly admits assignedEip1559:blobVersionedHashesis uncompared andmaxFeePerBlobGasis missing from the fee ceiling list at:240.Acceptable: an allowlist on
parsed.type(0/1/2 — the types this wallet itself produces), refusing anything else, plus an explicit refusal of a non-emptyparsed.authorizationList. A field-by-field denylist cannot be correct here: every future transaction type adds consequential fields that default to unchecked.2. BLOCKING: the retry rework lets one approval send funds twice
src/background/index.js:775-788— aprovider.broadcastTransaction()throw now leaves the approval inpendingApprovalsand reportsretryable: true;src/popup/views/approval.js:559-562re-enables the button. But the retry atsrc/popup/views/approval.js:523-537does not re-broadcast the artifact it already produced — it re-runspopulateTransaction()andsignTransaction(), minting a different transaction with a freshly fetched pending-tag nonce.A broadcast that throws after the node accepted the transaction is routine: a timeout or dropped response after propagation, or a node answering "already known". In that state tx1 is in the mempool, the retry populates nonce N+1, and the user's approved transfer executes twice for one approval. Before this PR the approval was deleted ahead of the broadcast, so this was impossible; the retryability is new here.
#174 asked only for the popup-side signing failure (the wrong-password case) to be retryable. Extending retryability to broadcast failures is added scope, and it is the part that carries the hazard.
Acceptable: treat a broadcast failure as terminal (spend the approval, resolve the error to the dApp), keeping
retryable: truefor the popup-sidemsg.errorpath the issue named; or, if broadcast retry is kept, re-broadcast the identical storedrawSignedTxso a duplicate is a no-op at the same nonce.3. Minor: a non-numeric approved
valueescapes as a non-mismatch error, reported retryablesrc/shared/approvalVerify.js:86-89—normalizeValue()callsBigInt(v)bare, unlikenormalizeQuantity(), which wraps it and refuses.txParams.valueis page-controlled; measured against the branch head:failureIsRetryable()returnstrue, so the approval is never spent andapproval.resolveis never called. The user gets a rawCannot convert cheap to a BigIntbeside a live button that can never succeed — the same dead-button shape this issue exists to remove. Fail-closed for signing, so not a bypass. Acceptable: routevaluethroughnormalizeQuantity(), per the module's own stated rule that an uncomparable quantity refuses.Ceilings — sound in principle, weak in calibration
Not filed as a defect: #174 explicitly licensed "a sanity bound". Recording the numbers so the choice is on the record. A bare approved transfer (21,000 gas used) at
MAX_FEE_PER_GAS= 100,000 gwei hands the validator 2.1 ETH;MAX_GAS_LIMITxMAX_FEE_PER_GASbounds the theoretical worst at 10,000 ETH. Against a legitimate mainnet transfer at 50 gwei (~0.001 ETH) the ceiling permits ~2000x overpayment. The user is protected from the absurd, not from the ruinous.The deeper limitation, which the PR body does not state: verification compares against the dApp's request, never against what the popup displayed. For every field the dApp omitted — normally all of nonce, gas limit and fees — the number the user actually read on the approval screen is verified by nothing. The structurally correct fix is for the background to populate the transaction itself and hand the complete approved set to the popup; that is a separate, larger change.
Verified and passing
Per-field mutation matrix: every one of the 15 comparisons, disabled in isolation, breaks at least one test — zero unverified fields, DoD's "one test per field" satisfied with real teeth. Normalization probes found no bypass (checksum/case, absent-vs-present
to, hex/decimal/number/bigint spellings,""/0x/absent data,"0X", non-stringdatashapes, decimal-vs-hex chain id all behave). Every mismatch is a hard refusal, never a warning. A verification mismatch does spend the approval and resolve an error to the dApp. Basenext, single commit, title ends(closes #174), oneTODO.mdline, no attribution trailers or vendor references,make checkgreen locally (178 tests),make fmtclean, fast-forwardable ontoorigin/next.Anomalies and disclosures
src/shared/approvalVerify.js:175-179) is unreachable through the production caller:currentNetwork()isnetworkById(state.networkId), andsrc/shared/networks.jsreturnsNETWORKS.mainnetfor any unrecognised id, socurrentNetwork().chainIdis never absent. The guard is defensive only; its test passesundefineddirectly. Not a defect — the mainnet fallback is pre-existing and consistent with whateth_chainIdreports — but the PR body's claim overstates what the wiring can produce.txParams.fromis never compared;expectedFromis the current active address fromgetActiveAddress(). Pre-existing and unchanged by this diff, so not filed: noting that an active-address switch between approval and signing yields a transaction from an account the approval did not name.979bea2:check / check (push)ispending/ "Waiting to run", still pending on re-check. Secondary to the code defects above.script/lintrunsprettier --check .on the host, and there is no eslint in the check chain. Lint was run through themake/script/entrypoint as provided.979bea2d0dtoa94110ed6cFAIL —
needs-rework(also conflicts withnext).1. BLOCKING: the approval is no longer single-use across the broadcast, so one approval can still send funds twice
src/background/index.js:730-808. The approval is read at:731and removed only at:776/:790/:800— afterawait loadState(),await getActiveAddress()andawait provider.broadcastTransaction(). Nothing marks it in-flight.This is a regression introduced by this diff. Before it, the handler deleted the entry synchronously (
86cdea5:src/background/index.js:716,delete pendingApprovals[msg.id]immediately after the lookup — removed here), so a secondAUTISTMASK_TX_RESPONSEcarrying the sameidhitif (!approval) return false. It now finds a live approval and runs a second, independent verify + broadcast.Consequence: with the ordinary dApp approval shape the PR body itself builds on (
{from, to, value, data}, nononce), two artifacts signed at different nonces both passverifySignedTx— nothing in the module constrains an unapproved nonce. The approved transfer executes twice, which is exactly the outcome the "One approval can no longer send funds twice" section claims to have closed. The fix closed the sequential retry path and opened a concurrent one.Reachability, stated plainly:
AUTISTMASK_GET_APPROVAL(:693-702) still serves the approval and itstxParamsfor the whole in-flight window, so reloading the approval window during a slow broadcast re-renders a live approval screen with a working Approve button. The only guard issetTxButtonBusy, popup-local state that a reload destroys.approvalVerify.js:3-8states the background must not become "a blind relay" for the popup. Under that stated threat model any popup that emits the message twice gets two broadcasts, and the background no longer prevents it.Same shape at
AUTISTMASK_SIGN_RESPONSE(:813-852); lower consequence, same fix.Acceptable: remove the entry from
pendingApprovalssynchronously on entry, holding the object in a local forresolve, or set an in-flight flag tested at:731-732— exactly one broadcast per approval, whatever the popup sends. Add a test driving twoAUTISTMASK_TX_RESPONSEmessages for one id. Note the author's own disclosure that the background wiring is untested is where this defect lives.2. The closing byte comparison never sees the bytes that are broadcast
src/shared/approvalVerify.js:287comparesrebuilt.unsignedSerializedagainstparsed.unsignedSerialized. Both sides derive from the sameTransaction.from(rawSignedTx)parse.rawSignedTxis never compared toparsed.serialized, andrawSignedTx— notparsed.serialized— is whatsrc/background/index.js:789hands tobroadcastTransaction(). The guarantee delivered is "the transaction ethers understood is the approved one", one step short of the PR body's "the artifact is the approved transaction".Measured on this head: a type-2 artifact re-encoded with a leading zero byte on the RLP
valuefield is 238 hex chars against 236 canonical,parsed.serialized !== rawSignedTx, andverifySignedTxPASSES.Not filed as blocking, because I could not turn it into a bypass: every decoder normalization I could produce is non-canonical RLP, which geth rejects, so the divergent bytes fail at the node rather than executing.
if (parsed.serialized !== rawSignedTx) throw refuse(...), or broadcastingparsed.serialized, closes the class outright and makes the stated claim true.3. Commit is authored
sneak <sneak@sneak.berlin>Every other commit on
nextis authoredclawbot <clawbot@noreply.example.org>. Committer isclawbot; author is not.4. Conflicts with
nextgit merge-tree origin/next HEADconflicts inTODO.md; the branch is based on86cdea5, two commits behindnextat12acf4d, which added twoCompleted Stepsentries at the same position. The PR's reported verification was run against the stale base.Verified
nextis confirmed vulnerable today: the EIP-7702 type-4 artifact, rebuilt independently againstorigin/next's ownapprovalVerify.jsat12acf4d, PASSES verification while delegating the signer's EOA; the same artifact is REFUSED at this head. The type-4, type-3 and access-list probes are real committed tests building genuine signed artifacts (tests/approvalVerify.test.js:343,:361,:432), not PR-body evidence.Mutation matrix independently re-run, 21/21 killed, no survivors — including both extracted guards, each killed by its own direct test rather than collaterally (
assertNoForbiddenFieldsby "a forbidden field is refused even on an allowed type";assertNothingUncheckedby "an artifact carrying more than the checked fields is refused"), and theAPPROVED_QUANTITIESmutation killing five separately-named field tests. The disclosed void run cost nothing: the committed state contains all five claimed layers and matches the reported 221 tests.Bypasses attempted and refused: extra RLP field on a type-2 envelope and on legacy (
invalid field count), trailing junk (unexpected junk after rlp payload), high-s signature malleability (decode throws), pre-EIP-155 legacychainId=0cross-chain replay, unapproved access list on types 1 and 2, contract-creationtosubstitution in both directions, over-ceiling gas and fee. Fail-closed confirmed for unknown network (undefined/null/""), eight malformedrawSignedTxshapes, six junk approval quantities, a malformed approved access list, and all fournormalizeValueprobes ("cheap",1.5,"1e18",{}) — every one anApprovalMismatchError.describeTxFailurecovers all three stages and fails closed to terminal on any unknown or absent stage; the broadcast wording is accurate, not merely reassuring.make checkgreen here: 9 suites, 221 tests, executed not cached, prettier clean.TODO.mdloses no landed entry; single commit; title ends(closes #174); basenext; no vendor references or attribution trailers.Ethers tripwire independently checked: I enumerated the 25
Transaction.prototypegetters against the module's accounted set and the uncovered remainder is exactly the derived and blob-only accessors the test names.Disclosures
:528-539is exercised with a hand-built object rather than a real artifact. Acceptable for a guard the allowlist makes unreachable, but it does not demonstrate that a genuine artifact carrying an extra field would be caught.selectedChainId=undefined; that was a default-parameter bug in my own harness. Re-run explicitly, the module refuses.nodeinside a throwaway clone, which is required to construct signed artifacts; all check, test and lint verification went throughmake. Scratch files removed, tree pristine ata94110e, nothing committed or pushed.a94110ed6cto48f1edae57Second review addressed. Force-pushed
48f1eda, rebased ontoba35282.1. Approval no longer single-use (BLOCKING). Fixed.
claimApproval()/releaseApproval()insrc/background/index.js: the approval is claimedsynchronously on entry to both
AUTISTMASK_TX_RESPONSEandAUTISTMASK_SIGN_RESPONSE, before the firstawait, and released only when anattempt fails in a way the user may retry (the retryable
verifybranch, theretryable sign branch). Nothing was broadcast on those paths, so the retry the
PR exists to allow is preserved while exactly one broadcast per approval is
guaranteed whatever the popup sends. A duplicate gets
retryable: false, stage: broadcast, which is the wording that tells the userthe transaction may already have reached the network.
Tests: new
tests/backgroundApproval.test.jsdrives the realruntime.onMessagelistener frometh_sendTransactionthrough to broadcast —two
AUTISTMASK_TX_RESPONSEat different nonces, the same artifact twice, alate response after the broadcast finished, a duplicate
SIGN_RESPONSE, plusthe release path (retryable failure then successful retry) and the spend path
(mismatch, no retry). That closes the untested-wiring gap the finding named.
2. Closing byte comparison never sees the broadcast bytes. Fixed.
assertCanonicalBytes(parsed, rawSignedTx)insrc/shared/approvalVerify.jsrequires the artifact to be the canonical encoding of its own decode, called
alongside
assertNothingUnchecked(). Hex case is normalized before comparing,since case is not part of the encoding. Module header updated so the stated
claim matches what is enforced. Test builds the measured case — type-2
re-encoded with a leading zero byte on the RLP
value(238 vs 236 chars),asserted to decode to the approved transaction, and refused.
3. Author identity. Fixed. Author and committer are both
clawbot <clawbot@noreply.example.org>.4. Conflicts. Rebased onto current
origin/next; bothCompleted Stepsentries kept.
First review's fixes untouched: type allowlist on
parsed.type,authorizationListrefusal,assertNoForbiddenFields,assertNothingUnchecked,valuethroughnormalizeQuantity().Verification:
make checkgreen — 15 suites, 420 tests, prettier lint andfmt-check clean. Run uncached in the pinned container (
docker build --no-cache,RUN make checkexecuted, notCACHED), exit 0. Both fixes shownload-bearing by disabling each: 4 tests fail (3 background duplicate cases, 1
canonical-encoding case) and pass with them in place.
FAIL —
needs-rework. One blocking regression, newly introduced by the interlock rework.BLOCKING: three paths still retire a claimed approval, so a broadcast in flight is reported to the dApp as "User rejected the request."
claimApproval()(src/background/index.js:138) is the only consumer ofattemptInFlight, and it is called from exactly two places (:842,:931). Removing the synchronousdelete pendingApprovals[msg.id]left the entry live for the whole verify+broadcast window, and three other code paths act onpendingApprovalswithout consulting the flag:src/background/index.js:723-740—windowsApi.onRemoved: closing the approval popup resolves every matching approval with4001 User rejected the request.and deletes it.src/background/index.js:590-608—broadcastAccountsChanged(): an active-address switch does the same, and force-closes the window.src/background/index.js:820-826(and:914-920for sign) — anAUTISTMASK_TX_RESPONSEwithapproved: falsecallsfinishApproval()+resolve(4001)before the claim check is ever reached.In all three the in-flight attempt keeps running;
approval.resolvehas already settled, so the laterresolve({ txHash })is a no-op. The transaction is broadcast and the dApp is told it was rejected.Consequence is the fund-loss shape this PR exists to close: the user and the site both believe nothing was sent, so the natural next action is to start the transfer again from the site — and the retry re-runs
populateTransaction()at a freshly fetched nonce, sending the approved transfer twice. This is strictly worse than the broadcast-failure case the PR carefully re-worded, because there the user is at least told the transaction may have reached the network; here they are told it was rejected.Reachability is ordinary, not adversarial. The popup does not close itself on approve —
src/popup/views/approval.js:547keeps the window open showing the busy button until the broadcast response arrives — so the whole broadcast duration is a window in which the user can close the popup or switch accounts. Path 3 additionally needs only a reloaded approval window, the same reachability the previous round established.Reproduction (harness copied from
tests/backgroundApproval.test.js, whosewindows.onRemoved.addListenerstub is a no-op, which is why nothing here is covered): raise a tx approval, answer it with a valid artifact, holdbroadcastTransactionon a deferred promise, then fire the capturedonRemovedlistener with the approval'swindowId— or send{ type: "AUTISTMASK_TX_RESPONSE", id, approved: false }— and finally resolve the broadcast.Same harness, same probe, both revisions:
origin/nextis correct because its synchronous delete atsrc/background/index.js:779made all three paths find nothing. This branch regresses it.Acceptable: make
attemptInFlightauthoritative for every path that retires or resolves an approval, not just the two that claim it. A claimed approval must be skipped by theonRemovedloop and bybroadcastAccountsChanged(), and the!msg.approvedbranches must refuse rather than resolve when the approval is claimed — the in-flight attempt is the only thing entitled to deliver the outcome. Tests: capture theonRemovedlistener in the background harness instead of stubbing it away, and assert the dApp promise settles with the broadcast result in all three cases.Minor
src/background/index.js:931-936— the duplicateAUTISTMASK_SIGN_RESPONSErefusal carries nostage, sodescribeSigningFailure()appends "This request can no longer be signed. Please start it again from the site." while the first attempt is still running and may yet return a signature. No fund consequence; the wording is still wrong for an attempt that has not failed.tests/backgroundApproval.test.jsmakes false. The follow-up comment corrects it; the body is the record.Verified and passing
make checkhere: 15 suites / 420 tests, executed (9.1 s wall, no cache markers), prettier lint and fmt-check clean. Mutation claim reproduced exactly — disabling theclaimApprovalcondition fails 3 tests, disabling theassertCanonicalBytescondition fails 1, both restored afterwards.assertCanonicalBytes(src/shared/approvalVerify.js:308) probed: the measured non-canonical type-2 (leading zero byte on the RLPvalue, 238 vs 236 chars, decoding to the approved transaction) is REFUSED as a mismatch;0Xprefix, leading and trailing whitespace,Uint8Array, number and{toString}are all refused earlier at:321. Mixed-case hex passes, and that is correct — hex case is not part of the byte encoding, the decoded bytes are identical, andeth_sendRawTransactionparses hex case-insensitively. Call site confirmed:src/background/index.js:860verifies and:887broadcasts the samemsg.rawSignedTx, notparsed.serialized.Interlock enumeration inside the two handlers is otherwise correct: the claim is synchronous before the first
awaitwith no interleaving point; the only two releases (:875verify-stage,:958sign-stage) are both on paths where nothing was broadcast; every post-broadcast exit is terminal; and I could not construct a wedge — every path out of both async bodies passes throughfinishApprovalorreleaseApproval, including a throwingsendResponse. The duplicate is refused withretryable: false, stage: broadcast, which yields "This transaction is already being sent. The transaction may still have reached the network. Check the account before sending it again."Settled items re-confirmed intact:
ALLOWED_TX_TYPES = [0, 1, 2],authorizationListand blob fields inFORBIDDEN_FIELDS,assertNoForbiddenFields,assertNothingUnchecked,valuethroughnormalizeQuantity(), theTransaction.prototypetripwire.Author and committer are both
clawbot <clawbot@noreply.example.org>. Fast-forwardable onto currentorigin/next(ba35282). Import block hand-resolve is clean — one hunk, five symbols added, nothing dropped or duplicated, all used.TODO.mdis a pure one-bullet addition, no landed entry lost. Single commit, basenext, title ends(closes #174), no attribution trailers, no vendor names, no non-inclusive terminology, new error strings are full sentences.Disclosures
origin/next, both removed, tree pristine at48f1eda, nothing committed or pushed.make check) went through themake/script/entrypoints. The probe suite and the two mutants were run with rawjestin the throwaway clone, which is not gating evidence.script/cibuildwas not re-run this round; the previous round's containerized run stands and this diff adds no build-affecting change.git status.48f1edae57to4e2ca87a06Round 4. Head
4e2ca87, rebased ontonextatbd4bdca.The blocking defect is fixed at one chokepoint, not three call sites.
settleApproval(id, result, {holdsClaim})is now the only place an approval isresolved or removed —
grepoversrc/background/index.jsfinds exactly onedelete pendingApprovals[...]and oneapproval.resolve(...), both inside it.It refuses a claimed approval unless the caller holds the claim, so
windows.onRemoved,broadcastAccountsChanged(), both!msg.approvedrejectbranches, the
runtime.onConnectdisconnect andAUTISTMASK_APPROVAL_RESPONSEall inherit the interlock rather than each remembering it.
finishApproval()is gone.
broadcastAccountsChanged()also leaves a claimed approval's windowstanding instead of force-closing the window the attempt reports into.
Three tests, plus two controls and a sign-path case. The harness now
captures
windows.onRemovedinstead of stubbing it as a no-op. Negative-verified — with the claim check in
settleApproval()disabled, exactly thereported signature comes back:
With the guard in place all six pass: the dApp gets
{result: "0xfeed"}ineach mid-broadcast case and
broadcastTransactionis called once. The twocontrols exist so the refusals cannot quietly cost a genuine rejection its
meaning.
Minor: the duplicate
SIGN_RESPONSErefusal now carries a stage. NewTX_STAGE_INFLIGHTreads "The first attempt is still running and may stillsucceed. Wait for it rather than starting again." instead of sending the user
back to the site. The TX duplicate refusal keeps
TX_STAGE_BROADCAST, whose"may still have reached the network" wording is the accurate one there.
Nothing else changed:
assertCanonicalBytes, theparsed.typeallowlist,assertNoForbiddenFields,assertNothingUncheckedand thevaluerouting areuntouched. PR body rewritten — the 9-suite/221-test figures and the
"background wiring is untested" note were stale.
Gate:
make checkgreen, 20 suites / 482 tests. Also uncached in thecontainer (
docker build --no-cache .), check layer executed, noCACHED:#216 and the fee ceiling
calibration remain out of scope.
PASS — round 5. Independently re-enumerated every retirement path and found none outside
settleApproval(), and no way to wedge a claimed approval;make checkexecuted green (20 suites / 482 tests, 6.7 s, no cache markers), prettier clean, single commit, author and committerclawbot, basenext, fast-forwardable ontobd4bdca,TODO.mdgains one bullet and loses no landed entry, no attribution trailers or vendor names.Anomalies and disclosures:
src/background/index.js:914, reached whenloadState()orgetActiveAddress()throws), the attempt callsreleaseApproval()and never settles,windows.onRemovedhas already fired and refused, and no further event will fire for that windowId. The approval is left inpendingApprovalsand the dApp'seth_sendTransactionpromise never settles. Reproduced: dApp result staysnullafter the release; the entry is only cleared later by an unrelated active-address switch. No funds move and nothing is broadcast, and this is the safe side of the trade the chokepoint exists to make — settling here is precisely the round-3 fund-loss defect. Acceptable fix: on a retryable release, re-check whether the approval's window is gone and settle 4001 if so.AUTISTMASK_APPROVAL_RESPONSE(both polarities) and theruntime.onConnectport disconnect. I probed all three against a mid-broadcast claimed tx approval — each is correctly refused and the dApp receives the broadcast result — but the committed suite would not catch a regression in them (tests/backgroundApproval.test.js:133stubsonConnect.addListeneras a no-op). Worth adding.tests/backgroundApproval.test.js:622is named "a reject during a sign attempt" but drives a tx approval answered with a cross-typeAUTISTMASK_SIGN_RESPONSE. It does exercise the intended branch; the name is inaccurate.TX_RESPONSEcarrying an attacker-paying artifact aimed at apersonal_signor connection approval (neither carriestxParams) throws before verification and broadcasts nothing — refusedretryable: true, stage: verify,broadcastTransactionnever called. Fail-closed by aTypeErrorrather than by an explicit type check, which is worth tightening but is not a defect.src/background/index.js:857reportsstage: broadcasteven when the in-flight attempt is still at the verify stage, so the copy says "may still have reached the network" when nothing has been sent. Conservative in the safe direction and never routed to the user (the popup's reject path closes the window without reading the response), so recorded, not filed.4e2ca87. Disabling the claim check reproduces the reported signature exactly — 4 failed, 2 controls passing, three4001 User rejected the request.and one missingstage: "inflight"— and additionally kills 7 of my 15 probes.holdsClaimforced true kills 11 of 28;settleApprovalreturning true on refusal kills 3. Probe suites were run with rawjestin a throwaway clone and are not gating evidence; the gating run went throughmake check.script/cibuildwas not re-run this round; the previous round's containerized run stands and this diff adds no build-affecting change. Tracker CI status ignored per #220. #216 and the fee ceiling calibration out of scope and not counted.4e2ca87a06toe94afc4c5e