fix: typed data for another chain cannot be signed (closes #519)
EIP-712 has a wallet refuse typed data whose domain chainId is not the active chain, because the signature is valid on the chain it names. The sign screen now shows such a request with the reason on its error line and Sign disabled, the same way it refuses a mismatched primary type. The background checks again when the signature comes back, against the network active then, and answers the site with that refusal instead of the signature. A chain id is read as ethers signs it, so 1, "1" and "0x1" are one chain. Typed data with no chainId signs as before. Model: opus-5-5
This commit is contained in:
@@ -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
|
- 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
|
would be signed as, or that cannot be read → shown with the error line
|
||||||
saying so and "Sign" disabled; only "Reject" remains
|
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
|
- 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
|
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
|
signing) → shown as plain text, with the error line "This message is plain
|
||||||
|
|||||||
+17
-1
@@ -22,6 +22,7 @@ const { debugFetch, log } = require("../shared/log");
|
|||||||
const {
|
const {
|
||||||
verifySignedTx,
|
verifySignedTx,
|
||||||
verifySignature,
|
verifySignature,
|
||||||
|
typedDataChainRefusal,
|
||||||
failureIsRetryable,
|
failureIsRetryable,
|
||||||
describeTxFailure,
|
describeTxFailure,
|
||||||
sameAddress,
|
sameAddress,
|
||||||
@@ -1764,7 +1765,8 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
|
|||||||
|
|
||||||
(async () => {
|
(async () => {
|
||||||
try {
|
try {
|
||||||
const activeAddress = await getActiveAddress();
|
const s = await getState();
|
||||||
|
const activeAddress = activeAddressOf(s);
|
||||||
// Same as the transaction path: the address the approval named
|
// Same as the transaction path: the address the approval named
|
||||||
// is the one that must have signed, and a switch since then is
|
// is the one that must have signed, and a switch since then is
|
||||||
// a refusal rather than a signature from another account.
|
// a refusal rather than a signature from another account.
|
||||||
@@ -1783,6 +1785,20 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => {
|
|||||||
signature,
|
signature,
|
||||||
approval.approvedFrom,
|
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 });
|
settleApproval(msg.id, { signature }, { holdsClaim: true });
|
||||||
sendResponse({ signature });
|
sendResponse({ signature });
|
||||||
} catch (e) {
|
} catch (e) {
|
||||||
|
|||||||
@@ -12,7 +12,7 @@ const {
|
|||||||
formatFee,
|
formatFee,
|
||||||
tokenLabel,
|
tokenLabel,
|
||||||
} = require("./helpers");
|
} = require("./helpers");
|
||||||
const { state, saveState } = require("../../shared/state");
|
const { state, saveState, currentNetwork } = require("../../shared/state");
|
||||||
const {
|
const {
|
||||||
networkById,
|
networkById,
|
||||||
networkByChainId,
|
networkByChainId,
|
||||||
@@ -46,7 +46,10 @@ const {
|
|||||||
const { decryptWithPassword } = require("../../shared/vault");
|
const { decryptWithPassword } = require("../../shared/vault");
|
||||||
const { getSignerForAddress } = require("../../shared/wallet");
|
const { getSignerForAddress } = require("../../shared/wallet");
|
||||||
const { walletDefect } = require("../../shared/walletDefects");
|
const { walletDefect } = require("../../shared/walletDefects");
|
||||||
const { describeSigningFailure } = require("../../shared/approvalVerify");
|
const {
|
||||||
|
describeSigningFailure,
|
||||||
|
typedDataChainRefusal,
|
||||||
|
} = require("../../shared/approvalVerify");
|
||||||
const txStatus = require("./txStatus");
|
const txStatus = require("./txStatus");
|
||||||
const uniswap = require("../../shared/uniswap");
|
const uniswap = require("../../shared/uniswap");
|
||||||
const { notify, runtimeApi, sendMessage } = require("../../shared/browserApi");
|
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,
|
// 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
|
// 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
|
// 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) {
|
function typedDataRefusal(sp) {
|
||||||
if (sp.method === "personal_sign" || sp.method === "eth_sign") return null;
|
if (sp.method === "personal_sign" || sp.method === "eth_sign") return null;
|
||||||
let data;
|
let data;
|
||||||
@@ -466,7 +470,7 @@ function typedDataRefusal(sp) {
|
|||||||
", so it cannot be signed."
|
", so it cannot be signed."
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
return null;
|
return typedDataChainRefusal(data.domain, currentNetwork());
|
||||||
}
|
}
|
||||||
|
|
||||||
// The largest amount a Permit2 allowance can hold, a uint160. Permit2 treats
|
// The largest amount a Permit2 allowance can hold, a uint160. Permit2 treats
|
||||||
|
|||||||
@@ -53,6 +53,7 @@ const {
|
|||||||
accessListify,
|
accessListify,
|
||||||
formatEther,
|
formatEther,
|
||||||
getAddress,
|
getAddress,
|
||||||
|
getBigInt,
|
||||||
getBytes,
|
getBytes,
|
||||||
toQuantity,
|
toQuantity,
|
||||||
verifyMessage,
|
verifyMessage,
|
||||||
@@ -635,6 +636,33 @@ function verifySignature(signParams, signature, expectedFrom) {
|
|||||||
return recovered;
|
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
|
// The stage a transaction approval failed at. Which stage it is decides
|
||||||
// whether the approval survives the failure.
|
// whether the approval survives the failure.
|
||||||
const TX_STAGE_SIGN = "sign";
|
const TX_STAGE_SIGN = "sign";
|
||||||
@@ -796,6 +824,7 @@ function describeSigningFailure(response, fallbackMessage) {
|
|||||||
module.exports = {
|
module.exports = {
|
||||||
verifySignedTx,
|
verifySignedTx,
|
||||||
verifySignature,
|
verifySignature,
|
||||||
|
typedDataChainRefusal,
|
||||||
assertNoForbiddenFields,
|
assertNoForbiddenFields,
|
||||||
assertNothingUnchecked,
|
assertNothingUnchecked,
|
||||||
assertCanonicalBytes,
|
assertCanonicalBytes,
|
||||||
|
|||||||
@@ -397,7 +397,13 @@ function loadBackground(options) {
|
|||||||
rpcResult = r;
|
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
|
// 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 () => {
|
test("a sign request from another address raises no approval at all", async () => {
|
||||||
const bg = loadBackground();
|
const bg = loadBackground();
|
||||||
const pending = bg.requestSign(other.address);
|
const pending = bg.requestSign(other.address);
|
||||||
|
|||||||
@@ -240,6 +240,7 @@ function request(data) {
|
|||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
state.trackedTokens = [];
|
state.trackedTokens = [];
|
||||||
state.wallets = [];
|
state.wallets = [];
|
||||||
|
state.networkId = "mainnet";
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("a token permission is warned about, naming spender and amount", () => {
|
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);
|
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);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user