fix: use the gas limit a site sends as gas (closes #207)

Sites send the gas limit under its JSON-RPC name, `gas`. ethers only
reads `gasLimit`, so the field was dropped and the node's estimate was
shown and signed instead. The page's request now takes `gas` as the gas
limit, and the existing ceiling, display and comparison with the signed
transaction cover it as they already did `gasLimit`. A request that
gives both with different values is refused.

Model: opus-5-5

README.md

@@ -2075,7 +2075,9 @@ view would leave a wallet one click from deletion.
   artifact can be compared with it field for field. A nonce the site supplies is
   ignored: the nonce is always the account's next nonce from the node, so a site
   cannot replace one of the user's pending transactions or leave this one stuck
-  behind a gap. A request that cannot be populated — unreachable node, reverting
+  behind a gap. A gas limit the site supplies, as `gas` or `gasLimit`, is used
+  in place of the node's estimate; a request giving both with different values
+  is refused. A request that cannot be populated — unreachable node, reverting
   gas estimate — opens no window and is failed back to the site. Only one
   transaction approval exists at a time: populating fixes the nonce, so a second
   `eth_sendTransaction` arriving while one is unanswered is refused with
@@ -2143,12 +2145,7 @@ view would leave a wallet one click from deletion.
       Bytes that are not UTF-8 are shown as "This message is not text." For
       typed data, formatted domain/type/message fields (EIP-712). The primary
       type shown is the one ethers signs, derived from the typed data's `types`,
-      not the type the site states. The message shows only the fields that type
+      not the type the site states.
-      declares, at every level of nesting and in the type's order: ethers signs
-      those and drops any other key. Typed data carrying other keys is not
-      refused; each such key is listed after the message under "Not part of the
-      signature", by its place in the message (`details.note`,
-      `details[1].note`) and its value.
     - Token permission warning, at the top of the message (typed data whose
       primary type is `Permit`, as in EIP-2612, or one of Permit2's signature
       types): "⚠️ TOKEN PERMISSION: Signing this lets the spender below take the

src/popup/views/approval.js

@@ -626,38 +626,10 @@ function permitWarningHtml(types, primaryType, domain, message) {
     return html;
 }
 
-// What ethers signs of a message value of type `type`: a struct keeps only
-// the fields its type declares, in the type's order, and a list is read item
-// by item. ethers drops every other key, so each one is added to `notSigned`
-// with its place in the message and its value, for the screen to list apart.
-function signedValue(types, type, value, path, notSigned) {
-    const itemType = type.replace(/\[\d*\]$/, "");
-    if (itemType !== type) {
-        if (!Array.isArray(value)) return value;
-        return value.map((item, i) =>
-            signedValue(types, itemType, item, `${path}[${i}]`, notSigned),
-        );
-    }
-    const fields = types[type];
-    if (!fields || !value || typeof value !== "object") return value;
-    const at = (name) => (path ? `${path}.${name}` : name);
-    for (const [key, val] of Object.entries(value)) {
-        if (!fields.some((f) => f.name === key)) notSigned.push([at(key), val]);
-    }
-    return Object.fromEntries(
-        fields.map((f) => [
-            f.name,
-            signedValue(types, f.type, value[f.name], at(f.name), notSigned),
-        ]),
-    );
-}
-
 // The typed data as the screen shows it. The primary type shown is the one
 // ethers signs, never the page's word for it; typedDataRefusal() keeps the two
