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

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.

tests/approvalVerify.test.js

@@ -3,6 +3,10 @@ const {
     verifySignedTx,
     verifySignature,
     sameAddress,
+    failureIsRetryable,
+    describeSigningFailure,
+    MAX_GAS_LIMIT,
+    MAX_FEE_PER_GAS,
 } = require("../src/shared/approvalVerify");
 const { getSignerForAddress } = require("../src/shared/wallet");
 
@@ -18,6 +22,10 @@ const other = new Wallet(OTHER_KEY);
 const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
 const OTHER_RECIPIENT = "0xdAC17F958D2ee523a2206206994597C13D831ec7";
 
+// The chain id of the selected network, as networks.js carries it.
+const SELECTED = "0x1";
+const SEPOLIA = "0xaa36a7";
+
 // Approved parameters as a dApp would supply them over eth_sendTransaction.
 const TX_PARAMS = {
     from: signer.address,
@@ -27,25 +35,38 @@ const TX_PARAMS = {
     gas: "0x5208",
 };
 
+// The values populateTransaction() fills in when the dApp fixed none of them.
+const POPULATED = {
+    chainId: 1,
+    nonce: 7,
+    gasLimit: 100000n,
+    maxFeePerGas: 2000000000n,
+    maxPriorityFeePerGas: 1000000000n,
+    type: 2,
+};
+
 // Build a signable transaction from approved params. The popup does the same
 // thing through populateTransaction(); here the fields are fixed so the test
-// needs no provider.
-function txFor(params) {
+// needs no provider. `overrides` stands in for what a tampered or misbuilt
+// popup would put on the wire.
+function txFor(params, overrides) {
     return {
-        chainId: 1,
-        nonce: 7,
-        gasLimit: 100000n,
-        maxFeePerGas: 2000000000n,
-        maxPriorityFeePerGas: 1000000000n,
-        type: 2,
+        ...POPULATED,
         to: params.to,
         value: params.value === undefined ? 0n : BigInt(params.value),
         data: params.data || "0x",
+        ...(overrides || {}),
     };
 }
 
-async function signedFor(params, withWallet) {
-    return (withWallet || signer).signTransaction(txFor(params));
+async function signedFor(params, withWallet, overrides) {
+    return (withWallet || signer).signTransaction(txFor(params, overrides));
+}
+
+// Sign the approved transaction with one field changed from what was
+// populated, which is the shape of every tamper case below.
+async function signedWith(overrides) {
+    return signedFor(TX_PARAMS, signer, overrides);
 }
 
 describe("sameAddress", () => {
@@ -71,7 +92,7 @@ describe("sameAddress", () => {
 describe("verifySignedTx", () => {
     test("accepts the approved transaction signed by the approved address", async () => {
         const raw = await signedFor(TX_PARAMS);
-        const parsed = verifySignedTx(raw, TX_PARAMS, signer.address);
+        const parsed = verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED);
         expect(parsed.from).toBe(signer.address);
         expect(parsed.hash).toBe(Transaction.from(raw).hash);
     });
@@ -79,14 +100,16 @@ describe("verifySignedTx", () => {
     test("accepts a contract creation with no recipient", async () => {
         const params = { to: undefined, value: "0x0", data: "0x600160005500" };
         const raw = await signedFor(params);
-        expect(() => verifySignedTx(raw, params, signer.address)).not.toThrow();
+        expect(() =>
+            verifySignedTx(raw, params, signer.address, SELECTED),
+        ).not.toThrow();
     });
 
     test("accepts an absent value as zero", async () => {
         const approved = { to: RECIPIENT, data: "0x" };
         const raw = await signedFor(approved);
         expect(() =>
-            verifySignedTx(raw, approved, signer.address),
+            verifySignedTx(raw, approved, signer.address, SELECTED),
         ).not.toThrow();
     });
 
@@ -94,7 +117,7 @@ describe("verifySignedTx", () => {
         const approved = { to: RECIPIENT, value: "0x0", data: "0xDEADBEEF" };
         const raw = await signedFor(approved);
         expect(() =>
-            verifySignedTx(raw, approved, signer.address),
+            verifySignedTx(raw, approved, signer.address, SELECTED),
         ).not.toThrow();
     });
 
@@ -103,9 +126,9 @@ describe("verifySignedTx", () => {
             ...TX_PARAMS,
             to: OTHER_RECIPIENT,
         });
-        expect(() => verifySignedTx(raw, TX_PARAMS, signer.address)).toThrow(
-            /approved recipient/,
-        );
+        expect(() =>
+            verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+        ).toThrow(/approved recipient/);
     });
 
     test("rejects an inflated value", async () => {
@@ -113,48 +136,48 @@ describe("verifySignedTx", () => {
             ...TX_PARAMS,
             value: "0x4563918244f40000",
         });
-        expect(() => verifySignedTx(raw, TX_PARAMS, signer.address)).toThrow(
-            /approved value/,
-        );
+        expect(() =>
+            verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+        ).toThrow(/approved value/);
     });
 
     test("rejects substituted call data", async () => {
         const raw = await signedFor({ ...TX_PARAMS, data: "0xc0ffee" });
-        expect(() => verifySignedTx(raw, TX_PARAMS, signer.address)).toThrow(
-            /approved call data/,
-        );
+        expect(() =>
+            verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+        ).toThrow(/approved call data/);
     });
 
     test("rejects a transaction signed by a different address", async () => {
         const raw = await signedFor(TX_PARAMS, other);
-        expect(() => verifySignedTx(raw, TX_PARAMS, signer.address)).toThrow(
-            /different address/,
-        );
+        expect(() =>
+            verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+        ).toThrow(/different address/);
     });
 
     test("rejects an unsigned transaction", () => {
         const unsigned = Transaction.from(txFor(TX_PARAMS)).unsignedSerialized;
         expect(() =>
-            verifySignedTx(unsigned, TX_PARAMS, signer.address),
+            verifySignedTx(unsigned, TX_PARAMS, signer.address, SELECTED),
         ).toThrow(/no valid signature/);
     });
 
     test("rejects a missing or malformed payload", () => {
         expect(() =>
-            verifySignedTx(undefined, TX_PARAMS, signer.address),
+            verifySignedTx(undefined, TX_PARAMS, signer.address, SELECTED),
         ).toThrow(/missing or malformed/);
-        expect(() => verifySignedTx("nope", TX_PARAMS, signer.address)).toThrow(
-            /missing or malformed/,
-        );
         expect(() =>
-            verifySignedTx("0xc0ffee", TX_PARAMS, signer.address),
+            verifySignedTx("nope", TX_PARAMS, signer.address, SELECTED),
+        ).toThrow(/missing or malformed/);
+        expect(() =>
+            verifySignedTx("0xc0ffee", TX_PARAMS, signer.address, SELECTED),
         ).toThrow(/could not be decoded/);
     });
 
     test("every rejection message is a full sentence", async () => {
         const raw = await signedFor({ ...TX_PARAMS, to: OTHER_RECIPIENT });
         try {
-            verifySignedTx(raw, TX_PARAMS, signer.address);
+            verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED);
             throw new Error("expected a rejection");
         } catch (e) {
             expect(e.message).toMatch(/^[A-Z].*\.$/);
@@ -162,6 +185,234 @@ describe("verifySignedTx", () => {
     });
 });
 
+// One case per consequential field: the field alone differs from what was
+// approved, and that alone must refuse the signature.
+describe("verifySignedTx field comparison", () => {
+    test("rejects a chain id that is not the selected network", async () => {
+        const raw = await signedWith({ chainId: 11155111 });
+        expect(() =>
+            verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+        ).toThrow(/different network than the one that is selected/);
+    });
+
+    test("rejects a chain id that is not the approved one", async () => {
+        // Selected network and signed chain id agree; the dApp asked for a
+        // different chain, so the artifact is not what was approved.
+        const approved = { ...TX_PARAMS, chainId: SEPOLIA };
+        const raw = await signedWith({});
+        expect(() =>
+            verifySignedTx(raw, approved, signer.address, SELECTED),
+        ).toThrow(/different network than the one that was approved/);
+    });
+
+    test("refuses when the selected network is unknown", async () => {
+        const raw = await signedWith({});
+        expect(() =>
+            verifySignedTx(raw, TX_PARAMS, signer.address, undefined),
+        ).toThrow(/selected network is unknown/);
+    });
+
+    test("rejects a substituted nonce", async () => {
+        const approved = { ...TX_PARAMS, nonce: 7 };
+        const raw = await signedWith({ nonce: 8 });
+        expect(() =>
+            verifySignedTx(raw, approved, signer.address, SELECTED),
+        ).toThrow(/approved nonce/);
+    });
+
+    test("rejects a substituted gas limit", async () => {
+        const approved = { ...TX_PARAMS, gasLimit: "0x186a0" };
+        const raw = await signedWith({ gasLimit: 250000n });
+        expect(() =>
+            verifySignedTx(raw, approved, signer.address, SELECTED),
+        ).toThrow(/approved gas limit/);
+    });
+
+    test("rejects a substituted maximum fee per gas", async () => {
+        const approved = { ...TX_PARAMS, maxFeePerGas: "0x77359400" };
+        const raw = await signedWith({ maxFeePerGas: 900000000000n });
+        expect(() =>
+            verifySignedTx(raw, approved, signer.address, SELECTED),
+        ).toThrow(/approved maximum fee per gas/);
+    });
+
+    test("rejects a substituted maximum priority fee per gas", async () => {
+        const approved = { ...TX_PARAMS, maxPriorityFeePerGas: "0x3b9aca00" };
+        const raw = await signedWith({ maxPriorityFeePerGas: 1500000000n });
+        expect(() =>
+            verifySignedTx(raw, approved, signer.address, SELECTED),
+        ).toThrow(/approved maximum priority fee per gas/);
+    });
+
+    test("rejects a substituted legacy gas price", async () => {
+        const approved = { ...TX_PARAMS, gasPrice: "0x77359400" };
+        const legacy = {
+            type: 0,
+            gasPrice: 9000000000n,
+            maxFeePerGas: null,
+            maxPriorityFeePerGas: null,
+        };
+        const raw = await signedWith(legacy);
+        expect(() =>
+            verifySignedTx(raw, approved, signer.address, SELECTED),
+        ).toThrow(/approved gas price/);
+    });
+
+    test("rejects an approved legacy fee signed as an EIP-1559 fee", async () => {
+        const approved = { ...TX_PARAMS, gasPrice: "0x77359400" };
+        const raw = await signedWith({});
+        expect(() =>
+            verifySignedTx(raw, approved, signer.address, SELECTED),
+        ).toThrow(/approved fee mechanism/);
+    });
+
+    test("rejects an approved EIP-1559 fee signed as a legacy fee", async () => {
+        const approved = { ...TX_PARAMS, maxFeePerGas: "0x77359400" };
+        const raw = await signedWith({
+            type: 0,
+            gasPrice: 2000000000n,
+            maxFeePerGas: null,
+            maxPriorityFeePerGas: null,
+        });
+        expect(() =>
+            verifySignedTx(raw, approved, signer.address, SELECTED),
+        ).toThrow(/approved fee mechanism/);
+    });
+
+    test("rejects a gas limit above anything a supported network accepts", async () => {
+        const raw = await signedWith({ gasLimit: MAX_GAS_LIMIT + 1n });
+        expect(() =>
+            verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+        ).toThrow(/gas limit no network this wallet supports/);
+    });
+
+    test("rejects an absurd fee per gas the approval never fixed", async () => {
+        const raw = await signedWith({
+            maxFeePerGas: MAX_FEE_PER_GAS + 1n,
+            maxPriorityFeePerGas: MAX_FEE_PER_GAS + 1n,
+        });
+        expect(() =>
+            verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+        ).toThrow(/fee per gas far above any plausible value/);
+    });
+
+    test("every field mismatch is a refusal, not a warning", async () => {
+        const raw = await signedWith({ nonce: 8 });
+        try {
+            verifySignedTx(
+                raw,
+                { ...TX_PARAMS, nonce: 7 },
+                signer.address,
+                SELECTED,
+            );
+            throw new Error("expected a rejection");
+        } catch (e) {
+            expect(e.approvalMismatch).toBe(true);
+            expect(e.message).toMatch(/^[A-Z].*\.$/);
+        }
+    });
+});
+
+// The approval and the artifact spell the same values differently. None of
+// these differences is tampering, so none may refuse the signature.
+describe("verifySignedTx normalization", () => {
+    test("accepts a decimal chain id against a hex selected network", async () => {
+        const raw = await signedWith({});
+        expect(() =>
+            verifySignedTx(raw, TX_PARAMS, signer.address, 1),
+        ).not.toThrow();
+        expect(() =>
+            verifySignedTx(raw, TX_PARAMS, signer.address, "1"),
+        ).not.toThrow();
+    });
+
+    test("accepts an approved chain id written in hex", async () => {
+        const raw = await signedWith({});
+        const approved = { ...TX_PARAMS, chainId: "0x1" };
+        expect(() =>
+            verifySignedTx(raw, approved, signer.address, SELECTED),
+        ).not.toThrow();
+    });
+
+    test("accepts a hex nonce against a numeric one", async () => {
+        const raw = await signedWith({ nonce: 7 });
+        expect(() =>
+            verifySignedTx(
+                raw,
+                { ...TX_PARAMS, nonce: "0x7" },
+                signer.address,
+                SELECTED,
+            ),
+        ).not.toThrow();
+    });
+
+    test("accepts a decimal gas limit against a hex one", async () => {
+        const raw = await signedWith({ gasLimit: 100000n });
+        expect(() =>
+            verifySignedTx(
+                raw,
+                { ...TX_PARAMS, gasLimit: "100000" },
+                signer.address,
+                SELECTED,
+            ),
+        ).not.toThrow();
+    });
+
+    test("accepts fee fields spelled as hex, decimal, number and bigint", async () => {
+        const raw = await signedWith({});
+        for (const maxFee of [
+            "0x77359400",
+            "2000000000",
+            2000000000,
+            2000000000n,
+        ]) {
+            expect(() =>
+                verifySignedTx(
+                    raw,
+                    { ...TX_PARAMS, maxFeePerGas: maxFee },
+                    signer.address,
+                    SELECTED,
+                ),
+            ).not.toThrow();
+        }
+    });
+
+    test("accepts an approval that fixes no nonce, gas or fee at all", async () => {
+        const raw = await signedWith({});
+        expect(() =>
+            verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+        ).not.toThrow();
+    });
+
+    test("accepts an approval whose recipient case differs", async () => {
+        const raw = await signedWith({});
+        const approved = { ...TX_PARAMS, to: RECIPIENT.toLowerCase() };
+        expect(() =>
+            verifySignedTx(raw, approved, signer.address, SELECTED),
+        ).not.toThrow();
+    });
+
+    test("accepts absent call data against 0x", async () => {
+        const approved = { to: RECIPIENT, value: "0x0" };
+        const raw = await signedFor({ ...approved, data: "0x" });
+        expect(() =>
+            verifySignedTx(raw, approved, signer.address, SELECTED),
+        ).not.toThrow();
+    });
+
+    test("refuses an approved quantity that is not a number", async () => {
+        const raw = await signedWith({});
+        expect(() =>
+            verifySignedTx(
+                raw,
+                { ...TX_PARAMS, maxFeePerGas: "cheap" },
+                signer.address,
+                SELECTED,
+            ),
+        ).toThrow(/is not a number/);
+    });
+});
+
 const TYPED_DATA = JSON.stringify({
     domain: {
         name: "AutistMask Test",
@@ -281,6 +532,66 @@ describe("verifySignature", () => {
     });
 });
 
+// What happens after a signing attempt fails: the background keeps the
+// approval for anything the user can correct, and the popup only offers the
+// button again when it did.
+describe("signing failure and retry", () => {
+    test("a failure that is not a mismatch leaves the approval retryable", () => {
+        expect(failureIsRetryable(new Error("The node is unreachable."))).toBe(
+            true,
+        );
+        expect(failureIsRetryable(undefined)).toBe(true);
+    });
+
+    test("a mismatch spends the approval", async () => {
+        const raw = await signedFor({ ...TX_PARAMS, to: OTHER_RECIPIENT });
+        try {
+            verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED);
+            throw new Error("expected a rejection");
+        } catch (e) {
+            expect(failureIsRetryable(e)).toBe(false);
+        }
+    });
+
+    test("a retryable failure keeps the button usable and says only what failed", () => {
+        const outcome = describeSigningFailure(
+            { error: "The node rejected the transaction.", retryable: true },
+            "The transaction could not be sent.",
+        );
+        expect(outcome.retryable).toBe(true);
+        expect(outcome.message).toBe("The node rejected the transaction.");
+    });
+
+    test("a refusal tells the user to start again from the site", () => {
+        const outcome = describeSigningFailure(
+            {
+                error: "The signed transaction does not go to the approved recipient.",
+                retryable: false,
+            },
+            "The transaction could not be sent.",
+        );
+        expect(outcome.retryable).toBe(false);
+        expect(outcome.message).toMatch(/start it again from the site\.$/);
+    });
+
+    test("a response the background never sent is treated as a spent approval", () => {
+        const outcome = describeSigningFailure(
+            undefined,
+            "The transaction could not be sent.",
+        );
+        expect(outcome.retryable).toBe(false);
+        expect(outcome.message).toMatch(/^The transaction could not be sent\./);
+    });
+
+    test("every failure message is a full sentence", () => {
+        const outcome = describeSigningFailure(
+            { error: "The node is on fire", retryable: true },
+            "The transaction could not be sent.",
+        );
+        expect(outcome.message).toMatch(/^[A-Z].*\.$/);
+    });
+});
+
 // End-to-end over the messaging boundary, without a browser: run the exact
 // sequence the approval popup runs, then hand the artifact to the exact check
 // the background runs before it broadcasts or resolves. Only what the popup
@@ -314,7 +625,12 @@ describe("popup signing sequence to background verification", () => {
 
     test("a populated, signed transaction is accepted and broadcastable", async () => {
         const rawSignedTx = await popupSignsTx(TX_PARAMS);
-        const parsed = verifySignedTx(rawSignedTx, TX_PARAMS, signer.address);
+        const parsed = verifySignedTx(
+            rawSignedTx,
+            TX_PARAMS,
+            signer.address,
+            SELECTED,
+        );
         expect(parsed.nonce).toBe(7);
         expect(parsed.chainId).toBe(1n);
         expect(parsed.gasLimit).toBe(21000n);
@@ -349,7 +665,14 @@ describe("popup signing sequence to background verification", () => {
             to: OTHER_RECIPIENT,
         });
         expect(() =>
-            verifySignedTx(rawSignedTx, TX_PARAMS, signer.address),
+            verifySignedTx(rawSignedTx, TX_PARAMS, signer.address, SELECTED),
         ).toThrow(/approved recipient/);
     });
+
+    test("the background rejects a transaction populated on another network", async () => {
+        const rawSignedTx = await popupSignsTx(TX_PARAMS);
+        expect(() =>
+            verifySignedTx(rawSignedTx, TX_PARAMS, signer.address, SEPOLIA),
+        ).toThrow(/different network than the one that is selected/);
+    });
 });