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/contracts/HubPool.sol b/contracts/HubPool.sol index 3ee9f0c9e..b2369166f 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -662,6 +662,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)", @@ -817,7 +821,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 * @@ -866,6 +872,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)", @@ -992,6 +1002,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)", diff --git a/contracts/chain-adapters/Arbitrum_Adapter.sol b/contracts/chain-adapters/Arbitrum_Adapter.sol index d06f59c5f..c9ccbc39c 100644 --- a/contracts/chain-adapters/Arbitrum_Adapter.sol +++ b/contracts/chain-adapters/Arbitrum_Adapter.sol @@ -39,6 +39,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 { using SafeERC20 for IERC20; diff --git a/contracts/chain-adapters/CrossDomainEnabled.sol b/contracts/chain-adapters/CrossDomainEnabled.sol index d9cd759a0..adf613e8d 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 bdf58581a..f0b87432c 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 6889413fd..3e5f209fd 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 e9c1e1839..b01b141f6 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 = 2_000_000; diff --git a/contracts/chain-adapters/Polygon_Adapter.sol b/contracts/chain-adapters/Polygon_Adapter.sol index 764fe655d..aa307a8a1 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/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); } } 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 .",