sneak/AutistMask
1
0
Fork 0
Code Issues 50 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-08-20 11:58:39 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-20 11:58:39 +02:00
Collaborator
Copy Link

Found by the pre-1.0 deployability audit (#303). Blocker: signs a transfer the user did not approve.

src/popup/views/confirmTx.js:448-450:

const decimals = await contract.decimals();
const amount = parseUnits(pendingTx.amount, decimals);
tx = await contract.transfer(pendingTx.to, amount);

The confirmation screen is built entirely from the indexer's decimals (src/shared/balances.js:75-76). The transfer is encoded from the contract's decimals(), read at signing time. Nothing compares the two. Unlike the dApp approval path there is no approvalVerify equivalent here at all — confirmTx signs and broadcasts directly.

Reproduction

Blockscout reports 6 decimals; an overlay makes decimals() answer 18 at signing time only:

[P3] L: send screen shows "Current balance: 1.5 E2E"
[P3] L: confirm-tx ... "Amount 0.25 E2E   Your balance 1.5 E2E ... Sign & Send"
[P3] P3: answered decimals() with 18 (indexer says 6)
[P3] L: ERC-20 signed artifact -> {"fn":"transfer","recipient":"0xC0FfEE...",
       "rawAmount":"250000000000000000","asIf6Decimals":250000000000,"asIf18Decimals":0.25}

The user approved 0.25. The signed artifact moves 250,000,000,000 tokens — 10^12 times more.

Consequence

The user loses their entire balance of any token whose on-chain decimals() disagrees with the indexer's cached value: an upgradeable or proxy token, a caller-dependent token, a stale or wrong indexer entry, or a compromised Blockscout. Precondition: the user sends a non-bundled ERC-20 through the wallet's own Send screen.

Related: the Chrome e2e suite never clicks #btn-confirm-send, so the popup's own Send → ConfirmTx → Sign & Send → WaitTx path has no end-to-end coverage at all. That is how this survived.

Definition of done

  • The decimals used to encode the transfer are the same value the screen rendered — carried forward from pendingTx, not re-read at signing time.
  • A mismatch between the displayed decimals and the contract's aborts with a message naming the discrepancy, rather than signing either value.
  • Test: a contract lying about decimals() at signing time fails to broadcast.
  • Test: the popup's own ERC-20 send path is driven end to end through #btn-confirm-send to a broadcast, and the signed artifact's amount is asserted against what the confirmation screen displayed.
  • make check green.
Found by the pre-1.0 deployability audit (https://git.eeqj.de/sneak/AutistMask/issues/303). **Blocker: signs a transfer the user did not approve.** `src/popup/views/confirmTx.js:448-450`: ```js const decimals = await contract.decimals(); const amount = parseUnits(pendingTx.amount, decimals); tx = await contract.transfer(pendingTx.to, amount); ``` The confirmation screen is built entirely from the **indexer's** decimals (`src/shared/balances.js:75-76`). The transfer is encoded from the **contract's** `decimals()`, read at signing time. Nothing compares the two. Unlike the dApp approval path there is no `approvalVerify` equivalent here at all — `confirmTx` signs and broadcasts directly. ## Reproduction Blockscout reports 6 decimals; an overlay makes `decimals()` answer 18 at signing time only: ``` [P3] L: send screen shows "Current balance: 1.5 E2E" [P3] L: confirm-tx ... "Amount 0.25 E2E Your balance 1.5 E2E ... Sign & Send" [P3] P3: answered decimals() with 18 (indexer says 6) [P3] L: ERC-20 signed artifact -> {"fn":"transfer","recipient":"0xC0FfEE...", "rawAmount":"250000000000000000","asIf6Decimals":250000000000,"asIf18Decimals":0.25} ``` The user approved `0.25`. The signed artifact moves 250,000,000,000 tokens — 10^12 times more. ## Consequence The user loses their entire balance of any token whose on-chain `decimals()` disagrees with the indexer's cached value: an upgradeable or proxy token, a caller-dependent token, a stale or wrong indexer entry, or a compromised Blockscout. Precondition: the user sends a non-bundled ERC-20 through the wallet's own Send screen. Related: the Chrome e2e suite never clicks `#btn-confirm-send`, so the popup's own Send → ConfirmTx → Sign & Send → WaitTx path has no end-to-end coverage at all. That is how this survived. ## Definition of done - [ ] The decimals used to encode the transfer are the same value the screen rendered — carried forward from `pendingTx`, not re-read at signing time. - [ ] A mismatch between the displayed decimals and the contract's aborts with a message naming the discrepancy, rather than signing either value. - [ ] Test: a contract lying about `decimals()` at signing time fails to broadcast. - [ ] Test: the popup's own ERC-20 send path is driven end to end through `#btn-confirm-send` to a broadcast, and the signed artifact's amount is asserted against what the confirmation screen displayed. - [ ] `make check` green.
clawbot added this to the 1.0.0 milestone 2026-08-20 11:58:39 +02:00
clawbot commented 2026-08-20 12:06:05 +02:00
Author
Collaborator
Copy Link

Plan:

  • send.js carries tokenDecimals on the object it hands to showConfirmTx, taken from the same tokenBalances entry the screen's symbol and balance come from. Every token the send dropdown offers is built from addr.tokenBalances, which only fetchTokenBalances() writes, so a selectable token always has a decimals value.
  • New src/shared/transferAmount.js, the confirmTx counterpart to approvalVerify.js: transferAmountUnits(amount, displayedDecimals, contractDecimals) returns base units only when the two agree, and otherwise throws a full sentence naming both numbers. Absent or unusable displayed decimals is a refusal too — an amount that cannot be compared with what was shown has not been checked.
  • confirmTx.js encodes from pendingTx.tokenDecimals in both places it encodes: the gas estimate (which no longer reads decimals() at all) and the signing path, where the contract's decimals() is read purely to be compared. Neither value is silently preferred.

Tests:

  • jest unit tests for transferAmount.js (agreement, disagreement in both directions, absent displayed value, non-integer / out-of-uint8 values).
  • Chrome e2e: the popup's own Send → ConfirmTx → Sign & Send path driven through #btn-confirm-send to a broadcast, asserting the signed artifact's transfer() amount against the amount read off the confirmation screen; and the same path with the fixture's decimals() flipped to 18 after the screen was built, asserting the error screen names both numbers and that nothing reached eth_sendRawTransaction. The fixture gains a decimals() override and a receipt so the wait screen resolves rather than polling for the rest of the run.
Plan: - `send.js` carries `tokenDecimals` on the object it hands to `showConfirmTx`, taken from the same `tokenBalances` entry the screen's symbol and balance come from. Every token the send dropdown offers is built from `addr.tokenBalances`, which only `fetchTokenBalances()` writes, so a selectable token always has a decimals value. - New `src/shared/transferAmount.js`, the `confirmTx` counterpart to `approvalVerify.js`: `transferAmountUnits(amount, displayedDecimals, contractDecimals)` returns base units only when the two agree, and otherwise throws a full sentence naming both numbers. Absent or unusable displayed decimals is a refusal too — an amount that cannot be compared with what was shown has not been checked. - `confirmTx.js` encodes from `pendingTx.tokenDecimals` in both places it encodes: the gas estimate (which no longer reads `decimals()` at all) and the signing path, where the contract's `decimals()` is read purely to be compared. Neither value is silently preferred. Tests: - jest unit tests for `transferAmount.js` (agreement, disagreement in both directions, absent displayed value, non-integer / out-of-uint8 values). - Chrome e2e: the popup's own Send → ConfirmTx → Sign & Send path driven through `#btn-confirm-send` to a broadcast, asserting the signed artifact's `transfer()` amount against the amount read off the confirmation screen; and the same path with the fixture's `decimals()` flipped to 18 after the screen was built, asserting the error screen names both numbers and that nothing reached `eth_sendRawTransaction`. The fixture gains a `decimals()` override and a receipt so the wait screen resolves rather than polling for the rest of the run.
clawbot commented 2026-08-20 12:16:16 +02:00
Author
Collaborator
Copy Link

Built as planned, in #314 (branch issue-305-erc20-decimals, base next).

The scale is carried on pendingTx.tokenDecimals from the tokenBalances entry the screen's own amount, balance and symbol come from; confirmTx encodes from it in both places it encodes, and reads the contract's decimals() at signing time only to compare. A disagreement throws a sentence naming both numbers and reaches the error screen. The gas estimate no longer calls decimals() at all.

Verification:

  • make check green: 758 tests in 32 suites, test-verify-build 18 cases, lint and format clean in the pinned container.
  • make test-e2e green, 54/54. The reproduction from the issue body, now as an assertion:
# 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 against a vacuous pass: reverting only the signing-side comparison to parseUnits(pendingTx.amount, await contract.decimals()) gives 53/54 with not ok 43, everything else green.

Case 42 covers the path that had no coverage — #btn-confirm-send was never clicked anywhere in the suite before this — and stays green under that mutation on purpose, since the fixture's two scales agree there; case 43 is the one that discriminates.

Built as planned, in [#314](https://git.eeqj.de/sneak/AutistMask/pulls/314) (branch `issue-305-erc20-decimals`, base `next`). The scale is carried on `pendingTx.tokenDecimals` from the `tokenBalances` entry the screen's own amount, balance and symbol come from; `confirmTx` encodes from it in both places it encodes, and reads the contract's `decimals()` at signing time only to compare. A disagreement throws a sentence naming both numbers and reaches the error screen. The gas estimate no longer calls `decimals()` at all. Verification: - `make check` green: 758 tests in 32 suites, `test-verify-build` 18 cases, lint and format clean in the pinned container. - `make test-e2e` green, 54/54. The reproduction from the issue body, now as an assertion: ``` # 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 against a vacuous pass: reverting only the signing-side comparison to `parseUnits(pendingTx.amount, await contract.decimals())` gives 53/54 with `not ok 43`, everything else green. Case 42 covers the path that had no coverage — `#btn-confirm-send` was never clicked anywhere in the suite before this — and stays green under that mutation on purpose, since the fixture's two scales agree there; case 43 is the one that discriminates.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#305
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?