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

approvalVerify now compares every field of the signed artifact against the
approval, not a subset. Transaction types are allowlisted to 0/1/2 and any
field the module does not check is refused outright, so a future transaction
type cannot smuggle consequential fields past verification -- an EIP-7702
type-4 artifact that delegates the signer's own EOA while matching every
displayed field was accepted before this change. The serialized bytes handed
to broadcastTransaction are compared against the parsed artifact, so the
guarantee covers the bytes that actually go to the node.

Signing failures in the popup are retryable again. To make that safe, an
approval is claimed synchronously before the first await and every path that
resolves or removes one goes through a single chokepoint that refuses a claimed
approval. Without it, closing the approval window, switching the active address
or a late reject would report "User rejected the request." to the dApp while
the broadcast completed -- the user then redoes the transfer at a fresh nonce
and it sends twice.

Failure copy distinguishes the stage reached, so a user is never told to start
again from the site when the first attempt may already have reached the network.

src/background/index.js

@@ -13,7 +13,16 @@ const {
 } = require("../shared/state");
 const { refreshBalances, getProvider } = require("../shared/balances");
 const { debugFetch, log } = require("../shared/log");
-const { verifySignedTx, verifySignature } = require("../shared/approvalVerify");
+const {
+    verifySignedTx,
+    verifySignature,
+    failureIsRetryable,
+    describeTxFailure,
+    TX_STAGE_SIGN,
+    TX_STAGE_VERIFY,
+    TX_STAGE_BROADCAST,
+    TX_STAGE_INFLIGHT,
+} = require("../shared/approvalVerify");
 const {
     isPhishingDomain,
     refreshPhishingListOnSchedule,
@@ -107,6 +116,55 @@ function resetPopupUrl() {
     }
 }
 
+// Settle a pending approval: hand `result` to the promise the requesting page
+// is waiting on and retire the approval. This is the ONLY place an approval is
+// resolved or removed — the popup closing, an active-address switch, a reject
+// from the popup and the attempt that signs and broadcasts all come through
+// here — because a settlement that bypasses the claim below is a fund-loss bug
+// and enumerating the call sites has repeatedly missed one.
+//
+// A claimed approval belongs to the attempt holding the claim, and only that
+// attempt may settle it. Anything else settling first would leave the attempt
+// running to completion against an already-settled promise: the transaction
+// reaches the chain while the page is told "User rejected the request", and the
+// user's natural response is to send it again at a fresh nonce.
+//
+// Returns false when the approval is gone or claimed by someone else, so the
+// caller can refuse instead of assuming it settled.
+function settleApproval(id, result, options) {
+    const approval = pendingApprovals[id];
+    if (!approval) return false;
+    const holdsClaim = !!(options && options.holdsClaim);
+    if (approval.attemptInFlight && !holdsClaim) return false;
+    delete pendingApprovals[id];
+    approval.resolve(result);
+    resetPopupUrl();
+    return true;
+}
+
+// Take exclusive hold of a pending approval for one attempt, or refuse.
+//
+// An approval that failed retryably has to stay in pendingApprovals, so its
+// presence cannot be the interlock against a second attempt; this flag is. It
+// is set synchronously, before the handler's first await, so a second response
+// carrying the same id — a reloaded approval window re-rendering a live
+// Approve button, a popup that emits the message twice — finds the attempt
+// already running instead of starting an independent verify and broadcast.
+// Without it one approval can put two transactions on the chain: with the
+// ordinary dApp approval shape the page fixes no nonce, so two artifacts
+// signed at different nonces both verify.
+function claimApproval(approval) {
+    if (approval.attemptInFlight) return false;
+    approval.attemptInFlight = true;
+    return true;
+}
+
+// Release an approval whose attempt failed in a way the user can retry.
+// Nothing was broadcast, so the next attempt may claim it.
+function releaseApproval(approval) {
+    approval.attemptInFlight = false;
+}
+
 // Open approval in a separate popup window.
 // This is the primary mechanism for tx/sign approvals (triggered programmatically,
 // not from a user gesture) and the fallback for site-connection approvals.
@@ -215,8 +273,7 @@ runtime.onConnect.addListener((port) => {
                     // Keep pending — user can reopen the toolbar popup
                     return;
                 }
-                approval.resolve({ approved: false, remember: false });
-                delete pendingApprovals[id];
+                settleApproval(id, { approved: false, remember: false });
             }
             resetPopupUrl();
         });
