diff --git a/README.md b/README.md index a4505e2..751d174 100644 --- a/README.md +++ b/README.md @@ -2160,6 +2160,13 @@ view would leave a wallet one click from deletion. - Typed data that states no primary type, or one other than the type it would be signed as, or that cannot be read → shown with the error line saying so and "Sign" disabled; only "Reject" remains + - Typed data whose domain `chainId` names a chain other than the active + network's → shown with the error line naming both chain ids and "Sign" + disabled; only "Reject" remains. EIP-712 asks for this, because the + signature is valid on the chain the domain names. Typed data with no + `chainId` is not refused. The background refuses such a signature too, + against the network active when the signature reaches it, and answers the + site with the same sentence - A `personal_sign` or `eth_sign` message that is not hex (`0x` or `0X` and an even number of hex digits, the form ethers' `getBytes` reads when signing) → shown as plain text, with the error line "This message is plain diff --git a/src/background/index.js b/src/background/index.js index d5a6fc4..8620482 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -22,6 +22,7 @@ const { debugFetch, log } = require("../shared/log"); const { verifySignedTx, verifySignature, + typedDataChainRefusal, failureIsRetryable, describeTxFailure, sameAddress, @@ -1764,7 +1765,8 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { (async () => { try { - const activeAddress = await getActiveAddress(); + const s = await getState(); + const activeAddress = activeAddressOf(s); // Same as the transaction path: the address the approval named // is the one that must have signed, and a switch since then is // a refusal rather than a signature from another account. @@ -1783,6 +1785,20 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { signature, approval.approvedFrom, ); + // The sign screen refuses typed data whose domain names + // another chain. A signature for it that arrives anyway is + // refused here, against the network active now. + const sp = approval.signParams; + if ( + sp.method === "eth_signTypedData_v4" || + sp.method === "eth_signTypedData" + ) { + const refusal = typedDataChainRefusal( + JSON.parse(sp.typedData).domain, + networkById(s.networkId), + ); + if (refusal) throw new ApprovalMismatchError(refusal); + } settleApproval(msg.id, { signature }, { holdsClaim: true }); sendResponse({ signature }); } catch (e) { diff --git a/src/popup/views/approval.js b/src/popup/views/approval.js index 166a4a5..823e465 100644 --- a/src/popup/views/approval.js +++ b/src/popup/views/approval.js @@ -12,7 +12,7 @@ const { formatFee, tokenLabel, } = require("./helpers"); -const { state, saveState } = require("../../shared/state"); +const { state, saveState, currentNetwork } = require("../../shared/state"); const { networkById, networkByChainId, @@ -46,7 +46,10 @@ const { const { decryptWithPassword } = require("../../shared/vault"); const { getSignerForAddress } = require("../../shared/wallet"); const { walletDefect } = require("../../shared/walletDefects"); -const { describeSigningFailure } = require("../../shared/approvalVerify"); +const { + describeSigningFailure, + typedDataChainRefusal, +} = require("../../shared/approvalVerify"); const txStatus = require("./txStatus"); const uniswap = require("../../shared/uniswap"); const { notify, runtimeApi, sendMessage } = require("../../shared/browserApi"); @@ -443,7 +446,8 @@ function signedPrimaryType(types) { // Why a signature request cannot be signed, as a sentence for the error line, // or null when it can. Only typed data is refused: the `primaryType` the page // states has to be the type ethers will sign, or this screen would name one -// message while another is signed. +// message while another is signed. Its domain may name no chain but the active +// network's. function typedDataRefusal(sp) { if (sp.method === "personal_sign" || sp.method === "eth_sign") return null; let data; @@ -466,7 +470,7 @@ function typedDataRefusal(sp) { ", so it cannot be signed." ); } - return null; + return typedDataChainRefusal(data.domain, currentNetwork()); } // The largest amount a Permit2 allowance can hold, a uint160. Permit2 treats diff --git a/src/shared/approvalVerify.js b/src/shared/approvalVerify.js index cf5e737..4aa1a6a 100644 --- a/src/shared/approvalVerify.js +++ b/src/shared/approvalVerify.js @@ -53,6 +53,7 @@ const { accessListify, formatEther, getAddress, + getBigInt, getBytes, toQuantity, verifyMessage, @@ -635,6 +636,33 @@ function verifySignature(signParams, signature, expectedFrom) { return recovered; } +// Why typed data cannot be signed while `network` is the active network, as a +// sentence, or null when it can. EIP-712 has a wallet refuse typed data whose +// domain names another chain, because the signature is valid on that chain. +// ethers signs no chain id when the domain's is null or absent, and reads any +// other with getBigInt(), so 1, "1" and "0x1" are the same chain here too. +function typedDataChainRefusal(domain, network) { + const chainId = domain?.chainId; + if (chainId === undefined || chainId === null) return null; + let named; + try { + named = getBigInt(chainId); + } catch { + return "This typed data cannot be read, so it cannot be signed."; + } + const active = getBigInt(network.chainId); + if (named === active) return null; + return ( + "This typed data is for chain id " + + named + + ", but the active network is " + + network.name + + " (chain id " + + active + + "), so it cannot be signed." + ); +} + // The stage a transaction approval failed at. Which stage it is decides // whether the approval survives the failure. const TX_STAGE_SIGN = "sign"; @@ -796,6 +824,7 @@ function describeSigningFailure(response, fallbackMessage) { module.exports = { verifySignedTx, verifySignature, + typedDataChainRefusal, assertNoForbiddenFields, assertNothingUnchecked, assertCanonicalBytes, diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index 40d3e6c..f0ad395 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -397,7 +397,13 @@ function loadBackground(options) { rpcResult = r; }, ); - return { result: () => rpcResult }; + return { + id: () => + new URL(created[created.length - 1].url).searchParams.get( + "approval", + ), + result: () => rpcResult, + }; } // A dApp asking to connect. The origin defaults to one the persisted @@ -1484,6 +1490,55 @@ describe("what the approval is verified against", () => { }); }); + // Typed data whose domain names another chain is refused on the sign + // screen; a signature for it that reaches the background anyway is not + // handed to the page (https://git.eeqj.de/sneak/AutistMask/issues/519). + // TYPED_DATA names mainnet. + async function answerTypedData(bg, pending) { + const { domain, types, message } = JSON.parse(TYPED_DATA); + delete types.EIP712Domain; + const signature = await signer.signTypedData(domain, types, message); + const answer = bg.send( + { + type: "AUTISTMASK_SIGN_RESPONSE", + id: pending.id(), + approved: true, + signature, + }, + { url: bg.fromPopup.url }, + ); + await settle(); + return { answer, signature }; + } + + test("typed data for the active chain is handed back signed", async () => { + const bg = loadBackground(); + const pending = bg.requestTypedData(); + await settle(); + + const { answer, signature } = await answerTypedData(bg, pending); + + expect(answer.sendResponse).toHaveBeenCalledWith({ signature }); + expect(pending.result()).toEqual({ result: signature }); + }); + + test("typed data for another chain is refused, not handed back", async () => { + const bg = loadBackground(); + bg.setNetwork(SEPOLIA); + const pending = bg.requestTypedData(); + await settle(); + + const { answer } = await answerTypedData(bg, pending); + + const refusal = + "This typed data is for chain id 1, but the active network is Sepolia Testnet (chain id 11155111), so it cannot be signed."; + expect(answer.sendResponse).toHaveBeenCalledWith({ + error: refusal, + retryable: false, + }); + expect(pending.result()).toEqual({ error: { message: refusal } }); + }); + test("a sign request from another address raises no approval at all", async () => { const bg = loadBackground(); const pending = bg.requestSign(other.address); diff --git a/tests/typedDataPermit.test.js b/tests/typedDataPermit.test.js index ff34c67..a9fcf96 100644 --- a/tests/typedDataPermit.test.js +++ b/tests/typedDataPermit.test.js @@ -240,6 +240,7 @@ function request(data) { beforeEach(() => { state.trackedTokens = []; state.wallets = []; + state.networkId = "mainnet"; }); describe("a token permission is warned about, naming spender and amount", () => { @@ -532,3 +533,66 @@ describe("the sign screen refuses a mismatched primary type", () => { expect(node("btn-approve-sign").disabled).toBe(true); }); }); + +// EIP-712: a wallet refuses typed data whose domain names a chain other than +// the active one, because the signature is valid on the chain it names +// (https://git.eeqj.de/sneak/AutistMask/issues/519). The wallet is on Sepolia +// and the site asks for a mainnet USDC Permit. +describe("the sign screen refuses typed data for another chain", () => { + const REFUSAL = + "This typed data is for chain id 1, but the active network is Sepolia Testnet (chain id 11155111), so it cannot be signed."; + + function withChainId(chainId) { + return { ...PERMIT, domain: { ...PERMIT.domain, chainId } }; + } + + beforeEach(() => { + decryptWithPassword.mockClear(); + state.networkId = "sepolia"; + }); + + test("another chain shows the error, with Sign disabled", async () => { + await openSignScreen(PERMIT); + expect(node("approve-sign-error").textContent).toBe(REFUSAL); + expect(node("btn-approve-sign").disabled).toBe(true); + }); + + test("Sign clicked anyway decrypts and signs nothing", async () => { + const sent = await openSignScreen(PERMIT); + node("approve-sign-password").value = "any password"; + await click("btn-approve-sign"); + + expect(decryptWithPassword).not.toHaveBeenCalled(); + expect(sent.map((msg) => msg.type)).not.toContain( + "AUTISTMASK_SIGN_RESPONSE", + ); + expect(node("approve-sign-error").textContent).toBe(REFUSAL); + }); + + // ethers signs 1, "1" and "0x1" as the same chain id. + test("the chain id is refused in each form ethers signs", () => { + for (const chainId of [1, "1", "0x1"]) { + expect(typedDataRefusal(request(withChainId(chainId)))).toBe( + REFUSAL, + ); + } + }); + + test("a chain id that is not a number is refused", () => { + expect(typedDataRefusal(request(withChainId("mainnet")))).toBe( + "This typed data cannot be read, so it cannot be signed.", + ); + }); + + test("the active chain leaves Sign enabled", async () => { + await openSignScreen(withChainId("0xaa36a7")); + expect(node("approve-sign-error").textContent).toBe(""); + expect(node("btn-approve-sign").disabled).toBe(false); + }); + + test("no chain id leaves Sign enabled", async () => { + await openSignScreen(withChainId(undefined)); + expect(node("approve-sign-error").textContent).toBe(""); + expect(node("btn-approve-sign").disabled).toBe(false); + }); +});