thirdweb A-10

TieredDropLogic inherits from DefaultOperatorFiltererUpgradeable and overrides NFT transfers to ensure the operator is not blacklisted by the filter registry, provided the operator restriction is set.

However, __OperatorFilterer_init is required to be called to register/subscribe the contract to a filter registry, and it is never called. This causes onlyAllowedOperator and onlyAllowedOperatorApproval modifiers to always return true, no matter the operator. It may also prevent OpenSea from honouring royalties accrued from any collection that implements this, as there is no registration/subscription to their registry.

Remediations to Consider

Call __OperatorFilterer_init in TieredDrop's initializer function, registering the contract to the filter registry.

TieredDropLogic inherits from DelayedReveal, LazyMintWithTier, and BatchMintMetadata which don’t follow the storage struct pattern, as it is expected for contracts using the extension pattern. This causes the potential for storage collisions to occur, since the contracts use the proxy pattern and use delegate calls to read and write to storage. Storage collisions can allow data to be set and/or read from that is not intended, causing many potential issues and vulnerabilities.

Remediations to Consider

Ensure all contracts storage follow the storage struct pattern, to prevent storage collisions.

In TieredDrop.sol’s initializer function, it initializes permissions using the PermissionsInit’s _setupRole(), which allows TieredDrop to set the permission without inheriting from the entire permissions contract.

However, the extension TieredDropLogic inherits from PermissionsEnumerable, with is compatible with permissions, but stores extra data on permission update to allow retrieval of all addresses with a given permission. Since TieredDrop initializes using Permission’s _setupRole(), rather than PermissionsEnumerable's _setupRole(), permissions initialized cannot be retrieved as it is expected to, and the count received by getRoleMemberCount() can be off.

Remediations to Consider

Create a PermissionsEnumerableInit contract that uses enumerable permissions to initialize TieredDrop, allowing accurate information regarding permissions.

TieredDrop.sol::_canSetExtension() uses msg.sender instead of _msgSender():

function _canSetExtension() internal view virtual override returns (bool) {
        bytes32 defaultAdminRole = 0x00; 
        return IPermissions(address(this)).hasRole(defaultAdminRole, msg.sender);          // @audit use of _msgSender
}

As a consequence, all calls relying on _canSetExtension() for authorization don’t work with meta transactions. In particular, the following calls are affected:

• BaseRouter.sol::addExtension
• BaseRouter.sol::updateExtension
• BaseRouter.sol::removeExtension

Remediations to Consider

Consider changing TieredDrop.sol to inherit from ERC2771ContextUpgradeable and use _msgSender() instead of msg.sender in _canSetExtension().

In OperatorFiltererUpgradeable.sol’s onlyAllowedOperator and onlyAllowedOperatorApproval modifiers, a check is made that will revert with OperatorNotAllowed if OPERATOR_FILTER_REGISTRY.isOperatorAllowed() returns false:

if (!OPERATOR_FILTER_REGISTRY.isOperatorAllowed(address(this), msg.sender)) {
    revert OperatorNotAllowed(msg.sender);
}

However, the operator filter registry’s isOperatorAllowed function never returns false, it either returns true, or reverts with its own custom error AddressFiltered or CodeHashFiltered:

function isOperatorAllowed(address registrant, address operator) external view returns (bool) {

    address registration = _registrations[registrant];

    if (registration != address(0)) {

        EnumerableSet.AddressSet storage filteredOperatorsRef;

        EnumerableSet.Bytes32Set storage filteredCodeHashesRef;
        
        filteredOperatorsRef = _filteredOperators[registration];
        
        filteredCodeHashesRef = _filteredCodeHashes[registration];
        
        if (filteredOperatorsRef.contains(operator)) {
        
            revert AddressFiltered(operator);
        
        }

        if (operator.code.length > 0) {
        
            bytes32 codeHash = operator.codehash;
            
            if (filteredCodeHashesRef.contains(codeHash)) {
            
                revert CodeHashFiltered(operator, codeHash);
            
            }
        
        }

    }

    return true;
}

Remediations to Consider

Remove the check if isOperatorAllowed returns false for both onlyAllowedOperator and onlyAllowedOperatorApproval, and allow it to revert with the relevant error, removing the need for OperatorNotAllowed.

In TieredDrop.sol OwnableInit is used to setup an owner of the contract in initialize() and TieredDropLogic.sol inherits Ownable and sets up who can change owner.

However, the owner and onlyOwner are never used, and is initialized for no apparent reason.

Remediations to Consider

Remove Ownable from TieredDropLogic.sol, and do not initialize OwnableInit in TieredDrop.sol, in order to save gas on deployments, and remove unnecessary functions.

For solidity versions 0.8.4+ it is advised to revert with custom errors over using require statements. Using them saves bytecode on deployment, a little gas on execution, and allows for more detailed error messages with the ability to pass in parameters.

Remediations to Consider

Replace require statements with custom errors to give more context about errors and save a bit of gas.

In TieredDropLogic.sol, there are multiple external calls to check if an address has a given role. Example:

return Permissions(address(this)).hasRole(DEFAULT_ADMIN_ROLE, _msgSender());

However, external calls cost additional gas, and in this case an external call can be avoided by directly retrieving the data using PermissionsStorage:

function _hasRole(bytes32 role, address addr) internal view returns(bool) {
    PermissionsStorage.Data storage data = PermissionsStorage.permissionsStorage();      
return data._hasRole[role][addr];
}

Remediations to Consider

Replace external calls to Permissions to query permissions by using PermissionsStorage in order to save gas.

TieredDrop.sol inherits from BaseRouter.sol, and overrides _canSetExtension() to allow the defaultAdminRole, set in its initializer as the _defaultAdmin address, to add any extension to the contract. However, an extension has complete control over writing to storage, since the calls are done via delegateCall. This allows a malicious admin to add an extension that can do anything to the contract, including steal all NFTs, or steal all tokens that the contract has been approved for.

Remediations to Consider

• Setup an extension registry, a contract where all valid extension are stored, and have it so only the team can set these extensions. Then when adding an extension, it is retrieved from the registry. This ensures extensions are still opt in, but are limited to vetted and safe extensions in order to prevent malicious functions being set.
• Create a version of TieredDrop.sol that inherits from RouterImmutable.sol, giving users the option to prevent additional extensions from being added after deployment.