thirdweb A-19

DelayedRevealBatchMetadataERC721 utilizes symmetric encryption to encrypt/decrypt the token URIs, meaning that the same encryption key is used for encrypting and decrypting the URI. The process is the following:

1. A minter (someone holding the MINTER_ROLE) encrypts the URI off-chain by using the secret encryption key.
2. The minter calls uploadMetadata and passes the encryptedURI in the _data param.
3. Once the minter calls reveal with the proper encryption key, the encryptedURI is decrypted and the revealed URI can be retrieved via tokenURI.

The above process is secure, as long as the encryption key can be kept secret and doesn’t get into the hand of a malicious actor. However, exactly this can happen when the minter calls either

• encryptDecrypt to encrypt the data
• or the helper function getRevealURI for e.g. verification purposes.

Even the calls are declared as pure and view function and don’t generate an actual transaction on the chain, they can still be potentially intercepted on the RPC node. If this happens, a malicious actor can gain knowledge of the secret to take advantage of revealing the encrypted URIs before they are actually revealed by the minter, and thereby defeating the whole purpose of the “delayed reveal” functionality.

Remediation to Consider

The data encryption process should be fully off-chain, allowing the encryptDecrypt function to be changed to private. Also, completely remove the public getRevealURI function from the DelayedRevealBatchMetadataERC721 contract.

Callback functions can be marked by the core contract as either OPTIONAL or REQUIRED. Marking the callback as OPTIONAL means that the execution shouldn’t revert if the callback function isn’t implemented by the extension.

However, this is currently not the case, as trying to execute the callback via _executeCallbackFunction reverts when an OPTIONAL callback is not implemented by the extension.

In ModularCore._executeCallbackFunction, the following logic implemented to check for callbacks:

  if (callbackFunction.implementation != address(0)) {
      (success, returndata) = callbackFunction.implementation.delegatecall(_abiEncodedCalldata);
  } else {
      if (callbackMode == CallbackMode.REQUIRED) {
          revert CallbackFunctionRequired();
      }
  }

  if (!success) {
      _revert(returndata, CallbackExecutionReverted.selector);
  }

Reference: ModularCore.sol#L309-L319

The above logic works correctly for REQUIRED callbacks because the tx reverts with an CallbackFunctionRequired() error when the callback is not implemented. However, it doesn’t work for OPTIONAL callbacks. In this case, the delegatecall would return success=false, causing the tx revert on the final !success check.

Remediation to Consider

Modify the above logic to not revert on OPTIONAL callbacks.

mint() function in ERC1155CoreInitializable and ERC1155Core contract:

    function mint(address to, uint256 tokenId, uint256 value, bytes memory data) external payable {
        _beforeMint(to, tokenId, value, data);
        _mint(to, tokenId, value, "");

        _totalSupply[tokenId] += value;
    }

Reference: ERC1155Core.sol#L169-L174 , ERC1155Core.sol#L184-L189

Notice here that the _totalSupply variable will get updated after _mint(). While in the _mint() function, it will call to to.onERC1155Received() :

    function _mint(address to, uint256 id, uint256 amount, bytes memory data) internal virtual {
        ... mint logic
        
        // basically calling to `to.onERC1155Received()` if `to` is a contract
        if (_hasCode(to)) _checkOnERC1155Received(address(0), to, id, amount, data);
    }

Because _totalSupply variable will get updated after _mint() , it can potentially lead to reentrancy. Even though, in the current code there’s no harm for reentrancy to exploit this because _totalSupply has no use case currently, it can be a huge risk when there are extensions that rely on _totalSupply . Let’s take an example:

• Builder builds an extension contract for ERC1155Core , which can limit the total supply of each tokenId to 5. To do that, the builder will have to develop beforeMintERC1155() function in the extension contract. It will look something like this (this is a pseudo-code):