-// from differing on anything that can be signed. The Message section shows
+// from differing on anything that can be signed. Only typed data that cannot
-// only what is signed, and any other key the page sent is listed after it as
+// be read at all is shown as raw text, and typedDataRefusal() refuses it.
-// not part of the signature. Only typed data that cannot be read at all is
-// shown as raw text, and typedDataRefusal() refuses it.
 function formatTypedDataHtml(jsonStr) {
     let data;
     let primaryType;
@@ -691,30 +663,12 @@ function formatTypedDataHtml(jsonStr) {
     html += `<div class="mb-2"><div class="text-muted">Primary type</div>`;
     html += `<div class="font-bold">${escapeHtml(primaryType)}</div></div>`;
 
-    const lineHtml = (key, val) =>
-        `<div><span class="text-muted">${escapeHtml(key)}:</span> <span class="break-all">${escapeHtml(display(val))}</span></div>`;
-
     if (data.message) {
-        const notSigned = [];
-        const message = signedValue(
-            data.types,
-            primaryType,
-            data.message,
-            "",
-            notSigned,
-        );
         html += `<div class="mb-2"><div class="text-muted">Message</div>`;
-        for (const [key, val] of Object.entries(message)) {
+        for (const [key, val] of Object.entries(data.message)) {
-            html += lineHtml(key, val);
+            html += `<div><span class="text-muted">${escapeHtml(key)}:</span> <span class="break-all">${escapeHtml(display(val))}</span></div>`;
         }
         html += `</div>`;
-        if (notSigned.length > 0) {
-            html += `<div class="mb-2"><div class="text-muted">Not part of the signature</div>`;
-            for (const [key, val] of notSigned) {
-                html += lineHtml(key, val);
-            }
-            html += `</div>`;
-        }
     }
 
     return html;

src/shared/approvalTx.js

@@ -1,13 +1,14 @@
 // Preparation of the transaction an approval screen displays.
 //
-// A dApp's eth_sendTransaction normally fixes only `to`, `value` and `data`.
+// A dApp's eth_sendTransaction normally fixes `to`, `value` and `data`, and
-// The nonce, the gas limit and the fees have to be filled in from the network
+// sometimes the gas limit. The nonce, the fees, and the gas limit when the
-// before anything can be signed, and whoever fills them in decides what the
+// site gives none have to be filled in from the network before anything can
-// user is shown. That work used to happen in the popup, after the user had
+// be signed, and whoever fills them in decides what the user is shown. That
-// already approved: the numbers on the approval screen came from the popup and
+// work used to happen in the popup, after the user had already approved: the
-// were compared against nothing, so a compromised popup could display one fee
+// numbers on the approval screen came from the popup and were compared against
-// and sign another, and the ceilings in approvalVerify.js were all that stood
+// nothing, so a compromised popup could display one fee and sign another, and
-// between the user and a fee that hands the validator the balance.
+// the ceilings in approvalVerify.js were all that stood between the user and a
+// fee that hands the validator the balance.
 //
 // So it happens here instead, in the background, before the approval window is
 // opened. The background populates the transaction, shows that object, and
@@ -31,6 +32,7 @@ const {
     VoidSigner,
     accessListify,
     getAddress,
+    getBigInt,
     getBytes,
     hexlify,
     toQuantity,
@@ -47,10 +49,11 @@ const {
 // minutes long, which is not a wait anyone will sit through.
 const POPULATE_TIMEOUT_MS = 20000;
 
-// The request fields taken from the page. Anything else is dropped rather than
+// The request fields taken from the page, plus `gas`, which requestFrom()
-// passed to ethers: the object is page-controlled, and a future ethers that
+// takes as the gas limit. Anything else is dropped rather than passed to
-// learns to carry a new transaction field must not start picking one up out of
+// ethers: the object is page-controlled, and a future ethers that learns to
-// it without this module knowing.
+// carry a new transaction field must not start picking one up out of it
+// without this module knowing.
 //
 // The nonce is not taken from the page; it is always the account's next nonce
 // from the network. A page that chose it could replace one of the user's
@@ -84,6 +87,16 @@ function present(v) {
     return v !== null && v !== undefined && v !== "";
 }
 
+// Whether two quantities from the page are the same number. One that is not a
+// number matches nothing.
+function sameQuantity(a, b) {
+    try {
+        return getBigInt(a) === getBigInt(b);
+    } catch {
+        return false;
+    }
+}
+
 // These strings reach the user through the requesting page, so they are full
 // sentences even when the tail of one came from ethers or from the node.
 function sentence(text) {
@@ -111,6 +124,20 @@ function requestFrom(txParams, from) {
     for (const key of REQUEST_FIELDS) {
         if (present(txParams[key])) request[key] = txParams[key];
     }
+    // JSON-RPC calls the gas limit `gas`, and that is the name sites send.
+    // ethers only reads `gasLimit`: left as `gas` it would be dropped and an
+    // estimate signed in its place.
+    if (present(txParams.gas)) {
+        if (
+            present(request.gasLimit) &&
+            !sameQuantity(request.gasLimit, txParams.gas)
+        ) {
+            throw fail(
+                "The site asked for two different gas limits for this transaction.",
+            );
+        }
+        request.gasLimit = txParams.gas;
+    }
     if (
         present(request.type) &&
         !ALLOWED_TX_TYPES.includes(Number(request.type))

tests/approvalTx.test.js

@@ -9,7 +9,7 @@
 // requesting page or the RPC node can say turns it into an approval that
 // should never have been raised.
 
-const { Network, Wallet } = require("ethers");
+const { Network, Transaction, Wallet, toQuantity } = require("ethers");
 const {
     prepareApprovalTx,
     serializeApprovedTx,
@@ -138,6 +138,43 @@ describe("prepareApprovalTx", () => {
         expect(approved.maxFeePerGas).toBe("0x12a05f200");
     });
 
+    // `gas` is the JSON-RPC name for the gas limit and the one sites send.
+    // The node would estimate 21000; the site's 200000 is what is shown and
+    // what is signed.
+    test("uses the gas limit a site sends as gas", async () => {
+        const approved = await prepareApprovalTx(
+            providerWith(),
+            signer.address,
+            { ...TX_PARAMS, gas: "0x30d40" },
+        );
+        expect(approved.gasLimit).toBe("0x30d40");
+        const signed = Transaction.from(
+            await signer.signTransaction({ ...approved }),
+        );
+        expect(signed.gasLimit).toBe(200000n);
+    });
+
+    test("accepts gas and gasLimit that name the same number", async () => {
+        const approved = await prepareApprovalTx(
+            providerWith(),
+            signer.address,
+            { ...TX_PARAMS, gas: "0x30d40", gasLimit: 200000 },
+        );
+        expect(approved.gasLimit).toBe("0x30d40");
+    });
+
+    test("refuses gas and gasLimit that disagree", async () => {
+        await expect(
+            prepareApprovalTx(providerWith(), signer.address, {
+                ...TX_PARAMS,
+                gas: "0x30d40",
+                gasLimit: "0x5208",
+            }),
+        ).rejects.toThrow(
+            "The site asked for two different gas limits for this transaction.",
+        );
+    });
+
     test("carries an access list the request asked for", async () => {
         const approved = await prepareApprovalTx(
             providerWith(),
@@ -212,6 +249,15 @@ describe("prepareApprovalTx", () => {
         ).rejects.toThrow(/gas limit no network this wallet supports/);
     });
 
+    test("refuses a gas limit the site sent above the ceiling", async () => {
+        await expect(
+            prepareApprovalTx(providerWith(), signer.address, {
+                ...TX_PARAMS,
+                gas: toQuantity(MAX_GAS_LIMIT + 1n),
+            }),
+        ).rejects.toThrow(/gas limit no network this wallet supports/);
+    });
+
     // The combined bound at population: a gas limit and a fee that are each
     // under their own ceiling but multiply to thousands of ETH is refused
     // before the approval window opens, so the user is never shown a

tests/approvalVerify.test.js

@@ -58,7 +58,6 @@ const TX_PARAMS = {
     to: RECIPIENT,
     value: "0x2386f26fc10000",
     data: "0xdeadbeef",
-    gas: "0x5208",
 };
 
 // The values populateTransaction() fills in when the dApp fixed none of them.

tests/typedDataPermit.test.js

@@ -233,26 +233,6 @@ function warningOf(data) {
     return html.slice(0, html.indexOf(">Domain<"));
 }
 
-const NOT_SIGNED = ">Not part of the signature<";
-
-// The Message section alone, without the keys listed after it as not part of
-// the signature.
-function messageOf(data) {
-    const html = formatTypedDataHtml(JSON.stringify(data));
-    const end = html.indexOf(NOT_SIGNED);
-    return html.slice(
-        html.indexOf(">Message<"),
-        end === -1 ? html.length : end,
-    );
-}
-
-// The keys listed as not part of the signature, or "" when there are none.
-function notSignedOf(data) {
-    const html = formatTypedDataHtml(JSON.stringify(data));
-    const start = html.indexOf(NOT_SIGNED);
-    return start === -1 ? "" : html.slice(start);
-}
-
 function request(data) {
     return { method: "eth_signTypedData_v4", typedData: JSON.stringify(data) };
 }
@@ -332,8 +312,7 @@ describe("the warning reads only the fields the signed type declares", () => {
     });
 
     // A Permit whose type names its spender `operator`; the page adds a
-    // `spender` key the type does not declare. The Message section names
+    // `spender` key the type does not declare.
-    // only the operator, and the added key is listed apart as not signed.
     test("an added key is not named as the spender", () => {
         const data = {
             ...PERMIT,
@@ -352,10 +331,6 @@ describe("the warning reads only the fields the signed type declares", () => {
         const warning = warningOf(data);
         expect(warning).toContain(WARNING);
         expect(warning).not.toContain(DECOY);
-        expect(messageOf(data)).toContain(SPENDER);
-        expect(messageOf(data)).not.toContain(DECOY);
-        expect(notSignedOf(data)).toContain("spender:");
-        expect(notSignedOf(data)).toContain(DECOY);
     });
 
     // The permit for a Uniswap v3 position NFT: a `Permit` with no amount
@@ -396,47 +371,6 @@ describe("the warning reads only the fields the signed type declares", () => {
     });
 });
 
-// ethers signs only the declared fields at every level of nesting, so an
-// added key inside a struct is not signed either. Such typed data is not
-// refused; the added key is listed apart, with where it sits in the message.
-describe("the Message section shows only the fields that are signed", () => {
-    const NOTE = "This signature only signs you in";
-
-    test("an added key inside a nested struct is listed as not signed", () => {
-        const data = {
-            ...PERMIT_SINGLE,
-            message: {
-                ...PERMIT_SINGLE.message,
-                details: { ...PERMIT_SINGLE.message.details, note: NOTE },
-            },
-        };
-        expect(messageOf(data)).toContain("expiration");
-        expect(messageOf(data)).not.toContain(NOTE);
-        expect(notSignedOf(data)).toContain("details.note:");
-        expect(notSignedOf(data)).toContain(NOTE);
-        expect(typedDataRefusal(request(data))).toBeNull();
-    });
-
-    test("an added key inside a list item is listed with its index", () => {
-        const [first, second] = PERMIT_BATCH.message.details;
-        const data = {
-            ...PERMIT_BATCH,
-            message: {
-                ...PERMIT_BATCH.message,
-                details: [first, { ...second, note: NOTE }],
-            },
-        };
-        expect(messageOf(data)).not.toContain(NOTE);
-        expect(notSignedOf(data)).toContain("details[1].note:");
-        expect(notSignedOf(data)).toContain(NOTE);
-    });
-
-    test("nothing is listed apart when every key is declared", () => {
-        expect(notSignedOf(PERMIT_SINGLE)).toBe("");
-        expect(notSignedOf(LOGIN)).toBe("");
-    });
-});
-
 describe("the primary type shown is the one ethers signs", () => {
     // A drain stated as a sign-in: the page says Login, the types say
     // PermitSingle, and ethers would sign PermitSingle.