Skip to content

fix(swap): decode non-EVM receiver addresses from Bitcoin #256

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 13, 2025
Merged

fix(swap): decode non-EVM receiver addresses from Bitcoin #256

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
175edb5
fix(swap): decode non-EVM receiver addresses from Bitcoin
fadeev Apr 15, 2025
c787d7b
Merge branch 'main' into swap-non-evm-target
fadeev May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions examples/swap/contracts/Swap.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ contract Swap is
error ApprovalFailed();
error TransferFailed(string);
error InsufficientAmount(string);
error InvalidMessageLength();

event TokenSwap(
bytes sender,
Expand Down Expand Up @@ -91,13 +92,16 @@ contract Swap is
});

if (context.chainID == BITCOIN_TESTNET || context.chainID == BITCOIN) {
if (message.length < 41) revert InvalidMessageLength();
Copy link

Choose a reason for hiding this comment

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

💡 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();
}

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I'm going to refactor this pretty soon to make sure it supports ABI encoding through inscriptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

for (uint256 i = 0; i < message.length - 21; i++) {
params.to[i] = message[20 + i];
}
params.withdraw = BytesHelperLib.bytesToBool(
message,
message.length - 1
);
} else {
Comment on lines 94 to 105
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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 {

(
address targetToken,
Expand Down