function beforeMintERC1155(address _caller, address _to, uint256 _id, uint256 _quantity, bytes memory _data)
        external
        payable
        virtual
        returns (bytes memory result)
    {
        ...
        currentTotalSupply = ERC1155_CORE.totalSuppy(_id);
        require(currentTotalSupply + _quantity <= 5, "Not allowed);
        ...
    }

• Malicious users notice that mint() doesn’t follow the CEI pattern, so they can build a malicious contract that looks something like this (this is a pseudo-code):

contract Malicious {
    function mint() external {
        // mint 4 token with tokenId = 999 to this address
        ERC1155Core.mint((address(this), 999, 4, "");
    }
    
    function onERC1155Received(address,address,uint256,uint256,bytes) external returns(bytes4){
        // check we minted 100 token with tokenId = 999 to this address
        if (ERC1155.balanceOf(address(this, 999) < 100) {
            // mint 4 token with tokenId = 999 to this address
            ERC1155Core.mint((address(this), 999, 4, "");
        }	
        return 0xf23a6e61;
    }
}

• In the end, malicious users can bypass the invariant “Not allowed to mint more than 5 tokens for each tokenId”

Remediations to Consider

Make mint() follow CEI pattern. Also while burn() is not affected by reentrancy, it’s recommended to follow the CEI pattern

During the installation and uninstallation of an extension, if registerInstallationCallback is set in the config, onInstall and onUninstall will be called respectively.

However, there could be extensions that only properly implement the onInstall callback, but are missing an onUninstall. In this case, the extension gets properly installed but is reverting when trying to get uninstalled in ModularCore._uninstallExtension:

if (config.registerInstallationCallback) {
    (bool success, bytes memory returndata) = _extension.call{value: msg.value}(
        abi.encodeCall(IInstallationCallback.onUninstall, (msg.sender, _data))
    );
    if (!success) {
        _revert(returndata, CallbackExecutionReverted.selector);
    }
}

Reference:

As per above, the call to onUninstall will fail for extensions not implementing an onUninstall function and returns success=false. Finally, it will revert on the last line with an CallbackExecutionReverted error.

This can be especially problematic where an extension gets compromised and it is needed to get an extension uninstalled.

Remediation to Consider

Make onUninstall optional and don’t revert if the call to onUninstall returns false.

In ClaimableERC20/721/1155, tokens can be claimed by either callers being part of the allowlist, or by callers specifying a properly signed ClaimRequest. For allowlisted callers, the overall price to be paid is determined by the current pricePerUnit and currency address set in the ClaimConditions. However, these values can be changed by an address holding the MINTER_ROLE any time. This can lead to situations where an allowlisted caller pays substantially more than what they expected to pay. Lets consider the following scenario:

1. Tx1: An NFT project is launched using the ClaimableERC721 extension. A privileged address holding the MINTER_ROLE sets the initial pricePerUnit to 1 ether and the currency address to USDC.
2. Tx2: An allowlisted caller initiates a transaction to buy 10 NFTs, expecting to pay 10 ether.
3. Tx3: The minter could potentially frontrun tx2 by sending a transaction to raise the price to 1,5 ether per NFT. Note that this can also happen accidentally and doesn’t necessarily require bad intention.
4. As a result, the whitelisted caller pays 15 ether (in USDC) as opposed to the expected 10 ether.

The above scenario only works when the caller approves a higher amount of tokens than the expected price to be paid. However, this is quite common for frontends to specify the max uint256 value as allowance, for better UX.

Remediation to Consider

Add the possibility for callers to specify the expected currency and pricePerUnit, so that the transaction reverts when there was a change in currency or price.

Extension contracts use the “Namespaced Storage Layout” to prevent collisions between different extensions as well as between extension and core contract.

However, this doesn’t protect against storage collisions between different version upgrades. It is important to layout the storage variables in a safe way to make them extendable for future versions. Specifically the layout of state variables used in /minting and /royalty cannot be considered safe for future upgrades.

Let’s take a look at the storage layout of ClaimableERC20.sol:

struct Data {
    // sale config: primary sale recipient, and platform fee recipient + BPS.
    ClaimableERC20.SaleConfig saleConfig;
    // claim condition
    ClaimableERC20.ClaimCondition claimCondition;
    // UID => whether it has been used
    mapping(bytes32 => bool) uidUsed;
}

saleConfig is stored at offset 0 (from the defined namespace) and only takes one storage slot since it only contains one address field:

struct SaleConfig {
    address primarySaleRecipient;
}

claimCondition is stored at offset 1 and takes multiple storage slots for the different fields defined in ClaimCondition.

Now lets assume we want to deploy a new version specifying an additional field platformFeeRecipient in SaleConfig.

struct SaleConfig {
    address primarySaleRecipient;
    address platformFeeRecipient;    // added in V1
}

Above change would result in a storage collision as platformFeeRecipient would overwrite the first field defined in claimConditions, which can lead to severe vulnerabilities. Similar issues exist in the other extensions of /minting and /royalty.

Remediation to Consider

Adapt storage layout of /minting and /royalty extensions to make them extendable and safe for future upgrades.

Adhering to ERC165 standard is especially important for architectures such as the present Modular Contract Framework to guarantee compatibility between different extensions and core contracts. There are a few issues being encountered by not adhering to the EIP165 specification.

1. ERC4906 (MetadataUpdate event): Some ERC721 extensions that change metadata are not complying to ERC4906. See L-6.

2. ERC2981 (NFT royalty): All the core contract’s supportsInterface functions already comply to ERC2981 (returning true for interfaceId == 0x2a55205a) even though they don’t support royalties by default. References:

   • ERC721CoreInitializable.sol#L100
   • ERC1155CoreInitializable.sol#L114
   • ERC721Core.sol#L89
   • ERC1155Core.sol#L113

   → Remediation: Remove 0x2a55205a form the core contract’s supportsInterface functions

3. ERC165 (supportsInterface): According to ERC165, supportsInterface must return true for interfaceId == 0x01ffc9a7, meaning the contract supports ERC165. This is currently not the case for the ERC20Core’s contracts. References:

   • ERC20Core.sol#L111
   • ERC20CoreInitializable.sol#L113

   → Remediation: Add 0x01ffc9a7 to the supportsInterface functions of above ERC20Core contracts.

4. ERC7572 (contractURI): All the core contracts comply to ERC7572 but doesn’t return true for ERC7525 interface in supportsInterface.

5. ERC173 (Ownership standard): The ModularCore contract complies to ERC173 but doesn’t return true for ERC173 interface in supportsInterface.

In ERC20Core and ERC1155Core, the name and symbol parameters are passed to the constructor and are meant to be assigned to their respective _name and _symbol state variables.

// Set contract metadata
_name = _name;
_symbol = _symbol;

Reference: ERC20Core.sol#L50-L51

However, as shown above, _name is assigned to _name instead of the passed parameter name. Same goes for the symbol parameter. As a result, name and symbol are not set on contract deployment and remain empty.

This could potentially hinder other protocols or frontends from integrating with Thirdweb’s ERC20 and ERC1155 tokens.

Remediation to Consider

Assign the passed parameter name and symbol to _name and _symbol, respectively.

In burn() function in ERC20Core and ERC20CoreInitializable contract

function burn(address from, uint256 amount, bytes calldata data) external {
    _beforeBurn(from, amount, data);

    _spendAllowance(from, msg.sender, amount);
    _burn(from, amount);
}

Reference: ERC20Core.sol#146-L151

Notice here that the function will always decrease from's allowance to caller. Even when the caller is also from , the function still decreases the allowance of themselves. As a consequence, in order to burn their own token, the caller must approve() to themselves first.

Remediations to Consider

Check if the caller is from , if it is, then the function should not decrease their allowance

In getExtensionConfig function in minting/ contracts, it’s not including EIP712’s eip712Domain() function as a fallback function.

As a result, eip712Domain() function can’t be called from the core contract, since it is not included as an allowed fallback function.

Remediations to Consider

Consider including eip712Domain() as a fallback function in extension’s getExtensionConfig() function. More specifically, the extension contracts that need to be fixed will be ClaimableERC20, ClaimableERC721, ClaimableERC1155, MintableERC20, MintableERC721, MintableERC1155.

In the core contracts, specifically ERC20/721/1155Core.sol and ERC20/721/1155CoreInitializable.sol, the initialize() functions contains the following check

require(extensions.length == extensions.length);

References:

ERC20CoreInitializable.sol#L58

ERC721CoreInitializable.sol#L54

ERC1155CoreInitializable.sol#L60

ERC20Core.sol#L56

ERC721Core.sol#L48

ERC1155Core.sol#L59

Obviously it will always pass this requirement. The intention here should be validating extension’s length and extensionInstallData’s length

Remediations to Consider

Make the following change to affected contracts:

-       require(extensions.length == extensions.length);
+       require(extensions.length == extensionInstallData.length);

In ModularCore contract, funds can be received via:

receive() external payable {}

ModularCore.sol#L80

This will allow anyone sending native token directly to the contract. Currently there’s no use case for this feature. Because of that, when this contract holds native tokens, there’s no easy way to withdraw them. The protocol owner would need to install an extensions specifically for withdrawing those native tokens.

Remediations to Consider

Consider removing receive() in ModularCore contract.

ERC4906 is an extension of EIP721 and defines the events when the token’s metadata is changed. Specifically, when the metadata for a single token is changed it should emit:

event MetadataUpdate(uint256 _tokenId);

and changing metadata for a range of tokens should emit:

event BatchMetadataUpdate(uint256 _fromTokenId, uint256 _toTokenId);

The following /metadata contracts comply to ERC4906:

• SimpleMetadataERC721: emits MetadataUpdate in setTokenURI
• OpenEditionMetadataERC721: emits BatchMetadataUpdate in setSharedMetadata

Whereas the following contracts don’t emit one of the above events and hence doesn’t comply to ERC4906:

• DelayedRevealBatchMetadataERC721
• BatchMetadataERC721

Additionally, 0x49064906 should be added to config.supportedInterfaces for all the above contracts as mentioned in the standard:

The supportsInterface method MUST return true when called with 0x49064906.

Remediations to Consider

Emit MetadataUpdate or BatchMetadataUpdate in DelayedRevealBatchMetadataERC721.uploadMetadata and BatchMetadataERC721.uploadMetadata functions. Additionally, consider adding 0x49064906 to config.supportedInterfaces.

As defined in ERC721 standard, calling tokenURI with an invalid id should revert:

/// @notice A distinct Uniform Resource Identifier (URI) for a given asset.
/// @dev Throws if `_tokenId` is not a valid NFT. URIs are defined in RFC
///  3986. The URI may point to a JSON file that conforms to the "ERC721
///  Metadata JSON Schema".
function tokenURI(uint256 _tokenId) external view returns (string);

ERC721Core.tokenURI correctly reverts when used with the following extensions:

• BatchMetadataERC721: onTokenURI reverts with BatchMetadataNoMetadataForTokenId for invalid ids.
• DelayedRevealBatchMetadataERC721: onTokenURI reverts with BatchMetadataNoMetadataForTokenId for invalid ids.

However, ERC721Core.tokenURI doesn’t revert when used with the following extensions:

• SimpleMetadataERC721: onTokenURI returns an empty string for not existing ids.
• OpenEditionMetadataERC721: onTokenURI returns a valid metadata JSON object.

Remediations to Consider

Consider reverting onTokenURI for SimpleMetadataERC721 and OpenEditionMetadataERC721 when provided id does not exist.

In ClaimERC20/721/1155 contracts, _validateClaimCondition makes updates to claimCondition but doesn’t return the changed conditions:

function _validateClaimCondition(address _recipient, uint256 _amount, bytes32[] memory _allowlistProof)
    internal
    returns (ClaimCondition memory condition)
{
    condition = _claimableStorage().claimCondition;
    
    ...
    
    _claimableStorage().claimCondition.availableSupply -= _amount;
}

Reference: ClaimableERC20.sol#L225

Notice here that _claimableStorage().claimCondition.availableSupply will be reduced at the end of the function, but the return parameter condition is assigned at the beginning. As a result, this function will return outdated condition, more specifically, outdated condition.availableSupply. In the current system, it will not affect anything but could potentially pose security risks in future updates.

Remediations to Consider

Make the assignment to the return parameter condition at the end of the function.

MintableERC20.sol#L234

In MintableERC20._distributeMintPrice() function, it doesn’t check if msg.value is zero when the currency is not the native token:

    if (_currency == NATIVE_TOKEN_ADDRESS) {
        if (msg.value != _price) {
            revert MintableIncorrectNativeTokenSent();
        }
        SafeTransferLib.safeTransferETH(saleConfig.primarySaleRecipient, _price);
    } else {
        SafeTransferLib.safeTransferFrom(_currency, _owner, saleConfig.primarySaleRecipient, _price);
    }
}

Reference: MintableERC20.sol#L233-L235

Consequently, the caller can potentially loose their native tokens when accidentally passed to the mint call.

Remediations to Consider

Consider adding a check msg.value == 0 when the currency is not the native token in the MintableERC20._distributeMintPrice() function.

In ERC20/721/1155Core, functions that contain a _before hook may be vulnerable to an reentrancy attack. Take ERC20Core.mint for example:

function mint(address to, uint256 amount, bytes calldata data) external payable {
_beforeMint(to, amount, data);
_mint(to, amount);
}

Due to the nature of the _before hook, _beforeMint is called before the _mint function, potentially making an external call before the state in _mint is updated. This violates the CEI (Checks-Effects-Interact) pattern.

Remediation to Consider

Add a reentrance guard to the callback functions to avoid reentering into the core contract.

In ERC721Core contracts, there’s only the mint() function that can mint NFT for users. However, the mint() function doesn’t check if the receiver is a contract and whether it can receive an NFT. This can lead to tokens being locked in the receiver contract.

In ERC1155Core contracts, there’s only the mint() function that can mint a ERC1155 token to users. However, the mint() function only supports minting a single tokenId. For the sake of usability and gas costs, a batchMint() function could be provided to mint multiple tokenIds.

Remediations to Consider

Consider adding safeMint() to ERC721Core and ERC721CoreInitializable contracts, and adding batchMint() to ERC1155Core and ERC1155CoreInitializable contracts.

Currently, an extension can only set a single requiredInterfaceId

function getExtensionConfig() external pure override returns (ExtensionConfig memory config) {
    ...
    
    config.requiredInterfaceId = 0xd9b67a26; // ERC1155
}

Reference: ClaimableERC1155.sol#L166

It could be useful for an extension to specify multiple interface ids, as e.g. the extension needs the core to support both ERC721 and ERC2981 for compatibility. However, currently only one requiredInterfaceId can be specified.

Remediations to Consider

To add more flexibility to the framework, consider adding support for specifying multiple requiredInterfaceIds.

Core contracts such as ERC20CoreInitializable, ERC721CoreInitializable, ERC1155CoreInitializable are meant to be deployed using the Clone pattern. This means that the user deploys a Clone proxy that points to the core contract’s implementation.

However, currently these core contracts can be deployed without being initialized and are also not protected against subsequent initialization.

This allows a malicious actor to take ownership of the core contract implementation by properly initializing the contract. As the owner, they can install a new extension containing selfdestruct logic.

It is important to note, since the Ethereum Dencun upgrade, calling selfdestruct doesn’t destroy the contract’s bytecode anymore as long as it is not triggered in the same transaction as contract creation, only funds held in the contract can be sent to the chosen target. Hence, with the new changes implemented in the Dencun upgrade, a malicious attacker cannot pose any severe harm on user deployments.

MintableERC721 contract is basically a combination of ClaimableERC721 contract and BatchMetadataERC721 contract. However, MintableERC721 contract doesn’t contain BatchMetadataERC721’s getAllMetadataBatches function

Remediations to Consider

Consider adding getAllMetadataBatches() function to MintableERC721 contract. This would also require to add it as an allowed callback function in MintableERC721.getExtensionConfig()

Applying the payable tags in core contracts is not consistent to applying payable tags in the callback functions:

• ERC721Core:
• transferFrom is payable but beforeTransferERC721 callback is not
• approve is payable but beforeApproveERC721 callback is not
• burn is not payable but beforeBurnERC721 is
• ERC20Core:
• burn is not payable but beforeBurnERC20 callback is
• ERC1155Core:
• burn is not payable but beforeBurnERC1155 callback is

Remediation to Consider

Apply payable only to function with a specific use case to receive native tokens.

1. In ModularCore._installExtension L178, this can be removed to avoid an external call. supportInterface() would need to be made public.

   - if (!this.supportsInterface(config.requiredInterfaceId)) {
   + if (!supportsInterface(config.requiredInterfaceId)) {
   

2. In ERC20Core._beforeTransfer L228, data param can be added to the callback for more flexibility.

3. In OpenEditionMetadataERC721.setSharedMetadata L95-100, improve readability and gas consumption by making the following change:

   -        OpenEditionMetadataStorage.data().sharedMetadata = SharedMetadata({
   -            name: _metadata.name,
   -            description: _metadata.description,
   -            imageURI: _metadata.imageURI,
   -            animationURI: _metadata.animationURI
   -        });
                  
   +        OpenEditionMetadataStorage.data().sharedMetadata = _metadata;
   

4. In RoyaltyERC721.sol#L65 and RoyaltyERC1155.sol#L10 the following line can be removed to save gas:

       function getExtensionConfig() external pure virtual override returns (ExtensionConfig memory config) {
   -       config.callbackFunctions = new CallbackFunction[](0);
           ...
       }
   

5. In ClaimableERC20._validateClaimRequest L299, it will revert with an underflow if _amount is > availableSupply. Consider reverting with a custom error when this is the case.

6. In ClaimableERC20._validateClaimRequest L272, there is the following time range check:

   if (block.timestamp < _req.startTimestamp || _req.endTimestamp <= block.timestamp) {
       revert ClaimableRequestExpired();
   }
   

   However, the error ClaimableRequestExpired() is misleading for the case of block.timestamp < _req.startTimestamp, as in this case the request is not expired.