From 969a40e8984c42a4d86ee152cb492d7961ab49ca Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 14 Oct 2026 02:10:38 +0000 Subject: [PATCH] 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 | 4 +++- src/shared/approvalTx.js | 34 ++++++++++++++++++++++++---- tests/approvalTx.test.js | 48 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 80 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index a4505e2..ebcb5e8 100644 --- a/README.md +++ b/README.md @@ -2071,7 +2071,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 diff --git a/src/shared/approvalTx.js b/src/shared/approvalTx.js index 9f7a514..9e64bca 100644 --- a/src/shared/approvalTx.js +++ b/src/shared/approvalTx.js @@ -31,6 +31,7 @@ const { VoidSigner, accessListify, getAddress, + getBigInt, getBytes, hexlify, toQuantity, @@ -47,10 +48,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 -// passed to ethers: the object is page-controlled, and a future ethers that -// learns to carry a new transaction field must not start picking one up out of -// it without this module knowing. +// The request fields taken from the page, plus `gas`, which requestFrom() +// takes as the gas limit. Anything else is dropped rather than passed to +// ethers: the object is page-controlled, and a future ethers that learns to +// 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 +86,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 +123,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)) diff --git a/tests/approvalTx.test.js b/tests/approvalTx.test.js index 2dcc9b4..47955f0 100644 --- a/tests/approvalTx.test.js +++ b/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