diff --git a/README.md b/README.md index edde902..8d973af 100644 --- a/README.md +++ b/README.md @@ -1173,8 +1173,8 @@ fetched, without that cross-address check, and shows the row's figure. **Decoded amount lines on the transaction approval screen:** the `Amount` line of a decoded ERC-20 call, and the `Amount` and `Min. received` lines of a decoded swap (see TxApproval below; `Min. received` reads `Min. paid out` unless -only the user is paid, see Transaction Decoding), do not always read as a -number. They can read: +only the user is paid and no allowance goes to a spender other than the router, +see Transaction Decoding), do not always read as a number. They can read: - A formatted quantity, e.g. `17.1900 USDT`, when the token's scale is known: truncated to four decimals, with the floor and the zero cases described above. @@ -3197,8 +3197,9 @@ Currently supported: `Min. paid out`, because the swap's minimum includes the fee. The minimum reads `Min. received` only when some step pays the transaction's sender (the From address on TxApproval, the sender the explorer reports on - TransactionDetail) and no `Paid to` line is shown. `Paid to` lines after - `Token Out` name each other address paid; one reads + TransactionDetail), no `Paid to` line is shown, and no permit or batch grants + an allowance to a spender other than the router (see below). `Paid to` lines + after `Token Out` name each other address paid; one reads `The bridge, which pays the deposit's recipient on another network` for a bridge deposit; one reads, for example, `The router, 2000.0000 USDT from the sender (later steps may not spend all of it)` @@ -3252,7 +3253,8 @@ Currently supported: (listed at the end of this section), a step whose input it cannot read, in whole or in part, and a step it fails on. Every other `PERMIT2_PERMIT`, before the step that sets the input side or after it, leaves `Token In` and the - `Amount` line as they read with no permit: a permit of 5,000 DAI to another + `Amount` line as they read with no permit, unless it lets a spender other than + the router take `Token In` (see below): a permit of 5,000 DAI to another address and then a V3 exact-in of 600 USDT read `Swap USDT → USDC` with `Amount 600.0000 USDT`, and so do that exact-in and then a permit of 5,000 DAI to the router. When a permit's amount stands and the step that sets the input @@ -3319,19 +3321,26 @@ Currently supported: `PERMIT2_PERMIT` or `PERMIT2_PERMIT_BATCH` whose input the decoder cannot read, such as one whose token or spender word has a nonzero top byte, may let any spender take any amount of any token: it gives one `Permit2 allowance` - line alone, reading `Unknown (could not be decoded)`, and leaves no `Amount` - line, whose figure might be too low. These lines change neither the `Paid to` - lines nor the minimum's label: a permit or batch pays nobody. A step goes - unread when its recipient does not decode as an address, when it is a V3 swap - step whose path is shorter than one token address or has bytes after its last - hop, or when it is a command the decoder does not read: every command other - than `PERMIT2_PERMIT`, `PERMIT2_PERMIT_BATCH`, `PERMIT2_TRANSFER_FROM`, - `V2_SWAP_EXACT_IN`, `V2_SWAP_EXACT_OUT`, `V3_SWAP_EXACT_IN`, - `V3_SWAP_EXACT_OUT`, `V4_SWAP`, `SWEEP`, `TRANSFER`, `PAY_PORTION`, - `PAY_PORTION_FULL_PRECISION`, `ACROSS_V4_DEPOSIT_V3`, `WRAP_ETH`, - `UNWRAP_WETH` and `BALANCE_CHECK_ERC20` (`PERMIT2_TRANSFER_FROM_BATCH`, - `EXECUTE_SUB_PLAN` and the NFT commands among them), or a type its release - names no command for. + line alone, reading `Unknown (could not be decoded)`. These lines change no + `Paid to` line: a permit or batch pays nobody. But code that a step runs, such + as a V4 pool's hook or a token's own code, can call a spender other than the + router, which can then take up to its allowance from the user's wallet before + the transaction ends. So an allowance to any spender but the router, or a + permit or batch the decoder cannot read, makes the minimum read + `Min. paid out`, and when its token is `Token In`, or the decoder does not + know its token or `Token In`, it leaves no `Amount` line, whose figure might + be too low: a permit of 5,000 USDT to another address and then a V3 exact-in + of 600 USDT read with no `Amount` line, and the 600 USDT gets lines of its + own. A step goes unread when its recipient does not decode as an address, when + it is a V3 swap step whose path is shorter than one token address or has bytes + after its last hop, or when it is a command the decoder does not read: every + command other than `PERMIT2_PERMIT`, `PERMIT2_PERMIT_BATCH`, + `PERMIT2_TRANSFER_FROM`, `V2_SWAP_EXACT_IN`, `V2_SWAP_EXACT_OUT`, + `V3_SWAP_EXACT_IN`, `V3_SWAP_EXACT_OUT`, `V4_SWAP`, `SWEEP`, `TRANSFER`, + `PAY_PORTION`, `PAY_PORTION_FULL_PRECISION`, `ACROSS_V4_DEPOSIT_V3`, + `WRAP_ETH`, `UNWRAP_WETH` and `BALANCE_CHECK_ERC20` + (`PERMIT2_TRANSFER_FROM_BATCH`, `EXECUTE_SUB_PLAN` and the NFT commands among + them), or a type its release names no command for. ### Non-Goals Forever diff --git a/src/shared/uniswap.js b/src/shared/uniswap.js index 31d12af..20a3920 100644 --- a/src/shared/uniswap.js +++ b/src/shared/uniswap.js @@ -1355,8 +1355,9 @@ function decode(data, toAddress, chainId, sources, fromAddress) { }; // A PERMIT2_PERMIT or PERMIT2_PERMIT_BATCH at `step` whose input this // decoder cannot read may let any spender take any amount of any - // token: one line says so, and there is no Amount line, whose figure - // may then be too low. + // token: one line says so, there is no Amount line, whose figure may + // then be too low, and the minimum reads Min. paid out (see + // toAnotherSpender). const permitUnread = (step) => { permits.push({ token: null, amount: null, spender: null, step }); stepsUnread.push(step); @@ -2029,8 +2030,9 @@ function decode(data, toAddress, chainId, sources, fromAddress) { // Every other PERMIT2_PERMIT leaves Token In and the Amount line as // they read with no permit, and gets lines of its own below. A step // that may take from the sender what this decoder did not read leaves - // no figure, permit or not (see spentUnread()), and so does a permit - // or batch whose input it cannot read (see permitUnread()). An + // no figure, permit or not (see spentUnread()), and so do a permit + // or batch whose input it cannot read (see permitUnread()) and an + // allowance of Token In to another spender (see toAnotherSpender). An // EXECUTE_SUB_PLAN step leaves both sides unknown (below). const stepCommands = Array.from( commandsBytes, @@ -2216,16 +2218,30 @@ function decode(data, toAddress, chainId, sources, fromAddress) { return amountText(amount, info); }; + // Each allowance to a spender other than the router. Code that a step + // runs, such as a V4 pool's hook or a token's own code, can call that + // spender, which can then take up to the allowance from the sender + // before the transaction ends. A permit or batch this decoder could + // not read may be to any spender, of any token. + const toAnotherSpender = permits.filter( + (p) => !sameAddress(p.spender, toAddress), + ); + // A null token in `spentFromSender`, a step this decoder did not read - // or a token it does not know, and one in `permits`, a permit or - // batch it could not read, leave no Amount figure, which may then be + // or a token it does not know, and an allowance to another spender + // that may be of Token In, leave no Amount figure, which may then be // too low. With no Amount line, each take the total adds up gets // lines of its own, first, so that every amount this decoder read is // shown. if ( present(inputAmount) && spentFromSender.every((s) => present(s.token)) && - permits.every((p) => present(p.token)) + toAnotherSpender.every( + (p) => + present(p.token) && + present(inputToken) && + !sameAddress(p.token, inputToken), + ) ) { const amount = spentText(inputAmount, inInfo); details.push({ @@ -2346,9 +2362,10 @@ function decode(data, toAddress, chainId, sources, fromAddress) { // step pays the bridge, when one moves the sender's tokens into the // router, when a token stays in the router, and when a payout could // not be read. The output is received by the sender only when some - // step pays the sender and there is no such line. When no step names - // anyone paid, one line says so. `paidTo` holds these lines and the - // token line after each line about a token in the router. + // step pays the sender, there is no such line, and no allowance goes + // to another spender (see toAnotherSpender). When no step names anyone + // paid, one line says so. `paidTo` holds these lines and the token + // line after each line about a token in the router. const outputHeld = present(outputToken) && heldByRouter.has(outputToken.toLowerCase()); const paidTo = paidElsewhere.map((address) => ({ @@ -2395,7 +2412,10 @@ function decode(data, toAddress, chainId, sources, fromAddress) { if (payeeUnread) { paidTo.push({ label: "Paid to", value: UNDECODED_CURRENCY }); } - const received = paidToSender && paidTo.length === 0; + const received = + paidToSender && + paidTo.length === 0 && + toAnotherSpender.length === 0; if (!paidToSender && paidTo.length === 0) { paidTo.push({ label: "Paid to", value: UNNAMED_CURRENCY }); } diff --git a/tests/uniswap.test.js b/tests/uniswap.test.js index 2abe38e..b2354f2 100644 --- a/tests/uniswap.test.js +++ b/tests/uniswap.test.js @@ -2108,12 +2108,6 @@ describe("uniswap decoder", () => { const ONE_USDT_LINES = (spender) => permitLines("1.0000 USDT", USDT_ADDR, "USDT", spender); test.each([ - { - kind: "to another spender", - commands: [0x0a], - permits: [ONE_USDT_PERMIT(OTHER_ADDR)], - lines: ONE_USDT_LINES(OTHER_ADDR), - }, { kind: "that the router lets fail", commands: [0x8a], @@ -2550,20 +2544,6 @@ describe("uniswap decoder", () => { ], lines: USDT_600_LINES, }, - { - kind: "a PERMIT2_PERMIT of 5,000 USDT to another spender, then a V3 exact-in of 600 USDT,", - commands: [0x0a, 0x00], - inputs: [ - encodePermit2(USDT_ADDR, USDT_5000, OTHER_ADDR), - V3_600, - ], - lines: permitLines( - "5000.0000 USDT", - USDT_ADDR, - "USDT", - OTHER_ADDR, - ), - }, { kind: "a PERMIT2_PERMIT of 5,000 USDT to the router that the router may let fail, then a V3 exact-in of 600 USDT,", commands: [0x8a, 0x00], @@ -2677,45 +2657,41 @@ describe("uniswap decoder", () => { const DAI_5000 = 5000000000000000000000n; const MAX_UINT160 = 2n ** 160n - 1n; - // A batch names one spender for all its allowances. - test.each([ - { kind: "the router", spender: ROUTER_ADDR }, - { kind: "another spender", spender: OTHER_ADDR }, - ])( - "a PERMIT2_PERMIT_BATCH of 5,000 USDT and unlimited DAI to $kind, then a V3 exact-in of 600 USDT, shows each allowance", - ({ spender }) => { - const result = decodeSteps( - [0x03, 0x00], - [ - encodePermit2Batch( - [ - [USDT_ADDR, USDT_5000], - [DAI_ADDR, MAX_UINT160], - ], - spender, - ), - V3_600, - ], - ); - expect(result.name).toBe("Swap USDT → USDC"); - expect(linesBetweenTokens(result)).toEqual([ - ["Amount", "600.0000 USDT"], - ...permitLines( - "5000.0000 USDT", - USDT_ADDR, - "USDT", - spender, + // A batch names one spender for all its allowances. One to + // another spender is under "an allowance to a spender other than + // the router" below. + test("a PERMIT2_PERMIT_BATCH of 5,000 USDT and unlimited DAI to the router, then a V3 exact-in of 600 USDT, shows each allowance", () => { + const result = decodeSteps( + [0x03, 0x00], + [ + encodePermit2Batch( + [ + [USDT_ADDR, USDT_5000], + [DAI_ADDR, MAX_UINT160], + ], + ROUTER_ADDR, ), - ...permitLines("Unlimited", DAI_ADDR, "DAI", spender), - ]); - // A batch pays nobody, so the swap's minimum is what the - // sender receives. - expect(detail(result, "Paid to")).toBeUndefined(); - expect(detail(result, "Min. received").value).toBe( - "590.0000 USDC", - ); - }, - ); + V3_600, + ], + ); + expect(result.name).toBe("Swap USDT → USDC"); + expect(linesBetweenTokens(result)).toEqual([ + ["Amount", "600.0000 USDT"], + ...permitLines( + "5000.0000 USDT", + USDT_ADDR, + "USDT", + ROUTER_ADDR, + ), + ...permitLines("Unlimited", DAI_ADDR, "DAI", ROUTER_ADDR), + ]); + // A batch pays nobody, so the swap's minimum is what the + // sender receives. + expect(detail(result, "Paid to")).toBeUndefined(); + expect(detail(result, "Min. received").value).toBe( + "590.0000 USDC", + ); + }); // A batch keeps a PERMIT2_PERMIT's amount from standing as the // Amount figure, before the permit as after it. @@ -2768,7 +2744,7 @@ describe("uniswap decoder", () => { ), }, ])( - "$kind that cannot be read, then a V3 exact-in of 600 USDT, says its allowances could not be decoded and shows no Amount figure", + "$kind that cannot be read, then a V3 exact-in of 600 USDT, says its allowances could not be decoded and shows no Amount figure and no Min. received", ({ command, input }) => { const result = decodeSteps( [command, 0x00], @@ -2778,6 +2754,14 @@ describe("uniswap decoder", () => { ...USDT_600_GROUP, ["Permit2 allowance", UNDECODED], ]); + // Its spender may be any address, as with an allowance + // to a spender other than the router (below). + // https://git.eeqj.de/sneak/AutistMask/issues/658 — on + // next at e296546 this read Min. received. + expect(detail(result, "Min. received")).toBeUndefined(); + expect(detail(result, "Min. paid out").value).toBe( + "590.0000 USDC", + ); }, ); @@ -2806,6 +2790,125 @@ describe("uniswap decoder", () => { }); }); + // https://git.eeqj.de/sneak/AutistMask/issues/658 — code that a step + // runs can call a spender other than the router, which can then take + // up to its allowance from the sender before the transaction ends. + // On next at e296546 the Amount figure and Min. received took no + // account of it. An allowance of Token In to another spender now + // leaves no Amount figure, the swap steps' own takes get lines of + // their own, and any allowance to another spender leaves no Min. + // received. + describe("an allowance to a spender other than the router", () => { + const USDT_5000 = 5000000000n; + const DAI_5000 = 5000000000000000000000n; + const MAX_UINT160 = 2n ** 160n - 1n; + const USDT_5000_LINES = permitLines( + "5000.0000 USDT", + USDT_ADDR, + "USDT", + OTHER_ADDR, + ); + + test.each([ + { + kind: "a PERMIT2_PERMIT of 5,000 USDT to another spender, then a V3 exact-in of 600 USDT,", + commands: [0x0a, 0x00], + inputs: [ + encodePermit2(USDT_ADDR, USDT_5000, OTHER_ADDR), + V3_600, + ], + lines: [...USDT_600_GROUP, ...USDT_5000_LINES], + minimum: "590.0000 USDC", + }, + { + kind: "a V3 exact-in of 600 USDT, then a PERMIT2_PERMIT of 5,000 USDT to another spender,", + commands: [0x00, 0x0a], + inputs: [ + V3_600, + encodePermit2(USDT_ADDR, USDT_5000, OTHER_ADDR), + ], + lines: [...USDT_600_GROUP, ...USDT_5000_LINES], + minimum: "590.0000 USDC", + }, + { + kind: "a PERMIT2_PERMIT_BATCH of 5,000 USDT and unlimited DAI to another spender, then a V3 exact-in of 600 USDT,", + commands: [0x03, 0x00], + inputs: [ + encodePermit2Batch( + [ + [USDT_ADDR, USDT_5000], + [DAI_ADDR, MAX_UINT160], + ], + OTHER_ADDR, + ), + V3_600, + ], + lines: [ + ...USDT_600_GROUP, + ...USDT_5000_LINES, + ...permitLines( + "Unlimited", + DAI_ADDR, + "DAI", + OTHER_ADDR, + ), + ], + minimum: "590.0000 USDC", + }, + { + kind: "a PERMIT2_PERMIT of 1 USDT to another spender, then a V3 exact-in of 600 USDT and a V2 exact-in of 400 USDT, both paid by the user,", + commands: [0x0a, 0x00, 0x08], + inputs: [ + ONE_USDT_PERMIT(OTHER_ADDR), + V3_600, + encodeV2SwapExactIn(USER_ADDR, USDT_400, 390000000n, [ + USDT_ADDR, + USDC_ADDR, + ]), + ], + lines: [ + ...USDT_600_GROUP, + ...USDT_400_GROUP, + ...ONE_USDT_LINES(OTHER_ADDR), + ], + // The last swap step's minimum. + minimum: "390.0000 USDC", + }, + ])( + "$kind shows each take's own lines and the allowances, and no Amount figure or Min. received", + ({ commands, inputs, lines, minimum }) => { + const result = decodeSteps(commands, inputs); + expect(result.name).toBe("Swap USDT → USDC"); + expect(linesBetweenTokens(result)).toEqual(lines); + expect(detail(result, "Paid to")).toBeUndefined(); + expect(detail(result, "Min. received")).toBeUndefined(); + expect(detail(result, "Min. paid out").value).toBe(minimum); + }, + ); + + // An allowance of another token leaves the Amount figure for + // Token In. + test("a PERMIT2_PERMIT of 5,000 DAI to another spender, then a V3 exact-in of 600 USDT, shows the swap's 600 USDT and no Min. received", () => { + const result = decodeSteps( + [0x0a, 0x00], + [encodePermit2(DAI_ADDR, DAI_5000, OTHER_ADDR), V3_600], + ); + expect(linesBetweenTokens(result)).toEqual([ + ["Amount", "600.0000 USDT"], + ...permitLines( + "5000.0000 DAI", + DAI_ADDR, + "DAI", + OTHER_ADDR, + ), + ]); + expect(detail(result, "Min. received")).toBeUndefined(); + expect(detail(result, "Min. paid out").value).toBe( + "590.0000 USDC", + ); + }); + }); + // A V3_SWAP_EXACT_OUT whose path is shorter than one token cannot be // read. It may pay any address and take from the sender, so it // leaves no Amount figure. diff --git a/tests/uniswapRecipient.test.js b/tests/uniswapRecipient.test.js index 5839033..3018ac8 100644 --- a/tests/uniswapRecipient.test.js +++ b/tests/uniswapRecipient.test.js @@ -2802,26 +2802,37 @@ describe("the allow-revert flag", () => { }); // Only the routers from UniversalRouterV2 on let a PERMIT2_PERMIT fail. + // The permit's spender is the router at ROUTER, which on every other + // router is an allowance to another spender, so the minimum reads + // Min. paid out there. test.each([ - { router: "UniversalRouterV1", address: ROUTER_V1, mark: "" }, + { + router: "UniversalRouterV1", + address: ROUTER_V1, + mark: "", + minimum: "Min. paid out", + }, { router: "UniversalRouterV1_2_V2Support", address: ROUTER_V1_2, mark: "", + minimum: "Min. paid out", }, { router: "UniversalRouterV2", address: ROUTER, mark: " (allowed to fail)", + minimum: "Min. received", }, { router: "UniversalRouterV2_1_2", address: ROUTER_V2_1_2, mark: " (allowed to fail)", + minimum: "Min. paid out", }, ])( "on $router, a PERMIT2_PERMIT with the flag is listed as 'Permit2 Permit$mark', and the swap's lines read as without the flag", - ({ address, mark }) => { + ({ address, mark, minimum }) => { const steps = [ permit2Permit(USDT, USDT_2000), swapToWeth(MSG_SENDER), @@ -2853,7 +2864,7 @@ describe("the allow-revert flag", () => { expect( labels(withFlag).filter((l) => permitLines.includes(l)), ).toEqual(permitLines); - expect(detail(withFlag, "Min. received").value).toBe("1.0000 WETH"); + expect(detail(withFlag, minimum).value).toBe("1.0000 WETH"); }, );