fix: honour a dust threshold of 0 and compare addresses case-insensitively (closes #179)
All checks were successful
check / check (push) Successful in 41s
All checks were successful
check / check (push) Successful in 41s
Two defects in the anti-poisoning filters, both silent over-filtering: the wallet hid transactions the user had asked to see. A dust threshold of 0 was read as `filters.dustThresholdGwei || 100000`, so the one value a user would pick to mean "show everything" was swallowed and replaced by the default. It is now `??`, making 0 a real threshold that hides nothing and agrees exactly with clearing the hide-dust checkbox. The Settings input rejects empty, negative, fractional and non-numeric entries outright instead of coercing them, and resyncs the field to the stored value so it never displays a threshold the wallet is not using. isSpoofedSymbol compared the contract address with `===` against a lowercased known address. EIP-55 mixed case is a checksum, not identity, so a genuine token arriving checksummed was classified as a spoof and hidden. All address comparisons in the module now go through one normalizeAddress helper, which also normalises the fraud contracts recorded from a detected spoof. Tests cover threshold 0 versus unset versus a set value, and the contract comparison in lowercase, uppercase and EIP-55 form as well as against a genuinely different address. The two `current behaviour:` tests pinning the old behaviour are inverted into regression guards, and a fixture pairing a real contract address with a null holder count pins the `tx.holders !== null` guard that no fixture previously reached.
This commit is contained in:
@@ -329,18 +329,42 @@ describe("known-symbol spoof verification", () => {
|
||||
expect(result.newFraudContracts).toEqual([]);
|
||||
});
|
||||
|
||||
// Documents current behaviour, not desired behaviour: the spoof check
|
||||
// compares tx.contractAddress against a lowercased known address with
|
||||
// ===, so a caller passing a checksummed address for a genuine token has
|
||||
// it treated as a spoof. In the app this cannot happen because
|
||||
// parseTokenTransfer lowercases, but the exported function is not
|
||||
// defensive about it the way the blocklist check is.
|
||||
test("current behaviour: a checksummed genuine contract is treated as a spoof", () => {
|
||||
const genuineButChecksummed = tokenTx({
|
||||
// Regression guard (#179): EIP-55 mixed case is a checksum over the
|
||||
// address, not part of its identity, so the contract comparison must be
|
||||
// case-insensitive in both directions — a genuine token in any casing is
|
||||
// genuine, and a spoof cannot escape detection by changing its casing.
|
||||
test("a genuine contract in all-lowercase form is not a spoof", () => {
|
||||
const tx = tokenTx({ contractAddress: USDC_CONTRACT });
|
||||
expect(filterTransactions([tx], filters()).transactions).toEqual([tx]);
|
||||
});
|
||||
|
||||
test("a genuine contract in EIP-55 checksummed form is not a spoof", () => {
|
||||
const tx = tokenTx({
|
||||
contractAddress: "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48",
|
||||
});
|
||||
const result = filterTransactions([genuineButChecksummed], filters());
|
||||
const result = filterTransactions([tx], filters());
|
||||
expect(result.transactions).toEqual([tx]);
|
||||
expect(result.newFraudContracts).toEqual([]);
|
||||
});
|
||||
|
||||
test("a genuine contract in all-uppercase form is not a spoof", () => {
|
||||
const tx = tokenTx({
|
||||
contractAddress: "0X" + USDC_CONTRACT.slice(2).toUpperCase(),
|
||||
});
|
||||
const result = filterTransactions([tx], filters());
|
||||
expect(result.transactions).toEqual([tx]);
|
||||
expect(result.newFraudContracts).toEqual([]);
|
||||
});
|
||||
|
||||
test("a genuinely different contract claiming USDC is still a spoof in any casing", () => {
|
||||
const tx = tokenTx({
|
||||
contractAddress: "0xD05339F9EA5AB9D9F03B9D57F671D2ABD1F55C82",
|
||||
});
|
||||
const result = filterTransactions([tx], filters());
|
||||
expect(result.transactions).toEqual([]);
|
||||
// The recorded fraud contract is normalised, so the persisted
|
||||
// blocklist matches later transfers whatever casing they arrive in.
|
||||
expect(result.newFraudContracts).toEqual([FAKE_ETH_CONTRACT]);
|
||||
});
|
||||
|
||||
// Documents current behaviour: README.md:810-814 says all four filters
|
||||
@@ -412,6 +436,21 @@ describe("low-holder token filtering (the 1,000-holder rule)", () => {
|
||||
expect(tx.holders).toBeNull();
|
||||
expect(filterTransactions([tx], filters()).transactions).toEqual([tx]);
|
||||
});
|
||||
|
||||
// Regression guard (#179): an unknown holder count on a real token — the
|
||||
// explorer rate-limited the call, or a self-hosted instance omits the
|
||||
// field — must not be read as zero holders. Reading it that way hides a
|
||||
// legitimate transfer from the user's history, the same over-filtering
|
||||
// harm as the zero-threshold bug. This pins the `tx.holders !== null`
|
||||
// guard, which no fixture previously reached.
|
||||
test("a token whose holder count is unknown is not filtered", () => {
|
||||
const tx = tokenTx({
|
||||
symbol: NOVEL_SPAM_SYMBOL,
|
||||
contractAddress: NOVEL_SPAM_CONTRACT,
|
||||
holders: null,
|
||||
});
|
||||
expect(filterTransactions([tx], filters()).transactions).toEqual([tx]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("fraud contract blocklist", () => {
|
||||
@@ -575,16 +614,50 @@ describe("dust threshold filtering", () => {
|
||||
expect(filterTransactions([tx], filters()).transactions).toEqual([tx]);
|
||||
});
|
||||
|
||||
// Documents current behaviour: the threshold is read as
|
||||
// `filters.dustThresholdGwei || 100000`, so a user who sets the threshold
|
||||
// to 0 (the natural way to ask for no dust filtering while leaving the
|
||||
// toggle on) silently gets the 100,000 gwei default instead.
|
||||
test("current behaviour: a threshold of 0 falls back to the 100,000 gwei default", () => {
|
||||
const result = filterTransactions(
|
||||
[dustOf(50)],
|
||||
// Regression guard (#179): 0 is a real threshold meaning "hide nothing",
|
||||
// not an absent one. It used to be swallowed by `|| 100000`, so the one
|
||||
// value a user would pick to see everything was the one that did not
|
||||
// work.
|
||||
test("a threshold of 0 hides nothing, leaving the toggle on", () => {
|
||||
const dust = dustOf(50);
|
||||
const zero = dustOf(0);
|
||||
const opts = filters({ dustThresholdGwei: 0 });
|
||||
expect(filterTransactions([dust], opts).transactions).toEqual([dust]);
|
||||
expect(filterTransactions([zero], opts).transactions).toEqual([zero]);
|
||||
});
|
||||
|
||||
test("a threshold of 0 agrees with clearing the hide-dust checkbox", () => {
|
||||
const tx = nativeDustTransfer();
|
||||
const thresholdZero = filterTransactions(
|
||||
[tx],
|
||||
filters({ dustThresholdGwei: 0 }),
|
||||
);
|
||||
expect(result.transactions).toEqual([]);
|
||||
const toggleOff = filterTransactions(
|
||||
[tx],
|
||||
filters({ hideDustTransactions: false }),
|
||||
);
|
||||
expect(thresholdZero.transactions).toEqual([tx]);
|
||||
expect(toggleOff.transactions).toEqual([tx]);
|
||||
});
|
||||
|
||||
test("0, unset and a set threshold are three distinct behaviours", () => {
|
||||
const tx = dustOf(50);
|
||||
expect(
|
||||
filterTransactions([tx], filters({ dustThresholdGwei: 0 }))
|
||||
.transactions,
|
||||
).toEqual([tx]);
|
||||
expect(
|
||||
filterTransactions([tx], filters({ dustThresholdGwei: undefined }))
|
||||
.transactions,
|
||||
).toEqual([]);
|
||||
expect(
|
||||
filterTransactions([tx], filters({ dustThresholdGwei: 40 }))
|
||||
.transactions,
|
||||
).toEqual([tx]);
|
||||
expect(
|
||||
filterTransactions([tx], filters({ dustThresholdGwei: 60 }))
|
||||
.transactions,
|
||||
).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user