fix: an approval claimed when its window closes is orphaned, leaving the dApp promise unsettled forever #262

New Issue

clawbot · 2026-08-12T11:00:04+02:00

clawbot commented

2026-08-12 11:00:04 +02:00

Found by the round-5 independent review of #205 and reproduced there. Non-blocking for that PR, which passed.

The approval window is closed while an attempt holds the claim, and the attempt then fails retryably at the verify stage (src/background/index.js:914). The release path returns the approval to the pool — but the window it would be retried from no longer exists, and nothing else will ever settle it. The dApp's eth_sendTransaction promise stays pending indefinitely.

This is the deliberate safe side of the round-3 trade, not a regression: settling on that path is precisely the fund-loss defect round 3 found, where an in-flight broadcast was reported to the dApp as 4001 User rejected the request. while the transaction actually sent. Liveness was given up to buy that safety. It should not be given up permanently.

Acceptable fix, from the review: re-check the window on release, and settle 4001 if it is gone. That distinguishes "retry is still possible" from "nobody can ever retry this", which the current release path conflates.

Also in scope: three settlement paths with no test coverage

The suite added by #205 covers the window-close, address-switch and reject paths against a claimed approval. Three more route through settleApproval() and are exercised only by the reviewer's throwaway probes:

AUTISTMASK_APPROVAL_RESPONSE, approve polarity
AUTISTMASK_APPROVAL_RESPONSE, reject polarity
the runtime.onConnect port disconnect

All three were verified correct by hand — each is refused against a mid-broadcast claimed approval and the dApp still receives its result — but nothing in the repo would catch a regression. Given that every one of the five review rounds on that PR found a defect in exactly this machinery, they should be pinned.

One tightening, not a defect

A TX_RESPONSE carrying an attacker-paying artifact aimed at a personal_sign or connection approval (neither has txParams) broadcasts nothing — but it fails closed via a TypeError rather than an explicit type check. Worth making explicit so a future refactor cannot turn an incidental throw into a path.

Definition of done

A retryable failure after the approval window has closed settles the dApp promise rather than orphaning it.
A retryable failure while the window is still open still leaves the approval retryable — the round-3 fund-loss path must not reopen.
Tests cover both, plus the three uncovered settlement paths above against a claimed approval.
The approval type is checked explicitly before a TX_RESPONSE is honoured.
TODO.md updated in the same commit.
make check passes.

Found by the round-5 independent review of https://git.eeqj.de/sneak/AutistMask/pulls/205 and reproduced there. Non-blocking for that PR, which passed. The approval window is closed while an attempt holds the claim, and the attempt then fails **retryably** at the verify stage (`src/background/index.js:914`). The release path returns the approval to the pool — but the window it would be retried from no longer exists, and nothing else will ever settle it. The dApp's `eth_sendTransaction` promise stays pending indefinitely. This is the deliberate safe side of the round-3 trade, not a regression: settling on that path is precisely the fund-loss defect round 3 found, where an in-flight broadcast was reported to the dApp as `4001 User rejected the request.` while the transaction actually sent. Liveness was given up to buy that safety. It should not be given up permanently. Acceptable fix, from the review: **re-check the window on release, and settle `4001` if it is gone.** That distinguishes "retry is still possible" from "nobody can ever retry this", which the current release path conflates. ## Also in scope: three settlement paths with no test coverage The suite added by https://git.eeqj.de/sneak/AutistMask/pulls/205 covers the window-close, address-switch and reject paths against a claimed approval. Three more route through `settleApproval()` and are exercised only by the reviewer's throwaway probes: - `AUTISTMASK_APPROVAL_RESPONSE`, approve polarity - `AUTISTMASK_APPROVAL_RESPONSE`, reject polarity - the `runtime.onConnect` port disconnect All three were verified correct by hand — each is refused against a mid-broadcast claimed approval and the dApp still receives its result — but nothing in the repo would catch a regression. Given that every one of the five review rounds on that PR found a defect in exactly this machinery, they should be pinned. ## One tightening, not a defect A `TX_RESPONSE` carrying an attacker-paying artifact aimed at a `personal_sign` or connection approval (neither has `txParams`) broadcasts nothing — but it fails closed via a `TypeError` rather than an explicit type check. Worth making explicit so a future refactor cannot turn an incidental throw into a path. ## Definition of done - [ ] A retryable failure after the approval window has closed settles the dApp promise rather than orphaning it. - [ ] A retryable failure while the window is still open still leaves the approval retryable — the round-3 fund-loss path must not reopen. - [ ] Tests cover both, plus the three uncovered settlement paths above against a claimed approval. - [ ] The approval type is checked explicitly before a `TX_RESPONSE` is honoured. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.

clawbot referenced this issue

2026-08-12 12:04:11 +02:00

harden: verify the signed transaction against what the popup displayed (closes #216) #269

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#262