harden: approvalVerify does not compare chainId, nonce or fee fields; failed signing leaves an unretryable button #174

New Issue

2026-08-09T05:02:55+02:00

clawbot commented

2026-08-09 05:02:55 +02:00

Two non-blocking findings from the independent review of PR #171 (issue #157),
recorded so they are not lost. Same file, same review, so grouping them.

1. `verifySignedTx` does not check the fee and ordering fields

src/shared/approvalVerify.js re-derives the signer from the artifact and
compares from, to, value and data against the held approval. It does
not compare chainId, nonce, gasLimit, or the fee fields
(maxFeePerGas / maxPriorityFeePerGas / gasPrice).

Not currently exploitable. POPUP_ONLY_TYPES at
src/background/index.js:663-674 restricts who can send the message, and only
a holder of the key can produce an artifact that passes verification at all.
But to/value/data deliberately exist to catch a signed transaction that
does not match what the user approved, and a hostile or buggy fee field is
precisely the case that set does not cover — an absurd maxFeePerGas drains
value to the validator without touching to or value.

Since the whole point of approvalVerify.js is that the background does not
blindly broadcast whatever an extension page hands it, the check should be
complete rather than nearly complete.

Requirements

Extend verifySignedTx to compare chainId and the fee fields against the
approval, with full-sentence rejection messages matching the existing style.
nonce and gasLimit need thought rather than a blanket equality check:
they are produced by populateTransaction in the popup, not supplied by the
dApp, so the approval may legitimately not carry a value to compare against.
Decide per-field whether the correct rule is equality, a sanity bound, or no
check, and record the reasoning in the PR. Do not add a comparison that
fails on a legitimate transaction.
chainId in particular must be checked against the currently selected
network, not merely against whatever the artifact claims — a cross-chain
replay is the failure this prevents.
Extend tests/approvalVerify.test.js with a tamper case per newly checked
field, mirroring the existing tamper tests.

2. A failed popup-side signing leaves a button that cannot succeed

src/popup/views/approval.js:644-651 re-enables the approve button after a
popup-side signing failure, but the background has already deleted the
approval at src/background/index.js:772. A retry therefore cannot succeed —
the user clicks again and nothing can happen.

The reviewer notes this is a pre-existing shape, not a regression introduced by
#171. It is still wrong: per the README Clickable Affordance policy, a control
that looks actionable must do something.

Requirements

Decide the correct behaviour and implement it: either keep the approval alive
in the background so a retry genuinely works, or disable the button and show
a full-sentence explanation that the request must be re-initiated from the
site. The first is better UX; the second is simpler and honest. State the
choice and reasoning in the PR.
Note the contrast with the wrong-password path, which #171 made properly
retryable by failing in the popup before any message is sent. Match that
behaviour if it is reachable here.
No layout shift when the message appears.

Definition of done

verifySignedTx compares chainId and the fee fields; nonce and
gasLimit are handled per a documented, justified rule.
chainId is validated against the selected network, not just the
artifact.
A tamper test exists for each newly compared field and fails without the
fix.
No legitimate transaction is rejected by the new checks — confirm the
existing round-trip test still passes unmodified.
The failed-signing path no longer presents a button that cannot succeed.
TODO.md updated in the same commit.
make check passes.

Two non-blocking findings from the independent review of PR #171 (issue #157), recorded so they are not lost. Same file, same review, so grouping them. ## 1. `verifySignedTx` does not check the fee and ordering fields `src/shared/approvalVerify.js` re-derives the signer from the artifact and compares `from`, `to`, `value` and `data` against the held approval. It does **not** compare `chainId`, `nonce`, `gasLimit`, or the fee fields (`maxFeePerGas` / `maxPriorityFeePerGas` / `gasPrice`). Not currently exploitable. `POPUP_ONLY_TYPES` at `src/background/index.js:663-674` restricts who can send the message, and only a holder of the key can produce an artifact that passes verification at all. But `to`/`value`/`data` deliberately exist to catch a signed transaction that does not match what the user approved, and a hostile or buggy fee field is precisely the case that set does not cover — an absurd `maxFeePerGas` drains value to the validator without touching `to` or `value`. Since the whole point of `approvalVerify.js` is that the background does not blindly broadcast whatever an extension page hands it, the check should be complete rather than nearly complete. **Requirements** - Extend `verifySignedTx` to compare `chainId` and the fee fields against the approval, with full-sentence rejection messages matching the existing style. - `nonce` and `gasLimit` need thought rather than a blanket equality check: they are produced by `populateTransaction` in the popup, not supplied by the dApp, so the approval may legitimately not carry a value to compare against. Decide per-field whether the correct rule is equality, a sanity bound, or no check, and record the reasoning in the PR. Do not add a comparison that fails on a legitimate transaction. - `chainId` in particular must be checked against the currently selected network, not merely against whatever the artifact claims — a cross-chain replay is the failure this prevents. - Extend `tests/approvalVerify.test.js` with a tamper case per newly checked field, mirroring the existing tamper tests. ## 2. A failed popup-side signing leaves a button that cannot succeed `src/popup/views/approval.js:644-651` re-enables the approve button after a popup-side signing failure, but the background has already deleted the approval at `src/background/index.js:772`. A retry therefore cannot succeed — the user clicks again and nothing can happen. The reviewer notes this is a pre-existing shape, not a regression introduced by #171. It is still wrong: per the README Clickable Affordance policy, a control that looks actionable must do something. **Requirements** - Decide the correct behaviour and implement it: either keep the approval alive in the background so a retry genuinely works, or disable the button and show a full-sentence explanation that the request must be re-initiated from the site. The first is better UX; the second is simpler and honest. State the choice and reasoning in the PR. - Note the contrast with the wrong-password path, which #171 made properly retryable by failing in the popup before any message is sent. Match that behaviour if it is reachable here. - No layout shift when the message appears. ## Definition of done - [ ] `verifySignedTx` compares `chainId` and the fee fields; `nonce` and `gasLimit` are handled per a documented, justified rule. - [ ] `chainId` is validated against the selected network, not just the artifact. - [ ] A tamper test exists for each newly compared field and fails without the fix. - [ ] No legitimate transaction is rejected by the new checks — confirm the existing round-trip test still passes unmodified. - [ ] The failed-signing path no longer presents a button that cannot succeed. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.

clawbot added this to the 1.0.0 milestone 2026-08-09 05:02:55 +02:00

clawbot referenced this issue from a commit

2026-08-11 14:25:45 +02:00

harden: verify all approval fields and make failed signing retryable (closes #174)

clawbot referenced a pull request that will close this issue

2026-08-11 14:26:19 +02:00

harden: verify all approval fields and make failed signing retryable (closes #174) #205

clawbot referenced this issue from a commit

2026-08-11 14:27:09 +02:00

harden: verify all approval fields and make failed signing retryable (closes #174)

clawbot referenced this issue

2026-08-11 14:28:48 +02:00