thirdweb 21

ClaimableERC20 and MintableERC20 modules were recently updated to calculate the mint price by dividing pricePerUnit by 1e18.

function beforeMintWithSignatureERC20(address _to, uint256 _amount, bytes memory _data, address _signer)
    external
    payable
    virtual
    override
    returns (bytes memory)
{
    ...
>>  _distributeMintPrice(msg.sender, _params.currency, (_amount * _params.pricePerUnit) / 1e18);
}

Reference: ClaimableERC20.sol#L216

This approach works correctly if the specified currency uses 18 decimal representation. However, the price calculation becomes inaccurate when dealing with currencies that have a different number of decimals.

Consider a scenario where the specified currency is USDC, which only has 6 decimals. The pricePerUnit will be expressed using the 6-decimal representation, but it's still divided by 1e18. Consequently, the calculated mint price will be significantly lower than intended.

Remediation to Consider

Instead of using a hardcoded 1e18 divisor, consider using the decimals value of the specified currency.

Modules use the “Namespaced Storage Layout” to prevent collisions between different modules as well as between the modules and the core contracts. However, this doesn’t protect against storage collisions between different version upgrades of the same module.

ClaimableERC20/721/1155 were recently updated to include a maxMintPerWallet param in the ClaimCondition struct:

    struct ClaimCondition {
        uint256 availableSupply;       // slot 1
        bytes32 allowlistMerkleRoot;   // slot 2
        uint256 pricePerUnit;          // slot 3
        address currency;              // slot 4
>>      uint256 maxMintPerWallet;      // slot 5
        uint48 startTimestamp;
        uint48 endTimestamp;
        string auxData;
    }

Reference: ClaimableERC20.sol#L75

The ClaimCondition is set in the storage using the following layout:

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;
    // address => how many tokens have been minted
    mapping(address => uint256) totalMinted;
}

Reference: ClaimableERC20.sol#L21-L30

This change is considered an unsafe storage layout change, since an upgrade would overwrite subsequent parameters. This can lead to severe vulnerabilities such as unintentionally setting wrong values for claim conditions or breaking the module’s functionality at all.

Remediation to Consider

Consider changing the storage layout so that upgrades can be made in a safe way. E.g. only add new params at the end of the storage layout or define a new storage layout for each version that reads from a separate storage slot.

ERC721Base and ERC1155Base added support for updateMetadataERC721 and updateMetadataERC1155 callbacks, respectively. This allows for setting the baseURI on the mint and mintWithSignature functions.

The callback and thus the respective BatchMetadata module is called when a baseURI is provided, otherwise the callback is skipped:

function mint(address to, uint256 amount, string calldata baseURI, bytes calldata data) external payable {
    uint256 tokenId = _nextTokenId();
>>  if (bytes(baseURI).length > 0) {
        _updateMetadata(to, tokenId, amount, baseURI);
    }
    _beforeMint(to, tokenId, amount, data);
    _safeMint(to, amount, "");
}

Reference: ERC721Base.sol#L191

This approach allows minting tokens without invoking the BatchMetadata module by leaving the baseURI empty. In such cases, the tokenId in ERC721Base or ERC1155Base increments, while the nextTokenIdRangeStart in the module's storage remains at 0. As a result, when a token is subsequently minted with a provided baseURI, the metadata is incorrectly applied to tokenId = 0 instead of the newly minted tokenId.

For example, imagine 100 NFTs are minted for ERC721Base without a baseURI. Then, 1 NFT is minted with a baseURI of ipfs://base/, intended to apply to tokenId = 100. However, this doesn't happen as expected. Because the module wasn't informed of the 100 previously minted NFTs and the nextTokenIdRangeStart is still 0, it mistakenly applies the provided baseURI to tokenId = 0.

Remediation to Consider

Consider changing the logic in the BatchMetadata modules, so that the metadata is correctly applied to the provided token range.

SequentialTokenIdERC1155 increments the tokenId when a value of uint.max is passed to updateTokenIdERC1155. This works well when the module is installed before minting the first token, but can lead to unwanted behavior if installed at a later point.

Consider a scenario where 10 tokens (range 0-9) have been already minted by ERC1155Core, and only then the SequentialTokenIdERC1155 is installed. The nextTokenId in SequentialTokenIdERC1155 would start from id 0 (instead of 10), which is not the desired behavior. Instead, it should increment the value from the most recent tokenId.

Remediation to Consider

Add a onInstall function to the module to allow setting the nextTokenId accordingly.

In TokenPaymaster’s _postOp function, refillEntryPointDeposit is called to refill the deposit on EntryPoint with the Paymaster’s token balance. If the EntryPoint balance falls below the minEntryPointBalance threshold, tokens are swapped to wei using Uniswap:

if (currentEntryPointBalance < tokenPaymasterConfig.minEntryPointBalance) {
    uint256 swappedWeth = _maybeSwapTokenToWeth(token, _cachedPrice);
    unwrapWeth(swappedWeth);
    entryPoint.depositTo{value: address(this).balance}(address(this));
}

Reference: TokenPaymaster.sol#L178-L182

In _maybeSwapTokenToWeth, the amountOutMin is calculated to provide slippage protection for the Uniswap swap operation.

uint256 amountOutMin = addSlippage(tokenToWei(tokenBalance, quote), uniswapHelperConfig.slippage);

Reference: UniswapHelper.sol#L55

The TokenPaymaster’s implementation has been recently changed to support tokens with decimals < 18. However, this is not considered in the calculation of the amountOutMin value. Consequently, the amountOutMin calculated might be significant lower than what is required for sufficient slippage protection.

Let’s consider a scenario where USDC is used (decimal: 6) for the payment token. In this case, the calculated amountOutMint would be by a magnitude of 10^12 (10^18-6) smaller than what is required, effectively removing slippage protection.

This makes the swap operation vulnerable to a sandwich attack, meaning that the Paymaster potentially receives much less WETH for the amount of tokens provided.

Remediation to Consider

Consider the token’s decimal value when calculating the amountOutMin value.

The TokenPaymaster contract was modified to accommodate tokens with decimals other than 18:

uint256 tokenAmount = weiToToken(preChargeNative, cachedPriceWithMarkup) / (10 ** (18 - _tokenDecimals));

Reference: TokenPaymaster.sol#L131

While this implementation works for tokens with decimals ≤ 18, it fails for tokens with decimals > 18. In such cases, the transaction would revert, rendering the Paymaster contract non-functional.

Remediation to Consider

• Modify the implementation to correctly handle tokens with decimals > 18, or
• add a check in the constructor to revert if a token with decimals > 18 is provided

For ClaimableERC20/721/1155, the FallbackFunction array is defined with a size of 5, but only needs to hold 4 elements. Consider reducing the size of the array to 4.

Natspec is missing or incorrect on the following occurrences:

• missing natspec for _startTokenId param here and here
• missing natspec for _id param here
• missing natspec for deadline, v, r, s params here
• outdated natspec for ClaimSignatureParamsERC20 here
• outdated natspec for ClaimParamsERC20 here
• incorrect natspec for beforeMintWithSignatureERC20 here. It misses natspec for all the params and incorrectly says “for the ERC20Core.mint” instead of “ERC20Core.mintWithSignature”
• incorrect natspec here. It mentions “platform fee recipient + BPS”, but this is not used anymore.
• obsolete natspec for recipient here.