Fireblocks Upgradeable Tokens Audit - ERC721F

OpenZeppelin Security | March 12, 2025

Table of Contents

Summary

Type
DeFi
Timeline
From 2023-11-13
To 2023-11-17
Languages
Solidity
Total Issues
14 (10 resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
1 (1 resolved)
Medium Severity Issues
1 (1 resolved)
Low Severity Issues
2 (1 resolved, 1 partially)
Notes & Additional Information
10 (7 resolved)

Scope

We audited the Fireblocks repository at commit e7003dfb.

Following the conclusion of the audit, Fireblocks has updated the license to AGPL-3.0-or-later in pull request #1 at commit 6d8327b. This update has had no impact on the security of the source code logic.

In scope were the following contracts:

 contracts
├── ERC721F.sol
└── library
    ├── AccessRegistry
    │   ├── AccessListUpgradeable.sol
    │   ├── AccessRegistrySubscriptionUpgradeable.sol
    │   ├── AllowList.sol
    │   ├── DenyList.sol
    │   └── interface
    │       └── IAccessRegistry.sol
    ├── Errors
    │   ├── LibErrors.sol
    │   └── interface
    │       └── IERC721Errors.sol
    └── Utils
        ├── ContractUriUpgradeable.sol
        ├── MarketplaceOperatorUpgradeable.sol
        ├── PauseUpgradeable.sol
        ├── RoleAccessUpgradeable.sol
        ├── SalvageUpgradeable.sol
        └── TokenUriUpgradeable.sol

Update: This codebase was migrated to a new repository at https://github.com/fireblocks/fireblocks-smart-contracts, which was audited until commit a8942e9 in PR#3. This PR was later merged into the main branch at commit 108a92f.

System Overview

The Fireblocks Upgradeable Tokens project implements three upgradeable token contracts, namely, ERC721F. This is a compliant with the ERC-721 token standard and additional functionality has been integrated for managing access control. The token facilitate the configuration of an access list for token transfers, providing the flexibility of implementing either an allowlist, where only addresses on the list can utilize the tokens, or a denylist, which restricts addresses on the list from interacting with the contract.

Privileged Roles

The protocol implements multiple privileged roles that are tied to the ERC721F contract.

The holder of DEFAULT_ADMIN_ROLE can:

  • Grant roles to other addresses.

The holder of UPGRADER_ROLE can:

  • Upgrade the implementation contracts to a new version.

The holder of PAUSER_ROLE can:

  • Pause and unpause a contract.

The holder of CONTRACT_ADMIN_ROLE can:

  • Change the used access list address.
  • Update the contract URI.
  • Update the token URI address.
  • Update the marketplace operator address.

The holder of MINTER_ROLE can:

  • Mint tokens.

The holder of BURNER_ROLE can:

  • Burn tokens.

The holder of RECOVERY_ROLE can:

  • Recover tokens from an account that is missing access.

The holder of SALVAGE_ROLE can:

  • Recover ETH and ERC-20 tokens that were sent to a token contract itself.

Trust Assumptions

  • The holders of the DEFAULT_ROLE_ADMIN role are expected to be non-malicious and to act in the project's best interest.
 
 

High Severity

ERC721F Transfer Access List Can Be Bypassed

The ERC721F contract implements logic to verify whether the sender and recipient have access. This functionality is introduced by overriding the transferFrom function and including the necessary access checks. However, an issue arises because the contract inherits from the ERC721Upgradeable contract, which includes two additional functions, both named safeTransferFrom but having different parameters. These functions do not internally utilize the overridden transferFrom function but instead rely on the _safeTransfer function. Consequently, a user without the appropriate access can use the safeTransferFrom functionality to circumvent the access list checks and successfully transfer the tokens.

Consider introducing additional checks to ensure that proper handling of transfers is conducted through the safeTransferFrom functionality.

Update: Acknowledged, resolved in commit 7710940.

Medium Severity

Burner Can Burn Any ERC721F Token

The ERC721F contract implements a burn function designed to be executed exclusively by the address with the BURNER_ROLE role. The function is intended to allow the burning of only those tokens owned by the caller, as indicated by the function's NatSpec description. However, the function directly calls the _burn function without performing any essential checks to verify whether the caller is the owner of the token being burned or has the approval to burn it.

Consider incorporating a check using the _isApprovedOrOwner function to ensure that _msgSender() is either the approved party or the owner of the specified token.

Update: Acknowledged, resolved in commit a5e2488.

Low Severity

Follow Checks-Effects-Interactions (CEI) Pattern

The ERC721F contract implements a mint function that does not follow the CEI pattern. The function mints a token using _safeMint, which ensures that the recipient is a contract using the onERC721Received hook. This allows a recipient to hijack the execution and then set the token URI through the _tokenUriUpdate function.

Consider first updating the token URI and then calling _safeMint.

Update: Acknowledged, resolved in commit db999c8.

Retrieving the Whole Access List Might Revert

The AccessListUpgradeable contract includes a getAccessList function which retrieves all values from the access list. This function utilizes the values function of EnumerableSetUpgradeable.AddressSet, copying the entire storage into memory. This might lead to problems when the access list is excessively large as the function execution remains susceptible to the block gas limit, resulting in the call getting reverted.

Consider implementing an additional function that can be used when the value set gets too large. This function should return only a subset or slice of the values from the entire set.

Update: Acknowledged, partially resolved in commit 130dbb5. The Fireblocks team stated:

To address this issue, we've tackled it by incorporating additional documentation into the function. This decision was prompted by the fact that the purpose of getAccessList() is to serve as a support function tailored for fetching compact access lists in smaller projects. In the case of larger projects equipped with more extensive access lists, it is advisable to devise custom indexing logic to efficiently retrieve all addresses from the access list.

Notes & Additional Information

Lack of Security Contact

Providing a specific security contact (e.g., an email address or ENS name) within a smart contract greatly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. Additionally, if the contract incorporates third-party libraries and a bug surfaces in them, it becomes easier for the maintainers of those libraries to contact the appropriate person about the problem and provide mitigation instructions.

Consider adding a NatSpec comment containing a security contact on top of the contract definitions. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.

Update: Acknowledged, not resolved. The Fireblocks team stated:

Because these contracts are crafted as templates for other companies and entities to deploy on a larger scale. There cannot be a single security contact. Therefore, when a company deploys one of these contracts, they are are welcome to provide their own security contact independently through the contract URI.

Missing Named Parameters in Mapping

Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of a mapping's purpose.

For instance, the _tokenUri mapping in the TokenUriUpgradeable contract does not utilize named parameters.

Consider adding named parameters to the mappings in order to improve readability and maintainability of the codebase.

Update: Acknowledged, resolved in commit 967c097.

Unnecessary Variable Cast

In the AccessRegistrySubscriptionUpgradeable contract, the _accessRegistry variable in the _accessRegistryUpdate function is unnecessarily casted.

To improve the overall clarity, intent, and readability of the codebase, consider removing any unnecessary casts.

Update: Acknowledged, resolved in commit 69c5add.

Unused Imports

Throughout the codebase, there are multiple imports that are unused and could be removed:

Consider removing unused imports to improve the overall clarity and readability of the codebase.

Update: Acknowledged, resolved in commit 4243399.

Lack of __gap Variables

Throughout the codebase, there are multiple upgradeable contracts that do not have a __gap variable. For instance:

Note that the current implementation does not require the implementation of the __gap storage variable, as the listed contracts are not being inherited. However, it is still recommended to include it as it implies that the contract implementations might be upgraded and additional storage variables might be introduced in the future.

Consider adding a __gap storage variable to avoid future storage clashes in upgradeable contracts.

Update: Acknowledged, not resolved. The Fireblocks team stated:

Contracts lacking __gap variables are not needed for our use case since upgrades are intended to occur through contract inheritance. In this process, the initial implementation, V1, is inherited by V2, where the new logic resides. This enables the addition of storage slots without any conflict with existing slots.

Incorrect Storage Gap Size in AccessListUpgradeable

The AccessListUpgradeable contract uses a __gap storage variable of size 49. It assumes that the _accessList storage variable occupies one storage slot, whereas in reality, it takes two storage slots. The _accessList is of type EnumerableSetUpgradeable.AddressSet which is a struct that uses 2 storage slots.

Consider changing the size of the __gap storage variable in the AccessListUpgradeable contract from 49 to 48.

Update: Acknowledged, resolved in commit 519e415.

Confusing Error Used

The ERC721F contract features a recoverTokens function designed to recover tokens from accounts lacking access. The problem arises when the account already has access to the tokens, resulting in transactions reverting with a potentially confusing error such as ERC721InvalidReceiver. In this context, since the account is not the receiver, the error should not imply receiver-related issues.

Consider adding a new error which indicates that the account has access to the tokens.

Update: Acknowledged, resolved in commit 6b3f311.

Typographical Errors and Incorrect Comments

Consider addressing the following typographical errors and incorrect comments:

Update: Acknowledged, resolved in commit 7a5e68e.

Gas Optimization

In the codebase, there are several instances of potential gas optimizations:

  • The _requireHasAccess function, present in the ERC721F contract, redundantly checks the isSender value twice. It would be more efficient to initially verify if the account has access through the accessRegistry contract. If it does not have access, then based on the isSender value, either revert with an invalid sender or invalid receiver error.
  • The codebase follows a common pattern of checking access after updating state variables. For instance, in the RoleAccessUpgradeable contract, the revokeRole function calls the _authorizeRoleAccess function after updating the role. To prioritize failing early and conserving gas, consider performing the access check before updating the state variables.

To improve gas consumption, readability and code quality, consider refactoring wherever possible.

Update: Acknowledged, resolved in commit f84408b.

Missing Functionality For Rescuing ERC-721 and ERC-1155 Tokens

The SalvageUpgradeable abstract contract, inherited by the ERC721F contract, implements the logic for rescuing ETH and ERC-20 tokens. This is useful for when tokens are accidentally sent to the token contract itself instead of a valid recipient. However, the rescue logic only works for rescuing ETH and ERC-20 tokens, not ERC-721 or ERC-1155 tokens. As such, while the ERC-20 tokens accidentally sent to any of the three token contracts can be rescued, ERC-721 or ERC-1155 tokens cannot be.

Consider adding the functionality for rescuing ERC-721 and ERC-1155 tokens accidentally sent to the ERC721F contract.

Update: Acknowledged, not resolved. The Fireblocks team stated:

Currently, there is no observed customer demand for salvaging an ERC-721 or ERC-1155 token. While this functionality could potentially be incorporated later through an upgrade, we are cautious about subjecting users to additional bytecode and gas fees for a feature that may not be utilized at the present moment.

 
 
 

Conclusion

The Fireblocks Upgradeable Tokens project introduces upgradeable token contract adhering to ERC-721 standard, with added features for access control. These tokens allow the creation of an access list for transfers, enabling the implementation of either an allow list, restricting usage to specified addresses, or a deny list, preventing interactions with addresses on the list.

One high and one medium-severity vulnerability was identified, along with multiple lower-severity issues. In addition, various recommendations have been made to enhance the quality and documentation of the codebase. The Blockfold team was responsive and timely answered any questions we had about the codebase.

Request Audit

 

Related Posts