fix(swap): decode non-EVM receiver addresses from Bitcoin #256
Conversation
📝 WalkthroughWalkthroughA new custom error, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SwapContract
Caller->>SwapContract: onCall(chainId, message)
alt Bitcoin or Bitcoin Testnet
SwapContract->>SwapContract: Check message length >= 41
alt Invalid length
SwapContract-->>Caller: revert InvalidMessageLength()
else Valid length
SwapContract->>SwapContract: Parse params.to from message[20:]
SwapContract->>SwapContract: Parse withdraw flag from last byte
end
else Other chains
SwapContract->>SwapContract: Standard parsing logic
end
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
|
@zeta-chain/smart-contracts please, review. |
|
I have a few security improvement proposals that might come in handy. |
But this PR is intended to handle variable length receiver address. The reason why have a loop is to handle any kind of address, both Sui and Solana. |
![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
🧹 Nitpick comments (3)
examples/swap/contracts/Swap.sol (3)
97-100: Replace manual byte copying with more gas-efficient methods.The current implementation manually copies bytes one at a time which is gas-inefficient. Consider using memory copy operations for better performance.
- params.to = new bytes(message.length - 21); - for (uint256 i = 0; i < message.length - 21; i++) { - params.to[i] = message[20 + i]; - } + // Extract recipient address from message starting at byte 20 + uint256 recipientLength = message.length - 21; + params.to = new bytes(recipientLength); + assembly { + let toPtr := add(params.to, 0x20) + let msgPtr := add(add(message, 0x20), 20) // 0x20 (array header) + 20 (starting position) + mstore(toPtr, mload(msgPtr)) + + // If recipient is longer than 32 bytes, copy the rest + if gt(recipientLength, 32) { + mstore(add(toPtr, 32), mload(add(msgPtr, 32))) + } + }Alternatively, for clarity and maintainability at a slight gas cost:
- params.to = new bytes(message.length - 21); - for (uint256 i = 0; i < message.length - 21; i++) { - params.to[i] = message[20 + i]; - } + uint256 recipientLength = message.length - 21; + params.to = new bytes(recipientLength); + for (uint256 i = 0; i < recipientLength; i += 32) { + // Copy up to 32 bytes at a time + uint256 chunkSize = recipientLength - i < 32 ? recipientLength - i : 32; + bytes32 chunk; + assembly { + chunk := mload(add(add(add(message, 0x20), 20), i)) + } + for (uint256 j = 0; j < chunkSize; j++) { + params.to[i + j] = bytes1(uint8(chunk[j])); + } + }
101-104: Reading withdraw flag from last byte aligns with variable-length design.This change correctly reads the withdraw flag from the last byte of the message, supporting variable-length addresses as required. However, consider adding documentation to clarify the expected message format.
Add a comment explaining the expected message format:
+ // Message format for Bitcoin chains: + // [0:20] - Target token address + // [20:(message.length-1)] - Recipient address of variable length + // [message.length-1] - Withdraw flag params.withdraw = BytesHelperLib.bytesToBool( message, message.length - 1 );
94-105: Document the Bitcoin message format in a comment block.The changes properly implement variable-length address support for non-EVM chains, but lack documentation to explain the expected format.
Add a detailed comment block explaining the Bitcoin message format:
if (context.chainID == BITCOIN_TESTNET || context.chainID == BITCOIN) { + // Bitcoin message format: + // - Minimum length: 41 bytes + // - Structure: + // * Bytes 0-19: Target token address (20 bytes) + // * Bytes 20-(N-2): Recipient address (variable length) + // * Byte N-1: Withdraw flag (1 byte) + // This format supports variable-length recipient addresses from different chains + // such as Sui and Solana that don't use fixed 20-byte addresses like EVM chains. if (message.length < 41) revert InvalidMessageLength();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/swap/contracts/Swap.sol(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test (examples/token)
- GitHub Check: test (examples/swap)
- GitHub Check: test (examples/nft)
- GitHub Check: test (examples/hello)
- GitHub Check: test (examples/call)
- GitHub Check: slither (examples/swap, swap.sarif)
- GitHub Check: slither (examples/call, call.sarif)
- GitHub Check: slither (examples/hello, hello.sarif)
🔇 Additional comments (1)
examples/swap/contracts/Swap.sol (1)
37-37: Custom error added for better error reporting.Adding a custom error for invalid message length enhances error reporting and follows best practices for error handling in Solidity. This change is appropriate given the new validation check.
| @@ -91,13 +92,16 @@ contract Swap is | |||
| }); | |||
|
|
|||
| if (context.chainID == BITCOIN_TESTNET || context.chainID == BITCOIN) { | |||
| if (message.length < 41) revert InvalidMessageLength(); | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding maximum length validation.
While you've added a minimum length validation which is good, consider also adding a maximum length check to prevent excessively large messages that could lead to out-of-gas errors when processing.
- if (message.length < 41) revert InvalidMessageLength();
+ if (message.length < 41 || message.length > 100) revert InvalidMessageLength();Let's check if the codebase handles any potential upper bounds for messages elsewhere:
🏁 Script executed:
#!/bin/bash
# Search for any message length validations in the repo
rg -A 2 -B 2 "message.length" --glob "*.sol"Length of output: 1025
Enforce an upper bound on message.length
To prevent out-of-gas attacks from excessively large payloads, add a maximum length check alongside the existing minimum check.
• File: examples/swap/contracts/Swap.sol
Line: 95
Suggested diff:
@@ examples/swap/contracts/Swap.sol:95
- if (message.length < 41) revert InvalidMessageLength();
+ // Validate both minimum and maximum size to guard against oversized messages
+ if (message.length < 41 || message.length > 100) revert InvalidMessageLength();Consider defining a named constant for clarity:
uint256 private constant MIN_MESSAGE_LENGTH = 41;
uint256 private constant MAX_MESSAGE_LENGTH = 100;
...
if (message.length < MIN_MESSAGE_LENGTH || message.length > MAX_MESSAGE_LENGTH) {
revert InvalidMessageLength();
}| if (context.chainID == BITCOIN_TESTNET || context.chainID == BITCOIN) { | ||
| if (message.length < 41) revert InvalidMessageLength(); | ||
| params.target = BytesHelperLib.bytesToAddress(message, 0); | ||
| params.to = abi.encodePacked( | ||
| BytesHelperLib.bytesToAddress(message, 20) | ||
| ); | ||
| if (message.length >= 41) { | ||
| params.withdraw = BytesHelperLib.bytesToBool(message, 40); | ||
| params.to = new bytes(message.length - 21); | ||
| for (uint256 i = 0; i < message.length - 21; i++) { | ||
| params.to[i] = message[20 + i]; | ||
| } | ||
| params.withdraw = BytesHelperLib.bytesToBool( | ||
| message, | ||
| message.length - 1 | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate target address to prevent security issues.
There is no validation that the target address (extracted at byte 0) is not the zero address, which could lead to security vulnerabilities.
if (message.length < 41) revert InvalidMessageLength();
params.target = BytesHelperLib.bytesToAddress(message, 0);
+ if (params.target == address(0)) revert InvalidAddress();
params.to = new bytes(message.length - 21);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (context.chainID == BITCOIN_TESTNET || context.chainID == BITCOIN) { | |
| if (message.length < 41) revert InvalidMessageLength(); | |
| params.target = BytesHelperLib.bytesToAddress(message, 0); | |
| params.to = abi.encodePacked( | |
| BytesHelperLib.bytesToAddress(message, 20) | |
| ); | |
| if (message.length >= 41) { | |
| params.withdraw = BytesHelperLib.bytesToBool(message, 40); | |
| params.to = new bytes(message.length - 21); | |
| for (uint256 i = 0; i < message.length - 21; i++) { | |
| params.to[i] = message[20 + i]; | |
| } | |
| params.withdraw = BytesHelperLib.bytesToBool( | |
| message, | |
| message.length - 1 | |
| ); | |
| } else { | |
| if (context.chainID == BITCOIN_TESTNET || context.chainID == BITCOIN) { | |
| if (message.length < 41) revert InvalidMessageLength(); | |
| params.target = BytesHelperLib.bytesToAddress(message, 0); | |
| if (params.target == address(0)) revert InvalidAddress(); | |
| params.to = new bytes(message.length - 21); | |
| for (uint256 i = 0; i < message.length - 21; i++) { | |
| params.to[i] = message[20 + i]; | |
| } | |
| params.withdraw = BytesHelperLib.bytesToBool( | |
| message, | |
| message.length - 1 | |
| ); | |
| } else { |
| ); | ||
| if (message.length >= 41) { | ||
| params.withdraw = BytesHelperLib.bytesToBool(message, 40); | ||
| params.to = new bytes(message.length - 21); |
There was a problem hiding this comment.
i think some comments might help here, and some constants, but up to you
btw do we need to open issue to not forget about solana related discussion regarding this topic? or is already there?
There was a problem hiding this comment.
Thanks for the suggestion! I'm going to refactor this pretty soon to make sure it supports ABI encoding through inscriptions.
There was a problem hiding this comment.
I think the Solana issue is just that it doesn't fit in OP_RETURN, nothing has changed on that side. What we should do is switch to inscriptions by default to make Bitcoin naturally compatible with other chains, like swapping BTC into SOL.
✅ Sui
https://mempool.space/testnet4/tx/af6366356176878d5a0b8a3337e38472885d725227033d864f0dae38c5e56156
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/af6366356176878d5a0b8a3337e38472885d725227033d864f0dae38c5e56156
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0xda4963954fba112504f65479872ccfc848ce7c9a6c97213479a43805bf784d0c
❌ Solana
https://mempool.space/testnet4/tx/9b5977918764a643690794292ed2a9412bd4bfd4f7337884e8799cd38f2ed64b
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/9b5977918764a643690794292ed2a9412bd4bfd4f7337884e8799cd38f2ed64b
Solana doesn't work, because we're sending base58 bytes as the recipient, but the protocol expects receiver as bytes, which doesn't fit in OP_RETURN.
✅ Base
Just checking that nothing is broken with regular EVM transfers.
https://mempool.space/testnet4/tx/2a663048ccca46a8c3b5bd31ae9e02f29e829e94bbe43c00e1203aacca037ca5
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/2a663048ccca46a8c3b5bd31ae9e02f29e829e94bbe43c00e1203aacca037ca5
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0x53b6d81b7e45cd46588a46386563d0a524854bf11ae66762999b5397ae698962
Summary by CodeRabbit
New Features
Bug Fixes