feat: refund tokens inside onRevert and onAbort #61
Conversation
📝 WalkthroughWalkthroughThe pull request updates the error handling and refund logic in both NFT and Token core contracts for EVM and ZetaChain implementations. New error types are introduced— Changes
Sequence Diagram(s)sequenceDiagram
participant G as Gateway
participant C as Contract
participant S as Sender
G->>C: call onRevert(context)
C->>C: Check if context.amount > 0?
alt Refund Required
C->>S: Transfer Ether/Token for refund
S-->>C: Transfer result (Success/Failure)
alt Transfer Fails
C->>C: Revert with error (GasTokenRefundFailed/TokenRefundFailed)
else Transfer Succeeds
C->>G: Emit event and complete execution
end
else
C->>G: Complete execution without refund
end
Possibly related PRs
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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
|
| @@ -178,12 +179,16 @@ | |||
| * @param context The revert context containing metadata and revert message. | |||
| */ | |||
| function onRevert(RevertContext calldata context) external onlyGateway { | |||
| (uint256 amount, address receiver) = abi.decode( | |||
| (uint256 amount, address sender) = abi.decode( | |||
Check notice
Code scanning / Slither
Missing zero address validation Low
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contracts/nft/package.json (1)
34-34: Update of@zetachain/localnetDependency VersionThe dependency version has been updated from an earlier release (presumably
6.0.0-rc6) to7.1.0-rc1in the devDependencies. This aligns with the PR’s objectives for upgrading localnet to support the enhanced refund functionality. Please ensure that this upgrade does not introduce any breaking changes and that all integration tests covering localnet interactions are updated accordingly.contracts/token/package.json (1)
31-31: Consistent Localnet Dependency UpgradeThe
@zetachain/localnetdependency in this package has been updated to"7.1.0-rc1", matching the change in the NFT package. This consistency is crucial for ensuring uniform behavior across contracts. Verify that any changes in the new dependency version (such as API alterations or behavioral modifications) are fully compatible with the token contracts and that related tests reflect these updates.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
contracts/nft/yarn.lockis excluded by!**/yarn.lock,!**/*.lockcontracts/token/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
contracts/nft/contracts/evm/UniversalNFTCore.sol(2 hunks)contracts/nft/contracts/zetachain/UniversalNFTCore.sol(3 hunks)contracts/nft/package.json(2 hunks)contracts/nft/scripts/localnet.sh(2 hunks)contracts/token/contracts/evm/UniversalTokenCore.sol(2 hunks)contracts/token/contracts/zetachain/UniversalTokenCore.sol(3 hunks)contracts/token/package.json(2 hunks)contracts/token/scripts/localnet.sh(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
contracts/token/contracts/evm/UniversalTokenCore.sol
[notice] 182-182: Missing zero address validation
UniversalTokenCore.onRevert(RevertContext).sender (contracts/evm/UniversalTokenCore.sol#182) lacks a zero-check on :
- (success,None) = address(sender).call{value: context.amount}() (contracts/evm/UniversalTokenCore.sol#188)
contracts/nft/contracts/evm/UniversalNFTCore.sol
[warning] 186-200: Low-level calls
Low level call in UniversalNFTCore.onRevert(RevertContext) (contracts/evm/UniversalNFTCore.sol#186-200):
- (success,None) = address(sender).call{value: context.amount}() (contracts/evm/UniversalNFTCore.sol#196)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: slither (contracts/token, token.sarif)
- GitHub Check: slither (contracts/nft, nft.sarif)
🔇 Additional comments (17)
contracts/token/contracts/zetachain/UniversalTokenCore.sol (3)
49-49: Added a new error type for specific token refund failures.This new error type improves error clarity by distinguishing token refund failures from other transfer failures.
258-258: Improved error specificity in onRevert function.Changed from generic
TransferFailedto the more specificTokenRefundFailederror, which better describes the failure context.
271-275: Added token refunding to onAbort function.This change ensures that unused ZRC-20 tokens are properly refunded when a cross-chain transfer from ZetaChain to a connected chain fails, matching the PR objectives.
contracts/token/contracts/evm/UniversalTokenCore.sol (2)
39-39: Added a new error type for gas token refund failures.This new error type improves error clarity by specifically identifying gas token refund failures.
182-192:❓ Verification inconclusive
Implemented gas token refunding in onRevert function.
The changes correctly refund unused gas tokens to the sender when a cross-chain transfer fails, aligning with the PR objectives. However, there's a potential issue with the implementation.
The code is missing a zero address check for the sender before attempting to refund gas tokens, which could lead to tokens being lost if the sender is the zero address.
- (bool success, ) = payable(sender).call{value: context.amount}(""); - if (!success) revert GasTokenRefundFailed(); + if (sender != address(0)) { + (bool success, ) = payable(sender).call{value: context.amount}(""); + if (!success) revert GasTokenRefundFailed(); + }Run the following script to check if there are other instances of this pattern:
🏁 Script executed:
#!/bin/bash # Search for call with value but missing zero address check rg -A 5 -e 'call\{value:' --no-filename | grep -v 'if.*address\(0\)' | grep -A 2 'call\{value:'Length of output: 116
Gas Token Refund: Validate Zero Address Check in onRevert
The changes effectively add gas token refunding, but please note that the implementation currently lacks a check for a zero address before attempting the refund. Without this check, if the sender is the zero address, tokens could be lost. The proposed diff below demonstrates the necessary update:
- (bool success, ) = payable(sender).call{value: context.amount}(""); - if (!success) revert GasTokenRefundFailed(); + if (sender != address(0)) { + (bool success, ) = payable(sender).call{value: context.amount}(""); + if (!success) revert GasTokenRefundFailed(); + }A verification script was run to scan for similar patterns (i.e. calls with value not guarded by a zero address check), but it produced an error due to unmatched curly braces in the grep pattern. To ensure comprehensive coverage, please manually verify (or re-run with a corrected script, such as using fixed-string search) that no other parts of the codebase are missing this safeguard.
🧰 Tools
🪛 GitHub Check: Slither
[notice] 182-182: Missing zero address validation
UniversalTokenCore.onRevert(RevertContext).sender (contracts/evm/UniversalTokenCore.sol#182) lacks a zero-check on :
- (success,None) = address(sender).call{value: context.amount}() (contracts/evm/UniversalTokenCore.sol#188)contracts/token/scripts/localnet.sh (2)
7-7: Simplified localnet script initialization.The script now unconditionally starts the localnet without requiring a "start" argument, which simplifies usage.
76-76: Ensured localnet is always stopped after execution.The script now always stops the localnet when finished, ensuring proper cleanup regardless of how the script was invoked.
contracts/nft/contracts/zetachain/UniversalNFTCore.sol (3)
50-50: Added a new error type for specific token refund failures.This new error type improves error clarity by distinguishing token refund failures from other transfer failures, maintaining consistency with the token contracts.
272-272: Improved error specificity in onRevert function.Changed from generic
TransferFailedto the more specificTokenRefundFailederror, which better describes the failure context.
295-299: Added token refunding to onAbort function.This change ensures that unused ZRC-20 tokens are properly refunded when a cross-chain NFT transfer from ZetaChain to a connected chain fails, matching the PR objectives and providing a consistent pattern with the token contracts.
contracts/nft/contracts/evm/UniversalNFTCore.sol (3)
40-40: Good addition of specific error typeAdding the
GasTokenRefundFailederror provides clear error reporting when a token refund fails during the revert process.
186-188: Appropriate payable modifier addedThe
payablemodifier is correctly added to theonRevertfunction since it now handles value transfers for gas token refunds.🧰 Tools
🪛 GitHub Check: Slither
[warning] 186-200: Low-level calls
Low level call in UniversalNFTCore.onRevert(RevertContext) (contracts/evm/UniversalNFTCore.sol#186-200):
- (success,None) = address(sender).call{value: context.amount}() (contracts/evm/UniversalNFTCore.sol#196)
195-198:✅ Verification successful
Effective implementation of gas token refund
The implementation correctly handles refunding gas tokens to the sender by:
- Checking if there's an amount to refund
- Attempting the transfer
- Reverting with a specific error if the transfer fails
This aligns with the PR objective to refund unused gas tokens when a cross-chain transfer fails.
🏁 Script executed:
#!/bin/bash # Check for similar refund patterns in other contracts to ensure consistency rg -A 3 'if \(context.amount > 0\)' --type solLength of output: 76
Refund functionality verified, pending manual consistency check
The refund implementation in
contracts/nft/contracts/evm/UniversalNFTCore.sol(lines 195–198) correctly:
- Checks if there's a gas token refund amount
- Attempts the transfer using a low-level
call- Reverts with
GasTokenRefundFailed()if the transfer failsThis approach aligns with the PR objective to refund unused gas tokens when a cross-chain transfer fails.
For consistency, please manually verify that the same refund pattern is used in other contracts. Note that our earlier ripgrep command reported an “unrecognized file type: sol” error; the revised command above should help you identify similar patterns across Solidity files.
--- 🏁 Script executed: ```shell #!/bin/bash # Revised search: Use the glob pattern to match Solidity files rg -A 3 'if \(context.amount > 0\)' -g "*.sol"Length of output: 943
Consistent Refund Implementation Verified
The refund logic in
contracts/nft/contracts/evm/UniversalNFTCore.sol(lines 195–198) has been confirmed to be consistent with similar logic incontracts/token/contracts/evm/UniversalTokenCore.sol. The implementation correctly:
- Checks if there's a refund amount (
context.amount > 0)- Performs a low-level call to refund the gas tokens
- Reverts with
GasTokenRefundFailed()if the transfer failsThis complies with the PR objective to refund unused gas tokens when a cross-chain transfer fails.
contracts/nft/scripts/localnet.sh (4)
7-7: Simplified script flowRemoving the conditional start makes the script simpler and ensures the localnet always starts when the script runs.
70-74: Transfer from BNB to ZetaChain commented outThe BNB to ZetaChain transfer and subsequent checks have been disabled. Is this intentional as part of the refund token implementation, or is this a temporary change for testing?
76-76: Network stop command commented outThe command to stop the localnet has been commented out. This means the network will continue running after the script completes. Ensure this is the intended behavior for your development workflow.
65-65:❓ Verification inconclusive
Significant gas amount reduction
The gas amount for transferring from Ethereum to BNB has been reduced from 1 to 0.000000000001, which is a very small value. This might be to test the refund functionality or to optimize gas usage, but ensure it's sufficient for the operation.
🏁 Script executed:
#!/bin/bash # Check similar gas amount parameters in other scripts for consistency rg "gas-amount" --type shLength of output: 738
Gas Parameter Discrepancy: Confirm Intentional Low Value
The NFT transfer command in
contracts/nft/scripts/localnet.sh(line 65) uses--gas-amount 0.000000000001, whereas similar commands (both in NFT and token transfers) use--gas-amount 1. This significant reduction might be intentional for testing refund functionality or optimizing gas usage, but please verify that this lower value is sufficient for the operation. If it’s deliberate, consider adding a clarifying comment to avoid future confusion.
- Affected Command:
npx hardhat nft:transfer --network localhost --json --token-id "$NFT_ID" --contract "$CONTRACT_ETHEREUM" --destination "$ZRC20_BNB" --gas-amount 0.000000000001- Consistency Check: Other transfer scripts (e.g., in
contracts/token/scripts/localnet.sh) use a gas amount of1.
|
@zeta-chain/smart-contracts please, review. |
| @@ -290,6 +291,12 @@ abstract contract UniversalNFTCore is | |||
| _safeMint(sender, tokenId); | |||
| _setTokenURI(tokenId, uri); | |||
| emit TokenTransferAborted(sender, tokenId, uri, context.outgoing); | |||
|
|
|||
| if (context.amount > 0 && context.asset != address(0)) { | |||
There was a problem hiding this comment.
for all functions, do we want to include refund information in events somewhere?
There was a problem hiding this comment.
Added refund asset and amount to revert and abort events: 44c11aa
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contracts/token/contracts/evm/UniversalTokenCore.sol (1)
184-193:⚠️ Potential issueImplement the token refund functionality but missing zero address check.
The implementation correctly refunds gas tokens to the sender when a cross-chain transfer fails, which prevents tokens from being locked in the contract. However, there's no validation that
senderis not the zero address before transferring funds.Add a zero address check before performing the refund:
function onRevert(RevertContext calldata context) external onlyGateway { (uint256 amount, address sender) = abi.decode( context.revertMessage, (uint256, address) ); _mint(sender, amount); if (context.amount > 0) { + if (sender == address(0)) revert InvalidAddress(); (bool success, ) = payable(sender).call{value: context.amount}(""); if (!success) revert GasTokenRefundFailed(); } emit TokenTransferReverted(sender, amount, address(0), context.amount); }
🧹 Nitpick comments (1)
contracts/token/contracts/shared/UniversalTokenEvents.sol (1)
14-25: Consider adding indexed parameters for refundAsset.While the enhanced event signatures are an improvement, you might want to consider indexing the
refundAssetparameter in both events. This would make it more efficient to filter logs and query for events related to specific token assets being refunded, which could be valuable for monitoring systems and user interfaces.event TokenTransferReverted( address indexed sender, uint256 amount, - address refundAsset, + address indexed refundAsset, uint256 refundAmount ); event TokenTransferAborted( address indexed sender, uint256 amount, - address refundAsset, + address indexed refundAsset, uint256 refundAmount );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contracts/nft/contracts/evm/UniversalNFTCore.sol(2 hunks)contracts/nft/contracts/shared/UniversalNFTEvents.sol(1 hunks)contracts/nft/contracts/zetachain/UniversalNFTCore.sol(3 hunks)contracts/token/contracts/evm/UniversalTokenCore.sol(2 hunks)contracts/token/contracts/shared/UniversalTokenEvents.sol(1 hunks)contracts/token/contracts/zetachain/UniversalTokenCore.sol(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/nft/contracts/zetachain/UniversalNFTCore.sol
⏰ Context from checks skipped due to timeout of 90000ms (4)
-
<
8000
li>GitHub Check: test (contracts/token)
- GitHub Check: test (contracts/nft)
- GitHub Check: slither (contracts/token, token.sarif)
- GitHub Check: slither (contracts/nft, nft.sarif)
🔇 Additional comments (9)
contracts/token/contracts/evm/UniversalTokenCore.sol (1)
39-39: Good addition of specific error type.Adding a dedicated error type for gas token refund failures improves error handling clarity and follows good practices for specific error reporting.
contracts/token/contracts/zetachain/UniversalTokenCore.sol (3)
49-49: Good addition of specific error type.Adding a dedicated error type for token refund failures improves error handling clarity and follows good practices for specific error reporting.
254-264: Updated event emission with refund details.The event emission now includes additional context about the refund asset and amount, which improves traceability and debugging capabilities.
274-285: Properly implemented token refund in onAbort function.The function now correctly refunds ZRC-20 tokens to the sender when a cross-chain transfer from ZetaChain to a connected chain fails, preventing tokens from being locked in the contract. The implementation mirrors the logic in the onRevert function, maintaining consistency in the codebase.
contracts/nft/contracts/evm/UniversalNFTCore.sol (1)
40-40: Good addition of specific error type.Adding a dedicated error type for gas token refund failures improves error handling clarity and follows good practices for specific error reporting.
contracts/nft/contracts/shared/UniversalNFTEvents.sol (2)
19-25: Enhanced event signature with refund information.Adding refund asset and amount parameters to the event provides better traceability for cross-chain transfer failures, which will help with monitoring and debugging.
26-33: Enhanced event signature with refund information.Adding refund asset and amount parameters to the event provides better traceability for cross-chain transfer aborts, which will help with monitoring and debugging.
contracts/token/contracts/shared/UniversalTokenEvents.sol (2)
14-19: Enhanced event signature improves refund transparency.The addition of
refundAssetandrefundAmountparameters to theTokenTransferRevertedevent significantly improves traceability of cross-chain transfer failures. These new fields allow for precise tracking of what assets and amounts are being refunded when transfers revert, which aligns well with the PR's goal of improving refund handling.
20-25: Enhanced event signature for aborted transfers.Similar to the reverted event, the
TokenTransferAbortedevent has been improved with the addition ofrefundAssetandrefundAmountparameters. This creates consistency in the event structure between revert and abort scenarios, making it easier for developers and monitoring tools to track refund operations across different failure modes.
onReverton EVM when cross-chain transfer from one connected chain to another fails. Before, the unused gas tokens remained in the NFT contract, now we're refunding them back to the sender.onAborton ZetaChain when cross-chain transfer from ZetaChain to a connected chain fails.startfrom the localnet script as it should always start localnet even without thestartargumentv7.1.0-rc1Summary by CodeRabbit
Bug Fixes
New Features
Chores