clawbot
918e581ef3
Some checks failed
check / check (push) Has been cancelled
Verification compared the signed artifact against the dApp's request object. For every field the dApp omitted -- normally nonce, gas limit and all the fee fields, since the popup filled them in -- the number the user actually read on screen was verified by nothing, and only absolute ceilings stood behind it. The transaction is now populated in the background before the approval window opens, and that populated object is both what the popup displays and what the signed artifact is verified against. Every consequential field becomes an equality comparison; the ceilings remain as a backstop. Population failing means no approval and no window, and the error goes to the requesting page -- earlier than before, where the same estimate failed after the password had been typed. The account is pinned too: `from` is compared against the address named at approval time rather than whichever address is active at signing, so switching accounts mid-flow refuses instead of signing from an account the approval did not name. The message-signing path had the same defect and gets the same fix. Nonce selection moves earlier as a consequence; the concurrent-approval case that follows from it is tracked at #271.
310 lines
11 KiB
JavaScript
310 lines
11 KiB
JavaScript
// 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");
|
|
});
|
|
});
|