thirdweb 20

In PayGatewayModule’s initiateTokenPurchase, native tokens are directly forwarded to the forwardAddress when directTransfer is set.

if (req.directTransfer) {
    if (_isTokenNative(req.tokenAddress)) {
        req.forwardAddress.call{ value: sendValue }("");

Reference: PayGatewayModule.sol#L225

However, the return value is not checked on the .call function, which can lead to unwanted behavior in case the call function returns false.

Remediations to Consider

Consider checking the return value of the .call function accordingly or use SafeTransferLib.safeTransferETH

In RoyaltyERC721, RoyaltyERC1155, and CreatorTokenERC20, the MANAGER_ROLE can set the validator address via the setTransferValidator function. However, the MANAGER_ROLE is not permissioned to set transfer policies, this is because of the validator’s implementation, where only the owner, admin, or contract itself is allowed to set policies, see _requireCallerIsNFTOrContractOwnerOrAdmin here. As a result, while the MANAGER_ROLE can set the validator’s address, only the Core’s owner can configure transfer policies on this validator, such as whitelisting or blacklisting accounts.

In particular, this can lead to problems where ownership wants to be renounced from the Core contract in order to remove upgradeability, but the possibility to configure transfer policies want to be maintained.

Remediations to Consider

hasRole function could be implemented in the modules so that it returns true for accounts having the MANAGER_ROLE assigned.

In PayGatewayModule, the external isProcessed function is used to check if a given transactionId has been used. Given the modular contract pattern, a function needs to be set in the fallbackFunctions array in order to be callable. This is not the case with the isProcessed function and thus it can’t be called.

Remediations to Consider

Add isProcessed function to the fallbackFunctions array.

In PayGatewayModule’s initiateTokenPurchase, when directTransfer = true and _istTokenERC20 = true, tokens are transferred to the contract and approved to the forwardAddress. However, if native tokens are sent with the call (msg.value > 0), they won’t be forwarded and remain in the contract.

Remediations to Consider

When directTransfer = true and _istTokenERC20 = true, enforce that no native tokens are sent with the call (msg.value == 0).

In PayGatewayModule’s initiateTokenPurchase, an error is thrown when sendValue is smaller than req.tokenAmount:

if (sendValue < req.tokenAmount) {
    revert PayGatewayMismatchedValue(sendValue, req.tokenAmount);
}

Reference: PayGatewayModule.sol#L219

The declaration of the error is:

error PayGatewayMismatchedValue(uint256 expected, uint256 actual);

As per above definition, the first parameter provided should be the expected value (= req.tokenAmount) and the second parameter provided should be the actual value (= sendValue). However, the error is thrown with the values in reverse order.

Remediations to Consider

Reverse the parameters to throw the error according to the defintion.

In CreatorTokenERC20, the function getTransferValidationFunction() is implemented to return the function selector for the transfer validation function:

function getTransferValidationFunction() external pure returns (bytes4 functionSignature, bool isViewFunction) {
    functionSignature = bytes4(keccak256("validateTransfer(address,address,address,uint256, uint256)"));
}

Reference: CreatorTokenERC20.sol#L94

However, the string being used to calculate the signature contains a space between the last two parameters, causing to return an incorrect function selector.

Remediations to Consider

Remove the space between the last two parameters.

The PaymentsGateway has been recently renamed to PayGateway including its references. However, in PayGatewayModule.sol, the storage location is still referring to Payments instead of Pay.

library PayGatewayModuleStorage {
    /// @custom:storage-location erc7201:payments.gateway.module
    bytes32 public constant PAYMENTS_GATEWAY_EXTENSION_STORAGE_POSITION =
        keccak256(abi.encode(uint256(keccak256("payments.gateway.module")) - 1)) & ~bytes32(uint256(0xff));

Reference: PayGatewayModule.sol#L14-L16

Additionally, the error definition PaymentsGatewayMsgValueNotZero is still referring to the old name.

Remediations to Consider

For consistency sake, update all above mentioned occurrences to the new name.

In PayGatewayModule’s initiateTokenPurchase function, there is the following check for msg.value:

if (_isTokenNative(req.tokenAddress)) {
    if (msg.value < req.tokenAmount) {
        revert PayGatewayMismatchedValue(req.tokenAmount, msg.value);
    }
}

Reference: PayGatewayModule.sol#L202-L204

A few lines further down, there is a similar check:

if (_isTokenNative(req.tokenAddress)) {
    sendValue = msg.value - totalFeeAmount;

    if (sendValue < req.tokenAmount) {
        revert PayGatewayMismatchedValue(sendValue, req.tokenAmount);
    }
}

Reference: PayGatewayModule.sol#L215-L221

As the second check always covers the first one (they are the same when totalFeeAmount is 0), the first check is redundant and can be removed.

Remediations to Consider

To improve readability, remove the first check.

All core token implementation (ERC20Core, ERC721Core, and ERC1155Core) support ERC165 and thus return true for the ERC165 interface id in their supportsInterface function:

function supportsInterface(bytes4 interfaceId) public view returns (bool) {
    return interfaceId == 0x01ffc9a7 // ERC165 Interface ID for ERC165
    ...

As the Core contract by itself already implements the supportsInterface function, this check can be moved to the Core’s supportsInterface function and removed from all the Core’s token implementations.

Remediations to Consider

Move the ERC165 interface ID check from the Core’s token implementations to the Core contract itself.

encodeBytesOnInstall and encodeBytesOnUninstall has been added to all modules implementing the onInstall/onUninstall functions.

Remediations to Consider

Consider adding those two functions to the IInstallationCallback interface to improve integration experience.

RoyaltyERC721 and RoyaltyERC1155 both implement all necessary functions of ICreatorToken interface, but don’t inherit from it. This is in contrast to the CreatorTokenERC20 module, which inherits the ICreatorToken interface.

Additionally, for all three modules, consider adding ICreatorToken to the supportedInterfaces in getModuleConfig().

Remediations to Consider

Consider changing RoyaltyERC721 and RoyaltyERC1155 to inherit from ICreatorToken interface and add it to the supportedInterfaces array.

1. In PayGatewayModule.sol, incorrect comment on L53: the @param tag is still referring to feeBPS.
2. In PayGatewayModule.sol, missing @param tag for newly added directTransfer flag on L81
3. In the readme of the PayGateway, it refers to TransferStart and TransferEnd event, but it should be TokenPurchaseInitiated and TokenPurchaseCompleted events.
4. In ERC20Core.sol, unused import of ReentrancyGuard on L8.
5. When using “named return variables” as in the function definition of getTransferValidator() in RoyaltyERC721, RoyaltyERC1155, and CreatorTokenERC20, values need to be assigned to the return variable but there is no need to return the value itself, e.g. see here.