fix: pin what signLegacyUpgradeTx signs; 0.19.1 - #22
Merged
Merged
Conversation
0.19.0 pinned the legacy upgrade auth entry to the transaction's own top-level call on the named wallet with no sub-invocations, but not to the function name or the target hash. A hostile transaction rooted at another wallet-admin call (update_signer, add_signer, update_contract_code to an arbitrary hash) could therefore be signed through this path, which matters for patched legacy wallets that have no other remote takeover route. - signLegacyUpgradeTx now accepts only update_contract_code or migrate_signers, and an update_contract_code may only carry the canonical legacy-line target (expectedTarget overrides). Two new tests. - buildLegacyUpgradeTx refuses a wallet already on the target. - inspectLegacyWallet counts an entry as live through liveUntil inclusive. - LegacyWalletError points at the kit's own helpers. - Docs: README capability sentence and method rows; guide application section, protocol-23 auto-restore, policy-signer boundary, migrate timing, and the live testnet verification of both cohorts through the SDK path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The upgrade guide still recommends vulnerable version 0.19.0, and the corrected TTL boundary lacks a regression test.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Pins legacy-upgrade signing to approved functions and WASM targets while improving legacy-wallet guidance and versioning.
Changes:
- Restricts legacy signing and fixes TTL boundary handling.
- Rejects redundant upgrades and adds regression tests.
- Updates documentation and releases version 0.19.1.
File summaries
| File | Description |
|---|---|
src/version.ts |
Bumps generated version. |
src/kit/legacy-ops.ts |
Pins signed functions and upgrade targets. |
src/kit/legacy-ops.test.ts |
Tests signing restrictions. |
src/kit.ts |
Rejects redundant upgrades and exposes target override. |
src/kit.test.ts |
Tests redundant-upgrade rejection. |
src/errors.ts |
Improves legacy-wallet recovery guidance. |
README.md |
Documents legacy helper APIs. |
package.json |
Bumps package version. |
docs/releasing.md |
Updates release version reference. |
docs/legacy-wallet-upgrade.md |
Expands upgrade and verification guidance. |
contracts-legacy/smart-wallet/src/tests/mod.rs |
Corrects legacy test documentation. |
CHANGELOG.md |
Records 0.19.1 changes. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| with an Ed25519 signer key the wallet holds), and submit it through your | ||
| relayer or a funded source. The current SDK (`0.17.0` and later) does not | ||
| connect to these wallets and cannot be used for this step. | ||
| relayer or a funded source. Or use the current SDK (`0.19.0` and later): it |
| const liveUntil = entry.liveUntilLedgerSeq; | ||
| liveByKey.set(entry.key.toXDR("base64"), liveUntil !== undefined && liveUntil > latest); | ||
| // An entry is live through its liveUntil ledger inclusive. | ||
| liveByKey.set(entry.key.toXDR("base64"), liveUntil !== undefined && liveUntil >= latest); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Round-3 independent review of
0.19.0found thatsignLegacyUpgradeTxpinned the auth entry to the transaction's own top-level call on the named wallet with no sub-invocations, but not to the function name or the target hash. A hostile transaction rooted at another wallet-admin call (update_signer,add_signer, orupdate_contract_codeto an arbitrary hash) could be signed through this path. That matters most for patched legacy wallets, which have no other remote takeover route.What
signLegacyUpgradeTxaccepts onlyupdate_contract_codeormigrate_signers, and anupdate_contract_codemay only carry the canonical legacy-line target (expectedTargetoverrides). Two new tests.buildLegacyUpgradeTxrefuses a wallet already on the target.inspectLegacyWalletcounts an entry as live throughliveUntilinclusive.LegacyWalletErrorpoints at the kit's own helpers, not only the 0.10.20–0.12.x line.Live verification
The full SDK path was run on testnet against wallets deployed from the real mainnet bytecode of both cohorts (
inspectLegacyWallet→buildLegacyUpgradeTx→signLegacyUpgradeTx→ relayer-style{func, auth}submit →buildLegacyMigrateTx→get_signer). Both wallets moved toc079d3a4…; the bare cohort'smigrate_signersrewrote one entry then zero; an unauthenticatedupdate_signerfails on-chain afterwards. Wallets and tx ids are indocs/legacy-wallet-upgrade.md.Suite: 363 passing; legacy contract tests 8/8.
🤖 Generated with Claude Code