fix: typed data with a missing field or a bool that is not true or false is refused (closes #552)

ethers signs a bool field by whether its value is truthy, so the string
"false" was shown as false and signed as true, and a bool field left out
was shown as nothing and signed as false. The sign screen now refuses
typed data whose message leaves out a declared field, at any level of
nesting, or holds a bool that is not true or false: the error line names
the field and Sign is disabled. The background refuses a signature for it
too. Presence is an own-key test, so a field named like a built-in
property such as constructor is not found by accident. signedPrimaryType()
moves to the shared module so the background reads the message as the
same type.

Model: opus-5-5

src/background/index.js

@@ -22,6 +22,8 @@ const { debugFetch, log } = require("../shared/log");
 const {
     verifySignedTx,
     verifySignature,
+    signedPrimaryType,
+    typedDataMessageRefusal,
     typedDataChainRefusal,
     failureIsRetryable,
     describeTxFailure,
@@ -1787,18 +1789,27 @@ 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.
+                // The sign screen refuses typed data whose message ethers
+                // would sign differently from how it is shown, or 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),
-                    );
+                    const data = JSON.parse(sp.typedData);
+                    const refusal =
+                        typedDataMessageRefusal(
+                            data.types,
+                            signedPrimaryType(data.types),
+                            data.message,
+                            "",
+                        ) ||
+                        typedDataChainRefusal(
+                            data.domain,
+                            networkById(s.networkId),
+                        );
                     if (refusal) throw new ApprovalMismatchError(refusal);
                 }
                 settleApproval(msg.id, { signature }, { holdsClaim: true });

src/popup/views/approval.js

@@ -27,7 +27,6 @@ const {
     Interface,
     MaxUint256,
     toUtf8String,
-    TypedDataEncoder,
 } = require("ethers");
 const { getPrice, formatUsd } = require("../../shared/prices");
 const { ERC20_ABI } = require("../../shared/constants");
@@ -48,6 +47,8 @@ const { getSignerForAddress } = require("../../shared/wallet");
 const { walletDefect } = require("../../shared/walletDefects");
 const {
     describeSigningFailure,
+    signedPrimaryType,
+    typedDataMessageRefusal,
     typedDataChainRefusal,
 } = require("../../shared/approvalVerify");
 const txStatus = require("./txStatus");
@@ -432,22 +433,12 @@ function markInvisibleCharacters(text) {
         );
 }
 
-// The type ethers will sign typed data as. ethers does not read the page's
-// `primaryType`: it takes the one struct in `types` that no other struct
-// refers to. Throws when the types name no such single struct, which ethers
-// would refuse to sign as well.
-function signedPrimaryType(types) {
-    const structs = { ...types };
-    // ethers derives EIP712Domain itself and rejects it as an input.
-    delete structs.EIP712Domain;
-    return TypedDataEncoder.getPrimaryType(structs);
-}
-
 // 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. Its domain may name no chain but the active
-// network's.
+// message while another is signed. Its message has to hold every declared
+// field, with each `bool` true or false, and 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;
@@ -470,7 +461,10 @@ function typedDataRefusal(sp) {
             ", so it cannot be signed."
         );
     }
-    return typedDataChainRefusal(data.domain, currentNetwork());
+    return (
+        typedDataMessageRefusal(data.types, signed, data.message, "") ||
+        typedDataChainRefusal(data.domain, currentNetwork())
+    );
 }
 
 // The largest amount a Permit2 allowance can hold, a uint160. Permit2 treats

src/shared/approvalVerify.js

@@ -56,6 +56,7 @@ const {
     getBigInt,
     getBytes,
     toQuantity,
+    TypedDataEncoder,
     verifyMessage,
     verifyTypedData,
 } = require("ethers");
@@ -636,6 +637,63 @@ function verifySignature(signParams, signature, expectedFrom) {
     return recovered;
 }
 
+// The type ethers will sign typed data as. ethers does not read the page's
+// `primaryType`: it takes the one struct in `types` that no other struct
+// refers to. Throws when the types name no such single struct, which ethers
+// would refuse to sign as well.
+function signedPrimaryType(types) {
+    const structs = { ...types };
+    // ethers derives EIP712Domain itself and rejects it as an input.
+    delete structs.EIP712Domain;
+    return TypedDataEncoder.getPrimaryType(structs);
+}
+
+// Why a typed data message cannot be signed as the screen shows it, as a
+// sentence, or null when it can. ethers signs a `bool` field by whether its
+// value is truthy, so the string "false" is signed as true and a field left
+// out as false. Each field that `type` declares, at every level of nesting,
+// must be one of the value's own keys, and each `bool` must be true or false.
+// Called with the type ethers signs, the message, and a `path` of "".
+function typedDataMessageRefusal(types, type, value, path) {
+    const itemType = type.replace(/\[\d*\]$/, "");
+    if (itemType !== type) {
+        // ethers refuses to sign a list field that is not a list.
+        if (!Array.isArray(value)) return null;
+        for (let i = 0; i < value.length; i++) {
+            const refusal = typedDataMessageRefusal(
+                types,
+                itemType,
+                value[i],
+                `${path}[${i}]`,
+            );
+            if (refusal) return refusal;
+        }
+        return null;
+    }
+    if (type === "bool" && value !== true && value !== false) {
+        return `The field "${path}" is not true or false, so this typed data cannot be signed.`;
+    }
+    for (const field of types[type] || []) {
+        const at = path ? `${path}.${field.name}` : field.name;
+        // An own key only: a plain lookup of `constructor` finds a function.
+        const present =
+            value !== null &&
+            typeof value === "object" &&
+            Object.prototype.hasOwnProperty.call(value, field.name);
+        if (!present) {
+            return `The field "${at}" is missing from the message, so this typed data cannot be signed.`;
+        }
+        const refusal = typedDataMessageRefusal(
+            types,
+            field.type,
+            value[field.name],
+            at,
+        );
+        if (refusal) return refusal;
+    }
+    return null;
+}
+
 // 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.
@@ -824,6 +882,8 @@ function describeSigningFailure(response, fallbackMessage) {
 module.exports = {
     verifySignedTx,
     verifySignature,
+    signedPrimaryType,
+    typedDataMessageRefusal,
     typedDataChainRefusal,
     assertNoForbiddenFields,
     assertNothingUnchecked,