harden: approval verification compares against the dApp's request, not against what the popup displayed #216
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
verifySignedTxcompares the signed transaction against the dApp's request object. It never compares against the values the popup actually rendered to the user.For every field the dApp omitted — normally all of
nonce,gasLimitand the fee fields, becausepopulateTransaction()fills them in the popup — the number the user read on screen is verified by nothing. #174 works around this with absolute ceilings instead of equality, which bounds the absurd but not the ruinous: a bare 21,000-gas transfer at the currentMAX_FEE_PER_GASceiling hands the validator 2.1 ETH, andMAX_GAS_LIMIT * MAX_FEE_PER_GASbounds the worst case at 10,000 ETH, against roughly 0.001 ETH for a legitimate 50-gwei transfer.The ceilings are not the defect — the issue asked for a sanity bound and got one. The defect is structural: the thing being verified is not the thing the user approved.
Related, and worth resolving in the same change:
txParams.fromis never compared.expectedFromis the current active address fromgetActiveAddress(), so an active-address switch between approval and signing produces a transaction from an account the approval did not name. Pre-existing, not introduced by #174.Found by the independent review of #205.
Options
Recommendation
(a). It is the only option where the verified object and the displayed object are the same object, which is the property the module is supposed to provide. (b) asks the untrusted side to report what it showed. (c) leaves a wallet able to sign a transaction whose fee the user never agreed to.
Definition of done
to,value,data,chainId,nonce, gas limit and all fee fields.fromis compared against the address named at approval time, not the current active address; an address switch mid-flow refuses rather than signs.TODO.mdupdated in the same commit.make checkpasses.Plan, implementing option (a).
Where population goes. New
src/shared/approvalTx.js:prepareApprovalTx(provider, from, txParams)runsVoidSigner(from, provider).populateTransaction()— the same sequence the popup ran — and serializes the result to a JSON-safe wire object carrying exactly the serialized fields of its type (SERIALIZED_FIELDSfromapprovalVerify.js), plusfrom. The background calls it ineth_sendTransactionbeforerequestTxApproval(), so a pending approval only ever exists fully populated.Failure behaviour. No approval record and no window are created until population succeeds. On failure — RPC down, estimate revert, a populated type outside
ALLOWED_TX_TYPES, a fee past the ceilings, or a 20s timeout —eth_sendTransactionreturns the error to the dApp and no window opens. The cost is that the wallet window appears only after the estimate returns and the user sees nothing in the wallet on failure; the gain is that the alternative (open first, populate behind a spinner) needs a half-initialised approval record, which is the state shape the settle interlock from #205 exists to protect. Today an estimate failure happens after the user has typed their password; this moves it earlier, not later.from. The active address is captured asapproval.approvedFromwhen the approval is raised, and that — notgetActiveAddress()— is theexpectedFromhanded toverifySignedTx()/verifySignature(). The signing handler additionally refuses when the current active address is no longerapprovedFrom(a refusal, so the approval is spent). A dApp-suppliedtxParams.fromthat is not the active address refuses the request up front.fromalso goes to the popup inside the approved object, so ethers' owntransaction from address mismatchcatches a switch popup-side too.Verification.
verifySignedTxcompares the artifact against the populated-and-displayed object field by field overSERIALIZED_FIELDS[type]— nonce, gas limit and the fee fields become exact equality instead of "compare only if the dApp fixed it" — plus an exacttypecomparison. A quantity the approval does not carry is now a refusal, not a skip.assertNothingUnchecked,assertCanonicalBytes,assertNoForbiddenFields, the type allowlist and the ceilings all stay; the ceilings are re-documented as a backstop against a lying RPC (they now bound what the node can talk the wallet into displaying, not what the popup can sign).Popup. No
populateTransaction()and no provider at signing time — it signs the object it was given. The approval screen gains the fields it now vouches for: network, gas limit, max fee per gas, max total fee, nonce.Tests. Address switch between approval and signing (both directions: popup signs as the new address; active address moved under an artifact signed as the approved one); a fee and a nonce differing from the displayed value; population unit tests over a stub provider. The duplicate-broadcast and settle-interlock tests keep asserting on the claim path's own message, so they still discriminate the interlock from a verification refusal.
Implemented in #269 (branch
harden/issue-216-verify-displayed-values, basenext). The PR body carries the full record; the definition of done, item by item:src/shared/approvalTx.jsbefore the approval window opens; that object is displayed, signed as given, and compared field by field overSERIALIZED_FIELDS[type]plus the type itself.nonce, gas limit and every fee field are now exact equality, and a quantity the approval does not fix is a refusal rather than a skipped comparison.fromat approval time. The approval pinsapprovedFrom; verification uses it instead ofgetActiveAddress(). A switch between approval and signing refuses (and spends the approval), including a switch during population, and on the message-signing path, which had the same defect. A request naming a non-active address is refused before any window opens.assertWithinCeilings()now also runs at population.TODO.mdupdated in the same commit.make checkgreen on the rebased branch: 26 suites, 613 tests,test-verify-build18 cases,prettier --checkclean.make buildemits both bundles withDEBUGoff.Failure behaviour worth flagging for review: population failure raises no approval and opens no window — the error goes back to the requesting page, bounded by a 20-second timeout. Reasoning is in the PR body.