From 1f5ec1c8ded3587a87b8a17c851d0da13de87e13 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 18 Oct 2026 22:46:07 +0200 Subject: [PATCH] fix: Send refuses every amount the confirmation screen would refuse (closes #532) Send's Review accepted any amount parseFloat() read as above zero, so 1e-3, 1abc and amounts with too many decimal places reached ConfirmTx, which then refused them or failed to estimate the fee. isValidAmount() in src/shared/txValidation.js is now the one amount check, used by Send's Review and Max and by validateTransfer() for ConfirmTx: plain digits with at most one decimal point, above zero, nothing but zeros past the token's places. Zeros past those places are accepted, since the transfer encodes the amount exactly. Send now reads the token's decimal places before the ENS lookup, because the check needs them. Judgement call: ConfirmTx now also refuses zero, as Send already did. Model: opus-5-5 --- README.md | 8 +- src/popup/views/confirmTx.js | 1 + src/popup/views/send.js | 51 +++++---- src/shared/txValidation.js | 48 ++++++-- tests/sendAmountRule.test.js | 213 +++++++++++++++++++++++++++++++++++ tests/transferAmount.test.js | 6 + tests/txValidation.test.js | 65 +++++++++++ 7 files changed, 359 insertions(+), 33 deletions(-) create mode 100644 tests/sendAmountRule.test.js diff --git a/README.md b/README.md index d976a9a..a4f6f98 100644 --- a/README.md +++ b/README.md @@ -1686,7 +1686,13 @@ view would leave a wallet one click from deletion. - **Transitions**: - "Review" (valid inputs, ENS resolved) → **ConfirmTx** - "Review" with an unresolvable ENS name or an invalid amount → flash - message, no screen change + message, no screen change. An amount is valid when it is a number above + zero, written as digits with at most one decimal point, with nothing but + zeros after the token's last decimal place. ETH has 18 places, and a token + with more, or with an unknown number, is held to 18. Zeros past those + places leave the amount unchanged, so `1.0000000` of a 6-place token + passes and is sent as exactly 1. ConfirmTx makes the same check before + anything can be sent - "Back" → previous screen (Home, AddressDetail, or AddressToken) #### ConfirmTx (`confirm-tx`) diff --git a/src/popup/views/confirmTx.js b/src/popup/views/confirmTx.js index 216862a..90c74ae 100644 --- a/src/popup/views/confirmTx.js +++ b/src/popup/views/confirmTx.js @@ -278,6 +278,7 @@ function renderValidation(txInfo) { const { canSend, codes } = validateTransfer({ isErc20, amount: txInfo.amount, + tokenDecimals: txInfo.tokenDecimals, ethBalance: txInfo.balance, tokenBalance: txInfo.tokenBalance, feeStatus, diff --git a/src/popup/views/send.js b/src/popup/views/send.js index fe738bb..29350d6 100644 --- a/src/popup/views/send.js +++ b/src/popup/views/send.js @@ -24,6 +24,7 @@ const { } = require("../../shared/amountDisplay"); const { feeReserveWei, + isValidAmount, maxEthAmount, maxTokenAmount, } = require("../../shared/txValidation"); @@ -255,13 +256,18 @@ async function fillMaxAmount() { const token = state.selectedToken || $("send-token").value; if (token !== "ETH") { - const bal = tokenBalanceAndDecimals(addr, token).tokenBalance; + const { tokenBalance: bal, tokenDecimals } = tokenBalanceAndDecimals( + addr, + token, + ); if (bal == null) { showFlash("This token's balance is unknown."); return; } const amount = maxTokenAmount(bal); - if (!(parseFloat(amount) > 0)) { + // The balance is stored at the token's own decimal places and cut to + // 18 above, so the only balance this refuses is zero. + if (!isValidAmount(amount, tokenDecimals)) { showFlash("This token's balance is zero."); return; } @@ -367,7 +373,28 @@ function init(_ctx) { ); return; } - if (!amount || isNaN(parseFloat(amount)) || parseFloat(amount) <= 0) { + + const token = state.selectedToken || $("send-token").value; + const addr = currentAddress(); + + let tokenSymbol = null; + let tokenBalance = null; + let tokenDecimals = null; + if (token !== "ETH") { + tokenSymbol = resolveSymbol( + token, + addr.tokenBalances, + state.trackedTokens, + ); + ({ tokenBalance, tokenDecimals } = tokenBalanceAndDecimals( + addr, + token, + )); + } + + // The confirmation screen's own amount check, so it is never shown + // an amount it would refuse. + if (!isValidAmount(amount, tokenDecimals)) { showFlash("Please enter a valid amount."); return; } @@ -391,24 +418,6 @@ function init(_ctx) { } } - const token = state.selectedToken || $("send-token").value; - const addr = currentAddress(); - - let tokenSymbol = null; - let tokenBalance = null; - let tokenDecimals = null; - if (token !== "ETH") { - tokenSymbol = resolveSymbol( - token, - addr.tokenBalances, - state.trackedTokens, - ); - ({ tokenBalance, tokenDecimals } = tokenBalanceAndDecimals( - addr, - token, - )); - } - ctx.showConfirmTx({ from: addr.address, to: resolvedTo, diff --git a/src/shared/txValidation.js b/src/shared/txValidation.js index 6948692..4c9d5df 100644 --- a/src/shared/txValidation.js +++ b/src/shared/txValidation.js @@ -1,4 +1,5 @@ -// Balance arithmetic for the Send and transaction confirmation screens. +// The amount check and balance arithmetic for the Send and transaction +// confirmation screens. // // Pure: no DOM, no network, no state. Everything is exact integer math on // 18-decimal fixed point (wei for ETH), so it can be unit tested directly @@ -20,7 +21,7 @@ const FEE_KNOWN = "known"; const FEE_UNAVAILABLE = "unavailable"; const CODES = { - // The amount is not a non-negative number we can do exact arithmetic on. + // The amount is one isValidAmount() refuses. AMOUNT_INVALID: "amount-invalid", // ERC-20: the token amount exceeds the token balance. INSUFFICIENT_TOKEN: "insufficient-token", @@ -87,6 +88,27 @@ function toFixedPoint(value) { } } +// Whether `amount` can be sent: a number above zero, written as digits with +// at most one decimal point ("0.5", "12", ".5"), with nothing but zeros after +// the token's last decimal place, `tokenDecimals`, and nothing but zeros after +// the 18th, the places the arithmetic here works in, which are ETH's. ETH, and +// a token whose decimal places are unknown, pass null and are held to the 18 +// (the confirmation screen refuses to send such a token anyway). Zeros past +// those places are accepted because parseUnits() drops them here and again +// when the transfer is encoded, so "1.0000000" of a 6-place token is exactly +// 1 everywhere. Refuses "1e-3", "1abc", "0" and "-1". The Send screen checks +// it before the confirmation screen opens, and validateTransfer() checks it +// again there. +function isValidAmount(amount, tokenDecimals = null) { + if (typeof amount !== "string") return false; + const places = Math.min(tokenDecimals ?? SCALE_DECIMALS, SCALE_DECIMALS); + try { + return parseUnits(amount.trim(), places) > 0n; + } catch { + return false; + } +} + // The most ETH a send can carry: the exact balance minus the fee reserve from // feeReserveWei(), as a decimal string, so validateTransfer() passes it with // exactly that reserve left behind. `ethBalance` is the exact decimal string @@ -112,9 +134,11 @@ function maxTokenAmount(tokenBalance) { // Validate a pending transfer against the balances that must cover it. // // isErc20 — token transfer rather than a native ETH transfer -// amount — human decimal string being sent, non-negative. Anything -// else, a negative value included, is an unusable amount +// amount — human decimal string being sent. One isValidAmount() +// refuses, a negative value included, is an unusable amount // rather than an amount that passes every comparison. +// tokenDecimals — ERC-20: the token's decimal places, or null when they +// are unknown. Passed to isValidAmount(). // ethBalance — human decimal string, the sender's ETH balance // tokenBalance — human decimal string, the sender's token balance // feeStatus — FEE_PENDING, FEE_KNOWN or FEE_UNAVAILABLE. Anything else @@ -128,6 +152,7 @@ function maxTokenAmount(tokenBalance) { function validateTransfer({ isErc20 = false, amount, + tokenDecimals = null, ethBalance, tokenBalance, feeStatus = FEE_PENDING, @@ -135,17 +160,17 @@ function validateTransfer({ } = {}) { const codes = []; - const amountFp = toFixedPoint(amount); - const ethFp = toFixedPoint(ethBalance) ?? 0n; - - // A negative amount parses to a valid bigint, so every comparison below - // is trivially false and the send clears the screen — then dies at encode - // time in parseEther(). Unusable, on the same footing as a malformed fee. - if (amountFp === null || amountFp < 0n) { + // An amount refused here never reaches the comparisons below: a negative + // one would pass every one of them, then die at encode time in + // parseEther(). + if (!isValidAmount(amount, isErc20 ? tokenDecimals : null)) { codes.push(CODES.AMOUNT_INVALID); return { canSend: false, codes }; } + const amountFp = toFixedPoint(amount); + const ethFp = toFixedPoint(ethBalance) ?? 0n; + // Fail closed. Anything that is not a usable fee under a recognised // status — a malformed feeWei, or a status this module does not know — // is an unavailable estimate, never a fee of zero. Every such input errs @@ -195,6 +220,7 @@ module.exports = { SCALE_DECIMALS, feeReserveWei, feeEstimateWei, + isValidAmount, maxEthAmount, maxTokenAmount, toFixedPoint, diff --git a/tests/sendAmountRule.test.js b/tests/sendAmountRule.test.js new file mode 100644 index 0000000..407b13c --- /dev/null +++ b/tests/sendAmountRule.test.js @@ -0,0 +1,213 @@ +// The Send screen refuses every amount the confirmation screen would refuse, +// by the same check (https://git.eeqj.de/sneak/AutistMask/issues/532). +// +// Send used to accept anything parseFloat() read as above zero, so "1e-3", +// "1abc" and an amount with more decimal places than the token has went on to +// a confirmation screen that then refused them. Both screens now call +// isValidAmount() in src/shared/txValidation.js. +// +// Driven through the real Send review handler and the real confirmation +// screen, with only the node and the DOM stubbed. + +"use strict"; + +jest.mock("../src/shared/log", () => ({ + log: { + debugf: () => {}, + infof: () => {}, + warnf: () => {}, + errorf: () => {}, + }, + debugFetch: jest.fn(), + setRuntimeDebug: () => {}, + isDebug: () => false, +})); + +// What the confirmation screen asks the node for. +const mockProvider = { + getFeeData: async () => ({ + maxFeePerGas: 2000000000n, + gasPrice: 1000000000n, + }), + estimateGas: async () => 21000n, + getCode: async () => "0x", + getTransactionCount: async () => 1, + getBalance: async () => 0n, +}; + +jest.mock("../src/shared/balances", () => { + const actual = jest.requireActual("../src/shared/balances"); + return { ...actual, getProvider: () => mockProvider }; +}); + +// The confirmation screen's Etherscan label lookup fails, as it does offline. +global.fetch = jest.fn(() => { + throw new Error("tests must not perform network requests"); +}); + +const { makeStorageStub } = require("./support/storageStub"); +global.chrome = { storage: makeStorageStub(), runtime: { sendMessage() {} } }; + +// A stub DOM: every id resolves to a recording element. +const elements = new Map(); + +function makeEl(id) { + const handlers = new Map(); + return { + id, + textContent: "", + innerHTML: "", + value: "", + disabled: false, + style: {}, + dataset: {}, + classList: { + add() {}, + remove() {}, + toggle() {}, + contains: () => false, + }, + handlers, + addEventListener(name, fn) { + handlers.set(name, fn); + }, + appendChild(child) { + return child; + }, + querySelectorAll: () => [], + querySelector: () => null, + remove() {}, + focus() {}, + }; +} + +global.document = { + getElementById(id) { + if (!elements.has(id)) elements.set(id, makeEl(id)); + return elements.get(id); + }, + createElement: (tag) => makeEl(tag), + body: { prepend() {}, appendChild() {} }, + addEventListener() {}, +}; +global.navigator = { clipboard: { writeText() {} } }; + +const { state } = require("../src/shared/state"); +const send = require("../src/popup/views/send"); +const confirmTx = require("../src/popup/views/confirmTx"); + +const HOLDER = "0x" + "a".repeat(40); +const RECIPIENT = "0xC0FfEE0000000000000000000000000000c0fFEe"; +// A token with 6 decimal places, as USDC has. +const TOKEN = "0x" + "d".repeat(40); +const TOKEN_BY_SYMBOL = { ETH: "ETH", TOK: TOKEN }; + +function el(id) { + return global.document.getElementById(id); +} + +// Press Review on the Send screen with `amount` entered, for a send of `token` +// ("ETH" or TOKEN), and return what it handed the confirmation screen, or null +// when it handed nothing over. +async function review(amount, token) { + let handed = null; + send.init({ showConfirmTx: (info) => (handed = info) }); + state.selectedToken = token; + el("send-to").value = RECIPIENT; + el("send-amount").value = amount; + await el("btn-send-review").handlers.get("click")(); + return handed; +} + +// Show the confirmation screen for the send Review hands over for 0.5, with +// `amount` in its place, and let its fee estimate settle. +async function confirm(amount, token) { + const txInfo = await review("0.5", token); + confirmTx.show({ ...txInfo, amount }); + for (let i = 0; i < 10; i++) await new Promise((r) => setTimeout(r, 0)); +} + +beforeEach(() => { + elements.clear(); + state.wallets = [ + { + name: "Wallet 1", + addresses: [ + { + address: HOLDER, + balance: "1.0", + tokenBalances: [ + { + address: TOKEN, + name: "Token", + symbol: "TOK", + decimals: 6, + balance: "250.0", + holders: 50000, + }, + ], + }, + ], + }, + ]; + state.selectedWallet = 0; + state.selectedAddress = 0; + state.trackedTokens = [ + { address: TOKEN, symbol: "TOK", name: "Token", decimals: 6 }, + ]; + state.fraudContracts = []; + state.selectedToken = null; + state.currentView = null; +}); + +describe.each([ + ["1e-3", "ETH"], + ["1e-3", "TOK"], + ["1abc", "ETH"], + // 19 decimal places; ETH has 18. + ["0.0000000000000000001", "ETH"], + // 7 decimal places; the token has 6. + ["0.0000001", "TOK"], + ["0", "ETH"], + ["-1", "ETH"], +])("%s %s", (amount, symbol) => { + const token = TOKEN_BY_SYMBOL[symbol]; + + test("Send refuses it with a flash message and opens nothing", async () => { + expect(await review(amount, token)).toBe(null); + expect(el("flash-msg").textContent).toBe( + "Please enter a valid amount.", + ); + }); + + test("the confirmation screen refuses it too", async () => { + await confirm(amount, token); + expect(el("confirm-errors").innerHTML).toContain( + "Please enter a valid amount to send.", + ); + expect(el("btn-confirm-send").disabled).toBe(true); + }); +}); + +describe.each([ + ["0.5", "ETH"], + ["0.5", "TOK"], + // Exactly the 6 decimal places the token has. + ["0.000001", "TOK"], + // Only zeros past the token's 6 places, and past ETH's 18. + ["1.0000000", "TOK"], + ["0.5000000000000000000", "ETH"], +])("%s %s", (amount, symbol) => { + const token = TOKEN_BY_SYMBOL[symbol]; + + test("Send hands it to the confirmation screen", async () => { + expect((await review(amount, token)).amount).toBe(amount); + expect(el("flash-msg").textContent).toBe(""); + }); + + test("the confirmation screen lets it be sent", async () => { + await confirm(amount, token); + expect(el("confirm-errors").innerHTML).toBe(""); + expect(el("btn-confirm-send").disabled).toBe(false); + }); +}); diff --git a/tests/transferAmount.test.js b/tests/transferAmount.test.js index 16bce0e..540a6c6 100644 --- a/tests/transferAmount.test.js +++ b/tests/transferAmount.test.js @@ -116,6 +116,12 @@ describe("transferAmountUnits", () => { expect(() => transferAmountUnits("0.0000001", 6, 6n)).toThrow(); }); + test("encodes zeros past the token's scale as the amount typed", () => { + // The Send screen accepts "1.0000000" for a 6-place token, so the + // transfer must move exactly 1 of it. + expect(transferAmountUnits("1.0000000", 6, 6n)).toBe(1000000n); + }); + test("every refusal is a full sentence", () => { const messages = []; for (const args of [ diff --git a/tests/txValidation.test.js b/tests/txValidation.test.js index f08039a..0de250e 100644 --- a/tests/txValidation.test.js +++ b/tests/txValidation.test.js @@ -6,6 +6,7 @@ const { FEE_UNAVAILABLE, feeReserveWei, feeEstimateWei, + isValidAmount, maxEthAmount, maxTokenAmount, toFixedPoint, @@ -33,6 +34,49 @@ describe("toFixedPoint", () => { }); }); +// The one amount check the Send screen and the confirmation screen share. +// The screens themselves are driven in tests/sendAmountRule.test.js. +describe("isValidAmount", () => { + test("accepts a number above zero written as digits and a decimal point", () => { + expect(isValidAmount("0.5")).toBe(true); + expect(isValidAmount("12")).toBe(true); + expect(isValidAmount(".5")).toBe(true); + expect(isValidAmount("0.000000000000000001")).toBe(true); + }); + + test("refuses an exponent, trailing text, zero and a negative amount", () => { + expect(isValidAmount("1e-3")).toBe(false); + expect(isValidAmount("1abc")).toBe(false); + expect(isValidAmount("0")).toBe(false); + expect(isValidAmount("0.0")).toBe(false); + expect(isValidAmount("-1")).toBe(false); + expect(isValidAmount("")).toBe(false); + expect(isValidAmount(undefined)).toBe(false); + }); + + test("refuses a digit other than 0 past the token's places", () => { + expect(isValidAmount("0.000001", 6)).toBe(true); + expect(isValidAmount("0.0000001", 6)).toBe(false); + expect(isValidAmount("5", 0)).toBe(true); + expect(isValidAmount("0.5", 0)).toBe(false); + }); + + test("accepts zeros past the token's places, which leave the amount unchanged", () => { + expect(isValidAmount("1.0000000", 6)).toBe(true); + expect(isValidAmount("5.0", 0)).toBe(true); + expect(isValidAmount("1.0000000000000000000")).toBe(true); + expect(isValidAmount("0.0000000", 6)).toBe(false); + }); + + test("holds ETH, a token of unknown places, and any token to 18 places", () => { + const nineteen = "0.0000000000000000001"; + expect(isValidAmount(nineteen)).toBe(false); + expect(isValidAmount(nineteen, null)).toBe(false); + expect(isValidAmount(nineteen, 24)).toBe(false); + expect(isValidAmount("0.000000000000000001", 24)).toBe(true); + }); +}); + describe("validateTransfer, native ETH", () => { const eth = (over) => ({ isErc20: false, @@ -134,6 +178,17 @@ describe("validateTransfer, native ETH", () => { ).toEqual({ canSend: false, codes: [CODES.AMOUNT_INVALID] }); }); + test("rejects a zero amount and an exponent", () => { + expect(validateTransfer(eth({ amount: "0" }))).toEqual({ + canSend: false, + codes: [CODES.AMOUNT_INVALID], + }); + expect(validateTransfer(eth({ amount: "1e-3" }))).toEqual({ + canSend: false, + codes: [CODES.AMOUNT_INVALID], + }); + }); + test("treats a missing balance as zero, not as unlimited", () => { const r = validateTransfer(eth({ ethBalance: undefined })); expect(r.codes).toEqual([CODES.INSUFFICIENT_ETH]); @@ -205,6 +260,16 @@ describe("validateTransfer, ERC-20", () => { expect(r).toEqual({ canSend: false, codes: [CODES.AMOUNT_INVALID] }); }); + test("rejects more decimal places than the token has", () => { + const r = validateTransfer( + erc20({ amount: "0.0000001", tokenDecimals: 6 }), + ); + expect(r).toEqual({ canSend: false, codes: [CODES.AMOUNT_INVALID] }); + expect( + validateTransfer(erc20({ amount: "0.000001", tokenDecimals: 6 })), + ).toEqual({ canSend: true, codes: [] }); + }); + test("treats a missing token balance as zero", () => { const r = validateTransfer(erc20({ tokenBalance: undefined })); expect(r.codes).toEqual([CODES.INSUFFICIENT_TOKEN]);