-
Notifications
You must be signed in to change notification settings - Fork 300
feat: add wallet address verification for eth and eth like #7563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
zahin-mohammad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, lets also get an ETH-ALT review for the same
857a725 to
3292e9d
Compare
TICKET: WP-6461
|
@claude review this pr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes critical security vulnerabilities in Ethereum wallet address verification by removing unconditional bypass logic for forwarder versions 0, 3, and 5, and implementing proper validation for MPC (TSS) wallets.
Key Changes:
- Implements MPC-based address verification for wallet versions 3, 5 (base addresses), and 6 using secp256k1 curve derivation
- Adds factory and implementation addresses for wallet versions 1, 2, and 4 across Ethereum networks
- Refactors
isWalletAddressto properly route between BIP32 and TSS verification based on wallet version and address type
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/statics/src/networks.ts | Adds wallet and forwarder factory/implementation addresses for V1, V2, and V4 wallets across Ethereum, Hoodi, Polygon, Optimism, and ZkSync networks |
| modules/statics/test/unit/resources/amsTokenConfig.ts | Updates test configuration with new wallet factory addresses for consistency with network definitions |
| modules/sdk-core/src/index.ts | Exports new address verification functions for MPC wallets |
| modules/sdk-core/src/bitgo/utils/tss/addressVerification.ts | Implements unified MPC address verification supporting both secp256k1 and ed25519 curves with correct public key size handling |
| modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts | Updates TSS verification types to accept flexible index types and adds type guard for identifying TSS verification options |
| modules/abstract-eth/src/abstractEthLikeNewCoins.ts | Major refactoring of isWalletAddress to properly handle TSS wallets (V3, V5, V6), BIP32 wallets (V1, V2, V4), and forwarder address verification with version-specific factory addresses |
| modules/sdk-coin-eth/test/unit/eth.ts | Comprehensive test coverage for all wallet versions and forwarder types including TSS, BIP32, base addresses, and deposit addresses |
| modules/sdk-coin-polygon/test/unit/polygon.ts | Adds test for Polygon wallet V5 with forwarder V4 address verification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
abhishekagrawal080
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
zahin-mohammad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general this lgtm, we can address some of the comments as a follow up.
Fix
AbstractEthLikeNewCoins.isWalletAddressforwarder version bypassFix
forwarder versionbug in address verificationwalletV4ForwarderFactoryAddressandwalletV4ForwarderImplementationAddressTICKET: WP-6461