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.

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

feat: erc20 auth calls #401

Merged
merged 14 commits into from
Oct 18, 2024
Merged

feat: erc20 auth calls #401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0b7df97
disable ZETA functions and remove legacy call functions
lumtis Oct 15, 2024
0572bce
first test changes
lumtis Oct 15, 2024
203d624
fix tests
lumtis Oct 15, 2024
11a1f78
fix warning
lumtis Oct 15, 2024
7e13d44
fix test
lumtis Oct 16, 2024
40c5aaa
generate
lumtis Oct 16, 2024
86bca2c
erc20 auth calls wip
skosito Oct 16, 2024
17f952d
generate
skosito Oct 16, 2024
ce5c0e7
Merge branch 'main' into erc20-auth-calls
skosito Oct 16, 2024
c70fec4
generate
skosito Oct 16, 2024
4c3be31
unit tests
skosito Oct 17, 2024
d163373
PR comment
skosito Oct 17, 2024
f6c9514
Merge branch 'main' into erc20-auth-calls
skosito Oct 17, 2024
f98eb2c
yarn generate
skosito Oct 17, 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
6 changes: 4 additions & 2 deletions v2/contracts/evm/ERC20Custody.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
pragma solidity 0.8.26;

import { IERC20Custody } from "./interfaces/IERC20Custody.sol";
import { IGatewayEVM } from "./interfaces/IGatewayEVM.sol";
import { IGatewayEVM, MessageContext } from "./interfaces/IGatewayEVM.sol";

import { RevertContext } from "../../contracts/Revert.sol";

Expand Down Expand Up @@ -140,11 +140,13 @@ contract ERC20Custody is

/// @notice WithdrawAndCall transfers tokens to Gateway and call a contract through the Gateway.
/// @dev This function can only be called by the TSS address.
/// @param messageContext Message context containing sender.
/// @param to Address of the contract to call.
/// @param token Address of the ERC20 token.
/// @param amount Amount of tokens to withdraw.
/// @param data Calldata to pass to the contract call.
function withdrawAndCall(
MessageContext calldata messageContext,
address to,
address token,
uint256 amount,
Expand All @@ -161,7 +163,7 @@ contract ERC20Custody is
IERC20(token).safeTransfer(address(gateway), amount);

// Forward the call to the Gateway contract
gateway.executeWithERC20(token, to, amount, data);
gateway.executeWithERC20(messageContext, token, to, amount, data);

emit WithdrawnAndCalled(to, token, amount, data);
}
Expand Down
44 changes: 18 additions & 26 deletions v2/contracts/evm/GatewayEVM.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ contract GatewayEVM is
emit Reverted(destination, address(0), msg.value, data, revertContext);
}

/// @notice Executes an authenticated call to a destination address without ERC20 tokens.
/// @notice Executes a call to a destination address without ERC20 tokens.
/// @dev This function can only be called by the TSS address and it is payable.
/// @param messageContext Message context containing sender.
/// @param destination Address to call.
Expand All @@ -142,30 +142,14 @@ contract GatewayEVM is
{
if (destination == address(0)) revert ZeroAddress();
bytes memory result;
result = _executeAuthenticatedCall(messageContext, destination, data);

emit Executed(destination, msg.value, data);

return result;
}

