diff --git a/README.md b/README.md index db9fc7b..2bee4fd 100644 --- a/README.md +++ b/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 diff --git a/src/shared/uniswap.js b/src/shared/uniswap.js index 787d2ba..88c5a6a 100644 --- a/src/shared/uniswap.js +++ b/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]); diff --git a/tests/uniswapRecipient.test.js b/tests/uniswapRecipient.test.js index c1bebdb..9295b56 100644 --- a/tests/uniswapRecipient.test.js +++ b/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(); diff --git a/tests/uniswapUndeterminedTokenIn.test.js b/tests/uniswapUndeterminedTokenIn.test.js index bb70995..ad1f2d1 100644 --- a/tests/uniswapUndeterminedTokenIn.test.js +++ b/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", () => {