From 33fa25adca2864addd4355de23804363a83433cd Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 21 Sep 2026 21:45:34 +0200 Subject: [PATCH] =?UTF-8?q?harden:=20bound=20the=20total=20network=20fee?= =?UTF-8?q?=20by=20gasLimit=20=C3=97=20fee,=20on=20both=20send=20paths=20(?= =?UTF-8?q?closes=20#399)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two per-field ceilings in approvalVerify.js were checked independently, but the fee a validator is paid is gasLimit × fee per gas: a gas limit and a fee each under their own ceiling still multiply to thousands of ETH, which a gas-consuming contract really collects. assertWithinCeilings now also bounds that product against MAX_TOTAL_FEE (1 ETH), so both callers — populating the dApp transaction and verifying the signed artifact — refuse it with a full sentence naming the fee and the limit. The wallet's own send in confirmTx.js pinned no fee fields, so ethers filled them from the node with no bound; it now populates the transaction and runs the same check before signing, showing the same error in the confirmation screen's reserved errors box so nothing on screen moves. Model: opus-4-8 --- TODO.md | 22 ++++++ src/popup/views/confirmTx.js | 75 ++++++++++++++------- src/shared/approvalVerify.js | 37 ++++++++++- tests/approvalTx.test.js | 18 +++++ tests/approvalVerify.test.js | 114 ++++++++++++++++++++++++++++++++ tests/confirmTxFeeBound.test.js | 100 ++++++++++++++++++++++++++++ 6 files changed, 340 insertions(+), 26 deletions(-) create mode 100644 tests/confirmTxFeeBound.test.js diff --git a/TODO.md b/TODO.md index 72cdb34..5fba454 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,28 @@ but the review is broader than any of them. # Completed Steps +- 2026-09-21: The network fee a transaction can commit is bounded by the product + of the gas limit and the fee per gas, not by each field alone, and the + wallet's own send is bounded the same way + ([#399](https://git.eeqj.de/sneak/AutistMask/issues/399)). The two per-field + ceilings in `src/shared/approvalVerify.js` were checked independently, so a + gas limit and a fee that were each under their own ceiling still multiplied to + thousands of ETH — a fee a gas-consuming contract really collects — while the + comment claimed the ceiling caught exactly that. `assertWithinCeilings` now + also refuses a transaction whose gas limit times its fee per gas + (`maxFeePerGas` for a type-2 transaction, `gasPrice` for a legacy or type-1 + one) exceeds `MAX_TOTAL_FEE`, a new constant of 1 ETH beside the existing + ceilings, so both callers — where the dApp transaction is populated and where + the signed artifact is verified — reject it with a full sentence naming the + fee and the limit. The wallet's own send in `src/popup/views/confirmTx.js` + pinned no fee fields, so ethers filled them from whatever the configured node + answered with nothing bounding them; it now populates the transaction and runs + the same check before signing, showing the same error in the confirmation + screen's reserved errors box so nothing on screen moves. Deliberately out of + scope: comparing a supplied fee against the node's own suggested fee, which + the absolute bound already makes unnecessary for the balance-draining case. 1 + ETH is a plain constant, one line to change; the owner may prefer another + figure. - 2026-09-21: The test recovery phrase no longer survives in a release bundle, and the committed-key guard matches by content ([#351](https://git.eeqj.de/sneak/AutistMask/issues/351)). `DEBUG_MNEMONIC` in diff --git a/src/popup/views/confirmTx.js b/src/popup/views/confirmTx.js index aa4d97b..338cbde 100644 --- a/src/popup/views/confirmTx.js +++ b/src/popup/views/confirmTx.js @@ -30,6 +30,7 @@ const { displayedDecimals, transferAmountUnits, } = require("../../shared/transferAmount"); +const { assertWithinCeilings } = require("../../shared/approvalVerify"); const { CODES, FEE_PENDING, @@ -394,6 +395,46 @@ async function estimateGas(txInfo) { } } +// Populate the transaction this send describes, enforce the fee bound against +// the fees that were actually filled in, then sign and broadcast it. The send +// pins no fee fields, so ethers fills maxFeePerGas and the gas limit from what +// the configured RPC node answers, with nothing otherwise bounding what a +// hostile node can set — the dApp path's ceilings never reached this one. +// Populating before the check is what makes assertWithinCeilings() see the +// same numbers that would be signed; it throws an ApprovalMismatchError when +// the product gasLimit × maxFeePerGas is over the bound, which the caller +// shows in the reserved error area rather than sending. +async function populateVerifyAndSend(connectedSigner, tx) { + let request; + if (tx.token === "ETH") { + request = { to: tx.to, value: parseEther(tx.amount) }; + } else { + const contract = new Contract(tx.token, ERC20_ABI, connectedSigner); + // The contract's decimals() is read to be COMPARED with the scale the + // screen rendered this amount at, not to encode with: encoding from it + // signs whatever the contract answers now, which is not what the user + // read. A disagreement throws. See transferAmount.js. + const amount = transferAmountUnits( + tx.amount, + tx.tokenDecimals, + await contract.decimals(), + ); + request = await contract.transfer.populateTransaction(tx.to, amount); + } + const populated = await connectedSigner.populateTransaction(request); + assertWithinCeilings(populated); + return connectedSigner.sendTransaction(populated); +} + +// Show a full-sentence send failure in the reserved errors box, the same +// element and markup renderValidation() uses for messages carrying the user's +// own numbers, so it never moves anything on the screen. +function showSendError(message) { + const el = $("confirm-errors"); + el.innerHTML = `
${escapeHtml(message)}
`; + el.style.visibility = "visible"; +} + async function checkRecipientHistory(txInfo) { try { const provider = getProvider(state.rpcUrl, state.networkId); @@ -467,29 +508,7 @@ function init(_ctx) { const provider = getProvider(state.rpcUrl, state.networkId); const connectedSigner = signer.connect(provider); - if (pendingTx.token === "ETH") { - tx = await connectedSigner.sendTransaction({ - to: pendingTx.to, - value: parseEther(pendingTx.amount), - }); - } else { - const contract = new Contract( - pendingTx.token, - ERC20_ABI, - connectedSigner, - ); - // The contract's decimals() is read to be COMPARED with the - // scale the screen rendered this amount at, not to encode with: - // encoding from it signs whatever the contract answers now, - // which is not what the user read. A disagreement throws and is - // reported on the error screen. See transferAmount.js. - const amount = transferAmountUnits( - pendingTx.amount, - pendingTx.tokenDecimals, - await contract.decimals(), - ); - tx = await contract.transfer(pendingTx.to, amount); - } + tx = await populateVerifyAndSend(connectedSigner, pendingTx); // Best-effort: clear decrypted secret after use. // Note: JS strings are immutable; this nulls the reference but @@ -498,6 +517,14 @@ function init(_ctx) { txStatus.showWait(pendingTx, tx.hash); } catch (e) { decryptedSecret = null; + // A fee over the bound is refused before anything is broadcast, so + // there is no transaction that may have reached the network to warn + // about: the message stays on the confirmation screen where the + // user can go back, rather than routing to the sent/failed screen. + if (e && e.approvalMismatch) { + showSendError(e.message); + return; + } const hash = tx ? tx.hash : null; txStatus.showError(pendingTx, hash, e.shortMessage || e.message); } finally { @@ -511,4 +538,4 @@ function init(_ctx) { }); } -module.exports = { init, show, restore }; +module.exports = { init, show, restore, populateVerifyAndSend }; diff --git a/src/shared/approvalVerify.js b/src/shared/approvalVerify.js index 18d2878..a642f7e 100644 --- a/src/shared/approvalVerify.js +++ b/src/shared/approvalVerify.js @@ -51,6 +51,7 @@ const { Transaction, accessListify, + formatEther, getAddress, getBytes, verifyMessage, @@ -134,10 +135,19 @@ const FORBIDDEN_FIELDS = [ const MAX_GAS_LIMIT = 100000000n; // 100,000 gwei per gas: orders of magnitude above the highest fee either -// supported network has produced, and low enough to catch a fee that would -// hand the validator the balance. +// supported network has produced. const MAX_FEE_PER_GAS = 100000000000000n; +// The largest total fee this wallet will sign, in wei. The two ceilings above +// bound the gas limit and the price per gas each on its own, but the fee a +// validator is actually paid is their product, and a gas limit and a price +// that are each under their own ceiling still multiply to thousands of ETH — +// 30,000,000 gas at 100,000 gwei is about 3,000 ETH. Bounding the product is +// what catches a fee that would hand the validator the balance; the per-field +// ceilings alone do not. A full 30,000,000-gas block at 33 gwei reaches this, +// which no ordinary wallet transaction approaches. +const MAX_TOTAL_FEE = 1000000000000000000n; // 1 ETH + // A refusal to act on an artifact: it is not the thing that was approved, so // the approval it was offered against is spent and must not be retried. Every // throw in this module is one of these; the background distinguishes them from @@ -384,6 +394,28 @@ function assertWithinCeilings(tx) { ); } } + // The product: gasLimit × the most this transaction could pay per gas — + // maxFeePerGas for a type-2 transaction, gasPrice for a legacy or type-1 + // one. This is the fee a gas-consuming contract can really extract, and it + // is the bound the two per-field ceilings above cannot express. + if (present(tx.gasLimit)) { + const gasLimit = normalizeQuantity(tx.gasLimit, "gas limit"); + let price = null; + if (present(tx.maxFeePerGas)) { + price = normalizeQuantity(tx.maxFeePerGas, "maximum fee per gas"); + } else if (present(tx.gasPrice)) { + price = normalizeQuantity(tx.gasPrice, "gas price"); + } + if (price !== null && gasLimit * price > MAX_TOTAL_FEE) { + throw refuse( + "This transaction would allow a network fee of up to " + + formatEther(gasLimit * price) + + " ETH, which is more than the " + + formatEther(MAX_TOTAL_FEE) + + " ETH this wallet will sign for.", + ); + } + } } // Refuse a field only a transaction type this wallet does not sign can carry. @@ -775,4 +807,5 @@ module.exports = { TX_STAGE_NONCE, MAX_GAS_LIMIT, MAX_FEE_PER_GAS, + MAX_TOTAL_FEE, }; diff --git a/tests/approvalTx.test.js b/tests/approvalTx.test.js index 744a1bd..f943abd 100644 --- a/tests/approvalTx.test.js +++ b/tests/approvalTx.test.js @@ -212,6 +212,24 @@ describe("prepareApprovalTx", () => { ).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 + // balance-draining fee to click past. + test("refuses a fee whose product with the gas limit is over the bound", async () => { + const gouging = providerWith({ + estimateGas: async () => 30000000n, + getFeeData: async () => ({ + gasPrice: MAX_FEE_PER_GAS, + maxFeePerGas: MAX_FEE_PER_GAS, + maxPriorityFeePerGas: 1000000000n, + }), + }); + await expect( + prepareApprovalTx(gouging, signer.address, TX_PARAMS), + ).rejects.toThrow(/network fee of up to/); + }); + // No approval and no window: the failure goes back to the page the click // came from, in a sentence. test("reports a failed estimate as a full sentence", async () => { diff --git a/tests/approvalVerify.test.js b/tests/approvalVerify.test.js index 1763b3d..1e0ba83 100644 --- a/tests/approvalVerify.test.js +++ b/tests/approvalVerify.test.js @@ -28,6 +28,7 @@ const { TX_STAGE_NONCE, MAX_GAS_LIMIT, MAX_FEE_PER_GAS, + MAX_TOTAL_FEE, } = require("../src/shared/approvalVerify"); const { prepareApprovalTx } = require("../src/shared/approvalTx"); const { getSignerForAddress } = require("../src/shared/wallet"); @@ -475,18 +476,131 @@ describe("verifySignedTx field comparison", () => { assertWithinCeilings({ [key]: MAX_FEE_PER_GAS + 1n }), ).toThrow(/fee per gas far above any plausible value/); } + // Each field at its own ceiling multiplies to about 10,000 ETH, which + // is exactly the combination the per-field ceilings cannot see and the + // product bound is for: it is refused, not accepted. expect(() => assertWithinCeilings({ gasLimit: MAX_GAS_LIMIT, maxFeePerGas: MAX_FEE_PER_GAS, maxPriorityFeePerGas: MAX_FEE_PER_GAS, }), + ).toThrow(/network fee of up to/); + // An ordinary transaction — a modest gas limit and a modest fee, each + // far under its ceiling and their product far under the bound — passes. + expect(() => + assertWithinCeilings({ + gasLimit: 21000n, + maxFeePerGas: 2000000000n, + maxPriorityFeePerGas: 1000000000n, + }), ).not.toThrow(); // Nothing to bound is not a failure: a type 2 approval carries no gas // price, and a bare object must not be refused for lacking one. expect(() => assertWithinCeilings({})).not.toThrow(); }); + // The defect this issue closes: gasLimit and maxFeePerGas each under their + // own ceiling, but their product — the fee a gas-consuming contract can + // really extract — thousands of ETH. The per-field ceilings accept it; the + // product bound refuses it, on either side of the screen. + describe("the combined fee bound", () => { + // A gas limit and a fee that are each comfortably under their own + // ceiling but multiply to well over 1 ETH: 30,000,000 gas at 100,000 + // gwei is about 3,000 ETH. + const OVER = { gasLimit: 30000000n, maxFeePerGas: 100000000000000n }; + + test("each field is under its own ceiling", () => { + expect(OVER.gasLimit).toBeLessThan(MAX_GAS_LIMIT); + expect(OVER.maxFeePerGas).toBeLessThanOrEqual(MAX_FEE_PER_GAS); + expect(OVER.gasLimit * OVER.maxFeePerGas).toBeGreaterThan( + MAX_TOTAL_FEE, + ); + }); + + test("assertWithinCeilings refuses the product over the bound", () => { + expect(() => + assertWithinCeilings({ + ...OVER, + maxPriorityFeePerGas: 1000000000n, + }), + ).toThrow(/network fee of up to 3000\.0 ETH/); + }); + + test("assertWithinCeilings bounds a legacy gasPrice the same way", () => { + expect(() => + assertWithinCeilings({ + gasLimit: OVER.gasLimit, + gasPrice: OVER.maxFeePerGas, + }), + ).toThrow(/network fee of up to/); + }); + + // The boundary itself, pinned rather than only some value well past + // it. Both fields stay under their own ceilings, so it is the product + // and nothing else that decides these two cases: a gas limit of 10,000 + // at the per-gas ceiling is exactly 1 ETH. + test("assertWithinCeilings accepts a product exactly at the bound and refuses one wei over", () => { + expect(MAX_FEE_PER_GAS * 10000n).toBe(MAX_TOTAL_FEE); + expect(() => + assertWithinCeilings({ + gasLimit: 10000n, + maxFeePerGas: MAX_FEE_PER_GAS, + }), + ).not.toThrow(); + expect(() => + assertWithinCeilings({ + gasLimit: 10001n, + maxFeePerGas: MAX_FEE_PER_GAS, + }), + ).toThrow(/network fee of up to/); + }); + + // The dApp path: an artifact whose fee is within each field's ceiling + // but over the product bound, both displayed and signed, is refused at + // verification just as it is at population. + test("verifySignedTx refuses an over-bound product even when displayed", async () => { + const raw = await signedWith(OVER); + expect(() => + verifySignedTx( + raw, + approvedFor(TX_PARAMS, OVER), + signer.address, + SELECTED, + ), + ).toThrow(/network fee of up to/); + }); + + test("verifySignedTx accepts a product just under the bound", async () => { + // 21,000 gas at 40 gwei is 0.00084 ETH — an ordinary send. + const under = { gasLimit: 21000n, maxFeePerGas: 40000000000n }; + expect(under.gasLimit * under.maxFeePerGas).toBeLessThan( + MAX_TOTAL_FEE, + ); + const raw = await signedWith(under); + expect(() => + verifySignedTx( + raw, + approvedFor(TX_PARAMS, under), + signer.address, + SELECTED, + ), + ).not.toThrow(); + }); + + test("the refusal names the fee and the limit in a full sentence", () => { + try { + assertWithinCeilings(OVER); + throw new Error("expected a rejection"); + } catch (e) { + expect(e.approvalMismatch).toBe(true); + expect(e.message).toMatch(/^[A-Z].*\.$/); + expect(e.message).toContain("3000.0 ETH"); + expect(e.message).toContain("1.0 ETH"); + } + }); + }); + test("every field mismatch is a refusal, not a warning", async () => { const raw = await signedWith({ nonce: 8 }); try { diff --git a/tests/confirmTxFeeBound.test.js b/tests/confirmTxFeeBound.test.js new file mode 100644 index 0000000..7385930 --- /dev/null +++ b/tests/confirmTxFeeBound.test.js @@ -0,0 +1,100 @@ +// The wallet's OWN send path enforces the same combined fee bound the dApp +// path does (https://git.eeqj.de/sneak/AutistMask/issues/399). +// +// The send in src/popup/views/confirmTx.js pins no fee fields, so ethers fills +// maxFeePerGas and the gas limit from whatever the configured RPC node +// answers. Nothing bounded that: a hostile node could report a fee whose +// product with the gas limit is thousands of ETH, and it would be both +// displayed and signed. populateVerifyAndSend() populates the transaction and +// runs assertWithinCeilings() on the populated fees before signing, so an +// over-bound send is refused before anything is broadcast. +// +// The check is driven here with a fake connected signer rather than a real +// one: populateTransaction() returns the fees the node would have produced, +// and sendTransaction() records whether the send actually happened. The real +// DOM path around it — reading the fee error into the reserved errors box — is +// covered by the Chrome e2e suite. + +globalThis.chrome = { + storage: { local: { get: async () => ({}), set: async () => {} } }, +}; + +global.fetch = jest.fn(() => { + throw new Error("tests must not perform network requests"); +}); + +const { populateVerifyAndSend } = require("../src/popup/views/confirmTx"); +const { + MAX_FEE_PER_GAS, + MAX_TOTAL_FEE, +} = require("../src/shared/approvalVerify"); + +const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; + +// A signer whose populateTransaction() fills in the fees a node quoted and +// whose sendTransaction() records the call, so a test can assert whether the +// send was reached at all. +function fakeSigner(fees) { + const sent = []; + return { + sent, + populateTransaction: async (request) => ({ + ...request, + from: RECIPIENT, + nonce: 0, + type: 2, + chainId: 1n, + gasLimit: fees.gasLimit, + maxFeePerGas: fees.maxFeePerGas, + maxPriorityFeePerGas: 1000000000n, + }), + sendTransaction: async (tx) => { + sent.push(tx); + return { hash: "0xabc" }; + }, + }; +} + +const ETH_SEND = { token: "ETH", to: RECIPIENT, amount: "1.0" }; + +describe("populateVerifyAndSend enforces the combined fee bound", () => { + // A gas limit and a fee that are each under their own field ceiling, but + // multiply to about 3,000 ETH — the combination the per-field ceilings + // cannot see. + const OVER = { gasLimit: 30000000n, maxFeePerGas: MAX_FEE_PER_GAS }; + + test("each field is under its ceiling but the product is over the bound", () => { + expect(OVER.maxFeePerGas).toBeLessThanOrEqual(MAX_FEE_PER_GAS); + expect(OVER.gasLimit * OVER.maxFeePerGas).toBeGreaterThan( + MAX_TOTAL_FEE, + ); + }); + + test("refuses an over-bound send without broadcasting it", async () => { + const signer = fakeSigner(OVER); + let thrown; + try { + await populateVerifyAndSend(signer, ETH_SEND); + } catch (e) { + thrown = e; + } + expect(thrown).toBeDefined(); + expect(thrown.approvalMismatch).toBe(true); + expect(thrown.message).toMatch(/^[A-Z].*\.$/); + expect(thrown.message).toContain("3000.0 ETH"); + expect(thrown.message).toContain("1.0 ETH"); + // The one guarantee that matters: nothing was signed or sent. + expect(signer.sent).toHaveLength(0); + }); + + test("broadcasts a send whose product is just under the bound", async () => { + // 21,000 gas at 40 gwei is 0.00084 ETH — an ordinary send. + const under = { gasLimit: 21000n, maxFeePerGas: 40000000000n }; + expect(under.gasLimit * under.maxFeePerGas).toBeLessThan(MAX_TOTAL_FEE); + const signer = fakeSigner(under); + const tx = await populateVerifyAndSend(signer, ETH_SEND); + expect(tx.hash).toBe("0xabc"); + expect(signer.sent).toHaveLength(1); + expect(signer.sent[0].gasLimit).toBe(under.gasLimit); + }); +});