Add threat model and rename contract

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.

contracts/multicall/GuardedMulticallerV2.sol → 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)));
     }

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. 
 

test/multicall/GuardedMulticallerV2.t.sol → 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,16 +87,16 @@ 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);
     }
 
     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,16 +106,16 @@ 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);
     }
 
     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,35 +127,35 @@ 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);
     }
 
     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,22 +195,22 @@ 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);
     }
 
     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);
     }