Merge audit v2 remediation to main (#1727)

### Description

Merge
[`audit-v2`](https://github.com/hyperlane-xyz/hyperlane-monorepo/releases/tag/audit-v2)
into `main`

See [changelog](034e75b632...audit-v2)

.github/workflows/node.yml

@@ -161,6 +161,9 @@ jobs:
       - name: Forge build
         run: cd solidity && forge build --build-info
 
+      - name: lint
+        run: yarn workspace @hyperlane-xyz/core run lint
+
       - name: Run Slither
         uses: crytic/slither-action@main
         id: slither

solidity/.solhint.json

@@ -1,10 +1,16 @@
 {
   "extends": "solhint:recommended",
   "rules": {
-    "compiler-version": ["error", "^0.6.11"],
+    "compiler-version": ["error", ">=0.6.11"],
     "func-visibility": ["warn", {"ignoreConstructors":true}],
     "not-rely-on-time": "off",
     "avoid-low-level-calls": "off",
-    "no-inline-assembly": "off"
+    "no-inline-assembly": "off",
-  }
+    "code-complexity": ["warn", 4],
+    "var-name-mixedcase": "off",
+    "func-name-mixedcase": "off",
+    "reason-string": ["warn",{"maxLength":64}],
+    "prettier/prettier": "error"
+  },
+  "plugins": ["prettier"]
 }

solidity/.solhintignore

@@ -0,0 +1,2 @@
+contracts/mock
+contracts/test

solidity/README.md

@@ -6,7 +6,6 @@ On-chain implementations of Hyperlane in Solidity.
 
 - `npm install --dev`
 - `brew install jq`   OR   `sudo apt-get install jq`
-- `npm install -g solhint`, check it is installed using `solhint --version`
 
 ## Build
 

solidity/contracts/Mailbox.sol

@@ -189,7 +189,7 @@ contract Mailbox is
      * @return root The root of the Mailbox's merkle tree.
      * @return index The index of the last element in the tree.
      */
-    function latestCheckpoint() public view returns (bytes32, uint32) {
+    function latestCheckpoint() external view returns (bytes32, uint32) {
         return (root(), count() - 1);
     }
 
@@ -241,6 +241,7 @@ contract Mailbox is
             if (address(_val) != address(0)) {
                 return _val;
             }
+            // solhint-disable-next-line no-empty-blocks
         } catch {}
         return defaultIsm;
     }

solidity/contracts/Router.sol

