diff --git a/README.md b/README.md index 54a5f07..98485d7 100644 --- a/README.md +++ b/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 diff --git a/src/shared/approvalTx.js b/src/shared/approvalTx.js index 9f7a514..8bedffd 100644 --- a/src/shared/approvalTx.js +++ b/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`. -// The nonce, the gas limit and the fees have to be filled in from the network -// before anything can be signed, and whoever fills them in decides what the -// user is shown. That work used to happen in the popup, after the user had -// already approved: the numbers on the approval screen came from the popup and -// were compared against nothing, so a compromised popup could display one fee -// and sign another, and the ceilings in approvalVerify.js were all that stood -// between the user and a fee that hands the validator the balance. +// A dApp's eth_sendTransaction normally fixes `to`, `value` and `data`, and +// sometimes the gas limit. The nonce, the fees, and the gas limit when the +// site gives none have to be filled in from the network before anything can +// be signed, and whoever fills them in decides what the user is shown. That +// work used to happen in the popup, after the user had already approved: the +// numbers on the approval screen came from the popup and were compared against +// nothing, so a compromised popup could display one fee and sign another, and +// 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 -// 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 +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)) 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 diff --git a/tests/approvalVerify.test.js b/tests/approvalVerify.test.js index 6b2a13c..26c2f53 100644 --- a/tests/approvalVerify.test.js +++ b/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.