`removeOwner` Function Allowing Threshold Manipulation Can Be Leveraged by Attacker

[hats-bug-reporter[bot]]

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

Description: Description\ The removeOwner function in the PalmeraModule.sol contract can be called by various roles within the DAO, including Safe Lead, Safe without execOnBehalf, SuperSafe, and Root Safe. However, there is a potential security vulnerability related to the threshold parameter. If any of these roles are malicious, they could manipulate the threshold parameter to reduce the number of required signatures to execute transactions, compromising the security of the multisig wallet.

Attack Scenario\ Consider a Decentralized Autonomous Organization (DAO) that uses a multisig wallet to manage its funds. The wallet has 5 owners, and the current threshold for executing transactions is set to 3 (i.e., 3 signatures are required).

1. Initial State: The DAO's multisig wallet has 5 owners. The current threshold is 3.
2. Malicious Action: A malicious actor with one of the roles (Safe Lead, Safe without execOnBehalf, SuperSafe, or Root Safe) calls the removeOwner function with the threshold parameter set to 1. The function removes an owner and sets the new threshold to 1.
3. Result: The multisig wallet now requires only 1 signature to execute transactions. The malicious actor can now execute any transaction with just their own signature, compromising the security of the DAO's funds.

Attachments

1. Proof of Concept (PoC) File

   function removeOwner(//@audit-direct threshold manipution by any authorized role
       address prevOwner,
       address ownerRemoved,
       uint256 threshold,
       address targetSafe,
       bytes32 org
   ) external SafeRegistered(targetSafe) requiresAuth {
       address caller = _msgSender();
       if (
           prevOwner == address(0) || ownerRemoved == address(0)
               || prevOwner == Constants.SENTINEL_ADDRESS
               || ownerRemoved == Constants.SENTINEL_ADDRESS
       ) {
           revert Errors.ZeroAddressProvided();
       }
   
       if (hasNotPermissionOverTarget(caller, org, targetSafe)) {
           revert Errors.NotAuthorizedRemoveOwner();
       }
       ISafe safeTarget = ISafe(targetSafe);
       /// if Owner Not found
       if (!safeTarget.isOwner(ownerRemoved)) {
           revert Errors.OwnerNotFound();
       }
   
       bytes memory data = abi.encodeCall(
           ISafe.removeOwner, (prevOwner, ownerRemoved, threshold)
       );
   
       /// Execute transaction from target safe
       _executeModuleTransaction(targetSafe, data);
   }

2. Revised Code File (Optional)

• To mitigate this vulnerability, the threshold parameter should not be taken as input. Instead, the current threshold should be retrieved directly from the ISafe contract using the getThreshold function. This ensures that the threshold cannot be maliciously lowered.

 // Get the current threshold
    uint256 threshold = safeTarget.getThreshold();

    bytes memory data = abi.encodeCall(
        ISafe.removeOwner, (prevOwner, ownerRemoved, threshold)
    );

[alfredolopez80]

Is not issue, we follow the same approach of safe, in this case and is important have the option to adjust the threshold if you remove any owner

about the option to run this with the execTransactionFromModule method it is an option for the user when they enable the Palmera Module, it is the same for execTransactionOnBehalf, we put the correct check to prevent bad actors from using the method but in the end it is RootSafe assigning the correct role to an inappropriate or wrong Wallet is already something that is beyond the scope of the module's role management

[batmanBinary]

hey @alfredolopez80 ,thank you for the response on this issue,

as i mentioned In a typical Safe wallet, a valid caller can directly execute transactions, with straightforward permissions. However, the Palmera Module introduces a hierarchical structure with multiple layers of safes and roles (e.g., Safe Lead, Safe without execOnBehalf, SuperSafe, Root Safe) authorized to execute transactions on behalf of others. This complexity, while flexible, introduces significant security risks.

The removeOwner function in PalmeraModule.sol allows privileged roles to remove an owner and manipulate the threshold parameter. A malicious actor could exploit this to reduce the threshold to a very low number, compromising the multisig wallet's security.

Attack Scenario:

• TheDAO's multisig wallet has 5 owners.
• The current threshold for executing transactions is 3.
• A privileged role calls removeOwner and sets the threshold to 1 while removing an owner.
• The wallet now requires only 1 signature for transactions.
• The malicious actor can execute any transaction alone, compromising the DAO's funds

we should mitigate this issue by : Restrict threshold manipulation to only the Root Safe role. Other roles should fetch the threshold using a getThreshold() function without modifying it.

This change enhances security by preventing unauthorized threshold manipulation while maintaining flexibility for managing ownership changes. Only the Root Safe role can make critical threshold adjustments, safeguarding the DAO's funds and operations.