fix: fix Bitcoin incorrect 'origin' in zContext caused by revertAddress option #3192
Conversation
…ress on BTC inbound revert
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several significant updates to the ZetaChain node, including new features such as the ability to whitelist SPL tokens on Solana, enhancements to build reproducibility, and the integration of SPL deposits and withdrawals. It also refactors existing components, removes the HSM signer from the zetaclient, and improves the zetaclientd CLI. Various fixes address issues related to messaging, peer discovery, and transaction handling. Additionally, the changelog has been updated to reflect these changes and provide a comprehensive overview of the improvements made across multiple versions. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
ws4charlie
added
audit
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
gartnera
added
CONSENSUS_BREAKING_ACK
|
Removing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
x/crosschain/types/revert_options_test.go (1)
50-50: Consider adding more test cases for broader coverage.Consider adding these test cases to strengthen the test suite:
- Mainnet Bitcoin addresses to ensure proper network validation
- Specific error message validation for each failure case
Example addition:
t.Run("mainnet address on testnet chain", func(t *testing.T) { addr := sample.BtcAddressP2WPKH(t, &chaincfg.MainNetParams) actualAddr, valid := types.RevertOptions{ RevertAddress: addr, }.GetBTCRevertAddress(chains.BitcoinTestnet.ChainId) require.False(t, valid) require.Empty(t, actualAddr) })x/crosschain/types/revert_options.go (1)
79-90: Enhance documentation while maintaining solid implementationThe implementation is robust and aligns well with the PR objectives. However, the documentation could be more detailed.
Consider enhancing the documentation to be more explicit about the return values and chainID parameter:
-// GetBTCRevertAddress validates and returns the BTC revert address +// GetBTCRevertAddress validates and returns the BTC revert address. +// Parameters: +// - chainID: The Bitcoin network chain ID (e.g., mainnet, testnet) +// Returns: +// - string: The encoded Bitcoin address if valid, empty string otherwise +// - bool: true if the address is valid and supported, false otherwisezetaclient/chains/bitcoin/observer/event.go (1)
224-225: Enhance comment clarity regarding security implicationsThe current comment could be more explicit about the security implications and the relationship to the sender impersonation fix.
Consider updating the comment to:
- // inject the 'revertAddress' specified in the memo, so that - // zetacore will create a revert outbound that points to the custom revert address. + // Use RevertOptions to specify the custom revert address from memo while preserving + // the original sender address, preventing potential sender impersonation issues.x/crosschain/types/cctx_test.go (2)
139-154: Consider enhancing Bitcoin-specific test assertionsWhile the test comprehensively verifies the revert outbound parameters, consider adding assertions specific to Bitcoin:
- Verify the Bitcoin address format validity
- Assert that the address belongs to the correct Bitcoin network (testnet)
t.Run("successfully set BTC revert address V1", func(t *testing.T) { cctx := sample.CrossChainTx(t, "test") cctx.InboundParams.SenderChainId = chains.BitcoinTestnet.ChainId cctx.OutboundParams = cctx.OutboundParams[:1] btcAddress := sample.BtcAddressP2WPKH(t, &chaincfg.TestNet3Params) cctx.RevertOptions.RevertAddress = btcAddress + + // Verify Bitcoin address format and network + _, err := btcutil.DecodeAddress(btcAddress, &chaincfg.TestNet3Params) + require.NoError(t, err, "should be a valid Bitcoin testnet address") err := cctx.AddRevertOutbound(100) require.NoError(t, err)
156-170: Consider adding Ethereum address format validationThe test should verify that the revert address follows the Ethereum address format:
t.Run("successfully set EVM revert address V2", func(t *testing.T) { cctx := sample.CrossChainTxV2(t, "test") cctx.OutboundParams = cctx.OutboundParams[:1] - cctx.RevertOptions.RevertAddress = sample.EthAddress().Hex() + ethAddress := sample.EthAddress().Hex() + cctx.RevertOptions.RevertAddress = ethAddress + + // Verify Ethereum address format + require.True(t, common.IsHexAddress(ethAddress), "should be a valid Ethereum address") err := cctx.AddRevertOutbound(100) require.NoError(t, err)testutil/sample/crosschain.go (1)
220-234: Consider adding Bitcoin-specific test data generation.Given that this PR addresses Bitcoin's incorrect 'origin' in zContext, it would be beneficial to add specific test data generation for Bitcoin transactions with the new protocol version.
Consider adding a helper function like:
func BitcoinCrossChainTxV2(t *testing.T, index string, revertAddress string) *types.CrossChainTx { cctx := CrossChainTxV2(t, index) cctx.InboundParams.CoinType = coin.CoinType_Bitcoin if revertAddress != "" { cctx.RevertOptions = types.NewRevertOptions(revertAddress) } return cctx }changelog.md (1)
37-37: Consider enhancing the changelog entry to better reflect security implications.The current entry could be more descriptive about the security implications of the fix. Based on the PR objectives, consider expanding it to:
-* [3192](https://github.com/zeta-chain/node/pull/3192) - fix incorrect zContext origin caused by the replacement of 'sender' with 'revertAddress' +* [3192](https://github.com/zeta-chain/node/pull/3192) - fix Bitcoin incorrect zContext origin to prevent sender impersonation when revertAddress is specified in memo
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
changelog.md(1 hunks)testutil/sample/crosschain.go(1 hunks)x/crosschain/types/cctx.go(2 hunks)x/crosschain/types/cctx_test.go(2 hunks)x/crosschain/types/revert_options.go(2 hunks)x/crosschain/types/revert_options_test.go(2 hunks)zetaclient/chains/bitcoin/observer/event.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
testutil/sample/crosschain.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/cctx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/cctx_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/revert_options.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/revert_options_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/event.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (7)
x/crosschain/types/revert_options_test.go (2)
4-11: LGTM: Import statements are well-organized and necessary.
The new imports for Bitcoin address handling (btcd/chaincfg and chains) are appropriately added and organized according to Go conventions.
50-89: LGTM: Well-structured and comprehensive test cases.
The test function follows good testing practices with:
- Clear test case descriptions
- Comprehensive coverage of edge cases
- Consistent assertion patterns
- Proper use of test helpers
x/crosschain/types/revert_options.go (2)
9-9: LGTM: Import addition is appropriate
The addition of the chains package import is necessary and well-placed for the new Bitcoin address validation functionality.
79-90: Verify test coverage for Bitcoin address validation
The implementation looks solid, but let's ensure comprehensive test coverage exists.
✅ Verification successful
Test coverage for Bitcoin address validation is comprehensive
The implementation is thoroughly tested with:
- Valid Bitcoin testnet P2WPKH address validation
- Invalid network address detection (regtest vs testnet)
- Empty address handling
- Unsupported address format rejection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of the new GetBTCRevertAddress method
# Look for test files and relevant test cases
rg -l "TestRevertOptions_GetBTCRevertAddress" --type go
rg "func.*Test.*BTCRevertAddress" --type go
# Check for test cases with different Bitcoin address types
rg "TestCase.*btc.*address" --type go -A 5
Length of output: 289
Script:
#!/bin/bash
# Let's examine the actual test implementation
cat x/crosschain/types/revert_options_test.go
Length of output: 3324
zetaclient/chains/bitcoin/observer/event.go (1)
Line range hint 226-251: Implementation looks good with minor suggestions
The implementation correctly addresses the sender impersonation issue by:
- Preserving the original sender address
- Using RevertOptions for custom revert addresses
- Maintaining V1 protocol compatibility
Consider adding validation for RevertOptions before message creation, although the upstream ValidateStandardMemo function already handles this.
Let's verify the validation of revert addresses:
✅ Verification successful
Revert address validation is properly implemented
The implementation correctly validates revert addresses through multiple layers:
-
The Bitcoin observer's
ValidateStandardMemofunction (lines 145-165) performs thorough validation:- Checks if the revert address is a valid BTC address for the given chain
- Verifies the address format is supported
- Returns descriptive errors for invalid addresses
-
The core types provide chain-specific validation through:
GetBTCRevertAddressfor Bitcoin addressesGetEVMRevertAddressfor Ethereum addresses
-
The validation is consistently used across the codebase, with proper error handling and chain-specific address format verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if revert address validation is consistent across the codebase
# Search for revert address validation patterns
ast-grep --pattern 'func $_ ($_ *memo.InboundMemo, $_ int64) error {
$$$
revertAddress := $_.RevertOptions.RevertAddress
if revertAddress != "" {
$$$
}
$$$
}'
Length of output: 174
Script:
#!/bin/bash
# Let's search for revert address validation patterns with a broader approach
# Search for RevertOptions validation
rg -A 5 "RevertOptions.*RevertAddress"
# Search for ValidateStandardMemo implementations
ast-grep --pattern 'func ValidateStandardMemo($$$) error {
$$$
}'
# Search for any validation related to revert addresses
rg -A 5 "validate.*[Rr]evert"
Length of output: 17700
x/crosschain/types/cctx_test.go (2)
7-7: LGTM: Required imports added correctly
The new imports are necessary for Bitcoin network parameters and chain identifiers, properly organized in alphabetical order.
Also applies to: 10-10
139-170: LGTM: Comprehensive test coverage for revert address handling
The test cases effectively cover both Bitcoin V1 and Ethereum V2 revert address scenarios, aligning well with the PR objectives to fix Bitcoin's incorrect 'origin' in zContext. While some enhancements for address validation have been suggested, the current implementation provides solid test coverage.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3192 +/- ##
===========================================
+ Coverage 62.70% 62.74% +0.03%
===========================================
Files 425 425
Lines 30209 30226 +17
===========================================
+ Hits 18942 18964 +22
+ Misses 10429 10425 -4
+ Partials 838 837 -1
|
check added: d0cc073 |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
| @@ -249,5 +248,6 @@ func (ob *Observer) NewInboundVoteFromStdMemo( | |||
| 0, | |||
| crosschaintypes.ProtocolContractVersion_V1, | |||
| false, // not relevant for v1 | |||
| crosschaintypes.WithRevertOptions(revertOptions), | |||
There was a problem hiding this comment.
Why not use event.MemoStd.RevertOptions here instead of revertOptions which only contains RevertAddress and omitting CallOnRevert, AbortAddress, etc.?
There was a problem hiding this comment.
The zetacored currently only takes the field of RevertAddress from RevertOptions struct in the V1 architecture. So zetaclientd screens out all noises except the RevertAddress before migration to V2 architecture. In V2, we'll open them up in both zetacored and zetaclientd.
Description
The current implementation of Bitcoin revert address simply replaces
senderaddress withrevertAddressin zetaclient, so that the refund should go to the customrevertAddress.However, the replacement of
senderaddress will allow impersonating the sender. For example, the sender (it isrevertAddresson revert) will be set as thezContext.Originin zetacore, which is incorrect address.The solution:
senderaddress untouched inzetaclient.crosschaintypes.RevertOptionswith 'revertAddress' in theMsgVoteInboundstruct to vote (protocol V1).zetacorecode to take the customrevertAddressfor V1 AND Bitcoin chain only.How Has This Been Tested?