harden: bound the total network fee by gasLimit × fee, on both send paths (closes #399)
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
This commit is contained in:
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user