fix: Send refuses every amount the confirmation screen would refuse (closes #532)

Send accepted any amount parseFloat() read as above zero, so "1e-3",
"1abc" and an amount with more decimal places than the token has reached
ConfirmTx, which then refused them. isValidAmount() in
src/shared/txValidation.js is now the one check: Review and Max on Send
call it with the token's decimal places, and validateTransfer() calls it
for ConfirmTx, which now also refuses a zero amount and a token amount
with more places than the token has.

Model: opus-5-5

README.md

@@ -1686,7 +1686,10 @@ 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 no more
+      decimal places than the token has and never more than 18 (ETH's own),
+      which is also the check ConfirmTx makes before anything can be sent
     - "Back" → previous screen (Home, AddressDetail, or AddressToken)
 
 #### ConfirmTx (`confirm-tx`)

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,

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,

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,24 @@ 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 no more decimal places
+// than `tokenDecimals`, a token's own, and never more than the 18 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). 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 +131,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 +149,7 @@ function maxTokenAmount(tokenBalance) {
 function validateTransfer({
     isErc20 = false,
     amount,
+    tokenDecimals = null,
     ethBalance,
     tokenBalance,
     feeStatus = FEE_PENDING,
@@ -135,17 +157,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 +217,7 @@ module.exports = {
     SCALE_DECIMALS,
     feeReserveWei,
     feeEstimateWei,
+    isValidAmount,
     maxEthAmount,
     maxTokenAmount,
     toFixedPoint,

tests/sendAmountRule.test.js

@@ -0,0 +1,210 @@
+// 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"],
+])("%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);
+    });
+});

tests/txValidation.test.js

@@ -6,6 +6,7 @@ const {
     FEE_UNAVAILABLE,
     feeReserveWei,
     feeEstimateWei,
+    isValidAmount,
     maxEthAmount,
     maxTokenAmount,
     toFixedPoint,
@@ -33,6 +34,42 @@ 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 more decimal places than the token has", () => {
+        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("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 +171,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 +253,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]);