fix: a fake token impersonating ETH is filtered from history and send, but still shows in the balance list #235
Reference in New Issue
Block a user
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?
KNOWN_SYMBOLS.set("ETH", null)(src/shared/tokenList.js:3613) is the only null-mapped entry in the known-symbol table, and the three spoof checks treat it inconsistently:src/shared/transactions.js:245—if (legit === null) return true;→ a fakeETHIS caught in transaction historysrc/popup/views/send.js:122— same → caught in the send selectorsrc/shared/balances.js:84-89—legitAddr !== undefined && legitAddr !== null && tokenAddr !== legitAddr→ a null mapping fails the guard, so a fakeETHis NOT caught in the balance listSo an ERC-20 calling itself
ETHis hidden from history and cannot be selected to send, but appears in the user's balance list as a holding named ETH.That is the worst of the three surfaces to miss. The balance list is where a user forms their belief about what they own, and a token displaying as
ETHnext to their real ETH is precisely the confusion the known-symbol check exists to prevent. It is also the exact scenario the README uses as its worked example for this filter.Pre-existing; not introduced by #176. Found by the independent review of #226, which is separately correcting the documentation to describe the current behaviour.
The question to settle first
Establish whether the
balances.jsexemption is a deliberate native-token special case or an oversight.ETHbeing the sole null entry suggests the null was meant to mean "the native asset has no contract address", and the other two call sites read that as "anything claiming to be it is a spoof" whilebalances.jsreads it as "no check possible". One of those readings is wrong.If the code cannot settle it, say so and escalate rather than guessing — but the consequence strongly favours filtering.
Implementation requirements
balances.jsalone.Definition of done
ETHand a contract address is filtered from the balance list.ETHtoken on all three surfaces, demonstrated failing against the current code first.TODO.mdupdated in the same commit.make checkpasses.The question, settled: oversight, not a deliberate exemption
The commit that added the balance-list check,
5af8a78"Filter spam tokens frombalance display", says in its own message:
> Also rejects tokens spoofing a known symbol from a different contract address
> (same check used for transaction filtering).
It was written to be the same check as
transactions.js, and it is not. ThelegitAddr !== nullclause reads as a reflexive nullish guard rather than anative-token carve-out: the author states the intent to duplicate a check that
had already shipped the opposite reading a day earlier in
b5b4f75(
if (legit === null) return true; // "ETH" as ERC-20 is always fake).Two further points against "deliberate": the exemption is nowhere commented,
where the same file comments its holder-count reasoning at length; and nothing
in the balance list needs it, because the native ETH balance never enters that
loop — it comes from
provider.getBalance()inrefreshBalances, whilefetchTokenBalancesiterates explorer rows and skips anything that is notitem.token.type === "ERC-20". The clause protects nothing and costs the check.Plan
Extract the rule to one module,
src/shared/symbolSpoof.js, the way#230 extracted
src/shared/holders.js, and have all three sites call it:isSpoofedSymbol(symbol, contractAddress)— no contract address means thenative asset, which is never a spoof; a known symbol mapped to
nullmeans nocontract may bear it, so any contract bearing it is a spoof; otherwise compare
addresses case-insensitively.
ETHbeing the only null entry:the native guard is "has no contract", which holds for any symbol that becomes
null-mapped later.
src/shared/transactions.js,src/popup/views/send.jsandsrc/shared/balances.jslose their local copies and call the module.ETHERC-20 on all three surfaces, written and watchedfailing on the balance list against unmodified
nextfirst.nullexemption becomea statement that all three surfaces apply the check identically.
Built as planned in #257; that
PR body is the record of what changed and why.
Verification: the two balance-list cases in
tests/symbolSpoof.test.jswerewatched failing with the module in place and the three call sites untouched —
the history and Send selector cases passed alongside them, which is the
disagreement itself — and pass after the call sites were switched over.
make checkruns green in the container viascript/cibuildon the rebased branch:20 suites, 434 tests, Prettier clean.
refreshBalancesis driven end to endagainst a balance list whose only row is a fake
ETH; the native balance comesback exactly as the node reported it.