-
Notifications
You must be signed in to change notification settings - Fork 63
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
feat: v2 contracts access control #247
Changes from 8 commits
5b5cc4b
f1cf789
8b9669e
1851b89
ba0f25f
3f4990c
a686f66
58160e8
0f8ab79
9c324a0
3cc2443
57f1c6c
77f2bf0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,44 +2,47 @@ | |||||
pragma solidity ^0.8.0; | ||||||
|
||||||
import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | ||||||
import "./IGatewayEVM.sol"; | ||||||
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; | ||||||
import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; | ||||||
|
||||||
import "./IGatewayEVM.sol"; | ||||||
import "./IERC20CustodyNew.sol"; | ||||||
|
||||||
// As the current version, ERC20CustodyNew hold the ERC20s deposited on ZetaChain | ||||||
// This version include a functionality allowing to call a contract | ||||||
// ERC20Custody doesn't call smart contract directly, it passes through the Gateway contract | ||||||
contract ERC20CustodyNew is ReentrancyGuard{ | ||||||
contract ERC20CustodyNew is IERC20CustodyNewEvents, IERC20CustodyNewErrors, ReentrancyGuard { | ||||||
using SafeERC20 for IERC20; | ||||||
error ZeroAddress(); | ||||||
|
||||||
IGatewayEVM public gateway; | ||||||
address public tssAddress; | ||||||
Check warning Code scanning / Slither State variables that could be declared immutable Warning
ERC20CustodyNew.tssAddress should be immutable
fbac marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
event Withdraw(address indexed token, address indexed to, uint256 amount); | ||||||
event WithdrawAndCall(address indexed token, address indexed to, uint256 amount, bytes data); | ||||||
event WithdrawAndRevert(address indexed token, address indexed to, uint256 amount, bytes data); | ||||||
// @dev Only TSS address allowed modifier. | ||||||
modifier onlyTSS() { | ||||||
if (msg.sender != tssAddress) { | ||||||
revert InvalidSender(); | ||||||
} | ||||||
_; | ||||||
} | ||||||
|
||||||
constructor(address _gateway) { | ||||||
if (_gateway == address(0)) { | ||||||
constructor(address _gateway, address _tssAddress) { | ||||||
if (_gateway == address(0) || _tssAddress == address(0)) { | ||||||
revert ZeroAddress(); | ||||||
} | ||||||
gateway = IGatewayEVM(_gateway); | ||||||
tssAddress = _tssAddress; | ||||||
} | ||||||
|
||||||
// Withdraw is called by TSS address, it directly transfers the tokens to the destination address without contract call | ||||||
// TODO: Finalize access control | ||||||
// https://github.com/zeta-chain/protocol-contracts/issues/204 | ||||||
function withdraw(address token, address to, uint256 amount) external nonReentrant { | ||||||
function withdraw(address token, address to, uint256 amount) external nonReentrant onlyTSS { | ||||||
IERC20(token).safeTransfer(to, amount); | ||||||
|
||||||
emit Withdraw(token, to, amount); | ||||||
} | ||||||
|
||||||
// WithdrawAndCall is called by TSS address, it transfers the tokens and call a contract | ||||||
// For this, it passes through the Gateway contract, it transfers the tokens to the Gateway contract and then calls the contract | ||||||
// TODO: Finalize access control | ||||||
// https://github.com/zeta-chain/protocol-contracts/issues/204 | ||||||
function withdrawAndCall(address token, address to, uint256 amount, bytes calldata data) public nonReentrant { | ||||||
function withdrawAndCall(address token, address to, uint256 amount, bytes calldata data) public nonReentrant onlyTSS { | ||||||
// Transfer the tokens to the Gateway contract | ||||||
IERC20(token).safeTransfer(address(gateway), amount); | ||||||
|
||||||
|
@@ -51,9 +54,7 @@ | |||||
|
||||||
// WithdrawAndRevert is called by TSS address, it transfers the tokens and call a contract | ||||||
// For this, it passes through the Gateway contract, it transfers the tokens to the Gateway contract and then calls the contract | ||||||
// TODO: Finalize access control | ||||||
// https://github.com/zeta-chain/protocol-contracts/issues/204 | ||||||
function withdrawAndRevert(address token, address to, uint256 amount, bytes calldata data) public nonReentrant { | ||||||
function withdrawAndRevert(address token, address to, uint256 amount, bytes calldata data) public nonReentrant onlyTSS { | ||||||
// Transfer the tokens to the Gateway contract | ||||||
IERC20(token).safeTransfer(address(gateway), amount); | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -27,6 +27,22 @@ | |||||||
/// @notice The address of the Zeta token contract. | ||||||||
address public zetaToken; | ||||||||
|
||||||||
// @dev Only TSS address allowed modifier. | ||||||||
modifier onlyTSS() { | ||||||||
if (msg.sender != tssAddress) { | ||||||||
revert InvalidSender(); | ||||||||
} | ||||||||
_; | ||||||||
} | ||||||||
|
||||||||
// @dev Only custody address allowed modifier. | ||||||||
skosito marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
modifier onlyCustodyOrConnector() { | ||||||||
skosito marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
if (msg.sender != custody && msg.sender != zetaConnector) { | ||||||||
revert InvalidSender(); | ||||||||
} | ||||||||
_; | ||||||||
} | ||||||||
|
||||||||
/// @custom:oz-upgrades-unsafe-allow constructor | ||||||||
constructor() { | ||||||||
_disableInitializers(); | ||||||||
|
@@ -56,24 +72,24 @@ | |||||||
|
||||||||
// Called by the TSS | ||||||||
// Calling onRevert directly | ||||||||
function executeRevert(address destination, bytes calldata data) public payable { | ||||||||
function executeRevert(address destination, bytes calldata data) public payable onlyTSS { | ||||||||
Check notice Code scanning / Slither Missing zero address validation Low
GatewayEVM.executeRevert(address,bytes).destination lacks a zero-check on :
- (success,result) = destination.call{value: msg.value}() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add zero address validation for The + if (destination == address(0)) revert ZeroAddress(); Committable suggestion
Suggested change
ToolsGitHub Check: Slither
|
||||||||
(bool success, bytes memory result) = destination.call{value: msg.value}(""); | ||||||||
if (!success) revert ExecutionFailed(); | ||||||||
Revertable(destination).onRevert(data); | ||||||||
|
||||||||
emit Reverted(destination, msg.value, data); | ||||||||
} | ||||||||
Check notice Code scanning / Slither Reentrancy vulnerabilities Low
Reentrancy in GatewayEVM.executeRevert(address,bytes):
External calls: - (success,result) = destination.call{value: msg.value}() - Revertable(destination).onRevert(data) External calls sending eth: - (success,result) = destination.call{value: msg.value}() Event emitted after the call(s): - Reverted(destination,msg.value,data) Check warning Code scanning / Slither Low-level calls Warning |
||||||||
|
||||||||
// Called by the TSS | ||||||||
// Execution without ERC20 tokens, it is payable and can be used in the case of WithdrawAndCall for Gas ZRC20 | ||||||||
// It can be also used for contract call without asset movement | ||||||||
function execute(address destination, bytes calldata data) external payable returns (bytes memory) { | ||||||||
function execute(address destination, bytes calldata data) external payable onlyTSS returns (bytes memory) { | ||||||||
bytes memory result = _execute(destination, data); | ||||||||
|
||||||||
emit Executed(destination, msg.value, data); | ||||||||
|
||||||||
return result; | ||||||||
} | ||||||||
Check notice Code scanning / Slither Reentrancy vulnerabilities Low
Reentrancy in GatewayEVM.execute(address,bytes):
External calls: - result = _execute(destination,data) - (success,result) = destination.call{value: msg.value}(data) Event emitted after the call(s): - Executed(destination,msg.value,data) |
||||||||
|
||||||||
// Called by the ERC20Custody contract | ||||||||
// It call a function using ERC20 transfer | ||||||||
|
@@ -84,7 +100,7 @@ | |||||||
address to, | ||||||||
uint256 amount, | ||||||||
bytes calldata data | ||||||||
) public nonReentrant { | ||||||||
) public nonReentrant onlyCustodyOrConnector { | ||||||||
if (amount == 0) revert InsufficientERC20Amount(); | ||||||||
// Approve the target contract to spend the tokens | ||||||||
if(!resetApproval(token, to)) revert ApprovalFailed(); | ||||||||
|
@@ -111,7 +127,7 @@ | |||||||
address to, | ||||||||
uint256 amount, | ||||||||
bytes calldata data | ||||||||
) external nonReentrant { | ||||||||
) external nonReentrant onlyCustodyOrConnector { | ||||||||
if (amount == 0) revert InsufficientERC20Amount(); | ||||||||
|
||||||||
IERC20(token).safeTransfer(address(to), amount); | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.0; | ||
|
||
interface IERC20CustodyNewEvents { | ||
event Withdraw(address indexed token, address indexed to, uint256 amount); | ||
event WithdrawAndCall(address indexed token, address indexed to, uint256 amount, bytes data); | ||
event WithdrawAndRevert(address indexed token, address indexed to, uint256 amount, bytes data); | ||
} | ||
|
||
interface IERC20CustodyNewErrors { | ||
error ZeroAddress(); | ||
error InvalidSender(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.0; | ||
|
||
interface IZetaConnectorEvents { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion the interface shouldn't be specific for events or errors. My suggestion here and in all the interface files would be grouping events, errors and functions under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree, one use case for having separate is if we want to import only events and errors in situations like tests, it is nice to be more granular if we are using interfaces maybe dont have errors and events in interface then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will follow up on this one, good point |
||
event Withdraw(address indexed to, uint256 amount); | ||
event WithdrawAndCall(address indexed to, uint256 amount, bytes data); | ||
event WithdrawAndRevert(address indexed to, uint256 amount, bytes data); | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,28 +2,37 @@ | |||||||||||||
pragma solidity 0.8.7; | ||||||||||||||
|
||||||||||||||
import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | ||||||||||||||
import "./IGatewayEVM.sol"; | ||||||||||||||
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; | ||||||||||||||
import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; | ||||||||||||||
|
||||||||||||||
abstract contract ZetaConnectorNewBase is ReentrancyGuard { | ||||||||||||||
import "./IGatewayEVM.sol"; | ||||||||||||||
import "./IZetaConnector.sol"; | ||||||||||||||
|
||||||||||||||
abstract contract ZetaConnectorNewBase is IZetaConnectorEvents, ReentrancyGuard { | ||||||||||||||
using SafeERC20 for IERC20; | ||||||||||||||
|
||||||||||||||
error ZeroAddress(); | ||||||||||||||
error InvalidSender(); | ||||||||||||||
|
||||||||||||||
IGatewayEVM public immutable gateway; | ||||||||||||||
address public immutable zetaToken; | ||||||||||||||
address public tssAddress; | ||||||||||||||
Check warning Code scanning / Slither State variables that could be declared immutable Warning
ZetaConnectorNewBase.tssAddress should be immutable
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider marking The state variable - address public tssAddress;
+ address public immutable tssAddress; Committable suggestion
Suggested change
ToolsGitHub Check: Slither
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As analyzed by slither:
Suggested change
tssAddress shouldn't change after setting it at deployment time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets see if we will need a way to update it with tssUpdater There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will follow up here: #255 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
|
||||||||||||||
event Withdraw(address indexed to, uint256 amount); | ||||||||||||||
event WithdrawAndCall(address indexed to, uint256 amount, bytes data); | ||||||||||||||
event WithdrawAndRevert(address indexed to, uint256 amount, bytes data); | ||||||||||||||
// @dev Only TSS address allowed modifier. | ||||||||||||||
modifier onlyTSS() { | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: onlyTSS() is defined two times, we could consider moving it to an external file which defines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what would be a good solution here, would we need separate contract which defines this modifier and inherit from it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can aim for something in the direction of open-zeppelin This way we can abstract the access control logic into a single contract, something like
Regarding the upgrading logic, bear with me as I still lack some context on when and how the TSS address is updated I see a couple options:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i like having |
||||||||||||||
if (msg.sender != tssAddress) { | ||||||||||||||
revert InvalidSender(); | ||||||||||||||
} | ||||||||||||||
_; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
constructor(address _gateway, address _zetaToken) { | ||||||||||||||
if (_gateway == address(0) || _zetaToken == address(0)) { | ||||||||||||||
constructor(address _gateway, address _zetaToken, address _tssAddress) { | ||||||||||||||
if (_gateway == address(0) || _zetaToken == address(0) || _tssAddress == address(0)) { | ||||||||||||||
revert ZeroAddress(); | ||||||||||||||
} | ||||||||||||||
gateway = IGatewayEVM(_gateway); | ||||||||||||||
zetaToken = _zetaToken; | ||||||||||||||
tssAddress = _tssAddress; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
function withdraw(address to, uint256 amount, bytes32 internalSendHash) external virtual; | ||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in other comment, the ideal imho would be:
contract ERC20CustodyNew is IERC20Custody, ReentrancyGuard