harden: bound the total network fee by gasLimit × fee, on both send paths (closes #399)
The two per-field ceilings in approvalVerify.js were checked independently, but the fee a validator is paid is gasLimit × fee per gas: a gas limit and a fee each under their own ceiling still multiply to thousands of ETH, which a gas-consuming contract really collects. assertWithinCeilings now also bounds that product against MAX_TOTAL_FEE (1 ETH), so both callers — populating the dApp transaction and verifying the signed artifact — refuse it with a full sentence naming the fee and the limit. The wallet's own send in confirmTx.js pinned no fee fields, so ethers filled them from the node with no bound; it now populates the transaction and runs the same check before signing, showing the same error in the confirmation screen's reserved errors box so nothing on screen moves. Model: opus-4-8
This commit was merged in pull request #411.
This commit is contained in:
@@ -30,6 +30,7 @@ const {
|
||||
displayedDecimals,
|
||||
transferAmountUnits,
|
||||
} = require("../../shared/transferAmount");
|
||||
const { assertWithinCeilings } = require("../../shared/approvalVerify");
|
||||
const {
|
||||
CODES,
|
||||
FEE_PENDING,
|
||||
@@ -394,6 +395,46 @@ async function estimateGas(txInfo) {
|
||||
}
|
||||
}
|
||||
|
||||
// Populate the transaction this send describes, enforce the fee bound against
|
||||
// the fees that were actually filled in, then sign and broadcast it. The send
|
||||
// pins no fee fields, so ethers fills maxFeePerGas and the gas limit from what
|
||||
// the configured RPC node answers, with nothing otherwise bounding what a
|
||||
// hostile node can set — the dApp path's ceilings never reached this one.
|
||||
// Populating before the check is what makes assertWithinCeilings() see the
|
||||
// same numbers that would be signed; it throws an ApprovalMismatchError when
|
||||
// the product gasLimit × maxFeePerGas is over the bound, which the caller
|
||||
// shows in the reserved error area rather than sending.
|
||||
async function populateVerifyAndSend(connectedSigner, tx) {
|
||||
let request;
|
||||
if (tx.token === "ETH") {
|
||||
request = { to: tx.to, value: parseEther(tx.amount) };
|
||||
} else {
|
||||
const contract = new Contract(tx.token, ERC20_ABI, connectedSigner);
|
||||
// The contract's decimals() is read to be COMPARED with the scale the
|
||||
// screen rendered this amount at, not to encode with: encoding from it
|
||||
// signs whatever the contract answers now, which is not what the user
|
||||
// read. A disagreement throws. See transferAmount.js.
|
||||
const amount = transferAmountUnits(
|
||||
tx.amount,
|
||||
tx.tokenDecimals,
|
||||
await contract.decimals(),
|
||||
);
|
||||
request = await contract.transfer.populateTransaction(tx.to, amount);
|
||||
}
|
||||
const populated = await connectedSigner.populateTransaction(request);
|
||||
assertWithinCeilings(populated);
|
||||
return connectedSigner.sendTransaction(populated);
|
||||
}
|
||||
|
||||
// Show a full-sentence send failure in the reserved errors box, the same
|
||||
// element and markup renderValidation() uses for messages carrying the user's
|
||||
// own numbers, so it never moves anything on the screen.
|
||||
function showSendError(message) {
|
||||
const el = $("confirm-errors");
|
||||
el.innerHTML = `<div class="text-xs">${escapeHtml(message)}</div>`;
|
||||
el.style.visibility = "visible";
|
||||
}
|
||||
|
||||
async function checkRecipientHistory(txInfo) {
|
||||
try {
|
||||
const provider = getProvider(state.rpcUrl, state.networkId);
|
||||
@@ -467,29 +508,7 @@ function init(_ctx) {
|
||||
const provider = getProvider(state.rpcUrl, state.networkId);
|
||||
const connectedSigner = signer.connect(provider);
|
||||
|
||||
if (pendingTx.token === "ETH") {
|
||||
tx = await connectedSigner.sendTransaction({
|
||||
to: pendingTx.to,
|
||||
value: parseEther(pendingTx.amount),
|
||||
});
|
||||
} else {
|
||||
const contract = new Contract(
|
||||
pendingTx.token,
|
||||
ERC20_ABI,
|
||||
connectedSigner,
|
||||
);
|
||||
// The contract's decimals() is read to be COMPARED with the
|
||||
// scale the screen rendered this amount at, not to encode with:
|
||||
// encoding from it signs whatever the contract answers now,
|
||||
// which is not what the user read. A disagreement throws and is
|
||||
// reported on the error screen. See transferAmount.js.
|
||||
const amount = transferAmountUnits(
|
||||
pendingTx.amount,
|
||||
pendingTx.tokenDecimals,
|
||||
await contract.decimals(),
|
||||
);
|
||||
tx = await contract.transfer(pendingTx.to, amount);
|
||||
}
|
||||
tx = await populateVerifyAndSend(connectedSigner, pendingTx);
|
||||
|
||||
// Best-effort: clear decrypted secret after use.
|
||||
// Note: JS strings are immutable; this nulls the reference but
|
||||
@@ -498,6 +517,14 @@ function init(_ctx) {
|
||||
txStatus.showWait(pendingTx, tx.hash);
|
||||
} catch (e) {
|
||||
decryptedSecret = null;
|
||||
// A fee over the bound is refused before anything is broadcast, so
|
||||
// there is no transaction that may have reached the network to warn
|
||||
// about: the message stays on the confirmation screen where the
|
||||
// user can go back, rather than routing to the sent/failed screen.
|
||||
if (e && e.approvalMismatch) {
|
||||
showSendError(e.message);
|
||||
return;
|
||||
}
|
||||
const hash = tx ? tx.hash : null;
|
||||
txStatus.showError(pendingTx, hash, e.shortMessage || e.message);
|
||||
} finally {
|
||||
@@ -511,4 +538,4 @@ function init(_ctx) {
|
||||
});
|
||||
}
|
||||
|
||||
module.exports = { init, show, restore };
|
||||
module.exports = { init, show, restore, populateVerifyAndSend };
|
||||
|
||||
@@ -51,6 +51,7 @@
|
||||
const {
|
||||
Transaction,
|
||||
accessListify,
|
||||
formatEther,
|
||||
getAddress,
|
||||
getBytes,
|
||||
verifyMessage,
|
||||
@@ -134,10 +135,19 @@ const FORBIDDEN_FIELDS = [
|
||||
const MAX_GAS_LIMIT = 100000000n;
|
||||
|
||||
// 100,000 gwei per gas: orders of magnitude above the highest fee either
|
||||
// supported network has produced, and low enough to catch a fee that would
|
||||
// hand the validator the balance.
|
||||
// supported network has produced.
|
||||
const MAX_FEE_PER_GAS = 100000000000000n;
|
||||
|
||||
// The largest total fee this wallet will sign, in wei. The two ceilings above
|
||||
// bound the gas limit and the price per gas each on its own, but the fee a
|
||||
// validator is actually paid is their product, and a gas limit and a price
|
||||
// that are each under their own ceiling still multiply to thousands of ETH —
|
||||
// 30,000,000 gas at 100,000 gwei is about 3,000 ETH. Bounding the product is
|
||||
// what catches a fee that would hand the validator the balance; the per-field
|
||||
// ceilings alone do not. A full 30,000,000-gas block at 33 gwei reaches this,
|
||||
// which no ordinary wallet transaction approaches.
|
||||
const MAX_TOTAL_FEE = 1000000000000000000n; // 1 ETH
|
||||
|
||||
// A refusal to act on an artifact: it is not the thing that was approved, so
|
||||
// the approval it was offered against is spent and must not be retried. Every
|
||||
// throw in this module is one of these; the background distinguishes them from
|
||||
@@ -384,6 +394,28 @@ function assertWithinCeilings(tx) {
|
||||
);
|
||||
}
|
||||
}
|
||||
// The product: gasLimit × the most this transaction could pay per gas —
|
||||
// maxFeePerGas for a type-2 transaction, gasPrice for a legacy or type-1
|
||||
// one. This is the fee a gas-consuming contract can really extract, and it
|
||||
// is the bound the two per-field ceilings above cannot express.
|
||||
if (present(tx.gasLimit)) {
|
||||
const gasLimit = normalizeQuantity(tx.gasLimit, "gas limit");
|
||||
let price = null;
|
||||
if (present(tx.maxFeePerGas)) {
|
||||
price = normalizeQuantity(tx.maxFeePerGas, "maximum fee per gas");
|
||||
} else if (present(tx.gasPrice)) {
|
||||
price = normalizeQuantity(tx.gasPrice, "gas price");
|
||||
}
|
||||
if (price !== null && gasLimit * price > MAX_TOTAL_FEE) {
|
||||
throw refuse(
|
||||
"This transaction would allow a network fee of up to " +
|
||||
formatEther(gasLimit * price) +
|
||||
" ETH, which is more than the " +
|
||||
formatEther(MAX_TOTAL_FEE) +
|
||||
" ETH this wallet will sign for.",
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Refuse a field only a transaction type this wallet does not sign can carry.
|
||||
@@ -775,4 +807,5 @@ module.exports = {
|
||||
TX_STAGE_NONCE,
|
||||
MAX_GAS_LIMIT,
|
||||
MAX_FEE_PER_GAS,
|
||||
MAX_TOTAL_FEE,
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user