@@ -11,7 +11,7 @@ import {EnumerableMapExtended} from "./libs/EnumerableMapExtended.sol";
 abstract contract Router is HyperlaneConnectionClient, IMessageRecipient {
     using EnumerableMapExtended for EnumerableMapExtended.UintToBytes32Map;
 
-    string constant NO_ROUTER_ENROLLED_REVERT_MESSAGE =
+    string private constant NO_ROUTER_ENROLLED_REVERT_MESSAGE =
         "No router enrolled for domain. Did you specify the right domain ID?";
 
     // ============ Mutable Storage ============
@@ -71,8 +71,9 @@ abstract contract Router is HyperlaneConnectionClient, IMessageRecipient {
     // ============ External functions ============
     function domains() external view returns (uint32[] memory) {
         bytes32[] storage rawKeys = _routers.keys();
-        uint32[] memory keys = new uint32[](rawKeys.length);
+        uint256 length = rawKeys.length;
-        for (uint256 i = 0; i < rawKeys.length; i++) {
+        uint32[] memory keys = new uint32[](length);
+        for (uint256 i = 0; i < length; i++) {
             keys[i] = uint32(uint256(rawKeys[i]));
         }
         return keys;
@@ -109,7 +110,8 @@ abstract contract Router is HyperlaneConnectionClient, IMessageRecipient {
         bytes32[] calldata _addresses
     ) external virtual onlyOwner {
         require(_domains.length == _addresses.length, "!length");
-        for (uint256 i = 0; i < _domains.length; i += 1) {
+        uint256 length = _domains.length;
+        for (uint256 i = 0; i < length; i += 1) {
             _enrollRemoteRouter(_domains[i], _addresses[i]);
         }
     }
@@ -125,7 +127,6 @@ abstract contract Router is HyperlaneConnectionClient, IMessageRecipient {
         bytes32 _sender,
         bytes calldata _message
     ) external virtual override onlyMailbox onlyRemoteRouter(_origin, _sender) {
-        // TODO: callbacks on success/failure
         _handle(_origin, _sender, _message);
     }
 

solidity/contracts/interfaces/IAggregationIsm.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
-pragma solidity >=0.6.0;
+pragma solidity >=0.6.11;
 
 import {IInterchainSecurityModule} from "./IInterchainSecurityModule.sol";
 

solidity/contracts/interfaces/isms/IMultisigIsm.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
-pragma solidity >=0.6.0;
+pragma solidity >=0.6.11;
 
 import {IInterchainSecurityModule} from "../IInterchainSecurityModule.sol";
 

solidity/contracts/isms/aggregation/AbstractAggregationIsm.sol

@@ -17,6 +17,7 @@ import {AggregationIsmMetadata} from "../../libs/isms/AggregationIsmMetadata.sol
 abstract contract AbstractAggregationIsm is IAggregationIsm {
     // ============ Constants ============
 
+    // solhint-disable-next-line const-name-snakecase
     uint8 public constant moduleType =
         uint8(IInterchainSecurityModule.Types.MULTISIG);
 

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol

@@ -20,6 +20,7 @@ import {MerkleLib} from "../../libs/Merkle.sol";
 abstract contract AbstractMultisigIsm is IMultisigIsm {
     // ============ Constants ============
 
+    // solhint-disable-next-line const-name-snakecase
     uint8 public constant moduleType =
         uint8(IInterchainSecurityModule.Types.MULTISIG);
 
@@ -109,12 +110,12 @@ abstract contract AbstractMultisigIsm is IMultisigIsm {
                 MultisigIsmMetadata.signatureAt(_metadata, i)
             );
             // Loop through remaining validators until we find a match
-            for (
+            while (
-                ;
                 _validatorIndex < _validatorCount &&
-                    _signer != _validators[_validatorIndex];
+                _signer != _validators[_validatorIndex]
-                ++_validatorIndex
+            ) {
-            ) {}
+                ++_validatorIndex;
+            }
             // Fail if we never found a match
             require(_validatorIndex < _validatorCount, "!threshold");
             ++_validatorIndex;

solidity/contracts/isms/multisig/LegacyMultisigIsm.sol

@@ -29,6 +29,7 @@ contract LegacyMultisigIsm is IMultisigIsm, Ownable {
 
     // ============ Constants ============
 
+    // solhint-disable-next-line const-name-snakecase
     uint8 public constant moduleType =
         uint8(IInterchainSecurityModule.Types.LEGACY_MULTISIG);
 
@@ -102,10 +103,12 @@ contract LegacyMultisigIsm is IMultisigIsm, Ownable {
         uint32[] calldata _domains,
         address[][] calldata _validators
     ) external onlyOwner {
-        require(_domains.length == _validators.length, "!length");
+        uint256 domainsLength = _domains.length;
-        for (uint256 i = 0; i < _domains.length; i += 1) {
+        require(domainsLength == _validators.length, "!length");
+        for (uint256 i = 0; i < domainsLength; i += 1) {
             address[] calldata _domainValidators = _validators[i];
-            for (uint256 j = 0; j < _domainValidators.length; j += 1) {
+            uint256 validatorsLength = _domainValidators.length;
+            for (uint256 j = 0; j < validatorsLength; j += 1) {
                 _enrollValidator(_domains[i], _domainValidators[j]);
             }
             _updateCommitment(_domains[i]);
@@ -155,8 +158,9 @@ contract LegacyMultisigIsm is IMultisigIsm, Ownable {
         uint32[] calldata _domains,
         uint8[] calldata _thresholds
     ) external onlyOwner {
-        require(_domains.length == _thresholds.length, "!length");
+        uint256 length = _domains.length;
-        for (uint256 i = 0; i < _domains.length; i += 1) {
+        require(length == _thresholds.length, "!length");
+        for (uint256 i = 0; i < length; i += 1) {
             setThreshold(_domains[i], _thresholds[i]);
         }
     }
@@ -202,7 +206,7 @@ contract LegacyMultisigIsm is IMultisigIsm, Ownable {
      * @param _message Formatted Hyperlane message (see Message.sol).
      */
     function verify(bytes calldata _metadata, bytes calldata _message)
-        public
+        external
         view
         returns (bool)
     {
@@ -340,12 +344,12 @@ contract LegacyMultisigIsm is IMultisigIsm, Ownable {
         for (uint256 i = 0; i < _threshold; ++i) {
             address _signer = ECDSA.recover(_digest, _metadata.signatureAt(i));
             // Loop through remaining validators until we find a match
-            for (
+            while (
-                ;
                 _validatorIndex < _validatorCount &&
-                    _signer != _metadata.validatorAt(_validatorIndex);
+                _signer != _metadata.validatorAt(_validatorIndex)
-                ++_validatorIndex
+            ) {
-            ) {}
+                ++_validatorIndex;
+            }
             // Fail if we never found a match
             require(_validatorIndex < _validatorCount, "!threshold");
             ++_validatorIndex;

solidity/contracts/libs/MetaProxy.sol

@@ -3,9 +3,9 @@ pragma solidity >=0.7.6;
 
 /// @dev Adapted from https://eips.ethereum.org/EIPS/eip-3448
 library MetaProxy {
-    bytes32 constant PREFIX =
+    bytes32 private constant PREFIX =
         hex"600b380380600b3d393df3363d3d373d3d3d3d60368038038091363936013d73";
-    bytes13 constant SUFFIX = hex"5af43d3d93803e603457fd5bf3";
+    bytes13 private constant SUFFIX = hex"5af43d3d93803e603457fd5bf3";
 
     function bytecode(address _implementation, bytes memory _metadata)
         internal

solidity/contracts/libs/MinimalProxy.sol

@@ -3,8 +3,9 @@ pragma solidity >=0.6.11;
 
 // Library for building bytecode of minimal proxies (see https://eips.ethereum.org/EIPS/eip-1167)
 library MinimalProxy {
-    bytes20 constant PREFIX = hex"3d602d80600a3d3981f3363d3d373d3d3d363d73";
+    bytes20 private constant PREFIX =
-    bytes15 constant SUFFIX = hex"5af43d82803e903d91602b57fd5bf3";
+        hex"3d602d80600a3d3981f3363d3d373d3d3d363d73";
+    bytes15 private constant SUFFIX = hex"5af43d82803e903d91602b57fd5bf3";
 
     function bytecode(address implementation)
         internal

solidity/contracts/middleware/InterchainQueryRouter.sol

@@ -136,7 +136,6 @@ contract InterchainQueryRouter is Router, IInterchainQueryRouter {
             address senderAddress = sender.bytes32ToAddress();
             bytes[] memory rawCalls = InterchainQueryMessage.rawCalls(_message);
             CallLib.multicallto(senderAddress, rawCalls);
-            //
             emit QueryResolved(_origin, senderAddress);
         } else {
             assert(false);

solidity/contracts/middleware/liquidity-layer/LiquidityLayerRouter.sol

@@ -11,8 +11,11 @@ import {ILiquidityLayerMessageRecipient} from "../../interfaces/ILiquidityLayerM
 import {TypeCasts} from "../../libs/TypeCasts.sol";
 
 import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 
 contract LiquidityLayerRouter is Router, ILiquidityLayerRouter {
+    using SafeERC20 for IERC20;
+
     // Token bridge => adapter address
     mapping(string => address) public liquidityLayerAdapters;
 
@@ -50,12 +53,7 @@ contract LiquidityLayerRouter is Router, ILiquidityLayerRouter {
         ILiquidityLayerAdapter _adapter = _getAdapter(_bridge);
 
         // Transfer the tokens to the adapter
-        // TODO: use safeTransferFrom
+        IERC20(_token).safeTransferFrom(msg.sender, address(_adapter), _amount);
-        // TODO: Are there scenarios where a transferFrom fails and it doesn't revert?
-        require(
-            IERC20(_token).transferFrom(msg.sender, address(_adapter), _amount),
-            "!transfer in"
-        );
 
         // Reverts if the bridge was unsuccessful.
         // Gets adapter-specific data that is encoded into the message

solidity/contracts/middleware/liquidity-layer/adapters/CircleBridgeAdapter.sol

@@ -8,8 +8,11 @@ import {ICircleMessageTransmitter} from "../interfaces/circle/ICircleMessageTran
 import {ILiquidityLayerAdapter} from "../interfaces/ILiquidityLayerAdapter.sol";
 
 import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 
 contract CircleBridgeAdapter is ILiquidityLayerAdapter, Router {
+    using SafeERC20 for IERC20;
+
     /// @notice The TokenMessenger contract.
     ITokenMessenger public tokenMessenger;
 
@@ -22,7 +25,7 @@ contract CircleBridgeAdapter is ILiquidityLayerAdapter, Router {
     /// @notice Hyperlane domain => Circle domain.
     /// ATM, known Circle domains are Ethereum = 0 and Avalanche = 1.
     /// Note this could result in ambiguity between the Circle domain being
-    /// Ethereum or unknown. TODO fix?
+    /// Ethereum or unknown.
     mapping(uint32 => uint32) public hyperlaneDomainToCircleDomain;
 
     /// @notice Token symbol => address of token on local chain.
@@ -70,8 +73,8 @@ contract CircleBridgeAdapter is ILiquidityLayerAdapter, Router {
         address _tokenMessenger,
         address _circleMessageTransmitter,
         address _liquidityLayerRouter
-    ) public initializer {
+    ) external initializer {
-        // Transfer ownership of the contract to deployer
+        __Ownable_init();
         _transferOwnership(_owner);
 
         tokenMessenger = ITokenMessenger(_tokenMessenger);
@@ -96,11 +99,7 @@ contract CircleBridgeAdapter is ILiquidityLayerAdapter, Router {
         uint32 _circleDomain = hyperlaneDomainToCircleDomain[
             _destinationDomain
         ];
-        bytes32 _remoteRouter = routers(_destinationDomain);
+        bytes32 _remoteRouter = _mustHaveRemoteRouter(_destinationDomain);
-        require(
-            _remoteRouter != bytes32(0),
-            "CircleBridgeAdapter: No router for domain"
-        );
 
         // Approve the token to Circle. We assume that the LiquidityLayerRouter
         // has already transferred the token to this contract.
@@ -127,6 +126,7 @@ contract CircleBridgeAdapter is ILiquidityLayerAdapter, Router {
         uint256 _amount,
         bytes calldata _adapterData // The adapter data from the message
     ) external onlyLiquidityLayerRouter returns (address, uint256) {
+        _mustHaveRemoteRouter(_originDomain);
         // The origin Circle domain
         uint32 _originCircleDomain = hyperlaneDomainToCircleDomain[
             _originDomain
@@ -151,10 +151,9 @@ contract CircleBridgeAdapter is ILiquidityLayerAdapter, Router {
         );
 
         // Transfer the token out to the recipient
-        // TODO: use safeTransfer
         // Circle doesn't charge any fee, so we can safely transfer out the
         // exact amount that was bridged over.
-        require(_token.transfer(_recipient, _amount), "!transfer out");
+        _token.safeTransfer(_recipient, _amount);
 
         return (address(_token), _amount);
     }

solidity/contracts/middleware/liquidity-layer/adapters/PortalAdapter.sol

@@ -21,7 +21,7 @@ contract PortalAdapter is ILiquidityLayerAdapter, Router {
     /// @notice transferId => token address
     mapping(bytes32 => address) public portalTransfersProcessed;
 
-    uint32 localDomain;
+    uint32 public localDomain;
 
     // We could technically use Portal's sequence number here but it doesn't
     // get passed through, so we would have to parse the VAA twice

solidity/package.json

@@ -44,6 +44,7 @@
   "repository": "https://github.com/hyperlane-xyz/hyperlane-monorepo",
   "scripts": {
     "build": "hardhat compile && tsc",
+    "lint": "solhint contracts/**/*.sol",
     "clean": "hardhat clean && rm -rf ./dist ./cache",
     "coverage": "./coverage.sh",
     "prettier": "prettier --write ./contracts ./test",

solidity/test/LiquidityLayerRouter.t.sol

@@ -99,7 +99,7 @@ contract LiquidityLayerRouterTest is Test {
             TypeCasts.addressToBytes32(address(destinationBridgeAdapter))
         );
         destinationBridgeAdapter.enrollRemoteRouter(
-            destinationDomain,
+            originDomain,
             TypeCasts.addressToBytes32(address(originBridgeAdapter))
         );