refactor: ImmutableERC1155 threat model (#229)

* capitalize * formatting * add diagram * update threat model * add functions * update table * add links and git hash * typo * typo * update wording * add attack mitigation * update attack mitigation * update function permission descriptions * update description * add githash links * fix link * formatting
immutable · Jun 17, 2024 · fbf54a8 · fbf54a8
1 parent 1ddb3dd
commit fbf54a8
Show file tree

Hide file tree

Showing 4 changed files with 113 additions and 30 deletions.
diff --git a/audits/token/202309-threat-model-preset-erc721.md b/audits/token/202309-threat-model-preset-erc721.md
@@ -1,11 +1,12 @@
 ## Introduction
+
 This document is a thread model for two preset erc721 token contracts built by Immutable.
 
-This document encompasses information for all contracts under the [token](../contracts/token/) directory as well as the [allowlist](../contracts/allowlist/) directory. 
+This document encompasses information for all contracts under the [token](../contracts/token/) directory as well as the [allowlist](../contracts/allowlist/) directory.
 
 ## Context
 
-The ERC721 presets built by immutable were done with the requirements of cheaper onchain minting and flexible project management for games. Namely:
+The ERC721 presets built by Immutable were done with the requirements of cheaper onchain minting and flexible project management for games. Namely:
 
 - Studios should be able to mint multiple tokens efficiently to multiple addresses.
 
@@ -17,12 +18,11 @@ The ERC721 presets built by immutable were done with the requirements of cheaper
 
 - Contracts should not be upgradeable to prevent external developers from getting around royalty requirements.
 
-
 ## Design and Implementation
 
 ### ImmutableERC721
 
-The ImmutableERC721 contract is a hybrid of Openzepplin implementation of [ERC721Burnable](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/extensions/ERC721Burnable.sol) and the [ERC721Psi](https://github.com/estarriolvetch/ERC721Psi/blob/main/contracts/ERC721Psi.sol) implementation. This is to give the studios flexibility on their minting strategies depending on their use cases. 
+The ImmutableERC721 contract is a hybrid of Openzepplin implementation of [ERC721Burnable](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/extensions/ERC721Burnable.sol) and the [ERC721Psi](https://github.com/estarriolvetch/ERC721Psi/blob/main/contracts/ERC721Psi.sol) implementation. This is to give the studios flexibility on their minting strategies depending on their use cases.
 
 The contract interface allows users to call methods to bulk mint multiple tokens either by ID or by quantity to multiple addresses.
 
@@ -41,23 +41,25 @@ The ImmutableERC721MintByID contract is a subset of the ImmutableERC721 contract
 - Modified ERC721Psi `_safeMint` and `safeMint` methods to not call the overridden `_mint` methods but to call its own internally defined `_mint`
 - Added a `_idMintTotalSupply` to help keep track of how many tokens have been minted and belong to a non-zero address for the `totalSupply()` method.
 - Added Modifiers to `transfer` and `approve` related methods to enforce correct operator permissions.
-- Added various bulk minting methods to allow the minting of multiple tokens to multiple addresses. These methods come with new structs. 
-- Added support for EIP4494 Permits. This feature comes with an additional nonce mapping that is needed to help keep track of the validity of permits. We decided to remove support for allowing `approved` contract addresses to validate and use permits as it does not fit any of the uses cases in Immutable's ecosystem, and there is no reliable method of getting all of the approved operators of a token. 
-
+- Added various bulk minting methods to allow the minting of multiple tokens to multiple addresses. These methods come with new structs.
+- Added support for EIP4494 Permits. This feature comes with an additional nonce mapping that is needed to help keep track of the validity of permits. We decided to remove support for allowing `approved` contract addresses to validate and use permits as it does not fit any of the uses cases in Immutable's ecosystem, and there is no reliable method of getting all of the approved operators of a token.
 
 ## Attack Surfaces
 
-The contract has no access to any funds. The risks will come from compromised keys that are responsible for managing the admin roles that control the collection. As well as permits and approves if an user was tricked into creating a permit that can be validated by a malicious eip1271 wallet giving them permissions to the user's token. 
+The contract has no access to any funds. The risks will come from compromised keys that are responsible for managing the admin roles that control the collection. As well as permits and approves if an user was tricked into creating a permit that can be validated by a malicious eip1271 wallet giving them permissions to the user's token.
 
 Potential Attacks:
+
 - Compromised Admin Keys:
-    - The compromised keys are able to assign the `MINTER_ROLE` to malicious parties and allow them to mint tokens to themselves without restriction
-    - The compromised keys are able to update the `OperatorAllowList` to white list malicious contracts to be approved to operate on tokens within the collection
+  - The compromised keys are able to assign the `MINTER_ROLE` to malicious parties and allow them to mint tokens to themselves without restriction
+  - The compromised keys are able to update the `OperatorAllowList` to white list malicious contracts to be approved to operate on tokens within the collection
 - Compromised Offchain auth:
-    - Since EIP4494 combined with EIP1271 relies on off chain signatures that are not standard to the ethereum signature scheme, user auth info can be compromised and be used to create valid EIP1271 signatures.
+  - Since EIP4494 combined with EIP1271 relies on off chain signatures that are not standard to the ethereum signature scheme, user auth info can be compromised and be used to create valid EIP1271 signatures.
 
 ## Tests
-`npx hardhat test` will run all the related tests for the above mentioned repos. The test plan and cases are written in the test files describing the scenario is it testing for. 
 
-## Diagram 
-![](./202309-threat-model-preset-erc721/immutableERC721.png)
+`npx hardhat test` will run all the related tests for the above mentioned repos. The test plan and cases are written in the test files describing the scenario is it testing for.
+
+## Diagram
+
+![](./202309-threat-model-preset-erc721/immutableERC721.png)
diff --git a/audits/token/202312-threat-model-preset-erc1155.md b/audits/token/202312-threat-model-preset-erc1155.md
@@ -1,28 +1,41 @@
 ## Introduction
-This document is a thread model for the preset erc1155 token contracts built by Immutable.
 
-This document encompasses information for all contracts under the [token](../contracts/token/erc1155) directory
+This document is a thread model for the preset ERC1155 token contracts built by Immutable.
+
+Contracts covered under this model include:
+
+- [ImmutableERC1155](https://github.com/immutable/contracts/blob/1ddb3dd78a7d9352572a226d56e39e7a82776585/contracts/token/erc1155/preset/ImmutableERC1155.sol)
+
+as found in the commit hash `1ddb3dd` of the Immutable [contracts repository](https://github.com/immutable/contracts).
 
 ## Context
 
-The ERC1155 presets built by immutable were done with the requirements of supply tracking and permits
+The ERC1155 presets built by Immutable were done with the requirements of supply tracking and permits.
 
 - Clients should be able to track how many tokens of a specific token id in a collection is in circulation
 
 - Clients should be able to create permits for unapproved wallets to operate on their behalf
 
-- Minting should be restricted to addresses that were granted the `minter` role.
-
-- Only allow operators should be able to modify and assign roles to addresses for administering the collection on chain.
+- Minting should be restricted to addresses that were granted the `minter` role
 
-- Contracts should not be upgradeable to prevent external developers from getting around royalty requirements.
+- Only allowed operators should be able to modify and assign roles to addresses for administering the collection on chain
 
+- Contracts should not be upgradeable to prevent external developers from getting around royalty requirements
 
 ## Design and Implementation
 
 ### ImmutableERC1155
-The ImmutableERC1155 extends OZ's `ERC1155Burnable` contract inheriting the public burn methods to be used by the client. 
-Permit is added to allow for Gasless transactions from the token owners.
+
+`ImmutableERC1155` inherits the [ImmutableERC1155Base](https://github.com/immutable/contracts/blob/1ddb3dd78a7d9352572a226d56e39e7a82776585/contracts/token/erc1155/abstract/ImmutableERC1155Base.sol) contract and provides public functions for single and batch minting that are access controlled.
+
+`ImmutableERC1155Base` inherits contracts:
+
+- [OperatorAllowlistEnforced](https://github.com/immutable/contracts/blob/1ddb3dd78a7d9352572a226d56e39e7a82776585/contracts/allowlist/OperatorAllowlistEnforced.sol) - for setting an OperatorAllowlist that enables the restriction of approvals and transfers to allowlisted users
+- [ERC1155Permit](https://github.com/immutable/contracts/blob/1ddb3dd78a7d9352572a226d56e39e7a82776585/contracts/token/erc1155/abstract/ERC1155Permit.sol) - an implementation based on an [Open Zeppelin permit extension](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/IERC20Permit.sol), for allowing approvals to be made via EIP712 signatures in order to allow for gasless transactions from the token owners
+- [ERC2981](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/common/ERC2981.sol) - an implementation of the NFT Royalty Standard for retrieving royalty payment information
+- [MintingAccessControl](https://github.com/immutable/contracts/blob/1ddb3dd78a7d9352572a226d56e39e7a82776585/contracts/access/MintingAccessControl.sol) - implements access control for the `minter` role
+
+The `ERC1155Permit` implementation inherits the OpenZeppelin [ERC1155Burnable](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/extensions/ERC1155Burnable.sol) contract, which provides the public burn methods to be used by the client.
 
 #### Modifications From Base Implementation
 
@@ -31,12 +44,79 @@ Permit is added to allow for Gasless transactions from the token owners.
 - Override `uri` to return `baseURI` field to keep in standard with ImmutableERC721
 - Added `baseURI` to replace `uri` to encourage the usage of `baseURI`
 
-
 ## Attack Surfaces
 
-ERC1155 only has `setApproveForAll` as it's approval method. Meaning any flow that requires a 3rd party to operator on a set of tokens owned by another wallet will grant the third party access to all of that specific wallet's tokens. The third party needs to be entirely trustworthy. The owner needs to be diligent on revoking unrestricted access when not needed. 
-
-We can consider implementing a more complicated approval schema if needed. i.e by token id or by token id and amount. 
+ERC1155 only has `setApprovalForAll` as it's approval method. Meaning any flow that requires a 3rd party to operate on a set of tokens owned by another wallet will grant the third party access to all of that specific wallet's tokens. The third party needs to be entirely trustworthy. The owner needs to be diligent on revoking unrestricted access when not needed.
+
+The contract has no access to any funds. Additional risks can come from compromised keys that are responsible for managing the admin roles that control the collection. As well as permits and approves if an user was tricked into creating a permit that can be validated by a malicious eip1271 wallet giving them permissions to the user's token.
+
+Potential Attacks:
+
+- Compromised Admin Keys:
+  - The compromised keys are able to assign the `MINTER_ROLE` to malicious parties and allow them to mint tokens to themselves without restriction
+- Compromised Offchain auth:
+  - Since EIP4494 combined with EIP1271 relies on off chain signatures that are not standard to the ethereum signature scheme, user auth info can be compromised and be used to create valid EIP1271 signatures.
+
+## Attack Mitigation
+
+- The contract contains access control patterns to limit the vulnerabilty of compromised roles
+- The keys associated with the `DEFAULT_ADMIN_ROLE` should be operated by secure measures, for example a multi-signature wallet such that an attacker would need to compromise multiple signers simultaneously, or a securely stored hardware wallet.
+
+### Externally Visible Functions
+
+An attacker could formulate an attack in which they send one or more transactions that execute one or more of these functions.
+
+Functions that _change_ state:
+| Name | Function Selector | Access Control |
+| ------------------------------------------------------------- | ----------------- | --------------------- |
+| burn(address,uint256,uint256) | f5298aca | Caller must be token owner or approved |
+| burnBatch(address,uint256[],uint256[]) | 6b20c454 | Caller must be token owner or approved |
+| grantMinterRole(address) | 3dd1eb61 | DEFAULT_ADMIN_ROLE |
+| grantRole(bytes32,address) | 2f2ff15d | DEFAULT_ADMIN_ROLE |
+| permit(address,address,bool,uint256,bytes) | d6b0b3f1 | The approval of token spend is authorised by the offchain signing of an EIP712 blob which is validated to be originating from the token owner |
+| renounceRole(bytes32,address) | 36568abe | Caller must be the account being revoked |
+| revokeMinterRole(address) | 69e2f0fb | DEFAULT_ADMIN_ROLE |
+| revokeRole(bytes32,address) | d547741f | DEFAULT_ADMIN_ROLE |
+| safeBatchTransferFrom(address,address,uint256[],uint256[],bytes) | 2eb2c2d6 | Caller must be token owner or approved |
+| safeMint(address,uint256,uint256,bytes) | 5cfa9297 | MINTER_ROLE |
+| safeMintBatch(address,uint256[],uint256[],bytes) | c39dfed8 | MINTER_ROLE |
+| safeTransferFrom(address,address,uint256,uint256,bytes) | f242432a | Caller must be token owner or approved |
+| setApprovalForAll(address,bool) | a22cb465 | None - permisionless. Caller can only set approval for their own tokens |
+| setBaseURI(string) | 55f804b3 | DEFAULT_ADMIN_ROLE |
+| setContractURI(string) | 938e3d7b | DEFAULT_ADMIN_ROLE |
+| setDefaultRoyaltyReceiver(address,uint96) | 885e7a08 | DEFAULT_ADMIN_ROLE |
+| setNFTRoyaltyReceiver(uint256,address,uint96) | 439aed34 | MINTER_ROLE |
+| setNFTRoyaltyReceiverBatch(uint256[],address,uint96) | a7012816 | MINTER_ROLE |
+
+Functions that _do not change_ state (they are all permissionless):
+| Name | Function Selector |
+| ------------------------------------------------------------- | ----------------- |
+| DEFAULT_ADMIN_ROLE() | a217fddf |
+| DOMAIN_SEPARATOR() | 3644e515 |
+| MINTER_ROLE() | d5391393 |
+| balanceOf(address,uint256) | 00fdd58e |
+| balanceOfBatch(address[],uint256[]) | 4e1273f4 |
+| baseURI() | 6c0360eb |
+| contractURI() | e8a3d485 |
+| eip712Domain() | 84b0196e |
+| exists(uint256) | 4f558e79 |
+| getAdmins() | 31ae450b |
+| getRoleAdmin(bytes32) | 248a9ca3 |
+| getRoleMember(bytes32,uint256) | 9010d07c |
+| getRoleMemberCount(bytes32) | ca15c873 |
+| hasRole(bytes32,address) | 91d14854 |
+| isApprovedForAll(address,address) | e985e9c5 |
+| nonces(address) | 7ecebe00 |
+| operatorAllowlist() | 29326f29 |
+| royaltyInfo(uint256,uint256) | 2a55205a |
+| supportsInterface(bytes4) | 01ffc9a7 |
+| totalSupply(uint256) | bd85b039 |
+| uri(uint256) | 0e89341c |
 
 ## Tests
-`forge test` will run all the related tests.
+
+`forge test` will run all the related tests.
+
+## Diagram
+
+![](./202312-threat-model-preset-erc1155/ImmutableERC1155.jpg)
diff --git a/audits/token/202312-threat-model-preset-erc1155/ImmutableERC1155.jpg b/audits/token/202312-threat-model-preset-erc1155/ImmutableERC1155.jpg
diff --git a/contracts/token/erc1155/preset/README.md b/contracts/token/erc1155/preset/README.md
@@ -2,12 +2,13 @@
 
 The ImmutableERC1155 contracts allows clients to mint multiple different tokens with different token ids within the same collection. The contract features methods to allow for minting multiples of multiple token ids to simplify the minting flow and reduce gas costs. This contract is built on top of the [Openzeppelin implemention of EIP-1155](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/ERC1155.sol).
 
-[Read more On the Threat Model](../../../audits/202312-threat-model-preset-erc1155.md)  
+[Read more On the Threat Model](../../../../audits/token/202312-threat-model-preset-erc1155.md)
+
 [Read more On the EIP](https://eips.ethereum.org/EIPS/eip-1155)
 
 ## preset/ImmutableERC1155
 
-The ImmutableERC1155 contract is a version of the Immutable's preset 1155 contract. It has been internally audited and is ready to be used. The contract contains all external facing interfaces that are needed to interact(read and write) with deployed ERC1155 collections. 
+The ImmutableERC1155 contract is a version of the Immutable's preset 1155 contract. It has been internally audited and is ready to be used. The contract contains all external facing interfaces that are needed to interact(read and write) with deployed ERC1155 collections.
 
 ## abstract/ERC1155Permit