@@ -547,15 +604,21 @@ async function broadcastAccountsChanged() {
     for (const key of Object.keys(connectedSites)) {
         delete connectedSites[key];
     }
-    // Reject and close any pending approval popups so they don't hang
+    // Reject and close any pending approval popups so they don't hang. An
+    // approval an attempt has already claimed is left alone entirely: it is
+    // being signed and broadcast right now, and neither rejecting it to the
+    // page nor closing the window it is reporting into is survivable.
     for (const [id, approval] of Object.entries(pendingApprovals)) {
-        if (approval.type === "tx" || approval.type === "sign") {
-            approval.resolve({
-                error: { code: 4001, message: "User rejected the request." },
-            });
-        } else {
-            approval.resolve({ approved: false, remember: false });
-        }
+        const rejection =
+            approval.type === "tx" || approval.type === "sign"
+                ? {
+                      error: {
+                          code: 4001,
+                          message: "User rejected the request.",
+                      },
+                  }
+                : { approved: false, remember: false };
+        if (!settleApproval(id, rejection)) continue;
         if (approval.windowId) {
             windowsApi.remove(approval.windowId, () => {
                 if (runtime.lastError) {
@@ -563,7 +626,6 @@ async function broadcastAccountsChanged() {
                 }
             });
         }
-        delete pendingApprovals[id];
     }
     resetPopupUrl();
     const s = await getState();
@@ -679,23 +741,26 @@ if (runtime.onStartup) {
 }
 startBackgroundJobs();
 
-// When approval window is closed without a response, treat as rejection
+// When approval window is closed without a response, treat as rejection.
+// "Without a response" is the operative part: the popup stays open across the
+// verify and broadcast it is waiting on, so a user closing an apparently-hung
+// window is an ordinary event with an attempt already in flight behind it.
+// settleApproval() refuses those, which leaves the attempt to report its real
+// outcome to the page.
 if (windowsApi && windowsApi.onRemoved) {
     windowsApi.onRemoved.addListener((windowId) => {
         for (const [id, approval] of Object.entries(pendingApprovals)) {
-            if (approval.windowId === windowId) {
-                if (approval.type === "tx" || approval.type === "sign") {
-                    approval.resolve({
-                        error: {
-                            code: 4001,
-                            message: "User rejected the request.",
-                        },
-                    });
-                } else {
-                    approval.resolve({ approved: false, remember: false });
-                }
-                delete pendingApprovals[id];
-            }
+            if (approval.windowId !== windowId) continue;
+            const rejection =
+                approval.type === "tx" || approval.type === "sign"
+                    ? {
+                          error: {
+                              code: 4001,
+                              message: "User rejected the request.",
+                          },
+                      }
+                    : { approved: false, remember: false };
+            settleApproval(id, rejection);
         }
     });
 }
@@ -761,14 +826,10 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
     }
 
     if (msg.type === "AUTISTMASK_APPROVAL_RESPONSE") {
-        const approval = pendingApprovals[msg.id];
-        if (approval) {
-            approval.resolve({
-                approved: msg.approved,
-                remember: msg.remember,
-            });
-            delete pendingApprovals[msg.id];
-        }
+        settleApproval(msg.id, {
+            approved: msg.approved,
+            remember: msg.remember,
+        });
         resetPopupUrl();
         return false;
     }
@@ -776,21 +837,50 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
     if (msg.type === "AUTISTMASK_TX_RESPONSE") {
         const approval = pendingApprovals[msg.id];
         if (!approval) return false;
-        delete pendingApprovals[msg.id];
-        resetPopupUrl();
 
+        // A reject arriving while an attempt holds the approval is refused,
+        // not honoured: the attempt is on its way to broadcasting the
+        // transaction, and resolving 4001 here would tell the page the request
+        // was rejected while it goes out.
         if (!msg.approved) {
-            approval.resolve({
-                error: { code: 4001, message: "User rejected the request." },
-            });
+            if (
+                !settleApproval(msg.id, {
+                    error: {
+                        code: 4001,
+                        message: "User rejected the request.",
+                    },
+                })
+            ) {
+                sendResponse({
+                    error: "This transaction is already being sent.",
+                    retryable: false,
+                    stage: TX_STAGE_BROADCAST,
+                });
+                return false;
+            }
             return true;
         }
 
-        // The popup signs; it reports back here when it could not. Fail the
-        // request the same way this handler used to when it did the signing.
+        // The popup signs; it reports back here when it could not. Keep the
+        // approval so the user can correct the problem and try again with the
+        // transaction they already saw.
         if (msg.error) {
-            approval.resolve({ error: { message: msg.error } });
-            sendResponse({ error: msg.error });
+            const outcome = describeTxFailure(TX_STAGE_SIGN, msg.error);
+            sendResponse({
+                error: outcome.error,
+                retryable: outcome.retryable,
+                stage: TX_STAGE_SIGN,
+            });
+            return false;
+        }
+
+        // Exactly one broadcast per approval, whatever the popup sends.
+        if (!claimApproval(approval)) {
+            sendResponse({
+                error: "This transaction is already being sent.",
+                retryable: false,
+                stage: TX_STAGE_BROADCAST,
+            });
             return false;
         }
 
@@ -800,22 +890,63 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
                 const activeAddress = await getActiveAddress();
                 // The popup holds the secret, but the background stays the
                 // authority on what is broadcast: the raw transaction must be
-                // the approved one, signed by the approved address.
+                // the approved one, signed by the approved address, on the
+                // network that is selected.
                 verifySignedTx(
                     msg.rawSignedTx,
                     approval.txParams,
                     activeAddress,
+                    currentNetwork().chainId,
                 );
+            } catch (e) {
+                // A signed transaction that is not the approved one is not
+                // retried against that approval; it is refused outright.
+                // Anything else that failed before the check ran is the
+                // user's to retry.
+                const outcome = describeTxFailure(TX_STAGE_VERIFY, e);
+                if (outcome.spendApproval) {
+                    settleApproval(
+                        msg.id,
+                        { error: { message: outcome.error } },
+                        { holdsClaim: true },
+                    );
+                } else {
+                    releaseApproval(approval);
+                }
+                sendResponse({
+                    error: outcome.error,
+                    retryable: outcome.retryable,
+                    stage: TX_STAGE_VERIFY,
+                });
+                return;
+            }
+
+            try {
                 const provider = getProvider(state.rpcUrl);
                 const tx = await provider.broadcastTransaction(msg.rawSignedTx);
-                approval.resolve({ txHash: tx.hash });
+                settleApproval(
+                    msg.id,
+                    { txHash: tx.hash },
+                    { holdsClaim: true },
+                );
                 sendResponse({ txHash: tx.hash });
             } catch (e) {
-                const errMsg = e.shortMessage || e.message;
-                approval.resolve({
-                    error: { message: errMsg },
+                // Terminal, never retried: the node may have accepted the
+                // transaction and still failed to answer, and the popup's
+                // retry re-signs at a freshly fetched nonce rather than
+                // re-broadcasting these bytes. Retrying would send the
+                // approved transfer a second time.
+                const outcome = describeTxFailure(TX_STAGE_BROADCAST, e);
+                settleApproval(
+                    msg.id,
+                    { error: { message: outcome.error } },
+                    { holdsClaim: true },
+                );
+                sendResponse({
+                    error: outcome.error,
+                    retryable: outcome.retryable,
+                    stage: TX_STAGE_BROADCAST,
                 });
-                sendResponse({ error: errMsg });
             }
         })();
         return true;
@@ -824,21 +955,43 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
     if (msg.type === "AUTISTMASK_SIGN_RESPONSE") {
         const approval = pendingApprovals[msg.id];
         if (!approval) return false;
-        delete pendingApprovals[msg.id];
-        resetPopupUrl();
 
+        // Same as the transaction path: a reject cannot retire an approval an
+        // attempt already holds.
         if (!msg.approved) {
-            approval.resolve({
-                error: { code: 4001, message: "User rejected the request." },
-            });
+            if (
+                !settleApproval(msg.id, {
+                    error: {
+                        code: 4001,
+                        message: "User rejected the request.",
+                    },
+                })
+            ) {
+                sendResponse({
+                    error: "This request is already being signed.",
+                    retryable: false,
+                    stage: TX_STAGE_INFLIGHT,
+                });
+                return false;
+            }
             return true;
         }
 
-        // The popup signs; it reports back here when it could not. Fail the
-        // request the same way this handler used to when it did the signing.
+        // The popup signs; it reports back here when it could not. Keep the
+        // approval so the user can correct the problem and try again with the
+        // message they already saw.
         if (msg.error) {
-            approval.resolve({ error: { message: msg.error } });
-            sendResponse({ error: msg.error });
+            sendResponse({ error: msg.error, retryable: true });
+            return false;
+        }
+
+        // Exactly one signature handed back per approval.
+        if (!claimApproval(approval)) {
+            sendResponse({
+                error: "This request is already being signed.",
+                retryable: false,
+                stage: TX_STAGE_INFLIGHT,
+            });
             return false;
         }
 
@@ -851,14 +1004,21 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
                 // address.
                 const signature = msg.signature;
                 verifySignature(approval.signParams, signature, activeAddress);
-                approval.resolve({ signature });
+                settleApproval(msg.id, { signature }, { holdsClaim: true });
                 sendResponse({ signature });
             } catch (e) {
                 const errMsg = e.shortMessage || e.message;
-                approval.resolve({
-                    error: { message: errMsg },
-                });
-                sendResponse({ error: errMsg });
+                const retryable = failureIsRetryable(e);
+                if (!retryable) {
+                    settleApproval(
+                        msg.id,
+                        { error: { message: errMsg } },
+                        { holdsClaim: true },
+                    );
+                } else {
+                    releaseApproval(approval);
+                }
+                sendResponse({ error: errMsg, retryable });
             }
         })();
         return true;