From 7d6dad20cc7359d501b44b1d213279eff0cc0f08 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 14 Oct 2026 11:57:47 +0000 Subject: [PATCH] fix: estimate a request even when the site sends its gas limit (closes #554) ethers asks the node for a gas estimate only when the request has no gas limit, and that estimate is the wallet's only check that a transaction does not revert. Since a site's `gas` became the gas limit, a reverting request carrying one opened the approval window and could be signed and mined to fail. Such a request is now estimated first, with the site's limit and under the same timeout; a failure goes back to the site with no window, and the site's gas limit is still the one shown and signed. A limit too low for the call now fails the same way. Model: opus-5-5 --- README.md | 6 +++-- src/shared/approvalTx.js | 17 +++++++++++--- tests/approvalTx.test.js | 50 +++++++++++++++++++++++++++++++++++++--- 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 1c61e41..fc4de61 100644 --- a/README.md +++ b/README.md @@ -2100,8 +2100,10 @@ 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 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 + behind a gap. A gas limit the site supplies, as `gas` or `gasLimit`, is the + one shown and signed in place of the node's estimate, but the node still + estimates the transaction with that limit, as the check that it neither + reverts nor runs out of gas; a request that gives 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 diff --git a/src/shared/approvalTx.js b/src/shared/approvalTx.js index 8bedffd..eb088ce 100644 --- a/src/shared/approvalTx.js +++ b/src/shared/approvalTx.js @@ -201,6 +201,7 @@ async function prepareApprovalTx(provider, from, txParams) { throw fail("There is no active address to send this transaction from."); } const request = requestFrom(txParams || {}, from); + const signer = new VoidSigner(getAddress(from), provider); let populated; try { @@ -208,10 +209,20 @@ async function prepareApprovalTx(provider, from, txParams) { // nonce, gas, fee and chain id are populated exactly as they were when // the popup did this. VoidSigner cannot sign, which is the point: the // background prepares, the popup signs. + // + // ethers asks the node for a gas estimate only when the request has + // no gas limit, and that estimate is the only check that the + // transaction does not revert. So a request carrying the site's gas + // limit is estimated first, with that limit: one that reverts, or + // needs more gas than the site gave it, fails here as well. The + // site's gas limit is still the one populated. populated = await withTimeout( - new VoidSigner(getAddress(from), provider).populateTransaction( - request, - ), + (async () => { + if (present(request.gasLimit)) { + await signer.estimateGas(request); + } + return signer.populateTransaction(request); + })(), POPULATE_TIMEOUT_MS, "The transaction could not be prepared: the network did not answer in time.", ); diff --git a/tests/approvalTx.test.js b/tests/approvalTx.test.js index 47955f0..b9ddacb 100644 --- a/tests/approvalTx.test.js +++ b/tests/approvalTx.test.js @@ -139,14 +139,20 @@ describe("prepareApprovalTx", () => { }); // `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. + // The node is still asked for its estimate, with the site's gas limit, + // because the estimate is the check that the transaction does not revert. + // It estimates 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 estimateGas = jest.fn(async () => 21000n); const approved = await prepareApprovalTx( - providerWith(), + providerWith({ estimateGas }), signer.address, { ...TX_PARAMS, gas: "0x30d40" }, ); + expect(estimateGas).toHaveBeenCalledWith( + expect.objectContaining({ gasLimit: 200000n }), + ); expect(approved.gasLimit).toBe("0x30d40"); const signed = Transaction.from( await signer.signTransaction({ ...approved }), @@ -297,6 +303,22 @@ describe("prepareApprovalTx", () => { expect(thrown.message).toMatch(/^[A-Z].*\.$/); }); + test("fails back a reverting request that sends its own gas limit", async () => { + const reverting = providerWith({ + estimateGas: async () => { + throw new Error("execution reverted: ERC20: transfer amount"); + }, + }); + await expect( + prepareApprovalTx(reverting, signer.address, { + ...TX_PARAMS, + gas: "0x30d40", + }), + ).rejects.toThrow( + /^The transaction could not be prepared: execution reverted/, + ); + }); + // Without a bound, an unreachable node leaves the page's promise pending // with nothing on screen to explain it. test("gives up on a node that never answers", async () => { @@ -319,6 +341,28 @@ describe("prepareApprovalTx", () => { jest.useRealTimers(); } }); + + // The estimate of a request carrying the site's gas limit is a call of its + // own, made before populating, and is bounded the same way. + test("gives up on a node that never answers the estimate of a request that sends its own gas limit", async () => { + jest.useFakeTimers(); + try { + const hanging = providerWith({ + estimateGas: () => new Promise(() => {}), + }); + const pending = prepareApprovalTx(hanging, signer.address, { + ...TX_PARAMS, + gas: "0x30d40", + }); + const settled = expect(pending).rejects.toThrow( + /did not answer in time/, + ); + await jest.advanceTimersByTimeAsync(POPULATE_TIMEOUT_MS + 1); + await settled; + } finally { + jest.useRealTimers(); + } + }); }); describe("serializeApprovedTx", () => { -- 2.54.0