/// @notice Executes an arbitrary call to a destination address without ERC20 tokens.
/// @dev This function can only be called by the TSS address and it is payable.
/// @param destination Address to call.
/// @param data Calldata to pass to the call.
/// @return The result of the call.
function execute(
address destination,
bytes calldata data
)
external
payable
onlyRole(TSS_ROLE)
whenNotPaused
returns (bytes memory)
{
if (destination == address(0)) revert ZeroAddress();
bytes memory result = _executeArbitraryCall(destination, data);
// Execute the call on the target contract
// if sender is provided in messageContext call is authenticated and target is Callable.onCall
// otherwise, call is arbitrary
if (messageContext.sender == address(0)) {
skosito marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this can cause problems to developer experience if not properly documented what it means to fill or not the sender.

Copy link
Contributor Author

@skosito skosito Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this part is called by protocol, but i see your point, i think currently comment explain what its doing: sender = authenticated call, no sender = arbitrary call

when devs use this feature, they would set up callOptions.isArbitrary field on zevm side, so there is more important from devex side of things

result = _executeArbitraryCall(destination, data);
} else {
result = _executeAuthenticatedCall(messageContext, destination, data);
}
Comment on lines +148 to +152
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

Consider using an explicit flag to indicate call type instead of checking messageContext.sender

Currently, the code distinguishes between arbitrary and authenticated calls by checking if messageContext.sender == address(0). While this approach works, it relies on a special value for the sender address, which can be less readable and might lead to errors if not handled carefully.

Consider introducing an explicit boolean flag in MessageContext, such as isAuthenticated, to make the distinction clearer and improve code readability:

struct MessageContext {
    address sender;
    bool isAuthenticated;
}

Then update the condition:

-if (messageContext.sender == address(0)) {
+if (!messageContext.isAuthenticated) {
    result = _executeArbitraryCall(destination, data);
} else {
    result = _executeAuthenticatedCall(messageContext, destination, data);
}

Comment on lines +145 to +152
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

Consider refactoring duplicated conditional logic into a private function

The conditional logic that checks messageContext.sender == address(0) to decide between _executeArbitraryCall and _executeAuthenticatedCall is duplicated in both the execute and executeWithERC20 functions. Extracting this logic into a private helper function can improve maintainability and reduce code duplication.


emit Executed(destination, msg.value, data);

Expand All @@ -175,11 +159,13 @@ contract GatewayEVM is
/// @notice Executes a call to a destination contract using ERC20 tokens.
/// @dev This function can only be called by the custody or connector address.
/// It uses the ERC20 allowance system, resetting gateway allowance at the end.
/// @param messageContext Message context containing sender.
/// @param token Address of the ERC20 token.
/// @param to Address of the contract to call.
/// @param amount Amount of tokens to transfer.
/// @param data Calldata to pass to the call.
function executeWithERC20(
MessageContext calldata messageContext,
address token,
address to,
uint256 amount,
Expand All @@ -196,7 +182,13 @@ contract GatewayEVM is
if (!_resetApproval(token, to)) revert ApprovalFailed();
if (!IERC20(token).approve(to, amount)) revert ApprovalFailed();
// Execute the call on the target contract
_executeArbitraryCall(to, data);
// if sender is provided in messageContext call is authenticated and target is Callable.onCall
// otherwise, call is arbitrary
if (messageContext.sender == address(0)) {
_executeArbitraryCall(to, data);
} else {
_executeAuthenticatedCall(messageContext, to, data);
}
Comment on lines +187 to +191
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

Return value of executeWithERC20 is not being captured

The return value of executeWithERC20 is not captured or returned in any of its calls. To ensure proper logging, error handling, or further processing, consider capturing the execution result as done in the execute function.

🔗 Analysis chain

Verify if the execution result in executeWithERC20 should be captured

In the execute function, the result of the call is captured and returned to the caller. However, in executeWithERC20, the result of _executeArbitraryCall or _executeAuthenticatedCall is neither captured nor returned. If the execution result is important for logging, error handling, or further processing, consider capturing and handling it similarly in executeWithERC20.

You can run the following script to check if any usage or overrides of executeWithERC20 expect a return value:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for usages of `executeWithERC20` and check if the return value is utilized.

# Find all Solidity files that call `executeWithERC20`
rg --type solidity 'executeWithERC20\('

# Check if any derived contracts override `executeWithERC20` with a return value
rg --type solidity 'function executeWithERC20\('

Length of output: 158

Script:

#!/bin/bash
# Description: Search for usages of `executeWithERC20` and check if the return value is utilized within Solidity files.

# Find all Solidity files that call `executeWithERC20`
rg 'executeWithERC20\(' --glob '*.sol'

# Check if any derived contracts override `executeWithERC20` with a return value
rg 'function executeWithERC20\(' --glob '*.sol'

Length of output: 1554

Script:

#!/bin/bash
# Description: Check if the return values of `executeWithERC20` are captured or utilized within Solidity files.

# Search for `executeWithERC20` calls that are part of an assignment
rg '=\s*executeWithERC20\(' --glob '*.sol'

# Search for `executeWithERC20` calls that are returned directly
rg 'return\s+executeWithERC20\(' --glob '*.sol'

# Search for `executeWithERC20` calls used in expressions (e.g., inside require or assert)
rg '(require|assert)\s*\(\s*executeWithERC20\(' --glob '*.sol'

Length of output: 159


// Reset approval
if (!_resetApproval(token, to)) revert ApprovalFailed();
Expand Down
9 changes: 8 additions & 1 deletion v2/contracts/evm/ZetaConnectorBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

import { RevertContext } from "../../contracts/Revert.sol";
import { IGatewayEVM, IGatewayEVMErrors, IGatewayEVMEvents } from "../../contracts/evm/interfaces/IGatewayEVM.sol";
import {
IGatewayEVM,
IGatewayEVMErrors,
IGatewayEVMEvents,
MessageContext
} from "../../contracts/evm/interfaces/IGatewayEVM.sol";
import "../../contracts/evm/interfaces/IZetaConnector.sol";

/// @title ZetaConnectorBase
Expand Down Expand Up @@ -112,11 +117,13 @@ abstract contract ZetaConnectorBase is
function withdraw(address to, uint256 amount, bytes32 internalSendHash) external virtual;

/// @notice Withdraw tokens and call a contract through Gateway.
/// @param messageContext Message context containing sender.
/// @param to The address to withdraw tokens to.
/// @param amount The amount of tokens to withdraw.
/// @param data The calldata to pass to the contract call.
/// @param internalSendHash A hash used for internal tracking of the transaction.
function withdrawAndCall(
MessageContext calldata messageContext,
address to,
uint256 amount,
bytes calldata data,
Expand Down
4 changes: 3 additions & 1 deletion v2/contracts/evm/ZetaConnectorNative.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,15 @@ contract ZetaConnectorNative is ZetaConnectorBase {
}

/// @notice Withdraw tokens and call a contract through Gateway.
/// @param messageContext Message context containing sender.
/// @param to The address to withdraw tokens to.
/// @param amount The amount of tokens to withdraw.
/// @param data The calldata to pass to the contract call.
//// @param internalSendHash A hash used for internal tracking of the transaction (not used currently
// https://github.com/zeta-chain/protocol-contracts/issues/398)
/// @dev This function can only be called by the TSS address.
function withdrawAndCall(
MessageContext calldata messageContext,
address to,
uint256 amount,
bytes calldata data,
Expand All @@ -68,7 +70,7 @@ contract ZetaConnectorNative is ZetaConnectorBase {
IERC20(zetaToken).safeTransfer(address(gateway), amount);

// Forward the call to the Gateway contract
gateway.executeWithERC20(address(zetaToken), to, amount, data);
gateway.executeWithERC20(messageContext, address(zetaToken), to, amount, data);

emit WithdrawnAndCalled(to, amount, data);
}
Expand Down
4 changes: 3 additions & 1 deletion v2/contracts/evm/ZetaConnectorNonNative.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,14 @@ contract ZetaConnectorNonNative is ZetaConnectorBase {
}

/// @notice Withdraw tokens and call a contract through Gateway.
/// @param messageContext Message context containing sender.
/// @param to The address to withdraw tokens to.
/// @param amount The amount of tokens to withdraw.
/// @param data The calldata to pass to the contract call.
/// @param internalSendHash A hash used for internal tracking of the transaction.
/// @dev This function can only be called by the TSS address, and mints if supply is not reached.
function withdrawAndCall(
MessageContext calldata messageContext,
address to,
uint256 amount,
bytes calldata data,
Expand All @@ -82,7 +84,7 @@ contract ZetaConnectorNonNative is ZetaConnectorBase {
_mintTo(address(gateway), amount, internalSendHash);

// Forward the call to the Gateway contract
gateway.executeWithERC20(address(zetaToken), to, amount, data);
gateway.executeWithERC20(messageContext, address(zetaToken), to, amount, data);

emit WithdrawnAndCalled(to, amount, data);
}
Expand Down
12 changes: 11 additions & 1 deletion v2/contracts/evm/interfaces/IERC20Custody.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
pragma solidity 0.8.26;

import { RevertContext } from "../../../contracts/Revert.sol";

import { MessageContext } from "./IGatewayEVM.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

/// @title IERC20CustodyEvents
Expand Down Expand Up @@ -71,11 +73,19 @@ interface IERC20Custody is IERC20CustodyEvents, IERC20CustodyErrors {

/// @notice WithdrawAndCall transfers tokens to Gateway and call a contract through the Gateway.
/// @dev This function can only be called by the TSS address.
/// @param messageContext Message context containing sender.
/// @param token Address of the ERC20 token.
/// @param to Address of the contract to call.
/// @param amount Amount of tokens to withdraw.
/// @param data Calldata to pass to the contract call.
function withdrawAndCall(address token, address to, uint256 amount, bytes calldata data) external;
function withdrawAndCall(
MessageContext calldata messageContext,
address token,
address to,
uint256 amount,
bytes calldata data
)
external;

/// @notice WithdrawAndRevert transfers tokens to Gateway and call a contract with a revert functionality through
/// the Gateway.
Expand Down
16 changes: 9 additions & 7 deletions v2/contracts/evm/interfaces/IGatewayEVM.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,19 @@ interface IGatewayEVMErrors {
/// @notice Interface for the GatewayEVM contract.
interface IGatewayEVM is IGatewayEVMErrors, IGatewayEVMEvents {
/// @notice Executes a call to a contract using ERC20 tokens.
/// @param messageContext Message context containing sender and arbitrary call flag.
/// @param token The address of the ERC20 token.
/// @param to The address of the contract to call.
/// @param amount The amount of tokens to transfer.
/// @param data The calldata to pass to the contract call.
Comment on lines +103 to 107
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

Update Documentation to Match MessageContext Structure

The documentation for the messageContext parameter mentions that it contains "sender and arbitrary call flag," but the MessageContext struct only includes address sender;. Please update the documentation to accurately reflect the actual structure of MessageContext or include the "arbitrary call flag" in the struct if necessary.

If the "arbitrary call flag" is not required, apply this diff to update the documentation:

 /// @param messageContext Message context containing sender and arbitrary call flag.
+/// @param messageContext Message context containing the sender address.
📝 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
/// @param messageContext Message context containing sender and arbitrary call flag.
/// @param token The address of the ERC20 token.
/// @param to The address of the contract to call.
/// @param amount The amount of tokens to transfer.
/// @param data The calldata to pass to the contract call.
/// @param messageContext Message context containing the sender address.
/// @param token The address of the ERC20 token.
/// @param to The address of the contract to call.
/// @param amount The amount of tokens to transfer.
/// @param data The calldata to pass to the contract call.

function executeWithERC20(address token, address to, uint256 amount, bytes calldata data) external;
function executeWithERC20(
MessageContext calldata messageContext,
address token,
address to,
uint256 amount,
bytes calldata data
)
external;

/// @notice Transfers msg.value to destination contract and executes it's onRevert function.
/// @dev This function can only be called by the TSS address and it is payable.
Expand All @@ -119,12 +127,6 @@ interface IGatewayEVM is IGatewayEVMErrors, IGatewayEVMEvents {
external
payable;

/// @notice Executes a call to a contract.
/// @param destination The address of the contract to call.
/// @param data The calldata to pass to the contract call.
/// @return The result of the contract call.
function execute(address destination, bytes calldata data) external payable returns (bytes memory);

/// @notice Executes a call to a destination address without ERC20 tokens.
/// @dev This function can only be called by the TSS address and it is payable.
/// @param messageContext Message context containing sender and arbitrary call flag.
Expand Down
5 changes: 2 additions & 3 deletions v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ WithdrawAndCall transfers tokens to Gateway and call a contract through the Gate

```solidity
function withdrawAndCall(
MessageContext calldata messageContext,
address to,
address token,
uint256 amount,
Expand All @@ -222,6 +223,7 @@ function withdrawAndCall(

|Name|Type|Description|
|----|----|-----------|
|`messageContext`|`MessageContext`|Message context containing sender.|
|`to`|`address`|Address of the contract to call.|
|`token`|`address`|Address of the ERC20 token.|
|`amount`|`uint256`|Amount of tokens to withdraw.|
Expand Down Expand Up @@ -264,9 +266,6 @@ function withdrawAndRevert(

Deposits asset to custody and pay fee in zeta erc20.

**Note:**
This method is deprecated.


```solidity
function deposit(
Expand Down
39 changes: 3 additions & 36 deletions v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,6 @@ uint256 public constant MAX_PAYLOAD_SIZE = 1024;
## Functions
### constructor

**Note:**
constructor


```solidity
constructor();
Expand Down Expand Up @@ -182,7 +179,7 @@ function executeRevert(

### execute

Executes an authenticated call to a destination address without ERC20 tokens.
Executes a call to a destination address without ERC20 tokens.

*This function can only be called by the TSS address and it is payable.*

Expand Down Expand Up @@ -215,38 +212,6 @@ function execute(
|`<none>`|`bytes`|The result of the call.|


### execute

Executes an arbitrary call to a destination address without ERC20 tokens.

*This function can only be called by the TSS address and it is payable.*


```solidity
function execute(
address destination,
bytes calldata data
)
external
payable
onlyRole(TSS_ROLE)
whenNotPaused
returns (bytes memory);
```
**Parameters**

|Name|Type|Description|
|----|----|-----------|
|`destination`|`address`|Address to call.|
|`data`|`bytes`|Calldata to pass to the call.|

**Returns**

|Name|Type|Description|
|----|----|-----------|
|`<none>`|`bytes`|The result of the call.|


### executeWithERC20

Executes a call to a destination contract using ERC20 tokens.
Expand All @@ -257,6 +222,7 @@ It uses the ERC20 allowance system, resetting gateway allowance at the end.*

```solidity
function executeWithERC20(
MessageContext calldata messageContext,
address token,
address to,
uint256 amount,
Expand All @@ -271,6 +237,7 @@ function executeWithERC20(

|Name|Type|Description|
|----|----|-----------|
|`messageContext`|`MessageContext`|Message context containing sender.|
|`token`|`address`|Address of the ERC20 token.|
|`to`|`address`|Address of the contract to call.|
|`amount`|`uint256`|Amount of tokens to transfer.|
Expand Down
11 changes: 10 additions & 1 deletion v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,21 @@ Withdraw tokens and call a contract through Gateway.


```solidity
function withdrawAndCall(address to, uint256 amount, bytes calldata data, bytes32 internalSendHash) external virtual;
function withdrawAndCall(
MessageContext calldata messageContext,
address to,
uint256 amount,
bytes calldata data,
bytes32 internalSendHash
)
external
virtual;
```
**Parameters**

|Name|Type|Description|
|----|----|-----------|
|`messageContext`|`MessageContext`|Message context containing sender.|
|`to`|`address`|The address to withdraw tokens to.|
|`amount`|`uint256`|The amount of tokens to withdraw.|
|`data`|`bytes`|The calldata to pass to the contract call.|
Expand Down
2 changes: 2 additions & 0 deletions v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ Withdraw tokens and call a contract through Gateway.

```solidity
function withdrawAndCall(
MessageContext calldata messageContext,
address to,
uint256 amount,
bytes calldata data,
Expand All @@ -77,6 +78,7 @@ function withdrawAndCall(

|Name|Type|Description|
|----|----|-----------|
|`messageContext`|`MessageContext`|Message context containing sender.|
|`to`|`address`|The address to withdraw tokens to.|
|`amount`|`uint256`|The amount of tokens to withdraw.|
|`data`|`bytes`|The calldata to pass to the contract call.|
Expand Down
Loading
Loading