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

fix: a dust threshold of 0 silently becomes 100,000 gwei, and isSpoofedSymbol is case-sensitive on the contract address #179

New Issue
Closed
opened 2026-08-09 07:08:34 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 07:08:34 +02:00
Collaborator
Copy Link

Two small defects found while writing the first test coverage for the
anti-poisoning filters (#160 / PR #175), deliberately not fixed there. Both are
in src/shared/transactions.js and both are a few lines, so grouping them.

1. A dust threshold of 0 is silently replaced by the default

src/shared/transactions.js:257:

filters.dustThresholdGwei || 100000

0 is falsy, so a user who sets the threshold to 0 — the obvious way to say
"do not hide anything as dust" — gets the 100,000 gwei default applied instead.
The wallet then hides transactions the user explicitly asked to see, with no
indication why.

This is reachable: the threshold is a free-text editable field at
src/popup/views/settings.js:305-310. It also directly contradicts
README.md:810-814, which promises the user can "configure the wallet to show
everything unfiltered, unix-style" — 0 is exactly how a user would express
that, and it is the one value that does not work.

Requirements

  • Distinguish "unset" from "zero". Use a nullish check (??) or an explicit
    undefined/null test rather than ||.
  • Decide and document what 0 means: almost certainly "disable dust
    filtering", which is also what the separate enable/disable checkbox does.
    Make sure the two controls agree and neither silently overrides the other.
  • Check the input validation path in settings.js:305-310 — confirm what
    happens for empty string, negative numbers, and non-numeric input, and make
    each behave sensibly rather than coercing to a surprising default. Say in
    the PR what each now does.
  • Audit src/shared/ for the same ||-as-default pattern applied to any other
    numeric setting; this class of bug rarely appears once. Report what you find
    even if you do not fix it here.

2. isSpoofedSymbol is case-sensitive on the contract address

src/shared/transactions.js:209 compares the contract address without
normalising case, unlike the blocklist check at transactions.js:215-217,
which lowercases correctly.

Ethereum addresses are routinely rendered in EIP-55 mixed-case checksum form.
A checksummed address for a genuine, known contract will therefore fail the
equality test and be misclassified as a spoof — a false positive that hides
a legitimate token transfer from the user's history.

The reporter notes this is currently unreachable in-app, because the addresses
reaching this function happen to already be lowercased upstream. That makes it
latent rather than live: isSpoofedSymbol is an exported function, the
inconsistency with the adjacent blocklist check is exactly the kind of thing a
later refactor trips over, and the failure direction is silent data loss from
the history view.

Requirements

  • Normalise case on both sides of the comparison at transactions.js:209, the
    same way :215-217 already does.
  • Check every other address comparison in transactions.js for the same
    inconsistency and make them uniform. Prefer one shared normalisation helper
    over repeating .toLowerCase() at each site, so the next comparison added
    cannot get it wrong.
  • Add a test passing a checksummed genuine contract address and asserting it is
    not flagged as a spoof. tests/transactions.test.js already has a
    current behaviour: test pinning the case-sensitive behaviour — invert it
    rather than deleting it.

Definition of done

  • A dust threshold of 0 disables dust filtering rather than applying the
    100,000 gwei default.
  • Empty, negative and non-numeric threshold input each behave sensibly, and
    the PR states what each does.
  • Any other ||-as-numeric-default in src/shared/ is reported.
  • isSpoofedSymbol treats a checksummed genuine contract as genuine.
  • Address comparisons in transactions.js are consistently normalised,
    ideally through one helper.
  • Both current behaviour: tests are inverted into regression guards.
  • All other tests in tests/transactions.test.js still pass unmodified.
  • TODO.md updated in the same commit.
  • make check passes.
Two small defects found while writing the first test coverage for the anti-poisoning filters (#160 / PR #175), deliberately not fixed there. Both are in `src/shared/transactions.js` and both are a few lines, so grouping them. ## 1. A dust threshold of 0 is silently replaced by the default `src/shared/transactions.js:257`: ```js filters.dustThresholdGwei || 100000 ``` `0` is falsy, so a user who sets the threshold to `0` — the obvious way to say "do not hide anything as dust" — gets the 100,000 gwei default applied instead. The wallet then hides transactions the user explicitly asked to see, with no indication why. This is reachable: the threshold is a free-text editable field at `src/popup/views/settings.js:305-310`. It also directly contradicts `README.md:810-814`, which promises the user can "configure the wallet to show everything unfiltered, unix-style" — `0` is exactly how a user would express that, and it is the one value that does not work. **Requirements** - Distinguish "unset" from "zero". Use a nullish check (`??`) or an explicit `undefined`/`null` test rather than `||`. - Decide and document what `0` means: almost certainly "disable dust filtering", which is also what the separate enable/disable checkbox does. Make sure the two controls agree and neither silently overrides the other. - Check the input validation path in `settings.js:305-310` — confirm what happens for empty string, negative numbers, and non-numeric input, and make each behave sensibly rather than coercing to a surprising default. Say in the PR what each now does. - Audit `src/shared/` for the same `||`-as-default pattern applied to any other numeric setting; this class of bug rarely appears once. Report what you find even if you do not fix it here. ## 2. `isSpoofedSymbol` is case-sensitive on the contract address `src/shared/transactions.js:209` compares the contract address without normalising case, unlike the blocklist check at `transactions.js:215-217`, which lowercases correctly. Ethereum addresses are routinely rendered in EIP-55 mixed-case checksum form. A checksummed address for a genuine, known contract will therefore fail the equality test and be misclassified as a **spoof** — a false positive that hides a legitimate token transfer from the user's history. The reporter notes this is currently unreachable in-app, because the addresses reaching this function happen to already be lowercased upstream. That makes it latent rather than live: `isSpoofedSymbol` is an exported function, the inconsistency with the adjacent blocklist check is exactly the kind of thing a later refactor trips over, and the failure direction is silent data loss from the history view. **Requirements** - Normalise case on both sides of the comparison at `transactions.js:209`, the same way `:215-217` already does. - Check every other address comparison in `transactions.js` for the same inconsistency and make them uniform. Prefer one shared normalisation helper over repeating `.toLowerCase()` at each site, so the next comparison added cannot get it wrong. - Add a test passing a checksummed genuine contract address and asserting it is **not** flagged as a spoof. `tests/transactions.test.js` already has a `current behaviour:` test pinning the case-sensitive behaviour — invert it rather than deleting it. ## Definition of done - [ ] A dust threshold of `0` disables dust filtering rather than applying the 100,000 gwei default. - [ ] Empty, negative and non-numeric threshold input each behave sensibly, and the PR states what each does. - [ ] Any other `||`-as-numeric-default in `src/shared/` is reported. - [ ] `isSpoofedSymbol` treats a checksummed genuine contract as genuine. - [ ] Address comparisons in `transactions.js` are consistently normalised, ideally through one helper. - [ ] Both `current behaviour:` tests are inverted into regression guards. - [ ] All other tests in `tests/transactions.test.js` still pass unmodified. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.
clawbot added this to the 1.0.0 milestone 2026-08-09 07:08:34 +02:00
clawbot commented 2026-08-09 07:17:57 +02:00
Author
Collaborator
Copy Link

Additional requirement, from the mutation testing done during the review of
PR #175.

The reviewer applied 15 mutations to src/shared/transactions.js; 13 were
killed by the new suite. One of the two survivors falls in the filter logic
this issue touches:

src/shared/transactions.js:244 — removing the tx.holders !== null guard
survives undetected. No fixture pairs a non-null contractAddress with a
null holders value, so the guard that stops the low-holder rule from acting
on an unknown holder count is unpinned.

That is worth closing here rather than leaving it, because the failure
direction is bad: without the guard, a token whose holder count could not be
fetched (a Blockscout hiccup, a rate limit, a self-hosted instance that does
not return the field) would be treated as having zero holders and silently
filtered out of the user's history. That is over-filtering — the same class of
harm as the zero-threshold bug in part 1 of this issue, where the user asks to
see everything and the wallet hides things anyway.

Please add:

  • A fixture with a real contractAddress and holders: null, asserting the
    transaction is not filtered by the low-holder rule.
  • The same for holders: undefined if that is reachable from the Blockscout
    response shape — check rather than assume.
  • Verification that the mutation is now caught: delete the !== null guard,
    confirm at least one test fails, revert, and state the result in the PR as
    PR #175 did.

Note this pairs naturally with part 1 of this issue. Both are cases where a
missing or falsy value is silently coerced into filtering behaviour the user
did not ask for — || 100000 turning "show everything" into the default
threshold, and a null holder count turning "unknown" into "suspicious". Worth
checking whether there are other instances of the same shape in
transactions.js while you are there, and reporting them even if you do not
fix them.

Additional requirement, from the mutation testing done during the review of PR #175. The reviewer applied 15 mutations to `src/shared/transactions.js`; 13 were killed by the new suite. One of the two survivors falls in the filter logic this issue touches: **`src/shared/transactions.js:244` — removing the `tx.holders !== null` guard survives undetected.** No fixture pairs a non-null `contractAddress` with a null `holders` value, so the guard that stops the low-holder rule from acting on an unknown holder count is unpinned. That is worth closing here rather than leaving it, because the failure direction is bad: without the guard, a token whose holder count could not be fetched (a Blockscout hiccup, a rate limit, a self-hosted instance that does not return the field) would be treated as having zero holders and silently filtered out of the user's history. That is over-filtering — the same class of harm as the zero-threshold bug in part 1 of this issue, where the user asks to see everything and the wallet hides things anyway. Please add: - A fixture with a real `contractAddress` and `holders: null`, asserting the transaction is **not** filtered by the low-holder rule. - The same for `holders: undefined` if that is reachable from the Blockscout response shape — check rather than assume. - Verification that the mutation is now caught: delete the `!== null` guard, confirm at least one test fails, revert, and state the result in the PR as PR #175 did. Note this pairs naturally with part 1 of this issue. Both are cases where a missing or falsy value is silently coerced into filtering behaviour the user did not ask for — `|| 100000` turning "show everything" into the default threshold, and a null holder count turning "unknown" into "suspicious". Worth checking whether there are other instances of the same shape in `transactions.js` while you are there, and reporting them even if you do not fix them.
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#179
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?