Reach out for an audit or to learn more about Macro
or Message on Telegram

thirdweb A-19

Security Audit

June 27th, 2024

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for thirdweb's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from May 23, 2024 to June 5, 2024.

The purpose of this audit is to review the source code of certain thirdweb Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
Medium 7 - 1 6
Low 10 - - 10
Code Quality 6 - - 6

thirdweb was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Source Code

The following source code was reviewed during the audit:

We audited the following contracts:

Contract SHA256
src/ModularCore.sol

ae19e8e8d34f66705444640a7f44df186e4ebc4a0dc0073ed8311595f8aa48bf

src/ModularExtension.sol

80d5a51c7be7a8e8350ba576606676fd0c6dec2c38671bea09dede195527afbe

src/core/token/ERC1155Core.sol

fa701578242e6981ea365ee6a21ed7af8b2be5a90fc568a263e397d68b033401

src/core/token/ERC1155CoreInitializable.sol

ef2297cca5d8dd17cea50f906545d145df687814b2172a3f93a60a50afb84702

src/core/token/ERC20Core.sol

691c47b18184241cd8dc4d314b65f4c956e555e8032ccbbf933deb83d4830e54

src/core/token/ERC20CoreInitializable.sol

9f7181263dc594336382e162bc59176485c00a0f87d577bd507f7d134edb0ece

src/core/token/ERC721Core.sol

c8c1085407680d200874fc97733d97f5cc6f11eda9fc0de266e0fb4469d14889

src/core/token/ERC721CoreInitializable.sol

b333f9fa01d6a26a538a510b2bf13be48e2fdcb676d4af06f7835f09e0150382

src/extension/token/metadata/BatchMetadataERC1155.sol

38cde7db577c5de04c585e4636303d35a99944534ee52106373fcdfbb7fee3de

src/extension/token/metadata/BatchMetadataERC721.sol

2efa4fb0e5532f9f9aa1cf749fb02b567a4cd7b6fef6012f9ae73aa5c0aef9e8

src/extension/token/metadata/DelayedRevealBatchMetadataERC721.sol

e2533589fcd64db50b36b90a8bd69aecf6788d7362cc271808f32a2791afa9c9

src/extension/token/metadata/OpenEditionMetadataERC1155.sol

6d4f43a542d87116aa46743a64bf2d888810ed30340d8c7b9c487deb074afd8d

src/extension/token/metadata/OpenEditionMetadataERC721.sol

215ac25d9202d502e69f0c01161fb5b5e267838068150412fa6e368df1d5e153

src/extension/token/metadata/SimpleMetadataERC1155.sol

b6a07a2aeb0d118bd0332142c773f72cec20b7fbcaf9b405183745b7627f731c

src/extension/token/metadata/SimpleMetadataERC721.sol

d745e7c4a2de0f4e2d33465ece610a40dcbcd4fa969eec3dc09a5a08096e63ea

src/extension/token/minting/ClaimableERC1155.sol

2dd5152b98f99c021173fd61e69e4af969fd9d700c4a3d35f996d88f583186b3

src/extension/token/minting/ClaimableERC20.sol

60c5979661a220233af66fe27ca66c1f24f8c5998fcb09746865489014af54db

src/extension/token/minting/ClaimableERC721.sol

a0a6daa24b6bcf2632f41d163e279e9d1ae5a54193a44d6817d3cc2a2ea72d59

src/extension/token/minting/MintableERC1155.sol

a9d8f6b43a3e8a8cfff02a4628b928afb6635eecbc158955e466166b862d1d66

src/extension/token/minting/MintableERC20.sol

f5f74c274ae587b35813b9b0cd166c1547b291749f16b223b1197877fd91fd58

src/extension/token/minting/MintableERC721.sol

74cc6f1e90d5dfc2b922e9843ff69363b138d3652330020021717900e9ece1da

src/extension/token/royalty/RoyaltyERC1155.sol

8437a25e1b361f3293c6290a2444bd9109d07a9860249b9481ca90097ea8d897

src/extension/token/royalty/RoyaltyERC721.sol

9c7f86f31a135bbd52f28b9f519f34ce8f24274261cebf2e86a846a145f86c7e

src/extension/token/transferable/TransferableERC1155.sol

69a8bbb9ab8e7939c26f52923b4677e84b9fc95ad546c839780e8ba1bddef794

src/extension/token/transferable/TransferableERC20.sol

95c2103f823e32f64071833594eb361d60f92cb48c016259eaee44aff1bdd507

src/extension/token/transferable/TransferableERC721.sol

d7f10ba2e6fe3fe742ed9a3fc6a66090ce42d44089cf3dbd15c4581f705acf43

src/callback/BeforeApproveCallbackERC20.sol

7f49901663e38b816fe853c9eba5c5ad871f0a426bdd7b959a3ba5501d4e282c

src/callback/BeforeApproveCallbackERC721.sol

945e8a2b73e296964d0a5f921ec43d0e91f2419b8902d38244297537d6d8ec40

src/callback/BeforeApproveForAllCallback.sol

d53d04e8023f85a76458ef05d46566a1844d0d4656f600054f37dd0419b5a034

