security: the transaction fee backstop is per-field, so the fee can reach thousands of ETH, and the wallet's own send path has no backstop at all #399

New Issue

2026-09-21T20:37:56+02:00

clawbot commented

2026-09-21 20:37:56 +02:00

Severity: funds-or-keys-at-risk.

Where. src/shared/approvalVerify.js:134-139 (MAX_GAS_LIMIT 100,000,000 and
MAX_FEE_PER_GAS 100,000 gwei) and 370-387 (assertWithinCeilings checks each
alone); src/shared/approvalTx.js:54-66 (the page supplies gasLimit and the fee
fields); src/popup/views/confirmTx.js:315-345, 462-476 (the wallet's own send
populates fees from the RPC with no ceiling).

What is wrong. The two ceilings are checked independently, but the fee a
validator can take is gasLimit × effective price. gasLimit 30,000,000 with
maxFeePerGas and maxPriorityFeePerGas both at 100,000 gwei — each under its
own ceiling — is about 3,000 ETH, and the comment at approvalVerify.js:121-133
says the ceiling exists "to catch a fee that would hand the validator the
balance", which it does not. Against a contract that consumes the gas, that fee
is really paid. Separately, the wallet's OWN send (confirmTx.js) calls
sendTransaction / transfer with no fee overrides, so ethers populates the
fees from getFeeData with no ceiling applied at all: a hostile configured RPC
can set an arbitrary fee that is both displayed and signed.

Why it matters. The fee line is shown, but a user who reads the value and
not the fee approves a balance-draining fee; the dApp path's stated backstop
fails at its own purpose, and the wallet's own send path has no backstop.

Reproduction. A connected page sends eth_sendTransaction to a
gas-consuming contract with gasLimit 0x1c9c380 and
maxPriorityFeePerGas/maxFeePerGas 0x5af3107a4000; the approval opens with a
~3,000 ETH max fee. For the own path: a configured RPC returning a large
getFeeData produces the same on an ordinary send.

Acceptable. Bound the product gasLimit × maxFeePerGas (an absolute cap, or
a small multiple of the value) rather than each field alone; apply the same
bound to the wallet's own send path; and refuse a page- or node-supplied fee far
above the node's suggested value.

Definition of done.

A combined fee bound (the product, not each field) is enforced where the
dApp transaction is populated and where the artifact is verified.
The wallet's own send path enforces the same bound before signing.
A transaction whose gasLimit × fee exceeds the bound is refused with a
full-sentence error, on both paths.
Tests drive both paths with a fee that is within each field's ceiling but
over the product bound and assert refusal.

Related: #207

Model: fable-5-1 (review); opus-4-8 (filing)

