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

Add revert handling to the Swap example #219

Merged
merged 70 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
70 commits
Select commit Hold shift + click to select a range
2b827bc
Update hello to auth calls
fadeev Nov 8, 2024
afa11f1
remove omnichain examples
fadeev Nov 8, 2024
b6a444d
wip
fadeev Nov 8, 2024
a5e15a1
swap works
fadeev Nov 8, 2024
50e7217
swap
fadeev Nov 9, 2024
f8357ad
deps and remove omnichain swap
fadeev Nov 11, 2024
b0f3d1b
nft: update toolkit
fadeev Nov 11, 2024
3150e9c
nft: sender and update localnet
fadeev Nov 11, 2024
d687d9c
hello: rename contracts to universal and connected
fadeev Nov 11, 2024
ff28b85
rename hello to calls
fadeev Nov 11, 2024
e5f6c5f
Merge branch 'main' into auth-call-hello-swap
fadeev Nov 11, 2024
b6886bf
rename to call
fadeev Nov 11, 2024
d420ab7
rename to call
fadeev Nov 11, 2024
936d399
call: depositAndCall
fadeev Nov 11, 2024
7476874
call: add deposit function
fadeev Nov 11, 2024
c2410f0
call: universal withdraw
fadeev Nov 11, 2024
ed3e800
call: withdraw and call arbitrary
fadeev Nov 11, 2024
68217a5
call: fix withdraw and call
fadeev Nov 12, 2024
c5e2348
onCall: remove return
fadeev Nov 12, 2024
5e05fb0
call: deposit/depositAndCall ERC-20s
fadeev Nov 12, 2024
deffb4d
returns bytes 4
fadeev Nov 12, 2024
382e08c
slither
fadeev Nov 12, 2024
f05a97c
call: reverts
fadeev Nov 12, 2024
f7b583a
hello example
fadeev Nov 13, 2024
5eb0da9
remove unused improts
fadeev Nov 13, 2024
1d72aee
slither
fadeev Nov 13, 2024
5b6234f
wip
fadeev Nov 15, 2024
44f7d72
nft: fix localnet
fadeev Nov 15, 2024
57ceeb8
rename test.sh to localnet.sh
fadeev Nov 16, 2024
6513b52
rename test.sh to localnet.sh
fadeev Nov 16, 2024
c1ab35e
rename test.sh to localnet.sh
fadeev Nov 16, 2024
8331f22
token: consistency
fadeev Nov 17, 2024
01700ef
wip
fadeev Nov 17, 2024
29bb170
fix
fadeev Nov 17, 2024
0b7ac4c
immutable
fadeev Nov 17, 2024
34e87b4
import types
fadeev Nov 17, 2024
822cb23
fix revert
fadeev Nov 17, 2024
85f91be
nft: rename sender to receiver
fadeev Nov 18, 2024
1495357
replace system contract with uniswap router
fadeev Nov 18, 2024
1df6664
token: deploy task
fadeev Nov 18, 2024
f5ad522
token: replace system contract with uniswap router
fadeev Nov 18, 2024
051ee41
default addresses
fadeev Nov 18, 2024
cfaf6fb
default addresses
fadeev Nov 18, 2024
9985dc7
add localnet to gitignore
fadeev Nov 18, 2024
757c438
remove default uniswap address
fadeev Nov 18, 2024
0cbaa24
nft: withdrawAndCall
fadeev Nov 18, 2024
13f3299
nft: return extra tokens to sender, handle a case where receiver is n…
fadeev Nov 18, 2024
75438c3
token: return to sender
fadeev Nov 18, 2024
aa55fa3
nft/token: handle revert on zetachain
fadeev Nov 18, 2024
fd03f00
fix
fadeev Nov 18, 2024
91b8121
remove placeholder event
fadeev Nov 18, 2024
1bda9b7
token: zero address validation
fadeev Nov 19, 2024
84beed4
swap: zero address validation
fadeev Nov 19, 2024
4cfaa55
immutable gas limit
fadeev Nov 19, 2024
16d8c13
rename counterparty
fadeev Nov 20, 2024
5013b00
rename counterparty
fadeev Nov 20, 2024
6a15652
rename counterparty
fadeev Nov 20, 2024
c89c6da
gateway revert
fadeev Nov 20, 2024
786d629
gateway revert
fadeev Nov 20, 2024
a6d9809
safeTransferFrom
fadeev Nov 20, 2024
83ab10b
revert
fadeev Nov 20, 2024
acd8da7
fix: Swap handle reverts
fadeev Nov 22, 2024
f425a77
Swap to any
fadeev Nov 22, 2024
581f201
remove extra approve
fadeev Nov 22, 2024
2d82f6c
Refactor
fadeev Nov 22, 2024
e815844
gas limit
fadeev Nov 22, 2024
5dfba30
gas immutable and deploy fix
fadeev Nov 22, 2024
a279ac1
handle errors
fadeev Nov 22, 2024
e111f44
throw error
fadeev Nov 22, 2024
77418a8
merge main
fadeev Nov 28, 2024
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
93 changes: 75 additions & 18 deletions examples/swap/contracts/Swap.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,27 @@ contract Swap is UniversalContract {
address public immutable uniswapRouter;
GatewayZEVM public gateway;
uint256 constant BITCOIN = 18332;
uint256 public immutable gasLimit;

error InvalidAddress();
error Unauthorized();
error ApprovalFailed();

modifier onlyGateway() {
if (msg.sender != address(gateway)) revert Unauthorized();
_;
}

constructor(address payable gatewayAddress, address uniswapRouterAddress) {
constructor(
address payable gatewayAddress,
address uniswapRouterAddress,
uint256 gasLimitAmount
) {
if (gatewayAddress == address(0) || uniswapRouterAddress == address(0))
revert InvalidAddress();
uniswapRouter = uniswapRouterAddress;
gateway = GatewayZEVM(gatewayAddress);
gasLimit = gasLimitAmount;
}

struct Params {
Expand Down Expand Up @@ -57,15 +64,19 @@ contract Swap is UniversalContract {
params.to = recipient;
}

swapAndWithdraw(zrc20, amount, params.target, params.to);
(uint256 out, address gasZRC20, uint256 gasFee) = handleGasAndSwap(
zrc20,
amount,
params.target
);
withdraw(params, context.sender, gasFee, gasZRC20, out, zrc20);
}

function swapAndWithdraw(
function handleGasAndSwap(
address inputToken,
uint256 amount,
address targetToken,
bytes memory recipient
) internal {
address targetToken
) internal returns (uint256, address, uint256) {
uint256 inputForGas;
address gasZRC20;
uint256 gasFee;
Expand Down Expand Up @@ -93,29 +104,75 @@ contract Swap is UniversalContract {
targetToken,
0
);
return (outputAmount, gasZRC20, gasFee);
}

if (gasZRC20 == targetToken) {
IZRC20(gasZRC20).approve(address(gateway), outputAmount + gasFee);
function withdraw(
Params memory params,
address sender,
uint256 gasFee,
address gasZRC20,
uint256 outputAmount,
address inputToken
) public {
if (gasZRC20 == params.target) {
if (
!IZRC20(gasZRC20).approve(
address(gateway),
outputAmount + gasFee
)
) {
revert ApprovalFailed();
}
} else {
IZRC20(gasZRC20).approve(address(gateway), gasFee);
IZRC20(targetToken).approve(address(gateway), outputAmount);
if (!IZRC20(gasZRC20).approve(address(gateway), gasFee)) {
revert ApprovalFailed();
}
if (
!IZRC20(params.target).approve(address(gateway), outputAmount)
) {
revert ApprovalFailed();
}
}

gateway.withdraw(
recipient,
params.to,
outputAmount,
targetToken,
params.target,
RevertOptions({
revertAddress: address(this),
callOnRevert: true,
abortAddress: address(0),
revertMessage: abi.encode(sender, inputToken),
onRevertGasLimit: gasLimit
})
);
}

function onRevert(RevertContext calldata context) external onlyGateway {
(address sender, address zrc20) = abi.decode(
context.revertMessage,
(address, address)
);
(uint256 out, , ) = handleGasAndSwap(
context.asset,
context.amount,
zrc20
);
gateway.withdraw(
abi.encodePacked(sender),
out,
zrc20,
RevertOptions({
revertAddress: address(0),
revertAddress: sender,
callOnRevert: false,
abortAddress: address(0),
revertMessage: "",
onRevertGasLimit: 0
onRevertGasLimit: gasLimit
})
);
}

function onRevert(
RevertContext calldata revertContext
) external onlyGateway {}
fallback() external payable {}

receive() external payable {}
Comment on lines +175 to +177
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure fallback and receive functions revert as intended

The fallback and receive functions are currently empty, which means they will accept Ether sent to the contract without any action. According to the PR objectives, these functions should revert when called to handle cases where the withdraw function fails on the destination chain. This ensures that any unexpected Ether transfers are properly rejected and the onRevert function is triggered appropriately.

Apply this diff to make the functions revert when called:

-fallback() external payable {}
+fallback() external payable {
+    revert("");
+}

-receive() external payable {}
+receive() external payable {
+    revert("");
+}
📝 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
fallback() external payable {}
receive() external payable {}
fallback() external payable {
revert("");
}
receive() external payable {
revert("");
}

}
176 changes: 122 additions & 54 deletions examples/swap/contracts/SwapToAnyToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,28 @@ contract SwapToAnyToken is UniversalContract {
address public immutable uniswapRouter;
GatewayZEVM public gateway;
uint256 constant BITCOIN = 18332;
uint256 public immutable gasLimit;

error InvalidAddress();
error Unauthorized();
error ApprovalFailed();
error TransferFailed();

modifier onlyGateway() {
if (msg.sender != address(gateway)) revert Unauthorized();
_;
}

constructor(address payable gatewayAddress, address uniswapRouterAddress) {
constructor(
address payable gatewayAddress,
address uniswapRouterAddress,
uint256 gasLimitAmount
) {
Comment on lines +31 to +35
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation for gasLimitAmount in the constructor

It's recommended to validate the gasLimitAmount parameter in the constructor to ensure it's within an acceptable range (e.g., greater than zero). This prevents potential misconfigurations that could lead to failed transactions due to insufficient gas limits.

Apply this suggested change:

     constructor(
         address payable gatewayAddress,
         address uniswapRouterAddress,
         uint256 gasLimitAmount
     ) {
+        if (gasLimitAmount == 0) {
+            revert InvalidGasLimit();
+        }
         if (gatewayAddress == address(0) || uniswapRouterAddress == address(0))
             revert InvalidAddress();
         uniswapRouter = uniswapRouterAddress;
         gateway = GatewayZEVM(gatewayAddress);
         gasLimit = gasLimitAmount;
     }

Add a new error declaration:

+    error InvalidGasLimit();

Also applies to: 40-40

if (gatewayAddress == address(0) || uniswapRouterAddress == address(0))
revert InvalidAddress();
uniswapRouter = uniswapRouterAddress;
gateway = GatewayZEVM(gatewayAddress);
gasLimit = gasLimitAmount;
}

struct Params {
Expand Down Expand Up @@ -69,95 +77,155 @@ contract SwapToAnyToken is UniversalContract {
params.withdraw = withdrawFlag;
}

swapAndWithdraw(
(uint256 out, address gasZRC20, uint256 gasFee) = handleGasAndSwap(
zrc20,
amount,
params.target,
params.to,
params.withdraw
params.target
);
withdraw(params, context.sender, gasFee, gasZRC20, out, zrc20);
}

function swapAndWithdraw(
function swap(
address inputToken,
uint256 amount,
address targetToken,
bytes memory recipient,
bool withdraw
) internal {
bool withdrawFlag
) public {
bool success = IZRC20(inputToken).transferFrom(
msg.sender,
address(this),
amount
);
if (!success) {
revert TransferFailed();
}

(uint256 out, address gasZRC20, uint256 gasFee) = handleGasAndSwap(
inputToken,
amount,
targetToken
);

withdraw(
Params({
target: targetToken,
to: recipient,
withdraw: withdrawFlag
}),
msg.sender,
gasFee,
gasZRC20,
out,
inputToken
);
}
Comment on lines +88 to +122
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider adding reentrancy protection to the swap function

The swap function involves token transfers and external calls, which could potentially be exploited in a reentrancy attack. It is advisable to add reentrancy protection using OpenZeppelin's ReentrancyGuard by inheriting from it and applying the nonReentrant modifier to the swap function.

Apply the following changes:

  1. Import ReentrancyGuard and inherit from it:
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

-contract SwapToAnyToken is UniversalContract {
+contract SwapToAnyToken is UniversalContract, ReentrancyGuard {
     address public immutable uniswapRouter;
     GatewayZEVM public gateway;
     uint256 constant BITCOIN = 18332;
     uint256 public immutable gasLimit;
  1. Apply the nonReentrant modifier to the swap function:
     function swap(
         address inputToken,
         uint256 amount,
         address targetToken,
         bytes memory recipient,
         bool withdrawFlag
-    ) public {
+    ) public nonReentrant {
📝 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
function swap(
address inputToken,
uint256 amount,
address targetToken,
bytes memory recipient,
bool withdraw
) internal {
bool withdrawFlag
) public {
bool success = IZRC20(inputToken).transferFrom(
msg.sender,
address(this),
amount
);
if (!success) {
revert TransferFailed();
}
(uint256 out, address gasZRC20, uint256 gasFee) = handleGasAndSwap(
inputToken,
amount,
targetToken
);
withdraw(
Params({
target: targetToken,
to: recipient,
withdraw: withdrawFlag
}),
msg.sender,
gasFee,
gasZRC20,
out,
inputToken
);
}
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
contract SwapToAnyToken is UniversalContract, ReentrancyGuard {
address public immutable uniswapRouter;
GatewayZEVM public gateway;
uint256 constant BITCOIN = 18332;
uint256 public immutable gasLimit;
function swap(
address inputToken,
uint256 amount,
address targetToken,
bytes memory recipient,
bool withdrawFlag
) public nonReentrant {
bool success = IZRC20(inputToken).transferFrom(
msg.sender,
address(this),
amount
);
if (!success) {
revert TransferFailed();
}
(uint256 out, address gasZRC20, uint256 gasFee) = handleGasAndSwap(
inputToken,
amount,
targetToken
);
withdraw(
Params({
target: targetToken,
to: recipient,
withdraw: withdrawFlag
}),
msg.sender,
gasFee,
gasZRC20,
out,
inputToken
);
}


function handleGasAndSwap(
address inputToken,
uint256 amount,
address targetToken
) internal returns (uint256, address, uint256) {
uint256 inputForGas;
address gasZRC20;
uint256 gasFee;
uint256 swapAmount = amount;
uint256 swapAmount;

if (withdraw) {
(gasZRC20, gasFee) = IZRC20(targetToken).withdrawGasFee();
(gasZRC20, gasFee) = IZRC20(targetToken).withdrawGasFee();

if (gasZRC20 == inputToken) {
swapAmount = amount - gasFee;
} else {
inputForGas = SwapHelperLib.swapTokensForExactTokens(
uniswapRouter,
inputToken,
gasFee,
gasZRC20,
amount
);
swapAmount = amount - inputForGas;
}
if (gasZRC20 == inputToken) {
swapAmount = amount - gasFee;
} else {
inputForGas = SwapHelperLib.swapTokensForExactTokens(
uniswapRouter,
inputToken,
gasFee,
gasZRC20,
amount
);
swapAmount = amount - inputForGas;
}

uint256 outputAmount = SwapHelperLib.swapExactTokensForTokens(
uint256 out = SwapHelperLib.swapExactTokensForTokens(
uniswapRouter,
inputToken,
swapAmount,
targetToken,
0
);
return (out, gasZRC20, gasFee);
}
Comment on lines +132 to +157
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure swapAmount is validated to be greater than zero

The swapAmount might be zero if gasFee equals amount or inputForGas equals amount, leading to potential issues when performing swaps with zero amount. It's prudent to add a check to ensure that swapAmount > 0 before proceeding.

Implement the following check:

     if (gasZRC20 == inputToken) {
         swapAmount = amount - gasFee;
+        require(swapAmount > 0, "Swap amount must be greater than zero");
     } else {
         inputForGas = SwapHelperLib.swapTokensForExactTokens(
             uniswapRouter,
             inputToken,
             gasFee,
             gasZRC20,
             amount
         );
         swapAmount = amount - inputForGas;
+        require(swapAmount > 0, "Swap amount must be greater than zero");
     }
📝 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
uint256 swapAmount;
if (withdraw) {
(gasZRC20, gasFee) = IZRC20(targetToken).withdrawGasFee();
(gasZRC20, gasFee) = IZRC20(targetToken).withdrawGasFee();
if (gasZRC20 == inputToken) {
swapAmount = amount - gasFee;
} else {
inputForGas = SwapHelperLib.swapTokensForExactTokens(
uniswapRouter,
inputToken,
gasFee,
gasZRC20,
amount
);
swapAmount = amount - inputForGas;
}
if (gasZRC20 == inputToken) {
swapAmount = amount - gasFee;
} else {
inputForGas = SwapHelperLib.swapTokensForExactTokens(
uniswapRouter,
inputToken,
gasFee,
gasZRC20,
amount
);
swapAmount = amount - inputForGas;
}
uint256 outputAmount = SwapHelperLib.swapExactTokensForTokens(
uint256 out = SwapHelperLib.swapExactTokensForTokens(
uniswapRouter,
inputToken,
swapAmount,
targetToken,
0
);
return (out, gasZRC20, gasFee);
}
uint256 swapAmount;
(gasZRC20, gasFee) = IZRC20(targetToken).withdrawGasFee();
if (gasZRC20 == inputToken) {
swapAmount = amount - gasFee;
require(swapAmount > 0, "Swap amount must be greater than zero");
} else {
inputForGas = SwapHelperLib.swapTokensForExactTokens(
uniswapRouter,
inputToken,
gasFee,
gasZRC20,
amount
);
swapAmount = amount - inputForGas;
require(swapAmount > 0, "Swap amount must be greater than zero");
}
uint256 out = SwapHelperLib.swapExactTokensForTokens(
uniswapRouter,
inputToken,
swapAmount,
targetToken,
0
);
return (out, gasZRC20, gasFee);
}


if (withdraw) {
if (gasZRC20 == targetToken) {
IZRC20(gasZRC20).approve(
address(gateway),
outputAmount + gasFee
);
function withdraw(
Params memory params,
address sender,
uint256 gasFee,
address gasZRC20,
uint256 out,
address inputToken
) public {
if (params.withdraw) {
if (gasZRC20 == params.target) {
if (!IZRC20(gasZRC20).approve(address(gateway), out + gasFee)) {
revert ApprovalFailed();
}
} else {
IZRC20(gasZRC20).approve(address(gateway), gasFee);
IZRC20(targetToken).approve(address(gateway), outputAmount);
if (!IZRC20(gasZRC20).approve(address(gateway), gasFee)) {
revert ApprovalFailed();
}
if (!IZRC20(params.target).approve(address(gateway), out)) {
revert ApprovalFailed();
}
}
gateway.withdraw(
recipient,
outputAmount,
targetToken,
abi.encodePacked(params.to),
out,
params.target,
RevertOptions({
revertAddress: address(0),
callOnRevert: false,
revertAddress: address(this),
callOnRevert: true,
abortAddress: address(0),
revertMessage: "",
onRevertGasLimit: 0
revertMessage: abi.encode(sender, inputToken),
onRevertGasLimit: gasLimit
})
);
} else {
IWETH9(targetToken).transfer(
address(uint160(bytes20(recipient))),
outputAmount
bool success = IWETH9(params.target).transfer(
address(uint160(bytes20(params.to))),
out
);
if (!success) {
revert TransferFailed();
}
Comment on lines +193 to +199
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use IZRC20 interface instead of IWETH9 for token transfers

In the withdraw function, when params.withdraw is false, the contract attempts to transfer tokens using the IWETH9 interface:

bool success = IWETH9(params.target).transfer(
    address(uint160(bytes20(params.to))),
    out
);

Unless params.target is guaranteed to be an IWETH9 compliant token, this may result in unexpected behavior. It is recommended to use the IZRC20 interface for standard token transfers to ensure compatibility with general ERC20 tokens.

Apply the following change:

-bool success = IWETH9(params.target).transfer(
+bool success = IZRC20(params.target).transfer(
    address(uint160(bytes20(params.to))),
    out
);

}
}

function swap(
address inputToken,
uint256 amount,
address targetToken,
bytes memory recipient,
bool withdraw
) public {
IZRC20(inputToken).transferFrom(msg.sender, address(this), amount);
function onRevert(RevertContext calldata context) external onlyGateway {
(address sender, address zrc20) = abi.decode(
context.revertMessage,
(address, address)
);
(uint256 out, , ) = handleGasAndSwap(
context.asset,
context.amount,
zrc20
);

swapAndWithdraw(inputToken, amount, targetToken, recipient, withdraw);
gateway.withdraw(
abi.encodePacked(sender),
out,
zrc20,
RevertOptions({
revertAddress: sender,
callOnRevert: false,
abortAddress: address(0),
revertMessage: "",
onRevertGasLimit: gasLimit
})
);
}

function onRevert(
RevertContext calldata revertContext
) external onlyGateway {}
fallback() external payable {}

receive() external payable {}
Comment on lines +228 to +230
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure fallback and receive functions revert as per PR objectives

According to the PR objectives, the fallback and receive functions should revert when called to handle situations where the withdraw function may fail on the destination chain. The current implementation adds empty functions that do not revert. To align with the intended functionality, these functions should be modified to revert upon invocation.

Apply the following changes:

-fallback() external payable {}
+fallback() external payable {
+    revert("");
+}

-receive() external payable {}
+receive() external payable {
+    revert("");
+}
📝 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
fallback() external payable {}
receive() external payable {}
fallback() external payable {
revert("");
}
receive() external payable {
revert("");
}

}
Loading