harden: verify the signed transaction against what the popup displayed (closes #216)
All checks were successful
check / check (push) Successful in 32s
All checks were successful
check / check (push) Successful in 32s
The signed artifact was compared with the dApp's request object, so every field the dApp left out — normally the nonce, the gas limit and every fee field, because populateTransaction() filled them in the popup — was checked by nothing but the absolute ceilings. A bare transfer at the fee ceiling hands the validator 2.1 ETH. The ceilings were never the defect: the thing being verified was not the thing the user approved. The transaction is now populated in the background, before the approval window opens, and that populated object is what is displayed, what the popup signs, and what the artifact is verified against. Every consequential field is compared exactly. - src/shared/approvalTx.js populates the request through a VoidSigner over the configured RPC and serializes the result to the fields its type serializes, as hex quantities that survive the JSON messaging boundary. Fields the wallet does not act on are dropped before ethers sees the page's object. - Population failure raises no approval and opens no window: the error goes back to the requesting page, bounded by a 20-second timeout. A half-initialised approval record would be exactly the state the settle interlock exists to keep out of that record, and the same estimate previously failed after the user had typed their password. - verifySignedTx compares the artifact field by field over SERIALIZED_FIELDS[type], plus the type itself. A quantity the approval does not fix is a refusal rather than a skipped comparison. The ceilings stay as a documented backstop and now also apply at population, where they bound what an RPC node can talk the wallet into displaying. - The approval pins the address it was raised for. Verification uses that address, not getActiveAddress(), and an address switch between approval and signing refuses rather than signing from an account the screen never named — including a switch during population, and on the message-signing path. A request naming an address that is not the active one is refused outright. - The approval screen shows the network, gas limit, fee per gas, maximum fee and nonce it now vouches for, and the popup signs the object it was given with no provider and no population of its own. The settle chokepoint is untouched: one delete of pendingApprovals and one approval.resolve(), both inside settleApproval(), the claim taken synchronously before the first await, and a refused settle still leaving the approval window standing.
This commit is contained in:
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user