Severity: funds-or-keys-at-risk. **Where.** `src/shared/approvalVerify.js:134-139` (`MAX_GAS_LIMIT` 100,000,000 and `MAX_FEE_PER_GAS` 100,000 gwei) and `370-387` (`assertWithinCeilings` checks each alone); `src/shared/approvalTx.js:54-66` (the page supplies `gasLimit` and the fee fields); `src/popup/views/confirmTx.js:315-345, 462-476` (the wallet's own send populates fees from the RPC with no ceiling). **What is wrong.** The two ceilings are checked independently, but the fee a validator can take is `gasLimit × effective price`. `gasLimit` 30,000,000 with `maxFeePerGas` and `maxPriorityFeePerGas` both at 100,000 gwei — each under its own ceiling — is about 3,000 ETH, and the comment at `approvalVerify.js:121-133` says the ceiling exists "to catch a fee that would hand the validator the balance", which it does not. Against a contract that consumes the gas, that fee is really paid. Separately, the wallet's OWN send (`confirmTx.js`) calls `sendTransaction` / `transfer` with no fee overrides, so ethers populates the fees from `getFeeData` with no ceiling applied at all: a hostile configured RPC can set an arbitrary fee that is both displayed and signed. **Why it matters.** The fee line is shown, but a user who reads the value and not the fee approves a balance-draining fee; the dApp path's stated backstop fails at its own purpose, and the wallet's own send path has no backstop. **Reproduction.** A connected page sends `eth_sendTransaction` to a gas-consuming contract with `gasLimit` `0x1c9c380` and `maxPriorityFeePerGas`/`maxFeePerGas` `0x5af3107a4000`; the approval opens with a ~3,000 ETH max fee. For the own path: a configured RPC returning a large `getFeeData` produces the same on an ordinary send. **Acceptable.** Bound the product `gasLimit × maxFeePerGas` (an absolute cap, or a small multiple of the value) rather than each field alone; apply the same bound to the wallet's own send path; and refuse a page- or node-supplied fee far above the node's suggested value. **Definition of done.** - [ ] A combined fee bound (the product, not each field) is enforced where the dApp transaction is populated and where the artifact is verified. - [ ] The wallet's own send path enforces the same bound before signing. - [ ] A transaction whose `gasLimit × fee` exceeds the bound is refused with a full-sentence error, on both paths. - [ ] Tests drive both paths with a fee that is within each field's ceiling but over the product bound and assert refusal. Related: https://git.eeqj.de/sneak/AutistMask/issues/207 Model: fable-5-1 (review); opus-4-8 (filing)

clawbot added this to the 1.0.0 milestone 2026-09-21 20:37:56 +02:00

clawbot referenced this issue

2026-09-21 20:39:01 +02:00

security: pre-1.0 review of key handling, DEBUG mode and RPC/dApp input validation #383

clawbot commented

2026-09-21 21:00:54 +02:00

Plan for the implementer.

One named constant in src/shared/approvalVerify.js next to the existing ceilings: the largest total fee the wallet will sign, gasLimit × maxFeePerGas (or gasLimit × gasPrice for type 0/1), set to 1 ETH in wei. The two per-field ceilings stay as they are.
assertWithinCeilings checks the product as well as each field, so both places that already call it (where the dApp transaction is populated, and where the signed artifact is verified) refuse an over-bound transaction with a full-sentence error that names the fee and the limit.
The wallet's own send in src/popup/views/confirmTx.js populates the transaction first, applies the same check before signing, and shows the same full-sentence error in the existing error area (no layout shift).
Tests: on each path, a fee within each field ceiling but over 1 ETH in product is refused; a fee just under it is accepted.
Out of scope here: comparing a supplied fee against the node's suggested fee. The absolute bound already closes the balance-draining case; the comparison can be its own issue if wanted.

Judgement call: 1 ETH is my choice of bound, as a plain constant that is one line to change. A full 30M-gas block at 33 gwei reaches it, which no ordinary wallet transaction approaches. The owner may prefer another figure; nothing waits on that.

Model: fable-5-1

Plan for the implementer. - One named constant in `src/shared/approvalVerify.js` next to the existing ceilings: the largest total fee the wallet will sign, `gasLimit × maxFeePerGas` (or `gasLimit × gasPrice` for type 0/1), set to 1 ETH in wei. The two per-field ceilings stay as they are. - `assertWithinCeilings` checks the product as well as each field, so both places that already call it (where the dApp transaction is populated, and where the signed artifact is verified) refuse an over-bound transaction with a full-sentence error that names the fee and the limit. - The wallet's own send in `src/popup/views/confirmTx.js` populates the transaction first, applies the same check before signing, and shows the same full-sentence error in the existing error area (no layout shift). - Tests: on each path, a fee within each field ceiling but over 1 ETH in product is refused; a fee just under it is accepted. - Out of scope here: comparing a supplied fee against the node's suggested fee. The absolute bound already closes the balance-draining case; the comparison can be its own issue if wanted. Judgement call: 1 ETH is my choice of bound, as a plain constant that is one line to change. A full 30M-gas block at 33 gwei reaches it, which no ordinary wallet transaction approaches. The owner may prefer another figure; nothing waits on that. Model: fable-5-1

clawbot referenced this issue from a commit

2026-09-21 21:13:10 +02:00

harden: bound the total network fee by gasLimit × fee, on both send paths (closes #399)

clawbot referenced a pull request that will close this issue

2026-09-21 21:13:33 +02:00

harden: bound the total network fee by gasLimit × fee, on both send paths (closes #399) #411

clawbot commented

2026-09-21 21:13:55 +02:00

Implemented in #411.

assertWithinCeilings in src/shared/approvalVerify.js now bounds the product gas limit × fee per gas (maxFeePerGas for a type-2 transaction, gasPrice for a legacy or type-1 one) against a new MAX_TOTAL_FEE constant of 1 ETH, added beside the two existing per-field ceilings, which are unchanged. Both callers — populating the dApp transaction and verifying the signed artifact — refuse an over-bound transaction with a full sentence naming the fee and the limit.

The wallet's own send in src/popup/views/confirmTx.js now populates the transaction and runs the same check before signing, showing the same error in the confirmation screen's existing reserved errors box (no layout shift).

Tests drive both paths with a fee within each field's ceiling but over the product bound (refused) and just under it (accepted).

Out of scope, per the plan: comparing a supplied fee against the node's own suggested fee.

Judgement call: 1 ETH is my bound, a plain one-line constant; the owner may prefer another figure and nothing waits on that.

Model: opus-4-8

Implemented in https://git.eeqj.de/sneak/AutistMask/pulls/411. `assertWithinCeilings` in `src/shared/approvalVerify.js` now bounds the product gas limit × fee per gas (`maxFeePerGas` for a type-2 transaction, `gasPrice` for a legacy or type-1 one) against a new `MAX_TOTAL_FEE` constant of 1 ETH, added beside the two existing per-field ceilings, which are unchanged. Both callers — populating the dApp transaction and verifying the signed artifact — refuse an over-bound transaction with a full sentence naming the fee and the limit. The wallet's own send in `src/popup/views/confirmTx.js` now populates the transaction and runs the same check before signing, showing the same error in the confirmation screen's existing reserved errors box (no layout shift). Tests drive both paths with a fee within each field's ceiling but over the product bound (refused) and just under it (accepted). Out of scope, per the plan: comparing a supplied fee against the node's own suggested fee. Judgement call: 1 ETH is my bound, a plain one-line constant; the owner may prefer another figure and nothing waits on that. Model: opus-4-8

clawbot closed this issue

2026-09-21 21:45:35 +02:00

clawbot referenced this issue from a commit

2026-09-21 21:45:35 +02:00

harden: bound the total network fee by gasLimit × fee, on both send paths (closes #399)

clawbot referenced this issue

2026-09-21 21:46:03 +02:00

milestone 1.0.0 release readiness: user-facing fixes, security review, release procedure #388

Sign in to join this conversation.