diff --git a/TODO.md b/TODO.md index 28d093b..7568b28 100644 --- a/TODO.md +++ b/TODO.md @@ -44,6 +44,17 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-12: The known-symbol spoof rule now judges the symbol a user actually + sees. `isSpoofedSymbol()` normalizes before the lookup — NFKC, then every + Unicode format character removed, then trimmed — so `" ETH "`, a no-break + space, a zero-width space and a fullwidth `ＥＴＨ` are all caught on the + balance list, the history and the send selector at once. Confusables that are + distinct letters (Cyrillic `Е`) and bidi reordering stay knowingly open and + are asserted as open in the suite. No bundled symbol contains whitespace or a + non-ASCII character, so nothing legitimate is newly filtered; the balance + list's token-type gate also became case-insensitive, which no longer drops a + real holding if an explorer writes `erc-20` + ([#260](https://git.eeqj.de/sneak/AutistMask/issues/260)). - 2026-08-12: The transaction confirmation screen has browser coverage. The end-to-end suite reaches ConfirmTx for both the native ETH and the ERC-20 path off a funded-balance fixture, and asserts the pending, funded, over-balance diff --git a/src/shared/balances.js b/src/shared/balances.js index a66bc5c..5974603 100644 --- a/src/shared/balances.js +++ b/src/shared/balances.js @@ -66,7 +66,12 @@ async function fetchTokenBalances(address, blockscoutUrl, trackedTokens) { const balances = []; for (const item of items) { - if (item.token?.type !== "ERC-20") continue; + // Case-insensitive: the token type is an explorer's label, not a + // protocol value, and an exact comparison silently drops a real + // holding if one ever writes "erc-20". Which types are admitted + // is unchanged. + const type = String(item.token?.type || "").toUpperCase(); + if (type !== "ERC-20") continue; const decimals = parseInt(item.token.decimals || "18", 10); const bal = formatTokenBalance(item.value || "0", decimals); if (bal === "0.0") continue; diff --git a/src/shared/symbolSpoof.js b/src/shared/symbolSpoof.js index b106528..3c36d56 100644 --- a/src/shared/symbolSpoof.js +++ b/src/shared/symbolSpoof.js @@ -13,6 +13,11 @@ // which has no contract at all, so no contract may bear it and every one // that does is a spoof. "ETH" is the only such entry today; the rule is // written so that a second one needs no change here or at any call site. +// +// The symbol is attacker-controlled — it is whatever the ERC-20 contract +// returns — so the lookup is done on a normalized form (issue #260): the +// question is whether the symbol reaches the user's eye as a known one, +// since that is what the user acts on. const { KNOWN_SYMBOLS } = require("./tokenList"); @@ -22,6 +27,37 @@ function normalizeAddress(addr) { return (addr || "").toLowerCase(); } +// Fold a symbol onto what a user actually sees, and no further: +// +// NFKC collapses compatibility variants that render as the ASCII +// letters they imitate — fullwidth ETH, styled mathematical +// letters — and maps the non-ASCII spaces onto U+0020. +// \p{Cf} drops format characters: zero-width space, joiner and +// non-joiner, word joiner, soft hyphen, byte-order mark, the +// bidi marks and overrides. These render as nothing at all, +// anywhere in the string, so they are removed everywhere and +// not merely at the ends. +// trim removes surrounding whitespace, which HTML collapses: +// `" ETH "` is painted next to the user's real ETH as `ETH`. +// toUpperCase makes the comparison case-insensitive, as before. +// +// Deliberately not folded, and asserted as open in tests/symbolSpoof.test.js: +// interior whitespace (`E T H` renders as `E T H`, so folding it would filter +// a token nobody could confuse with the native asset), confusables that are +// distinct letters rather than compatibility variants (Cyrillic capital Ie, +// U+0415; Greek capital Epsilon, U+0395), and bidi reordering, which needs +// the bidi algorithm rather than a character filter. +// +// This decides only how the question is asked. Nothing here changes what a +// surface displays; a token still shows the symbol it reports. +function normalizeSymbol(symbol) { + return String(symbol || "") + .normalize("NFKC") + .replace(/\p{Cf}/gu, "") + .trim() + .toUpperCase(); +} + // True when a token bearing `symbol` from contract `contractAddress` is // impersonating a known symbol. // @@ -31,7 +67,7 @@ function normalizeAddress(addr) { function isSpoofedSymbol(symbol, contractAddress) { const contract = normalizeAddress(contractAddress); if (!contract) return false; - const sym = (symbol || "").toUpperCase(); + const sym = normalizeSymbol(symbol); if (!KNOWN_SYMBOLS.has(sym)) return false; const legit = KNOWN_SYMBOLS.get(sym); if (legit === null) return true; diff --git a/tests/symbolSpoof.test.js b/tests/symbolSpoof.test.js index b0e612a..02e412b 100644 --- a/tests/symbolSpoof.test.js +++ b/tests/symbolSpoof.test.js @@ -124,6 +124,125 @@ describe("the shared rule", () => { }); }); +// Issue #260: the symbol is whatever the ERC-20 contract returns, and HTML +// collapses leading and trailing whitespace, so a token calling itself +// `" ETH "` reaches the user's eye as `ETH` while missing a raw +// KNOWN_SYMBOLS lookup. Normalizing inside the shared rule fixes all three +// surfaces at once, which is what consolidating the rule bought. +// +// Every character under test here is built from its code point rather than +// pasted in: most of them are invisible, and an invisible character in a +// test file is unreviewable. +const cp = (...codes) => String.fromCodePoint(...codes); +const NBSP = cp(0x00a0); // no-break space +const FIGURE_SPACE = cp(0x2007); +const IDEOGRAPHIC_SPACE = cp(0x3000); +const ZWSP = cp(0x200b); // zero-width space +const BOM = cp(0xfeff); // zero-width no-break space +const WORD_JOINER = cp(0x2060); +const SOFT_HYPHEN = cp(0x00ad); +const LRM = cp(0x200e); // left-to-right mark +const RLO = cp(0x202e); // right-to-left override +const FULLWIDTH_ETH = cp(0xff25, 0xff34, 0xff28); +const FULLWIDTH_USDC = cp(0xff55, 0xff53, 0xff44, 0xff43); // lowercase +const CYRILLIC_CAPITAL_IE = cp(0x0415); + +describe("the shared rule: symbols that render as a known symbol", () => { + test("ASCII padding does not buy a pass", () => { + expect(isSpoofedSymbol(" ETH ", FAKE_ETH_CONTRACT)).toBe(true); + expect(isSpoofedSymbol("\tETH\n", FAKE_ETH_CONTRACT)).toBe(true); + expect(isSpoofedSymbol(" usdc ", FAKE_ETH_CONTRACT)).toBe(true); + }); + + test("non-breaking and other Unicode spaces do not either", () => { + expect(isSpoofedSymbol(NBSP + "ETH" + NBSP, FAKE_ETH_CONTRACT)).toBe( + true, + ); + expect( + isSpoofedSymbol( + FIGURE_SPACE + "ETH" + IDEOGRAPHIC_SPACE, + FAKE_ETH_CONTRACT, + ), + ).toBe(true); + }); + + // These render as nothing at all, in any position, so they are removed + // wherever they sit rather than only at the ends. + test("zero-width characters are stripped wherever they sit", () => { + expect(isSpoofedSymbol("E" + ZWSP + "TH", FAKE_ETH_CONTRACT)).toBe( + true, + ); + expect(isSpoofedSymbol(BOM + "ETH", FAKE_ETH_CONTRACT)).toBe(true); + expect( + isSpoofedSymbol("ET" + WORD_JOINER + "H", FAKE_ETH_CONTRACT), + ).toBe(true); + expect( + isSpoofedSymbol("E" + SOFT_HYPHEN + "TH", FAKE_ETH_CONTRACT), + ).toBe(true); + }); + + // An LRM is invisible and, in all-Latin text, moves nothing: dropping it + // leaves exactly the string the user saw. + test("an invisible bidi mark does not hide a known symbol", () => { + expect(isSpoofedSymbol(LRM + "ETH", FAKE_ETH_CONTRACT)).toBe(true); + }); + + test("compatibility forms fold onto the symbol they imitate", () => { + expect(isSpoofedSymbol(FULLWIDTH_ETH, FAKE_ETH_CONTRACT)).toBe(true); + expect(isSpoofedSymbol(FULLWIDTH_USDC, FAKE_ETH_CONTRACT)).toBe(true); + }); + + // The two knowingly open classes, asserted here so that the boundary is + // a fact in the suite and not a claim in a PR body. A Cyrillic capital + // Ie is a distinct letter rather than a compatibility variant, so NFKC + // leaves it alone; and a right-to-left override reverses the rendering + // of what follows it, which dropping the control character does not + // undo. Closing either needs a confusables table or a bidi resolver, + // and both are a separate change from this one. + test("a Cyrillic homoglyph is knowingly still not caught", () => { + expect( + isSpoofedSymbol(CYRILLIC_CAPITAL_IE + "TH", FAKE_ETH_CONTRACT), + ).toBe(false); + }); + + test("a bidi-reordered symbol is knowingly still not caught", () => { + expect(isSpoofedSymbol(RLO + "HTE", FAKE_ETH_CONTRACT)).toBe(false); + }); + + // Normalization does not reach the native-asset exemption, which turns + // on the absence of a contract address and never on the symbol. + test("a padded symbol with no contract is still not a spoof", () => { + expect(isSpoofedSymbol(" ETH ", null)).toBe(false); + expect(isSpoofedSymbol(NBSP + "ETH", "")).toBe(false); + }); + + test("a genuine contract still bears its own padded symbol", () => { + expect(isSpoofedSymbol(" USDC ", USDC_CONTRACT)).toBe(false); + expect(isSpoofedSymbol(ZWSP + "WETH", WETH_CONTRACT)).toBe(false); + }); + + // Normalization must not invent a match. Interior ASCII whitespace is + // left alone: `E T H` renders as `E T H`, not as `ETH`, so folding it + // would filter a token no user could confuse with the native asset. + test("a symbol that renders differently is not judged a spoof", () => { + expect(isSpoofedSymbol("E T H", FAKE_ETH_CONTRACT)).toBe(false); + expect(isSpoofedSymbol("ETH2", FAKE_ETH_CONTRACT)).toBe(false); + expect(isSpoofedSymbol("MY ETH", FAKE_ETH_CONTRACT)).toBe(false); + }); + + // The false-positive question, answered against the shipped data rather + // than by assertion: no bundled symbol carries whitespace or a + // non-ASCII character, so the normalization cannot newly filter one. + test("no bundled symbol is touched by the normalization", () => { + for (const [symbol, address] of KNOWN_SYMBOLS) { + expect(symbol).toBe(symbol.trim()); + expect(symbol).toMatch(/^[ -~]+$/); + if (address === null) continue; + expect(isSpoofedSymbol(symbol, address)).toBe(false); + } + }); +}); + describe("surface 1: the transaction history", () => { function fakeEthTransfer() { return { @@ -147,6 +266,22 @@ describe("surface 1: the transaction history", () => { expect(result.transactions).toEqual([]); }); + // Issue #260 on this surface: the same transfer with a padded symbol. + test("a padded fake ETH token transfer is filtered too", () => { + const padded = { ...fakeEthTransfer(), symbol: " ETH " }; + const result = filterTransactions([padded], { + hideSpoofedSymbols: true, + hideFraudContracts: true, + hideLowHolderTokens: true, + hideDustTransactions: true, + dustThresholdGwei: 100000, + }); + expect(result.transactions).toEqual([]); + // The contract is learned as fraudulent, exactly as for the + // unpadded symbol: the padding must not cost the blocklist entry. + expect(result.newFraudContracts).toEqual([FAKE_ETH_CONTRACT]); + }); + test("a real native ETH transfer survives", () => { const native = { hash: "0x" + "2".repeat(64), @@ -201,6 +336,36 @@ describe("surface 2: the Send token selector", () => { expect(select.children).toEqual([]); }); + // Issue #260 on this surface: the option text is rendered into HTML, + // which collapses the padding, so an unfiltered padded token would sit + // in the selector reading exactly `ETH`. + test("a padded fake ETH token is not selectable either", () => { + render([ + { + address: FAKE_ETH_CONTRACT, + symbol: " ETH ", + decimals: 18, + balance: "0.005", + holders: 900000, + }, + ]); + expect(select.children).toEqual([]); + }); + + test("a genuine token with a padded symbol stays selectable", () => { + render([ + { + address: USDC_CONTRACT, + symbol: " USDC ", + decimals: 6, + balance: "12.5", + holders: 900000, + }, + ]); + expect(select.children).toHaveLength(1); + expect(select.children[0].value).toBe(USDC_CONTRACT); + }); + test("native ETH remains the always-present option", () => { render([]); expect(select.innerHTML).toBe(''); @@ -251,6 +416,35 @@ describe("surface 3: the balance list", () => { expect(balances).toEqual([]); }); + // Issue #260 on this surface: the balance list is where the user forms + // their belief about what they own, and it renders the symbol into HTML. + test("a padded fake ETH token is filtered too", async () => { + respondWith([fakeEthItem({ symbol: " ETH " })]); + expect(await fetchTokenBalances(HOLDER, BLOCKSCOUT, [])).toEqual([]); + }); + + test("a fake ETH token padded with a no-break space is filtered", async () => { + respondWith([fakeEthItem({ symbol: NBSP + "ETH" + NBSP })]); + expect(await fetchTokenBalances(HOLDER, BLOCKSCOUT, [])).toEqual([]); + }); + + // The false-positive direction on the surface that matters most: a real + // holding whose symbol happens to carry padding is still listed, and the + // list still shows the symbol the token actually reports. + test("a genuine token with a padded symbol is not newly filtered", async () => { + respondWith([ + fakeEthItem({ + address_hash: USDC_CONTRACT, + symbol: " USDC ", + name: "USD Coin", + decimals: "6", + }), + ]); + const balances = await fetchTokenBalances(HOLDER, BLOCKSCOUT, []); + expect(balances).toHaveLength(1); + expect(balances[0].symbol).toBe(" USDC "); + }); + test("a genuine token keeps its place in the list", async () => { respondWith([ fakeEthItem({ @@ -274,6 +468,33 @@ describe("surface 3: the balance list", () => { expect(await fetchTokenBalances(HOLDER, BLOCKSCOUT, [])).toEqual([]); }); + // The adjacent finding from the same review as issue #260: the type gate + // compared exactly, so an explorer that ever varied the casing would + // silently drop a real holding before any filter ran. The comparison is + // now case-insensitive, which changes nothing about which types are + // admitted. + test("a differently-cased ERC-20 type still lists a real holding", async () => { + respondWith([ + fakeEthItem({ + type: "erc-20", + address_hash: USDC_CONTRACT, + symbol: "USDC", + name: "USD Coin", + decimals: "6", + }), + ]); + const balances = await fetchTokenBalances(HOLDER, BLOCKSCOUT, []); + expect(balances).toHaveLength(1); + expect(balances[0].symbol).toBe("USDC"); + }); + + test("case insensitivity does not admit another token type", async () => { + respondWith([fakeEthItem({ type: "erc-721" })]); + expect(await fetchTokenBalances(HOLDER, BLOCKSCOUT, [])).toEqual([]); + respondWith([fakeEthItem({ type: "ERC-20-EXTRA" })]); + expect(await fetchTokenBalances(HOLDER, BLOCKSCOUT, [])).toEqual([]); + }); + // The money test: the user holds real ETH and has been airdropped a fake // ETH ERC-20. The fake is gone from the list of tokens; the real balance // is exactly what the node reported.