End blind signing: the approval drawer says what the transaction actually does - #235
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f91543ded
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4f91543 to
8be609a
Compare
Codex P1 on #235: coinSymbol is the receipt's only unit and history reads amount, fee and network from it, so a token transfer is mislabelled whichever coin it is filed under. Recorded with the explorer-link item, which needs the same asset-vs-chain split.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e28605e2d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6339b2bb7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Ten decisions for the calldata phase. The load-bearing ones: no network call on the signing path, an unresolved token shows raw units rather than a guessed 18 decimals, and a known router with an unrecognised selector falls through to "unknown call" instead of a partial guess. Also records two things the scout found that change the shape of the work -- web3dart already ships the ABI codec the roadmap assumed we'd build, and personal_sign/eth_signTypedData currently throw before their drawer opens and leave the dApp with no response at all.
…-07 sign-method fix
Checked the claim against our own recorded live response rather than the router's documentation. arg0 and arg1 at fixed offsets are exactly fromToken.address and fromAmount; toToken appears only nested at a route-dependent position and toAmount not at all. So the drawer says what is being spent and admits it cannot read what is being received. Locating the destination by scanning the 2.3KB blob for a known token address is a heuristic a hostile payload can seed, and the last screen before a signature is the wrong place for one. This does not meet DAP-02's literal "X to Y" and the phase should report that as a deviation, not as a pass.
Wave 1 is a tracer: one ERC-20 transfer decoded from calldata to drawer, with the branch baseline measured rather than assumed. Waves 2-4 expand out from it - approve and the unverified token, the unknown-call surface and the two sign methods that currently hang the caller, then Squid's input side and the phase gate. Settles the open question research left the planner: a resolved-token transfer reuses SendTransactionDetails (two symbol params, not one, so gas stays native); everything else gets one new content widget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- analyze 0 issues/exit 0, tests +1216 ~3/exit 0, brace + raw-colors clean - whole-tree dart format is already red at baseline (365 generated files under squidrouter/ and banxa/); CI's scoped 'dart format lib test' is the real gate
…data Covers the real recipient and amount, the selector-vs-ABI cross-check, and every case that must NOT become a confident token send: an unknown token, an unreadable decimals value (never assumed to be 18), a transfer that also moves native value, and malformed calldata.
…nsfer An eth_sendTransaction carrying transfer(address,uint256) used to render the token CONTRACT as the payee and the token amount as ETH. It now decodes the calldata offline and, when the contract is a coin the wallet already holds, shows the true recipient, the amount at that coin's decimals, and its symbol. - decoding reads tx without writing to it; the signed bytes are unchanged - symbol and decimals come only from the wallet's own coin list, never from calldata, so a hostile contract cannot name itself on screen - an unknown token, an unreadable decimals value, or a transfer that also moves native currency stays on the undecoded path; 18 is never assumed - SendTransactionDetails takes amount and fee symbols separately, since gas is always paid in the native currency - transfer/approve join the existing ERC-20 ABI, and a test pins the hardcoded selectors against the ones that ABI derives
The tx map travels on to the signer by reference, so a key normalised while reading it is a difference between what the drawer showed and what the user signed. Snapshots the map before a decode and compares it key by key afterwards, with a mixed-case contract address so an in-place lowercase is the one thing that goes red. Verified by injecting tx['to'] = target into summarizeTransaction: the test failed with 'to was rewritten during decoding'. Reverted.
…p owed Records the measured before/after for all five gates, the three deviations from the plan, and the pre-existing decision IDs in lib/reown/ comments that block this plan's own grep gate.
Marks 30-01 done in the roadmap, points STATE at the executed tracer, and logs the two cleanups this plan found but deliberately did not make: the stale decision IDs in untouched lib/reown/ comments, and the STATE.md corruption the gsd state verbs produce on this repo.
An approval, an unreadable token and a call that moves two things at once all need a body that states what could be read and warns about the rest. The send body cannot say any of that: its hero asserts an amount leaves the wallet. - DappCallRow carries label, value and whether the value must reach the clipboard whole - no amount hero and no "You send" row, by construction - the Details heading travels with its rows, so an empty list draws no empty frame - pumped through the real drawer in both appearances
An approve was falling through to the native-send path, which describes money leaving the wallet -- the one thing an approval does not do. It now decodes to its own kind, naming the spender, the token and the allowance, and never reaches the send body. The unlimited case is a single comparison at half of 2^256: it catches the max-uint256 sentinel exactly, tolerates the off-by-a-little variants dApp SDKs emit, and sits far above any real supply, so it needs no totalSupply call on the signing path. Headline, warning and rows are pure functions rather than inline drawer code, so a test can hold the whole path from calldata to the words on screen.
An unresolved coin, an unreadable decimals value and a token call that also moves native currency were all landing on the unknown-call path, which said nothing at all. They now share one honest surface: the raw base-unit integer, marked as smallest units, beside the full contract address. Nothing substitutes eighteen for a missing decimals value -- that guess renders a confident wrong amount, which is worse than saying the token is unknown. A group of decoder cases fails if a convenience default is ever added. A non-zero native value keeps BOTH figures on screen rather than hiding one behind the other. A value that cannot be read at all is still not a zero value, and stays on the unknown-call path.
Case 6 asserts every decimal on screen is one of the four values the drawer was handed, which a decoded amount fails by construction. The answer is a sibling case with its own fixture and its own allow-set, not a looser assertion in the one that already ships: Case 6 and all six approve/reject/dismiss outcomes are untouched. The new case also pins the half the symbol parameter exists for -- the row stating what leaves the wallet names the token, while the gas rows below it still name the chain's own currency.
Calldata that decodes to nothing is no longer filed as a plain send: a payload present but unreadable becomes an unknown contract call carrying the contract, the native value and the selector, and a warning that says the wallet could not read it. The raw `event.params.toString()` block is deleted rather than restyled. It was the last fixed dark fill on this path, and a debug dump read as reassurance is worse than reading nothing.
…tion The transaction map was cast out of params[0] before the method was read. personal_sign and eth_signTypedData carry a String there, so the cast threw, the catch-all swallowed it, and the dApp got no response at all. The method is now read first and the cast lives inside the only branch whose first parameter is a transaction object. Rejections shipped Errors.USER_REJECTED.toInt(), which parses a lookup key as a number and yields null; the serializer then omits the field. It is now the SDK error, code 5000. The signing-FAILURE site sends a server error instead: a failure is not a user saying no, and a dApp that cannot tell them apart retries the wrong one. Two further silences answered: an approval with no network selected, and the catch-all itself. The three signing methods get a screen that says the wallet cannot display what would be signed, and are declined whichever button is pressed -- there is no typed-data renderer behind them yet.
The Hive transaction and the result drawer both took a hard-coded ETH, so an approved USDC transfer entered history as ETH and contradicted the drawer that authorised it. Both now read the symbol off the same summary the drawer was built from. A token the wallet cannot name is filed under its contract address rather than an invented ticker, and a plain send takes the unit of the selected network instead of assuming Ethereum.
fb94ccd to
cc6c83c
Compare
…three review rounds recorded
…s and the live walk
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcded22b0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review: Squid names native currency with the 0xEeee sentinel in word 0 and carries the same amount in value. The sentinel matched no coin, so the drawer showed an unverified token in wei AND 'also sending' the same ETH -- one spend presented as two assets. The decoder now takes the wallet's native symbol, names a sentinel input as that coin, hides the sentinel address, and drops the duplicate native figure when value agrees with the calldata (it stays when they disagree).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6703584da2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ount Codex review, two P2 findings: - A retry after a dropped relay answered with a generic server error even when the user had rejected, telling the dApp the wallet failed. The attempted response is kept and resent as is; the test now pins the rejection code on the retried answer (it read -32000 before). - The rows view had no From row, while the send body always did, so an approval or swap could not be checked against the selected account. dappCallRows takes the transaction's from and leads every kind with it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87258c9041
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ove does not move the token Codex review, two findings: - ERC-721 approve(address,uint256) shares the ERC-20 selector, and its second word is a token ID. An approve to a contract the wallet cannot name as a fungible token was read as an allowance in smallest units, with ERC-20 semantics in the copy. It is now the unknown-call presentation, which says the call could not be read and that approving may move funds. The unlimited-allowance flag on an unverified approve goes with it: that was a reading the decoder cannot make. Only a transfer stays on the unverified-token path, since its selector has no ERC-721 twin. - An approve carrying native value said the call moves native currency 'as well as a token'; an approve changes an allowance. Approval-specific copy.
9ab57d9 to
3fa0270
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fa0270da1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…essions cover every chain Codex review, three findings: - The request's chain was never read: the decoder used the selected network's router list and tokens, and the signer its RPC, so a mainnet request received while Base was selected would be described as Base and signed on Base. A request whose eip155 chain differs from the selected one is now declined before decoding, with a drawer saying which network to switch to, and answered UNSUPPORTED_CHAINS (5100). A request with no network selected keeps its old path. - A dropped hash answer closed the request instead of retrying it, now that the retry preserves semantics. It is tried once more; the hash stays in `attempted` so nothing later can send an error for a broadcast. - Sessions were approved for eip155:1 only, so a conforming dApp could never send the Base request the router allow-list exists for. The eip155 namespace now names every chain in networks.json with the same account on each, built by a pure, tested helper.
529510e to
c41eb4e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c41eb4e28c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ive swap's two figures are called native Codex review, two findings: - The session namespace named every catalogue entry, including two with no RPC URL; a request on either would have been approved and then failed at the RPC client. canSignOn (chain id and a non-empty RPC) filters the namespace, and the handler treats an empty RPC as absent. - A native-input swap whose calldata amount disagrees with tx.value kept both figures but warned that the call moves native currency "as well as a token", when both are native. Native-swap copy says the two figures disagree and to check them on the dApp.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9c57b078a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…h it is Codex review: the native-input swap branch hid the call's value when it was zero, so calldata declaring a spend with nothing attached read as a clean swap. Only equality suppresses the second figure now; the row is labelled 'Value attached' for a native swap, since it is not an extra send, and the disagreement caution fires.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2df6af458c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | Commitment | Status | Evidence | | ||
| |---|---|---| | ||
| | **D-02** — no path assumes 18 decimals | ✓ CONFIRMED | `grep -n "18" lib/reown/calldata_decoder.dart` matches only `kSquidSwapSelector = '0x58181a80'`. `_resolveToken` returns null on null / empty / unparseable / negative / >36 decimals and on a blank symbol; the group `UNRESOLVED decimals are never assumed to be eighteen` covers all six, each asserting `unverifiedToken`, `amount == '1500000'`, no `.`, `symbol == null`. The drawer then labels the row `Amount (smallest units)`. | | ||
| | **D-04** — unlimited warning at 2^255, reaching the screen | ✓ CONFIRMED | `kUnlimitedApprovalThreshold = BigInt.two.pow(255)`, comparison `>=`, pinned inclusive at exactly 2^255, negative one wei below, and the constant asserted to be 2^255 and not 2^256−1. Flag → words: `dappCallWarning` prepends the unlimited sentence, and the widget test drives real max-uint calldata through `summarizeTransaction` → `DappCallDetails` and asserts the word `unlimited` is on screen (and absent for a 1.5 USDC allowance). Also fires for *unverified* tokens — same drain vector. | |
There was a problem hiding this comment.
After the ambiguous-approve fix, this security claim is no longer true: summarizeTransaction returns unknownCall for every unresolved approve at calldata_decoder.dart:443-449, before isUnlimitedAllowance can be set, and the test at calldata_decoder_test.dart:468-475 now explicitly expects that behavior. Update this verification row and the matching claim in 30-02-SUMMARY.md:30-32; otherwise future work may incorrectly rely on unlimited approvals for unrecognized contracts receiving the explicit drain warning.
Useful? React with 👍 / 👎.
2 participants
When a dApp asks this wallet to sign, the drawer now describes the transaction instead of showing raw calldata or nothing at all.
What changed
Verification
flutter test1539 pass / 5 skip / 0 fail on the tip rebased ontodevelopafter #233 and #234 merged (the phase's own delta over the develop it was cut from is +126; the rest is phases 26 and 29 arriving underneath it, plus the three Codex rounds' tests) ·flutter analyzeno issues ·dart formatclean · brace, raw-colour, seed-safety, key-logging and agent-sync gates all pass. No package was added: the ABI decoding usesweb3dart, already a dependency.The byte-identity property — decoding never alters what gets signed — was checked by deliberately mutating the transaction map and confirming three separate tests fail.
Deliberately not here