0x0 - `UPDATE_CONTRACT_ROLE` Unable To Revoke NFT Port Permissions (721)

[sherlock-admin]

0x0

high

UPDATE_CONTRACT_ROLE Unable To Revoke NFT Port Permissions (721)

Summary

It's not possible to revoke NFT Port Permissions with an account holding solely UPDATE_CONTRACT_ROLE.

Vulnerability Detail

ERC721NFTProduct.update

The docstring for ERC721NFTProduct.update states that accounts with either roles UPDATE_CONTRACT_ROLE or ADMIN_ROLE should be able to update configuration items. The implementation does not reflect this as GranularRoles.revokeNFTPortPermissions requires msg.sender to additionally have ADMIN_ROLE.

Impact

• Accounts holding UPDATE_CONTRACT_ROLE are unable to revoke NFT Port Permissions

Code Snippet

function update(
    Config.Runtime calldata newConfig,
    RolesAddresses[] memory rolesAddresses,
    bool isRevokeNFTPortPermissions
) public onlyRole(UPDATE_CONTRACT_ROLE) {
    // If metadata is frozen, baseURI cannot be updated
    require(
        metadataUpdatable ||
            (keccak256(abi.encodePacked(newConfig.baseURI)) ==
                keccak256(abi.encodePacked(baseURI))),
        "Update: Metadata is frozen"
    );

    baseURI = newConfig.baseURI;
    royaltiesAddress = newConfig.royaltiesAddress;
    royaltiesBasisPoints = newConfig.royaltiesBps;

    if (!newConfig.tokensTransferable) {
        tokensTransferable = false;
    }
    if (!newConfig.metadataUpdatable && metadataUpdatable) {
        metadataUpdatable = false;
        emit PermanentURIGlobal();
    }

    _updateRoles(rolesAddresses);

    if (isRevokeNFTPortPermissions) {
        revokeNFTPortPermissions();
    }
}

function revokeNFTPortPermissions() public onlyRole(ADMIN_ROLE) {
    _revokeRole(ADMIN_ROLE, _nftPort);
    _nftPort = address(0);
}

Tool used

Manual Review

Recommendation

• Ensure that modifiers that validate if an account is able to access a function has the correct role
• Enforce a clear separation between roles UPDATE_CONTRACT_ROLE and ADMIN_ROLE

Duplicate of #40