From e61e6ed58556ac6a4672a9964aa509f211ec7386 Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Thu, 22 Aug 2024 15:57:14 +1000 Subject: [PATCH 01/18] Multi-caller v2 --- contracts/multicall/GuardedMulticallerV2.sol | 271 +++++++++++++++++++ 1 file changed, 271 insertions(+) create mode 100644 contracts/multicall/GuardedMulticallerV2.sol diff --git a/contracts/multicall/GuardedMulticallerV2.sol b/contracts/multicall/GuardedMulticallerV2.sol new file mode 100644 index 00000000..086f5e89 --- /dev/null +++ b/contracts/multicall/GuardedMulticallerV2.sol @@ -0,0 +1,271 @@ +// Copyright Immutable Pty Ltd 2018 - 2023 +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.19; + +// Signature Validation +import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; + +// Access Control +import {AccessControl} from "@openzeppelin/contracts/access/AccessControl.sol"; + +// Reentrancy Guard +import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; + +// EIP-712 Typed Structs +import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; + +/** + * + * @title GuardedMulticaller contract + * @author Immutable Game Studio + * @notice This contract is used to batch calls to other contracts. + * @dev This contract is not designed to be upgradeable. If an issue is found with this contract, + * a new version will be deployed. All approvals granted to this contract will be revoked before + * a new version is deployed. Approvals will be granted to the new contract. + */ +contract GuardedMulticaller is AccessControl, ReentrancyGuard, EIP712 { + /// @dev Mapping of reference to executed status + // solhint-disable-next-line named-parameters-mapping + mapping(bytes32 => bool) private replayProtection; + + /// @dev Only those with MULTICALL_SIGNER_ROLE can generate valid signatures for execute function. + bytes32 public constant MULTICALL_SIGNER_ROLE = bytes32("MULTICALL_SIGNER_ROLE"); + + /// @dev EIP712 typehash for execute function + bytes32 internal constant MULTICALL_TYPEHASH = + keccak256("Multicall(bytes32 ref,address[] targets,bytes[] data,uint256 deadline)"); + + /// @dev Event emitted when execute function is called + event Multicalled( + address indexed _multicallSigner, + bytes32 indexed _reference, + address[] _targets, + bytes[] _data, + uint256 _deadline + ); + + /// @dev Error thrown when reference is invalid + error InvalidReference(bytes32 _reference); + + /// @dev Error thrown when reference has already been executed + error ReusedReference(bytes32 _reference); + + /// @dev Error thrown when address array is empty + error EmptyAddressArray(); + + /// @dev Error thrown when address array and data array have different lengths + error AddressDataArrayLengthsMismatch(uint256 _addressLength, uint256 _dataLength); + + /// @dev Error thrown when deadline is expired + error Expired(uint256 _deadline); + + /// @dev Error thrown when target address is not a contract + error NonContractAddress(address _target); + + /// @dev Error thrown when signer is not authorized + error UnauthorizedSigner(address _multicallSigner); + + /// @dev Error thrown when signature is invalid + error UnauthorizedSignature(bytes _signature); + + /// @dev Error thrown when call reverts + error FailedCall(address _target, string functionSignature, bytes _data); + + /// @dev Error thrown when call data is invalid + error InvalidCallData(address _target, bytes _data); + + /// @dev Error thrown when call data is unauthorized + error UnauthorizedFunction(address _target, string _functionSignature); + + /** + * + * @notice Grants DEFAULT_ADMIN_ROLE to the contract creator + * @param _owner Owner of the contract + * @param _name Name of the contract + * @param _version Version of the contract + */ + // solhint-disable-next-line no-unused-vars + constructor(address _owner, string memory _name, string memory _version) EIP712(_name, _version) { + _grantRole(DEFAULT_ADMIN_ROLE, _owner); + } + + /** + * + * @dev Returns hash of array of bytes + * + * @param _data Array of bytes + */ + function hashBytesArray(bytes[] memory _data) public pure returns (bytes32) { + bytes32[] memory hashedBytesArr = new bytes32[](_data.length); + for (uint256 i = 0; i < _data.length; i++) { + hashedBytesArr[i] = keccak256(_data[i]); + } + return keccak256(abi.encodePacked(hashedBytesArr)); + } + + /** + * + * @dev Returns hash of array of strings + * + * @param _data Array of strings + */ + function hashStringArray(string[] calldata _data) public pure returns (bytes32) { + bytes32[] memory hashedStringArr = new bytes32[](_data.length); + for (uint256 i = 0; i < _data.length; i++) { + hashedStringArr[i] = keccak256(bytes(_data[i])); + } + return keccak256(abi.encodePacked(hashedStringArr)); + } + + function stringBytes(string calldata _data) public pure returns (bytes memory) { + return bytes(_data); + } + + /** + * + * @notice Execute a list of calls. Returned data from calls are ignored. + * The signature must be generated by an address with EXECUTION_MULTICALL_SIGNER_ROLE + * The signature must be valid + * The signature must not be expired + * The reference must be unique + * The reference must not be executed before + * The list of calls must not be empty + * The list of calls is executed in order + * + * @param _multicallSigner Address of an approved signer + * @param _reference Reference + * @param _targets List of addresses to call + * @param _functionSignatures List of function signatures + * @param _data List of call data + * @param _deadline Expiration timestamp + * @param _signature Signature of the multicall signer + */ + // slither-disable-start low-level-calls,cyclomatic-complexity + // solhint-disable-next-line code-complexity + function execute( + address _multicallSigner, + bytes32 _reference, + address[] calldata _targets, + string[] calldata _functionSignatures, + bytes[] calldata _data, + uint256 _deadline, + bytes calldata _signature + ) external nonReentrant { + // solhint-disable-next-line not-rely-on-time + if (_deadline < block.timestamp) { + revert Expired(_deadline); + } + if (_reference == 0) { + revert InvalidReference(_reference); + } + if (replayProtection[_reference]) { + revert ReusedReference(_reference); + } + if (_targets.length == 0) { + revert EmptyAddressArray(); + } + if (_targets.length != _data.length) { + revert AddressDataArrayLengthsMismatch(_targets.length, _data.length); + } + for (uint256 i = 0; i < _targets.length; i++) { + if (_targets[i].code.length == 0) { + revert NonContractAddress(_targets[i]); + } + } + if (!hasRole(MULTICALL_SIGNER_ROLE, _multicallSigner)) { + revert UnauthorizedSigner(_multicallSigner); + } + + // Signature validation + if ( + !SignatureChecker.isValidSignatureNow( + _multicallSigner, + _hashTypedData(_reference, _targets, _functionSignatures, _data, _deadline), + _signature + ) + ) { + revert UnauthorizedSignature(_signature); + } + + replayProtection[_reference] = true; + + // Multicall + for (uint256 i = 0; i < _targets.length; i++) { + bytes4 functionSelector = bytes4(keccak256(bytes(_functionSignatures[i]))); + bytes memory callData = abi.encodePacked(functionSelector, _data[i]); + // solhint-disable avoid-low-level-calls + // slither-disable-next-line calls-loop + (bool success, bytes memory returnData) = _targets[i].call(callData); + if (!success) { + if (returnData.length == 0) { + revert FailedCall(_targets[i], _functionSignatures[i], _data[i]); + } + // solhint-disable-next-line no-inline-assembly + assembly { + revert(add(returnData, 32), mload(returnData)) + } + } + } + + emit Multicalled(_multicallSigner, _reference, _targets, _data, _deadline); + } + // slither-disable-end low-level-calls,cyclomatic-complexity + + /** + * @notice Grants MULTICALL_SIGNER_ROLE to a user. Only DEFAULT_ADMIN_ROLE can call this function. + * + * @param _user User to grant MULTICALL_SIGNER_ROLE to + */ + function grantMulticallSignerRole(address _user) external onlyRole(DEFAULT_ADMIN_ROLE) { + grantRole(MULTICALL_SIGNER_ROLE, _user); + } + + /** + * @notice Revokes MULTICALL_SIGNER_ROLE for a user. Only DEFAULT_ADMIN_ROLE can call this function. + * + * @param _user User to grant MULTICALL_SIGNER_ROLE to + */ + function revokeMulticallSignerRole(address _user) external onlyRole(DEFAULT_ADMIN_ROLE) { + revokeRole(MULTICALL_SIGNER_ROLE, _user); + } + + /** + * @notice Gets whether the reference has been executed before. + * + * @param _reference Reference to check + */ + function hasBeenExecuted(bytes32 _reference) external view returns (bool) { + return replayProtection[_reference]; + } + + /** + * + * @dev Returns EIP712 message hash for given parameters + * + * @param _reference Reference + * @param _targets List of addresses to call + * @param _data List of call data + * @param _deadline Expiration timestamp + */ + function _hashTypedData( + bytes32 _reference, + address[] calldata _targets, + string[] calldata _functionSignatures, + bytes[] calldata _data, + uint256 _deadline + ) internal view returns (bytes32) { + return + _hashTypedDataV4( + keccak256( + abi.encode( + MULTICALL_TYPEHASH, + _reference, + keccak256(abi.encodePacked(_targets)), + hashStringArray(_functionSignatures), + hashBytesArray(_data), + _deadline + ) + ) + ); + } +} From ff0016b2454d789893259101a541ebaa09bc51bb Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Thu, 22 Aug 2024 15:58:38 +1000 Subject: [PATCH 02/18] Update names --- contracts/multicall/GuardedMulticallerV2.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/multicall/GuardedMulticallerV2.sol b/contracts/multicall/GuardedMulticallerV2.sol index 086f5e89..30cad8e5 100644 --- a/contracts/multicall/GuardedMulticallerV2.sol +++ b/contracts/multicall/GuardedMulticallerV2.sol @@ -16,14 +16,14 @@ import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; /** * - * @title GuardedMulticaller contract - * @author Immutable Game Studio + * @title GuardedMulticallerV2 contract + * @author Immutable * @notice This contract is used to batch calls to other contracts. * @dev This contract is not designed to be upgradeable. If an issue is found with this contract, * a new version will be deployed. All approvals granted to this contract will be revoked before * a new version is deployed. Approvals will be granted to the new contract. */ -contract GuardedMulticaller is AccessControl, ReentrancyGuard, EIP712 { +contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { /// @dev Mapping of reference to executed status // solhint-disable-next-line named-parameters-mapping mapping(bytes32 => bool) private replayProtection; From a246b9f030d85ff951fe3752b286c92bc4f5b488 Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Thu, 22 Aug 2024 16:15:26 +1000 Subject: [PATCH 03/18] Use struct --- contracts/multicall/GuardedMulticallerV2.sol | 96 ++++++++------------ 1 file changed, 36 insertions(+), 60 deletions(-) diff --git a/contracts/multicall/GuardedMulticallerV2.sol b/contracts/multicall/GuardedMulticallerV2.sol index 30cad8e5..b4ec515b 100644 --- a/contracts/multicall/GuardedMulticallerV2.sol +++ b/contracts/multicall/GuardedMulticallerV2.sol @@ -24,6 +24,13 @@ import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; * a new version is deployed. Approvals will be granted to the new contract. */ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { + /// @dev Struct for call data + struct Call { + address target; + string functionSignature; + bytes data; + } + /// @dev Mapping of reference to executed status // solhint-disable-next-line named-parameters-mapping mapping(bytes32 => bool) private replayProtection; @@ -39,8 +46,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { event Multicalled( address indexed _multicallSigner, bytes32 indexed _reference, - address[] _targets, - bytes[] _data, + Call[] _calls, uint256 _deadline ); @@ -50,8 +56,8 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { /// @dev Error thrown when reference has already been executed error ReusedReference(bytes32 _reference); - /// @dev Error thrown when address array is empty - error EmptyAddressArray(); + /// @dev Error thrown when call array is empty + error EmptyCallArray(); /// @dev Error thrown when address array and data array have different lengths error AddressDataArrayLengthsMismatch(uint256 _addressLength, uint256 _dataLength); @@ -60,7 +66,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { error Expired(uint256 _deadline); /// @dev Error thrown when target address is not a contract - error NonContractAddress(address _target); + error NonContractAddress(Call _call); /// @dev Error thrown when signer is not authorized error UnauthorizedSigner(address _multicallSigner); @@ -69,7 +75,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { error UnauthorizedSignature(bytes _signature); /// @dev Error thrown when call reverts - error FailedCall(address _target, string functionSignature, bytes _data); + error FailedCall(Call _call); /// @dev Error thrown when call data is invalid error InvalidCallData(address _target, bytes _data); @@ -91,34 +97,16 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { /** * - * @dev Returns hash of array of bytes - * - * @param _data Array of bytes - */ - function hashBytesArray(bytes[] memory _data) public pure returns (bytes32) { - bytes32[] memory hashedBytesArr = new bytes32[](_data.length); - for (uint256 i = 0; i < _data.length; i++) { - hashedBytesArr[i] = keccak256(_data[i]); - } - return keccak256(abi.encodePacked(hashedBytesArr)); - } - - /** - * - * @dev Returns hash of array of strings + * @dev Returns hash of array of calls * - * @param _data Array of strings + * @param _calls Array of calls */ - function hashStringArray(string[] calldata _data) public pure returns (bytes32) { - bytes32[] memory hashedStringArr = new bytes32[](_data.length); - for (uint256 i = 0; i < _data.length; i++) { - hashedStringArr[i] = keccak256(bytes(_data[i])); + function hashCallArray(Call[] calldata _calls) public pure returns (bytes32) { + bytes32[] memory hashedCallArr = new bytes32[](_calls.length); + for (uint256 i = 0; i < _calls.length; i++) { + hashedCallArr[i] = keccak256(abi.encodePacked(_calls[i].target, _calls[i].functionSignature, _calls[i].data)); } - return keccak256(abi.encodePacked(hashedStringArr)); - } - - function stringBytes(string calldata _data) public pure returns (bytes memory) { - return bytes(_data); + return keccak256(abi.encodePacked(hashedCallArr)); } /** @@ -134,9 +122,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { * * @param _multicallSigner Address of an approved signer * @param _reference Reference - * @param _targets List of addresses to call - * @param _functionSignatures List of function signatures - * @param _data List of call data + * @param _calls List of calls * @param _deadline Expiration timestamp * @param _signature Signature of the multicall signer */ @@ -145,9 +131,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { function execute( address _multicallSigner, bytes32 _reference, - address[] calldata _targets, - string[] calldata _functionSignatures, - bytes[] calldata _data, + Call[] calldata _calls, uint256 _deadline, bytes calldata _signature ) external nonReentrant { @@ -161,15 +145,12 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { if (replayProtection[_reference]) { revert ReusedReference(_reference); } - if (_targets.length == 0) { - revert EmptyAddressArray(); - } - if (_targets.length != _data.length) { - revert AddressDataArrayLengthsMismatch(_targets.length, _data.length); + if (_calls.length == 0) { + revert EmptyCallArray(); } - for (uint256 i = 0; i < _targets.length; i++) { - if (_targets[i].code.length == 0) { - revert NonContractAddress(_targets[i]); + for (uint256 i = 0; i < _calls.length; i++) { + if (_calls[i].target.code.length == 0) { + revert NonContractAddress(_calls[i]); } } if (!hasRole(MULTICALL_SIGNER_ROLE, _multicallSigner)) { @@ -180,7 +161,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { if ( !SignatureChecker.isValidSignatureNow( _multicallSigner, - _hashTypedData(_reference, _targets, _functionSignatures, _data, _deadline), + _hashTypedData(_reference, _calls, _deadline), _signature ) ) { @@ -190,15 +171,15 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { replayProtection[_reference] = true; // Multicall - for (uint256 i = 0; i < _targets.length; i++) { - bytes4 functionSelector = bytes4(keccak256(bytes(_functionSignatures[i]))); - bytes memory callData = abi.encodePacked(functionSelector, _data[i]); + for (uint256 i = 0; i < _calls.length; i++) { + bytes4 functionSelector = bytes4(keccak256(bytes(_calls[i].functionSignature))); + bytes memory callData = abi.encodePacked(functionSelector, _calls[i].data); // solhint-disable avoid-low-level-calls // slither-disable-next-line calls-loop - (bool success, bytes memory returnData) = _targets[i].call(callData); + (bool success, bytes memory returnData) = _calls[i].target.call(callData); if (!success) { if (returnData.length == 0) { - revert FailedCall(_targets[i], _functionSignatures[i], _data[i]); + revert FailedCall(_calls[i]); } // solhint-disable-next-line no-inline-assembly assembly { @@ -207,7 +188,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { } } - emit Multicalled(_multicallSigner, _reference, _targets, _data, _deadline); + emit Multicalled(_multicallSigner, _reference, _calls, _deadline); } // slither-disable-end low-level-calls,cyclomatic-complexity @@ -243,15 +224,12 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { * @dev Returns EIP712 message hash for given parameters * * @param _reference Reference - * @param _targets List of addresses to call - * @param _data List of call data + * @param _calls List of calls * @param _deadline Expiration timestamp */ function _hashTypedData( bytes32 _reference, - address[] calldata _targets, - string[] calldata _functionSignatures, - bytes[] calldata _data, + Call[] calldata _calls, uint256 _deadline ) internal view returns (bytes32) { return @@ -260,9 +238,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { abi.encode( MULTICALL_TYPEHASH, _reference, - keccak256(abi.encodePacked(_targets)), - hashStringArray(_functionSignatures), - hashBytesArray(_data), + hashCallArray(_calls), _deadline ) ) From 1de0fd9461bdd504f8946db6dcdf57cf54f7f5eb Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Thu, 22 Aug 2024 23:37:40 +1000 Subject: [PATCH 04/18] Fix linting --- contracts/multicall/GuardedMulticallerV2.sol | 25 ++++++-------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/contracts/multicall/GuardedMulticallerV2.sol b/contracts/multicall/GuardedMulticallerV2.sol index b4ec515b..9faaacb3 100644 --- a/contracts/multicall/GuardedMulticallerV2.sol +++ b/contracts/multicall/GuardedMulticallerV2.sol @@ -43,12 +43,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { keccak256("Multicall(bytes32 ref,address[] targets,bytes[] data,uint256 deadline)"); /// @dev Event emitted when execute function is called - event Multicalled( - address indexed _multicallSigner, - bytes32 indexed _reference, - Call[] _calls, - uint256 _deadline - ); + event Multicalled(address indexed _multicallSigner, bytes32 indexed _reference, Call[] _calls, uint256 _deadline); /// @dev Error thrown when reference is invalid error InvalidReference(bytes32 _reference); @@ -104,7 +99,9 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { function hashCallArray(Call[] calldata _calls) public pure returns (bytes32) { bytes32[] memory hashedCallArr = new bytes32[](_calls.length); for (uint256 i = 0; i < _calls.length; i++) { - hashedCallArr[i] = keccak256(abi.encodePacked(_calls[i].target, _calls[i].functionSignature, _calls[i].data)); + hashedCallArr[i] = keccak256( + abi.encodePacked(_calls[i].target, _calls[i].functionSignature, _calls[i].data) + ); } return keccak256(abi.encodePacked(hashedCallArr)); } @@ -190,6 +187,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { emit Multicalled(_multicallSigner, _reference, _calls, _deadline); } + // slither-disable-end low-level-calls,cyclomatic-complexity /** @@ -224,7 +222,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { * @dev Returns EIP712 message hash for given parameters * * @param _reference Reference - * @param _calls List of calls + * @param _calls List of calls * @param _deadline Expiration timestamp */ function _hashTypedData( @@ -233,15 +231,6 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { uint256 _deadline ) internal view returns (bytes32) { return - _hashTypedDataV4( - keccak256( - abi.encode( - MULTICALL_TYPEHASH, - _reference, - hashCallArray(_calls), - _deadline - ) - ) - ); + _hashTypedDataV4(keccak256(abi.encode(MULTICALL_TYPEHASH, _reference, hashCallArray(_calls), _deadline))); } } From b6580d37d53b06471c1b80268d8658d85a723bdd Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Fri, 23 Aug 2024 01:35:53 +1000 Subject: [PATCH 05/18] Fix type hash --- contracts/multicall/GuardedMulticallerV2.sol | 43 ++++++------ test/multicall/GuardedMulticallerV2.t.sol | 72 ++++++++++++++++++++ test/multicall/SigUtils.t.sol | 41 +++++++++++ 3 files changed, 137 insertions(+), 19 deletions(-) create mode 100644 test/multicall/GuardedMulticallerV2.t.sol create mode 100644 test/multicall/SigUtils.t.sol diff --git a/contracts/multicall/GuardedMulticallerV2.sol b/contracts/multicall/GuardedMulticallerV2.sol index 9faaacb3..f5e8623d 100644 --- a/contracts/multicall/GuardedMulticallerV2.sol +++ b/contracts/multicall/GuardedMulticallerV2.sol @@ -38,9 +38,14 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { /// @dev Only those with MULTICALL_SIGNER_ROLE can generate valid signatures for execute function. bytes32 public constant MULTICALL_SIGNER_ROLE = bytes32("MULTICALL_SIGNER_ROLE"); + /// @dev EIP712 typehash for call + bytes32 internal constant CALL_TYPEHASH = keccak256("Call(address target,string functionSignature,bytes data)"); + /// @dev EIP712 typehash for execute function bytes32 internal constant MULTICALL_TYPEHASH = - keccak256("Multicall(bytes32 ref,address[] targets,bytes[] data,uint256 deadline)"); + keccak256( + "Multicall(bytes32 ref,Call[] call,uint256 deadline)Call(address target,string functionSignature,bytes data)" + ); /// @dev Event emitted when execute function is called event Multicalled(address indexed _multicallSigner, bytes32 indexed _reference, Call[] _calls, uint256 _deadline); @@ -90,22 +95,6 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { _grantRole(DEFAULT_ADMIN_ROLE, _owner); } - /** - * - * @dev Returns hash of array of calls - * - * @param _calls Array of calls - */ - function hashCallArray(Call[] calldata _calls) public pure returns (bytes32) { - bytes32[] memory hashedCallArr = new bytes32[](_calls.length); - for (uint256 i = 0; i < _calls.length; i++) { - hashedCallArr[i] = keccak256( - abi.encodePacked(_calls[i].target, _calls[i].functionSignature, _calls[i].data) - ); - } - return keccak256(abi.encodePacked(hashedCallArr)); - } - /** * * @notice Execute a list of calls. Returned data from calls are ignored. @@ -217,6 +206,22 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { return replayProtection[_reference]; } + /** + * + * @dev Returns hash of array of calls + * + * @param _calls Array of calls + */ + function _hashCallArray(Call[] calldata _calls) internal pure returns (bytes32) { + bytes32[] memory hashedCallArr = new bytes32[](_calls.length); + for (uint256 i = 0; i < _calls.length; i++) { + hashedCallArr[i] = keccak256( + abi.encode(CALL_TYPEHASH, _calls[i].target, _calls[i].functionSignature, _calls[i].data) + ); + } + return keccak256(abi.encodePacked(hashedCallArr)); + } + /** * * @dev Returns EIP712 message hash for given parameters @@ -229,8 +234,8 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { bytes32 _reference, Call[] calldata _calls, uint256 _deadline - ) internal view returns (bytes32) { + ) public view returns (bytes32) { return - _hashTypedDataV4(keccak256(abi.encode(MULTICALL_TYPEHASH, _reference, hashCallArray(_calls), _deadline))); + _hashTypedDataV4(keccak256(abi.encode(MULTICALL_TYPEHASH, _reference, _hashCallArray(_calls), _deadline))); } } diff --git a/test/multicall/GuardedMulticallerV2.t.sol b/test/multicall/GuardedMulticallerV2.t.sol new file mode 100644 index 00000000..322a9259 --- /dev/null +++ b/test/multicall/GuardedMulticallerV2.t.sol @@ -0,0 +1,72 @@ +// Copyright Immutable Pty Ltd 2018 - 2024 +// SPDX-License-Identifier: Apache 2.0 +pragma solidity 0.8.19; + +import "forge-std/Test.sol"; +import {GuardedMulticallerV2} from "../../contracts/multicall/GuardedMulticallerV2.sol"; +import {MockFunctions} from "../../contracts/mocks/MockFunctions.sol"; +import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; +import {SigUtils} from "./SigUtils.t.sol"; + +contract GuardedMulticallerV2Test is Test { + GuardedMulticallerV2 gmc; + SigUtils sigUtils; + MockFunctions target; + + address defaultAdmin = makeAddr("defaultAdmin"); + address signer; + uint256 signerPk; + + function setUp() public { + target = new MockFunctions(); + (signer, signerPk) = makeAddrAndKey("signer"); + + gmc = new GuardedMulticallerV2(defaultAdmin, "name", "1"); + vm.prank(defaultAdmin); + gmc.grantMulticallSignerRole(signer); + + sigUtils = new SigUtils("name", "1", address(gmc)); + } + + function test_Roles() public { + assertTrue(gmc.hasRole(gmc.DEFAULT_ADMIN_ROLE(), defaultAdmin)); + assertTrue(gmc.hasRole(gmc.MULTICALL_SIGNER_ROLE(), signer)); + } + + function test_Execute() public { + bytes32 ref = keccak256("ref"); + uint256 deadline = block.timestamp + 1; + GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); + calls[0] = GuardedMulticallerV2.Call(address(target), "succeed()", ""); + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + gmc.execute(signer, ref, calls, deadline, signature); + assertTrue(gmc.hasBeenExecuted(ref)); + } + + function test_RevertWhen_UnauthorizedSigner() public { + (address fakeSigner, uint256 fakeSignerPk) = makeAddrAndKey("fakeSigner"); + bytes32 ref = keccak256("ref"); + uint256 deadline = block.timestamp + 1; + GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); + calls[0] = GuardedMulticallerV2.Call(address(target), "revertWithNoReason()", ""); + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(fakeSignerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.UnauthorizedSigner.selector, fakeSigner)); + gmc.execute(fakeSigner, ref, calls, deadline, signature); + } + + function test_RevertWhen_CallFailed() public { + bytes32 ref = keccak256("ref"); + uint256 deadline = block.timestamp + 1; + GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); + calls[0] = GuardedMulticallerV2.Call(address(target), "revertWithNoReason()", ""); + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.FailedCall.selector, calls[0])); + gmc.execute(signer, ref, calls, deadline, signature); + } +} diff --git a/test/multicall/SigUtils.t.sol b/test/multicall/SigUtils.t.sol new file mode 100644 index 00000000..83bcd5be --- /dev/null +++ b/test/multicall/SigUtils.t.sol @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.19; + +import {GuardedMulticallerV2} from "../../contracts/multicall/GuardedMulticallerV2.sol"; + +contract SigUtils { + bytes32 private constant _TYPE_HASH = + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); + + bytes32 internal constant CALL_TYPEHASH = keccak256("Call(address target,string functionSignature,bytes data)"); + + bytes32 internal constant MULTICALL_TYPEHASH = + keccak256( + "Multicall(bytes32 ref,Call[] call,uint256 deadline)Call(address target,string functionSignature,bytes data)" + ); + + bytes32 private immutable cachedDomainSeparator; + + constructor(string memory _name, string memory _version, address _verifyingContract) { + cachedDomainSeparator = keccak256(abi.encode(_TYPE_HASH, keccak256(bytes(_name)), keccak256(bytes(_version)), block.chainid, _verifyingContract)); + } + + function _hashCallArray(GuardedMulticallerV2.Call[] calldata _calls) internal pure returns (bytes32) { + bytes32[] memory hashedCallArr = new bytes32[](_calls.length); + for (uint256 i = 0; i < _calls.length; i++) { + hashedCallArr[i] = keccak256( + abi.encode(CALL_TYPEHASH, _calls[i].target, _calls[i].functionSignature, _calls[i].data) + ); + } + return keccak256(abi.encodePacked(hashedCallArr)); + } + + function hashTypedData( + bytes32 _reference, + GuardedMulticallerV2.Call[] calldata _calls, + uint256 _deadline + ) public view returns (bytes32) { + bytes32 digest = keccak256(abi.encode(MULTICALL_TYPEHASH, _reference, _hashCallArray(_calls), _deadline)); + return keccak256(abi.encodePacked("\x19\x01", cachedDomainSeparator, digest)); + } +} From 8c41a1363a1fdfc8e3d82670c44d53e21eda2b7d Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Fri, 23 Aug 2024 10:48:39 +1000 Subject: [PATCH 06/18] Improve testing --- contracts/mocks/MockFunctions.sol | 4 + test/multicall/GuardedMulticallerV2.t.sol | 153 +++++++++++++++++++++- 2 files changed, 153 insertions(+), 4 deletions(-) diff --git a/contracts/mocks/MockFunctions.sol b/contracts/mocks/MockFunctions.sol index 1d4eacec..a21de2cb 100644 --- a/contracts/mocks/MockFunctions.sol +++ b/contracts/mocks/MockFunctions.sol @@ -17,4 +17,8 @@ contract MockFunctions { function nonPermitted() public pure { // This function is intentionally left empty to simulate a non-permitted action } + + function succeedWithUint256(uint256 value) public pure returns (uint256) { + return value; + } } diff --git a/test/multicall/GuardedMulticallerV2.t.sol b/test/multicall/GuardedMulticallerV2.t.sol index 322a9259..504c64f6 100644 --- a/test/multicall/GuardedMulticallerV2.t.sol +++ b/test/multicall/GuardedMulticallerV2.t.sol @@ -12,13 +12,22 @@ contract GuardedMulticallerV2Test is Test { GuardedMulticallerV2 gmc; SigUtils sigUtils; MockFunctions target; + MockFunctions target1; address defaultAdmin = makeAddr("defaultAdmin"); address signer; uint256 signerPk; + event Multicalled( + address indexed _multicallSigner, + bytes32 indexed _reference, + GuardedMulticallerV2.Call[] _calls, + uint256 _deadline + ); + function setUp() public { target = new MockFunctions(); + target1 = new MockFunctions(); (signer, signerPk) = makeAddrAndKey("signer"); gmc = new GuardedMulticallerV2(defaultAdmin, "name", "1"); @@ -36,37 +45,173 @@ contract GuardedMulticallerV2Test is Test { function test_Execute() public { bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp + 1; - GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); - calls[0] = GuardedMulticallerV2.Call(address(target), "succeed()", ""); + GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](3); + calls[0] = GuardedMulticallerV2.Call( + address(target), + "succeedWithUint256(uint256)", + abi.encodePacked(uint256(42)) + ); + calls[1] = GuardedMulticallerV2.Call(address(target), "succeed()", ""); + calls[2] = GuardedMulticallerV2.Call( + address(target1), + "succeedWithUint256(uint256)", + abi.encodePacked(uint256(42)) + ); + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); bytes memory signature = abi.encodePacked(r, s, v); + + vm.expectCall(address(target), abi.encodeCall(target.succeedWithUint256, (uint256(42)))); + vm.expectCall(address(target), abi.encodeCall(target.succeed, ())); + vm.expectCall(address(target1), abi.encodeCall(target1.succeedWithUint256, (uint256(42)))); + vm.expectEmit(true, true, false, true, address(gmc)); + emit Multicalled(signer, ref, calls, deadline); + gmc.execute(signer, ref, calls, deadline, signature); + assertTrue(gmc.hasBeenExecuted(ref)); } + function test_RevertWhen_Expired() public { + bytes32 ref = keccak256("ref"); + uint256 deadline = block.timestamp - 1; + GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); + calls[0] = GuardedMulticallerV2.Call( + address(target), + "succeedWithUint256(uint256)", + abi.encodePacked(uint256(42)) + ); + + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.Expired.selector, deadline)); + + gmc.execute(signer, ref, calls, deadline, signature); + } + + function test_RevertWhen_InvalidReference() public { + bytes32 ref = ""; + uint256 deadline = block.timestamp + 1; + GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); + calls[0] = GuardedMulticallerV2.Call( + address(target), + "succeedWithUint256(uint256)", + abi.encodePacked(uint256(42)) + ); + + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.InvalidReference.selector, ref)); + + gmc.execute(signer, ref, calls, deadline, signature); + } + + function test_RevertWhen_ReusedReference() public { + bytes32 ref = "ref"; + uint256 deadline = block.timestamp + 1; + GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); + calls[0] = GuardedMulticallerV2.Call( + address(target), + "succeedWithUint256(uint256)", + abi.encodePacked(uint256(42)) + ); + + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + gmc.execute(signer, ref, calls, deadline, signature); + + vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.ReusedReference.selector, ref)); + gmc.execute(signer, ref, calls, deadline, signature); + } + + function test_RevertWhen_EmptyCallArray() public { + bytes32 ref = "ref"; + uint256 deadline = block.timestamp + 1; + GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](0); + + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.expectRevert(GuardedMulticallerV2.EmptyCallArray.selector); + + gmc.execute(signer, ref, calls, deadline, signature); + } + + function test_RevertWhen_NonContractAddress() public { + bytes32 ref = "ref"; + uint256 deadline = block.timestamp + 1; + GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); + calls[0] = GuardedMulticallerV2.Call(address(0), "succeedWithUint256(uint256)", abi.encodePacked(uint256(42))); + + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.NonContractAddress.selector, calls[0])); + + gmc.execute(signer, ref, calls, deadline, signature); + } + function test_RevertWhen_UnauthorizedSigner() public { (address fakeSigner, uint256 fakeSignerPk) = makeAddrAndKey("fakeSigner"); bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp + 1; GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); - calls[0] = GuardedMulticallerV2.Call(address(target), "revertWithNoReason()", ""); + calls[0] = GuardedMulticallerV2.Call( + address(target), + "succeedWithUint256(uint256)", + abi.encodePacked(uint256(42)) + ); + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); (uint8 v, bytes32 r, bytes32 s) = vm.sign(fakeSignerPk, digest); bytes memory signature = abi.encodePacked(r, s, v); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.UnauthorizedSigner.selector, fakeSigner)); + gmc.execute(fakeSigner, ref, calls, deadline, signature); } - function test_RevertWhen_CallFailed() public { + function test_RevertWhen_UnauthorizedSignature() public { + (, uint256 fakeSignerPk) = makeAddrAndKey("fakeSigner"); + bytes32 ref = keccak256("ref"); + uint256 deadline = block.timestamp + 1; + GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); + calls[0] = GuardedMulticallerV2.Call( + address(target), + "succeedWithUint256(uint256)", + abi.encodePacked(uint256(42)) + ); + + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(fakeSignerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.UnauthorizedSignature.selector, signature)); + + gmc.execute(signer, ref, calls, deadline, signature); + } + + function test_RevertWhen_FailedCall() public { bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp + 1; GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); calls[0] = GuardedMulticallerV2.Call(address(target), "revertWithNoReason()", ""); + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); bytes memory signature = abi.encodePacked(r, s, v); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.FailedCall.selector, calls[0])); + gmc.execute(signer, ref, calls, deadline, signature); } } From 68f26e956099ddb02415e6635818d81ae45c114b Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Mon, 26 Aug 2024 22:34:21 +1000 Subject: [PATCH 07/18] Add threat model and rename contract --- .../202408-threat-model-multicaller.md | 77 +++++++++++++++++++ ...ticallerV2.sol => GuardedMulticaller2.sol} | 8 +- contracts/multicall/README.md | 31 +++++++- ...llerV2.t.sol => GuardedMulticaller2.t.sol} | 64 +++++++-------- test/multicall/SigUtils.t.sol | 6 +- 5 files changed, 146 insertions(+), 40 deletions(-) create mode 100644 audits/multicall/202408-threat-model-multicaller.md rename contracts/multicall/{GuardedMulticallerV2.sol => GuardedMulticaller2.sol} (97%) rename test/multicall/{GuardedMulticallerV2.t.sol => GuardedMulticaller2.t.sol} (77%) diff --git a/audits/multicall/202408-threat-model-multicaller.md b/audits/multicall/202408-threat-model-multicaller.md new file mode 100644 index 00000000..56ef2f68 --- /dev/null +++ b/audits/multicall/202408-threat-model-multicaller.md @@ -0,0 +1,77 @@ +# Background +Currently the Primary Sales product uses the pattern of Multicall to transfer ERC20 tokens and mint ERC721/ERC1155 tokens in a single transaction. The Multicaller contract is a common pattern in the space and is used by many projects, this has some added security features that will be mentioned below. + +# Architecture + +## Contract High-Level Design +The core of the `Guarded Multi-caller` system is `Multi-call`, allowing for the minting and burning of multiple NFTs from various collections in a single transaction. Due to the high level of security needed when working with NFTs, additional safety measures have been implemented to meet security standards. + +- `Signature Validation` prevents multi-call instructions from random parties and only allows multi-call instructions from trusted parties. +- `Signer Access Control` manages these trusted parties. +- `References` provide anti-replay protection and help sync with any web2 system listening to events. + +## System High-Level Design +--- +![alt text](202309-threat-model-multicaller/architecture.png "Architecture") +### Components +--- +| Component | Ownership | Description | +|------------------------------ |----------- |------------------------------------------------------------------------------------------------------------------------- | +| Client | Customer | This can be a game client or a mobile client that the players interact with. | +| Central Authority | Customer | It generates a list of function calls to be executed and gets a valid signature for those calls from `Multi-call Signer`. | +| Multi-call Signer | Customer | It takes a list of function calls and generates a valid signature using a `EOA` with `MULTICALL_SIGNER_ROLE`. | +| Guarded Multicaller Contract | Customer | It validates an input signature and executes an authorized list of function calls. | + +### Flow +--- +Let’s look at the flow for basic crafting, where players burn one NFT from the `ERC721Card` contract and mint a new NFT on the `ERC721Pet` contract: + +1. An `EOA` with `DEFAULT_ADMIN_ROLE` calls the `Guarded Multi-caller` contract to permit mint function `mint(address,uint256)` with the `ERC721Pet` contract. +2. An `EOA` with `DEFAULT_ADMIN_ROLE` calls the `ERC721Pet` contract to grant `MINTER_ROLE` to the `Guarded Multi-caller` contract. +3. A client requests the `Central Authority` to generate a list of function calls to burn and mint and request a signature from the `Multi-call Signer`. +4. The `Multi-call Signer` uses an account with `MULTICALL_SIGNER_ROLE` to sign the function calls and returns the signature back to the `Central Authority`. +5. The `Central Authority` returns the list of function calls and the signature to the client. +6. The `Client` approves the `Guarded Multi-caller` contract as a spender for their `ERC721Card` NFT. +7. The `Client` submits a transaction to the `Guarded Multi-caller` contract to execute the list of function calls. +8. The `GuardedMulticaller` contract calls the `ERC721Card` contract to burn the NFT and calls the `ERC721Pet` contract to mint a new NFT to the player’s wallet. + +# Attack Surfaces + +## Compromised Admin Keys +The compromised admin keys are able to assign the `MULTICALL_SIGNER_ROLE` to malicious parties and allow them to generate signatures that are valid to invoke function calls that are damaging, e.g. minting tokens on ERC20 contracts to attackers' addresses, or burning tokens from wallets that grant the contract approvals to their tokens. + +## Compromised Signer Keys +The compromised signer keys can generate signatures that are valid to invoke function calls that are damaging, e.g. minting tokens on ERC20 contracts to attackers' addresses, or burning tokens from wallets that grant the contract approvals to their tokens. + +# Attack Mitigation + +- The keys associated with the `DEFAULT_ADMIN_ROLE` and `MULTICALL_SIGNER_ROLE` should be operated by a secure manner, for example a multi-signature wallet such that an attacker would need to compromise multiple signers simultaneously, or a securely stored hardware wallet. +- If admin keys are compromised, admins of token contracts should revoke `MINTER_ROLE` granted on the Guarded Multi-caller contract. +- If signer keys are compromised, admins of Guarded Multi-caller contracts should revoke the signer keys. +- If users grant access to their tokens, they should only grant approvals to the tokens needed for the multi-call transaction. If users have to grant access to all of their tokens, they should revoke the access immediately after the multi-call transaction is completed. With smart contract wallet's batch transaction, users can batch approvals, multi-call transaction, or approval revocation in one batch transaction. + +# Functions + +Functions that _change_ state: +| Name | Function Selector | Access Control | +| ------------------------------------------------------------- | ----------------- | --------------------- | +| execute(address,bytes32,(address,string,bytes)[],uint256,bytes) | | Caller must have a valid signature | +| grantMulticallSignerRole(address) | | DEFAULT_ADMIN_ROLE | +| revokeMulticallSignerRole(address) | | DEFAULT_ADMIN_ROLE | +| grantRole(bytes32,address) | | DEFAULT_ADMIN_ROLE | +| revokeRole(bytes32,address) | | DEFAULT_ADMIN_ROLE | +| renounceRole(bytes32,address) | | DEFAULT_ADMIN_ROLE | + +Functions that _do not change_ state (they are all permissionless): +| Name | Function Selector | +| ------------------------------------------------------------- | ----------------- | +| DEFAULT_ADMIN_ROLE() | | +| MULTICALL_SIGNER_ROLE() | | +| eip712Domain() | | +| getRoleAdmin(bytes32) | | +| hasBeenExecuted(bytes32) | | +| hasRole(bytes32,address) | | + +## Tests + +`forge test` will run all the related tests. diff --git a/contracts/multicall/GuardedMulticallerV2.sol b/contracts/multicall/GuardedMulticaller2.sol similarity index 97% rename from contracts/multicall/GuardedMulticallerV2.sol rename to contracts/multicall/GuardedMulticaller2.sol index f5e8623d..cc0d585b 100644 --- a/contracts/multicall/GuardedMulticallerV2.sol +++ b/contracts/multicall/GuardedMulticaller2.sol @@ -16,14 +16,14 @@ import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; /** * - * @title GuardedMulticallerV2 contract + * @title GuardedMulticaller2 contract * @author Immutable * @notice This contract is used to batch calls to other contracts. * @dev This contract is not designed to be upgradeable. If an issue is found with this contract, * a new version will be deployed. All approvals granted to this contract will be revoked before * a new version is deployed. Approvals will be granted to the new contract. */ -contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { +contract GuardedMulticaller2 is AccessControl, ReentrancyGuard, EIP712 { /// @dev Struct for call data struct Call { address target; @@ -98,7 +98,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { /** * * @notice Execute a list of calls. Returned data from calls are ignored. - * The signature must be generated by an address with EXECUTION_MULTICALL_SIGNER_ROLE + * The signature must be generated by an address with MULTICALL_SIGNER_ROLE * The signature must be valid * The signature must not be expired * The reference must be unique @@ -234,7 +234,7 @@ contract GuardedMulticallerV2 is AccessControl, ReentrancyGuard, EIP712 { bytes32 _reference, Call[] calldata _calls, uint256 _deadline - ) public view returns (bytes32) { + ) internal view returns (bytes32) { return _hashTypedDataV4(keccak256(abi.encode(MULTICALL_TYPEHASH, _reference, _hashCallArray(_calls), _deadline))); } diff --git a/contracts/multicall/README.md b/contracts/multicall/README.md index 1a53f8f1..dd86f380 100644 --- a/contracts/multicall/README.md +++ b/contracts/multicall/README.md @@ -1,4 +1,33 @@ -# GuardedMulticaller +# GuardedMulticaller2 + +The GuardedMulticaller2 is a signatured-based multi-call contract. It provides functionality to call multiple functions across different target contracts, the function signatures are validated to ensure they are permitted. In use cases such as crafting and Primary sales, the GuardedMulticaller2 executes mint, burn, or transfer functions on different target contracts in a single transaction. + +### Features + +- Signature validation: Only approved signers can authorise the `execute` on the multicall contract. +- Expiry: Ability to set an expiry for the multicall. +- References: Map multicall executions to a reference string to be used by the application. + +# Status + +Contract audits and threat models: + +| Description | Date |Version Audited | Link to Report | +|---------------------------|------------------|-----------------|----------------| +| V2 Threat Model | | --- | [202408-threat-model-multicaller](../../audits/multicall/202408-threat-model-multicaller.md) | +| V1 Threat Model | Sept 26, 2023 | --- | [202309-threat-model-multicaller](../../audits/multicall/202309-threat-model-multicaller.md) | +| V1 External audit | Sept 26, 2023 | [e59b72a](https://github.com/immutable/contracts/blob/e59b72a69294bd6d5857a1e2d019044bbfb14632/contracts/multicall) | [202309-external-audit-multicaller](../../audits/multicall/202309-external-audit-multicaller.pdf) | + + +# Architecture + +The architecture of the GuardedMulticaller system is shown below. + +![GuardedMulticaller Architecture](../../audits/multicall/202309-threat-model-multicaller/architecture.png) + +--- +--- +# GuardedMulticaller [DEPRECATED] The GuardedMulticaller contract provides functionality to call multiple functions across different target contracts, the function signatures are validated to ensure they are permitted. Currently one of the use cases we have is in the Primary Sales flow, the GuardedMulticaller executes `transferFrom()` and `safeMint()` functions on different target contracts in a single transaction. diff --git a/test/multicall/GuardedMulticallerV2.t.sol b/test/multicall/GuardedMulticaller2.t.sol similarity index 77% rename from test/multicall/GuardedMulticallerV2.t.sol rename to test/multicall/GuardedMulticaller2.t.sol index 504c64f6..f9310362 100644 --- a/test/multicall/GuardedMulticallerV2.t.sol +++ b/test/multicall/GuardedMulticaller2.t.sol @@ -3,13 +3,13 @@ pragma solidity 0.8.19; import "forge-std/Test.sol"; -import {GuardedMulticallerV2} from "../../contracts/multicall/GuardedMulticallerV2.sol"; +import {GuardedMulticaller2} from "../../contracts/multicall/GuardedMulticaller2.sol"; import {MockFunctions} from "../../contracts/mocks/MockFunctions.sol"; import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; import {SigUtils} from "./SigUtils.t.sol"; -contract GuardedMulticallerV2Test is Test { - GuardedMulticallerV2 gmc; +contract GuardedMulticaller2Test is Test { + GuardedMulticaller2 gmc; SigUtils sigUtils; MockFunctions target; MockFunctions target1; @@ -21,7 +21,7 @@ contract GuardedMulticallerV2Test is Test { event Multicalled( address indexed _multicallSigner, bytes32 indexed _reference, - GuardedMulticallerV2.Call[] _calls, + GuardedMulticaller2.Call[] _calls, uint256 _deadline ); @@ -30,7 +30,7 @@ contract GuardedMulticallerV2Test is Test { target1 = new MockFunctions(); (signer, signerPk) = makeAddrAndKey("signer"); - gmc = new GuardedMulticallerV2(defaultAdmin, "name", "1"); + gmc = new GuardedMulticaller2(defaultAdmin, "name", "1"); vm.prank(defaultAdmin); gmc.grantMulticallSignerRole(signer); @@ -45,14 +45,14 @@ contract GuardedMulticallerV2Test is Test { function test_Execute() public { bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp + 1; - GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](3); - calls[0] = GuardedMulticallerV2.Call( + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](3); + calls[0] = GuardedMulticaller2.Call( address(target), "succeedWithUint256(uint256)", abi.encodePacked(uint256(42)) ); - calls[1] = GuardedMulticallerV2.Call(address(target), "succeed()", ""); - calls[2] = GuardedMulticallerV2.Call( + calls[1] = GuardedMulticaller2.Call(address(target), "succeed()", ""); + calls[2] = GuardedMulticaller2.Call( address(target1), "succeedWithUint256(uint256)", abi.encodePacked(uint256(42)) @@ -76,8 +76,8 @@ contract GuardedMulticallerV2Test is Test { function test_RevertWhen_Expired() public { bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp - 1; - GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); - calls[0] = GuardedMulticallerV2.Call( + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); + calls[0] = GuardedMulticaller2.Call( address(target), "succeedWithUint256(uint256)", abi.encodePacked(uint256(42)) @@ -87,7 +87,7 @@ contract GuardedMulticallerV2Test is Test { (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); bytes memory signature = abi.encodePacked(r, s, v); - vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.Expired.selector, deadline)); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticaller2.Expired.selector, deadline)); gmc.execute(signer, ref, calls, deadline, signature); } @@ -95,8 +95,8 @@ contract GuardedMulticallerV2Test is Test { function test_RevertWhen_InvalidReference() public { bytes32 ref = ""; uint256 deadline = block.timestamp + 1; - GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); - calls[0] = GuardedMulticallerV2.Call( + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); + calls[0] = GuardedMulticaller2.Call( address(target), "succeedWithUint256(uint256)", abi.encodePacked(uint256(42)) @@ -106,7 +106,7 @@ contract GuardedMulticallerV2Test is Test { (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); bytes memory signature = abi.encodePacked(r, s, v); - vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.InvalidReference.selector, ref)); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticaller2.InvalidReference.selector, ref)); gmc.execute(signer, ref, calls, deadline, signature); } @@ -114,8 +114,8 @@ contract GuardedMulticallerV2Test is Test { function test_RevertWhen_ReusedReference() public { bytes32 ref = "ref"; uint256 deadline = block.timestamp + 1; - GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); - calls[0] = GuardedMulticallerV2.Call( + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); + calls[0] = GuardedMulticaller2.Call( address(target), "succeedWithUint256(uint256)", abi.encodePacked(uint256(42)) @@ -127,20 +127,20 @@ contract GuardedMulticallerV2Test is Test { gmc.execute(signer, ref, calls, deadline, signature); - vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.ReusedReference.selector, ref)); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticaller2.ReusedReference.selector, ref)); gmc.execute(signer, ref, calls, deadline, signature); } function test_RevertWhen_EmptyCallArray() public { bytes32 ref = "ref"; uint256 deadline = block.timestamp + 1; - GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](0); + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](0); bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); bytes memory signature = abi.encodePacked(r, s, v); - vm.expectRevert(GuardedMulticallerV2.EmptyCallArray.selector); + vm.expectRevert(GuardedMulticaller2.EmptyCallArray.selector); gmc.execute(signer, ref, calls, deadline, signature); } @@ -148,14 +148,14 @@ contract GuardedMulticallerV2Test is Test { function test_RevertWhen_NonContractAddress() public { bytes32 ref = "ref"; uint256 deadline = block.timestamp + 1; - GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); - calls[0] = GuardedMulticallerV2.Call(address(0), "succeedWithUint256(uint256)", abi.encodePacked(uint256(42))); + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); + calls[0] = GuardedMulticaller2.Call(address(0), "succeedWithUint256(uint256)", abi.encodePacked(uint256(42))); bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); bytes memory signature = abi.encodePacked(r, s, v); - vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.NonContractAddress.selector, calls[0])); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticaller2.NonContractAddress.selector, calls[0])); gmc.execute(signer, ref, calls, deadline, signature); } @@ -164,8 +164,8 @@ contract GuardedMulticallerV2Test is Test { (address fakeSigner, uint256 fakeSignerPk) = makeAddrAndKey("fakeSigner"); bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp + 1; - GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); - calls[0] = GuardedMulticallerV2.Call( + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); + calls[0] = GuardedMulticaller2.Call( address(target), "succeedWithUint256(uint256)", abi.encodePacked(uint256(42)) @@ -175,7 +175,7 @@ contract GuardedMulticallerV2Test is Test { (uint8 v, bytes32 r, bytes32 s) = vm.sign(fakeSignerPk, digest); bytes memory signature = abi.encodePacked(r, s, v); - vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.UnauthorizedSigner.selector, fakeSigner)); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticaller2.UnauthorizedSigner.selector, fakeSigner)); gmc.execute(fakeSigner, ref, calls, deadline, signature); } @@ -184,8 +184,8 @@ contract GuardedMulticallerV2Test is Test { (, uint256 fakeSignerPk) = makeAddrAndKey("fakeSigner"); bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp + 1; - GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); - calls[0] = GuardedMulticallerV2.Call( + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); + calls[0] = GuardedMulticaller2.Call( address(target), "succeedWithUint256(uint256)", abi.encodePacked(uint256(42)) @@ -195,7 +195,7 @@ contract GuardedMulticallerV2Test is Test { (uint8 v, bytes32 r, bytes32 s) = vm.sign(fakeSignerPk, digest); bytes memory signature = abi.encodePacked(r, s, v); - vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.UnauthorizedSignature.selector, signature)); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticaller2.UnauthorizedSignature.selector, signature)); gmc.execute(signer, ref, calls, deadline, signature); } @@ -203,14 +203,14 @@ contract GuardedMulticallerV2Test is Test { function test_RevertWhen_FailedCall() public { bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp + 1; - GuardedMulticallerV2.Call[] memory calls = new GuardedMulticallerV2.Call[](1); - calls[0] = GuardedMulticallerV2.Call(address(target), "revertWithNoReason()", ""); + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); + calls[0] = GuardedMulticaller2.Call(address(target), "revertWithNoReason()", ""); bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); bytes memory signature = abi.encodePacked(r, s, v); - vm.expectRevert(abi.encodeWithSelector(GuardedMulticallerV2.FailedCall.selector, calls[0])); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticaller2.FailedCall.selector, calls[0])); gmc.execute(signer, ref, calls, deadline, signature); } diff --git a/test/multicall/SigUtils.t.sol b/test/multicall/SigUtils.t.sol index 83bcd5be..fe8798d0 100644 --- a/test/multicall/SigUtils.t.sol +++ b/test/multicall/SigUtils.t.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.19; -import {GuardedMulticallerV2} from "../../contracts/multicall/GuardedMulticallerV2.sol"; +import {GuardedMulticaller2} from "../../contracts/multicall/GuardedMulticaller2.sol"; contract SigUtils { bytes32 private constant _TYPE_HASH = @@ -20,7 +20,7 @@ contract SigUtils { cachedDomainSeparator = keccak256(abi.encode(_TYPE_HASH, keccak256(bytes(_name)), keccak256(bytes(_version)), block.chainid, _verifyingContract)); } - function _hashCallArray(GuardedMulticallerV2.Call[] calldata _calls) internal pure returns (bytes32) { + function _hashCallArray(GuardedMulticaller2.Call[] calldata _calls) internal pure returns (bytes32) { bytes32[] memory hashedCallArr = new bytes32[](_calls.length); for (uint256 i = 0; i < _calls.length; i++) { hashedCallArr[i] = keccak256( @@ -32,7 +32,7 @@ contract SigUtils { function hashTypedData( bytes32 _reference, - GuardedMulticallerV2.Call[] calldata _calls, + GuardedMulticaller2.Call[] calldata _calls, uint256 _deadline ) public view returns (bytes32) { bytes32 digest = keccak256(abi.encode(MULTICALL_TYPEHASH, _reference, _hashCallArray(_calls), _deadline)); From 144f688fe96b0eb73e7b5ee56e794e0721e99f1a Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Mon, 26 Aug 2024 23:43:25 +1000 Subject: [PATCH 08/18] Add more test cases --- contracts/mocks/MockFunctions.sol | 7 +++ contracts/multicall/GuardedMulticaller2.sol | 2 + test/multicall/GuardedMulticaller2.t.sol | 62 ++++++++++++++++++--- 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/contracts/mocks/MockFunctions.sol b/contracts/mocks/MockFunctions.sol index a21de2cb..c0e2b6ad 100644 --- a/contracts/mocks/MockFunctions.sol +++ b/contracts/mocks/MockFunctions.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.19; contract MockFunctions { + error RevertWithData(uint256 value); + // solhint-disable-next-line no-empty-blocks function succeed() public pure { // This function is intentionally left empty to simulate a successful call @@ -21,4 +23,9 @@ contract MockFunctions { function succeedWithUint256(uint256 value) public pure returns (uint256) { return value; } + + function revertWithData(uint256 value) public pure { + // solhint-disable-next-line custom-errors,reason-string + revert RevertWithData(value); + } } diff --git a/contracts/multicall/GuardedMulticaller2.sol b/contracts/multicall/GuardedMulticaller2.sol index cc0d585b..88b77e6e 100644 --- a/contracts/multicall/GuardedMulticaller2.sol +++ b/contracts/multicall/GuardedMulticaller2.sol @@ -164,11 +164,13 @@ contract GuardedMulticaller2 is AccessControl, ReentrancyGuard, EIP712 { // slither-disable-next-line calls-loop (bool success, bytes memory returnData) = _calls[i].target.call(callData); if (!success) { + // Look for revert reason and bubble it up if present if (returnData.length == 0) { revert FailedCall(_calls[i]); } // solhint-disable-next-line no-inline-assembly assembly { + // The easiest way to bubble the revert reason is using memory via assembly revert(add(returnData, 32), mload(returnData)) } } diff --git a/test/multicall/GuardedMulticaller2.t.sol b/test/multicall/GuardedMulticaller2.t.sol index f9310362..7c3c9094 100644 --- a/test/multicall/GuardedMulticaller2.t.sol +++ b/test/multicall/GuardedMulticaller2.t.sol @@ -73,7 +73,7 @@ contract GuardedMulticaller2Test is Test { assertTrue(gmc.hasBeenExecuted(ref)); } - function test_RevertWhen_Expired() public { + function test_RevertWhen_ExecuteExpired() public { bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp - 1; GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); @@ -92,7 +92,7 @@ contract GuardedMulticaller2Test is Test { gmc.execute(signer, ref, calls, deadline, signature); } - function test_RevertWhen_InvalidReference() public { + function test_RevertWhen_ExecuteInvalidReference() public { bytes32 ref = ""; uint256 deadline = block.timestamp + 1; GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); @@ -111,7 +111,7 @@ contract GuardedMulticaller2Test is Test { gmc.execute(signer, ref, calls, deadline, signature); } - function test_RevertWhen_ReusedReference() public { + function test_RevertWhen_ExecuteReusedReference() public { bytes32 ref = "ref"; uint256 deadline = block.timestamp + 1; GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); @@ -131,7 +131,7 @@ contract GuardedMulticaller2Test is Test { gmc.execute(signer, ref, calls, deadline, signature); } - function test_RevertWhen_EmptyCallArray() public { + function test_RevertWhen_ExecuteEmptyCallArray() public { bytes32 ref = "ref"; uint256 deadline = block.timestamp + 1; GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](0); @@ -145,7 +145,7 @@ contract GuardedMulticaller2Test is Test { gmc.execute(signer, ref, calls, deadline, signature); } - function test_RevertWhen_NonContractAddress() public { + function test_RevertWhen_ExecuteNonContractAddress() public { bytes32 ref = "ref"; uint256 deadline = block.timestamp + 1; GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); @@ -160,7 +160,7 @@ contract GuardedMulticaller2Test is Test { gmc.execute(signer, ref, calls, deadline, signature); } - function test_RevertWhen_UnauthorizedSigner() public { + function test_RevertWhen_ExecuteUnauthorizedSigner() public { (address fakeSigner, uint256 fakeSignerPk) = makeAddrAndKey("fakeSigner"); bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp + 1; @@ -180,7 +180,7 @@ contract GuardedMulticaller2Test is Test { gmc.execute(fakeSigner, ref, calls, deadline, signature); } - function test_RevertWhen_UnauthorizedSignature() public { + function test_RevertWhen_ExecuteUnauthorizedSignature() public { (, uint256 fakeSignerPk) = makeAddrAndKey("fakeSigner"); bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp + 1; @@ -200,7 +200,7 @@ contract GuardedMulticaller2Test is Test { gmc.execute(signer, ref, calls, deadline, signature); } - function test_RevertWhen_FailedCall() public { + function test_RevertWhen_ExecuteFailedCall() public { bytes32 ref = keccak256("ref"); uint256 deadline = block.timestamp + 1; GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); @@ -214,4 +214,50 @@ contract GuardedMulticaller2Test is Test { gmc.execute(signer, ref, calls, deadline, signature); } + + function test_RevertWhen_ExecuteRevokeMinterRole() public { + bytes32 ref = keccak256("ref"); + uint256 deadline = block.timestamp + 1; + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); + calls[0] = GuardedMulticaller2.Call( + address(target), + "succeedWithUint256(uint256)", + abi.encodePacked(uint256(42)) + ); + + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + gmc.execute(signer, ref, calls, deadline, signature); + + vm.startPrank(defaultAdmin); + gmc.revokeMulticallSignerRole(signer); + vm.stopPrank(); + + bytes32 ref1 = keccak256("ref1"); + bytes32 digest1 = sigUtils.hashTypedData(ref1, calls, deadline); + (v, r, s) = vm.sign(signerPk, digest1); + bytes memory signature1 = abi.encodePacked(r, s, v); + + vm.expectRevert(abi.encodeWithSelector(GuardedMulticaller2.UnauthorizedSigner.selector, signer)); + gmc.execute(signer, ref1, calls, deadline, signature1); + bool executed = gmc.hasBeenExecuted(ref1); + assertFalse(executed); + } + + function test_RevertWhen_ExecuteBubbleUpRevertReason() public { + bytes32 ref = keccak256("ref"); + uint256 deadline = block.timestamp + 1; + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); + calls[0] = GuardedMulticaller2.Call(address(target), "revertWithData(uint256)", abi.encodePacked(uint256(42))); + + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.expectRevert(abi.encodeWithSelector(MockFunctions.RevertWithData.selector, uint256(42))); + + gmc.execute(signer, ref, calls, deadline, signature); + } } From 820ab85c45b3876e718271fb29d674fcaed8502c Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Tue, 27 Aug 2024 00:07:38 +1000 Subject: [PATCH 09/18] Check valid function signatures --- contracts/multicall/GuardedMulticaller2.sol | 23 +++++++++------------ test/multicall/GuardedMulticaller2.t.sol | 15 ++++++++++++++ 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/contracts/multicall/GuardedMulticaller2.sol b/contracts/multicall/GuardedMulticaller2.sol index 88b77e6e..54f5e078 100644 --- a/contracts/multicall/GuardedMulticaller2.sol +++ b/contracts/multicall/GuardedMulticaller2.sol @@ -56,32 +56,26 @@ contract GuardedMulticaller2 is AccessControl, ReentrancyGuard, EIP712 { /// @dev Error thrown when reference has already been executed error ReusedReference(bytes32 _reference); - /// @dev Error thrown when call array is empty - error EmptyCallArray(); - - /// @dev Error thrown when address array and data array have different lengths - error AddressDataArrayLengthsMismatch(uint256 _addressLength, uint256 _dataLength); - /// @dev Error thrown when deadline is expired error Expired(uint256 _deadline); - /// @dev Error thrown when target address is not a contract - error NonContractAddress(Call _call); - /// @dev Error thrown when signer is not authorized error UnauthorizedSigner(address _multicallSigner); /// @dev Error thrown when signature is invalid error UnauthorizedSignature(bytes _signature); + /// @dev Error thrown when call array is empty + error EmptyCallArray(); + /// @dev Error thrown when call reverts error FailedCall(Call _call); - /// @dev Error thrown when call data is invalid - error InvalidCallData(address _target, bytes _data); + /// @dev Error thrown when target address is not a contract + error NonContractAddress(Call _call); - /// @dev Error thrown when call data is unauthorized - error UnauthorizedFunction(address _target, string _functionSignature); + /// @dev Error thrown when function signature is invalid + error InvalidFunctionSignature(Call _call); /** * @@ -135,6 +129,9 @@ contract GuardedMulticaller2 is AccessControl, ReentrancyGuard, EIP712 { revert EmptyCallArray(); } for (uint256 i = 0; i < _calls.length; i++) { + if (bytes(_calls[i].functionSignature).length == 0) { + revert InvalidFunctionSignature(_calls[i]); + } if (_calls[i].target.code.length == 0) { revert NonContractAddress(_calls[i]); } diff --git a/test/multicall/GuardedMulticaller2.t.sol b/test/multicall/GuardedMulticaller2.t.sol index 7c3c9094..39ca3c56 100644 --- a/test/multicall/GuardedMulticaller2.t.sol +++ b/test/multicall/GuardedMulticaller2.t.sol @@ -260,4 +260,19 @@ contract GuardedMulticaller2Test is Test { gmc.execute(signer, ref, calls, deadline, signature); } + + function test_RevertWhen_ExecuteInvalidFunctionSignature() public { + bytes32 ref = keccak256("ref"); + uint256 deadline = block.timestamp + 1; + GuardedMulticaller2.Call[] memory calls = new GuardedMulticaller2.Call[](1); + calls[0] = GuardedMulticaller2.Call(address(target), "", abi.encodePacked(uint256(42))); + + bytes32 digest = sigUtils.hashTypedData(ref, calls, deadline); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.expectRevert(abi.encodeWithSelector(GuardedMulticaller2.InvalidFunctionSignature.selector, calls[0])); + + gmc.execute(signer, ref, calls, deadline, signature); + } } From 8b092006c08941645b99fde0b7ba939c20d3b6c4 Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Tue, 27 Aug 2024 00:12:59 +1000 Subject: [PATCH 10/18] Fix solidity version --- contracts/multicall/GuardedMulticaller2.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/multicall/GuardedMulticaller2.sol b/contracts/multicall/GuardedMulticaller2.sol index 54f5e078..57bd839e 100644 --- a/contracts/multicall/GuardedMulticaller2.sol +++ b/contracts/multicall/GuardedMulticaller2.sol @@ -1,6 +1,6 @@ // Copyright Immutable Pty Ltd 2018 - 2023 // SPDX-License-Identifier: MIT -pragma solidity ^0.8.19; +pragma solidity 0.8.19; // Signature Validation import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; From 8739256efdc6d9f328f6d68b4550d4f0a28d3e0c Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Tue, 27 Aug 2024 00:37:46 +1000 Subject: [PATCH 11/18] Update tests --- test/multicall/GuardedMulticaller2.t.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/multicall/GuardedMulticaller2.t.sol b/test/multicall/GuardedMulticaller2.t.sol index 39ca3c56..774be1e4 100644 --- a/test/multicall/GuardedMulticaller2.t.sol +++ b/test/multicall/GuardedMulticaller2.t.sol @@ -275,4 +275,7 @@ contract GuardedMulticaller2Test is Test { gmc.execute(signer, ref, calls, deadline, signature); } + + // TODO: test reentrancy + function test_RevertWhen_ExecuteReentrant() public {} } From 25482e9917687d4925d10208d484b8504ea245f3 Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Tue, 27 Aug 2024 09:39:41 +1000 Subject: [PATCH 12/18] Fix threat doc --- audits/multicall/202408-threat-model-multicaller.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/audits/multicall/202408-threat-model-multicaller.md b/audits/multicall/202408-threat-model-multicaller.md index 56ef2f68..a24e715d 100644 --- a/audits/multicall/202408-threat-model-multicaller.md +++ b/audits/multicall/202408-threat-model-multicaller.md @@ -1,5 +1,5 @@ # Background -Currently the Primary Sales product uses the pattern of Multicall to transfer ERC20 tokens and mint ERC721/ERC1155 tokens in a single transaction. The Multicaller contract is a common pattern in the space and is used by many projects, this has some added security features that will be mentioned below. +The Multicaller contract is a common pattern in the space and is used by many projects, this has some added security features that will be mentioned below. # Architecture From 4377245d9e0fb16a31e49ffca3e2926137a3aaba Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Tue, 27 Aug 2024 16:39:56 +1000 Subject: [PATCH 13/18] Add abi --- abi/generated.ts | 321 +++++++++++++++++++++++++++++++++++++++++++++++ abi/index.ts | 9 +- wagmi.config.ts | 5 + 3 files changed, 334 insertions(+), 1 deletion(-) diff --git a/abi/generated.ts b/abi/generated.ts index 25668731..33c8d324 100644 --- a/abi/generated.ts +++ b/abi/generated.ts @@ -362,6 +362,327 @@ export const guardedMulticallerAbi = [ }, ] as const +////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +// GuardedMulticaller2 +////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + +export const guardedMulticaller2Abi = [ + { + type: 'constructor', + inputs: [ + { name: '_owner', internalType: 'address', type: 'address' }, + { name: '_name', internalType: 'string', type: 'string' }, + { name: '_version', internalType: 'string', type: 'string' }, + ], + stateMutability: 'nonpayable', + }, + { + type: 'function', + inputs: [], + name: 'DEFAULT_ADMIN_ROLE', + outputs: [{ name: '', internalType: 'bytes32', type: 'bytes32' }], + stateMutability: 'view', + }, + { + type: 'function', + inputs: [], + name: 'MULTICALL_SIGNER_ROLE', + outputs: [{ name: '', internalType: 'bytes32', type: 'bytes32' }], + stateMutability: 'view', + }, + { + type: 'function', + inputs: [], + name: 'eip712Domain', + outputs: [ + { name: 'fields', internalType: 'bytes1', type: 'bytes1' }, + { name: 'name', internalType: 'string', type: 'string' }, + { name: 'version', internalType: 'string', type: 'string' }, + { name: 'chainId', internalType: 'uint256', type: 'uint256' }, + { name: 'verifyingContract', internalType: 'address', type: 'address' }, + { name: 'salt', internalType: 'bytes32', type: 'bytes32' }, + { name: 'extensions', internalType: 'uint256[]', type: 'uint256[]' }, + ], + stateMutability: 'view', + }, + { + type: 'function', + inputs: [ + { name: '_multicallSigner', internalType: 'address', type: 'address' }, + { name: '_reference', internalType: 'bytes32', type: 'bytes32' }, + { + name: '_calls', + internalType: 'struct GuardedMulticaller2.Call[]', + type: 'tuple[]', + components: [ + { name: 'target', internalType: 'address', type: 'address' }, + { name: 'functionSignature', internalType: 'string', type: 'string' }, + { name: 'data', internalType: 'bytes', type: 'bytes' }, + ], + }, + { name: '_deadline', internalType: 'uint256', type: 'uint256' }, + { name: '_signature', internalType: 'bytes', type: 'bytes' }, + ], + name: 'execute', + outputs: [], + stateMutability: 'nonpayable', + }, + { + type: 'function', + inputs: [{ name: 'role', internalType: 'bytes32', type: 'bytes32' }], + name: 'getRoleAdmin', + outputs: [{ name: '', internalType: 'bytes32', type: 'bytes32' }], + stateMutability: 'view', + }, + { + type: 'function', + inputs: [{ name: '_user', internalType: 'address', type: 'address' }], + name: 'grantMulticallSignerRole', + outputs: [], + stateMutability: 'nonpayable', + }, + { + type: 'function', + inputs: [ + { name: 'role', internalType: 'bytes32', type: 'bytes32' }, + { name: 'account', internalType: 'address', type: 'address' }, + ], + name: 'grantRole', + outputs: [], + stateMutability: 'nonpayable', + }, + { + type: 'function', + inputs: [{ name: '_reference', internalType: 'bytes32', type: 'bytes32' }], + name: 'hasBeenExecuted', + outputs: [{ name: '', internalType: 'bool', type: 'bool' }], + stateMutability: 'view', + }, + { + type: 'function', + inputs: [ + { name: 'role', internalType: 'bytes32', type: 'bytes32' }, + { name: 'account', internalType: 'address', type: 'address' }, + ], + name: 'hasRole', + outputs: [{ name: '', internalType: 'bool', type: 'bool' }], + stateMutability: 'view', + }, + { + type: 'function', + inputs: [ + { name: 'role', internalType: 'bytes32', type: 'bytes32' }, + { name: 'account', internalType: 'address', type: 'address' }, + ], + name: 'renounceRole', + outputs: [], + stateMutability: 'nonpayable', + }, + { + type: 'function', + inputs: [{ name: '_user', internalType: 'address', type: 'address' }], + name: 'revokeMulticallSignerRole', + outputs: [], + stateMutability: 'nonpayable', + }, + { + type: 'function', + inputs: [ + { name: 'role', internalType: 'bytes32', type: 'bytes32' }, + { name: 'account', internalType: 'address', type: 'address' }, + ], + name: 'revokeRole', + outputs: [], + stateMutability: 'nonpayable', + }, + { + type: 'function', + inputs: [{ name: 'interfaceId', internalType: 'bytes4', type: 'bytes4' }], + name: 'supportsInterface', + outputs: [{ name: '', internalType: 'bool', type: 'bool' }], + stateMutability: 'view', + }, + { type: 'event', anonymous: false, inputs: [], name: 'EIP712DomainChanged' }, + { + type: 'event', + anonymous: false, + inputs: [ + { + name: '_multicallSigner', + internalType: 'address', + type: 'address', + indexed: true, + }, + { + name: '_reference', + internalType: 'bytes32', + type: 'bytes32', + indexed: true, + }, + { + name: '_calls', + internalType: 'struct GuardedMulticaller2.Call[]', + type: 'tuple[]', + components: [ + { name: 'target', internalType: 'address', type: 'address' }, + { name: 'functionSignature', internalType: 'string', type: 'string' }, + { name: 'data', internalType: 'bytes', type: 'bytes' }, + ], + indexed: false, + }, + { + name: '_deadline', + internalType: 'uint256', + type: 'uint256', + indexed: false, + }, + ], + name: 'Multicalled', + }, + { + type: 'event', + anonymous: false, + inputs: [ + { name: 'role', internalType: 'bytes32', type: 'bytes32', indexed: true }, + { + name: 'previousAdminRole', + internalType: 'bytes32', + type: 'bytes32', + indexed: true, + }, + { + name: 'newAdminRole', + internalType: 'bytes32', + type: 'bytes32', + indexed: true, + }, + ], + name: 'RoleAdminChanged', + }, + { + type: 'event', + anonymous: false, + inputs: [ + { name: 'role', internalType: 'bytes32', type: 'bytes32', indexed: true }, + { + name: 'account', + internalType: 'address', + type: 'address', + indexed: true, + }, + { + name: 'sender', + internalType: 'address', + type: 'address', + indexed: true, + }, + ], + name: 'RoleGranted', + }, + { + type: 'event', + anonymous: false, + inputs: [ + { name: 'role', internalType: 'bytes32', type: 'bytes32', indexed: true }, + { + name: 'account', + internalType: 'address', + type: 'address', + indexed: true, + }, + { + name: 'sender', + internalType: 'address', + type: 'address', + indexed: true, + }, + ], + name: 'RoleRevoked', + }, + { type: 'error', inputs: [], name: 'EmptyCallArray' }, + { + type: 'error', + inputs: [{ name: '_deadline', internalType: 'uint256', type: 'uint256' }], + name: 'Expired', + }, + { + type: 'error', + inputs: [ + { + name: '_call', + internalType: 'struct GuardedMulticaller2.Call', + type: 'tuple', + components: [ + { name: 'target', internalType: 'address', type: 'address' }, + { name: 'functionSignature', internalType: 'string', type: 'string' }, + { name: 'data', internalType: 'bytes', type: 'bytes' }, + ], + }, + ], + name: 'FailedCall', + }, + { + type: 'error', + inputs: [ + { + name: '_call', + internalType: 'struct GuardedMulticaller2.Call', + type: 'tuple', + components: [ + { name: 'target', internalType: 'address', type: 'address' }, + { name: 'functionSignature', internalType: 'string', type: 'string' }, + { name: 'data', internalType: 'bytes', type: 'bytes' }, + ], + }, + ], + name: 'InvalidFunctionSignature', + }, + { + type: 'error', + inputs: [{ name: '_reference', internalType: 'bytes32', type: 'bytes32' }], + name: 'InvalidReference', + }, + { type: 'error', inputs: [], name: 'InvalidShortString' }, + { + type: 'error', + inputs: [ + { + name: '_call', + internalType: 'struct GuardedMulticaller2.Call', + type: 'tuple', + components: [ + { name: 'target', internalType: 'address', type: 'address' }, + { name: 'functionSignature', internalType: 'string', type: 'string' }, + { name: 'data', internalType: 'bytes', type: 'bytes' }, + ], + }, + ], + name: 'NonContractAddress', + }, + { + type: 'error', + inputs: [{ name: '_reference', internalType: 'bytes32', type: 'bytes32' }], + name: 'ReusedReference', + }, + { + type: 'error', + inputs: [{ name: 'str', internalType: 'string', type: 'string' }], + name: 'StringTooLong', + }, + { + type: 'error', + inputs: [{ name: '_signature', internalType: 'bytes', type: 'bytes' }], + name: 'UnauthorizedSignature', + }, + { + type: 'error', + inputs: [ + { name: '_multicallSigner', internalType: 'address', type: 'address' }, + ], + name: 'UnauthorizedSigner', + }, +] as const + ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // ImmutableERC1155 ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/abi/index.ts b/abi/index.ts index efe9383b..687c7916 100644 --- a/abi/index.ts +++ b/abi/index.ts @@ -3,6 +3,13 @@ import { immutableErc721Abi as ImmutableERC721Abi, immutableErc721MintByIdAbi as ImmutableERC721MintByIdAbi, immutableErc1155Abi as ImmutableERC1155Abi, + guardedMulticaller2Abi as GuardedMulticaller2Abi, } from "./generated"; -export { GuardedMulticallerAbi, ImmutableERC721Abi, ImmutableERC721MintByIdAbi, ImmutableERC1155Abi }; +export { + GuardedMulticallerAbi, + ImmutableERC721Abi, + ImmutableERC721MintByIdAbi, + ImmutableERC1155Abi, + GuardedMulticaller2Abi, +}; diff --git a/wagmi.config.ts b/wagmi.config.ts index 533e0c5b..e0efa059 100644 --- a/wagmi.config.ts +++ b/wagmi.config.ts @@ -5,6 +5,7 @@ import GuardedMulticaller from "./foundry-out/GuardedMulticaller.sol/GuardedMult import ImmutableERC721 from "./foundry-out/ImmutableERC721.sol/ImmutableERC721.json"; import ImmutableERC721MintByID from "./foundry-out/ImmutableERC721MintByID.sol/ImmutableERC721MintByID.json"; import ImmutableERC1155 from "./foundry-out/ImmutableERC1155.sol/ImmutableERC1155.json"; +import GuardedMulticaller2 from "./foundry-out/GuardedMulticaller2.sol/GuardedMulticaller2.json"; // https://github.com/wevm/viem/discussions/1009 export default defineConfig({ @@ -26,5 +27,9 @@ export default defineConfig({ name: "ImmutableERC1155", abi: ImmutableERC1155.abi as Abi, }, + { + name: "GuardedMulticaller2", + abi: GuardedMulticaller2.abi as Abi, + }, ], }); From b62a222ac6390ed1a0cb808a35a3c94a5f20927f Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Wed, 28 Aug 2024 23:02:19 +1000 Subject: [PATCH 14/18] Use encode in keccak256 --- contracts/multicall/GuardedMulticaller2.sol | 2 +- test/multicall/SigUtils.t.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/multicall/GuardedMulticaller2.sol b/contracts/multicall/GuardedMulticaller2.sol index 57bd839e..7e61fc30 100644 --- a/contracts/multicall/GuardedMulticaller2.sol +++ b/contracts/multicall/GuardedMulticaller2.sol @@ -218,7 +218,7 @@ contract GuardedMulticaller2 is AccessControl, ReentrancyGuard, EIP712 { abi.encode(CALL_TYPEHASH, _calls[i].target, _calls[i].functionSignature, _calls[i].data) ); } - return keccak256(abi.encodePacked(hashedCallArr)); + return keccak256(abi.encode(hashedCallArr)); } /** diff --git a/test/multicall/SigUtils.t.sol b/test/multicall/SigUtils.t.sol index fe8798d0..967b9b91 100644 --- a/test/multicall/SigUtils.t.sol +++ b/test/multicall/SigUtils.t.sol @@ -27,7 +27,7 @@ contract SigUtils { abi.encode(CALL_TYPEHASH, _calls[i].target, _calls[i].functionSignature, _calls[i].data) ); } - return keccak256(abi.encodePacked(hashedCallArr)); + return keccak256(abi.encode(hashedCallArr)); } function hashTypedData( From 6cdcfbf60b3e9ff6ac45fd75a0ad9118c495495d Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Thu, 29 Aug 2024 12:16:28 +1000 Subject: [PATCH 15/18] Address comments --- contracts/multicall/GuardedMulticaller2.sol | 9 ++++----- test/multicall/GuardedMulticaller2.t.sol | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/contracts/multicall/GuardedMulticaller2.sol b/contracts/multicall/GuardedMulticaller2.sol index 7e61fc30..fc949345 100644 --- a/contracts/multicall/GuardedMulticaller2.sol +++ b/contracts/multicall/GuardedMulticaller2.sol @@ -1,4 +1,4 @@ -// Copyright Immutable Pty Ltd 2018 - 2023 +// Copyright Immutable Pty Ltd 2018 - 2024 // SPDX-License-Identifier: MIT pragma solidity 0.8.19; @@ -32,7 +32,6 @@ contract GuardedMulticaller2 is AccessControl, ReentrancyGuard, EIP712 { } /// @dev Mapping of reference to executed status - // solhint-disable-next-line named-parameters-mapping mapping(bytes32 => bool) private replayProtection; /// @dev Only those with MULTICALL_SIGNER_ROLE can generate valid signatures for execute function. @@ -69,7 +68,7 @@ contract GuardedMulticaller2 is AccessControl, ReentrancyGuard, EIP712 { error EmptyCallArray(); /// @dev Error thrown when call reverts - error FailedCall(Call _call); + error FailedCall(Call _call, bytes _returnData); /// @dev Error thrown when target address is not a contract error NonContractAddress(Call _call); @@ -162,8 +161,8 @@ contract GuardedMulticaller2 is AccessControl, ReentrancyGuard, EIP712 { (bool success, bytes memory returnData) = _calls[i].target.call(callData); if (!success) { // Look for revert reason and bubble it up if present - if (returnData.length == 0) { - revert FailedCall(_calls[i]); + if (returnData.length < 4) { + revert FailedCall(_calls[i], returnData); } // solhint-disable-next-line no-inline-assembly assembly { diff --git a/test/multicall/GuardedMulticaller2.t.sol b/test/multicall/GuardedMulticaller2.t.sol index 774be1e4..7e73a6f9 100644 --- a/test/multicall/GuardedMulticaller2.t.sol +++ b/test/multicall/GuardedMulticaller2.t.sol @@ -210,7 +210,7 @@ contract GuardedMulticaller2Test is Test { (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPk, digest); bytes memory signature = abi.encodePacked(r, s, v); - vm.expectRevert(abi.encodeWithSelector(GuardedMulticaller2.FailedCall.selector, calls[0])); + vm.expectRevert(abi.encodeWithSelector(GuardedMulticaller2.FailedCall.selector, calls[0], "")); gmc.execute(signer, ref, calls, deadline, signature); } From b879470057ad3544c6d3075727601afab2af6ee5 Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Thu, 29 Aug 2024 12:21:45 +1000 Subject: [PATCH 16/18] Add param name to mapping --- contracts/multicall/GuardedMulticaller2.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/multicall/GuardedMulticaller2.sol b/contracts/multicall/GuardedMulticaller2.sol index fc949345..46d901a0 100644 --- a/contracts/multicall/GuardedMulticaller2.sol +++ b/contracts/multicall/GuardedMulticaller2.sol @@ -32,7 +32,7 @@ contract GuardedMulticaller2 is AccessControl, ReentrancyGuard, EIP712 { } /// @dev Mapping of reference to executed status - mapping(bytes32 => bool) private replayProtection; + mapping(bytes32 ref => bool executed) private replayProtection; /// @dev Only those with MULTICALL_SIGNER_ROLE can generate valid signatures for execute function. bytes32 public constant MULTICALL_SIGNER_ROLE = bytes32("MULTICALL_SIGNER_ROLE"); From 4fdb60c8794222d09a34ed198f452a671d59d854 Mon Sep 17 00:00:00 2001 From: Allan Almeida Date: Thu, 29 Aug 2024 13:16:57 +1000 Subject: [PATCH 17/18] update threat model --- .../202408-threat-model-multicaller.md | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/audits/multicall/202408-threat-model-multicaller.md b/audits/multicall/202408-threat-model-multicaller.md index a24e715d..08a044e2 100644 --- a/audits/multicall/202408-threat-model-multicaller.md +++ b/audits/multicall/202408-threat-model-multicaller.md @@ -1,9 +1,13 @@ # Background -The Multicaller contract is a common pattern in the space and is used by many projects, this has some added security features that will be mentioned below. + +The Guarded Multi-Caller contract is a security enhanced multi-caller contract. The Multicall contract pattern allows multiple smart contract function calls to be grouped into a single call. This provides the guarantee that function calls are executed atomically in a single transaction - they either all succeed or they all revert. + +Additionally, this has the benefit that it reduces the number of JSON RPC requests that need to be sent to the blockchain's RPC provider. The security enhancements offered by the Guarded Multi-caller system are described in the sections below. # Architecture ## Contract High-Level Design + The core of the `Guarded Multi-caller` system is `Multi-call`, allowing for the minting and burning of multiple NFTs from various collections in a single transaction. Due to the high level of security needed when working with NFTs, additional safety measures have been implemented to meet security standards. - `Signature Validation` prevents multi-call instructions from random parties and only allows multi-call instructions from trusted parties. @@ -29,7 +33,7 @@ Let’s look at the flow for basic crafting, where players burn one NFT from the 1. An `EOA` with `DEFAULT_ADMIN_ROLE` calls the `Guarded Multi-caller` contract to permit mint function `mint(address,uint256)` with the `ERC721Pet` contract. 2. An `EOA` with `DEFAULT_ADMIN_ROLE` calls the `ERC721Pet` contract to grant `MINTER_ROLE` to the `Guarded Multi-caller` contract. 3. A client requests the `Central Authority` to generate a list of function calls to burn and mint and request a signature from the `Multi-call Signer`. -4. The `Multi-call Signer` uses an account with `MULTICALL_SIGNER_ROLE` to sign the function calls and returns the signature back to the `Central Authority`. +4. The `Multi-call Signer` uses an account with `MULTICALL_SIGNER_ROLE` to sign the function calls and returns the signature back to the `Central Authority`. 5. The `Central Authority` returns the list of function calls and the signature to the client. 6. The `Client` approves the `Guarded Multi-caller` contract as a spender for their `ERC721Card` NFT. 7. The `Client` submits a transaction to the `Guarded Multi-caller` contract to execute the list of function calls. @@ -46,7 +50,7 @@ The compromised signer keys can generate signatures that are valid to invoke fun # Attack Mitigation - The keys associated with the `DEFAULT_ADMIN_ROLE` and `MULTICALL_SIGNER_ROLE` should be operated by a secure manner, for example a multi-signature wallet such that an attacker would need to compromise multiple signers simultaneously, or a securely stored hardware wallet. -- If admin keys are compromised, admins of token contracts should revoke `MINTER_ROLE` granted on the Guarded Multi-caller contract. +- If admin keys are compromised, admins of token contracts should revoke `MINTER_ROLE` granted on the Guarded Multi-caller contract. - If signer keys are compromised, admins of Guarded Multi-caller contracts should revoke the signer keys. - If users grant access to their tokens, they should only grant approvals to the tokens needed for the multi-call transaction. If users have to grant access to all of their tokens, they should revoke the access immediately after the multi-call transaction is completed. With smart contract wallet's batch transaction, users can batch approvals, multi-call transaction, or approval revocation in one batch transaction. @@ -55,22 +59,23 @@ The compromised signer keys can generate signatures that are valid to invoke fun Functions that _change_ state: | Name | Function Selector | Access Control | | ------------------------------------------------------------- | ----------------- | --------------------- | -| execute(address,bytes32,(address,string,bytes)[],uint256,bytes) | | Caller must have a valid signature | -| grantMulticallSignerRole(address) | | DEFAULT_ADMIN_ROLE | -| revokeMulticallSignerRole(address) | | DEFAULT_ADMIN_ROLE | -| grantRole(bytes32,address) | | DEFAULT_ADMIN_ROLE | -| revokeRole(bytes32,address) | | DEFAULT_ADMIN_ROLE | -| renounceRole(bytes32,address) | | DEFAULT_ADMIN_ROLE | +| execute(address,bytes32,(address,string,bytes)[],uint256,bytes) | 29d4244c | Caller must have a valid signature | +| grantMulticallSignerRole(address) | 5910fd78 | DEFAULT_ADMIN_ROLE | +| grantRole(bytes32,address) | 2f2ff15d | DEFAULT_ADMIN_ROLE | +| renounceRole(bytes32,address) | 36568abe | Caller must have role to be renounced | +| revokeMulticallSignerRole(address) | 8f8d6ba0 | DEFAULT_ADMIN_ROLE | +| revokeRole(bytes32,address) | d547741f | DEFAULT_ADMIN_ROLE | Functions that _do not change_ state (they are all permissionless): | Name | Function Selector | | ------------------------------------------------------------- | ----------------- | -| DEFAULT_ADMIN_ROLE() | | -| MULTICALL_SIGNER_ROLE() | | -| eip712Domain() | | -| getRoleAdmin(bytes32) | | -| hasBeenExecuted(bytes32) | | -| hasRole(bytes32,address) | | +| DEFAULT_ADMIN_ROLE() | a217fddf | +| eip712Domain() | 84b0196e | +| getRoleAdmin(bytes32) | 248a9ca3 | +| hasBeenExecuted(bytes32) | 3ddd8215 | +| hasRole(bytes32,address) | 91d14854 | +| supportsInterface(bytes4) | 01ffc9a7 | +| MULTICALL_SIGNER_ROLE() | 953ff95b | ## Tests From 2af8c046f91c38202f640a49dfe80645df5d0c25 Mon Sep 17 00:00:00 2001 From: Hiep Doan Date: Thu, 29 Aug 2024 14:38:04 +1000 Subject: [PATCH 18/18] Update threat model --- audits/multicall/202408-threat-model-multicaller.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/audits/multicall/202408-threat-model-multicaller.md b/audits/multicall/202408-threat-model-multicaller.md index 08a044e2..f3cfe85a 100644 --- a/audits/multicall/202408-threat-model-multicaller.md +++ b/audits/multicall/202408-threat-model-multicaller.md @@ -23,7 +23,7 @@ The core of the `Guarded Multi-caller` system is `Multi-call`, allowing for the |------------------------------ |----------- |------------------------------------------------------------------------------------------------------------------------- | | Client | Customer | This can be a game client or a mobile client that the players interact with. | | Central Authority | Customer | It generates a list of function calls to be executed and gets a valid signature for those calls from `Multi-call Signer`. | -| Multi-call Signer | Customer | It takes a list of function calls and generates a valid signature using a `EOA` with `MULTICALL_SIGNER_ROLE`. | +| Multi-call Signer | Customer | It takes a list of function calls and generates a valid signature using a `EOA` with `MULTICALL_SIGNER_ROLE`. | | Guarded Multicaller Contract | Customer | It validates an input signature and executes an authorized list of function calls. | ### Flow @@ -53,6 +53,10 @@ The compromised signer keys can generate signatures that are valid to invoke fun - If admin keys are compromised, admins of token contracts should revoke `MINTER_ROLE` granted on the Guarded Multi-caller contract. - If signer keys are compromised, admins of Guarded Multi-caller contracts should revoke the signer keys. - If users grant access to their tokens, they should only grant approvals to the tokens needed for the multi-call transaction. If users have to grant access to all of their tokens, they should revoke the access immediately after the multi-call transaction is completed. With smart contract wallet's batch transaction, users can batch approvals, multi-call transaction, or approval revocation in one batch transaction. +- If the last account with DEFAULT_ADMIN_ROLE accidentally renounces or revokes their role, the contract becomes permissionless, and admins can no longer grant/revoke accesses. The following should happen: + - Accounts with `MULTICALL_SIGNER_ROLE` should renounce their roles + - A new Guarded Multi-caller contract should be deployed. + - All roles/accesses granted to the contract should be revoked. # Functions