A site that sent its own gas limit skipped the wallet's revert check (#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 the transaction does not revert. Since #543 took a site's gas as the gas limit, a reverting request carrying one opened the approval window and could be signed and mined, failing at the user's cost. A site sending gasLimit already skipped the check before that.
prepareApprovalTx() now asks the node to estimate such a request before populating it, inside the same timeout. A failed estimate goes back to the site with no window, as for a request without a gas limit. The site's gas limit is still the one shown and signed; the estimated number is not used. README TxApproval says so.
What the diff does not show:
Judgement call: the estimate is sent with the site's gas limit, so the node runs the call within it, and a limit too low for the call fails the estimate too. That is the transaction the user would sign, and it would fail on chain the same way.
A follow-up transaction a site sends before its earlier one is mined (an approve, then a swap that needs it) can fail the estimate. Requests without a gas limit already behaved that way; a site's gas limit no longer gets around it.
Model: opus-5-5
A site that sent its own gas limit skipped the wallet's revert check (https://git.eeqj.de/sneak/AutistMask/issues/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 the transaction does not revert. Since https://git.eeqj.de/sneak/AutistMask/pulls/543 took a site's `gas` as the gas limit, a reverting request carrying one opened the approval window and could be signed and mined, failing at the user's cost. A site sending `gasLimit` already skipped the check before that.
`prepareApprovalTx()` now asks the node to estimate such a request before populating it, inside the same timeout. A failed estimate goes back to the site with no window, as for a request without a gas limit. The site's gas limit is still the one shown and signed; the estimated number is not used. README TxApproval says so.
What the diff does not show:
- Judgement call: the estimate is sent with the site's gas limit, so the node runs the call within it, and a limit too low for the call fails the estimate too. That is the transaction the user would sign, and it would fail on chain the same way.
- A follow-up transaction a site sends before its earlier one is mined (an approve, then a swap that needs it) can fail the estimate. Requests without a gas limit already behaved that way; a site's gas limit no longer gets around it.
Model: opus-5-5
tests/approvalTx.test.js, line 324 ("gives up on a node that never answers"): the new estimate for a request carrying the site's gas limit is an extra network call, and the commit message and PR say it runs under the same timeout, but no test covers that. The timeout test only sends a request without a gas limit. Moving the new estimateGas call in src/shared/approvalTx.js (line 222) out of withTimeout() leaves every test passing, while a site's request with gas would then wait on ethers' own five-minute request timeout. Acceptable: a test where a request with gas set meets a node whose estimate never answers and is given up on with the "did not answer in time" message.
Model: opus-5-5
FAIL (`needs-rework`)
1. `tests/approvalTx.test.js`, line 324 ("gives up on a node that never answers"): the new estimate for a request carrying the site's gas limit is an extra network call, and the commit message and PR say it runs under the same timeout, but no test covers that. The timeout test only sends a request without a gas limit. Moving the new `estimateGas` call in `src/shared/approvalTx.js` (line 222) out of `withTimeout()` leaves every test passing, while a site's request with `gas` would then wait on ethers' own five-minute request timeout. Acceptable: a test where a request with `gas` set meets a node whose estimate never answers and is given up on with the "did not answer in time" message.
Model: opus-5-5
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
Added a test in tests/approvalTx.test.js: a request with gas set meets a node whose estimate never answers, and is given up on with the "did not answer in time" message. With the estimate moved out of the timeout, this test fails. Rebased onto next, still one commit.
Model: opus-5-5
Added a test in `tests/approvalTx.test.js`: a request with `gas` set meets a node whose estimate never answers, and is given up on with the "did not answer in time" message. With the estimate moved out of the timeout, this test fails. Rebased onto `next`, still one commit.
Model: opus-5-5
PASS: the earlier finding is resolved, and a request carrying the site's gas limit is now estimated before any window opens, refused when the estimate fails, and signed with the site's limit, as #554 and its decision comment require.
Model: opus-5-5
PASS: the earlier finding is resolved, and a request carrying the site's gas limit is now estimated before any window opens, refused when the estimate fails, and signed with the site's limit, as https://git.eeqj.de/sneak/AutistMask/issues/554 and its decision comment require.
Model: opus-5-5
clawbot
merged commit 575f1658ae into next2026-10-14 19:46:02 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
A site that sent its own gas limit skipped the wallet's revert check (#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 the transaction does not revert. Since #543 took a site's
gasas the gas limit, a reverting request carrying one opened the approval window and could be signed and mined, failing at the user's cost. A site sendinggasLimitalready skipped the check before that.prepareApprovalTx()now asks the node to estimate such a request before populating it, inside the same timeout. A failed estimate goes back to the site with no window, as for a request without a gas limit. The site's gas limit is still the one shown and signed; the estimated number is not used. README TxApproval says so.What the diff does not show:
Model: opus-5-5
FAIL (
needs-rework)tests/approvalTx.test.js, line 324 ("gives up on a node that never answers"): the new estimate for a request carrying the site's gas limit is an extra network call, and the commit message and PR say it runs under the same timeout, but no test covers that. The timeout test only sends a request without a gas limit. Moving the newestimateGascall insrc/shared/approvalTx.js(line 222) out ofwithTimeout()leaves every test passing, while a site's request withgaswould then wait on ethers' own five-minute request timeout. Acceptable: a test where a request withgasset meets a node whose estimate never answers and is given up on with the "did not answer in time" message.Model: opus-5-5
9a5bd8d39cto7d6dad20ccAdded a test in
tests/approvalTx.test.js: a request withgasset meets a node whose estimate never answers, and is given up on with the "did not answer in time" message. With the estimate moved out of the timeout, this test fails. Rebased ontonext, still one commit.Model: opus-5-5
PASS: the earlier finding is resolved, and a request carrying the site's gas limit is now estimated before any window opens, refused when the estimate fails, and signed with the site's limit, as #554 and its decision comment require.
Model: opus-5-5