Raw data
diff --git a/src/popup/views/approval.js b/src/popup/views/approval.js
index 034c154..ebbc25c 100644
--- a/src/popup/views/approval.js
+++ b/src/popup/views/approval.js
@@ -10,6 +10,7 @@ const {
onViewLeave,
} = require("./helpers");
const { state, saveState, currentNetwork } = require("../../shared/state");
+const { networkByChainId } = require("../../shared/networks");
const {
formatEther,
formatUnits,
@@ -23,7 +24,6 @@ const { TOKEN_BY_ADDRESS } = require("../../shared/tokenList");
const { decryptWithPassword } = require("../../shared/vault");
const { getSignerForAddress } = require("../../shared/wallet");
const { walletDefect } = require("../../shared/walletDefects");
-const { getProvider } = require("../../shared/balances");
const { describeSigningFailure } = require("../../shared/approvalVerify");
const txStatus = require("./txStatus");
const uniswap = require("../../shared/uniswap");
@@ -159,21 +159,61 @@ function showPhishingWarning(elementId, isPhishing) {
}
}
+// The fields of the approved transaction the value and recipient lines do not
+// already carry: network, gas limit, fee per gas, the most the fee can come to,
+// and the nonce. The background compares every one of them against the signed
+// artifact, so every one of them has to be on the screen — a number that is
+// verified but never displayed is verified against nothing the user agreed to.
+function showTxFee(approvedTx, ethPrice) {
+ const network = networkByChainId(approvedTx.chainId);
+ $("approve-tx-network").textContent = network
+ ? network.name
+ : "Unknown network (chain id " + BigInt(approvedTx.chainId) + ")";
+
+ const gasLimit = BigInt(approvedTx.gasLimit);
+ const feePerGas = BigInt(approvedTx.maxFeePerGas || approvedTx.gasPrice);
+ const maxFeeEth = formatTxValue(formatEther(gasLimit * feePerGas));
+ const usdStr = formatUsd(
+ ethPrice ? parseFloat(maxFeeEth) * ethPrice : null,
+ );
+ $("approve-tx-fee").textContent =
+ maxFeeEth + " ETH" + (usdStr ? " (" + usdStr + ")" : "");
+
+ let detail =
+ gasLimit.toString() +
+ " gas at up to " +
+ formatUnits(feePerGas, 9) +
+ " gwei";
+ if (approvedTx.maxPriorityFeePerGas) {
+ detail +=
+ ", " +
+ formatUnits(approvedTx.maxPriorityFeePerGas, 9) +
+ " gwei priority";
+ }
+ $("approve-tx-fee-detail").textContent = detail;
+ $("approve-tx-nonce").textContent = BigInt(approvedTx.nonce).toString();
+}
+
function showTxApproval(details) {
showPhishingWarning(
"approve-tx-phishing-warning",
details.isPhishingDomain,
);
- pendingTxParams = details.txParams;
+ // The transaction the background populated. It is displayed as it stands,
+ // signed as it stands, and verified against as it stands — the popup fills
+ // nothing in, so there is no number on this screen that the background
+ // cannot compare with the artifact it gets back.
+ pendingTxParams = details.approvedTx;
+ const approvedTx = details.approvedTx;
- const toAddr = details.txParams.to;
+ const toAddr = approvedTx.to;
const token = toAddr ? TOKEN_BY_ADDRESS.get(toAddr.toLowerCase()) : null;
- const ethValue = formatEther(details.txParams.value || "0");
+ const ethValue = formatEther(approvedTx.value || "0");
// Build txInfo for status screens
pendingTxDetails = {
- from: state.activeAddress,
+ from: details.approvedFrom,
to: toAddr || "",
amount: formatTxValue(ethValue),
token: "ETH",
@@ -181,7 +221,7 @@ function showTxApproval(details) {
};
// If this is an ERC-20 call, try to extract the real recipient and amount
- const decoded = decodeCalldata(details.txParams.data, toAddr || "");
+ const decoded = decodeCalldata(approvedTx.data, toAddr || "");
if (decoded && decoded.details) {
let decodedTokenAddr = null;
let decodedTokenSymbol = null;
@@ -219,7 +259,7 @@ function showTxApproval(details) {
}
$("approve-tx-hostname").textContent = details.hostname;
- $("approve-tx-from").innerHTML = approvalAddressHtml(state.activeAddress);
+ $("approve-tx-from").innerHTML = approvalAddressHtml(details.approvedFrom);
// Show token symbol next to contract address if known
const symbol = toAddr ? tokenLabel(toAddr) : null;
@@ -235,7 +275,7 @@ function showTxApproval(details) {
}
const ethValueFormatted = formatTxValue(
- formatEther(details.txParams.value || "0"),
+ formatEther(approvedTx.value || "0"),
);
const ethPrice = getPrice("ETH");
const ethUsd = ethPrice ? parseFloat(ethValueFormatted) * ethPrice : null;
@@ -243,6 +283,8 @@ function showTxApproval(details) {
$("approve-tx-value").textContent =
ethValueFormatted + " ETH" + (usdStr ? " (" + usdStr + ")" : "");
+ showTxFee(approvedTx, ethPrice);
+
// Decode calldata (reuse decoded from above)
const decodedEl = $("approve-tx-decoded");
if (decoded) {
@@ -271,8 +313,8 @@ function showTxApproval(details) {
}
// Always show raw data when present
- if (details.txParams.data && details.txParams.data !== "0x") {
- $("approve-tx-data").textContent = details.txParams.data;
+ if (approvedTx.data && approvedTx.data !== "0x") {
+ $("approve-tx-data").textContent = approvedTx.data;
$("approve-tx-data-section").classList.remove("hidden");
} else {
$("approve-tx-data-section").classList.add("hidden");
@@ -283,7 +325,11 @@ function showTxApproval(details) {
showView("approve-tx");
attachCopyHandlers("view-approve-tx");
- gateOnWalletDefect("approve-tx-error", "btn-approve-tx");
+ gateOnWalletDefect(
+ "approve-tx-error",
+ "btn-approve-tx",
+ details.approvedFrom,
+ );
}
function decodeHexMessage(hex) {
@@ -342,9 +388,12 @@ function showSignApproval(details) {
const sp = details.signParams;
pendingSignParams = sp;
+ pendingSignFrom = details.approvedFrom;
$("approve-sign-hostname").textContent = details.hostname;
- $("approve-sign-from").innerHTML = approvalAddressHtml(sp.from);
+ $("approve-sign-from").innerHTML = approvalAddressHtml(
+ details.approvedFrom,
+ );
const isTyped =
sp.method === "eth_signTypedData_v4" ||
@@ -383,7 +432,11 @@ function showSignApproval(details) {
showView("approve-sign");
attachCopyHandlers("view-approve-sign");
- gateOnWalletDefect("approve-sign-error", "btn-approve-sign");
+ gateOnWalletDefect(
+ "approve-sign-error",
+ "btn-approve-sign",
+ details.approvedFrom,
+ );
}
function show(id) {
@@ -418,11 +471,15 @@ function show(id) {
let approvalId = null;
let pendingTxDetails = null;
-// The exact parameters shown to the user, kept so the popup signs what it
-// displayed rather than re-fetching anything at approval time. Both are
-// repopulated by show() when the popup is closed and reopened.
+// The exact objects shown to the user, kept so the popup signs what it
+// displayed rather than re-fetching or re-populating anything at approval
+// time. All are repopulated by show() when the popup is closed and reopened.
let pendingTxParams = null;
let pendingSignParams = null;
+// The address the approval was raised for. Signing uses this rather than the
+// active address, so that an address switch since the approval fails here
+// instead of producing a signature from an account the screen never named.
+let pendingSignFrom = null;
// Approve buttons stay disabled and muted while the popup derives the key and
// signs, which is slow enough (Argon2id) that a double click is likely.
@@ -437,12 +494,13 @@ function setSignButtonBusy(busy) {
}
// Say so on the approval screen itself, and disable the approve button, when
-// the active address belongs to a wallet whose keys cannot be derived. Without
-// this the screen would take a password and fail after deriving it. Reject
-// stays available; the wallet is not touched. Returns true when it gated.
-function gateOnWalletDefect(errorId, buttonId) {
- const active = findActiveWallet();
- const defect = active ? walletDefect(active.wallet) : null;
+// the address the approval was raised for belongs to a wallet whose keys
+// cannot be derived. Without this the screen would take a password and fail
+// after deriving it. Reject stays available; the wallet is not touched.
+// Returns true when it gated.
+function gateOnWalletDefect(errorId, buttonId, address) {
+ const owner = findWalletFor(address);
+ const defect = owner ? walletDefect(owner.wallet) : null;
if (!defect) return false;
showError(errorId, defect.shortMessage);
$(buttonId).disabled = true;
@@ -450,12 +508,14 @@ function gateOnWalletDefect(errorId, buttonId) {
return true;
}
-// Locate the wallet and the address index owning the currently active
-// address. Returns null when no wallet holds it.
-function findActiveWallet() {
+// Locate the wallet and the address index owning an address. Returns null when
+// no wallet holds it. Approvals look up the address they were raised for, not
+// whichever address is active now: the approval named one account, and signing
+// with another is what verification refuses.
+function findWalletFor(address) {
for (const wallet of state.wallets) {
for (let i = 0; i < wallet.addresses.length; i++) {
- if (wallet.addresses[i].address === state.activeAddress) {
+ if (wallet.addresses[i].address === address) {
return { wallet, addrIndex: i };
}
}
@@ -517,12 +577,12 @@ function init(ctx) {
hideError("approve-tx-error");
setTxButtonBusy(true);
- const active = findActiveWallet();
+ const active = findWalletFor(pendingTxParams.from);
if (!active) {
password = null;
showError(
"approve-tx-error",
- "No wallet was found for the active address.",
+ "No wallet was found for the address this transaction was approved for.",
);
setTxButtonBusy(false);
return;
@@ -569,15 +629,16 @@ function init(ctx) {
active.addrIndex,
decryptedSecret,
);
- const provider = getProvider(state.rpcUrl);
- const connected = signer.connect(provider);
- // This is the sequence ethers' own sendTransaction() runs
- // internally, so nonce, gas, fee and chain id population are
- // identical to when the background did the signing.
- const populated =
- await connected.populateTransaction(pendingTxParams);
- delete populated.from;
- payload.rawSignedTx = await connected.signTransaction(populated);
+ // Sign the approved transaction exactly as it was displayed. The
+ // background populated it before this screen was drawn and checks
+ // the artifact against it field for field, so there is nothing to
+ // fill in here and no provider to fill it in from. The copy is
+ // because ethers may strip `from` off what it is handed, and the
+ // approval has to survive a retry intact; keeping `from` on it
+ // makes ethers refuse a key that is not the approved address.
+ payload.rawSignedTx = await signer.signTransaction({
+ ...pendingTxParams,
+ });
} catch (e) {
payload.error =
e.shortMessage || e.message || "Transaction signing failed.";
@@ -626,12 +687,12 @@ function init(ctx) {
hideError("approve-sign-error");
setSignButtonBusy(true);
- const active = findActiveWallet();
+ const active = findWalletFor(pendingSignFrom);
if (!active) {
password = null;
showError(
"approve-sign-error",
- "No wallet was found for the active address.",
+ "No wallet was found for the address this request was approved for.",
);
setSignButtonBusy(false);
return;
diff --git a/src/shared/approvalTx.js b/src/shared/approvalTx.js
new file mode 100644
index 0000000..7dd7e7b
--- /dev/null
+++ b/src/shared/approvalTx.js
@@ -0,0 +1,213 @@
+// Preparation of the transaction an approval screen displays.
+//
+// A dApp's eth_sendTransaction normally fixes only `to`, `value` and `data`.
+// The nonce, the gas limit and the fees have to be filled in from the network
+// before anything can be signed, and whoever fills them in decides what the
+// user is shown. That work used to happen in the popup, after the user had
+// already approved: the numbers on the approval screen came from the popup and
+// were compared against nothing, so a compromised popup could display one fee
+// and sign another, and the ceilings in approvalVerify.js were all that stood
+// between the user and a fee that hands the validator the balance.
+//
+// So it happens here instead, in the background, before the approval window is
+// opened. The background populates the transaction, shows that object, and
+// verifies the signed artifact against that same object — the popup is handed
+// a finished transaction and signs it as given. Every field the user reads is
+// then a field that is compared.
+//
+// The cost is an RPC round trip before the approval window exists. Nothing is
+// displayed while it is in flight, and a failure — an unreachable node, a
+// reverting gas estimate, a transaction type this wallet does not sign, a fee
+// past the ceilings — means no approval and no window at all: the error goes
+// back to the requesting page, which is where the user's click came from. That
+// is deliberate. The alternative, opening the window first and populating
+// behind a spinner, needs a pending approval that exists before it can be
+// displayed or signed, and a half-initialised approval is exactly the state
+// the settle interlock in the background exists to keep out of that record.
+// The failure also lands earlier than it used to rather than later: the same
+// estimate previously failed after the user had typed their password.
+
+const {
+ VoidSigner,
+ accessListify,
+ getAddress,
+ getBytes,
+ hexlify,
+ toQuantity,
+} = require("ethers");
+const {
+ ALLOWED_TX_TYPES,
+ SERIALIZED_FIELDS,
+ assertWithinCeilings,
+} = require("./approvalVerify");
+
+// How long the population may take before the request is failed back to the
+// page. Without a bound a hung RPC endpoint leaves the dApp's promise pending
+// forever with nothing on screen to explain it; ethers' own request timeout is
+// minutes long, which is not a wait anyone will sit through.
+const POPULATE_TIMEOUT_MS = 20000;
+
+// The request fields taken from the page. Anything else is dropped rather than
+// passed to ethers: the object is page-controlled, and a future ethers that
+// learns to carry a new transaction field must not start picking one up out of
+// it without this module knowing.
+const REQUEST_FIELDS = [
+ "to",
+ "value",
+ "data",
+ "nonce",
+ "gasLimit",
+ "gasPrice",
+ "maxFeePerGas",
+ "maxPriorityFeePerGas",
+ "chainId",
+ "accessList",
+ "type",
+];
+
+class ApprovalPrepareError extends Error {
+ constructor(message) {
+ super(message);
+ this.name = "ApprovalPrepareError";
+ }
+}
+
+function fail(message) {
+ return new ApprovalPrepareError(message);
+}
+
+function present(v) {
+ return v !== null && v !== undefined && v !== "";
+}
+
+// These strings reach the user through the requesting page, so they are full
+// sentences even when the tail of one came from ethers or from the node.
+function sentence(text) {
+ return /[.!?]$/.test(text) ? text : text + ".";
+}
+
+// Reject a promise that has taken too long, and never leave the timer behind.
+async function withTimeout(promise, ms, message) {
+ let timer = null;
+ try {
+ return await Promise.race([
+ promise,
+ new Promise((_resolve, reject) => {
+ timer = setTimeout(() => reject(fail(message)), ms);
+ }),
+ ]);
+ } finally {
+ if (timer !== null) clearTimeout(timer);
+ }
+}
+
+// The page's request, reduced to the fields this wallet acts on.
+function requestFrom(txParams, from) {
+ const request = { from: getAddress(from) };
+ for (const key of REQUEST_FIELDS) {
+ if (present(txParams[key])) request[key] = txParams[key];
+ }
+ if (
+ present(request.type) &&
+ !ALLOWED_TX_TYPES.includes(Number(request.type))
+ ) {
+ throw fail(
+ "The site asked for a transaction of a type this wallet does not sign.",
+ );
+ }
+ return request;
+}
+
+// Turn a populated transaction into the object that crosses to the popup, is
+// displayed, and is compared with the signed artifact. It carries exactly the
+// fields its type serializes, plus the address it is to be signed by, and
+// every quantity as a hex string: extension messaging is JSON, which has no
+// bigint, and a field that did not survive the trip would be a field the user
+// was shown and nothing compared.
+function serializeApprovedTx(populated, from) {
+ const type = Number(populated.type);
+ if (!ALLOWED_TX_TYPES.includes(type)) {
+ throw fail(
+ "This transaction would have to be sent as a type this wallet does not sign.",
+ );
+ }
+ const approved = { type, from: getAddress(from) };
+ for (const key of SERIALIZED_FIELDS[type]) {
+ if (key === "to") {
+ approved.to = present(populated.to)
+ ? getAddress(populated.to)
+ : null;
+ } else if (key === "data") {
+ approved.data = present(populated.data)
+ ? hexlify(getBytes(populated.data))
+ : "0x";
+ } else if (key === "accessList") {
+ approved.accessList = accessListify(populated.accessList || []);
+ } else if (key === "value") {
+ approved.value = toQuantity(populated.value || 0);
+ } else if (!present(populated[key])) {
+ // Unreachable while populateTransaction() fills every quantity of
+ // the type it produced. If it ever does not, the approval must not
+ // be raised: an unfixed quantity is one the artifact cannot be
+ // checked against.
+ throw fail(
+ "The transaction could not be prepared: the network did not supply a " +
+ key +
+ ".",
+ );
+ } else {
+ approved[key] = toQuantity(populated[key]);
+ }
+ }
+ return approved;
+}
+
+// Populate the transaction a site asked for, as the address it will be signed
+// by, and return the object to display, sign and verify against. Throws with a
+// full sentence when no approval can be raised.
+async function prepareApprovalTx(provider, from, txParams) {
+ if (!present(from)) {
+ throw fail("There is no active address to send this transaction from.");
+ }
+ const request = requestFrom(txParams || {}, from);
+
+ let populated;
+ try {
+ // The sequence ethers' own sendTransaction() runs internally, so the
+ // nonce, gas, fee and chain id are populated exactly as they were when
+ // the popup did this. VoidSigner cannot sign, which is the point: the
+ // background prepares, the popup signs.
+ populated = await withTimeout(
+ new VoidSigner(getAddress(from), provider).populateTransaction(
+ request,
+ ),
+ POPULATE_TIMEOUT_MS,
+ "The transaction could not be prepared: the network did not answer in time.",
+ );
+ } catch (e) {
+ if (e instanceof ApprovalPrepareError) throw e;
+ throw fail(
+ sentence(
+ "The transaction could not be prepared: " +
+ (e.shortMessage ||
+ e.message ||
+ "the network did not answer"),
+ ),
+ );
+ }
+
+ const approved = serializeApprovedTx(populated, from);
+ // The backstop, applied before the user is shown anything rather than
+ // after they have approved it: what is displayed here is what gets signed,
+ // so an RPC node reporting an absurd fee has to be refused here.
+ assertWithinCeilings(approved);
+ return approved;
+}
+
+module.exports = {
+ prepareApprovalTx,
+ serializeApprovedTx,
+ ApprovalPrepareError,
+ POPULATE_TIMEOUT_MS,
+ REQUEST_FIELDS,
+};
diff --git a/src/shared/approvalVerify.js b/src/shared/approvalVerify.js
index e46c17b..d2fea3e 100644
--- a/src/shared/approvalVerify.js
+++ b/src/shared/approvalVerify.js
@@ -7,6 +7,13 @@
// the signer from the artifact and checks it against the approval it is
// holding before acting on it. All recovery is delegated to ethers.
//
+// What the artifact is checked against is the transaction the background
+// populated and the popup displayed (see approvalTx.js), not the request the
+// dApp made. The two differ in every field a dApp normally leaves out — nonce,
+// gas limit, fees — and those are the fields the user reads off the approval
+// screen, so comparing against the request would leave the numbers on screen
+// vouched for by nothing.
+//
// The check is an allowlist, in both directions, because a denylist cannot be
// correct against a transaction format that keeps gaining fields:
//
@@ -31,14 +38,12 @@
// never a warning: what the user approved is what gets broadcast, or nothing
// does.
//
-// Fields the approval does not carry are not treated as zero. The popup
-// populates nonce, gas limit, fee and chain id through populateTransaction()
-// when the requesting page did not fix them, so there is no approved value to
-// compare against; treating absent as zero would refuse every legitimate
-// transaction. Those fields are instead held to the absolute ceilings below,
-// and the chain id is always checked against the selected network rather than
-// against the approval alone, which is what makes a cross-chain replay
-// impossible.
+// The approved transaction is required to fix every field its type serializes,
+// so there is no "the approval did not say" branch to fall through: a quantity
+// the approval does not carry is a refusal, because an artifact that cannot be
+// compared with what was displayed has not been checked. The chain id is
+// checked against the selected network as well as against the approval, which
+// is what makes a cross-chain replay impossible.
//
// Every failure message is a full sentence, because these strings are shown to
// the user and returned to the dApp.
@@ -113,6 +118,17 @@ const FORBIDDEN_FIELDS = [
},
];
+// Absolute ceilings — a BACKSTOP, not the primary control.
+//
+// The primary control is equality: every field of the artifact is compared
+// with the populated transaction the user was shown, so nothing the popup
+// signs can differ from the screen. What equality cannot bound is the
+// populated transaction itself, which is built from what the configured RPC
+// node answered — a node that reports an absurd fee gets that fee displayed,
+// and a user who does not read the fee line would approve it. These ceilings
+// bound that, and they are therefore applied where the transaction is
+// populated (approvalTx.js) as well as here.
+//
// Above the block gas limit of every supported network (see networks.js), so
// no transaction that could ever be included is refused by it.
const MAX_GAS_LIMIT = 100000000n;
@@ -224,40 +240,151 @@ function normalizeData(v) {
return String(v).toLowerCase();
}
-// Quantity fields the requesting page may fix in the approval. Each is
-// compared exactly when the approval carries it, and left to the ceilings
-// above when it does not.
-const APPROVED_QUANTITIES = [
- {
- key: "nonce",
+// How each field of an approved transaction is compared with the artifact.
+// There is an entry here for every field any allowed type serializes — a test
+// pins that against SERIALIZED_FIELDS — so the comparison loop covers the
+// whole of what gets signed and cannot silently skip a field for want of a
+// comparator.
+//
+// `kind` decides how the two sides are made comparable. A `quantity` must be
+// fixed by the approval: it is one of the numbers on the approval screen, and
+// an absent one means the artifact cannot be checked against what was
+// displayed. `to`, `value`, `data` and `accessList` have canonical absent
+// forms — contract creation, zero, "0x" and the empty list — so they are
+// normalized on both sides instead.
+const APPROVED_FIELDS = {
+ chainId: {
+ kind: "quantity",
+ label: "network",
+ message:
+ "The signed transaction is for a different network than the one that was approved.",
+ },
+ nonce: {
+ kind: "quantity",
label: "nonce",
message: "The signed transaction does not carry the approved nonce.",
},
- {
- key: "gasLimit",
+ gasLimit: {
+ kind: "quantity",
label: "gas limit",
message:
"The signed transaction does not carry the approved gas limit.",
},
- {
- key: "gasPrice",
+ gasPrice: {
+ kind: "quantity",
label: "gas price",
message:
"The signed transaction does not carry the approved gas price.",
},
- {
- key: "maxFeePerGas",
+ maxFeePerGas: {
+ kind: "quantity",
label: "maximum fee per gas",
message:
"The signed transaction does not carry the approved maximum fee per gas.",
},
- {
- key: "maxPriorityFeePerGas",
+ maxPriorityFeePerGas: {
+ kind: "quantity",
label: "maximum priority fee per gas",
message:
"The signed transaction does not carry the approved maximum priority fee per gas.",
},
-];
+ to: {
+ kind: "address",
+ label: "recipient",
+ message:
+ "The signed transaction does not go to the approved recipient.",
+ },
+ value: {
+ kind: "value",
+ label: "value",
+ message: "The signed transaction does not carry the approved value.",
+ },
+ data: {
+ kind: "data",
+ label: "call data",
+ message:
+ "The signed transaction does not carry the approved call data.",
+ },
+ accessList: {
+ kind: "accessList",
+ label: "access list",
+ message:
+ "The signed transaction does not carry the approved access list.",
+ },
+};
+
+// Compare one field of the artifact with the approved transaction. A field
+// with no entry in the table above is refused rather than skipped: the loop
+// below runs over the fields the type serializes, so an unmatched key means
+// something that gets signed has no comparator at all.
+function assertFieldMatches(key, parsed, approvedTx) {
+ const field = APPROVED_FIELDS[key];
+ if (!field) {
+ throw refuse(
+ "The signed transaction carries a field this wallet cannot compare with the approval.",
+ );
+ }
+ switch (field.kind) {
+ case "quantity": {
+ if (!present(approvedTx[key])) {
+ throw refuse(
+ "The approved transaction fixes no " +
+ field.label +
+ ", so the signed transaction cannot be checked" +
+ " against what was shown.",
+ );
+ }
+ const approved = normalizeQuantity(approvedTx[key], field.label);
+ if (normalizeQuantity(parsed[key], field.label) !== approved) {
+ throw refuse(field.message);
+ }
+ return;
+ }
+ case "address":
+ if (!sameAddress(parsed[key], approvedTx[key])) {
+ throw refuse(field.message);
+ }
+ return;
+ case "value":
+ if (normalizeValue(parsed[key]) !== normalizeValue(approvedTx[key]))
+ throw refuse(field.message);
+ return;
+ case "data":
+ if (normalizeData(parsed[key]) !== normalizeData(approvedTx[key]))
+ throw refuse(field.message);
+ return;
+ default:
+ if (
+ normalizeAccessList(parsed[key]) !==
+ normalizeAccessList(approvedTx[key])
+ ) {
+ throw refuse(field.message);
+ }
+ }
+}
+
+// The ceilings, applied to a transaction that is either about to be displayed
+// or about to be broadcast. See MAX_GAS_LIMIT above for what they are for:
+// they bound what the RPC node can talk this wallet into showing the user,
+// which is the one thing comparing the artifact with the screen cannot do.
+function assertWithinCeilings(tx) {
+ if (
+ present(tx.gasLimit) &&
+ normalizeQuantity(tx.gasLimit, "gas limit") > MAX_GAS_LIMIT
+ ) {
+ throw refuse(
+ "The signed transaction sets a gas limit no network this wallet supports can accept.",
+ );
+ }
+ for (const key of ["gasPrice", "maxFeePerGas", "maxPriorityFeePerGas"]) {
+ if (!present(tx[key])) continue;
+ if (normalizeQuantity(tx[key], "fee per gas") > MAX_FEE_PER_GAS) {
+ throw refuse(
+ "The signed transaction sets a fee per gas far above any plausible value.",
+ );
+ }
+ }
+}
// Refuse a field only a transaction type this wallet does not sign can carry.
// The type allowlist keeps these unreachable in production, which is exactly
@@ -317,10 +444,28 @@ function assertCanonicalBytes(parsed, rawSignedTx) {
// signed by the address the approval was raised for, on the network that is
// selected. Returns the parsed ethers Transaction on success, throws
// otherwise.
-function verifySignedTx(rawSignedTx, txParams, expectedFrom, selectedChainId) {
+//
+// `approvedTx` is the populated transaction the approval screen displayed, and
+// `expectedFrom` is the address that was active when the approval was raised —
+// not whichever address is active now. An address switch between approval and
+// signing therefore refuses here rather than producing a transaction from an
+// account the approval did not name.
+function verifySignedTx(
+ rawSignedTx,
+ approvedTx,
+ expectedFrom,
+ selectedChainId,
+) {
if (typeof rawSignedTx !== "string" || !rawSignedTx.startsWith("0x")) {
throw refuse("The signed transaction is missing or malformed.");
}
+ // Nothing to compare against is a refusal like any other: an approval that
+ // does not carry the transaction it displayed cannot vouch for one.
+ if (!approvedTx || typeof approvedTx !== "object") {
+ throw refuse(
+ "There is no approved transaction to check the signed transaction against.",
+ );
+ }
let parsed;
try {
@@ -360,46 +505,15 @@ function verifySignedTx(rawSignedTx, txParams, expectedFrom, selectedChainId) {
"The signed transaction is for a different network than the one that is selected.",
);
}
- if (
- present(txParams.chainId) &&
- parsed.chainId !== normalizeQuantity(txParams.chainId, "network")
- ) {
- throw refuse(
- "The signed transaction is for a different network than the one that was approved.",
- );
- }
- if (!sameAddress(parsed.to, txParams.to)) {
- throw refuse(
- "The signed transaction does not go to the approved recipient.",
- );
- }
- if (normalizeValue(parsed.value) !== normalizeValue(txParams.value)) {
- throw refuse(
- "The signed transaction does not carry the approved value.",
- );
- }
- if (normalizeData(parsed.data) !== normalizeData(txParams.data)) {
- throw refuse(
- "The signed transaction does not carry the approved call data.",
- );
- }
- if (
- normalizeAccessList(parsed.accessList) !==
- normalizeAccessList(txParams.accessList)
- ) {
- throw refuse(
- "The signed transaction does not carry the approved access list.",
- );
- }
-
- // An approval that fixed EIP-1559 fees must not be signed as a legacy
- // transaction, and vice versa: the fee the user agreed to is only
- // meaningful under the mechanism it was quoted in.
+ // The approved fee mechanism, named before the type comparison below
+ // subsumes it: the fee the user agreed to is only meaningful under the
+ // mechanism it was quoted in, and saying so is more use than "a different
+ // transaction type".
const approvedEip1559 =
- present(txParams.maxFeePerGas) ||
- present(txParams.maxPriorityFeePerGas);
- const approvedLegacy = present(txParams.gasPrice);
+ present(approvedTx.maxFeePerGas) ||
+ present(approvedTx.maxPriorityFeePerGas);
+ const approvedLegacy = present(approvedTx.gasPrice);
const signedEip1559 = parsed.type === 2;
if (
(approvedEip1559 && !signedEip1559) ||
@@ -410,28 +524,33 @@ function verifySignedTx(rawSignedTx, txParams, expectedFrom, selectedChainId) {
);
}
- for (const field of APPROVED_QUANTITIES) {
- if (!present(txParams[field.key])) continue;
- const approved = normalizeQuantity(txParams[field.key], field.label);
- if (normalizeQuantity(parsed[field.key], field.label) !== approved) {
- throw refuse(field.message);
- }
- }
-
- if (parsed.gasLimit > MAX_GAS_LIMIT) {
+ // The type decides which fields are compared, so it is compared first and
+ // against the approval, not merely checked for membership of the
+ // allowlist above.
+ if (!present(approvedTx.type)) {
throw refuse(
- "The signed transaction sets a gas limit no network this wallet supports can accept.",
+ "The approved transaction fixes no transaction type, so the signed transaction cannot be checked against what was shown.",
);
}
- for (const key of ["gasPrice", "maxFeePerGas", "maxPriorityFeePerGas"]) {
- const fee = parsed[key];
- if (fee !== null && fee !== undefined && fee > MAX_FEE_PER_GAS) {
- throw refuse(
- "The signed transaction sets a fee per gas far above any plausible value.",
- );
- }
+ if (
+ BigInt(parsed.type) !==
+ normalizeQuantity(approvedTx.type, "transaction type")
+ ) {
+ throw refuse(
+ "The signed transaction does not use the approved transaction type.",
+ );
}
+ // Every field this type serializes, compared with the transaction the user
+ // was shown. Driving the loop off SERIALIZED_FIELDS is what keeps this
+ // exhaustive: the same table decides what assertNothingUnchecked() rebuilds
+ // from, so a field that gets signed and is not compared here cannot exist.
+ for (const key of SERIALIZED_FIELDS[parsed.type]) {
+ assertFieldMatches(key, parsed, approvedTx);
+ }
+
+ assertWithinCeilings(parsed);
+
assertNothingUnchecked(parsed);
assertCanonicalBytes(parsed, rawSignedTx);
@@ -504,10 +623,10 @@ function errorText(err) {
// approval. Anything else failed before the check ran and is retryable.
// - broadcast: always terminal. A broadcast that throws after the node
// accepted the transaction is routine (a timeout, a dropped response, a
-// node answering "already known"), and the popup's retry does not
-// re-broadcast these bytes — it re-runs populateTransaction() and signs
-// again at a freshly fetched pending-tag nonce. Retrying would therefore
-// put a second transaction on the chain for one approval.
+// node answering "already known"), so the wallet cannot tell a transaction
+// that never left from one that is already in the mempool. The approval is
+// spent and the requesting page has been given its outcome; a second
+// attempt against it would report a second outcome for one request.
function describeTxFailure(stage, err) {
const error = errorText(err);
const retryable =
@@ -553,6 +672,7 @@ module.exports = {
assertNoForbiddenFields,
assertNothingUnchecked,
assertCanonicalBytes,
+ assertWithinCeilings,
sameAddress,
failureIsRetryable,
describeTxFailure,
@@ -561,6 +681,7 @@ module.exports = {
ALLOWED_TX_TYPES,
SERIALIZED_FIELDS,
FORBIDDEN_FIELDS,
+ APPROVED_FIELDS,
TX_STAGE_SIGN,
TX_STAGE_VERIFY,
TX_STAGE_BROADCAST,
diff --git a/tests/approvalTx.test.js b/tests/approvalTx.test.js
new file mode 100644
index 0000000..744a1bd
--- /dev/null
+++ b/tests/approvalTx.test.js
@@ -0,0 +1,309 @@
+// Preparation of the transaction the approval screen displays.
+//
+// This is the half of the fix that makes the verification in
+// approvalVerify.test.js mean anything: the numbers the user reads have to be
+// produced before the screen is drawn and be the numbers that get signed. What
+// is asserted here is that the object leaving this module is complete (nothing
+// is left for the popup to fill in), that it survives the messaging boundary
+// (extension messaging is JSON, which has no bigint), and that nothing the
+// requesting page or the RPC node can say turns it into an approval that
+// should never have been raised.
+
+const { Network, Wallet } = require("ethers");
+const {
+ prepareApprovalTx,
+ serializeApprovedTx,
+ POPULATE_TIMEOUT_MS,
+} = require("../src/shared/approvalTx");
+const {
+ SERIALIZED_FIELDS,
+ MAX_FEE_PER_GAS,
+ MAX_GAS_LIMIT,
+} = require("../src/shared/approvalVerify");
+
+const SIGNER_KEY =
+ "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d";
+const signer = new Wallet(SIGNER_KEY);
+const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
+
+// The ordinary dApp request: recipient, value, call data, and nothing else.
+const TX_PARAMS = {
+ from: signer.address,
+ to: RECIPIENT,
+ value: "0x2386f26fc10000",
+ data: "0xdeadbeef",
+};
+
+function providerWith(overrides) {
+ return {
+ getNetwork: async () => Network.from(1),
+ getTransactionCount: async () => 7,
+ estimateGas: async () => 21000n,
+ getFeeData: async () => ({
+ gasPrice: 2000000000n,
+ maxFeePerGas: 2000000000n,
+ maxPriorityFeePerGas: 1000000000n,
+ }),
+ ...(overrides || {}),
+ };
+}
+
+// A node that only quotes a flat gas price, so populateTransaction produces a
+// legacy transaction rather than an EIP-1559 one.
+const legacyProvider = providerWith({
+ getFeeData: async () => ({
+ gasPrice: 2000000000n,
+ maxFeePerGas: null,
+ maxPriorityFeePerGas: null,
+ }),
+});
+
+describe("prepareApprovalTx", () => {
+ test("fills in everything the request left out", async () => {
+ const approved = await prepareApprovalTx(
+ providerWith(),
+ signer.address,
+ TX_PARAMS,
+ );
+ expect(approved).toEqual({
+ type: 2,
+ from: signer.address,
+ chainId: "0x1",
+ nonce: "0x7",
+ gasLimit: "0x5208",
+ maxPriorityFeePerGas: "0x3b9aca00",
+ maxFeePerGas: "0x77359400",
+ to: RECIPIENT,
+ value: TX_PARAMS.value,
+ data: TX_PARAMS.data,
+ accessList: [],
+ });
+ });
+
+ // The object is displayed, signed and verified against on the far side of
+ // chrome.runtime.sendMessage, which is JSON: a bigint would throw on the
+ // way out and a field that did not survive the trip would be a field the
+ // user was shown and nothing compared.
+ test("survives the messaging boundary unchanged", async () => {
+ const approved = await prepareApprovalTx(
+ providerWith(),
+ signer.address,
+ TX_PARAMS,
+ );
+ expect(JSON.parse(JSON.stringify(approved))).toEqual(approved);
+ for (const value of Object.values(approved)) {
+ expect(typeof value).not.toBe("bigint");
+ }
+ });
+
+ test("carries exactly the fields its type serializes, and the signer", async () => {
+ const approved = await prepareApprovalTx(
+ providerWith(),
+ signer.address,
+ TX_PARAMS,
+ );
+ expect(Object.keys(approved).sort()).toEqual(
+ ["type", "from", ...SERIALIZED_FIELDS[2]].sort(),
+ );
+ });
+
+ test("produces a legacy transaction when that is all the node quotes", async () => {
+ const approved = await prepareApprovalTx(
+ legacyProvider,
+ signer.address,
+ TX_PARAMS,
+ );
+ expect(approved.type).toBe(0);
+ expect(approved.gasPrice).toBe("0x77359400");
+ expect(approved.maxFeePerGas).toBeUndefined();
+ expect(Object.keys(approved).sort()).toEqual(
+ ["type", "from", ...SERIALIZED_FIELDS[0]].sort(),
+ );
+ });
+
+ test("keeps a nonce, gas limit and fee the request did fix", async () => {
+ const approved = await prepareApprovalTx(
+ providerWith(),
+ signer.address,
+ {
+ ...TX_PARAMS,
+ nonce: "0x2",
+ gasLimit: "0x30d40",
+ maxFeePerGas: "0x12a05f200",
+ maxPriorityFeePerGas: "0x3b9aca00",
+ },
+ );
+ expect(approved.nonce).toBe("0x2");
+ expect(approved.gasLimit).toBe("0x30d40");
+ expect(approved.maxFeePerGas).toBe("0x12a05f200");
+ });
+
+ test("carries an access list the request asked for", async () => {
+ const approved = await prepareApprovalTx(
+ providerWith(),
+ signer.address,
+ {
+ ...TX_PARAMS,
+ accessList: [{ address: RECIPIENT, storageKeys: [] }],
+ },
+ );
+ expect(approved.accessList).toEqual([
+ { address: RECIPIENT, storageKeys: [] },
+ ]);
+ });
+
+ // The request is page-controlled. Anything this wallet does not act on is
+ // dropped before ethers sees it, so a field a future ethers learns to
+ // carry cannot be picked up out of it without this module knowing.
+ test("drops request fields this wallet does not act on", async () => {
+ const approved = await prepareApprovalTx(
+ providerWith(),
+ signer.address,
+ {
+ ...TX_PARAMS,
+ authorizationList: [{ address: RECIPIENT }],
+ blobVersionedHashes: ["0x01" + "ab".repeat(31)],
+ customData: { anything: true },
+ },
+ );
+ expect(approved.authorizationList).toBeUndefined();
+ expect(approved.blobVersionedHashes).toBeUndefined();
+ expect(approved.customData).toBeUndefined();
+ expect(approved.type).toBe(2);
+ });
+
+ test("refuses a transaction type this wallet does not sign", async () => {
+ await expect(
+ prepareApprovalTx(providerWith(), signer.address, {
+ ...TX_PARAMS,
+ type: 4,
+ }),
+ ).rejects.toThrow(/type this wallet does not sign/);
+ });
+
+ test("refuses to raise an approval with no active address", async () => {
+ await expect(
+ prepareApprovalTx(providerWith(), null, TX_PARAMS),
+ ).rejects.toThrow(/no active address/);
+ });
+
+ // The ceilings as a backstop: equality with the screen cannot bound what
+ // the node talks the wallet into putting on the screen, so it is refused
+ // before the user is shown anything.
+ test("refuses a fee the node quoted above the ceiling", async () => {
+ const gouging = providerWith({
+ getFeeData: async () => ({
+ gasPrice: MAX_FEE_PER_GAS + 1n,
+ maxFeePerGas: MAX_FEE_PER_GAS + 1n,
+ maxPriorityFeePerGas: 1000000000n,
+ }),
+ });
+ await expect(
+ prepareApprovalTx(gouging, signer.address, TX_PARAMS),
+ ).rejects.toThrow(/fee per gas far above any plausible value/);
+ });
+
+ test("refuses a gas limit the node estimated above the ceiling", async () => {
+ const absurd = providerWith({
+ estimateGas: async () => MAX_GAS_LIMIT + 1n,
+ });
+ await expect(
+ prepareApprovalTx(absurd, signer.address, TX_PARAMS),
+ ).rejects.toThrow(/gas limit no network this wallet supports/);
+ });
+
+ // No approval and no window: the failure goes back to the page the click
+ // came from, in a sentence.
+ test("reports a failed estimate as a full sentence", async () => {
+ const reverting = providerWith({
+ estimateGas: async () => {
+ throw new Error("execution reverted: ERC20: transfer amount");
+ },
+ });
+ let thrown;
+ try {
+ await prepareApprovalTx(reverting, signer.address, TX_PARAMS);
+ } catch (e) {
+ thrown = e;
+ }
+ expect(thrown.message).toMatch(
+ /^The transaction could not be prepared/,
+ );
+ expect(thrown.message).toMatch(/execution reverted/);
+ expect(thrown.message).toMatch(/^[A-Z].*\.$/);
+ });
+
+ // Without a bound, an unreachable node leaves the page's promise pending
+ // with nothing on screen to explain it.
+ test("gives up on a node that never answers", async () => {
+ jest.useFakeTimers();
+ try {
+ const hanging = providerWith({
+ estimateGas: () => new Promise(() => {}),
+ });
+ const pending = prepareApprovalTx(
+ hanging,
+ signer.address,
+ TX_PARAMS,
+ );
+ const settled = expect(pending).rejects.toThrow(
+ /did not answer in time/,
+ );
+ await jest.advanceTimersByTimeAsync(POPULATE_TIMEOUT_MS + 1);
+ await settled;
+ } finally {
+ jest.useRealTimers();
+ }
+ });
+});
+
+describe("serializeApprovedTx", () => {
+ // Unreachable through prepareApprovalTx while the request type is checked
+ // first, which is what it is for: a node or an ethers upgrade that
+ // populates a type this wallet does not sign must not produce an approval.
+ test("refuses a populated transaction of a type this wallet does not sign", () => {
+ expect(() =>
+ serializeApprovedTx(
+ { type: 3, to: RECIPIENT, nonce: 7 },
+ signer.address,
+ ),
+ ).toThrow(/type this wallet does not sign/);
+ });
+
+ test("refuses a populated transaction missing a quantity", () => {
+ expect(() =>
+ serializeApprovedTx(
+ {
+ type: 2,
+ chainId: 1n,
+ nonce: 7,
+ gasLimit: 21000n,
+ maxFeePerGas: 2000000000n,
+ to: RECIPIENT,
+ value: 0n,
+ data: "0x",
+ },
+ signer.address,
+ ),
+ ).toThrow(/did not supply a maxPriorityFeePerGas/);
+ });
+
+ test("keeps a contract creation's absent recipient absent", () => {
+ const approved = serializeApprovedTx(
+ {
+ type: 0,
+ chainId: 1n,
+ nonce: 7,
+ gasPrice: 2000000000n,
+ gasLimit: 21000n,
+ to: null,
+ value: 0n,
+ data: "0x600160005500",
+ },
+ signer.address,
+ );
+ expect(approved.to).toBeNull();
+ expect(approved.value).toBe("0x0");
+ expect(approved.data).toBe("0x600160005500");
+ });
+});
diff --git a/tests/approvalVerify.test.js b/tests/approvalVerify.test.js
index 1979c47..99fc106 100644
--- a/tests/approvalVerify.test.js
+++ b/tests/approvalVerify.test.js
@@ -11,6 +11,7 @@ const {
assertNoForbiddenFields,
assertNothingUnchecked,
assertCanonicalBytes,
+ assertWithinCeilings,
sameAddress,
failureIsRetryable,
describeTxFailure,
@@ -18,12 +19,14 @@ const {
ALLOWED_TX_TYPES,
SERIALIZED_FIELDS,
FORBIDDEN_FIELDS,
+ APPROVED_FIELDS,
TX_STAGE_SIGN,
TX_STAGE_VERIFY,
TX_STAGE_BROADCAST,
MAX_GAS_LIMIT,
MAX_FEE_PER_GAS,
} = require("../src/shared/approvalVerify");
+const { prepareApprovalTx } = require("../src/shared/approvalTx");
const { getSignerForAddress } = require("../src/shared/wallet");
// Fixed test keys — never used for anything but these tests.
@@ -42,7 +45,10 @@ const OTHER_RECIPIENT = "0xdAC17F958D2ee523a2206206994597C13D831ec7";
const SELECTED = "0x1";
const SEPOLIA = "0xaa36a7";
-// Approved parameters as a dApp would supply them over eth_sendTransaction.
+// Parameters as a dApp would supply them over eth_sendTransaction. Note what
+// is missing: nonce, gas limit and fees. The background fills those in before
+// the approval screen is drawn, which is why the approval below and not this
+// object is what every comparison runs against.
const TX_PARAMS = {
from: signer.address,
to: RECIPIENT,
@@ -61,8 +67,8 @@ const POPULATED = {
type: 2,
};
-// Build a signable transaction from approved params. The popup does the same
-// thing through populateTransaction(); here the fields are fixed so the test
+// Build a signable transaction from a request. The background populates the
+// same fields through populateTransaction(); here they are fixed so the test
// needs no provider. `overrides` stands in for what a tampered or misbuilt
// popup would put on the wire.
function txFor(params, overrides) {
@@ -75,6 +81,21 @@ function txFor(params, overrides) {
};
}
+// The populated transaction the approval screen displayed, which is the object
+// the artifact is verified against. Built from the same fields as the signable
+// transaction above, because that is the point: displayed and verified are one
+// object.
+function approvedFor(params, overrides) {
+ return {
+ from: signer.address,
+ accessList: [],
+ ...txFor(params, overrides),
+ };
+}
+
+// The ordinary case: the dApp's request, populated.
+const APPROVED = approvedFor(TX_PARAMS);
+
async function signedFor(params, withWallet, overrides) {
return (withWallet || signer).signTransaction(txFor(params, overrides));
}
@@ -108,7 +129,7 @@ describe("sameAddress", () => {
describe("verifySignedTx", () => {
test("accepts the approved transaction signed by the approved address", async () => {
const raw = await signedFor(TX_PARAMS);
- const parsed = verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED);
+ const parsed = verifySignedTx(raw, APPROVED, signer.address, SELECTED);
expect(parsed.from).toBe(signer.address);
expect(parsed.hash).toBe(Transaction.from(raw).hash);
});
@@ -117,23 +138,23 @@ describe("verifySignedTx", () => {
const params = { to: undefined, value: "0x0", data: "0x600160005500" };
const raw = await signedFor(params);
expect(() =>
- verifySignedTx(raw, params, signer.address, SELECTED),
+ verifySignedTx(raw, approvedFor(params), signer.address, SELECTED),
).not.toThrow();
});
test("accepts an absent value as zero", async () => {
- const approved = { to: RECIPIENT, data: "0x" };
- const raw = await signedFor(approved);
+ const params = { to: RECIPIENT, data: "0x" };
+ const raw = await signedFor(params);
expect(() =>
- verifySignedTx(raw, approved, signer.address, SELECTED),
+ verifySignedTx(raw, approvedFor(params), signer.address, SELECTED),
).not.toThrow();
});
test("accepts call data whose case differs from the approval", async () => {
- const approved = { to: RECIPIENT, value: "0x0", data: "0xDEADBEEF" };
- const raw = await signedFor(approved);
+ const params = { to: RECIPIENT, value: "0x0", data: "0xDEADBEEF" };
+ const raw = await signedFor(params);
expect(() =>
- verifySignedTx(raw, approved, signer.address, SELECTED),
+ verifySignedTx(raw, approvedFor(params), signer.address, SELECTED),
).not.toThrow();
});
@@ -143,7 +164,7 @@ describe("verifySignedTx", () => {
to: OTHER_RECIPIENT,
});
expect(() =>
- verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
).toThrow(/approved recipient/);
});
@@ -153,47 +174,64 @@ describe("verifySignedTx", () => {
value: "0x4563918244f40000",
});
expect(() =>
- verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
).toThrow(/approved value/);
});
test("rejects substituted call data", async () => {
const raw = await signedFor({ ...TX_PARAMS, data: "0xc0ffee" });
expect(() =>
- verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
).toThrow(/approved call data/);
});
test("rejects a transaction signed by a different address", async () => {
const raw = await signedFor(TX_PARAMS, other);
expect(() =>
- verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
).toThrow(/different address/);
});
+ // The address the approval named, not whichever address is active when the
+ // artifact comes back: an approval raised for one account cannot be
+ // satisfied by a signature from another, whatever the wallet switched to
+ // in between.
+ test("rejects a signature from the address that is active now", async () => {
+ const raw = await signedFor(TX_PARAMS, other);
+ expect(() =>
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
+ ).toThrow(/different address than the one that was approved/);
+ // The same artifact against the same approval, verified for the other
+ // address, is what would have happened had expectedFrom been read from
+ // the wallet's current state.
+ expect(() =>
+ verifySignedTx(raw, APPROVED, other.address, SELECTED),
+ ).not.toThrow();
+ });
+
test("rejects an unsigned transaction", () => {
const unsigned = Transaction.from(txFor(TX_PARAMS)).unsignedSerialized;
expect(() =>
- verifySignedTx(unsigned, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(unsigned, APPROVED, signer.address, SELECTED),
).toThrow(/no valid signature/);
});
test("rejects a missing or malformed payload", () => {
expect(() =>
- verifySignedTx(undefined, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(undefined, APPROVED, signer.address, SELECTED),
).toThrow(/missing or malformed/);
expect(() =>
- verifySignedTx("nope", TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx("nope", APPROVED, signer.address, SELECTED),
).toThrow(/missing or malformed/);
expect(() =>
- verifySignedTx("0xc0ffee", TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx("0xc0ffee", APPROVED, signer.address, SELECTED),
).toThrow(/could not be decoded/);
});
test("every rejection message is a full sentence", async () => {
const raw = await signedFor({ ...TX_PARAMS, to: OTHER_RECIPIENT });
try {
- verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED);
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED);
throw new Error("expected a rejection");
} catch (e) {
expect(e.message).toMatch(/^[A-Z].*\.$/);
@@ -201,20 +239,113 @@ describe("verifySignedTx", () => {
});
});
+// The defect this file's approvals now stand against: for every field the dApp
+// left out, the old comparison had nothing to compare and skipped the field,
+// so the fee and the nonce the user read off the screen were checked by the
+// ceilings alone. A populated approval fixes all of them, and an approval that
+// does not fix one is a refusal rather than a pass.
+describe("verifySignedTx against what was displayed", () => {
+ test("a fee differing from the displayed one is refused", async () => {
+ // Ten times the fee the screen showed, and far below the ceiling: the
+ // artifact the old comparison would have accepted.
+ const inflated = 20000000000n;
+ expect(inflated).toBeLessThan(MAX_FEE_PER_GAS);
+ const raw = await signedWith({ maxFeePerGas: inflated });
+ expect(() =>
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
+ ).toThrow(/approved maximum fee per gas/);
+ });
+
+ test("a nonce differing from the displayed one is refused", async () => {
+ const raw = await signedWith({ nonce: 8 });
+ expect(() =>
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
+ ).toThrow(/approved nonce/);
+ });
+
+ test("a gas limit differing from the displayed one is refused", async () => {
+ const raw = await signedWith({ gasLimit: 250000n });
+ expect(() =>
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
+ ).toThrow(/approved gas limit/);
+ });
+
+ test("an approval fixing no quantity is refused, not waved through", async () => {
+ const raw = await signedWith({});
+ for (const key of [
+ "chainId",
+ "nonce",
+ "gasLimit",
+ "maxFeePerGas",
+ "maxPriorityFeePerGas",
+ ]) {
+ const incomplete = { ...APPROVED };
+ delete incomplete[key];
+ let thrown;
+ try {
+ verifySignedTx(raw, incomplete, signer.address, SELECTED);
+ throw new Error("expected a rejection for " + key);
+ } catch (e) {
+ thrown = e;
+ }
+ expect(thrown.message).toMatch(/fixes no /);
+ expect(thrown.approvalMismatch).toBe(true);
+ }
+ });
+
+ test("no approved transaction at all is refused", async () => {
+ const raw = await signedWith({});
+ for (const approved of [undefined, null, "0xdeadbeef"]) {
+ expect(() =>
+ verifySignedTx(raw, approved, signer.address, SELECTED),
+ ).toThrow(/no approved transaction/);
+ }
+ });
+
+ test("an approval fixing no transaction type is refused", async () => {
+ const raw = await signedWith({});
+ const incomplete = { ...APPROVED };
+ delete incomplete.type;
+ expect(() =>
+ verifySignedTx(raw, incomplete, signer.address, SELECTED),
+ ).toThrow(/fixes no transaction type/);
+ });
+
+ test("an artifact of a type other than the approved one is refused", async () => {
+ // Same fee mechanism on both sides, so only the type differs: a type 1
+ // artifact against a type 2 approval.
+ const approved = approvedFor(TX_PARAMS, {
+ type: 1,
+ gasPrice: 2000000000n,
+ maxFeePerGas: null,
+ maxPriorityFeePerGas: null,
+ });
+ const raw = await signedWith({
+ type: 0,
+ gasPrice: 2000000000n,
+ maxFeePerGas: null,
+ maxPriorityFeePerGas: null,
+ });
+ expect(() =>
+ verifySignedTx(raw, approved, signer.address, SELECTED),
+ ).toThrow(/approved transaction type/);
+ });
+});
+
// One case per consequential field: the field alone differs from what was
// approved, and that alone must refuse the signature.
describe("verifySignedTx field comparison", () => {
test("rejects a chain id that is not the selected network", async () => {
const raw = await signedWith({ chainId: 11155111 });
expect(() =>
- verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
).toThrow(/different network than the one that is selected/);
});
test("rejects a chain id that is not the approved one", async () => {
- // Selected network and signed chain id agree; the dApp asked for a
- // different chain, so the artifact is not what was approved.
- const approved = { ...TX_PARAMS, chainId: SEPOLIA };
+ // Selected network and signed chain id agree; the approval was raised
+ // for a different chain, so the artifact is not what was approved.
+ const approved = { ...APPROVED, chainId: SEPOLIA };
const raw = await signedWith({});
expect(() =>
verifySignedTx(raw, approved, signer.address, SELECTED),
@@ -224,58 +355,59 @@ describe("verifySignedTx field comparison", () => {
test("refuses when the selected network is unknown", async () => {
const raw = await signedWith({});
expect(() =>
- verifySignedTx(raw, TX_PARAMS, signer.address, undefined),
+ verifySignedTx(raw, APPROVED, signer.address, undefined),
).toThrow(/selected network is unknown/);
});
test("rejects a substituted nonce", async () => {
- const approved = { ...TX_PARAMS, nonce: 7 };
const raw = await signedWith({ nonce: 8 });
expect(() =>
- verifySignedTx(raw, approved, signer.address, SELECTED),
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
).toThrow(/approved nonce/);
});
test("rejects a substituted gas limit", async () => {
- const approved = { ...TX_PARAMS, gasLimit: "0x186a0" };
const raw = await signedWith({ gasLimit: 250000n });
expect(() =>
- verifySignedTx(raw, approved, signer.address, SELECTED),
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
).toThrow(/approved gas limit/);
});
test("rejects a substituted maximum fee per gas", async () => {
- const approved = { ...TX_PARAMS, maxFeePerGas: "0x77359400" };
const raw = await signedWith({ maxFeePerGas: 900000000000n });
expect(() =>
- verifySignedTx(raw, approved, signer.address, SELECTED),
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
).toThrow(/approved maximum fee per gas/);
});
test("rejects a substituted maximum priority fee per gas", async () => {
- const approved = { ...TX_PARAMS, maxPriorityFeePerGas: "0x3b9aca00" };
const raw = await signedWith({ maxPriorityFeePerGas: 1500000000n });
expect(() =>
- verifySignedTx(raw, approved, signer.address, SELECTED),
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
).toThrow(/approved maximum priority fee per gas/);
});
test("rejects a substituted legacy gas price", async () => {
- const approved = { ...TX_PARAMS, gasPrice: "0x77359400" };
const legacy = {
type: 0,
- gasPrice: 9000000000n,
+ gasPrice: 2000000000n,
maxFeePerGas: null,
maxPriorityFeePerGas: null,
};
- const raw = await signedWith(legacy);
+ const approved = approvedFor(TX_PARAMS, legacy);
+ const raw = await signedWith({ ...legacy, gasPrice: 9000000000n });
expect(() =>
verifySignedTx(raw, approved, signer.address, SELECTED),
).toThrow(/approved gas price/);
});
test("rejects an approved legacy fee signed as an EIP-1559 fee", async () => {
- const approved = { ...TX_PARAMS, gasPrice: "0x77359400" };
+ const approved = approvedFor(TX_PARAMS, {
+ type: 0,
+ gasPrice: 2000000000n,
+ maxFeePerGas: null,
+ maxPriorityFeePerGas: null,
+ });
const raw = await signedWith({});
expect(() =>
verifySignedTx(raw, approved, signer.address, SELECTED),
@@ -283,7 +415,6 @@ describe("verifySignedTx field comparison", () => {
});
test("rejects an approved EIP-1559 fee signed as a legacy fee", async () => {
- const approved = { ...TX_PARAMS, maxFeePerGas: "0x77359400" };
const raw = await signedWith({
type: 0,
gasPrice: 2000000000n,
@@ -291,36 +422,72 @@ describe("verifySignedTx field comparison", () => {
maxPriorityFeePerGas: null,
});
expect(() =>
- verifySignedTx(raw, approved, signer.address, SELECTED),
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED),
).toThrow(/approved fee mechanism/);
});
+ // The ceilings are a backstop against what the RPC node can talk the
+ // wallet into populating and displaying, so they are checked against an
+ // approval that carries the absurd value too — equality alone would accept
+ // it, which is exactly what the ceiling is there for.
test("rejects a gas limit above anything a supported network accepts", async () => {
- const raw = await signedWith({ gasLimit: MAX_GAS_LIMIT + 1n });
+ const overrides = { gasLimit: MAX_GAS_LIMIT + 1n };
+ const raw = await signedWith(overrides);
expect(() =>
- verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(
+ raw,
+ approvedFor(TX_PARAMS, overrides),
+ signer.address,
+ SELECTED,
+ ),
).toThrow(/gas limit no network this wallet supports/);
});
- test("rejects an absurd fee per gas the approval never fixed", async () => {
- const raw = await signedWith({
+ test("rejects an absurd fee per gas even when it was displayed", async () => {
+ const overrides = {
maxFeePerGas: MAX_FEE_PER_GAS + 1n,
maxPriorityFeePerGas: MAX_FEE_PER_GAS + 1n,
- });
+ };
+ const raw = await signedWith(overrides);
expect(() =>
- verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(
+ raw,
+ approvedFor(TX_PARAMS, overrides),
+ signer.address,
+ SELECTED,
+ ),
).toThrow(/fee per gas far above any plausible value/);
});
+ test("assertWithinCeilings is the same check on either side of the screen", () => {
+ expect(() =>
+ assertWithinCeilings({ gasLimit: MAX_GAS_LIMIT + 1n }),
+ ).toThrow(/gas limit no network this wallet supports/);
+ for (const key of [
+ "gasPrice",
+ "maxFeePerGas",
+ "maxPriorityFeePerGas",
+ ]) {
+ expect(() =>
+ assertWithinCeilings({ [key]: MAX_FEE_PER_GAS + 1n }),
+ ).toThrow(/fee per gas far above any plausible value/);
+ }
+ expect(() =>
+ assertWithinCeilings({
+ gasLimit: MAX_GAS_LIMIT,
+ maxFeePerGas: MAX_FEE_PER_GAS,
+ maxPriorityFeePerGas: MAX_FEE_PER_GAS,
+ }),
+ ).not.toThrow();
+ // Nothing to bound is not a failure: a type 2 approval carries no gas
+ // price, and a bare object must not be refused for lacking one.
+ expect(() => assertWithinCeilings({})).not.toThrow();
+ });
+
test("every field mismatch is a refusal, not a warning", async () => {
const raw = await signedWith({ nonce: 8 });
try {
- verifySignedTx(
- raw,
- { ...TX_PARAMS, nonce: 7 },
- signer.address,
- SELECTED,
- );
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED);
throw new Error("expected a rejection");
} catch (e) {
expect(e.approvalMismatch).toBe(true);
@@ -331,17 +498,17 @@ describe("verifySignedTx field comparison", () => {
// The transaction type decides which fields exist, so an artifact of a type
// this wallet does not sign carries consequences the approval cannot describe
-// and none of the field comparisons can see. The approval used here is the
-// ordinary dApp shape with no fee fields — the common case, since
-// populateTransaction() fills them — which is exactly the case the
-// fee-mechanism check cannot catch by accident.
+// and none of the field comparisons can see. The refusal has to come from the
+// type allowlist rather than from a field comparison, so these run against an
+// approval whose every other field matches the artifact exactly.
describe("verifySignedTx transaction type", () => {
- const BARE_APPROVAL = {
+ const BARE_REQUEST = {
from: signer.address,
to: RECIPIENT,
value: "0x2386f26fc10000",
data: "0x",
};
+ const BARE_APPROVAL = approvedFor(BARE_REQUEST);
// An EIP-7702 artifact that pays the approved amount to the approved
// recipient and, in the same transaction, installs the attacker's code at
@@ -353,7 +520,7 @@ describe("verifySignedTx transaction type", () => {
chainId: 1,
nonce: 8,
});
- const raw = await signedFor(BARE_APPROVAL, signer, {
+ const raw = await signedFor(BARE_REQUEST, signer, {
type: 4,
authorizationList: [authorization],
});
@@ -366,7 +533,7 @@ describe("verifySignedTx transaction type", () => {
});
test("refuses a type 3 blob artifact", async () => {
- const raw = await signedFor(BARE_APPROVAL, signer, {
+ const raw = await signedFor(BARE_REQUEST, signer, {
type: 3,
maxFeePerBlobGas: 1000000000n,
blobVersionedHashes: ["0x01" + "ab".repeat(31)],
@@ -390,7 +557,7 @@ describe("verifySignedTx transaction type", () => {
chainId: 1,
nonce: 8,
});
- const raw = await signedFor(BARE_APPROVAL, signer, {
+ const raw = await signedFor(BARE_REQUEST, signer, {
type: 4,
authorizationList: [authorization],
});
@@ -404,40 +571,45 @@ describe("verifySignedTx transaction type", () => {
});
test("accepts a legacy type 0 transaction", async () => {
- const approved = { ...BARE_APPROVAL, gasPrice: "0x77359400" };
- const raw = await signedFor(approved, signer, {
+ const legacy = {
type: 0,
gasPrice: 2000000000n,
maxFeePerGas: null,
maxPriorityFeePerGas: null,
- });
+ };
+ const raw = await signedFor(BARE_REQUEST, signer, legacy);
expect(() =>
- verifySignedTx(raw, approved, signer.address, SELECTED),
+ verifySignedTx(
+ raw,
+ approvedFor(BARE_REQUEST, legacy),
+ signer.address,
+ SELECTED,
+ ),
).not.toThrow();
});
test("accepts a type 1 transaction whose access list is the approved one", async () => {
- const accessList = [{ address: OTHER_RECIPIENT, storageKeys: [] }];
- const approved = {
- ...BARE_APPROVAL,
- gasPrice: "0x77359400",
- accessList,
- };
- const raw = await signedFor(approved, signer, {
+ const overrides = {
type: 1,
gasPrice: 2000000000n,
maxFeePerGas: null,
maxPriorityFeePerGas: null,
- accessList,
- });
+ accessList: [{ address: OTHER_RECIPIENT, storageKeys: [] }],
+ };
+ const raw = await signedFor(BARE_REQUEST, signer, overrides);
expect(Transaction.from(raw).type).toBe(1);
expect(() =>
- verifySignedTx(raw, approved, signer.address, SELECTED),
+ verifySignedTx(
+ raw,
+ approvedFor(BARE_REQUEST, overrides),
+ signer.address,
+ SELECTED,
+ ),
).not.toThrow();
});
test("refuses an access list the approval never carried", async () => {
- const raw = await signedFor(BARE_APPROVAL, signer, {
+ const raw = await signedFor(BARE_REQUEST, signer, {
accessList: [{ address: OTHER_RECIPIENT, storageKeys: [] }],
});
expect(() =>
@@ -446,8 +618,11 @@ describe("verifySignedTx transaction type", () => {
});
test("treats an absent access list and an empty one as the same thing", async () => {
- const approved = { ...BARE_APPROVAL, accessList: [] };
- const raw = await signedFor(BARE_APPROVAL, signer, {});
+ const approved = { ...BARE_APPROVAL };
+ delete approved.accessList;
+ expect(approved.accessList).toBeUndefined();
+ const raw = await signedFor(BARE_REQUEST, signer, {});
+ expect(Transaction.from(raw).accessList).toEqual([]);
expect(() =>
verifySignedTx(raw, approved, signer.address, SELECTED),
).not.toThrow();
@@ -498,6 +673,25 @@ describe("verifySignedTx exhaustiveness", () => {
expect(exposed.filter((name) => !accounted.has(name))).toEqual([]);
});
+ // The comparison loop runs over the fields a type serializes and refuses a
+ // field it has no comparator for. That refusal is unreachable only while
+ // the table covers the whole of SERIALIZED_FIELDS, so the coverage is
+ // pinned here rather than assumed: adding a field to a type without a
+ // comparator would otherwise turn every transaction of that type into a
+ // refusal, and adding a comparator without the field would be a check that
+ // never runs.
+ test("every field a type serializes has a comparator", () => {
+ const serialized = new Set(
+ Object.values(SERIALIZED_FIELDS).flat().sort(),
+ );
+ expect([...serialized].filter((key) => !APPROVED_FIELDS[key])).toEqual(
+ [],
+ );
+ expect(
+ Object.keys(APPROVED_FIELDS).filter((key) => !serialized.has(key)),
+ ).toEqual([]);
+ });
+
// The two layers behind the type allowlist. Nothing reachable through
// verifySignedTx can trip either of them while the allowlist holds — that
// is what they are for — so they are exercised directly rather than taken
@@ -553,49 +747,30 @@ describe("verifySignedTx exhaustiveness", () => {
test("an accepted artifact of each allowed type rebuilds byte for byte", async () => {
const shapes = [
{
- approved: { ...TX_PARAMS, gasPrice: "0x77359400" },
- overrides: {
- type: 0,
- gasPrice: 2000000000n,
- maxFeePerGas: null,
- maxPriorityFeePerGas: null,
- },
+ type: 0,
+ gasPrice: 2000000000n,
+ maxFeePerGas: null,
+ maxPriorityFeePerGas: null,
},
{
- approved: {
- ...TX_PARAMS,
- gasPrice: "0x77359400",
- accessList: [
- {
- address: RECIPIENT,
- storageKeys: ["0x" + "11".repeat(32)],
- },
- ],
- },
- overrides: {
- type: 1,
- gasPrice: 2000000000n,
- maxFeePerGas: null,
- maxPriorityFeePerGas: null,
- accessList: [
- {
- address: RECIPIENT,
- storageKeys: ["0x" + "11".repeat(32)],
- },
- ],
- },
+ type: 1,
+ gasPrice: 2000000000n,
+ maxFeePerGas: null,
+ maxPriorityFeePerGas: null,
+ accessList: [
+ {
+ address: RECIPIENT,
+ storageKeys: ["0x" + "11".repeat(32)],
+ },
+ ],
},
- { approved: TX_PARAMS, overrides: {} },
+ {},
];
- for (const shape of shapes) {
- const raw = await signedFor(
- shape.approved,
- signer,
- shape.overrides,
- );
+ for (const overrides of shapes) {
+ const raw = await signedFor(TX_PARAMS, signer, overrides);
const parsed = verifySignedTx(
raw,
- shape.approved,
+ approvedFor(TX_PARAMS, overrides),
signer.address,
SELECTED,
);
@@ -644,7 +819,7 @@ describe("verifySignedTx canonical encoding", () => {
test("refuses an artifact that is not its own canonical encoding", async () => {
const mutated = await nonCanonical();
expect(() =>
- verifySignedTx(mutated, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(mutated, APPROVED, signer.address, SELECTED),
).toThrow(/not encoded canonically/);
});
@@ -659,7 +834,7 @@ describe("verifySignedTx canonical encoding", () => {
const raw = await signedWith({});
const upper = "0x" + raw.slice(2).toUpperCase();
expect(() =>
- verifySignedTx(upper, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(upper, APPROVED, signer.address, SELECTED),
).not.toThrow();
});
});
@@ -670,46 +845,33 @@ describe("verifySignedTx normalization", () => {
test("accepts a decimal chain id against a hex selected network", async () => {
const raw = await signedWith({});
expect(() =>
- verifySignedTx(raw, TX_PARAMS, signer.address, 1),
+ verifySignedTx(raw, APPROVED, signer.address, 1),
).not.toThrow();
expect(() =>
- verifySignedTx(raw, TX_PARAMS, signer.address, "1"),
+ verifySignedTx(raw, APPROVED, signer.address, "1"),
).not.toThrow();
});
- test("accepts an approved chain id written in hex", async () => {
+ // The approved transaction crosses to the popup as JSON, so it comes back
+ // spelled in hex quantities rather than in the bigints it was populated
+ // with. None of that is tampering.
+ test("accepts an approval spelled as the wire spells it", async () => {
const raw = await signedWith({});
- const approved = { ...TX_PARAMS, chainId: "0x1" };
+ const wire = {
+ ...APPROVED,
+ chainId: "0x1",
+ nonce: "0x7",
+ gasLimit: "0x186a0",
+ maxFeePerGas: "0x77359400",
+ maxPriorityFeePerGas: "0x3b9aca00",
+ value: "0x2386f26fc10000",
+ };
expect(() =>
- verifySignedTx(raw, approved, signer.address, SELECTED),
+ verifySignedTx(raw, wire, signer.address, SELECTED),
).not.toThrow();
});
- test("accepts a hex nonce against a numeric one", async () => {
- const raw = await signedWith({ nonce: 7 });
- expect(() =>
- verifySignedTx(
- raw,
- { ...TX_PARAMS, nonce: "0x7" },
- signer.address,
- SELECTED,
- ),
- ).not.toThrow();
- });
-
- test("accepts a decimal gas limit against a hex one", async () => {
- const raw = await signedWith({ gasLimit: 100000n });
- expect(() =>
- verifySignedTx(
- raw,
- { ...TX_PARAMS, gasLimit: "100000" },
- signer.address,
- SELECTED,
- ),
- ).not.toThrow();
- });
-
- test("accepts fee fields spelled as hex, decimal, number and bigint", async () => {
+ test("accepts quantities spelled as hex, decimal, number and bigint", async () => {
const raw = await signedWith({});
for (const maxFee of [
"0x77359400",
@@ -720,7 +882,7 @@ describe("verifySignedTx normalization", () => {
expect(() =>
verifySignedTx(
raw,
- { ...TX_PARAMS, maxFeePerGas: maxFee },
+ { ...APPROVED, maxFeePerGas: maxFee },
signer.address,
SELECTED,
),
@@ -728,24 +890,19 @@ describe("verifySignedTx normalization", () => {
}
});
- test("accepts an approval that fixes no nonce, gas or fee at all", async () => {
- const raw = await signedWith({});
- expect(() =>
- verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED),
- ).not.toThrow();
- });
-
test("accepts an approval whose recipient case differs", async () => {
const raw = await signedWith({});
- const approved = { ...TX_PARAMS, to: RECIPIENT.toLowerCase() };
+ const approved = { ...APPROVED, to: RECIPIENT.toLowerCase() };
expect(() =>
verifySignedTx(raw, approved, signer.address, SELECTED),
).not.toThrow();
});
test("accepts absent call data against 0x", async () => {
- const approved = { to: RECIPIENT, value: "0x0" };
- const raw = await signedFor({ ...approved, data: "0x" });
+ const params = { to: RECIPIENT, value: "0x0" };
+ const approved = approvedFor(params);
+ delete approved.data;
+ const raw = await signedFor({ ...params, data: "0x" });
expect(() =>
verifySignedTx(raw, approved, signer.address, SELECTED),
).not.toThrow();
@@ -756,7 +913,7 @@ describe("verifySignedTx normalization", () => {
expect(() =>
verifySignedTx(
raw,
- { ...TX_PARAMS, maxFeePerGas: "cheap" },
+ { ...APPROVED, maxFeePerGas: "cheap" },
signer.address,
SELECTED,
),
@@ -774,7 +931,7 @@ describe("verifySignedTx normalization", () => {
try {
verifySignedTx(
raw,
- { ...TX_PARAMS, value },
+ { ...APPROVED, value },
signer.address,
SELECTED,
);
@@ -793,7 +950,7 @@ describe("verifySignedTx normalization", () => {
expect(() =>
verifySignedTx(
raw,
- { ...TX_PARAMS, accessList: ["nope"] },
+ { ...APPROVED, accessList: ["nope"] },
signer.address,
SELECTED,
),
@@ -934,7 +1091,7 @@ describe("signing failure and retry", () => {
test("a mismatch spends the approval", async () => {
const raw = await signedFor({ ...TX_PARAMS, to: OTHER_RECIPIENT });
try {
- verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED);
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED);
throw new Error("expected a rejection");
} catch (e) {
expect(failureIsRetryable(e)).toBe(false);
@@ -1007,7 +1164,7 @@ describe("signing failure and retry", () => {
const raw = await signedFor({ ...TX_PARAMS, to: OTHER_RECIPIENT });
let outcome;
try {
- verifySignedTx(raw, TX_PARAMS, signer.address, SELECTED);
+ verifySignedTx(raw, APPROVED, signer.address, SELECTED);
} catch (e) {
outcome = describeTxFailure(TX_STAGE_VERIFY, e);
}
@@ -1061,12 +1218,13 @@ describe("signing failure and retry", () => {
});
});
-// End-to-end over the messaging boundary, without a browser: run the exact
-// sequence the approval popup runs, then hand the artifact to the exact check
-// the background runs before it broadcasts or resolves. Only what the popup
-// puts on the wire is passed along, so this also pins down that the wire
-// payload is sufficient on its own.
-describe("popup signing sequence to background verification", () => {
+// End-to-end over the messaging boundary, without a browser: the background
+// populates the transaction, the object that produces crosses to the popup as
+// JSON and is signed there, and the artifact goes back to the exact check the
+// background runs before it broadcasts. Only what each side puts on the wire is
+// passed along, so this also pins down that the wire payloads are sufficient on
+// their own.
+describe("background population to popup signing to verification", () => {
// Stand-in for the JSON-RPC provider. populateTransaction only needs the
// nonce, the gas estimate, the network and the fee data.
const fakeProvider = {
@@ -1084,33 +1242,52 @@ describe("popup signing sequence to background verification", () => {
// through getSignerForAddress() the way the popup does.
const walletData = { type: "privkey" };
- async function popupSignsTx(txParams) {
- const localSigner = getSignerForAddress(walletData, 0, SIGNER_KEY);
- const connected = localSigner.connect(fakeProvider);
- const populated = await connected.populateTransaction(txParams);
- delete populated.from;
- return connected.signTransaction(populated);
+ // What the background does before the approval window opens.
+ async function backgroundPrepares(txParams) {
+ const approvedTx = await prepareApprovalTx(
+ fakeProvider,
+ signer.address,
+ txParams,
+ );
+ // Extension messaging is JSON; the popup sees the other side of it.
+ return JSON.parse(JSON.stringify(approvedTx));
}
- test("a populated, signed transaction is accepted and broadcastable", async () => {
- const rawSignedTx = await popupSignsTx(TX_PARAMS);
+ // What the popup does with it: signs it as given, populating nothing.
+ async function popupSigns(approvedTx) {
+ const localSigner = getSignerForAddress(walletData, 0, SIGNER_KEY);
+ return localSigner.signTransaction({ ...approvedTx });
+ }
+
+ test("the populated transaction is what gets signed and what gets checked", async () => {
+ const approvedTx = await backgroundPrepares(TX_PARAMS);
+ const rawSignedTx = await popupSigns(approvedTx);
const parsed = verifySignedTx(
rawSignedTx,
- TX_PARAMS,
+ approvedTx,
signer.address,
SELECTED,
);
expect(parsed.nonce).toBe(7);
expect(parsed.chainId).toBe(1n);
expect(parsed.gasLimit).toBe(21000n);
+ expect(parsed.maxFeePerGas).toBe(2000000000n);
expect(parsed.to).toBe(RECIPIENT);
expect(parsed.value).toBe(BigInt(TX_PARAMS.value));
expect(parsed.data).toBe(TX_PARAMS.data);
expect(parsed.signature).not.toBeNull();
+ // Every field the screen shows, and the artifact, are the same numbers.
+ expect(BigInt(approvedTx.nonce)).toBe(BigInt(parsed.nonce));
+ expect(BigInt(approvedTx.gasLimit)).toBe(parsed.gasLimit);
+ expect(BigInt(approvedTx.maxFeePerGas)).toBe(parsed.maxFeePerGas);
+ expect(BigInt(approvedTx.maxPriorityFeePerGas)).toBe(
+ parsed.maxPriorityFeePerGas,
+ );
});
test("the wire payload carries no password and no secret", async () => {
- const rawSignedTx = await popupSignsTx(TX_PARAMS);
+ const approvedTx = await backgroundPrepares(TX_PARAMS);
+ const rawSignedTx = await popupSigns(approvedTx);
const payload = {
type: "AUTISTMASK_TX_RESPONSE",
id: "test-approval-id",
@@ -1128,20 +1305,54 @@ describe("popup signing sequence to background verification", () => {
expect(wire).not.toContain(SIGNER_KEY.slice(2).toLowerCase());
});
+ // The popup is the component whose compromise this check exists to detect,
+ // so it is given the approved transaction and signs something else.
+ test("a popup that signs a different fee than it was given is refused", async () => {
+ const approvedTx = await backgroundPrepares(TX_PARAMS);
+ const rawSignedTx = await popupSigns({
+ ...approvedTx,
+ maxFeePerGas: "0x3b9aca000",
+ });
+ expect(() =>
+ verifySignedTx(rawSignedTx, approvedTx, signer.address, SELECTED),
+ ).toThrow(/approved maximum fee per gas/);
+ });
+
+ test("a popup that signs a different nonce than it was given is refused", async () => {
+ const approvedTx = await backgroundPrepares(TX_PARAMS);
+ const rawSignedTx = await popupSigns({ ...approvedTx, nonce: "0x8" });
+ expect(() =>
+ verifySignedTx(rawSignedTx, approvedTx, signer.address, SELECTED),
+ ).toThrow(/approved nonce/);
+ });
+
test("the background rejects a transaction the popup did not approve", async () => {
- const rawSignedTx = await popupSignsTx({
- ...TX_PARAMS,
+ const approvedTx = await backgroundPrepares(TX_PARAMS);
+ const rawSignedTx = await popupSigns({
+ ...approvedTx,
to: OTHER_RECIPIENT,
});
expect(() =>
- verifySignedTx(rawSignedTx, TX_PARAMS, signer.address, SELECTED),
+ verifySignedTx(rawSignedTx, approvedTx, signer.address, SELECTED),
).toThrow(/approved recipient/);
});
test("the background rejects a transaction populated on another network", async () => {
- const rawSignedTx = await popupSignsTx(TX_PARAMS);
+ const approvedTx = await backgroundPrepares(TX_PARAMS);
+ const rawSignedTx = await popupSigns(approvedTx);
expect(() =>
- verifySignedTx(rawSignedTx, TX_PARAMS, signer.address, SEPOLIA),
+ verifySignedTx(rawSignedTx, approvedTx, signer.address, SEPOLIA),
).toThrow(/different network than the one that is selected/);
});
+
+ // ethers refuses to sign for an address that is not the key's own, so a
+ // popup working from the approved object cannot quietly sign as whichever
+ // address the user has switched to.
+ test("the approved from stops the popup signing with another key", async () => {
+ const approvedTx = await backgroundPrepares(TX_PARAMS);
+ const otherSigner = getSignerForAddress(walletData, 0, OTHER_KEY);
+ await expect(
+ otherSigner.signTransaction({ ...approvedTx }),
+ ).rejects.toThrow(/from address mismatch/);
+ });
});
diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js
index ed313dd..d326b81 100644
--- a/tests/backgroundApproval.test.js
+++ b/tests/backgroundApproval.test.js
@@ -8,16 +8,24 @@
// already saw — which means the entry being present is not by itself proof
// that no attempt is running. A second response carrying the same id (a
// reloaded approval window re-rendering a live Approve button, a popup that
-// emits the message twice) must not start a second verify and broadcast: with
-// the ordinary dApp approval shape the page fixes no nonce, so two artifacts
-// signed at different nonces both verify, and the approved transfer would go
-// out twice.
+// emits the message twice) must not start a second verify and broadcast: the
+// same approved transaction signed twice verifies twice, and the transfer
+// would go out twice.
+//
+// It also covers what the approval is verified against. The approval now
+// carries the transaction the background populated and the screen displayed,
+// and the address that was active when it was raised — so a fee, a nonce or an
+// address that moved between approval and signing is refused rather than
+// signed.
-const { Wallet } = require("ethers");
+const { Network, Wallet } = require("ethers");
const SIGNER_KEY =
"0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d";
+const OTHER_KEY =
+ "0x5de4111afa1a4b94908f83103eb1f1706367c2e68ca870fc3fb9a804cdab365a";
const signer = new Wallet(SIGNER_KEY);
+const other = new Wallet(OTHER_KEY);
const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
const ORIGIN = "https://dapp.example";
@@ -33,9 +41,16 @@ const TX_PARAMS = {
data: "0x",
};
-// The fields the popup's populateTransaction() would fill in. The nonce is a
-// parameter because the duplicate case turns on the two artifacts differing
-// in exactly the field nothing constrains.
+// The nonce the stubbed node reports, and so the nonce the background
+// populates the approval with.
+const NONCE = 7;
+
+// "Hello AutistMask" as the hex string a dApp passes to personal_sign.
+const MESSAGE = "0x48656c6c6f204175746973744d61736b";
+
+// The transaction the background populates and the approval screen displays.
+// The nonce is a parameter because the duplicate case turns on two artifacts
+// differing in a field the dApp fixed nothing for.
function populated(nonce) {
return {
type: 2,
@@ -50,8 +65,26 @@ function populated(nonce) {
};
}
-function signedAtNonce(nonce) {
- return signer.signTransaction(populated(nonce));
+function signedAtNonce(nonce, withWallet) {
+ return (withWallet || signer).signTransaction(populated(nonce));
+}
+
+// The node the background populates against. Its answers are the numbers the
+// approval screen shows, so they are also the numbers every artifact below is
+// signed at.
+function fakeProvider(broadcastTransaction, overrides) {
+ return {
+ broadcastTransaction,
+ getNetwork: async () => Network.from(1),
+ getTransactionCount: async () => NONCE,
+ estimateGas: async () => 100000n,
+ getFeeData: async () => ({
+ gasPrice: 2000000000n,
+ maxFeePerGas: 2000000000n,
+ maxPriorityFeePerGas: 1000000000n,
+ }),
+ ...(overrides || {}),
+ };
}
// A promise whose settlement the test controls, so a broadcast can be held in
@@ -85,7 +118,7 @@ function loadBackground(options) {
currentNetwork: () => ({ chainId: "0x1" }),
}));
jest.doMock("../src/shared/balances", () => ({
- getProvider: () => ({ broadcastTransaction }),
+ getProvider: () => fakeProvider(broadcastTransaction, opts.provider),
refreshBalances: jest.fn(async () => {}),
}));
jest.doMock("../src/shared/phishingDomains", () => ({
@@ -171,7 +204,7 @@ function loadBackground(options) {
// Raise a pending transaction approval the way a dApp does, and dig the
// approval id back out of the popup URL the background opened.
- function requestTx() {
+ function requestTx(txParams) {
let rpcResult = null;
const sendResponse = jest.fn((r) => {
rpcResult = r;
@@ -180,7 +213,7 @@ function loadBackground(options) {
{
type: "AUTISTMASK_RPC",
method: "eth_sendTransaction",
- params: [TX_PARAMS],
+ params: [txParams || TX_PARAMS],
},
{ origin: ORIGIN },
sendResponse,
@@ -191,6 +224,30 @@ function loadBackground(options) {
};
}
+ // The same for a message-signing approval, which pins the signing address
+ // at approval time in exactly the same way.
+ function requestSign(from) {
+ let rpcResult = null;
+ messageListener(
+ {
+ type: "AUTISTMASK_RPC",
+ method: "personal_sign",
+ params: [MESSAGE, from || signer.address],
+ },
+ { origin: ORIGIN },
+ (r) => {
+ rpcResult = r;
+ },
+ );
+ return {
+ id: () =>
+ new URL(created[created.length - 1].url).searchParams.get(
+ "approval",
+ ),
+ result: () => rpcResult,
+ };
+ }
+
// The user closes the approval popup. `created` is index-aligned with the
// ids the window stub hands back, so window 1 is the first popup opened.
function closeWindow(windowId) {
@@ -200,18 +257,27 @@ function loadBackground(options) {
return {
send,
requestTx,
+ requestSign,
closeWindow,
broadcastTransaction,
loadState,
created,
removed,
+ // The user switching account in the toolbar popup, as the background
+ // sees it: the persisted active address changes underneath a pending
+ // approval.
+ setActiveAddress: (address) => {
+ persisted.activeAddress = address;
+ },
fromPopup: { url: EXT_URL + "src/popup/index.html" },
};
}
-// Let the handler's promise chain run to the next suspension point.
+// Let the handler's promise chain run to the next suspension point. Raising a
+// transaction approval now populates it against the node first, which is
+// several awaits deep before the window is opened.
async function settle() {
- for (let i = 0; i < 10; i++) await Promise.resolve();
+ for (let i = 0; i < 50; i++) await Promise.resolve();
}
afterEach(() => {
@@ -244,9 +310,11 @@ describe("one approval, one broadcast", () => {
await settle();
expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
- // A reloaded approval window signs the same approval again. Nothing
- // in the approval fixes a nonce, so this artifact verifies just as
- // well as the first one.
+ // A reloaded approval window signs the same approval again, at another
+ // nonce. The claim is taken before anything is verified, so what this
+ // asserts is the interlock and not the nonce comparison: the refusal
+ // below is the claim's own message, which a verification failure does
+ // not produce.
const second = bg.send(
{
type: "AUTISTMASK_TX_RESPONSE",
@@ -376,6 +444,319 @@ describe("one approval, one broadcast", () => {
});
});
+// The approval carries the transaction the user was shown and the address it
+// was raised for, and the artifact is checked against both. Every case here is
+// one the old comparison — against the dApp's request, for the address that is
+// active now — would have broadcast.
+describe("what the approval is verified against", () => {
+ // The approval screen showed the populated fee. An artifact at ten times
+ // that fee, still far below the ceilings, is what the ceilings alone could
+ // not catch.
+ test("a fee differing from the displayed one is refused, not sent", async () => {
+ const bg = loadBackground();
+ const pending = bg.requestTx();
+ await settle();
+ const id = pending.id();
+
+ const raw = await signer.signTransaction({
+ ...populated(NONCE),
+ maxFeePerGas: 20000000000n,
+ });
+ const answer = bg.send(
+ {
+ type: "AUTISTMASK_TX_RESPONSE",
+ id,
+ approved: true,
+ rawSignedTx: raw,
+ },
+ { url: bg.fromPopup.url },
+ );
+ await settle();
+
+ expect(bg.broadcastTransaction).not.toHaveBeenCalled();
+ expect(answer.sendResponse).toHaveBeenCalledWith(
+ expect.objectContaining({
+ error: expect.stringMatching(/approved maximum fee per gas/),
+ retryable: false,
+ stage: "verify",
+ }),
+ );
+ expect(pending.result()).toEqual({
+ error: {
+ message: expect.stringMatching(/approved maximum fee per gas/),
+ },
+ });
+ });
+
+ test("a nonce differing from the displayed one is refused, not sent", async () => {
+ const bg = loadBackground();
+ const pending = bg.requestTx();
+ await settle();
+ const id = pending.id();
+
+ const answer = bg.send(
+ {
+ type: "AUTISTMASK_TX_RESPONSE",
+ id,
+ approved: true,
+ rawSignedTx: await signedAtNonce(NONCE + 1),
+ },
+ { url: bg.fromPopup.url },
+ );
+ await settle();
+
+ expect(bg.broadcastTransaction).not.toHaveBeenCalled();
+ expect(answer.sendResponse).toHaveBeenCalledWith(
+ expect.objectContaining({
+ error: expect.stringMatching(/approved nonce/),
+ retryable: false,
+ stage: "verify",
+ }),
+ );
+ });
+
+ // The address switch. The approval named one account; the wallet is on
+ // another by the time the artifact arrives. Both halves are covered: the
+ // popup signing as the account that is active now, and the popup correctly
+ // signing as the approved account while the wallet has moved on.
+ test("an artifact signed by the address that is active now is refused", async () => {
+ const bg = loadBackground();
+ const pending = bg.requestTx();
+ await settle();
+ const id = pending.id();
+
+ bg.setActiveAddress(other.address);
+ const answer = bg.send(
+ {
+ type: "AUTISTMASK_TX_RESPONSE",
+ id,
+ approved: true,
+ rawSignedTx: await signedAtNonce(NONCE, other),
+ },
+ { url: bg.fromPopup.url },
+ );
+ await settle();
+
+ expect(bg.broadcastTransaction).not.toHaveBeenCalled();
+ expect(answer.sendResponse).toHaveBeenCalledWith(
+ expect.objectContaining({ retryable: false, stage: "verify" }),
+ );
+ expect(pending.result()).toEqual({
+ error: { message: expect.stringMatching(/active address changed/) },
+ });
+ });
+
+ test("an address switch refuses even the correctly signed artifact", async () => {
+ const bg = loadBackground();
+ const pending = bg.requestTx();
+ await settle();
+ const id = pending.id();
+
+ bg.setActiveAddress(other.address);
+ const answer = bg.send(
+ {
+ type: "AUTISTMASK_TX_RESPONSE",
+ id,
+ approved: true,
+ rawSignedTx: await signedAtNonce(NONCE),
+ },
+ { url: bg.fromPopup.url },
+ );
+ await settle();
+
+ expect(bg.broadcastTransaction).not.toHaveBeenCalled();
+ expect(answer.sendResponse).toHaveBeenCalledWith(
+ expect.objectContaining({
+ error: expect.stringMatching(/active address changed/),
+ retryable: false,
+ stage: "verify",
+ }),
+ );
+ // A refusal, so the approval is spent: the same artifact offered again
+ // finds nothing to answer.
+ const retry = bg.send(
+ {
+ type: "AUTISTMASK_TX_RESPONSE",
+ id,
+ approved: true,
+ rawSignedTx: await signedAtNonce(NONCE),
+ },
+ { url: bg.fromPopup.url },
+ );
+ await settle();
+ expect(retry.sendResponse).not.toHaveBeenCalled();
+ expect(bg.broadcastTransaction).not.toHaveBeenCalled();
+ });
+
+ test("a switch back to the approved address still sends", async () => {
+ const bg = loadBackground();
+ const pending = bg.requestTx();
+ await settle();
+ const id = pending.id();
+
+ bg.setActiveAddress(other.address);
+ bg.setActiveAddress(signer.address);
+ bg.broadcastTransaction.mockResolvedValue({ hash: "0xfeed" });
+ bg.send(
+ {
+ type: "AUTISTMASK_TX_RESPONSE",
+ id,
+ approved: true,
+ rawSignedTx: await signedAtNonce(NONCE),
+ },
+ { url: bg.fromPopup.url },
+ );
+ await settle();
+
+ expect(bg.broadcastTransaction).toHaveBeenCalledTimes(1);
+ expect(pending.result()).toEqual({ result: "0xfeed" });
+ });
+
+ // The popup is handed the populated transaction and the address it is for,
+ // and nothing else it would have to fetch or decide.
+ test("the popup is given the transaction it is to sign", async () => {
+ const bg = loadBackground();
+ const pending = bg.requestTx();
+ await settle();
+
+ const details = bg.send(
+ { type: "AUTISTMASK_GET_APPROVAL", id: pending.id() },
+ { url: bg.fromPopup.url },
+ );
+ const shown = details.sendResponse.mock.calls[0][0];
+ expect(shown.type).toBe("tx");
+ expect(shown.approvedFrom).toBe(signer.address);
+ expect(shown.approvedTx).toEqual({
+ type: 2,
+ from: signer.address,
+ chainId: "0x1",
+ nonce: "0x7",
+ gasLimit: "0x186a0",
+ maxFeePerGas: "0x77359400",
+ maxPriorityFeePerGas: "0x3b9aca00",
+ to: RECIPIENT,
+ value: TX_PARAMS.value,
+ data: "0x",
+ accessList: [],
+ });
+ });
+
+ // A request naming an account the wallet is not on is refused outright
+ // rather than signed as whichever account is active.
+ test("a request from another address raises no approval at all", async () => {
+ const bg = loadBackground();
+ const pending = bg.requestTx({ ...TX_PARAMS, from: other.address });
+ await settle();
+
+ expect(pending.result()).toEqual({
+ error: {
+ code: 4100,
+ message: expect.stringMatching(/not the active one/),
+ },
+ });
+ expect(bg.created).toEqual([]);
+ });
+
+ // Message signing pins the address the same way, and refuses the same way.
+ // A signature is not a transaction, but a permit signed by an account the
+ // approval did not name spends that account's tokens all the same.
+ test("a sign approval refuses a signature after an address switch", async () => {
+ const bg = loadBackground();
+ const pending = bg.requestSign();
+ await settle();
+
+ bg.setActiveAddress(other.address);
+ const answer = bg.send(
+ {
+ type: "AUTISTMASK_SIGN_RESPONSE",
+ id: pending.id(),
+ approved: true,
+ signature: await signer.signMessage(
+ Buffer.from(MESSAGE.slice(2), "hex"),
+ ),
+ },
+ { url: bg.fromPopup.url },
+ );
+ await settle();
+
+ expect(answer.sendResponse).toHaveBeenCalledWith(
+ expect.objectContaining({
+ error: expect.stringMatching(/active address changed/),
+ retryable: false,
+ }),
+ );
+ expect(pending.result()).toEqual({
+ error: { message: expect.stringMatching(/active address changed/) },
+ });
+ });
+
+ test("a sign request from another address raises no approval at all", async () => {
+ const bg = loadBackground();
+ const pending = bg.requestSign(other.address);
+ await settle();
+
+ expect(pending.result()).toEqual({
+ error: {
+ code: 4100,
+ message: expect.stringMatching(/not the active one/),
+ },
+ });
+ expect(bg.created).toEqual([]);
+ });
+
+ // Population is a network round trip with the user's hands free. An
+ // approval raised for the address that was active when it started could
+ // never be signed once the wallet has moved off it, so it is never raised.
+ test("an address switch during population raises no approval", async () => {
+ let bg;
+ bg = loadBackground({
+ provider: {
+ // The user switches account in the toolbar popup while the
+ // node is being asked for a gas estimate.
+ estimateGas: async () => {
+ bg.setActiveAddress(other.address);
+ return 100000n;
+ },
+ },
+ });
+ const pending = bg.requestTx();
+ await settle();
+
+ expect(pending.result()).toEqual({
+ error: {
+ message: expect.stringMatching(
+ /active address changed while this transaction was being prepared/,
+ ),
+ },
+ });
+ expect(bg.created).toEqual([]);
+ });
+
+ // Population happens before the window exists, so its failure is a failure
+ // of the request: no approval, no window, and the error goes back to the
+ // page the click came from.
+ test("a transaction that cannot be prepared opens no window", async () => {
+ const bg = loadBackground({
+ provider: {
+ estimateGas: async () => {
+ throw new Error("execution reverted");
+ },
+ },
+ });
+ const pending = bg.requestTx();
+ await settle();
+
+ expect(pending.result()).toEqual({
+ error: {
+ message: expect.stringMatching(
+ /could not be prepared.*execution reverted/,
+ ),
+ },
+ });
+ expect(bg.created).toEqual([]);
+ });
+});
+
// The interlock must not cost the retry the approval exists to allow.
describe("the interlock releases a failed attempt", () => {
test("a retryable failure before the broadcast leaves the approval usable", async () => {