fix: a balance check of another address sets no swap minimum (closes #600)

BALANCE_CHECK_ERC20 takes (owner, token, minBalance), and every Universal
Router release that has it checks the owner's balance as written, without
map(), so address(1) and address(2) there are plain addresses. The decoder
read only the token and the minimum, so a check of another address's balance
read as Min. received. A check whose owner is not the transaction's sender now
sets neither Token Out nor the minimum, and its step reads "Balance Check (of
another address)"; one whose input does not decode reads "Balance Check (could
not be decoded)". A check of the sender keeps its reading, including the zero
reading for one the router lets fail.

Model: opus-5-5

README.md

@@ -1198,11 +1198,13 @@ number. They can read:
   guarantees nothing: it sets `Token Out` and `Min. received` only when the
   output side holds no minimum, not even a zero one, at the point the check is
   reached, and otherwise leaves the current token and figure in place. A nonzero
-  `minBalance` sets both lines, as a swap step does. A `BALANCE_CHECK_ERC20`
-  step that the router lets fail without reverting the transaction (see
-  Transaction Decoding) guarantees nothing either, whatever its `minBalance`,
-  and is read as one whose `minBalance` is zero. A minimum stated by a V4 swap
-  step the router lets fail is likewise read as zero.
+  `minBalance` sets both lines, as a swap step does. Only a check of the
+  transaction's sender's own balance sets either line; a check of any other
+  address sets neither (see Transaction Decoding). A `BALANCE_CHECK_ERC20` step
+  that the router lets fail without reverting the transaction (see Transaction
+  Decoding) guarantees nothing either, whatever its `minBalance`, and is read as
+  one whose `minBalance` is zero. A minimum stated by a V4 swap step the router
+  lets fail is likewise read as zero.
 
 The swap's `Token In` and `Token Out` lines name a currency, not an amount; each
 reads `Unknown (not named in the calldata)` when the decoder found no token for
@@ -3018,11 +3020,20 @@ Currently supported:
   `ACROSS_V4_DEPOSIT_V3` pays the bridge, which pays the deposit's recipient on
   another network and refunds a deposit nobody fills to its depositor; the
   router passes both on as written, so `address(1)` and `address(2)` there are
-  addresses, and each that is not the transaction's sender is named. When a swap
-  step, `TAKE` or `TAKE_PORTION` pays the router the swap's output token, the
-  output stays in the router unless a later step pays all the router holds of it
-  to anyone but the router: a `SWEEP` of that token, an `UNWRAP_WETH` when it is
-  WETH, a `TRANSFER` of `CONTRACT_BALANCE`, a `PAY_PORTION` of 10,000 bips or a
+  addresses, and each that is not the transaction's sender is named. A
+  `BALANCE_CHECK_ERC20` pays nobody: the router checks that its owner holds at
+  least `minBalance` of its token, and reads the owner as written, so
+  `address(1)` and `address(2)` there are addresses too. A check of any address
+  but the transaction's sender's says nothing about what the user receives: it
+  sets neither `Token Out` nor the minimum, and it is listed as
+  `Balance Check (of another address)`. A check whose input does not decode,
+  such as one whose owner word has a nonzero top byte (the router reads the
+  owner from the word's last 20 bytes), likewise sets neither, and it is listed
+  as `Balance Check (could not be decoded)`. When a swap step, `TAKE` or
+  `TAKE_PORTION` pays the router the swap's output token, the output stays in
+  the router unless a later step pays all the router holds of it to anyone but
+  the router: a `SWEEP` of that token, an `UNWRAP_WETH` when it is WETH, a
+  `TRANSFER` of `CONTRACT_BALANCE`, a `PAY_PORTION` of 10,000 bips or a
   `PAY_PORTION_FULL_PRECISION` of 1e18 of that token, or an
   `ACROSS_V4_DEPOSIT_V3` of `CONTRACT_BALANCE` of that token, or of ETH when it
   sets `useNative`. The router sends ETH as written, so a `TRANSFER` of

src/shared/uniswap.js

@@ -388,7 +388,13 @@ function decodePermit2(input) {
     }
 }
 
