From 29aef2d2c3b34bbb0ff226a271fa85f189996332 Mon Sep 17 00:00:00 2001 From: Nick Pai Date: Fri, 18 Feb 2022 00:13:05 -0700 Subject: [PATCH 1/2] improve(spoke-pool): Move some logic to internal virtual methods to support other evms --- contracts/Arbitrum_SpokePool.sol | 12 +++++++++++ contracts/Ethereum_SpokePool.sol | 14 +++++++++++- contracts/Optimism_SpokePool.sol | 11 ++++++++++ contracts/SpokePool.sol | 37 ++++++++++++++++++++++++-------- contracts/test/MockSpokePool.sol | 12 +++++++++++ 5 files changed, 76 insertions(+), 10 deletions(-) diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol index ac9d096e0..1174be32f 100644 --- a/contracts/Arbitrum_SpokePool.sol +++ b/contracts/Arbitrum_SpokePool.sol @@ -90,6 +90,18 @@ contract Arbitrum_SpokePool is SpokePoolInterface, SpokePool { * INTERNAL FUNCTIONS * **************************************/ + function _chainId() internal view override returns (uint256) { + return block.chainid; + } + + function _verifyDepositorUpdateFeeMessage( + address depositor, + bytes32 ethSignedMessageHash, + bytes memory depositorSignature + ) internal view override { + _defaultVerifyDepositorUpdateFeeMessage(depositor, ethSignedMessageHash, depositorSignature); + } + function _bridgeTokensToHubPool(DestinationDistributionLeaf memory distributionLeaf) internal override { StandardBridgeLike(l2GatewayRouter).outboundTransfer( whitelistedTokens[distributionLeaf.l2TokenAddress], // _l1Token. Address of the L1 token to bridge over. diff --git a/contracts/Ethereum_SpokePool.sol b/contracts/Ethereum_SpokePool.sol index 86bcdc317..99c064b7e 100644 --- a/contracts/Ethereum_SpokePool.sol +++ b/contracts/Ethereum_SpokePool.sol @@ -28,6 +28,18 @@ contract Ethereum_SpokePool is SpokePoolInterface, SpokePool, Ownable { * ADMIN FUNCTIONS * **************************************/ + function _chainId() internal view override returns (uint256) { + return block.chainid; + } + + function _verifyDepositorUpdateFeeMessage( + address depositor, + bytes32 ethSignedMessageHash, + bytes memory depositorSignature + ) internal view override { + _defaultVerifyDepositorUpdateFeeMessage(depositor, ethSignedMessageHash, depositorSignature); + } + function setCrossDomainAdmin(address newCrossDomainAdmin) public override onlyOwner nonReentrant { _setCrossDomainAdmin(newCrossDomainAdmin); } @@ -38,7 +50,7 @@ contract Ethereum_SpokePool is SpokePoolInterface, SpokePool, Ownable { function setEnableRoute( address originToken, - uint32 destinationChainId, + uint256 destinationChainId, bool enable ) public override onlyOwner nonReentrant { _setEnableRoute(originToken, destinationChainId, enable); diff --git a/contracts/Optimism_SpokePool.sol b/contracts/Optimism_SpokePool.sol index d3da56984..e5c737f2b 100644 --- a/contracts/Optimism_SpokePool.sol +++ b/contracts/Optimism_SpokePool.sol @@ -87,6 +87,17 @@ contract Optimism_SpokePool is CrossDomainEnabled, SpokePoolInterface, SpokePool /************************************** * INTERNAL FUNCTIONS * **************************************/ + function _chainId() internal view override returns (uint256) { + return block.chainid; + } + + function _verifyDepositorUpdateFeeMessage( + address depositor, + bytes32 ethSignedMessageHash, + bytes memory depositorSignature + ) internal view override { + _defaultVerifyDepositorUpdateFeeMessage(depositor, ethSignedMessageHash, depositorSignature); + } function _setL1GasLimit(uint32 _l1Gas) internal { l1Gas = _l1Gas; diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index e1a3db8eb..06ca1558e 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -310,14 +310,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall // above keccak156 hash), then this will revert. bytes32 ethSignedMessageHash = ECDSA.toEthSignedMessageHash(expectedDepositorMessageHash); - // Note: no need to worry about reentrancy from contract deployed at `depositor` address since - // `SignatureChecker.isValidSignatureNow` is a non state-modifying STATICCALL: - // - https://github.com/OpenZeppelin/openzeppelin-contracts/blob/63b466901fb015538913f811c5112a2775042177/contracts/utils/cryptography/SignatureChecker.sol#L35 - // - https://github.com/ethereum/EIPs/pull/214 - require( - SignatureChecker.isValidSignatureNow(depositor, ethSignedMessageHash, depositorSignature), - "invalid signature" - ); + _verifyDepositorUpdateFeeMessage(depositor, ethSignedMessageHash, depositorSignature); } // Now follow the default `fillRelay` flow with the updated fee and the original relay hash. @@ -445,7 +438,7 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall **************************************/ function chainId() public view returns (uint256) { - return block.chainid; + return _chainId(); } /************************************** @@ -456,6 +449,32 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall internal virtual; + // Some L2s like ZKSync don't support the CHAIN_ID opcode so we allow the caller to manually set this. + function _chainId() internal view virtual returns (uint256); + + // Allow L2 to implement chain specific recovering of signers from signatures because some L2s might not support + // ecrecover, such as those with account abstraction like ZKSync. + function _verifyDepositorUpdateFeeMessage( + address depositor, + bytes32 ethSignedMessageHash, + bytes memory depositorSignature + ) internal view virtual; + + function _defaultVerifyDepositorUpdateFeeMessage( + address depositor, + bytes32 ethSignedMessageHash, + bytes memory depositorSignature + ) internal view { + // Note: no need to worry about reentrancy from contract deployed at `depositor` address since + // `SignatureChecker.isValidSignatureNow` is a non state-modifying STATICCALL: + // - https://github.com/OpenZeppelin/openzeppelin-contracts/blob/63b466901fb015538913f811c5112a2775042177/contracts/utils/cryptography/SignatureChecker.sol#L35 + // - https://github.com/ethereum/EIPs/pull/214 + require( + SignatureChecker.isValidSignatureNow(depositor, ethSignedMessageHash, depositorSignature), + "invalid signature" + ); + } + function _computeAmountPreFees(uint256 amount, uint64 feesPct) private pure returns (uint256) { return (1e18 * amount) / (1e18 - feesPct); } diff --git a/contracts/test/MockSpokePool.sol b/contracts/test/MockSpokePool.sol index 87b7cf00f..942c8254f 100644 --- a/contracts/test/MockSpokePool.sol +++ b/contracts/test/MockSpokePool.sol @@ -44,4 +44,16 @@ contract MockSpokePool is SpokePoolInterface, SpokePool { } function _bridgeTokensToHubPool(DestinationDistributionLeaf memory distributionLeaf) internal override {} + + function _chainId() internal view override returns (uint256) { + return block.chainid; + } + + function _verifyDepositorUpdateFeeMessage( + address depositor, + bytes32 ethSignedMessageHash, + bytes memory depositorSignature + ) internal view override { + _defaultVerifyDepositorUpdateFeeMessage(depositor, ethSignedMessageHash, depositorSignature); + } } From 7c7b36327ae4bad37dea4c8bf93f36089b7e22dd Mon Sep 17 00:00:00 2001 From: Nick Pai Date: Fri, 18 Feb 2022 19:10:21 -0700 Subject: [PATCH 2/2] fix --- contracts/Arbitrum_SpokePool.sol | 12 ------------ contracts/Ethereum_SpokePool.sol | 12 ------------ contracts/Optimism_SpokePool.sol | 11 ----------- contracts/SpokePool.sol | 16 ++++------------ contracts/test/MockSpokePool.sol | 12 ------------ 5 files changed, 4 insertions(+), 59 deletions(-) diff --git a/contracts/Arbitrum_SpokePool.sol b/contracts/Arbitrum_SpokePool.sol index 7decc5ae2..2616437e0 100644 --- a/contracts/Arbitrum_SpokePool.sol +++ b/contracts/Arbitrum_SpokePool.sol @@ -90,18 +90,6 @@ contract Arbitrum_SpokePool is SpokePoolInterface, SpokePool { * INTERNAL FUNCTIONS * **************************************/ - function _chainId() internal view override returns (uint256) { - return block.chainid; - } - - function _verifyDepositorUpdateFeeMessage( - address depositor, - bytes32 ethSignedMessageHash, - bytes memory depositorSignature - ) internal view override { - _defaultVerifyDepositorUpdateFeeMessage(depositor, ethSignedMessageHash, depositorSignature); - } - function _bridgeTokensToHubPool(RelayerRefundLeaf memory relayerRefundLeaf) internal override { StandardBridgeLike(l2GatewayRouter).outboundTransfer( whitelistedTokens[relayerRefundLeaf.l2TokenAddress], // _l1Token. Address of the L1 token to bridge over. diff --git a/contracts/Ethereum_SpokePool.sol b/contracts/Ethereum_SpokePool.sol index 7e6bac93d..359c32bf5 100644 --- a/contracts/Ethereum_SpokePool.sol +++ b/contracts/Ethereum_SpokePool.sol @@ -28,18 +28,6 @@ contract Ethereum_SpokePool is SpokePoolInterface, SpokePool, Ownable { * ADMIN FUNCTIONS * **************************************/ - function _chainId() internal view override returns (uint256) { - return block.chainid; - } - - function _verifyDepositorUpdateFeeMessage( - address depositor, - bytes32 ethSignedMessageHash, - bytes memory depositorSignature - ) internal view override { - _defaultVerifyDepositorUpdateFeeMessage(depositor, ethSignedMessageHash, depositorSignature); - } - function setCrossDomainAdmin(address newCrossDomainAdmin) public override onlyOwner nonReentrant { _setCrossDomainAdmin(newCrossDomainAdmin); } diff --git a/contracts/Optimism_SpokePool.sol b/contracts/Optimism_SpokePool.sol index 4cb095390..d2715de0b 100644 --- a/contracts/Optimism_SpokePool.sol +++ b/contracts/Optimism_SpokePool.sol @@ -87,17 +87,6 @@ contract Optimism_SpokePool is CrossDomainEnabled, SpokePoolInterface, SpokePool /************************************** * INTERNAL FUNCTIONS * **************************************/ - function _chainId() internal view override returns (uint256) { - return block.chainid; - } - - function _verifyDepositorUpdateFeeMessage( - address depositor, - bytes32 ethSignedMessageHash, - bytes memory depositorSignature - ) internal view override { - _defaultVerifyDepositorUpdateFeeMessage(depositor, ethSignedMessageHash, depositorSignature); - } function _setL1GasLimit(uint32 _l1Gas) internal { l1Gas = _l1Gas; diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index 4579f7724..7f6f4a902 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -425,8 +425,9 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall * VIEW FUNCTIONS * **************************************/ - function chainId() public view returns (uint256) { - return _chainId(); + // Some L2s like ZKSync don't support the CHAIN_ID opcode so we allow the caller to manually set this. + function chainId() public view virtual returns (uint256) { + return block.chainid; } /************************************** @@ -435,22 +436,13 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall function _bridgeTokensToHubPool(SpokePoolInterface.RelayerRefundLeaf memory relayerRefundLeaf) internal virtual; - // Some L2s like ZKSync don't support the CHAIN_ID opcode so we allow the caller to manually set this. - function _chainId() internal view virtual returns (uint256); - // Allow L2 to implement chain specific recovering of signers from signatures because some L2s might not support // ecrecover, such as those with account abstraction like ZKSync. function _verifyDepositorUpdateFeeMessage( address depositor, bytes32 ethSignedMessageHash, bytes memory depositorSignature - ) internal view virtual; - - function _defaultVerifyDepositorUpdateFeeMessage( - address depositor, - bytes32 ethSignedMessageHash, - bytes memory depositorSignature - ) internal view { + ) internal view virtual { // Note: no need to worry about reentrancy from contract deployed at `depositor` address since // `SignatureChecker.isValidSignatureNow` is a non state-modifying STATICCALL: // - https://github.com/OpenZeppelin/openzeppelin-contracts/blob/63b466901fb015538913f811c5112a2775042177/contracts/utils/cryptography/SignatureChecker.sol#L35 diff --git a/contracts/test/MockSpokePool.sol b/contracts/test/MockSpokePool.sol index 0a18d2536..934e072ba 100644 --- a/contracts/test/MockSpokePool.sol +++ b/contracts/test/MockSpokePool.sol @@ -41,16 +41,4 @@ contract MockSpokePool is SpokePoolInterface, SpokePool { } function _bridgeTokensToHubPool(RelayerRefundLeaf memory relayerRefundLeaf) internal override {} - - function _chainId() internal view override returns (uint256) { - return block.chainid; - } - - function _verifyDepositorUpdateFeeMessage( - address depositor, - bytes32 ethSignedMessageHash, - bytes memory depositorSignature - ) internal view override { - _defaultVerifyDepositorUpdateFeeMessage(depositor, ethSignedMessageHash, depositorSignature); - } }