From d21842403cb5b53264d3d66d93137596ab4b8c5a Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 21 Mar 2022 07:29:19 -0400 Subject: [PATCH 1/7] improve: address solhint warnings Signed-off-by: nicholaspai --- contracts/HubPool.sol | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 70b7c29b5..9528ad072 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -664,6 +664,10 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { if (groupIndex == 0) { // Relay root bundles to spoke pool on destination chain by // performing delegatecall to use the adapter's code with this contract's context. + + // We are ok with this low-level call since the adapter address is set by the admin and we've + // already checked that its not the zero address. + // solhint-disable-next-line avoid-low-level-calls (bool success, ) = adapter.delegatecall( abi.encodeWithSignature( "relayMessage(address,bytes)", @@ -819,7 +823,9 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * messages to Arbitrum. Relaying messages to Arbitrum requires that this contract has an ETH balance, so in this * case the caller would need to pre-load this contract with ETH before multicall-executing the leaf. */ - function loadEthForL2Calls() public payable override {} + function loadEthForL2Calls() public payable override { + /* solhint-disable-line no-empty-blocks */ + } /************************************************* * INTERNAL FUNCTIONS * @@ -867,6 +873,10 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { if (netSendAmounts[i] > 0) { // Perform delegatecall to use the adapter's code with this contract's context. Opt for delegatecall's // complexity in exchange for lower gas costs. + + // We are ok with this low-level call since the adapter address is set by the admin and we've + // already checked that its not the zero address. + // solhint-disable-next-line avoid-low-level-calls (bool success, ) = adapter.delegatecall( abi.encodeWithSignature( "relayTokens(address,address,uint256,address)", @@ -985,6 +995,10 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { (address adapter, address spokePool) = _getInitializedCrossChainContracts(chainId); // Perform delegatecall to use the adapter's code with this contract's context. + + // We are ok with this low-level call since the adapter address is set by the admin and we've + // already checked that its not the zero address. + // solhint-disable-next-line avoid-low-level-calls (bool success, ) = adapter.delegatecall( abi.encodeWithSignature( "relayMessage(address,bytes)", From 7eb7da9424cb1546d74db1e019085a4b62132f7c Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 21 Mar 2022 10:25:56 -0400 Subject: [PATCH 2/7] fix all --- contracts/chain-adapters/Arbitrum_Adapter.sol | 2 ++ contracts/chain-adapters/CrossDomainEnabled.sol | 6 +++--- contracts/chain-adapters/Ethereum_Adapter.sol | 5 ++++- contracts/chain-adapters/Mock_Adapter.sol | 6 +++++- contracts/chain-adapters/Optimism_Adapter.sol | 2 ++ contracts/chain-adapters/Polygon_Adapter.sol | 2 ++ contracts/test/MockSpokePool.sol | 8 +++++--- contracts/test/PolygonERC20Test.sol | 2 +- contracts/test/PolygonMocks.sol | 5 +++-- package.json | 1 + 10 files changed, 28 insertions(+), 11 deletions(-) diff --git a/contracts/chain-adapters/Arbitrum_Adapter.sol b/contracts/chain-adapters/Arbitrum_Adapter.sol index a8f4e16d6..0e582f6a8 100644 --- a/contracts/chain-adapters/Arbitrum_Adapter.sol +++ b/contracts/chain-adapters/Arbitrum_Adapter.sol @@ -34,6 +34,8 @@ interface ArbitrumL1ERC20GatewayLike { * For example, the HubPool will delegatecall these functions, therefore its only necessary that the HubPool's methods * that call this contract's logic guard against reentrancy. */ + +// solhint-disable-next-line contract-name-camelcase contract Arbitrum_Adapter is AdapterInterface { // Gas limit for immediate L2 execution attempt (can be estimated via NodeInterface.estimateRetryableTicket). // NodeInterface precompile interface exists at L2 address 0x00000000000000000000000000000000000000C8 diff --git a/contracts/chain-adapters/CrossDomainEnabled.sol b/contracts/chain-adapters/CrossDomainEnabled.sol index bcbec4d08..b637ca0e0 100644 --- a/contracts/chain-adapters/CrossDomainEnabled.sol +++ b/contracts/chain-adapters/CrossDomainEnabled.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity >0.5.0 <0.9.0; +pragma solidity ^0.8.0; /* Interface Imports */ import { ICrossDomainMessenger } from "@eth-optimism/contracts/libraries/bridge/ICrossDomainMessenger.sol"; @@ -27,11 +27,11 @@ contract CrossDomainEnabled { * authenticated to call this function. */ modifier onlyFromCrossDomainAccount(address _sourceDomainAccount) { - require(msg.sender == address(getCrossDomainMessenger()), "OVM_XCHAIN: messenger contract unauthenticated"); + require(msg.sender == address(getCrossDomainMessenger()), "invalid cross domain messenger"); require( getCrossDomainMessenger().xDomainMessageSender() == _sourceDomainAccount, - "OVM_XCHAIN: wrong sender of cross-domain message" + "invalid cross domain sender" ); _; diff --git a/contracts/chain-adapters/Ethereum_Adapter.sol b/contracts/chain-adapters/Ethereum_Adapter.sol index 2e9cd1d38..dc5b9ad51 100644 --- a/contracts/chain-adapters/Ethereum_Adapter.sol +++ b/contracts/chain-adapters/Ethereum_Adapter.sol @@ -17,6 +17,8 @@ import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; * For example, the HubPool will delegatecall these functions, therefore its only necessary that the HubPool's methods * that call this contract's logic guard against reentrancy. */ + +// solhint-disable-next-linecontract-name-camelcase contract Ethereum_Adapter is AdapterInterface { using SafeERC20 for IERC20; @@ -54,9 +56,10 @@ contract Ethereum_Adapter is AdapterInterface { // Note: this snippet of code is copied from Governor.sol. Source: https://github.com/UMAprotocol/protocol/blob/5b37ea818a28479c01e458389a83c3e736306b17/packages/core/contracts/oracle/implementation/Governor.sol#L190-L207 function _executeCall(address to, bytes memory data) private { // Note: this snippet of code is copied from Governor.sol and modified to not include any "value" field. - // solhint-disable-next-line no-inline-assembly bool success; + + // solhint-disable-next-line no-inline-assembly assembly { let inputData := add(data, 0x20) let inputDataSize := mload(data) diff --git a/contracts/chain-adapters/Mock_Adapter.sol b/contracts/chain-adapters/Mock_Adapter.sol index d6c2fa62a..8a6470562 100644 --- a/contracts/chain-adapters/Mock_Adapter.sol +++ b/contracts/chain-adapters/Mock_Adapter.sol @@ -7,6 +7,8 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; /** * @notice Contract used for testing communication between HubPool and Adapter. */ + +// solhint-disable-next-line contract-name-camelcase contract Mock_Adapter is AdapterInterface { event RelayMessageCalled(address target, bytes message, address caller); @@ -37,11 +39,13 @@ contract Mock_Adapter is AdapterInterface { // This contract is intended to "act like" a simple version of an L2 bridge. // It's primarily meant to better reflect how a true L2 bridge interaction might work to give better gas estimates. + +// solhint-disable-next-line contract-name-camelcase contract Mock_Bridge { event BridgedTokens(address token, uint256 amount); event BridgedMessage(address target, bytes message); - mapping(address => uint256) deposits; + mapping(address => uint256) public deposits; function bridgeTokens(address token, uint256 amount) public { IERC20(token).transferFrom(msg.sender, address(this), amount); diff --git a/contracts/chain-adapters/Optimism_Adapter.sol b/contracts/chain-adapters/Optimism_Adapter.sol index 715eeeb17..c9d9cd1c0 100644 --- a/contracts/chain-adapters/Optimism_Adapter.sol +++ b/contracts/chain-adapters/Optimism_Adapter.sol @@ -19,6 +19,8 @@ import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; * For example, the HubPool will delegatecall these functions, therefore its only necessary that the HubPool's methods * that call this contract's logic guard against reentrancy. */ + +// solhint-disable-next-linecontract-name-camelcase contract Optimism_Adapter is CrossDomainEnabled, AdapterInterface { using SafeERC20 for IERC20; uint32 public immutable l2GasLimit = 5_000_000; diff --git a/contracts/chain-adapters/Polygon_Adapter.sol b/contracts/chain-adapters/Polygon_Adapter.sol index 63cef029c..940ac04ad 100644 --- a/contracts/chain-adapters/Polygon_Adapter.sol +++ b/contracts/chain-adapters/Polygon_Adapter.sol @@ -28,6 +28,8 @@ interface IFxStateSender { * For example, the HubPool will delegatecall these functions, therefore its only necessary that the HubPool's methods * that call this contract's logic guard against reentrancy. */ + +// solhint-disable-next-linecontract-name-camelcase contract Polygon_Adapter is AdapterInterface { using SafeERC20 for IERC20; IRootChainManager public immutable rootChainManager; diff --git a/contracts/test/MockSpokePool.sol b/contracts/test/MockSpokePool.sol index 5572855ee..3fe65ffc2 100644 --- a/contracts/test/MockSpokePool.sol +++ b/contracts/test/MockSpokePool.sol @@ -9,18 +9,20 @@ import "../SpokePoolInterface.sol"; * @notice Implements abstract contract for testing. */ contract MockSpokePool is SpokePool { - uint256 chainId_; + uint256 private chainId_; + // solhint-disable-next-line no-empty-blocks constructor( address _crossDomainAdmin, address _hubPool, address _wethAddress, address timerAddress - ) SpokePool(_crossDomainAdmin, _hubPool, _wethAddress, timerAddress) {} + ) SpokePool(_crossDomainAdmin, _hubPool, _wethAddress, timerAddress) {} // solhint-disable-line no-empty-blocks + // solhint-disable-next-line no-empty-blocks function _bridgeTokensToHubPool(RelayerRefundLeaf memory relayerRefundLeaf) internal override {} - function _requireAdminSender() internal override {} + function _requireAdminSender() internal override {} // solhint-disable-line no-empty-blocks function chainId() public view override(SpokePool) returns (uint256) { // If chainId_ is set then return it, else do nothing and return the parent chainId(). diff --git a/contracts/test/PolygonERC20Test.sol b/contracts/test/PolygonERC20Test.sol index 2ced97548..25fb0e934 100644 --- a/contracts/test/PolygonERC20Test.sol +++ b/contracts/test/PolygonERC20Test.sol @@ -8,7 +8,7 @@ import "../PolygonTokenBridger.sol"; * @notice Simulated Polygon ERC20 for use in testing PolygonTokenBridger. */ contract PolygonERC20Test is ExpandedERC20, PolygonIERC20 { - constructor() ExpandedERC20("Polygon Test", "POLY_TEST", 18) {} + constructor() ExpandedERC20("Polygon Test", "POLY_TEST", 18) {} // solhint-disable-line no-empty-blocks function withdraw(uint256 amount) public { _burn(msg.sender, amount); diff --git a/contracts/test/PolygonMocks.sol b/contracts/test/PolygonMocks.sol index d788455d7..de14d83a6 100644 --- a/contracts/test/PolygonMocks.sol +++ b/contracts/test/PolygonMocks.sol @@ -2,15 +2,16 @@ pragma solidity ^0.8.0; contract RootChainManagerMock { - function depositEtherFor(address user) external payable {} + function depositEtherFor(address user) external payable {} // solhint-disable-line no-empty-blocks function depositFor( address user, address rootToken, bytes calldata depositData - ) external {} + ) external {} // solhint-disable-line no-empty-blocks } contract FxStateSenderMock { + // solhint-disable-next-line no-empty-blocks function sendMessageToChild(address _receiver, bytes calldata _data) external {} } diff --git a/package.json b/package.json index 8b50f31b9..269a92ba4 100644 --- a/package.json +++ b/package.json @@ -15,6 +15,7 @@ "types": "dist/index.d.ts", "main": "dist/index.js", "scripts": { + "lint-contracts": "yarn solhint ./contracts/**/*.sol", "lint": "yarn prettier --list-different", "lint-fix": "yarn prettier --write", "prettier": "prettier .", From c817ac6db2e05baf0b22444086da401a49e03dce Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 21 Mar 2022 21:16:23 -0400 Subject: [PATCH 3/7] add ci --- .github/workflows/solhint.yml | 20 ++++++++++++++++++++ .github/workflows/testGasAnalytics.yml | 25 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+) create mode 100644 .github/workflows/solhint.yml create mode 100644 .github/workflows/testGasAnalytics.yml diff --git a/.github/workflows/solhint.yml b/.github/workflows/solhint.yml new file mode 100644 index 000000000..1cb2a1584 --- /dev/null +++ b/.github/workflows/solhint.yml @@ -0,0 +1,20 @@ +name: CI +on: [push] +jobs: + build: + name: Solhint + runs-on: ubuntu-latest + steps: + - name: Checkout repo + uses: actions/checkout@v2 + + - name: Use Node ${{ matrix.node }} + uses: actions/setup-node@v2 + with: + node-version: ${{ matrix.node }} + + - name: Install deps and build (with cache) + uses: bahmutov/npm-install@v1 + + - name: Solhint + run: yarn lint-contracts diff --git a/.github/workflows/testGasAnalytics.yml b/.github/workflows/testGasAnalytics.yml new file mode 100644 index 000000000..1aaf26a23 --- /dev/null +++ b/.github/workflows/testGasAnalytics.yml @@ -0,0 +1,25 @@ +name: CI +on: [push] +jobs: + build: + name: Test-GasAnalytics + runs-on: ubuntu-latest + steps: + - name: Checkout repo + uses: actions/checkout@v2 + + - name: Use Node ${{ matrix.node }} + uses: actions/setup-node@v2 + with: + node-version: ${{ matrix.node }} + + - name: Install Additional dependencies + run: | + sudo apt-get update -y + sudo apt-get -y install rsync libudev-dev libusb-1.0-0-dev + + - name: Install deps and build (with cache) + uses: bahmutov/npm-install@v1 + + - name: Test + run: yarn test:gas-analytics From 4a3b4293aebba8d402cdc28cf8c95160230f51b2 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 21 Mar 2022 21:17:48 -0400 Subject: [PATCH 4/7] Update ArbitrumMocks.sol --- contracts/test/ArbitrumMocks.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/test/ArbitrumMocks.sol b/contracts/test/ArbitrumMocks.sol index 9b2ab711b..230b9ee34 100644 --- a/contracts/test/ArbitrumMocks.sol +++ b/contracts/test/ArbitrumMocks.sol @@ -3,17 +3,17 @@ pragma solidity ^0.8.0; contract ArbitrumMockErc20GatewayRouter { function outboundTransfer( - address _token, - address _to, - uint256 _amount, - uint256 _maxGas, - uint256 _gasPriceBid, + address, + address, + uint256, + uint256, + uint256, bytes calldata _data ) external payable returns (bytes memory) { return _data; } - function getGateway(address _token) external view returns (address) { + function getGateway(address) external view returns (address) { return address(this); } } From 81ccb06b4350a816dca35e32cd4fca756d47dc31 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 21 Mar 2022 21:21:42 -0400 Subject: [PATCH 5/7] test that solhint CI works --- contracts/test/ArbitrumMocks.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/test/ArbitrumMocks.sol b/contracts/test/ArbitrumMocks.sol index 230b9ee34..b39a09c23 100644 --- a/contracts/test/ArbitrumMocks.sol +++ b/contracts/test/ArbitrumMocks.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: AGPL-3.0-only -pragma solidity ^0.8.0; +pragma solidity >=0.6.0; contract ArbitrumMockErc20GatewayRouter { function outboundTransfer( From 0fb96120546d6df00acca6a1a11f6dd01b996c15 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 21 Mar 2022 21:22:52 -0400 Subject: [PATCH 6/7] Revert "test that solhint CI works" This reverts commit 81ccb06b4350a816dca35e32cd4fca756d47dc31. --- contracts/test/ArbitrumMocks.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/test/ArbitrumMocks.sol b/contracts/test/ArbitrumMocks.sol index b39a09c23..230b9ee34 100644 --- a/contracts/test/ArbitrumMocks.sol +++ b/contracts/test/ArbitrumMocks.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: AGPL-3.0-only -pragma solidity >=0.6.0; +pragma solidity ^0.8.0; contract ArbitrumMockErc20GatewayRouter { function outboundTransfer( From 709bf1d99e32e5a3bea7605c218020e9d6a1e1f5 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 21 Mar 2022 21:33:15 -0400 Subject: [PATCH 7/7] Delete testGasAnalytics.yml --- .github/workflows/testGasAnalytics.yml | 25 ------------------------- 1 file changed, 25 deletions(-) delete mode 100644 .github/workflows/testGasAnalytics.yml diff --git a/.github/workflows/testGasAnalytics.yml b/.github/workflows/testGasAnalytics.yml deleted file mode 100644 index 1aaf26a23..000000000 --- a/.github/workflows/testGasAnalytics.yml +++ /dev/null @@ -1,25 +0,0 @@ -name: CI -on: [push] -jobs: - build: - name: Test-GasAnalytics - runs-on: ubuntu-latest - steps: - - name: Checkout repo - uses: actions/checkout@v2 - - - name: Use Node ${{ matrix.node }} - uses: actions/setup-node@v2 - with: - node-version: ${{ matrix.node }} - - - name: Install Additional dependencies - run: | - sudo apt-get update -y - sudo apt-get -y install rsync libudev-dev libusb-1.0-0-dev - - - name: Install deps and build (with cache) - uses: bahmutov/npm-install@v1 - - - name: Test - run: yarn test:gas-analytics