Skip to content
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

fix(crosschain): add support to perform withdraws in ZetaChain onRevert call #3348

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

lumtis
Copy link
Member

@lumtis lumtis commented Jan 13, 2025

Description

  • Add the processLogs call after performing revert call to support new CCTX creating during onRevert call

  • Add same logic as in ef4c1d4 by using a temporary context to not commit EVM changes if logic in log parsing fails

  • Add a test that experiments the workflow of creating new withdraw during a revert call on ZetaChain

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for performing withdrawals during the onRevert call in ZetaChain
    • Enhanced cross-chain transaction handling for withdrawal scenarios
  • Testing

    • Introduced new end-to-end test cases for ETH withdrawal and contract call revert scenarios
    • Expanded test coverage for cross-chain transaction processing
  • Improvements

    • Updated contract interfaces and metadata to support more robust withdrawal mechanisms
    • Improved error handling and transaction response tracking in cross-chain operations
  • Documentation

    • Updated changelog to document new withdrawal support functionality

@lumtis lumtis requested a review from a team as a code owner January 13, 2025 15:36
@lumtis lumtis marked this pull request as draft January 13, 2025 15:36
Copy link
Contributor

coderabbitai bot commented Jan 13, 2025

📝 Walkthrough

Walkthrough

This pull request introduces enhancements to ZetaChain's cross-chain transaction handling, specifically focusing on withdrawal capabilities during the onRevert call. The changes span multiple components, including test infrastructure, contract interfaces, and keeper implementations. The primary objective is to improve the system's ability to manage complex cross-chain scenarios, particularly when transactions need to be reverted with associated withdrawals.

Changes

