fix: sign the ERC-20 amount the send screen displayed (closes #305) #314

Merged

clawbot merged 1 commits from issue-305-erc20-decimals into next 2026-08-20 12:31:28 +02:00

Conversation 1 Commits 1 Files Changed 8 +570 -16

clawbot commented 2026-08-20 12:15:57 +02:00

Collaborator

Copy Link

Closes #305.

What changed

• src/shared/transferAmount.js (new): transferAmountUnits(amount, displayedDecimals, contractDecimals) returns base units only when the two scales agree, and otherwise throws a full sentence naming both. An absent or unusable displayed value is a refusal too. This is the confirmTx counterpart to approvalVerify.js and takes the same stance: a quantity that cannot be compared with what was displayed has not been checked. Decimal types are enumerated rather than coerced — Number([]) is 0, and a coercing check would admit an empty array as a scale of zero.
• src/popup/views/send.js: carries tokenDecimals on the pending transaction, from the same tokenBalances entry the screen's amount, balance and symbol come from.
• src/popup/views/confirmTx.js: encodes from that carried value in both places it encodes. The gas estimate no longer reads decimals() at all, so it estimates the transfer that would actually be signed; the signing path reads the contract's decimals() purely to compare, and refuses on a disagreement rather than preferring either value.
• README.md: the new ConfirmTx transition.
• TODO.md: Completed Steps entry.

Tests

• tests/transferAmount.test.js: agreement, disagreement in both directions, absent displayed scale, non-integer / out-of-uint8 / wrong-type values from either side, an amount finer than the token's scale, and that every refusal is a full sentence.
• tests/e2e/run.js: the suite had never clicked #btn-confirm-send, so the popup's own Send → ConfirmTx → Sign & Send → WaitTx path had no coverage at all — which is how this shipped. Two new cases:
  • the path driven to a broadcast, with transfer() hand-decoded out of the raw signed bytes (not through an ethers Interface built from the extension's own ABI) and its amount asserted against the amount read off #confirm-amount;
  • the same path with the fixture's decimals() flipped to 18 after the screen was built and its estimate taken, asserting the error screen names both scales and that broadcastTransactions did not grow.
• tests/e2e/network.js: opts.tokenDecimalsOverride (the lying contract, read at request time) and opts.seedReceipt (so the wait screen resolves to the success view rather than polling for the rest of the run). eth_getTransactionReceipt moved out of the constant table into its own branch.

Verification

make check green on the rebased branch at 8fadc41:

Test Suites: 32 passed, 32 total
Tests:       758 passed, 758 total
test-verify-build: 18 case(s) passed
All matched files use Prettier code style!

make test-e2e green, 54/54, run in the pinned container. The two new cases and their probe lines:

# erc-20 send artifact: displayed="0.25 E2E" rawAmount=250000 asIf6Decimals=0.25 asIf18Decimals=0.00000000000025
ok 42 - the popup's own ERC-20 send broadcasts the amount it displayed (#305)
# erc-20 decimals refusal: displayed="0.25 E2E" contract=18 message="The transfer was not sent. The token contract reports 18 decimal places, but the amount was displayed using 6, so signing it would move a different amount than the one shown. Reopen the wallet to reload this token's details and try again."
ok 43 - a token that lies about decimals() at signing time broadcasts nothing (#305)

Mutation check, so the new coverage is not vacuous: with only the signing-side comparison reverted to parseUnits(pendingTx.amount, await contract.decimals()), the suite goes to 53/54 with not ok 43 and everything else green. Case 42 stays green under that mutation by design — the fixture's two scales agree, so it covers the path, and case 43 makes the discrimination.

The Firefox suite was not run: it has no ConfirmTx coverage and nothing here touches its fixtures.

Closes [#305](https://git.eeqj.de/sneak/AutistMask/issues/305). ## What changed - **`src/shared/transferAmount.js`** (new): `transferAmountUnits(amount, displayedDecimals, contractDecimals)` returns base units only when the two scales agree, and otherwise throws a full sentence naming both. An absent or unusable displayed value is a refusal too. This is the `confirmTx` counterpart to `approvalVerify.js` and takes the same stance: a quantity that cannot be compared with what was displayed has not been checked. Decimal types are enumerated rather than coerced — `Number([])` is `0`, and a coercing check would admit an empty array as a scale of zero. - **`src/popup/views/send.js`**: carries `tokenDecimals` on the pending transaction, from the same `tokenBalances` entry the screen's amount, balance and symbol come from. - **`src/popup/views/confirmTx.js`**: encodes from that carried value in both places it encodes. The gas estimate no longer reads `decimals()` at all, so it estimates the transfer that would actually be signed; the signing path reads the contract's `decimals()` purely to compare, and refuses on a disagreement rather than preferring either value. - **`README.md`**: the new ConfirmTx transition. - **`TODO.md`**: Completed Steps entry. ## Tests - `tests/transferAmount.test.js`: agreement, disagreement in both directions, absent displayed scale, non-integer / out-of-uint8 / wrong-type values from either side, an amount finer than the token's scale, and that every refusal is a full sentence. - `tests/e2e/run.js`: the suite had never clicked `#btn-confirm-send`, so the popup's own Send → ConfirmTx → Sign & Send → WaitTx path had no coverage at all — which is how this shipped. Two new cases: - the path driven to a broadcast, with `transfer()` hand-decoded out of the raw signed bytes (not through an ethers `Interface` built from the extension's own ABI) and its amount asserted against the amount read off `#confirm-amount`; - the same path with the fixture's `decimals()` flipped to 18 *after* the screen was built and its estimate taken, asserting the error screen names both scales and that `broadcastTransactions` did not grow. - `tests/e2e/network.js`: `opts.tokenDecimalsOverride` (the lying contract, read at request time) and `opts.seedReceipt` (so the wait screen resolves to the success view rather than polling for the rest of the run). `eth_getTransactionReceipt` moved out of the constant table into its own branch. ## Verification `make check` green on the rebased branch at `8fadc41`: ``` Test Suites: 32 passed, 32 total Tests: 758 passed, 758 total test-verify-build: 18 case(s) passed All matched files use Prettier code style! ``` `make test-e2e` green, 54/54, run in the pinned container. The two new cases and their probe lines: ``` # erc-20 send artifact: displayed="0.25 E2E" rawAmount=250000 asIf6Decimals=0.25 asIf18Decimals=0.00000000000025 ok 42 - the popup's own ERC-20 send broadcasts the amount it displayed (#305) # erc-20 decimals refusal: displayed="0.25 E2E" contract=18 message="The transfer was not sent. The token contract reports 18 decimal places, but the amount was displayed using 6, so signing it would move a different amount than the one shown. Reopen the wallet to reload this token's details and try again." ok 43 - a token that lies about decimals() at signing time broadcasts nothing (#305) ``` Mutation check, so the new coverage is not vacuous: with only the signing-side comparison reverted to `parseUnits(pendingTx.amount, await contract.decimals())`, the suite goes to 53/54 with `not ok 43` and everything else green. Case 42 stays green under that mutation by design — the fixture's two scales agree, so it covers the path, and case 43 makes the discrimination. The Firefox suite was not run: it has no ConfirmTx coverage and nothing here touches its fixtures.

clawbot added 1 commit 2026-08-20 12:15:58 +02:00

fix: sign the ERC-20 amount the send screen displayed (closes #305) ... 8fadc4107f

The wallet's own Send screen renders the amount, the balance and the symbol
from the block explorer's cached decimals, but confirmTx encoded the transfer
from decimals() read off the contract at signing time and nothing compared the
two. A token whose on-chain scale disagrees with the cached one -- an
upgradeable or proxy token, a caller-dependent one, a stale or wrong explorer
entry, a compromised Blockscout -- therefore signed an amount that was never
displayed, off by a power of ten for every decimal place of disagreement. The
reproduction on the issue approves 0.25 and signs 250,000,000,000.

The scale is now carried forward on the pending transaction, taken from the
same tokenBalances entry the screen's own numbers come from, and the contract's
decimals() is read at signing time only to be compared with it. A disagreement
is a refusal that names both numbers, never a preference for either: both
candidate transfers move an amount nobody approved. New
src/shared/transferAmount.js holds that check, as the confirmTx counterpart to
approvalVerify.js, and takes the same stance on an absent or unusable value --
a quantity that cannot be compared with what was displayed has not been
checked. The gas estimate encodes from the same carried value and no longer
reads decimals() at all, so the estimate is for the transfer that would be
signed.

Nothing in the e2e suite had ever clicked #btn-confirm-send, so the popup's own
Send -> ConfirmTx -> Sign & Send -> WaitTx path had no coverage at all, which is
how this shipped. It is now driven end to end to a broadcast, with the
transfer() amount hand-decoded out of the raw signed bytes and asserted against
the amount read off the confirmation screen, plus a case where the fixture's
decimals() starts answering 18 after the screen was built and nothing reaches
eth_sendRawTransaction. The fixture gains that override and a receipt, so the
wait screen resolves to the success view instead of polling for the rest of the
run.

clawbot added the needs-review label 2026-08-20 12:16:05 +02:00

clawbot self-assigned this 2026-08-20 12:16:07 +02:00

clawbot referenced this pull request 2026-08-20 12:16:16 +02:00

fix: the popup's own ERC-20 send signs an amount it never displayed (indexer decimals vs contract decimals) #305

clawbot referenced this pull request 2026-08-20 12:17:13 +02:00

test: WaitTx's timeout and consecutive-failed-lookup exit branches have no e2e coverage #315

clawbot commented 2026-08-20 12:30:55 +02:00

Author

Collaborator

Copy Link

PASS. Independently verified in a fresh clone at 8fadc41: make check green (32 suites / 758 tests, test-verify-build 18 cases, lint stage executed in-container in 5.8s, not CACHED), make test-e2e green 54/54 with the same probe lines, CI green on the head commit, base next, mergeable with next, one commit titled (closes #305) carrying the TODO.md update, no attribution trailers.

Mutation results, both performed here rather than taken on trust:

• signing-side comparison reverted to parseUnits(pendingTx.amount, await contract.decimals()): 53/54, not ok 43 and nothing else. Confirms the author's claim, including that case 42 stays green under it.
• carried scale mutated at the source (send.js handing confirmTx a scale other than the tokenBalances entry's): not ok 42 and not ok 43. The other way this can break is caught too.

Coverage limit, stated because no test establishes it: the fixture's explorer and contract scales agree, so nothing in the suite would catch a carried value that came from a second lookup which merely happens to agree. That the value is the same one the screen rendered from was established by inspection instead — send.js reads tb.decimals off the same tb object it reads tb.balance from, tb.balance is formatted with tb.decimals in fetchTokenBalances(), and addr.tokenBalances is written by nothing else. ERC20_ABI has exactly two encode sites, both now on the carried value; approval.js only decodes.

Non-blocking, worth a follow-up rather than a rework:

1. tests/e2e/network.js, ethCallResult(): word(opts.tokenDecimalsOverride || STUB_TOKEN.decimals) ignores an override of 0 because it is falsy, so a future test making the contract claim zero decimals — a real lying-token case — would pass vacuously against the fixture's own 6. Acceptable looks like an explicit == null check. The JSDoc also types the override {string} where it is a scale.
2. The two new cases restore neither env.routeOpts nor the popup's screen when they fail: seedReceipt = false and tokenDecimalsOverride = null sit after awaits that can throw. Observed in mutation 2 above — two genuine failures cascaded into seven unrelated ones (48 through 54, "the extension opened no approval window"). Given the suite's known load sensitivity (#287) a single flake here reads as an eight-failure run. A finally restoring both flags and returning to the address screen would contain it.
3. Under the reverted-comparison mutation, case 43 fails as a 60s #view-error-tx timeout, so its stronger assertion (nothing reached eth_sendRawTransaction) never executes on the failing path. Asserting the broadcast count before waiting on the screen would make the failure both faster and more precise.
4. mismatchMessage is exported from src/shared/transferAmount.js and used nowhere outside it.
5. transferAmountUnits("-1", 6, 6n) returns -1000000n. Unreachable — send.js and validateTransfer() both refuse a negative amount and ethers will not ABI-encode a negative uint256 — so no funds risk, but the module is the last gate before encoding.

Probed and clean: absent, null, undefined, 0, "0", "", "007", "255", "256", "6.0", "0x6", "1e1", "+6", "-0", leading/trailing whitespace, NaN, Infinity, floats, negatives, -1n, 2n**70n, true, {}, [], [6], new Number(6), and an object with a valueOf — every one refuses rather than defaulting, and the Number([]) === 0 claim holds because arrays never reach a numeric branch. Refusals reach the user as full sentences on ErrorTx, not as an unhandled throw; a missing carried scale also fails the estimate, and FEE_UNAVAILABLE keeps Send disabled.

PASS. Independently verified in a fresh clone at `8fadc41`: `make check` green (32 suites / 758 tests, `test-verify-build` 18 cases, lint stage executed in-container in 5.8s, not `CACHED`), `make test-e2e` green 54/54 with the same probe lines, CI green on the head commit, base `next`, mergeable with `next`, one commit titled ` (closes #305)` carrying the `TODO.md` update, no attribution trailers. Mutation results, both performed here rather than taken on trust: - signing-side comparison reverted to `parseUnits(pendingTx.amount, await contract.decimals())`: 53/54, `not ok 43` and nothing else. Confirms the author's claim, including that case 42 stays green under it. - carried scale mutated at the source (`send.js` handing `confirmTx` a scale other than the `tokenBalances` entry's): `not ok 42` and `not ok 43`. The other way this can break is caught too. Coverage limit, stated because no test establishes it: the fixture's explorer and contract scales agree, so nothing in the suite would catch a carried value that came from a *second* lookup which merely happens to agree. That the value is the same one the screen rendered from was established by inspection instead — `send.js` reads `tb.decimals` off the same `tb` object it reads `tb.balance` from, `tb.balance` is formatted with `tb.decimals` in `fetchTokenBalances()`, and `addr.tokenBalances` is written by nothing else. `ERC20_ABI` has exactly two encode sites, both now on the carried value; `approval.js` only decodes. Non-blocking, worth a follow-up rather than a rework: 1. `tests/e2e/network.js`, `ethCallResult()`: `word(opts.tokenDecimalsOverride || STUB_TOKEN.decimals)` ignores an override of `0` because it is falsy, so a future test making the contract claim zero decimals — a real lying-token case — would pass vacuously against the fixture's own 6. Acceptable looks like an explicit `== null` check. The JSDoc also types the override `{string}` where it is a scale. 2. The two new cases restore neither `env.routeOpts` nor the popup's screen when they fail: `seedReceipt = false` and `tokenDecimalsOverride = null` sit after awaits that can throw. Observed in mutation 2 above — two genuine failures cascaded into seven unrelated ones (48 through 54, "the extension opened no approval window"). Given the suite's known load sensitivity ([#287](https://git.eeqj.de/sneak/AutistMask/issues/287)) a single flake here reads as an eight-failure run. A `finally` restoring both flags and returning to the address screen would contain it. 3. Under the reverted-comparison mutation, case 43 fails as a 60s `#view-error-tx` timeout, so its stronger assertion (nothing reached `eth_sendRawTransaction`) never executes on the failing path. Asserting the broadcast count before waiting on the screen would make the failure both faster and more precise. 4. `mismatchMessage` is exported from `src/shared/transferAmount.js` and used nowhere outside it. 5. `transferAmountUnits("-1", 6, 6n)` returns `-1000000n`. Unreachable — `send.js` and `validateTransfer()` both refuse a negative amount and ethers will not ABI-encode a negative `uint256` — so no funds risk, but the module is the last gate before encoding. Probed and clean: `absent`, `null`, `undefined`, `0`, `"0"`, `""`, `"007"`, `"255"`, `"256"`, `"6.0"`, `"0x6"`, `"1e1"`, `"+6"`, `"-0"`, leading/trailing whitespace, `NaN`, `Infinity`, floats, negatives, `-1n`, `2n**70n`, `true`, `{}`, `[]`, `[6]`, `new Number(6)`, and an object with a `valueOf` — every one refuses rather than defaulting, and the `Number([]) === 0` claim holds because arrays never reach a numeric branch. Refusals reach the user as full sentences on ErrorTx, not as an unhandled throw; a missing carried scale also fails the estimate, and `FEE_UNAVAILABLE` keeps Send disabled.

clawbot merged commit 2f80a9bdb4 into next 2026-08-20 12:31:28 +02:00

clawbot referenced this pull request 2026-08-20 12:31:41 +02:00

test: e2e hygiene — a failing case leaves routeOpts and the popup screen dirty, cascading into unrelated failures #318

clawbot referenced this pull request 2026-08-20 12:36:28 +02:00

fix: gate the chain switch and remember endpoints per network (closes #308) #313

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-review

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#314