-
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 10 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 @@ contract ERC20CustodyNew is ReentrancyGuard{ | |||||
|
||||||
// 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 @@ contract GatewayEVM is Initializable, OwnableUpgradeable, UUPSUpgradeable, IGate | |||||||
/// @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 asset handler (custody, connector) allowed modifier. | ||||||||
modifier onlyAssetHandler() { | ||||||||
if (msg.sender != custody && msg.sender != zetaConnector) { | ||||||||
revert InvalidSender(); | ||||||||
} | ||||||||
_; | ||||||||
} | ||||||||
|
||||||||
/// @custom:oz-upgrades-unsafe-allow constructor | ||||||||
constructor() { | ||||||||
_disableInitializers(); | ||||||||
|
@@ -56,7 +72,7 @@ contract GatewayEVM is Initializable, OwnableUpgradeable, UUPSUpgradeable, IGate | |||||||
|
||||||||
// 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); | ||||||||
|
@@ -67,7 +83,7 @@ contract GatewayEVM is Initializable, OwnableUpgradeable, UUPSUpgradeable, IGate | |||||||
// 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); | ||||||||
|
@@ -84,7 +100,7 @@ contract GatewayEVM is Initializable, OwnableUpgradeable, UUPSUpgradeable, IGate | |||||||
address to, | ||||||||
uint256 amount, | ||||||||
bytes calldata data | ||||||||
) public nonReentrant { | ||||||||
) public nonReentrant onlyAssetHandler { | ||||||||
if (amount == 0) revert InsufficientERC20Amount(); | ||||||||
// Approve the target contract to spend the tokens | ||||||||
if(!resetApproval(token, to)) revert ApprovalFailed(); | ||||||||
|
@@ -111,7 +127,7 @@ contract GatewayEVM is Initializable, OwnableUpgradeable, UUPSUpgradeable, IGate | |||||||
address to, | ||||||||
uint256 amount, | ||||||||
bytes calldata data | ||||||||
) external nonReentrant { | ||||||||
) external nonReentrant onlyAssetHandler { | ||||||||
if (amount == 0) revert InsufficientERC20Amount(); | ||||||||
|
||||||||
IERC20(token).safeTransfer(address(to), amount); | ||||||||
|
@@ -163,14 +179,14 @@ contract GatewayEVM is Initializable, OwnableUpgradeable, UUPSUpgradeable, IGate | |||||||
emit Call(msg.sender, receiver, payload); | ||||||||
} | ||||||||
|
||||||||
function setCustody(address _custody) external { | ||||||||
function setCustody(address _custody) external onlyTSS { | ||||||||
Check warning Code scanning / Slither Conformance to Solidity naming conventions Warning
Parameter GatewayEVM.setCustody(address)._custody is not in mixedCase
|
||||||||
if (custody != address(0)) revert CustodyInitialized(); | ||||||||
if (_custody == address(0)) revert ZeroAddress(); | ||||||||
|
||||||||
custody = _custody; | ||||||||
} | ||||||||
Comment on lines
+182
to
187
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. Emit an event and follow naming conventions. The function should emit an event when the function setCustody(address _custody) external onlyTSS {
if (custody != address(0)) revert CustodyInitialized();
if (_custody == address(0)) revert ZeroAddress();
custody = _custody;
+ emit CustodySet(_custody);
}
ToolsGitHub Check: Slither
|
||||||||
|
||||||||
function setConnector(address _zetaConnector) external { | ||||||||
function setConnector(address _zetaConnector) external onlyTSS { | ||||||||
Check warning Code scanning / Slither Conformance to Solidity naming conventions Warning
Parameter GatewayEVM.setConnector(address)._zetaConnector is not in mixedCase
|
||||||||
if (zetaConnector != address(0)) revert CustodyInitialized(); | ||||||||
if (_zetaConnector == address(0)) revert ZeroAddress(); | ||||||||
|
||||||||
|
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