hats-finance / Palmera-0x5fee7541ddcd51ba9f4af606f87b2c42eea655be

Palmera hierarchical module
0 stars 4 forks source link

`enableAllowlist` and `enableDenylist` do not check the listcount > 0, possibly blocking or allowing transactions incorrectly #83

Open hats-bug-reporter[bot] opened 4 months ago

hats-bug-reporter[bot] commented 4 months ago

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

Description: Description\ The functions of enableAllowlist and enableDenylist do not check the listcount to make sure any changes do not enable or disable wallet transactions for listed addresses already in the listed array.

    function enableAllowlist() external IsRootSafe(_msgSender()) requiresAuth {
        bytes32 org = getOrgHashBySafe(_msgSender());
        allowFeature[org] = true;
        denyFeature[org] = false;
    }

    /// @dev Method to Enable Allowlist
    function enableDenylist() external IsRootSafe(_msgSender()) requiresAuth {
        bytes32 org = getOrgHashBySafe(_msgSender());
        allowFeature[org] = false;
        denyFeature[org] = true;
    }

Vulnerability\ If there are already listed addresses in the allowlist and the enableDenylist is set, it will lockout the previously allowed addresses temporarily till it is corrected. The other scenario is that current denied addresses could be allowed accidently.

Remediation\ First check the listcount and if there are any existing addresses first let them be removed.

alfredolopez80 commented 4 months ago

these actions are executed by RootSafe, so you need to know the consequences of your actions, since you know that the contract handles a single list for both actions and the behavior depends on the enabled feature. Therefore changing from one to the other will bring awareness that he must adjust at the list level, this is the reason why both options cannot be enabled at the same time!! as you can see in the contract.

Therefore I consider it not an issue!!