File Change Summary
changelog.md Added "Unreleased" section with reference to PR #3348 for withdrawal support
cmd/zetae2e/local/evm.go Added new test case for ETH withdrawal and revert workflow
e2e/e2etests/e2etests.go Reintroduced test name constants for withdrawal scenarios
pkg/contracts/testdappv2/TestDAppV2.sol Added new interfaces and enhanced onRevert logic for withdrawals
x/crosschain/keeper/* Updated method signatures and error handling for cross-chain transaction processing

Sequence Diagram

sequenceDiagram
    participant Sender
    participant ZetaChain
    participant Gateway
    participant ZRC20Token
    
    Sender->>ZetaChain: Initiate Cross-Chain Transaction
    ZetaChain->>Gateway: Process Transaction
    alt Transaction Requires Revert
        Gateway->>ZRC20Token: Check Withdrawal Conditions
        ZRC20Token-->>Gateway: Withdrawal Approval
        Gateway->>Sender: Execute Withdrawal
    end
Loading

Possibly related PRs

Suggested Labels

E2E, UPGRADE_LIGHT_TESTS

Suggested Reviewers

  • fbac
  • kingpinXD
  • skosito
  • brewmaster012
  • swift1337

The modifications represent a methodical approach to expanding ZetaChain's cross-chain transaction resilience, with particular emphasis on robust withdrawal mechanisms during transaction reversals.

Finishing Touches

  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

!!!WARNING!!!
nosec detected in the following files: e2e/e2etests/e2etests.go

Be very careful about using #nosec in code. It can be a quick way to suppress security warnings and move forward with development, it should be employed with caution. Suppressing warnings with #nosec can hide potentially serious vulnerabilities. Only use #nosec when you're absolutely certain that the security issue is either a false positive or has been mitigated in another way.

Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203
Broad #nosec annotations should be avoided, as they can hide other vulnerabilities. The CI will block you from merging this PR until you remove #nosec annotations that do not target specific rules.

Pay extra attention to the way #nosec is being used in the files listed above.

@github-actions github-actions bot added the nosec label Jan 13, 2025
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 46.66667% with 24 lines in your changes missing coverage. Please review.

Project coverage is 62.58%. Comparing base (8ce300b) to head (d9ab6f4).
Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
...hain/keeper/cctx_orchestrator_validate_outbound.go 61.76% 12 Missing and 1 partial ⚠️
x/fungible/keeper/v2_deposits.go 0.00% 11 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3348      +/-   ##
===========================================
+ Coverage    62.43%   62.58%   +0.14%     
===========================================
  Files          449      449              
  Lines        31710    31736      +26     
===========================================
+ Hits         19799    19861      +62     
+ Misses       11024    10987      -37     
- Partials       887      888       +1     
Files with missing lines Coverage Δ
pkg/contracts/testdappv2/TestDAppV2.go 0.00% <ø> (ø)
x/fungible/keeper/v2_deposits.go 21.31% <0.00%> (ø)
...hain/keeper/cctx_orchestrator_validate_outbound.go 87.40% <61.76%> (+16.57%) ⬆️

... and 1 file with indirect coverage changes

@lumtis lumtis linked an issue Jan 14, 2025 that may be closed by this pull request
@lumtis lumtis changed the title test(e2e): add a revert and withdraw test fix(crosschain): add support to perform withdraws in ZetaChain onRevert call Jan 14, 2025
@lumtis lumtis marked this pull request as ready for review January 14, 2025 17:54
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (14)
x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (2)

357-359: Maintain consistent context management

The temporary context tmpCtx and the corresponding commit function should be declared immediately before their usage to enhance readability and maintain code consistency.

Apply this diff to adjust the declaration:

-		// use a temporary context to not commit any state change in case of error
-		tmpCtx, commit := ctx.CacheContext()
-
		// process the revert on ZEVM
+		// use a temporary context to avoid committing state changes in case of error
+		tmpCtx, commit := ctx.CacheContext()

421-422: Provide detailed error messages for unexpected statuses

The default case in the switch statement returns a generic error message. Enhancing this message with more context can aid in debugging.

Apply this diff to improve the error message:

-		return fmt.Errorf("unexpected cctx status %s", cctx.CctxStatus.Status)
+		return fmt.Errorf("processFailedOutboundV2: unexpected CCTX status %s", cctx.CctxStatus.Status)
x/fungible/keeper/v2_deposits.go (1)

87-87: Clarify error messages for unsupported coin types

The error message for unsupported coinType could be more informative. Including the actual coinType value helps in quickly identifying the issue.

Apply this diff to enhance the error message:

-		return nil, errors.New("ZETA asset is currently unsupported for revert with V2 protocol contracts")
+		return nil, fmt.Errorf("ProcessV2RevertDeposit: coinType %s (ZETA asset) is unsupported for revert with V2 protocol contracts", coinType)
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 87-87: x/fungible/keeper/v2_deposits.go#L87
Added line #L87 was not covered by tests

x/crosschain/types/expected_keepers.go (1)

164-164: Update function comments to match signature changes

The comment for the ProcessV2RevertDeposit method should be updated to reflect the new return type. This maintains accurate documentation and aids in code comprehension.

Update the function comment to include the return of (*evmtypes.MsgEthereumTxResponse, error).

    	// ProcessV2RevertDeposit handles a revert deposit from an inbound tx with protocol version 2
+   	// Returns (*evmtypes.MsgEthereumTxResponse, error)
    	ProcessV2RevertDeposit(
testutil/keeper/crosschain.go (2)

311-319: Remove unused parameter documentation.

The commented parameter documentation is not being used and should be removed to avoid confusion.

-  // ctx types.Context
-  // inboundSender string
-  // amount *big.Int
-  // chainID int64
-  // coinType coin.CoinType
-  // asset string
-  // revertAddress common.Address
-  // callOnRevert bool
-  // revertMessage []byte

320-332: Consider using specific matchers instead of mock.Anything.

Using mock.Anything for all parameters makes the mock too permissive. Consider using specific matchers for type safety and better test coverage:

 m.On(
   "ProcessV2RevertDeposit",
-  mock.Anything,
-  mock.Anything,
-  mock.Anything,
-  mock.Anything,
-  mock.Anything,
-  mock.Anything,
-  mock.Anything,
-  mock.Anything,
-  mock.Anything,
+  mock.AnythingOfType("types.Context"),
+  mock.AnythingOfType("string"),
+  mock.AnythingOfType("*big.Int"),
+  mock.AnythingOfType("int64"),
+  mock.AnythingOfType("coin.CoinType"),
+  mock.AnythingOfType("string"),
+  mock.AnythingOfType("common.Address"),
+  mock.AnythingOfType("bool"),
+  mock.AnythingOfType("[]uint8"),
 ).Return(retEVMTxResponse, retErr).Once()
x/crosschain/keeper/cctx_orchestrator_validate_outbound_test.go (5)

5-11: Consider grouping imports by standard library, external, and internal packages.

The imports could be better organized for readability.

 import (
+  // Standard library
   "encoding/base64"
   "errors"
+  "math/big"
+  "testing"
+
+  // External packages
   "github.com/ethereum/go-ethereum/core/vm"
   evmtypes "github.com/zeta-chain/ethermint/x/evm/types"
-  "math/big"
-  "testing"
-
   cosmoserror "cosmossdk.io/errors"

63-84: Enhance test case description and error handling.

The test case could be more descriptive and avoid magic strings in error assertions.

 func TestKeeper_ValidateFailedOutboundV2(t *testing.T) {
-  t.Run("fail if can't fetch ZetaChain chain ID", func(t *testing.T) {
+  t.Run("should fail when ZetaChain chain ID cannot be parsed from context", func(t *testing.T) {
     // arrange
     k, ctx, _, _ := keepertest.CrosschainKeeper(t)
     cctx := sample.CrossChainTx(t, "test")
     cctx.ProtocolContractVersion = types.ProtocolContractVersion_V2
     cctx.CctxStatus.Status = types.CctxStatus_PendingOutbound

     ctx = ctx.WithChainID("invalid")

     // act
     err := k.ValidateOutboundObservers(
       ctx,
       cctx,
       observertypes.BallotStatus_BallotFinalized_FailureObservation,
       sample.String(),
     )

     // assert
     require.Error(t, err)
-    require.Contains(t, err.Error(), "failed to get ZetaChain chainID")
+    const expectedError = "failed to get ZetaChain chainID"
+    require.Contains(t, err.Error(), expectedError)
   })

146-147: Consider extracting error variable.

The error variable could be extracted for better reusability and consistency.

-  revertErr := vm.ErrExecutionReverted
+  const revertErr = vm.ErrExecutionReverted
   keepertest.MockProcessV2RevertDeposit(fungibleMock, nil, revertErr)

378-378: Consider using error constants.

Define error constants for better maintainability and reusability.

-  errorFailedZETARevertAndCallContract := cosmoserror.New("test", 999, "failed ZETARevertAndCallContract")
+  const (
+    errCodeZETARevertAndCallContract = 999
+    errMsgZETARevertAndCallContract  = "failed ZETARevertAndCallContract"
+  )
+  errorFailedZETARevertAndCallContract := cosmoserror.New("test", errCodeZETARevertAndCallContract, errMsgZETARevertAndCallContract)

63-63: Add test function documentation.

Consider adding documentation to describe the test function's purpose and the scenarios it covers.

+// TestKeeper_ValidateFailedOutboundV2 tests the validation of failed outbound transactions
+// for protocol contract version 2. It covers various failure scenarios including:
+// - Chain ID validation
+// - Source chain validation
+// - Revert outbound handling
+// - Transaction processing errors
 func TestKeeper_ValidateFailedOutboundV2(t *testing.T) {
pkg/contracts/testdappv2/TestDAppV2.sol (2)

179-179: Clarify error message for mismatched fee tokens

The error message in the require statement may confuse developers by suggesting that the ZRC20 token is not a gas token. Since the check is verifying that the fee token matches the asset being withdrawn, the error message should reflect the actual issue.

Apply this diff to improve the error message:

-    require(feeToken == revertContext.asset, "zrc20 is not gas token");
+    require(feeToken == revertContext.asset, "Fee token does not match the asset being withdrawn");

245-247: Optimize string comparison in isWithdrawMessage function

Using keccak256 with abi.encodePacked is unnecessary when comparing static strings. Comparing the hashes of the messages directly simplifies the function and reduces gas usage.

Simplify the isWithdrawMessage function:

-function isWithdrawMessage(bytes memory message) internal pure returns (bool) {
-    return keccak256(message) == keccak256(abi.encodePacked("withdraw"));
+function isWithdrawMessage(bytes memory message) internal pure returns (bool) {
+    bytes32 withdrawHash = keccak256("withdraw");
+    return keccak256(message) == withdrawHash;
}

This change caches the hash of the static string "withdraw", reducing repetitive computations.

e2e/e2etests/test_eth_withdraw_and_call_revert_with_withdraw.go (1)

14-49: Ensure error handling for critical operations

The test function lacks explicit error handling for critical operations, such as transactions and context retrieval. Adding checks for possible errors enhances test reliability and debuggability.

Consider adding error checks after critical operations:

// perform the withdraw
tx, err := r.ETHWithdrawAndArbitraryCall(
    r.TestDAppV2EVMAddr,
    amount,
    r.EncodeGasCall("revert"),
    gatewayzevm.RevertOptions{
        RevertAddress:    r.TestDAppV2ZEVMAddr,
        CallOnRevert:     true,
        RevertMessage:    []byte("withdraw"), // call withdraw in the onRevert hook
        OnRevertGasLimit: big.NewInt(0),
    },
)
require.NoError(r, err, "Failed to perform ETH withdrawal and arbitrary call")

// wait for the cctx to be mined
cctxRevert, err := utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout)
require.NoError(r, err, "Failed to wait for cctx to be mined")

This ensures that any failures are caught and reported appropriately during testing.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8ce300b and d9ab6f4.

⛔ Files ignored due to path filters (1)
  • pkg/contracts/testdappv2/TestDAppV2.bin is excluded by !**/*.bin
