Missing Denylist Check in `addOwnerWithThreshold` Function

[hats-bug-reporter[bot]]

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

Description: Description\ The addOwnerWithThreshold function in the PalmeraModule contract currently lacks a check to ensure that the ownerAdded address is not on the denylist.The function does not currently check if the ownerAdded address is on the denylist for the specified organization (org). This can lead to the following issues:

1. Security Vulnerability: Unauthorized or malicious addresses could be added as owners, potentially compromising the security of the Safe Multisig Wallet.
2. Inconsistency: If the organization uses allowlists or denylists to manage access control, the lack of this check makes the function inconsistent with other parts of the contract that enforce these lists.
3. Non-Compliance: Organizations may have policies or regulations that require certain addresses to be explicitly allowed or denied. The absence of this check could result in non-compliance with such policies.
4. Error Potential: Adding an address that should not have permissions can lead to operational errors and issues

Attack Scenario

1. Deploy the PalmeraModule contract.
2. Enable the denylist feature for an organization.
3. Add an address to the denylist.
4. Attempt to add the denied address as an owner using the addOwnerWithThreshold function.
5. Observe that the function does not revert, allowing the denied address to be added as an owner.

Attachments

1. Proof of Concept (PoC) File

   
   function addOwnerWithThreshold(
       address ownerAdded,//@audit-denied address can be added as owner
       uint256 threshold,
       address targetSafe,
       bytes32 org
   )
       external
       validAddress(ownerAdded)
       SafeRegistered(targetSafe)
       requiresAuth
   {
       address caller = _msgSender();
       if (hasNotPermissionOverTarget(caller, org, targetSafe)) {
           revert Errors.NotAuthorizedAddOwnerWithThreshold();
       }
   
       ISafe safeTarget = ISafe(targetSafe);
       /// If the owner is already an owner
       if (safeTarget.isOwner(ownerAdded)) {
           revert Errors.OwnerAlreadyExists();
       }
   
       bytes memory data =
           abi.encodeCall(ISafe.addOwnerWithThreshold, (ownerAdded, threshold));
       /// Execute transaction from target safe
       _executeModuleTransaction(targetSafe, data);
   }

2. **Revised Code File (Optional)**
<!-- If possible, please provide a second file containing the revised code that offers a potential fix for the vulnerability. This file should include the following information:
- Comment with a clear explanation of the proposed fix.
- The revised code with your suggested changes.
- Any additional comments or explanations that clarify how the fix addresses the vulnerability. -->

function addOwnerWithThreshold( address ownerAdded, uint256 threshold, address targetSafe, bytes32 org ) external validAddress(ownerAdded) SafeRegistered(targetSafe) requiresAuth Denied(org, ownerAdded) // Added Denied modifier to check if ownerAdded is denied { address caller = _msgSender(); if (hasNotPermissionOverTarget(caller, org, targetSafe)) { revert Errors.NotAuthorizedAddOwnerWithThreshold(); }

    ISafe safeTarget = ISafe(targetSafe);
    /// If the owner is already an owner
    if (safeTarget.isOwner(ownerAdded)) {
        revert Errors.OwnerAlreadyExists();
    }

    bytes memory data =
        abi.encodeCall(ISafe.addOwnerWithThreshold, (ownerAdded, threshold));
    /// Execute transaction from target safe
    _executeModuleTransaction(targetSafe, data);
}

[alfredolopez80]

is not issue, is by design, we handle the same approach of safe in this case and can't add any additional feature in this case of addOwnerWithThreshold or removeOwner, in all case is a proposal o additional feature to add, but not a issue!!