From 915a2b60e451bf67ff86596a789af6a74dc2c491 Mon Sep 17 00:00:00 2001 From: skosito Date: Mon, 14 Oct 2024 13:35:05 +0200 Subject: [PATCH] review fixes --- v2/contracts/evm/ERC20Custody.sol | 4 +-- v2/contracts/evm/GatewayEVM.sol | 32 ++++++++++--------- v2/contracts/evm/ZetaConnectorBase.sol | 4 +-- v2/contracts/evm/ZetaConnectorNonNative.sol | 2 +- v2/contracts/evm/interfaces/IERC20Custody.sol | 3 +- v2/contracts/evm/interfaces/IGatewayEVM.sol | 3 +- .../evm/interfaces/IZetaConnector.sol | 3 +- v2/contracts/zevm/GatewayZEVM.sol | 18 ++++++----- v2/test/ERC20Custody.t.sol | 2 +- v2/test/GatewayEVM.t.sol | 14 ++++---- v2/test/ZetaConnectorNative.t.sol | 2 +- .../upgrades/ERC20CustodyUpgradeTest.sol | 4 +-- .../utils/upgrades/GatewayEVMUpgradeTest.sol | 4 +-- 13 files changed, 51 insertions(+), 44 deletions(-) diff --git a/v2/contracts/evm/ERC20Custody.sol b/v2/contracts/evm/ERC20Custody.sol index 22481a0e..62462095 100644 --- a/v2/contracts/evm/ERC20Custody.sol +++ b/v2/contracts/evm/ERC20Custody.sol @@ -90,9 +90,9 @@ contract ERC20Custody is _grantRole(WITHDRAWER_ROLE, newTSSAddress); _grantRole(WHITELISTER_ROLE, newTSSAddress); - tssAddress = newTSSAddress; + emit UpdatedCustodyTSSAddress(tssAddress, newTSSAddress); - emit UpdatedCustodyTSSAddress(newTSSAddress); + tssAddress = newTSSAddress; } /// @notice Unpause contract. diff --git a/v2/contracts/evm/GatewayEVM.sol b/v2/contracts/evm/GatewayEVM.sol index 06a8d89e..ad299275 100644 --- a/v2/contracts/evm/GatewayEVM.sol +++ b/v2/contracts/evm/GatewayEVM.sol @@ -80,7 +80,7 @@ contract GatewayEVM is /// @param data Calldata to pass to the call. /// @return The result of the call. function _execute(address destination, bytes calldata data) internal returns (bytes memory) { - revertIfCallingOnRevert(data); + _revertIfCallingOnRevert(data); (bool success, bytes memory result) = destination.call{ value: msg.value }(data); if (!success) revert ExecutionFailed(); @@ -95,9 +95,9 @@ contract GatewayEVM is _revokeRole(TSS_ROLE, tssAddress); _grantRole(TSS_ROLE, newTSSAddress); - tssAddress = newTSSAddress; + emit UpdatedGatewayTSSAddress(tssAddress, newTSSAddress); - emit UpdatedGatewayTSSAddress(newTSSAddress); + tssAddress = newTSSAddress; } /// @notice Pause contract. @@ -178,18 +178,18 @@ contract GatewayEVM is if (amount == 0) revert InsufficientERC20Amount(); if (to == address(0)) revert ZeroAddress(); // Approve the target contract to spend the tokens - if (!resetApproval(token, to)) revert ApprovalFailed(); + if (!_resetApproval(token, to)) revert ApprovalFailed(); if (!IERC20(token).approve(to, amount)) revert ApprovalFailed(); // Execute the call on the target contract _execute(to, data); // Reset approval - if (!resetApproval(token, to)) revert ApprovalFailed(); + if (!_resetApproval(token, to)) revert ApprovalFailed(); // Transfer any remaining tokens back to the custody/connector contract uint256 remainingBalance = IERC20(token).balanceOf(address(this)); if (remainingBalance > 0) { - transferToAssetHandler(token, remainingBalance); + _transferToAssetHandler(token, remainingBalance); } emit ExecutedWithERC20(token, to, amount, data); @@ -238,6 +238,7 @@ contract GatewayEVM is if (revertOptions.callOnRevert) revert CallOnRevertNotSupported(); if (msg.value == 0) revert InsufficientETHAmount(); if (receiver == address(0)) revert ZeroAddress(); + if (revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); (bool deposited,) = tssAddress.call{ value: msg.value }(""); @@ -264,8 +265,9 @@ contract GatewayEVM is if (revertOptions.callOnRevert) revert CallOnRevertNotSupported(); if (amount == 0) revert InsufficientERC20Amount(); if (receiver == address(0)) revert ZeroAddress(); + if (revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); - transferFromToAssetHandler(msg.sender, asset, amount); + _transferFromToAssetHandler(msg.sender, asset, amount); emit Deposited(msg.sender, receiver, amount, asset, "", revertOptions); } @@ -287,7 +289,7 @@ contract GatewayEVM is if (revertOptions.callOnRevert) revert CallOnRevertNotSupported(); if (msg.value == 0) revert InsufficientETHAmount(); if (receiver == address(0)) revert ZeroAddress(); - if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); + if (payload.length + revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); (bool deposited,) = tssAddress.call{ value: msg.value }(""); @@ -316,9 +318,9 @@ contract GatewayEVM is if (revertOptions.callOnRevert) revert CallOnRevertNotSupported(); if (amount == 0) revert InsufficientERC20Amount(); if (receiver == address(0)) revert ZeroAddress(); - if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); + if (payload.length + revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); - transferFromToAssetHandler(msg.sender, asset, amount); + _transferFromToAssetHandler(msg.sender, asset, amount); emit Deposited(msg.sender, receiver, amount, asset, payload, revertOptions); } @@ -338,7 +340,7 @@ contract GatewayEVM is { if (revertOptions.callOnRevert) revert CallOnRevertNotSupported(); if (receiver == address(0)) revert ZeroAddress(); - if (payload.length + revertOptions.revertMessage.length >= MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); + if (payload.length + revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); emit Called(msg.sender, receiver, payload, revertOptions); } @@ -368,7 +370,7 @@ contract GatewayEVM is /// @param token Address of the ERC20 token. /// @param to Address to reset the approval for. /// @return True if the approval reset was successful, false otherwise. - function resetApproval(address token, address to) private returns (bool) { + function _resetApproval(address token, address to) private returns (bool) { return IERC20(token).approve(to, 0); } @@ -378,7 +380,7 @@ contract GatewayEVM is /// @param from Address of the sender. /// @param token Address of the ERC20 token. /// @param amount Amount of tokens to transfer. - function transferFromToAssetHandler(address from, address token, uint256 amount) private { + function _transferFromToAssetHandler(address from, address token, uint256 amount) private { if (token == zetaToken) { // transfer to connector // transfer amount to gateway @@ -399,7 +401,7 @@ contract GatewayEVM is /// type. /// @param token Address of the ERC20 token. /// @param amount Amount of tokens to transfer. - function transferToAssetHandler(address token, uint256 amount) private { + function _transferToAssetHandler(address token, uint256 amount) private { if (token == zetaToken) { // transfer to connector // approve connector to handle tokens depending on connector version (eg. lock or burn) @@ -414,7 +416,7 @@ contract GatewayEVM is } // @dev prevent spoofing onRevert functions - function revertIfCallingOnRevert(bytes calldata data) private pure { + function _revertIfCallingOnRevert(bytes calldata data) private pure { if (data.length >= 4) { bytes4 functionSelector; assembly { diff --git a/v2/contracts/evm/ZetaConnectorBase.sol b/v2/contracts/evm/ZetaConnectorBase.sol index 4cae6340..ab08cf6c 100644 --- a/v2/contracts/evm/ZetaConnectorBase.sol +++ b/v2/contracts/evm/ZetaConnectorBase.sol @@ -90,9 +90,9 @@ abstract contract ZetaConnectorBase is _grantRole(WITHDRAWER_ROLE, newTSSAddress); _grantRole(TSS_ROLE, newTSSAddress); + emit UpdatedZetaConnectorTSSAddress(tssAddress, newTSSAddress); + tssAddress = newTSSAddress; - - emit UpdatedZetaConnectorTSSAddress(newTSSAddress); } /// @notice Pause contract. diff --git a/v2/contracts/evm/ZetaConnectorNonNative.sol b/v2/contracts/evm/ZetaConnectorNonNative.sol index 33c4ae5a..0feab73a 100644 --- a/v2/contracts/evm/ZetaConnectorNonNative.sol +++ b/v2/contracts/evm/ZetaConnectorNonNative.sol @@ -123,7 +123,7 @@ contract ZetaConnectorNonNative is ZetaConnectorBase { } /// @dev mints to provided account and checks if totalSupply will be exceeded - function _mintTo(address to, uint256 amount, bytes32 internalSendHash) internal { + function _mintTo(address to, uint256 amount, bytes32 internalSendHash) private { if (amount + IERC20(zetaToken).totalSupply() > maxSupply) revert ExceedsMaxSupply(); IZetaNonEthNew(zetaToken).mint(address(to), amount, internalSendHash); } diff --git a/v2/contracts/evm/interfaces/IERC20Custody.sol b/v2/contracts/evm/interfaces/IERC20Custody.sol index 1feda341..525f494b 100644 --- a/v2/contracts/evm/interfaces/IERC20Custody.sol +++ b/v2/contracts/evm/interfaces/IERC20Custody.sol @@ -43,7 +43,8 @@ interface IERC20CustodyEvents { /// @notice Emitted when tss address is updated /// @param newTSSAddress new tss address - event UpdatedCustodyTSSAddress(address newTSSAddress); + /// @param oldTSSAddress old tss address + event UpdatedCustodyTSSAddress(address oldTSSAddress, address newTSSAddress); } /// @title IERC20CustodyErrors diff --git a/v2/contracts/evm/interfaces/IGatewayEVM.sol b/v2/contracts/evm/interfaces/IGatewayEVM.sol index adf976a6..364c39e8 100644 --- a/v2/contracts/evm/interfaces/IGatewayEVM.sol +++ b/v2/contracts/evm/interfaces/IGatewayEVM.sol @@ -52,7 +52,8 @@ interface IGatewayEVMEvents { /// @notice Emitted when tss address is updated /// @param newTSSAddress new tss address - event UpdatedGatewayTSSAddress(address newTSSAddress); + /// @param oldTSSAddress old tss address + event UpdatedGatewayTSSAddress(address oldTSSAddress, address newTSSAddress); } /// @title IGatewayEVMErrors diff --git a/v2/contracts/evm/interfaces/IZetaConnector.sol b/v2/contracts/evm/interfaces/IZetaConnector.sol index 103add97..d611b23f 100644 --- a/v2/contracts/evm/interfaces/IZetaConnector.sol +++ b/v2/contracts/evm/interfaces/IZetaConnector.sol @@ -26,5 +26,6 @@ interface IZetaConnectorEvents { /// @notice Emitted when tss address is updated /// @param newTSSAddress new tss address - event UpdatedZetaConnectorTSSAddress(address newTSSAddress); + /// @param oldTSSAddress old tss address + event UpdatedZetaConnectorTSSAddress(address oldTSSAddress, address newTSSAddress); } diff --git a/v2/contracts/zevm/GatewayZEVM.sol b/v2/contracts/zevm/GatewayZEVM.sol index b1fdd034..0f6a71e6 100644 --- a/v2/contracts/zevm/GatewayZEVM.sol +++ b/v2/contracts/zevm/GatewayZEVM.sol @@ -88,21 +88,21 @@ contract GatewayZEVM is _unpause(); } - /// @dev Internal function to withdraw ZRC20 tokens. + /// @dev Private function to withdraw ZRC20 tokens. /// @param amount The amount of tokens to withdraw. /// @param zrc20 The address of the ZRC20 token. /// @return The gas fee for the withdrawal. - function _withdrawZRC20(uint256 amount, address zrc20) internal returns (uint256) { + function _withdrawZRC20(uint256 amount, address zrc20) private returns (uint256) { // Use gas limit from zrc20 return _withdrawZRC20WithGasLimit(amount, zrc20, IZRC20(zrc20).GAS_LIMIT()); } - /// @dev Internal function to withdraw ZRC20 tokens with gas limit. + /// @dev Private function to withdraw ZRC20 tokens with gas limit. /// @param amount The amount of tokens to withdraw. /// @param zrc20 The address of the ZRC20 token. /// @param gasLimit Gas limit. /// @return The gas fee for the withdrawal. - function _withdrawZRC20WithGasLimit(uint256 amount, address zrc20, uint256 gasLimit) internal returns (uint256) { + function _withdrawZRC20WithGasLimit(uint256 amount, address zrc20, uint256 gasLimit) private returns (uint256) { (address gasZRC20, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit); if (!IZRC20(gasZRC20).transferFrom(msg.sender, FUNGIBLE_MODULE_ADDRESS, gasFee)) { revert GasFeeTransferFailed(); @@ -117,10 +117,10 @@ contract GatewayZEVM is return gasFee; } - /// @dev Internal function to transfer ZETA tokens. + /// @dev Private function to transfer ZETA tokens. /// @param amount The amount of tokens to transfer. /// @param to The address to transfer the tokens to. - function _transferZETA(uint256 amount, address to) internal { + function _transferZETA(uint256 amount, address to) private { if (!IWETH9(zetaToken).transferFrom(msg.sender, address(this), amount)) revert FailedZetaSent(); IWETH9(zetaToken).withdraw(amount); (bool sent,) = to.call{ value: amount }(""); @@ -145,6 +145,7 @@ contract GatewayZEVM is if (revertOptions.callOnRevert) revert CallOnRevertNotSupported(); if (receiver.length == 0) revert ZeroAddress(); if (amount == 0) revert InsufficientZRC20Amount(); + if (revertOptions.revertMessage.length > MAX_MESSAGE_SIZE) revert MessageSizeExceeded(); uint256 gasFee = _withdrawZRC20(amount, zrc20); emit Withdrawn( @@ -219,6 +220,7 @@ contract GatewayZEVM is if (revertOptions.callOnRevert) revert CallOnRevertNotSupported(); if (receiver.length == 0) revert ZeroAddress(); if (amount == 0) revert InsufficientZetaAmount(); + if (revertOptions.revertMessage.length > MAX_MESSAGE_SIZE) revert MessageSizeExceeded(); _transferZETA(amount, FUNGIBLE_MODULE_ADDRESS); emit Withdrawn(msg.sender, chainId, receiver, address(zetaToken), amount, 0, 0, "", 0, revertOptions); @@ -245,7 +247,7 @@ contract GatewayZEVM is if (revertOptions.callOnRevert) revert CallOnRevertNotSupported(); if (receiver.length == 0) revert ZeroAddress(); if (amount == 0) revert InsufficientZetaAmount(); - if (message.length + revertOptions.revertMessage.length >= MAX_MESSAGE_SIZE) revert MessageSizeExceeded(); + if (message.length + revertOptions.revertMessage.length > MAX_MESSAGE_SIZE) revert MessageSizeExceeded(); _transferZETA(amount, FUNGIBLE_MODULE_ADDRESS); emit Withdrawn(msg.sender, chainId, receiver, address(zetaToken), amount, 0, 0, message, 0, revertOptions); @@ -271,7 +273,7 @@ contract GatewayZEVM is if (revertOptions.callOnRevert) revert CallOnRevertNotSupported(); if (receiver.length == 0) revert ZeroAddress(); if (gasLimit == 0) revert InsufficientGasLimit(); - if (message.length + revertOptions.revertMessage.length >= MAX_MESSAGE_SIZE) revert MessageSizeExceeded(); + if (message.length + revertOptions.revertMessage.length > MAX_MESSAGE_SIZE) revert MessageSizeExceeded(); (address gasZRC20, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit); if (!IZRC20(gasZRC20).transferFrom(msg.sender, FUNGIBLE_MODULE_ADDRESS, gasFee)) { diff --git a/v2/test/ERC20Custody.t.sol b/v2/test/ERC20Custody.t.sol index ecb55235..1c44fdfb 100644 --- a/v2/test/ERC20Custody.t.sol +++ b/v2/test/ERC20Custody.t.sol @@ -107,7 +107,7 @@ contract ERC20CustodyTest is Test, IGatewayEVMErrors, IGatewayEVMEvents, IReceiv vm.startPrank(owner); vm.expectEmit(true, true, true, true, address(custody)); - emit UpdatedCustodyTSSAddress(newTSSAddress); + emit UpdatedCustodyTSSAddress(tssAddress, newTSSAddress); custody.updateTSSAddress(newTSSAddress); assertEq(newTSSAddress, custody.tssAddress()); diff --git a/v2/test/GatewayEVM.t.sol b/v2/test/GatewayEVM.t.sol index 4a7067b1..ebb7176f 100644 --- a/v2/test/GatewayEVM.t.sol +++ b/v2/test/GatewayEVM.t.sol @@ -100,7 +100,7 @@ contract GatewayEVMTest is Test, IGatewayEVMErrors, IGatewayEVMEvents, IReceiver vm.startPrank(owner); vm.expectEmit(true, true, true, true, address(gateway)); - emit UpdatedGatewayTSSAddress(newTSSAddress); + emit UpdatedGatewayTSSAddress(tssAddress, newTSSAddress); gateway.updateTSSAddress(newTSSAddress); assertEq(newTSSAddress, gateway.tssAddress()); @@ -501,8 +501,8 @@ contract GatewayEVMInboundTest is Test, IGatewayEVMErrors, IGatewayEVMEvents, IR function testDepositERC20ToCustodyWithPayloadFailsIfPayloadSizeExceeded() public { uint256 amount = 100_000; - bytes memory payload = new bytes(512); - revertOptions.revertMessage = new bytes(512); + bytes memory payload = new bytes(gateway.MAX_PAYLOAD_SIZE() / 2); + revertOptions.revertMessage = new bytes(gateway.MAX_PAYLOAD_SIZE() / 2 + 1); token.approve(address(gateway), amount); @@ -576,8 +576,8 @@ contract GatewayEVMInboundTest is Test, IGatewayEVMErrors, IGatewayEVMEvents, IR function testDepositEthToTssWithPayloadFailsIfPayloadSizeExceeded() public { uint256 amount = 100_000; - bytes memory payload = new bytes(512); - revertOptions.revertMessage = new bytes(512); + bytes memory payload = new bytes(gateway.MAX_PAYLOAD_SIZE() / 2); + revertOptions.revertMessage = new bytes(gateway.MAX_PAYLOAD_SIZE() / 2 + 1); vm.expectRevert(PayloadSizeExceeded.selector); gateway.depositAndCall{ value: amount }(destination, payload, revertOptions); @@ -608,8 +608,8 @@ contract GatewayEVMInboundTest is Test, IGatewayEVMErrors, IGatewayEVMEvents, IR } function testCallWithPayloadFailsIfPayloadSizeExceeded() public { - bytes memory payload = new bytes(512); - revertOptions.revertMessage = new bytes(512); + bytes memory payload = new bytes(gateway.MAX_PAYLOAD_SIZE() / 2); + revertOptions.revertMessage = new bytes(gateway.MAX_PAYLOAD_SIZE() / 2 + 1); vm.expectRevert(PayloadSizeExceeded.selector); gateway.call(destination, payload, revertOptions); diff --git a/v2/test/ZetaConnectorNative.t.sol b/v2/test/ZetaConnectorNative.t.sol index c7a35522..dbf1f713 100644 --- a/v2/test/ZetaConnectorNative.t.sol +++ b/v2/test/ZetaConnectorNative.t.sol @@ -104,7 +104,7 @@ contract ZetaConnectorNativeTest is vm.startPrank(owner); vm.expectEmit(true, true, true, true, address(zetaConnector)); - emit UpdatedZetaConnectorTSSAddress(newTSSAddress); + emit UpdatedZetaConnectorTSSAddress(tssAddress, newTSSAddress); zetaConnector.updateTSSAddress(newTSSAddress); assertEq(newTSSAddress, zetaConnector.tssAddress()); diff --git a/v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol b/v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol index 2c67a624..00fcf9cc 100644 --- a/v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol +++ b/v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol @@ -93,9 +93,9 @@ contract ERC20CustodyUpgradeTest is _grantRole(WITHDRAWER_ROLE, newTSSAddress); _grantRole(WHITELISTER_ROLE, newTSSAddress); - tssAddress = newTSSAddress; + emit UpdatedCustodyTSSAddress(tssAddress, newTSSAddress); - emit UpdatedCustodyTSSAddress(newTSSAddress); + tssAddress = newTSSAddress; } /// @notice Unpause contract. diff --git a/v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol b/v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol index d2e36048..f181e3d6 100644 --- a/v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol +++ b/v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol @@ -98,9 +98,9 @@ contract GatewayEVMUpgradeTest is _revokeRole(TSS_ROLE, tssAddress); _grantRole(TSS_ROLE, newTSSAddress); + emit UpdatedGatewayTSSAddress(tssAddress, newTSSAddress); + tssAddress = newTSSAddress; - - emit UpdatedGatewayTSSAddress(newTSSAddress); } /// @notice Pause contract.