Repository navigation
feat(wallet): discover invest assets via SEP-1 and SEP-6/24 (#V184) - #747
Conversation
|
@onajidavid87-web Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
@onajidavid87-web is attempting to deploy a commit to the miracle656's projects Team on Vercel. A member of the Team first needs to authorize it. |
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
Please don't merge this, and please don't run it against a funded account. There's a lot of real effort here, but as written the SEP-10 path will sign a transaction that moves a user's money.
Blocking — authenticateSep10 signs whatever the anchor sends back
frontend/wallet/lib/anchorDirectory.ts, lines 477-496. The validation is:
const manageDataOps = tx.operations.filter(
(op): op is Operation.ManageData => op.type === 'manageData',
)
if (manageDataOps.length === 0) {
throw new Error('SEP-10 challenge must contain at least one manage_data operation')
}
if (!tx.timeBounds) { … }
// …expiry check…
const rebuilt = TransactionBuilder.cloneFrom(tx).build()
rebuilt.sign(signerKeypair)It checks that at least one manageData op exists. It never checks that every op is manageData. So a challenge of [manageData("… auth"), payment(9999 XLM → attacker)] passes all three checks and gets signed in full. Nothing verifies the source account, the server's signature against the TOML SIGNING_KEY, that the sequence number is 0, or the home domain.
The entry point is a domain the user types into a search box, so any domain a user can be talked into entering is a drain vector.
Blocking — the anchor picks the network
Line 467:
const effectivePassphrase = network_passphrase || networkPassphraseThe remote anchor's response overrides the caller's network. A testnet call can therefore produce a mainnet-valid signature. These two defects compose: that is what turns "signs too much" into "signs a mainnet payment during a testnet flow".
Please use @stellar/stellar-sdk's WebAuth.readChallengeTx rather than hand-rolling this. It does the op, source, signature, sequence and home-domain checks for you, and it's the reason this class of bug shouldn't be written twice.
Blocking — it regresses passkey signing to raw-secret signing
app/invest/page.tsx (lines 48, 86, 101) reads veil_signer_secret out of storage and calls Keypair.fromSecret(...). Main's existing getSep10Jwt signs with the passkey. This also widens C3 ("localStorage S-key") from the August security audit rather than narrowing it.
Blocking — the issuer keys aren't valid Stellar accounts
anchorDirectory.ts adds a second VERIFIED_ASSET_REGISTRY, duplicating lib/assets.ts from the already-merged #745. Two of its three issuer keys fail StrKey.isValidEd25519PublicKey:
INVALID 747 USDC GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335WFGCCHVTLF2CCZAK27ZQQ625
INVALID 747 EURC GDHU6WR2KCEVDLWBVRWXZVH2AZ3ZX4BH4AXSSOQNTFQC2V3CQE37K3VC
VALID real USDC GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN
The USDC one shares a prefix with Circle's real issuer and then diverges. The practical result is backwards: the genuine Circle USDC would be flagged isImpersonating: true by this PR's own impersonation detector.
Also, matchesTomlAccounts = accounts.length === 0 || … means a TOML that declares no ACCOUNTS marks every currency in it as issuer-verified.
Blocking — iframe and postMessage
AnchorDirectoryModal.tsx:80—sandbox="… allow-top-navigation"lets a hostile anchor navigate the whole wallet away.AnchorDirectoryModal.tsx:25-38— themessagelistener has noevent.origincheck, so any frame can post{type:'sep24_complete'}.
Please drop the unrelated changes
sdk/src/core.ts—Horizon?.Server ?? Horizonassigns the whole namespace as the Server class when the optional chain misses. That's silent corruption inside security-critical SDK core to work around a test-mock problem; main fixed the actual cause in9d3ba04.frontend/wallet/next-env.d.tsis auto-generated and reverts a fix on main.frontend/wallet/public/sw.jsis regenerated from your local build, so the tracked precache manifest points at chunk hashes that won't exist in the real deploy.- The
TextEncoderpolyfill infeeBump.test.tsis no longer needed — main fixed that file in9d3ba04.
Direction
frontend/wallet/lib/sep24.ts already exists on main with signSep10Challenge, discoverAnchorInfo, getSep10Jwt, initiateDeposit/Withdraw — and it already enforces the home-domain check this one is missing. The PR imports from it and then reimplements it alongside, less safely. The right shape is to extend lib/sep24.ts, keep passkey signing, and delete the second registry in favour of lib/assets.ts.
The SEP-6/24 discovery UI is worth keeping and I'd like to see it come back on that base. Happy to talk through the SEP-10 validation if useful.
(Linkage note: the body says both "Closes #V184" — a wave ID, not an issue — and "Closes #737".)
|
Correction to my review above: I cited commit The substance is unchanged: Two other pre-existing main breakages were also fixed just now, in case they were showing red on your PR through no fault of yours: |
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
The new commit is Merge branch 'Miracle656:main' into main — the branch is current, which is welcome, but both blockers are untouched. I re-read the file at b58d9c8 rather than going from the diff:
every-op check : false
SIGNING_KEY verified : false
sequence-number check : false
anchor picks the network : still there
467| const effectivePassphrase = network_passphrase || networkPassphrase
472| tx = new Transaction(challengeXdr, effectivePassphrase)
478| const manageDataOps = tx.operations.filter(
479| (op): op is Operation.ManageData => op.type === 'manageData',
481| if (manageDataOps.length === 0) {
So a challenge of
[ manageData("example.com auth"), payment(9999 XLM → attacker) ]
still satisfies manageDataOps.length === 0 being false, still has timeBounds, still passes the expiry check, and is then cloned and signed in full. And line 467 still lets the remote response choose the network, so a testnet flow can produce a mainnet-valid signature.
The entry point is a domain typed into a search box. I am not going to approve a path where a domain someone can be talked into entering results in a signed payment from their wallet, and I would rather say that plainly than keep it in a list of review notes.
What closes it, concretely:
// every operation, not at least one
if (!tx.operations.every((op) => op.type === 'manageData')) throw …
// the caller's network, never the response's
const tx = new Transaction(challengeXdr, networkPassphrase)
// the anchor actually signed it, with the key its own TOML publishes
if (!Transaction.verifySignature(tx, tomlSigningKey)) throw …
// a challenge is not a real transaction
if (tx.sequence !== '0') throw …
// and the source is the account we asked aboutSEP-10 §"Verify the challenge transaction" lists these; the SDK also ships
WebAuth.readChallengeTx, which does the whole set and is what I would reach for
rather than hand-rolling the checks.
Everything else in the PR — the SEP-1 discovery, the TOML parsing, the asset listing — is genuinely useful work and I would like it to land. It is these two functions holding it.
|
Following up on my review with the why, because re-reading it I think I described the defect without explaining the mechanism, and that is not much help when you are the one who has to fix it. Why a SEP-10 challenge is normally harmlessThe anchor sends a transaction, you sign it, you send it back, you get a session token. That is safe because of one property: a challenge can never be executed. Its sequence number is Both halves of that matter. Lose either one and a "challenge" becomes a real transaction you just signed. What the current checks allow478| const manageDataOps = tx.operations.filter(op => op.type === 'manageData')
481| if (manageDataOps.length === 0) throw …
485| if (!tx.timeBounds) throw …
491| if (nowSec > maxTime) throw …
495| const rebuilt = TransactionBuilder.cloneFrom(tx).build()
496| rebuilt.sign(signerKeypair)The filter asks whether at least one operation is I built the transaction that exploits that and ran it through those exact lines: A hostile anchor returns The entry point is the domain in the asset-discovery search box, so it is reachable by anyone who can talk a user into typing a domain. The second one compounds it467| const effectivePassphrase = network_passphrase || networkPassphraseThe anchor's own response decides which network the signature is valid for. So the same exchange, run during a testnet flow, can hand back a mainnet-valid signature. The fix is smaller than the problemThe SDK already implements the whole of SEP-10's verification, and it is in the version this repo is on. Same hostile transaction, handed to it: So lines 467-496 can mostly become: import { WebAuth } from '@stellar/stellar-sdk'
// Our network, never the response's.
const { tx } = WebAuth.readChallengeTx(
challengeXdr,
serverAccountId, // SIGNING_KEY from the anchor's stellar.toml
networkPassphrase,
homeDomain,
webAuthDomain,
)
const rebuilt = TransactionBuilder.cloneFrom(tx).build()
rebuilt.sign(signerKeypair)That one call checks the sequence is If you would rather keep it hand-rolled, it is five checks:
One thing in your favour
To reproduce any of the above, this is the whole script: const { Account, Asset, Keypair, Networks, Operation, TransactionBuilder, BASE_FEE, WebAuth } =
require('@stellar/stellar-sdk')
const attacker = Keypair.random(), victim = Keypair.random()
const tx = new TransactionBuilder(
new Account(victim.publicKey(), '41237612384'),
{ fee: BASE_FEE, networkPassphrase: Networks.PUBLIC },
)
.addOperation(Operation.manageData({ name: 'evil.example auth', value: 'x' }))
.addOperation(Operation.payment({ destination: attacker.publicKey(), asset: Asset.native(), amount: '9999' }))
.setTimeout(300)
.build()
console.log(tx.operations.filter(o => o.type === 'manageData').length === 0) // false -> your check passes
WebAuth.readChallengeTx(tx.toXDR(), victim.publicKey(), Networks.PUBLIC, 'evil.example', 'evil.example')Everything else in this PR — the SEP-1 discovery, the TOML parsing, the asset listing — is good work and I want it to land. Happy to look again as soon as these two functions are updated, and happy to talk it through if any of the above is unclear. |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 37882553 | Triggered | Generic High Entropy Secret | 388052b | frontend/wallet/lib/tests/sep45.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
Head is now 388052b4, which is Merge branch 'Miracle656:main' into main — the second merge in two days. The branch is current and the code is unchanged; I read frontend/wallet/lib/anchorDirectory.ts at that SHA rather than the diff, and lines 467–481 are byte-identical to what I quoted this morning:
467| const effectivePassphrase = network_passphrase || networkPassphrase
472| tx = new Transaction(challengeXdr, effectivePassphrase)
478| const manageDataOps = tx.operations.filter(
479| (op): op is Operation.ManageData => op.type === 'manageData',
481| if (manageDataOps.length === 0) {
...
490| const rebuilt = TransactionBuilder.cloneFrom(tx).build()
491| rebuilt.sign(signerKeypair)I have said the substance twice, so here is the part I had not given you: you do not have to write any of these checks.
The SDK you already depend on ships the whole thing
frontend/wallet/package.json pins @stellar/stellar-sdk ^14.6.1, and I checked what that resolves to in this repo's node_modules:
WebAuth functions: InvalidChallengeError, gatherTxSigners, verifyTxSignedBy,
buildChallengeTx, readChallengeTx,
verifyChallengeTxSigners, verifyChallengeTxThreshold
readChallengeTx has this signature:
readChallengeTx(
challengeTx: string,
serverAccountID: string, // the anchor's SIGNING_KEY, from its own TOML
networkPassphrase: string, // yours — not the one in the response
homeDomains: string | string[],
webAuthDomain: string,
): { tx, clientAccountID, matchedHomeDomain, memo }Every argument is one of the checks that is missing, and it throws InvalidChallengeError rather than returning a transaction when any of them fails. Swapping new Transaction(...) plus the hand-rolled block for one call closes all four — every-op, signing key, network, and the home-domain/web_auth_domain binding — and deletes code rather than adding it.
One thing to add before you can call it
parseAnchorToml reads WEB_AUTH_ENDPOINT at line 244 but never reads SIGNING_KEY, so DiscoveredAnchorInfo has nowhere to carry it:
homeDomain, transferServerSep24, transferServerSep6,
webAuthEndpoint, kycServer, networkPassphrase, accounts, currenciesAdd it beside the WEB_AUTH_ENDPOINT line and put it on the interface. Without the anchor's own published key there is nothing to verify the signature against, which is why the current code cannot do this check even in principle.
While you are there: keep networkPassphrase from the TOML if you like, but use it as a cross-check against the caller's, never as the value passed to the parser. A response that picks the network is how a testnet flow produces a mainnet-valid signature.
If you would rather land the rest now
authenticateSep10 has exactly one call site. Everything else in this PR — parseTomlString, parseAnchorToml, fetchAnchorToml, registerDiscoveredAsset, isValidStellarPublicKey, isValidAssetCode, the registry and its tests — is discovery, needs no authentication, and is the bulk of the 1,405 lines. That half is good work and I would merge it.
So either path is fine by me:
- Finish it —
readChallengeTxplus theSIGNING_KEYparse, which is realistically an afternoon, and V184 is done. - Split it — drop
authenticateSep10and its call site, land discovery now, SEP-10 as a follow-up PR.
V184's criteria do name SEP-10 explicitly, so option 2 means the issue stays open behind the follow-up. That is a trade you should make deliberately rather than by accident, which is why I am asking rather than deciding it for you.
What I will not do is approve the current state. The entry point is a domain typed into a search box, and the end of it is rebuilt.sign(signerKeypair) over whatever operations that domain chose to send back.
The challenge was parsed and signed after three checks: that at least one
operation was a manage_data, that timeBounds existed, and that it had not
expired. A challenge of
[ manageData("example.com auth"), payment(9999 XLM -> attacker) ]
satisfied all three and was then cloned and signed in full, with the user's
real key. The entry point is a domain typed into a search box, so a domain
someone can be talked into entering was a drained wallet.
It now goes through WebAuth.readChallengeTx, which is the SDK's implementation
of SEP-10's own "Verify the challenge transaction" list: every operation is a
manage_data, the source is the server account, the sequence is 0, the time
bounds are current, the transaction is signed by the key the anchor publishes,
and the home domain and web auth domain are the ones we asked for. It throws
rather than returning a transaction. That is less code than the hand-rolled
version, not more.
Three things fall out of it:
- SIGNING_KEY is now parsed from the TOML, because without the anchor's own
published key there is nothing to check the signature against. An anchor that
omits it can be browsed but not authenticated with, which is stated rather
than silently skipped.
- The network comes from the caller and is never taken from the response.
`network_passphrase || networkPassphrase` let the anchor choose, which is how
a testnet flow produces a signature that is valid on mainnet. A response that
names a different network is now refused, in the client and on the page.
- The verified transaction is signed directly instead of
`cloneFrom(tx).build()`, which produced an unsigned copy and dropped the
anchor's own signature. SEP-10 requires the challenge back with both, so this
could not have worked against a real anchor.
Tests cover the attack itself (a spec-correct challenge plus one payment
operation, rejected before anything is posted), a challenge signed by the wrong
key, one issued for another account, a response naming another network, an
anchor with no SIGNING_KEY, and that the anchor's signature survives into what
is posted back.
Also reverted four files unrelated to the feature: a local Next artifact in
next-env.d.ts, minifier churn in public/sw.js, a TextEncoder polyfill in
feeBump.test.ts (the suite passes without it), and `Horizon?.Server ?? Horizon`
in sdk/src/core.ts, whose fallback would hand back the namespace where a
constructor is expected.
Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
The mobile half of this PR reached for WebAuth.readChallengeTx too, which is
the right call, but handed it values derived from the challenge being checked:
const expectedHomeDomain = homeDomain || firstKey.slice(0, -5)
const serverAccountID = anchorSigningKey || tx.source
Both arguments were optional, and the two live callers — buy.tsx:52 and
withdraw.tsx:151 — passed neither. So on the real paths the expected server
account was the transaction's own source and the expected home domain was read
out of the transaction's own manage_data name. An attacker's self-signed
challenge satisfies both, which means the check passed everything it was meant
to stop, while the tests were green because they passed the key explicitly.
The three arguments are now required and have no fallbacks: an anchor's
published SIGNING_KEY, the domain the TOML was fetched from, and the host of
the WEB_AUTH_ENDPOINT we called. `discoverAnchorInfo` parses SIGNING_KEY and
carries the home domain so callers have real values to pass, and both screens
refuse to authenticate with an anchor that publishes no SIGNING_KEY rather than
signing something they cannot verify. These screens sign with getSignerSecret(),
a real key, not a stub.
Also signs the verified transaction rather than `cloneFrom(tx).build()`, which
dropped the anchor's own signature — the same bug as the web client had.
Tests pin the two that matter: a forged challenge signed by a key of the
attacker's choosing is rejected even though it is internally consistent, and a
missing SIGNING_KEY refuses validation instead of falling back.
Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Same defect as the mobile half, on the path that deposit and withdraw actually
use. `lib/sep24.ts` called WebAuth.readChallengeTx with:
const expectedHomeDomain = homeDomain || firstKey.slice(0, -5)
const serverAccountID = anchorSigningKey || tx.source
and `getSep10Jwt` took both as optional. The three live callers —
app/withdraw/page.tsx:202 and components/DepositModal.tsx:110 — passed neither,
so the expected server account was the challenge's own source and the expected
home domain came out of its own manage_data name. A self-signed challenge
satisfies both.
What follows that check is a passkey assertion over the challenge's hash, so
the blind-signing exposure is the user's actual passkey, not just a keypair.
discoverAnchorInfo now parses SIGNING_KEY and carries the home domain;
homeDomain, anchorSigningKey and webAuthDomain are required with no fallbacks;
and getSep10Jwt refuses an anchor that publishes no SIGNING_KEY rather than
authenticating against an unverifiable challenge. Both call sites pass real
values.
Tests: omitting the signing key or the home domain now throws instead of
falling back. The existing wrong-SIGNING_KEY, wrong-home-domain, non-zero
sequence, expired and extra-payment cases still pass, and the legacy
tests/sep10.test.ts call sites were updated to the required signature.
Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
2 participants
Summary
Implements the Anchor Directory (V184) to dynamically discover invest assets via SEP-1, enforce strict issuer verification, authenticate via SEP-10 using the user's key, and open hosted SEP-24 deposit/withdraw flows with clean return to the wallet.
Key changes:
anchorDirectory.ts: SEP-1 TOML parsing, issuer verification, impersonation detection, and host defense against malicious TOML injections (registerDiscoveredAsset).authenticateSep10: SEP-10 challenge signing with zero user data leakage beyond the challenge signature.AnchorDirectoryModal.tsx: Popup/iframe modal for SEP-24 interactive deposit and withdrawal flows./invest: Anchor Directory UI page for domain discovery, issuer verification badges, and flow initiation.anchorDirectory.test.ts): 12 new unit tests covering TOML parsing, issuer verification, hostile TOML blocking, and SEP-10 authentication.Related issue
Closes #V184
Type of change
Component
Checklist
cargo testpasses (contracts)npm run typecheckpasses (wallet / sdk / agent)npm run buildpasses (wallet / agent)Screenshots / test output