addToList does not check for allowlist or blacklist

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x1872d6ef7ff11023e34a4d3463a6b294e1887278f1feb5c6240af1cf7ef4102f Severity: medium

Description:

Description

The protocol uses a Denyfeature/AllowFeature. These features are used inside the Denied Modifier to check if the address is whitelisted/blacklisted:

    modifier Denied(bytes32 org, address wallet) {
        if (wallet == address(0) || wallet == Constants.SENTINEL_ADDRESS) {
            revert Errors.InvalidAddressProvided();
->        } else if (allowFeature[org]) {
            if (!isListed(org, wallet)) revert Errors.AddresNotAllowed();
            _;
->       } else if (denyFeature[org]) {
            if (isListed(org, wallet)) revert Errors.AddressDenied();
            _;
        } else {
            _;
        }
    }

Furthermore, the denyFeature/allowFeature is used for a check inside function AddToList:

    function addToList(address[] memory users)
        external
        IsRootSafe(_msgSender())
        requiresAuth
    {
        if (users.length == 0) revert Errors.ZeroAddressProvided();
        bytes32 org = getOrgHashBySafe(_msgSender());
->        if (!allowFeature[org] && !denyFeature[org]) {
            revert Errors.DenyHelpersDisabled();
        }
        address currentWallet = Constants.SENTINEL_ADDRESS;
        for (uint256 i; i < users.length;) {
            address wallet = users[i];
            if (
                wallet == address(0) || wallet == Constants.SENTINEL_ADDRESS
                    || wallet == address(this) || currentWallet == wallet
            ) revert Errors.InvalidAddressProvided();
            // Avoid duplicate wallet
            if (listed[org][wallet] != address(0)) {
                revert Errors.UserAlreadyOnList();
            }
            // Add wallet to List
            listed[org][currentWallet] = wallet;
            currentWallet = wallet;
            unchecked {
                ++i;
            }
        }
        listed[org][currentWallet] = Constants.SENTINEL_ADDRESS;
        listCount[org] += users.length;
        emit Events.AddedToList(users);
    }

The problem however has to do with the check performed inside addToList:

->        if (!allowFeature[org] && !denyFeature[org]) {

This line of code checks if the Helpers(AllowFeature/DenyFeature) are enabled, if not it will revert.

But not anywhere inside the function does it check if the wallets of the users are whitelisted or blacklisted if one of the Helpers were to be enabled.

This means that users that are blacklisted can still be added to the List.

Tools used

Manual Review

Recommendation

Make sure to implement the Denied modifier inside this function to check for the wallets of the users.

[0xRizwan]

I think, its already checked for wallets too.

            // Avoid duplicate wallet
            if (listed[org][wallet] != address(0)) {
                revert Errors.UserAlreadyOnList();
            }

[alfredolopez80]

well two things!!

1. When you disable both feature, is impossible to add any address in the list
2. If you try to add again (duplicate) any address the method revert!!, inmediatelly in both method addToList and dropToList like mention @0xRizwan:

            // Avoid duplicate wallet
            if (listed[org][wallet] != address(0)) {
                revert Errors.UserAlreadyOnList();
            }

so taking into account all of this this is not a issue!!