We audited the thesandboxgame/sandbox-smart-contracts repository at the 5789d5a3f7dc4dc5c32293cd7615f97d395c93ae commit.
In scope were the following contracts:
packages/
└─ asset/
└─ contracts/
└─ Catalyst.sol
└─ interfaces/
└─ ICatalyst.sol
This system introduces Catalyst which is an ERC-1155 token. It adopts royalty distribution to users based on the ERC-2981 standard and complies with OpenSea’s operator filter registry across all chains to stay updated with prevailing standards. Catalysts have specific tiers that allow the minter role to distribute diverse tokens to users. Administrators have the flexibility to augment these tiers, increasing the range of token-minting possibilities in the future.
The Catalyst contract is used to manage distinctive ERC-1155 tokens, also called catalysts, and pay the royalty through the RoyaltyDistributor contract. These catalyst tokens can be minted individually or in a batch by the MINTER_ROLE privileged role. Similarly, they can be burnt, individually or in batches by the users or by the BURNER_ROLE privileged role.
The audited codebase defines 7 types of catalysts: TSB_EXCLUSIVE, COMMON, UNCOMMON, RARE, EPIC, LEGENDARY, and MYTHIC. Administrators have the flexibility to add new catalyst types, thereby increasing the range of token-minting possibilities in the future.
The audited contracts contain the following privileged roles:
The admin of the Catalyst contract can perform the following actions:
trustedForwardertokenIdThe MINTER_ROLE defined in the Catalyst contract can mint individual or batches of ERC-1155 catalyst tokens.
The BURNER_ROLE defined in the Catalyst contract can burn individual or batches of ERC-1155 catalyst tokens from a specific address.
During the audit, it is assumed that the privileged addresses are trusted entities and would work in the best interest of the platform and its users.
Several docstrings and inline comments throughout the codebase were found to be erroneous and should be fixed. In particular:
addNewCatalystType function states an incorrect definition of the ipfsCID variable.setOperatorRegistry function incorrectly states that the function "sets filter registry address deployed in test". The function will also be used on the mainnets.Update: Resolved in pull request #1111. The Sandbox team stated:
Documentation has been updated
The following functions do not emit relevant events after executing sensitive actions:
_baseUri is set in the initialize function in the Catalyst contract._baseUri is set in the setBaseURI function in the Catalyst contract.operatorFilterRegistry is set in the setOperatorRegistry function in the Catalyst contract.Consider emitting events after sensitive changes occur to facilitate tracking and notify off-chain clients following the contracts' activity.
Update: Resolved in pull request #1113. The Sandbox team stated:
Added all suggested events
gap VariablesThroughout the codebase, there are multiple upgradeable contracts that do not have a gap variable. For instance:
Consider adding a gap variable to avoid future storage clashes in upgradeable contracts.
Update: Resolved in pull request #1114. The Sandbox team stated:
Added gap variables.
Catalyst Contract Allows the Burning and Transfer of Non-Existent TokensA user can burn their own tokens or tokens that they are approved to use by calling the burn or burnBatch functions respectively. Moreover, the burnFrom and burnBatchFrom functions in the Catalyst contract allow the privileged addresses with the BURNER_ROLE to burn tokens from any account.
The safeTransferFrom and safeBatchTransferFrom functions can be called by whitelisted operators for transferring tokens to the users.
The Catalyst contract allows the minting or transferring of tokens that have a tokenId between 1 and highestTierIndex. However, the valid tokenIds are not checked during the burning or transfer of tokens, thereby allowing users to transfer or burn arbitrary ERC-1155 tokens.
Consider checking for valid tokenIDs before burning tokens.
Update: Resolved in pull request #1117. This is not an issue. The Sandbox team stated:
Its not possible to burn or transfer non-existing tokens due to checks implemented in ERC1155. Added test cases.
BURNER_ROLE Role Not Initialized During Catalyst Contract InitializationThe Catalyst contract defines a BURNER_ROLE role that can burn single or batch of tokens from a specified address by calling the burnFrom or burnBatchFrom functions respectively. However, no address is assigned to this role.
While the admin of the contract can call the grantRole function inherited from the AccessControlUpgradeable contract, the burnFrom and burnBatchFrom functions cannot be called until the BURNER_ROLE role is assigned.
Consider assigning the BURNER_ROLE role from within the initialize function.
Update: Resolved. This is not an issue. The Sandbox team stated:
Only AssetCreate will be given a burner role. The role will be assigned only after AssetCreate is deployed through deployment scripts. No action taken here.
public Functions That Should Have external VisibilityThe following public functions should be external:
initialize function in the Catalyst contract.Consider changing the visibility of these functions to external in order to clarify that these functions will only be called by external contracts.
Update: Resolved in pull request #1126.
To improve code readability, consider removing the following typographical errors from the codebase:
Catalyst.sol, "THis" should be "This"Catalyst.sol, "aditional" should be "additional"Catalyst.sol, "Opensea.can" should be "Opensea. Can"Update: Resolved in pull request #1128. The Sandbox team stated:
Fixed suggested and other found typographical errors.
Throughout the codebase, there are imports that are unused and could be removed. For instance:
ERC2981Upgradeable of Catalyst.solIRoyaltyManager of Catalyst.solIERC2981Upgradeable of Catalyst.solConsider removing unused imports to improve the overall clarity and readability of the codebase.
Update: Resolved. The issue was resolved here.The Sandbox team stated:
Removed as part of the code delivered shortly after audit start.
The Catalyst contract overrides the safeTransferFrom and safeBatchTransferFrom functions from the inherited ERC1155Upgradeable contract from the OpenZeppelin contracts library. The onlyAllowedOperator modifier has been added to these functions to ensure only valid operators are able to transfer tokens by validating the _msgSender against the operator filterer registry. These functions then call the internal _safeTransferFrom and _safeBatchTransferFrom functions from the ERC1155Upgradeable contract which do not perform allowance checks. As a result, anyone can call these functions and freely transfer tokens from any account to any other account.
Consider calling the safeTransferFrom and safeBatchTransferFrom functions instead of the _safeTransferFrom and _safeBatchTransferFrom functions from the ERC1155Upgradeable contract to ensure appropriate allowance checks are performed.
Update: Resolved in pull request #1350.
Several low-severity issues and notes were reported, mainly addressing improvement opportunities to the overall quality of the codebase.