thirdweb A-15

EnglishAuctionsLogic’s bidInAuction function is marked as payable to accept native tokens for bids. However, for auctions using ERC20 as currency, when a user unintentionally sends msg.value > 0 when calling bidInAuction, the transaction will succeed and the native tokens will be locked in the contract.

The tokens will be locked in the contract, unless a permissioned user adds an extension to enable withdrawing these tokens. In cases where extension permissions have been revoked, there would be no way to withdraw these tokens.

Note that the issue also applies to DirectListingsLogic’s buyFromListing function.

Remediations to Consider

Consider adding a check to verify that msg.value == 0 when currency is set to ERC20.

MarketplaceV3 supports sending native tokens directly to the contract via the receive() function. It is understood that this is needed in order to support WETH for listings.

Since there is no restriction on who can call receive(), native tokens can get locked when users accidentally send them to the contract.

Remediations to Consider

Consider adding a restriction to the receive() function so that it is callable only from the nativeTokenWrapper address. An example of restricting the receive() function to the WETH address can be found in the UniswapRouter contract.

PrimarySale and PlatformFee recipients can be set to 0x0 during initialization via BurntToClaimDropERC721.initialize or via setPrimarySaleRecipient and setPlatformFeeInfo respectively.

If one of those recipients is set to a 0x0 and corresponding fee > 0 this can result in the following behavior:

• For claims using ERC20 as currency, claiming a token will revert in _collectPriceOnClaim with “ERC20: transfer to the zero address”
• For claims using native token as currency, tokens will be lost as they are transferred to the 0x0 address.

Note that this issue applies to all contracts supporting PrimarySale and PlatformFee.

Remediations to Consider

Consider adding checks to prevent setting address(0) for both PrimarySale and PlatformFee recipients.

In MarketplaceV3 the ability to add or update extensions to the contract can be done by an account with the EXTENSION_ROLE. This role can only be granted and revoked by any account with the DEFAULT_ADMIN_ROLE, since there is no role admin set for the EXTENSION_ROLE. In the case where a project using these contracts wants to turn off the ability to add/update extensions, they would have to revoke all users with the EXTENSION_ROLE as well as users with the DEFAULT_ADMIN_ROLE, since they can grant the EXTENSION_ROLE to another user at a later time.

Revoking all accounts with the DEFAULT_ADMIN_ROLE may be undesirable as it also manages other roles like the LISTER_ROLE and ASSET_ROLE.

Remediations to Consider

Set the EXTENSION_ROLE as its own role admin in the initializer and set an initial account with the EXTENSION_ROLE, this will allow it so the contract can no longer be upgraded when there are no accounts with the EXTENSION_ROLE.

The intention of BurnToClaimDrop721Logic’s setMaxTotalSupply function is to set the maximum number of tokens to be minted rather than - as the name implies - setting the maximum number of token supply.

Remediations to Consider

Consider renaming the function setMaxTotalSupply and the state var maxTotalSupply to something more appropriate such as setMaxTotalMinted and maxTotalMinted.

In BurnToClaim.sol, an admin can set BurnToClaimInfo via setBurnToClaimInfo(..). However, both originContractAddress and currency parameter can be set to 0x0 as there is no check preventing this. As a result, subsequent calls to burnAndClaim would revert inside the verifyBurnToClaim function.

Remediations to Consider

It is recommended to prevent setting invalid config in the first place, thus consider adding != address(0) checks to the setBurnToClaimInfo function and remove the check in the verifyBurnToClaim function.

In BurnToClaimDropERC721 and BurnToClaimDrop721Logic, the OPERATOR_ROLE is defined but not used anywhere in the code.

Remediations to Consider

Consider removing OPERATOR_ROLE definitions and corresponding _setupRole logic from above contracts.

BurnToClaim.sol misses proper natspec documentation for most of the functions. In some of the other contracts such as BurnToClaimDropERC721.sol, BurnToClaimDrop721Logic.sol, and BurnToClaimDrop721Storage.sol, most of the functions take use of the @dev tag, but they tend to not include @param and @return tags.

Remediations to Consider

Add missing natspec documentation. Follow natspec guidelines to provide proper documentation of the code.

