From df7f26552a03a664d05bdd589a849fb83a4bea87 Mon Sep 17 00:00:00 2001 From: Local Dev Date: Sun, 4 Oct 2026 03:59:28 +0200 Subject: [PATCH] Aegis: judge EIP-712 risk from its structure and encode it strictly Non-Permit typed data was flagged only when its primaryType was on a short list (Seaport, SafeTx, ...); any other order, relay or account-abstraction type got a plain Sign button and no PIN. Every signed field is now listed with its declared type, and a payload that names an address other than the user's and the verifying contract together with an amount, or carries raw bytes, gets the danger button and the PIN; text-only payloads stay ordinary. The encoder signed "false" as true, turned non-hex characters into zero bytes, wrapped integers past their width (2^256+5 signed as 5) and signed a non-array as an empty array, each while the overlay showed the original value. Such input is now refused before any overlay. Domain rows and the chain check use only the fields EIP712Domain declares; others are labelled as not signed. --- bundled-addons/aegis/index.js | 88 ++++++++++++++++++++++++++---- bundled-addons/aegis/lib/eip712.js | 43 +++++++++++++-- 2 files changed, 117 insertions(+), 14 deletions(-) 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); }