diff --git a/bundled-addons/aegis/index.js b/bundled-addons/aegis/index.js index 0cdfa9e6..8c858cc9 100644 --- a/bundled-addons/aegis/index.js +++ b/bundled-addons/aegis/index.js @@ -3452,6 +3452,61 @@ function signedView(td) { return { message: walk(String(td?.primaryType || ""), td?.message), dropped }; } +// The chain check applies only when the domain actually signs a chainId. +function domDeclaredEarly(td) { + return Array.isArray(td?.types?.EIP712Domain) && td.types.EIP712Domain.some((x) => x && x.name === "chainId"); +} +// What a non-Permit typed-data signature can do, judged from its declared +// structure rather than its name. The name list (Seaport, SafeTx, …) missed +// every marketplace, relayer and account-abstraction type it did not know, +// and those got a primary "Sign" button and no PIN. Here every signed field +// is listed with its type, and the payload counts as asset-moving when it +// names an address other than the user's and the verifying contract +// together with an amount-like number, or carries raw bytes (an arbitrary +// call). Text-only payloads (logins, votes) stay ordinary. +function assessTypedData(td, view, ownAddress) { + const types = (td && typeof td.types === "object" && td.types) || {}; + const own = String(ownAddress || "").toLowerCase(); + const vc = String(td?.domain?.verifyingContract || "").toLowerCase(); + const fields = []; + let foreignAddr = false, amount = false, rawBytes = false, numbers = false, addresses = false; + const AMOUNTISH = /amount|value|price|wad|qty|quantity|fee|tip|salary|reward|consideration|offer|balance|allowance/i; + const DEADLINEISH = /deadline|expir|valid(to|until|before)|endtime|end_time|until/i; + const walk = (type, v, path, depth) => { + if (fields.length > 200 || depth > 8) return; + const arr = /^(.+)\[(\d*)\]$/.exec(type); + if (arr) { (Array.isArray(v) ? v : []).forEach((x, i) => walk(arr[1], x, `${path}[${i}]`, depth + 1)); return; } + if (Array.isArray(types[type])) { for (const fd of types[type]) if (fd && typeof fd.name === "string") walk(String(fd.type), v && typeof v === "object" ? v[fd.name] : undefined, path ? `${path}.${fd.name}` : fd.name, depth + 1); return; } + const name = path.split(".").pop().replace(/\[\d+\]$/, ""); + let shown = String(v); + if (type === "address") { + addresses = true; + const a = String(v).toLowerCase(); + if (a !== own && a !== vc && !/^0x0{40}$/.test(a)) foreignAddr = true; + shown = String(v) + (a === own ? " (you)" : a === vc ? " (the verifying contract)" : ""); + } else if (/^u?int\d*$/.test(type)) { + numbers = true; + let n = null; try { n = BigInt(v); } catch {} + if (n != null) { + if (DEADLINEISH.test(name) && n > 0n) shown = n >= 4102444800n ? `${n} (never expires in practice)` : `${n} (${new Date(Number(n) * 1000).toISOString().slice(0, 16).replace("T", " ")} UTC)`; + else if (n >= (1n << 96n)) { shown = `${n} (≈ unlimited)`; amount = true; } + else shown = n.toString(); + if (AMOUNTISH.test(name) && n > 0n) amount = true; + } + } else if (type === "bytes") { + const h = String(v || "").replace(/^0x/i, ""); + if (h.length) rawBytes = true; + shown = h.length ? `0x${h.slice(0, 64)}${h.length > 64 ? "…" : ""} (${h.length / 2} bytes)` : "(empty)"; + } else if (type === "string") { + shown = JSON.stringify(plainLabel(v, 200)); + } + fields.push(`${path}: ${shown} [${type}]`); + }; + walk(String(td?.primaryType || ""), view.message, "", 0); + const movesAssets = (foreignAddr && amount) || rawBytes; + return { fields, movesAssets, plain: !addresses && !numbers }; +} + function describePermit(td) { const primary = String(td?.primaryType || ""); if (!/^Permit/.test(primary)) return null; @@ -4063,19 +4118,27 @@ function registerPageMessages(api) { // A signature for another chain is the standard way to get an approval // the user believes is for a testnet or a sidechain. MetaMask refuses a // domain whose chainId is not the active chain; so does Aegis. - if (dom.chainId != null && dom.chainId !== "") { + if (domDeclaredEarly(td) && dom.chainId != null && dom.chainId !== "") { let domChain = null; try { domChain = BigInt(dom.chainId); } catch {} if (domChain === null || domChain !== BigInt(snapTd.chainId)) { throw ethError(`typed data is for chain ${dom.chainId} but this site is connected on chain ${snapTd.chainId}`, 4901); } } - const domainSummary = [dom.name, dom.version && `v${dom.version}`, dom.chainId && `chain ${dom.chainId}`].filter(Boolean).join(" · ") || "(no domain)"; + // Only the domain fields EIP712Domain declares are hashed. A site could + // declare [name] alone and still send verifyingContract/chainId, which + // were shown (and chain-checked) although the signature never covers + // them; they are now shown as not signed. + const domDeclared = Array.isArray(td.types?.EIP712Domain) ? td.types.EIP712Domain.map((x) => x && x.name) : []; + const dd = (k) => (domDeclared.includes(k) ? dom[k] : undefined); + const domainSummary = [dd("name") && plainLabel(dd("name"), 60), dd("version") && `v${plainLabel(dd("version"), 20)}`, dd("chainId") != null && `chain ${dd("chainId")}`].filter(Boolean).join(" · ") || "(no domain)"; + const undeclaredDomain = Object.keys(dom).filter((k) => !domDeclared.includes(k)); // Preview only what the signature covers (see signedView). const view = signedView(td); - const messagePreview = JSON.stringify(view.message, (_k, v) => (typeof v === "bigint" ? v.toString() : v), 2) || ""; + const assessed = assessTypedData(td, view, snapTd.address); const rows = [{ label: "Domain", value: domainSummary }]; - if (dom.verifyingContract) rows.push({ label: "Contract", value: String(dom.verifyingContract), mono: true }); + if (dd("verifyingContract")) rows.push({ label: "Contract", value: String(dd("verifyingContract")), mono: true }); + if (undeclaredDomain.length) rows.push({ label: "Not signed", value: `domain field${undeclaredDomain.length === 1 ? "" : "s"} ${undeclaredDomain.join(", ")} (sent but not covered by the signature)` }); rows.push({ label: "Primary type", value: String(td.primaryType || "") }); // Off-chain approvals. A Permit / Permit2 signature moves no gas and // shows no transaction, yet lets the spender take the tokens later — it @@ -4085,9 +4148,11 @@ function registerPageMessages(api) { if (permit) for (const r of permit.rows) rows.push(r); // Marketplace orders and multisig transactions are not permits but move // NFTs, tokens or a whole Safe just as surely once signed. + // The name list now only labels what the structure test already found. const MOVES_ASSETS = /^(OrderComponents|BulkOrder|Order|MakerOrder|TakerOrder|SafeTx|SafeMessage|MetaTransaction|ForwardRequest)$/; - const movesAssets = !permit && MOVES_ASSETS.test(String(td.primaryType || "")); - rows.push({ label: "Message", value: previewText(messagePreview, 3000), mono: true }); + const movesAssets = !permit && (assessed.movesAssets || MOVES_ASSETS.test(String(td.primaryType || ""))); + const fieldText = assessed.fields.slice(0, 60).join("\n") + (assessed.fields.length > 60 ? `\n… ${assessed.fields.length - 60} more fields are NOT shown but will be signed` : ""); + rows.push({ label: "Signed fields", value: previewText(fieldText || "(none)", 4000), mono: true }); if (view.dropped) rows.push({ label: "Not signed", value: `${view.dropped} field${view.dropped === 1 ? "" : "s"} the site sent are not covered by this signature and are not shown` }); rows.push({ label: "Address", value: snapTd.address, mono: true }); const risky = !!(permit && permit.risky) || movesAssets; @@ -4100,14 +4165,17 @@ function registerPageMessages(api) { ? "WARNING: signing this lets the spender below take these tokens at any time, without another prompt and without a transaction from you. Only sign if you fully trust this site." : "Signing this lets the spender below move the stated amount of your tokens without a transaction from you.") : movesAssets - ? `WARNING: a signed ${String(td.primaryType)} can move your NFTs, tokens or Safe without another prompt. Only sign if you fully trust this site and the details below are what you expect.` - : "The site is asking you to sign a structured message. Verify the domain matches the site you're on — a mismatched domain is the classic phishing tell.", + ? `WARNING: this ${plainLabel(td.primaryType, 60)} names another address together with an amount (or carries raw call data). Signed, it can move your tokens, NFTs or a Safe without another prompt. Only sign if you fully trust this site and every field below is what you expect.` + : assessed.plain + ? "The site is asking you to sign a structured message with no addresses or amounts in it. Verify the domain matches the site you're on." + : "The site is asking you to sign structured data that contains addresses or numbers. Verify the domain matches the site you're on and check every field — a mismatched domain is the classic phishing tell.", rows, actions: [{ id: "sign", label: risky ? "Sign anyway" : "Sign", primary: !risky, danger: risky }], }); if (pick !== "sign") throw new Error("user rejected"); - // A permit moves tokens as surely as a transaction does. - if (permit) await requireDappTxPin(origin, "token spending permit"); + // A permit, an order or a Safe transaction moves assets as surely as a + // transaction does. + if (permit || movesAssets) await requireDappTxPin(origin, permit ? "token spending permit" : "signature that can move assets"); return rt.adapter.signTypedDataDigest(digest); }); }); diff --git a/bundled-addons/aegis/lib/eip712.js b/bundled-addons/aegis/lib/eip712.js index 0280e37e..d66aeae8 100644 --- a/bundled-addons/aegis/lib/eip712.js +++ b/bundled-addons/aegis/lib/eip712.js @@ -23,9 +23,12 @@ module.exports = function makeEip712({ keccak_256 }) { for (const p of ps) { out.set(p, k); k += p.length; } return out; }; + // Strict: a character that is not hex used to become a zero byte, so the + // overlay showed one value and the signature covered another. const hex2bytes = (h) => { const s = String(h).replace(/^0x/i, ""); if (s.length % 2) throw new Error("hex: odd length"); + if (!/^[0-9a-fA-F]*$/.test(s)) throw new Error("hex: not a hex string"); const out = new Uint8Array(s.length / 2); for (let i = 0; i < out.length; i++) out[i] = parseInt(s.slice(i * 2, i * 2 + 2), 16); return out; @@ -74,7 +77,11 @@ module.exports = function makeEip712({ keccak_256 }) { const arr = /^(.+)\[(\d*)\]$/.exec(type); if (arr) { const baseType = arr[1]; - const items = Array.isArray(value) ? value : []; + // A non-array used to sign as an empty array while the overlay + // showed the value itself. + if (!Array.isArray(value)) throw new Error(`${type} expects an array`); + if (arr[2] !== "" && value.length !== Number(arr[2])) throw new Error(`${type} expects ${arr[2]} items, got ${value.length}`); + const items = value; const encoded = items.map((v) => encodeValue(baseType, v, types)); return keccak_256(concat(...encoded)); } @@ -97,8 +104,14 @@ module.exports = function makeEip712({ keccak_256 }) { return out; } if (type === "bool") { + // Truthiness signed "false" (a string) as true while the overlay + // showed false. Only values that say what they mean are accepted. + let b; + if (value === true || value === 1 || value === "true" || value === "1") b = 1; + else if (value === false || value === 0 || value === "false" || value === "0") b = 0; + else throw new Error("bool expects true or false"); const out = new Uint8Array(32); - out[31] = value ? 1 : 0; + out[31] = b; return out; } // bytesN (fixed): left-aligned in a 32-byte word. @@ -113,10 +126,32 @@ module.exports = function makeEip712({ keccak_256 }) { return out; } // uint* / int*: encode as 32-byte big-endian. + // Range-checked against the declared width: 2^256 + 5 used to sign as + // 5, and a uint8 of 300 as a word no contract would decode. + const intOf = (v) => { + if (typeof v === "bigint") return v; + if (typeof v === "number") { if (!Number.isSafeInteger(v)) throw new Error("integer is not exact"); return BigInt(v); } + const t = String(v ?? "").trim(); + if (!/^-?(0x[0-9a-fA-F]+|\d+)$/.test(t)) throw new Error(`not an integer: ${t.slice(0, 40)}`); + return t.startsWith("-") ? -BigInt(t.slice(1)) : BigInt(t); + }; const uintM = /^uint(\d*)$/.exec(type); - if (uintM) return bigToBe32(value, false); + if (uintM) { + const bits = Number(uintM[1] || 256); + if (bits < 8 || bits > 256 || bits % 8) throw new Error("bad integer width " + type); + const v = intOf(value); + if (v < 0n || v >= (1n << BigInt(bits))) throw new Error(`${type} out of range`); + return bigToBe32(v, false); + } const intM = /^int(\d*)$/.exec(type); - if (intM) return bigToBe32(value, true); + if (intM) { + const bits = Number(intM[1] || 256); + if (bits < 8 || bits > 256 || bits % 8) throw new Error("bad integer width " + type); + const v = intOf(value); + const lim = 1n << BigInt(bits - 1); + if (v < -lim || v >= lim) throw new Error(`${type} out of range`); + return bigToBe32(v, true); + } throw new Error("unsupported EIP-712 type: " + type); }