In BurnToClaimDrop721Logic, there are two ocurrences where named returns are defined but not assigned:

1. The function lazyMint defines a named return variable batchId, but the value is returned directly instead of assigning it to the variable.
2. The function _msgSender defines a named return variable sender, but the value is returned directly instead of assigning it to the variable.

Remediations to Consider

Consider either assigning the return value to the defined return variable or remove the variable at all.

In BurnToClaimDrop721Logic, the function _checkTokenSupply retrieves the state var via data storage but could use maxTotalSupply() instead to improve code readability.

Remediations to Consider

Consider using maxTotalSupply() to reduce code size and improve readability.

In BurnToClaimDropERC721, the function isAuthorizedCallToUpgrade is an internal function but not prefixed with an underscore.

According to Solidity naming conventions - and as applied everywhere else in the code - private and internal functions should be prefixed with an underscore.

Remediations to Consider

Consider renaming the function from isAuthorizedCallToUpgrade to _isAuthorizedCallToUpgrade.

In MarketplaceV3.sol the constant VERSION is set to 1, but version 2 was already used in a previous version of the Marketplace contracts.

Remediations to Consider

Consider increasing the VERSION to 3.

BurnToClaimDrop721Logic’s tokenURI function returns a valid URI once the tokenId is lazy minted. However, the tokens are technically not minted until they are claimed. Also for burned tokens the function will return a valid URI.

Although above behavior is as designed, it doesn’t fully comply to the ERC721 standard, which says that tokenURI should throw an exception for invalid tokenIds.

Remediations to Consider

Consider adding documentation to make users aware that above behavior slightly deviates from ERC721 standard.

In MarketplaceV3, the functions onERC721Received, onERC1155Received, and onERC1155BatchReceived are implemented to indicate support of retrieving ERC721 and ERC1155 tokens.

Remediations to Consider

Consider deriving from OpenZeppelin’s ERC721Holder and ERC1155Holder instead to improve readability.

In MarketplaceV3, _msgSender and _msgData are overridden from ERC2771ContextUpgradable and Permission and the ident function logic as from ERC2771ContextUpgradable is reimplemented again.

Remediations to Consider

Consider re-using the logic from ERC2771ContextUpgradable by calling ERC2771ContextUpgradeable._msgSender() and ERC2771ContextUpgradeable._msgData() respectively to improve readability and reduce code size.

Thirdweb uses the “namespaced storage” (aka “storage struct”) pattern for all upgradable contracts. However, those upgradable contracts are currently not adhering to the ERC7201 standard, which standardizes the storage location used for the “namespace”.

According to ERC7201, this is important because:

These storage usage patterns are invisible to the Solidity and Vyper compilers because they are not represented as Solidity state variables. Smart contract tools like static analyzers or blockchain explorers often need to know the storage location of contract data. Standardizing the location for storage layouts will allow these tools to correctly interpret contracts where these design patterns are used.

A great example of adhering to the ERC7201 standard can be seen in the recently released OpenZeppelin’s v5 pre-release contracts, e.g. see Initializable.sol.

Remediations to Consider

Consider changing the upgradable contracts to adhere to the ERC7201 standard.

In IMarketplace.sol, the struct members of Listing, Auction, and Offer can be reordered to save storage slots.

In particular, when the struct members TokenType (1 Byte), Status (1 Byte), and one of the address members (20 Bytes) are placed next to each other, they only take 1 storage slot.

Consider reordering the Listing struct from:

struct Listing {
    uint256 listingId;
    address listingCreator;
    address assetContract;
    uint256 tokenId;
    uint256 quantity;
    address currency;
    uint256 pricePerToken;
    uint128 startTimestamp;
    uint128 endTimestamp;
    bool reserved;
    TokenType tokenType;
    Status status;
}

to:

struct Listing {
    uint256 listingId;
    uint256 tokenId;
    uint256 quantity;
    uint256 pricePerToken;
    uint128 startTimestamp;
    uint128 endTimestamp;
    address listingCreator;
    address assetContract;
    address currency;
    TokenType tokenType;
    Status status;
    bool reserved;
}

The same rules apply to the Auction and Offer struct.