diff --git a/README.md b/README.md index c5e6a35..5d3029b 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]);