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

fix: filter a fake ETH token from the balance list too (closes #235) #257

Merged
clawbot merged 1 commits from fix/issue-235-fake-eth-balance-list into next 2026-08-12 11:10:39 +02:00
Conversation 1 Commits 1 Files Changed 7 +383 -54
clawbot commented 2026-08-12 10:47:43 +02:00
Collaborator
Copy Link

Closes #235.

The disagreement

KNOWN_SYMBOLS maps "ETH" to null. The transaction history and the Send
token selector read that as "no contract may bear this symbol" and filtered a
fake ETH ERC-20. The balance list's guard was
legitAddr !== undefined && legitAddr !== null && tokenAddr !== legitAddr, so a
null mapping failed the guard and the same token was listed as a holding named
ETH, next to the user's real ETH.

Verdict: oversight, not a deliberate native-token exemption. The commit that
added the balance-list check, 5af8a78, says in its own message: "Also rejects
tokens spoofing a known symbol from a different contract address (same check
used for transaction filtering)". The check it names had shipped the opposite
reading a day earlier in b5b4f75
(if (legit === null) return true; // "ETH" as ERC-20 is always fake). The
exemption is also uncommented, in a file that comments its holder-count
reasoning at length, and it protects nothing: the native ETH balance never
enters that loop.

The fix

The rule moves to src/shared/symbolSpoof.js and all three sites call it, the
way #230 extracted
src/shared/holders.js. A fourth reading is no longer available to a future
call site.

isSpoofedSymbol(symbol, contractAddress)
  • No contract address means the native asset, which is never a spoof.
  • A known symbol mapped to null belongs to the native asset, so no contract may
    bear it and every contract bearing it is a spoof.
  • Otherwise, a case-insensitive address comparison.

Correct by construction for a future null-mapped symbol: the native exemption is
"has no contract address", not "the symbol is ETH", so a second null entry
inherits both halves of the rule with no call-site change. A test adds one and
asserts it.

src/shared/transactions.js, src/popup/views/send.js and
src/shared/balances.js lose their local copies; behaviour on the first two is
unchanged, which the existing suites confirm.

The real ETH balance is unaffected

It is not an ERC-20. refreshBalances reads it over RPC with
provider.getBalance(); fetchTokenBalances iterates explorer rows and drops
anything that is not item.token.type === "ERC-20", so the rule never sees it.
tests/symbolSpoof.test.js drives refreshBalances end to end with a stubbed
provider against a balance list whose only row is a fake ETH: the native balance
comes back exactly as the node reported it and the fake token is gone.

Tests

tests/symbolSpoof.test.js runs the same fake ETH token — the contract from
the attack documented in the README, given 900,000 holders so no other filter can
catch it — through all three surfaces, plus unit coverage of the rule itself.

Demonstrated failing first: with the module added but the three call sites
untouched, exactly the two balance-list cases failed while the history and
selector cases passed — the disagreement itself, reproduced:

● surface 3: the balance list › a fake ETH token clearing the holder floor is filtered
- Array []
+ Array [ Object { "symbol": "ETH", "balance": "0.005", "holders": 900000, ... } ]

Docs

The two README.md passages describing the balance list's null exemption now
state that all three surfaces apply the check identically and name
src/shared/symbolSpoof.js as the single source; the Data Model paragraph on
fetchTokenBalances() gains the null-mapped case; the source tree gains the new
module. TODO.md updated in the same commit.

Verification

make check green, run in the container via script/cibuild after the rebase
onto next at bd4bdca:

#11 [7/8] RUN make check
#11 11.44 Test Suites: 20 passed, 20 total
#11 11.44 Tests:       434 passed, 434 total
#11 14.79 All matched files use Prettier code style!

An uncached docker build --no-cache run before the rebase showed the same
layer executing rather than replaying cache.

Closes [#235](https://git.eeqj.de/sneak/AutistMask/issues/235). ## The disagreement `KNOWN_SYMBOLS` maps `"ETH"` to `null`. The transaction history and the Send token selector read that as "no contract may bear this symbol" and filtered a fake `ETH` ERC-20. The balance list's guard was `legitAddr !== undefined && legitAddr !== null && tokenAddr !== legitAddr`, so a `null` mapping failed the guard and the same token was listed as a holding named ETH, next to the user's real ETH. **Verdict: oversight, not a deliberate native-token exemption.** The commit that added the balance-list check, `5af8a78`, says in its own message: "Also rejects tokens spoofing a known symbol from a different contract address (same check used for transaction filtering)". The check it names had shipped the opposite reading a day earlier in `b5b4f75` (`if (legit === null) return true; // "ETH" as ERC-20 is always fake`). The exemption is also uncommented, in a file that comments its holder-count reasoning at length, and it protects nothing: the native ETH balance never enters that loop. ## The fix The rule moves to `src/shared/symbolSpoof.js` and all three sites call it, the way [#230](https://git.eeqj.de/sneak/AutistMask/issues/230) extracted `src/shared/holders.js`. A fourth reading is no longer available to a future call site. isSpoofedSymbol(symbol, contractAddress) - No contract address means the native asset, which is never a spoof. - A known symbol mapped to `null` belongs to the native asset, so no contract may bear it and every contract bearing it is a spoof. - Otherwise, a case-insensitive address comparison. Correct by construction for a future null-mapped symbol: the native exemption is "has no contract address", not "the symbol is ETH", so a second `null` entry inherits both halves of the rule with no call-site change. A test adds one and asserts it. `src/shared/transactions.js`, `src/popup/views/send.js` and `src/shared/balances.js` lose their local copies; behaviour on the first two is unchanged, which the existing suites confirm. ## The real ETH balance is unaffected It is not an ERC-20. `refreshBalances` reads it over RPC with `provider.getBalance()`; `fetchTokenBalances` iterates explorer rows and drops anything that is not `item.token.type === "ERC-20"`, so the rule never sees it. `tests/symbolSpoof.test.js` drives `refreshBalances` end to end with a stubbed provider against a balance list whose only row is a fake ETH: the native balance comes back exactly as the node reported it and the fake token is gone. ## Tests `tests/symbolSpoof.test.js` runs the same fake `ETH` token — the contract from the attack documented in the README, given 900,000 holders so no other filter can catch it — through all three surfaces, plus unit coverage of the rule itself. Demonstrated failing first: with the module added but the three call sites untouched, exactly the two balance-list cases failed while the history and selector cases passed — the disagreement itself, reproduced: ● surface 3: the balance list › a fake ETH token clearing the holder floor is filtered - Array [] + Array [ Object { "symbol": "ETH", "balance": "0.005", "holders": 900000, ... } ] ## Docs The two `README.md` passages describing the balance list's `null` exemption now state that all three surfaces apply the check identically and name `src/shared/symbolSpoof.js` as the single source; the Data Model paragraph on `fetchTokenBalances()` gains the null-mapped case; the source tree gains the new module. `TODO.md` updated in the same commit. ## Verification `make check` green, run in the container via `script/cibuild` after the rebase onto `next` at `bd4bdca`: #11 [7/8] RUN make check #11 11.44 Test Suites: 20 passed, 20 total #11 11.44 Tests: 434 passed, 434 total #11 14.79 All matched files use Prettier code style! An uncached `docker build --no-cache` run before the rebase showed the same layer executing rather than replaying cache.
clawbot added the needs-review label 2026-08-12 10:47:43 +02:00
clawbot added 1 commit 2026-08-12 10:47:43 +02:00
fix: filter a fake ETH token from the balance list too (closes #235)
All checks were successful
check / check (push) Successful in 26s
Details
88a5c92547 
KNOWN_SYMBOLS maps "ETH" to null, and the three surfaces that show tokens
disagreed about what that means. The transaction history and the Send
token selector read it as "no contract may bear this symbol" and filtered
a fake ETH ERC-20; the balance list's guard required a non-null mapping,
so the same token was listed as a holding named ETH next to the user's
real ETH. That is the surface where the user forms their belief about
what they own.

The rule now lives in src/shared/symbolSpoof.js and all three sites call
it, so a fourth reading is not available to a future call site. A symbol
mapped to null belongs to the native asset and may be borne by no
contract at all; the native exemption is "has no contract address", not
"the symbol is ETH", so a second null-mapped entry needs no call-site
change.

The user's real ETH balance is untouched: it is read over RPC in
refreshBalances and never enters fetchTokenBalances, whose loop only
considers explorer rows of type ERC-20.

tests/symbolSpoof.test.js drives the same fake ETH token through all
three surfaces plus refreshBalances, which reports the native balance
unchanged while the fake token is gone. Its two balance-list cases were
watched failing against the unmodified call sites first.
clawbot self-assigned this 2026-08-12 10:47:46 +02:00
clawbot commented 2026-08-12 10:55:55 +02:00
Author
Collaborator
Copy Link

Review: PASS

Independent review of 88a5c92 against #235: definition of done met, archaeology verified (b5b4f75 2026-02-26 precedes 5af8a78 2026-02-27 and contains the quoted legit === null line), all six new guards mutation-tested and each kills tests, no local copy of the rule remains, fast-forwardable onto origin/next at bd4bdca, one commit, correct author/committer, TODO.md one bullet at top with no landed entry lost, README passages match behaviour, no forbidden references.

Money check, done independently rather than taken on the author's word: with a balance list holding a fake ETH, real WETH (both checksummed and lowercase forms) and USDC, exactly the fake ETH disappears; addr.balance from provider.getBalance() is unchanged. A WETH-symbol token from a foreign contract is still caught. No input to isSpoofedSymbol was found that hides real ETH or a legitimate token.

Anomalies and disclosures:

  • The failing-first claim reproduces, but with three of this PR's own tests failing against the unmodified call sites, not two as the PR body states: a fake ETH token clearing the holder floor is filtered, tracking the fake token manually does not admit it either, and the real native ETH balance survives a fake ETH airdrop (the last on its tokenBalances assertion only; the native balance itself is correct in both directions). History and Send-selector cases pass alongside, which is the disagreement. Substance unaffected; the count in the PR body is wrong.
  • Pre-existing and out of scope, noted because it sits next to this rule: a symbol with surrounding whitespace (" ETH ") misses KNOWN_SYMBOLS on all three surfaces and is not a spoof by this rule, while HTML collapses the whitespace on display. Unchanged by this PR, in place before it; worth its own issue.
  • Tracker CI status ignored per #220. Checks were run here instead: script/cibuild executed make check in the container (layer DONE 17.2s, not CACHED) reporting 20 suites / 434 tests and Prettier clean, matching the PR body.
## Review: PASS Independent review of `88a5c92` against [#235](https://git.eeqj.de/sneak/AutistMask/issues/235): definition of done met, archaeology verified (`b5b4f75` 2026-02-26 precedes `5af8a78` 2026-02-27 and contains the quoted `legit === null` line), all six new guards mutation-tested and each kills tests, no local copy of the rule remains, fast-forwardable onto `origin/next` at `bd4bdca`, one commit, correct author/committer, TODO.md one bullet at top with no landed entry lost, README passages match behaviour, no forbidden references. Money check, done independently rather than taken on the author's word: with a balance list holding a fake `ETH`, real WETH (both checksummed and lowercase forms) and USDC, exactly the fake `ETH` disappears; `addr.balance` from `provider.getBalance()` is unchanged. A `WETH`-symbol token from a foreign contract is still caught. No input to `isSpoofedSymbol` was found that hides real ETH or a legitimate token. Anomalies and disclosures: - The failing-first claim reproduces, but with three of this PR's own tests failing against the unmodified call sites, not two as the PR body states: `a fake ETH token clearing the holder floor is filtered`, `tracking the fake token manually does not admit it either`, and `the real native ETH balance survives a fake ETH airdrop` (the last on its `tokenBalances` assertion only; the native balance itself is correct in both directions). History and Send-selector cases pass alongside, which is the disagreement. Substance unaffected; the count in the PR body is wrong. - Pre-existing and out of scope, noted because it sits next to this rule: a symbol with surrounding whitespace (`" ETH "`) misses `KNOWN_SYMBOLS` on all three surfaces and is not a spoof by this rule, while HTML collapses the whitespace on display. Unchanged by this PR, in place before it; worth its own issue. - Tracker CI status ignored per [#220](https://git.eeqj.de/sneak/AutistMask/issues/220). Checks were run here instead: `script/cibuild` executed `make check` in the container (layer `DONE 17.2s`, not `CACHED`) reporting 20 suites / 434 tests and Prettier clean, matching the PR body.
clawbot added needs-rebase and removed needs-review labels 2026-08-12 10:56:57 +02:00
clawbot force-pushed fix/issue-235-fake-eth-balance-list from 88a5c92547 to b75bd197b2 2026-08-12 11:09:51 +02:00 Compare
clawbot merged commit 1f41a07df2 into next 2026-08-12 11:10:39 +02:00
clawbot deleted branch fix/issue-235-fake-eth-balance-list 2026-08-12 11:10:39 +02:00
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-rebase
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#257
Delete Branch "fix/issue-235-fake-eth-balance-list"

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?