From 55f40ad7602e616367b2483b5ce57eaf7db5420d Mon Sep 17 00:00:00 2001 From: Yorke Rhodes Date: Fri, 7 Apr 2023 17:48:47 -0400 Subject: [PATCH] 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](https://github.com/hyperlane-xyz/hyperlane-monorepo/compare/034e75b6321b989d88e9c0a879018e8cbaf84f41...audit-v2) --- .github/workflows/node.yml | 3 +++ solidity/.solhint.json | 12 ++++++--- solidity/.solhintignore | 2 ++ solidity/README.md | 1 - solidity/contracts/Mailbox.sol | 3 ++- solidity/contracts/Router.sol | 11 ++++---- .../contracts/interfaces/IAggregationIsm.sol | 2 +- .../interfaces/isms/IMultisigIsm.sol | 2 +- .../aggregation/AbstractAggregationIsm.sol | 1 + .../isms/multisig/AbstractMultisigIsm.sol | 11 ++++---- .../isms/multisig/LegacyMultisigIsm.sol | 26 +++++++++++-------- solidity/contracts/libs/MetaProxy.sol | 4 +-- solidity/contracts/libs/MinimalProxy.sol | 5 ++-- .../middleware/InterchainQueryRouter.sol | 1 - .../liquidity-layer/LiquidityLayerRouter.sol | 10 +++---- .../adapters/CircleBridgeAdapter.sol | 19 +++++++------- .../adapters/PortalAdapter.sol | 2 +- solidity/package.json | 1 + solidity/test/LiquidityLayerRouter.t.sol | 2 +- 19 files changed, 67 insertions(+), 51 deletions(-) create mode 100644 solidity/.solhintignore diff --git a/.github/workflows/node.yml b/.github/workflows/node.yml index aa433ba67..5933b326a 100644 --- a/.github/workflows/node.yml +++ b/.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 diff --git a/solidity/.solhint.json b/solidity/.solhint.json index 9c3fb3e5d..2cb9a69a6 100644 --- a/solidity/.solhint.json +++ b/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"] } diff --git a/solidity/.solhintignore b/solidity/.solhintignore new file mode 100644 index 000000000..9a4af1ecb --- /dev/null +++ b/solidity/.solhintignore @@ -0,0 +1,2 @@ +contracts/mock +contracts/test diff --git a/solidity/README.md b/solidity/README.md index 3e15b532c..a5795e809 100644 --- a/solidity/README.md +++ b/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 diff --git a/solidity/contracts/Mailbox.sol b/solidity/contracts/Mailbox.sol index 6512b2789..b6b5ab5a3 100644 --- a/solidity/contracts/Mailbox.sol +++ b/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; } diff --git a/solidity/contracts/Router.sol b/solidity/contracts/Router.sol index 256615a14..be8418bf1 100644 --- a/solidity/contracts/Router.sol +++ b/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); - for (uint256 i = 0; i < rawKeys.length; i++) { + uint256 length = rawKeys.length; + 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); } diff --git a/solidity/contracts/interfaces/IAggregationIsm.sol b/solidity/contracts/interfaces/IAggregationIsm.sol index 42bf0ffc5..4f7105c34 100644 --- a/solidity/contracts/interfaces/IAggregationIsm.sol +++ b/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"; diff --git a/solidity/contracts/interfaces/isms/IMultisigIsm.sol b/solidity/contracts/interfaces/isms/IMultisigIsm.sol index 1b5bcee2d..0d5c0f1af 100644 --- a/solidity/contracts/interfaces/isms/IMultisigIsm.sol +++ b/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"; diff --git a/solidity/contracts/isms/aggregation/AbstractAggregationIsm.sol b/solidity/contracts/isms/aggregation/AbstractAggregationIsm.sol index 4fbdd309a..ea85f25bf 100644 --- a/solidity/contracts/isms/aggregation/AbstractAggregationIsm.sol +++ b/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); diff --git a/solidity/contracts/isms/multisig/AbstractMultisigIsm.sol b/solidity/contracts/isms/multisig/AbstractMultisigIsm.sol index a0db0463f..e792b23ae 100644 --- a/solidity/contracts/isms/multisig/AbstractMultisigIsm.sol +++ b/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]; - ++_validatorIndex - ) {} + _signer != _validators[_validatorIndex] + ) { + ++_validatorIndex; + } // Fail if we never found a match require(_validatorIndex < _validatorCount, "!threshold"); ++_validatorIndex; diff --git a/solidity/contracts/isms/multisig/LegacyMultisigIsm.sol b/solidity/contracts/isms/multisig/LegacyMultisigIsm.sol index 4ed979fb7..521875857 100644 --- a/solidity/contracts/isms/multisig/LegacyMultisigIsm.sol +++ b/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"); - for (uint256 i = 0; i < _domains.length; i += 1) { + uint256 domainsLength = _domains.length; + 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"); - for (uint256 i = 0; i < _domains.length; i += 1) { + uint256 length = _domains.length; + 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); - ++_validatorIndex - ) {} + _signer != _metadata.validatorAt(_validatorIndex) + ) { + ++_validatorIndex; + } // Fail if we never found a match require(_validatorIndex < _validatorCount, "!threshold"); ++_validatorIndex; diff --git a/solidity/contracts/libs/MetaProxy.sol b/solidity/contracts/libs/MetaProxy.sol index 5a586ee64..eabd59e13 100644 --- a/solidity/contracts/libs/MetaProxy.sol +++ b/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 diff --git a/solidity/contracts/libs/MinimalProxy.sol b/solidity/contracts/libs/MinimalProxy.sol index a09924eff..fc26a91ac 100644 --- a/solidity/contracts/libs/MinimalProxy.sol +++ b/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"; - bytes15 constant SUFFIX = hex"5af43d82803e903d91602b57fd5bf3"; + bytes20 private constant PREFIX = + hex"3d602d80600a3d3981f3363d3d373d3d3d363d73"; + bytes15 private constant SUFFIX = hex"5af43d82803e903d91602b57fd5bf3"; function bytecode(address implementation) internal diff --git a/solidity/contracts/middleware/InterchainQueryRouter.sol b/solidity/contracts/middleware/InterchainQueryRouter.sol index da8df22ff..e917e5183 100644 --- a/solidity/contracts/middleware/InterchainQueryRouter.sol +++ b/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); diff --git a/solidity/contracts/middleware/liquidity-layer/LiquidityLayerRouter.sol b/solidity/contracts/middleware/liquidity-layer/LiquidityLayerRouter.sol index e8348a18e..534ce7510 100644 --- a/solidity/contracts/middleware/liquidity-layer/LiquidityLayerRouter.sol +++ b/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 - // TODO: Are there scenarios where a transferFrom fails and it doesn't revert? - require( - IERC20(_token).transferFrom(msg.sender, address(_adapter), _amount), - "!transfer in" - ); + IERC20(_token).safeTransferFrom(msg.sender, address(_adapter), _amount); // Reverts if the bridge was unsuccessful. // Gets adapter-specific data that is encoded into the message diff --git a/solidity/contracts/middleware/liquidity-layer/adapters/CircleBridgeAdapter.sol b/solidity/contracts/middleware/liquidity-layer/adapters/CircleBridgeAdapter.sol index 13150bff3..4a2041906 100644 --- a/solidity/contracts/middleware/liquidity-layer/adapters/CircleBridgeAdapter.sol +++ b/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 { - // Transfer ownership of the contract to deployer + ) external initializer { + __Ownable_init(); _transferOwnership(_owner); tokenMessenger = ITokenMessenger(_tokenMessenger); @@ -96,11 +99,7 @@ contract CircleBridgeAdapter is ILiquidityLayerAdapter, Router { uint32 _circleDomain = hyperlaneDomainToCircleDomain[ _destinationDomain ]; - bytes32 _remoteRouter = routers(_destinationDomain); - require( - _remoteRouter != bytes32(0), - "CircleBridgeAdapter: No router for domain" - ); + bytes32 _remoteRouter = _mustHaveRemoteRouter(_destinationDomain); // 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); } diff --git a/solidity/contracts/middleware/liquidity-layer/adapters/PortalAdapter.sol b/solidity/contracts/middleware/liquidity-layer/adapters/PortalAdapter.sol index c6688d9a5..fdacca767 100644 --- a/solidity/contracts/middleware/liquidity-layer/adapters/PortalAdapter.sol +++ b/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 diff --git a/solidity/package.json b/solidity/package.json index 141f47758..4e6f005b6 100644 --- a/solidity/package.json +++ b/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", diff --git a/solidity/test/LiquidityLayerRouter.t.sol b/solidity/test/LiquidityLayerRouter.t.sol index b84def49e..d9e970ba5 100644 --- a/solidity/test/LiquidityLayerRouter.t.sol +++ b/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)) );