📒 Files selected for processing (13)
  • changelog.md (1 hunks)
  • cmd/zetae2e/local/evm.go (1 hunks)
  • e2e/e2etests/e2etests.go (2 hunks)
  • e2e/e2etests/test_eth_withdraw_and_call_revert_with_withdraw.go (1 hunks)
  • pkg/contracts/testdappv2/TestDAppV2.go (1 hunks)
  • pkg/contracts/testdappv2/TestDAppV2.json (1 hunks)
  • pkg/contracts/testdappv2/TestDAppV2.sol (4 hunks)
  • testutil/keeper/crosschain.go (1 hunks)
  • testutil/keeper/mocks/crosschain/fungible.go (1 hunks)
  • x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (3 hunks)
  • x/crosschain/keeper/cctx_orchestrator_validate_outbound_test.go (3 hunks)
  • x/crosschain/types/expected_keepers.go (1 hunks)
  • x/fungible/keeper/v2_deposits.go (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • pkg/contracts/testdappv2/TestDAppV2.json
👮 Files not reviewed due to content moderation or server errors (3)
  • e2e/e2etests/e2etests.go
  • pkg/contracts/testdappv2/TestDAppV2.go
  • changelog.md
🧰 Additional context used
📓 Path-based instructions (10)
cmd/zetae2e/local/evm.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

testutil/keeper/crosschain.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/fungible/keeper/v2_deposits.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

e2e/e2etests/test_eth_withdraw_and_call_revert_with_withdraw.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

e2e/e2etests/e2etests.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

testutil/keeper/mocks/crosschain/fungible.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/keeper/cctx_orchestrator_validate_outbound_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/types/expected_keepers.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

pkg/contracts/testdappv2/TestDAppV2.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

🪛 GitHub Check: codecov/patch
x/fungible/keeper/v2_deposits.go

[warning] 62-62: x/fungible/keeper/v2_deposits.go#L62
Added line #L62 was not covered by tests


[warning] 72-72: x/fungible/keeper/v2_deposits.go#L72
Added line #L72 was not covered by tests


[warning] 80-81: x/fungible/keeper/v2_deposits.go#L80-L81
Added lines #L80 - L81 were not covered by tests


[warning] 84-84: x/fungible/keeper/v2_deposits.go#L84
Added line #L84 was not covered by tests


[warning] 87-87: x/fungible/keeper/v2_deposits.go#L87
Added line #L87 was not covered by tests


[warning] 91-91: x/fungible/keeper/v2_deposits.go#L91
Added line #L91 was not covered by tests


[warning] 99-99: x/fungible/keeper/v2_deposits.go#L99
Added line #L99 was not covered by tests


[warning] 102-103: x/fungible/keeper/v2_deposits.go#L102-L103
Added lines #L102 - L103 were not covered by tests


[warning] 107-107: x/fungible/keeper/v2_deposits.go#L107
Added line #L107 was not covered by tests

x/crosschain/keeper/cctx_orchestrator_validate_outbound.go

[warning] 386-390: x/crosschain/keeper/cctx_orchestrator_validate_outbound.go#L386-L390
Added lines #L386 - L390 were not covered by tests


[warning] 393-399: x/crosschain/keeper/cctx_orchestrator_validate_outbound.go#L393-L399
Added lines #L393 - L399 were not covered by tests

🔇 Additional comments (3)
x/fungible/keeper/v2_deposits.go (1)

Line range hint 62-72: Ensure safe handling of nil responses

When k.getAndCheckZRC20 returns an error, the function rightly returns nil for the response. However, it's crucial to ensure that all callers of ProcessV2RevertDeposit handle the possibility of a nil response to prevent nil pointer dereferences.

Please verify that all invocations of ProcessV2RevertDeposit properly handle a nil response in case of an error. This ensures robustness against potential nil pointer exceptions.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 62-62: x/fungible/keeper/v2_deposits.go#L62
Added line #L62 was not covered by tests

testutil/keeper/mocks/crosschain/fungible.go (1)

422-448: LGTM!

The implementation correctly handles the return values and follows mockery's best practices for type assertions and error handling.

cmd/zetae2e/local/evm.go (1)

62-62: Confirm the inclusion of the new test enhances coverage

The addition of TestETHWithdrawAndCallRevertWithWithdrawName to the revert cases for the gas token workflow appropriately expands test coverage for ETH withdrawals during reverts. Ensure that this test is correctly integrated and does not interfere with existing test sequences.

Comment on lines +373 to +379
if fungibletypes.IsContractReverted(evmTxResponse, err) {
// this is a contract revert, we commit the state to save the emitted logs related to revert
commit()
return errors.Wrap(err, "revert transaction reverted")
} else if err != nil {
// this should not happen and we don't commit the state to avoid inconsistent state
return errors.Wrap(err, "revert transaction couldn't be processed")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure comprehensive error handling

In the error handling of ProcessV2RevertDeposit, there might be unhandled scenarios which could lead to inconsistent state or unexpected behavior.

Review the error handling logic to ensure that all potential errors are properly managed and that the state is only committed when appropriate. This prevents possible state inconsistencies in cases of unexpected errors.

Comment on lines +382 to +402
// a withdrawal event in the logs could generate cctxs for outbound transactions.
if !evmTxResponse.Failed() {
logs := evmtypes.LogsToEthereum(evmTxResponse.Logs)
if len(logs) > 0 {
tmpCtx = tmpCtx.WithValue(InCCTXIndexKey, cctx.Index)
txOrigin := cctx.InboundParams.TxOrigin
if txOrigin == "" {
txOrigin = cctx.InboundParams.Sender
}

// process logs to process cctx events initiated during the contract call
err = k.ProcessLogs(tmpCtx, logs, to, txOrigin)
if err != nil {
// this happens if the cctx events are not processed correctly with invalid withdrawals
// in this situation we want the CCTX to be reverted, we don't commit the state so the contract call is not persisted
// the contract call is considered as reverted
return errors.Wrap(types.ErrCannotProcessWithdrawal, err.Error())
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance log processing robustness

The processing of logs from evmTxResponse could be improved with additional error checks and validations. This ensures that any new cross-chain transactions (CCTXs) generated from the logs are processed correctly.

Consider implementing more thorough checks around the ProcessLogs function to handle possible errors effectively. Ensure that any failure in processing the logs does not lead to unintended state commitments.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 386-390: x/crosschain/keeper/cctx_orchestrator_validate_outbound.go#L386-L390
Added lines #L386 - L390 were not covered by tests


[warning] 393-399: x/crosschain/keeper/cctx_orchestrator_validate_outbound.go#L393-L399
Added lines #L393 - L399 were not covered by tests

}
}

return fmt.Errorf("unsupported coin type for revert %s", coinType)
return nil, fmt.Errorf("unsupported coin type for revert %s", coinType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle all coin types explicitly

The default return statement should ensure that all possible coinType values are handled explicitly. This prevents unexpected behavior if an unsupported coinType is encountered.

Consider adding case statements for all possible coinType values or updating the error message to reflect the unsupported type accurately.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 107-107: x/fungible/keeper/v2_deposits.go#L107
Added line #L107 was not covered by tests

Comment on lines +175 to +192

// withdraw funds to the sender on connected chain
if (isWithdrawMessage(string(revertContext.revertMessage))) {
(address feeToken, uint256 feeAmount) = IZRC20(revertContext.asset).withdrawGasFee();
require(feeToken == revertContext.asset, "zrc20 is not gas token");
require(feeAmount <= revertContext.amount, "fee amount is higher than the amount");
uint256 withdrawAmount = revertContext.amount - feeAmount;

IZRC20(revertContext.asset).approve(msg.sender, revertContext.amount);

// caller is the gateway
IGatewayZEVM(msg.sender).withdraw(
abi.encode(revertContext.sender),
withdrawAmount,
revertContext.asset,
RevertOptions(address(0), false, address(0), "", 0)
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add reentrancy protection to onRevert function

The onRevert function performs external calls and token transfers, which can expose the contract to reentrancy attacks. Implementing reentrancy guards enhances the contract's security by preventing malicious reentrant calls.

Inherit from OpenZeppelin's ReentrancyGuard and mark the onRevert function with the nonReentrant modifier:

+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

-contract TestDAppV2 {
+contract TestDAppV2 is ReentrancyGuard {

    ...

-    function onRevert(RevertContext calldata revertContext) external {
+    function onRevert(RevertContext calldata revertContext) external nonReentrant {

        // function body remains unchanged
    }

Ensure that OpenZeppelin contracts are imported correctly and that the project complies with the MIT license of the imported code.

Committable suggestion skipped: line range outside the PR's diff.

require(feeAmount <= revertContext.amount, "fee amount is higher than the amount");
uint256 withdrawAmount = revertContext.amount - feeAmount;

IZRC20(revertContext.asset).approve(msg.sender, revertContext.amount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Adjust approval amount to match the withdrawal amount

Approving revertContext.amount may unintentionally approve more tokens than intended, especially if feeAmount is deducted. To enhance security, approve only the withdrawAmount calculated after subtracting the fee.

Apply this diff to correct the approval amount:

-    IZRC20(revertContext.asset).approve(msg.sender, revertContext.amount);
+    IZRC20(revertContext.asset).approve(msg.sender, withdrawAmount);

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +177 to +181
if (isWithdrawMessage(string(revertContext.revertMessage))) {
(address feeToken, uint256 feeAmount) = IZRC20(revertContext.asset).withdrawGasFee();
require(feeToken == revertContext.asset, "zrc20 is not gas token");
require(feeAmount <= revertContext.amount, "fee amount is higher than the amount");
uint256 withdrawAmount = revertContext.amount - feeAmount;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid unsafe casting from bytes to string

Casting bytes to string using string() can result in data corruption if the bytes are not valid UTF-8 encoded strings. It's safer and more gas-efficient to compare bytes directly without conversion. Modify the isWithdrawMessage function and related code to operate on bytes instead of string.

Apply this diff to compare bytes directly and avoid unsafe casting:

-function isWithdrawMessage(string memory message) internal pure returns (bool) {
-    return keccak256(abi.encodePacked(message)) == keccak256(abi.encodePacked("withdraw"));
+function isWithdrawMessage(bytes memory message) internal pure returns (bool) {
+    return keccak256(message) == keccak256("withdraw");
}

...

-function onRevert(RevertContext calldata revertContext) external {
+function onRevert(RevertContext calldata revertContext) external {

    ...

-    if (isWithdrawMessage(string(revertContext.revertMessage))) {
+    if (isWithdrawMessage(revertContext.revertMessage)) {
        // withdraw logic
    }
}

Committable suggestion skipped: line range outside the PR's diff.

func TestETHWithdrawAndCallRevertWithWithdraw(r *runner.E2ERunner, args []string) {
require.Len(r, args, 1)

amount := utils.ParseBigInt(r, args[0])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Add amount validation to prevent invalid test scenarios

The suggestion to validate the amount is correct. While utils.ParseBigInt ensures successful parsing, it doesn't validate that the amount is greater than zero. Adding this validation aligns with the production code's validation patterns found in the fungible keeper and bank precompiles, ensuring meaningful test scenarios.

require.True(r, amount.Cmp(big.NewInt(0)) > 0, "Amount must be greater than zero")
🔗 Analysis chain

Validate input amount before proceeding

The amount parsed from the arguments should be validated to ensure it is greater than zero. This prevents tests from running with invalid amounts, which could lead to misleading results.

Add a check to validate the amount:

require.True(r, amount.Cmp(big.NewInt(0)) > 0, "Amount must be greater than zero")
🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found


Script:

#!/bin/bash
# Check utils.ParseBigInt implementation
ast-grep --pattern 'func ParseBigInt($$$) $$$'

# Look at the complete test file
cat e2e/e2etests/test_eth_withdraw_and_call_revert_with_withdraw.go

# Search for similar validation patterns in test files
rg "amount.*Cmp.*big\.NewInt\(0\)" -A 2 -B 2

Length of output: 2597

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Withdraw inside onRevert does not trigger a CCTX
1 participant