src/callback/BeforeBatchTransferCallbackERC1155.sol

e5055e7d0584b00a7e13972174843b53d6b204a977a34038387a703cd7c3a574

src/callback/BeforeBurnCallbackERC1155.sol

e3cb2ca4a22b53fdde9877e7dc6ec54e8115ae0c9f0154a37c39a1b1851bed84

src/callback/BeforeBurnCallbackERC20.sol

558fef840b3e9bd7b098b7a28c61989f1e7f182e47d7cdda4cf44f5e4d05ab79

src/callback/BeforeBurnCallbackERC721.sol

66272c6809acc3f0b50f29f5e755d2e3a8ace5e9bb85f1534798a60afd5070a4

src/callback/BeforeMintCallbackERC1155.sol

7e41e692a6ef0e9bd1f948b0873f354073e94ae42562b3e4f00af1b06fdebcbc

src/callback/BeforeMintCallbackERC20.sol

7677068f4e8f69a489d2201496afbf63e117ed000ad3408b324582409c8952db

src/callback/BeforeMintCallbackERC721.sol

3530eca7d9ebfc56c73a5a9476470cb50369f629d25671cf42c4cc11436e0165

src/callback/BeforeTransferCallbackERC1155.sol

0c67c5b2a2ed2f14d07e37ad6fb50d300b575ba9cb475ae815ce8a82c39d11c4

src/callback/BeforeTransferCallbackERC20.sol

830a7f789d884f3b6e6d4988ffabfe06c67dd81f2239b16733b5c4bc7fcd9db2

src/callback/BeforeTransferCallbackERC721.sol

14450e05178b5bdd2f5132e3ee086f71cab957b75dfb710e2b9a248022e4a8d7

src/callback/OnTokenURICallback.sol

b1e82563f29831b90072c046f1a9542189b729cd91b227cc60efea327b25c788

Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

M-1

Encryption key can be potentially intercepted to retrieve revealed URI

Topic
Incentive Design
Status
Impact
High
Likelihood
Low

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.

M-2

Callback marked as OPTIONAL does revert when not implemented in extension

Topic
Protocol Design
Status
Impact
Medium
Likelihood
High

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.

M-3

ERC1155Core's mint() doesn’t follow the CEI pattern can lead to potential reentrancy

Topic
Reentrancy
Status
Impact
Medium
Likelihood
Medium

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

M-4

Extension may fail to get uninstalled

Topic
DoS
Status
Impact
High
Likelihood
Low

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.

M-5

Update to ClaimCondition can cause the caller paying more than expected

Topic
Frontrunning
Status
Impact
High
Likelihood
Low

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.

M-6

Unsafe storage layout can lead to storage collisions on upgrades

Topic
Upgradability
Status
Wont Do
Impact
High
Likelihood
Medium

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.

M-7

supportsInterface() issues

Topic
Interoperability
Impact
Medium
Likelihood
Medium

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:

    → 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:

    → 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.

L-1

name and symbol properties are not correctly initiated

Topic
Interoperability
Impact
Low
Likelihood
Medium

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.

L-2

User must approve themselves to burn their own token

Topic
Best Practice
Impact
Low
Likelihood
Medium

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

L-3

Not including EIP712’s eip712Domain() function as a fallback function, lead to the function unreachable

Topic
Protocol Design
Status
Impact
Low
Likelihood
Low

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.

L-4

Wrong requirement check in initialize() function

Topic
Validation
Status
Impact
Low
Likelihood
Low

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);
L-5

Allowing core contracts to receive native tokens without any efficient way to take it out

Topic
Locked funds
Status
Impact
Low
Likelihood
Low

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.

L-6

Not following ERC4906 in BatchMetadata and DelayedRevealBatchMetadata contracts

Topic
Interoperability
Impact
Low
Likelihood
Low

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.

L-7

tokenURI should revert for invalid tokenIds

Topic
Interoperability
Impact
Low
Likelihood
Low

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.

L-8

_validateClaimCondition() returns an outdated condition

Topic
Best Practice
Status
Impact
High
Likelihood
Low

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.

L-9

Native tokens can be locked in MintableERC20

Topic
Locked funds
Status
Impact
Low
Likelihood
Low

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.

L-10

Core functions may be vulnerable to reentrancy when callbacks make an external call

Topic
Reentrancy
Status
Impact
Medium
Likelihood
Low

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.

Q-1

batchMint for ERC1155Core contract and safeMint for ERC721Core contract are currently not supported

Topic
Protocol Design
Status
Quality Impact
Medium

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.

Q-2

Support for multiple requiredInterfaceIds

Topic
Protocol Design
Status
Quality Impact
High

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.

Q-3

Core contract implementations are not protected against initialization

Topic
Best Practice
Status
Quality Impact
Medium

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.

Q-4

MintableERC721 should have getAllMetadataBatches function

Topic
Consistency
Status
Quality Impact
Medium

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()

Q-5

Inconsistent payable tags

Topic
Consistency
Status
Quality Impact
Medium

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.

Q-6

Nitpicks

Topic
Nitpicks
Status
Quality Impact
Medium
  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.

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the thirdweb team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.