-// Decode BALANCE_CHECK_ERC20 (command 0x0e) input bytes.
+// Decode BALANCE_CHECK_ERC20 (command 0x0e) input bytes. The router passes the
+// check when `owner` holds at least `minBalance` of `token`, and reads `owner`
+// as written, without map(), on every release that has the command:
+// - UniversalRouterV1_2_V2Support: https://github.com/Uniswap/universal-router/blob/41183d6eb154f0ab0e74a0e911a5ef9ea51fc4bd/contracts/base/Dispatcher.sol
+// - UniversalRouterV2: https://github.com/Uniswap/universal-router/blob/3663f6db6e2fe121753cd2d899699c2dc75dca86/contracts/base/Dispatcher.sol
+// - UniversalRouterV2_1_1 and UniversalRouterV2_1_2, the same file at both
+//   commits: https://github.com/Uniswap/universal-router/blob/802fe4c18f47300e0f183e2a42e9146ec2ea9fc3/contracts/base/Dispatcher.sol
 // ABI: (address owner, address token, uint256 minBalance)
 function decodeBalanceCheck(input) {
     try {
@@ -1087,15 +1093,33 @@ function decode(data, toAddress, chainId, sources, fromAddress) {
                     }
                 } else if (cmdId === 0x0e) {
                     const b = decodeBalanceCheck(inputs[i]);
-                    // The router passes this check whenever the owner holds at
-                    // least minBalance, so a zero one guarantees nothing and
-                    // does not replace a minimum an earlier step stated. A
-                    // check the router lets fail guarantees nothing either, so
-                    // it is read as a zero one. Any other minBalance sets the
-                    // output side as a swap does. It pays nobody.
-                    const minBalance = b && !allowedToFail ? b.minBalance : 0n;
-                    if (b && !(minBalance === 0n && present(minOutput))) {
-                        setOutput(b.token, minBalance);
+                    if (!b) {
+                        // The router still runs a check whose input does not
+                        // decode, such as an owner word with a nonzero top
+                        // byte, of which it reads the last 20 bytes, so the
+                        // owner may be any address. It sets no side, and the
+                        // step says so.
+                        commandNames[i] += " (could not be decoded)";
+                    } else if (b.owner.toLowerCase() !== sender) {
+                        // The router reads the owner as written (see
+                        // decodeBalanceCheck()), so address(1) and address(2)
+                        // are not the sender and the router here. A check of
+                        // any address but the sender's says nothing about
+                        // what the user receives: it sets no side, and the
+                        // step says so.
+                        commandNames[i] += " (of another address)";
+                    } else {
+                        // The router passes this check whenever the owner
+                        // holds at least minBalance, so a zero one guarantees
+                        // nothing and does not replace a minimum an earlier
+                        // step stated. A check the router lets fail guarantees
+                        // nothing either, so it is read as a zero one. Any
+                        // other minBalance sets the output side as a swap
+                        // does. It pays nobody.
+                        const minBalance = allowedToFail ? 0n : b.minBalance;
+                        if (!(minBalance === 0n && present(minOutput))) {
+                            setOutput(b.token, minBalance);
+                        }
                     }
                 } else if (cmdId === 0x00) {
                     const s = decodeV3SwapExactIn(inputs[i]);

tests/uniswapRecipient.test.js

@@ -1,8 +1,10 @@
 // The address a swap's output is paid to
 // (https://git.eeqj.de/sneak/AutistMask/issues/594 and
-// https://git.eeqj.de/sneak/AutistMask/issues/599), and the allow-revert flag
+// https://git.eeqj.de/sneak/AutistMask/issues/599), the allow-revert flag
 // on the steps that pay it
-// (https://git.eeqj.de/sneak/AutistMask/issues/602).
+// (https://git.eeqj.de/sneak/AutistMask/issues/602), and the address whose
+// balance a BALANCE_CHECK_ERC20 checks
+// (https://git.eeqj.de/sneak/AutistMask/issues/600).
 //
 // Every swap command, SWEEP, WRAP_ETH, UNWRAP_WETH, TRANSFER, PAY_PORTION,
 // PAY_PORTION_FULL_PRECISION, PERMIT2_TRANSFER_FROM, ACROSS_V4_DEPOSIT_V3 and
@@ -47,6 +49,8 @@ const ROUTER = "0x66a9893cc07d91d95644aedd05d03f95e1dba8af";
 const ROUTER_V2_1_2 = "0x23617e59A5925b2A4Bf75d73ff6711cD0b29De85";
 // UniversalRouterV1 on Ethereum Mainnet.
 const ROUTER_V1 = "0xEf1c6E67703c7BD7107eed8303Fbe6EC2554BF6B";
+// UniversalRouterV1_2_V2Support on Ethereum Mainnet.
+const ROUTER_V1_2 = "0x3fC91A3afd70395Cd496C647d5a6CC9D4B2b7FAD";
 const USER = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
 const OTHER = "0x7777777777777777777777777777777777777777";
 const MSG_SENDER = "0x0000000000000000000000000000000000000001";
@@ -219,16 +223,20 @@ function unwrapWeth(recipient) {
     return [0x0c, coder.encode(["address", "uint256"], [recipient, 0n])];
 }
 
-function balanceCheck(token, minBalance) {
+// BALANCE_CHECK_ERC20 (0x0e) of `owner`'s balance of `token`.
+function balanceCheck(token, minBalance, owner = USER) {
     return [
         0x0e,
         coder.encode(
             ["address", "address", "uint256"],
-            [USER, token, minBalance],
+            [owner, token, minBalance],
         ),
     ];
 }
 
+// `step` with the allow-revert flag set on its command byte.
+const flagged = ([command, input]) => [command | 0x80, input];
+
 // `step`, a [command or action, input] pair, with the top byte of the input's
 // word `index` set. The decoder cannot read that word as an address.
 function withDirtyWord([id, input], index) {
@@ -1383,15 +1391,11 @@ describe("paths whose recipients were untested", () => {
 // and goes on to the next step only for some commands, none of them a swap or
 // a payment; on every other command the flag has no effect.
 describe("the allow-revert flag", () => {
-    // UniversalRouterV1_2_V2Support on Ethereum Mainnet.
-    const ROUTER_V1_2 = "0x3fC91A3afd70395Cd496C647d5a6CC9D4B2b7FAD";
     // UniversalRouterV2_1_1 on Ethereum Mainnet.
     const ROUTER_V2_1_1 = "0x4C82D1fBFe28C977cBB58D8C7FF8FCF9F70a2cCA";
     // The WETH/USDC pair on Ethereum Mainnet, as its V2 factory reports it.
     const PAIR = "0xB4e16d0168e52d35CaCD2c6185b44281Ec28C9Dc";
 
-    // `step` with the allow-revert flag set on its command byte.
-    const flagged = ([command, input]) => [command | 0x80, input];
     // `steps` with the flag set on the one at `index`.
     const flagStep = (steps, index) =>
         steps.map((s, i) => (i === index ? flagged(s) : s));
@@ -1714,6 +1718,104 @@ describe("the allow-revert flag", () => {
     );
 });
 
+// https://git.eeqj.de/sneak/AutistMask/issues/600: on every release that has
+// BALANCE_CHECK_ERC20, the router checks the balance of the owner as written,
+// without map(), so address(1) and address(2) there are those addresses, not
+// the sender and the router. Only a check of the sender's own balance says
+// anything about what the user receives.
+describe.each([
+    { router: "UniversalRouterV1_2_V2Support", address: ROUTER_V1_2 },
+    { router: "UniversalRouterV2", address: ROUTER },
+    { router: "UniversalRouterV2_1_2", address: ROUTER_V2_1_2 },
+])("on $router, the owner of a BALANCE_CHECK_ERC20", ({ address }) => {
+    // A swap of 2,000 USDT for WETH with no minimum, paid to the user.
+    const swap = v3SwapExactIn(MSG_SENDER, USDT_2000, 0n, USDT, WETH);
+    const NO_MINIMUM = "None (no minimum guaranteed)";
+
+    test("the sender: the check sets the minimum, unless the router lets it fail", () => {
+        const check = balanceCheck(WETH, ONE_WETH, USER);
+
+        const plain = decode(execute(swap, check), USER, address);
+        expect(detail(plain, "Steps").value).toBe(
+            "V3 Swap (Exact In) → Balance Check",
+        );
+        expect(detail(plain, "Min. received").value).toBe("1.0000 WETH");
+        // The sender as the From address gives it, in lower case.
+        expect(
+            decode(execute(swap, check), USER.toLowerCase(), address),
+        ).toEqual(plain);
+
+        const withFlag = decode(execute(swap, flagged(check)), USER, address);
+        expect(detail(withFlag, "Steps").value).toBe(
+            "V3 Swap (Exact In) → Balance Check (allowed to fail)",
+        );
+        expect(detail(withFlag, "Min. received").value).toBe(NO_MINIMUM);
+    });
+
+    const notTheSender = [
+        { kind: "address(1)", owner: MSG_SENDER },
+        { kind: "address(2)", owner: ADDRESS_THIS },
+        { kind: "another address", owner: OTHER },
+    ];
+
+    test.each(notTheSender)(
+        "$kind: after a swap paid to the user, the check sets no minimum and is said to be of another address",
+        ({ owner }) => {
+            const check = balanceCheck(WETH, ONE_WETH, owner);
+
+            const plain = decode(execute(swap, check), USER, address);
+            expect(detail(plain, "Steps").value).toBe(
+                "V3 Swap (Exact In) → Balance Check (of another address)",
+            );
+            expect(paidTo(plain)).toEqual([]);
+            expect(detail(plain, "Min. received").value).toBe(NO_MINIMUM);
+
+            const withFlag = decode(
+                execute(swap, flagged(check)),
+                USER,
+                address,
+            );
+            expect(detail(withFlag, "Steps").value).toBe(
+                "V3 Swap (Exact In) → Balance Check (of another address) (allowed to fail)",
+            );
+            expect(paidTo(withFlag)).toEqual([]);
+            expect(detail(withFlag, "Min. received").value).toBe(NO_MINIMUM);
+        },
+    );
+
+    test.each(notTheSender)(
+        "$kind: a check alone names no output token and states no minimum",
+        ({ owner }) => {
+            const check = balanceCheck(WETH, ONE_WETH, owner);
+
+            for (const step of [check, flagged(check)]) {
+                const result = decode(execute(step), USER, address);
+                expect(detail(result, "Token Out").value).toBe(UNNAMED);
+                expect(detail(result, "Min. received")).toBeUndefined();
+                expect(detail(result, "Min. paid out")).toBeUndefined();
+            }
+        },
+    );
+
+    test("an owner word with a nonzero top byte: the check is said not to decode and sets no minimum", () => {
+        // The router checks the balance of the address in the word's last 20
+        // bytes, here OTHER's.
+        const check = withDirtyWord(balanceCheck(WETH, ONE_WETH, OTHER), 0);
+
+        const plain = decode(execute(swap, check), USER, address);
+        expect(detail(plain, "Steps").value).toBe(
+            "V3 Swap (Exact In) → Balance Check (could not be decoded)",
+        );
+        expect(detail(plain, "Min. received").value).toBe(NO_MINIMUM);
+
+        const withFlag = decode(execute(swap, flagged(check)), USER, address);
+        expect(detail(withFlag, "Steps").value).toBe(
+            "V3 Swap (Exact In) → Balance Check (could not be decoded) (allowed to fail)",
+        );
+        expect(detail(withFlag, "Min. received").value).toBe(NO_MINIMUM);
+    });
+});
+
 // The DOM stub both screens are drawn into.
 function makeElement(id) {
     const classes = new Set();

tests/uniswapUndeterminedTokenIn.test.js

@@ -71,6 +71,8 @@ function exactInParams(currencyIn, path, amountIn, amountOutMin) {
 
 // BALANCE_CHECK_ERC20 (command 0x0e): (address owner, address token,
 // uint256 minBalance). It names the output token and nothing about the input.
+// Its owner is USER, the transaction's sender: a check of any other address
+// names no output token.
 function balanceCheck(token, minBalance) {
     return coder.encode(
         ["address", "address", "uint256"],
@@ -78,8 +80,13 @@ function balanceCheck(token, minBalance) {
     );
 }
 
+// A transaction from USER.
+function decode(data) {
+    return uniswap.decode(data, ROUTER, "0x1", {}, USER);
+}
+
 function detail(data, label) {
-    const decoded = uniswap.decode(data, ROUTER, "0x1", {});
+    const decoded = decode(data);
     expect(decoded).not.toBeNull();
     return decoded.details.find((d) => d.label === label);
 }
@@ -113,9 +120,7 @@ describe("an execute() whose input currency never decoded", () => {
     });
 
     test("does not name ETH in the swap title either", () => {
-        expect(uniswap.decode(data(), ROUTER, "0x1", {}).name).toBe(
-            "Uniswap Swap",
-        );
+        expect(decode(data()).name).toBe("Uniswap Swap");
     });
 });
 
@@ -125,9 +130,7 @@ describe("an execute() that names only an output token", () => {
 
     test("refuses the input rather than defaulting it to ETH", () => {
         expect(detail(data(), "Token In").value).toBe(REFUSAL);
-        expect(uniswap.decode(data(), ROUTER, "0x1", {}).name).toBe(
-            "Uniswap Swap",
-        );
+        expect(decode(data()).name).toBe("Uniswap Swap");
     });
 
     test("still states the output it did establish", () => {
@@ -175,9 +178,7 @@ describe("native ETH in, which V4 spells as the zero address", () => {
 
         expect(detail(data, "Token In").value).toBe("ETH (native)");
         expect(detail(data, "Amount").value).toBe("1.0000 ETH");
-        expect(uniswap.decode(data, ROUTER, "0x1", {}).name).toBe(
-            "Swap ETH → USDT",
-        );
+        expect(decode(data).name).toBe("Swap ETH → USDT");
     });
 
     test("WRAP_ETH is still ETH at 18 decimals", () => {