Inability to Execute Transactions via execTransactionOnBehalf When Super Safe is Removed

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xe4d08a68ff643785f381b8c45a8daacfee9ad47310ab7b066a4b892aa934468d Severity: high

Description: Description\ The execTransactionOnBehalf function in the PalmeraModule contract is designed to allow root safes,super safes, and safe leads to execute transactions on behalf of a target safe. However, if a super safe is not added or is removed by the root safe, the function will revert, preventing the transaction from being executed. This can lead to a situation where legitimate transactions cannot be processed, potentially disrupting the operations of the organization.

Attack Scenario\

1. Initial Setup: Organization OrgA has a root safe RootSafeA. RootSafeA has a super safe SuperSafeA. SuperSafeA has a target safe TargetSafeA.
2. Transaction Execution: A user attempts to execute a transaction on behalf of TargetSafeA using the execTransactionOnBehalf function. The function verifies that SuperSafeA is registered and has the necessary permissions.
3. Super Safe Removal: RootSafeA removes SuperSafeA from the organization. The user attempts to execute another transaction on behalf of TargetSafeA.
4. Transaction Failure: The execTransactionOnBehalf function reverts because SuperSafeA is no longer registered, and the function cannot verify the necessary permissions.

Attachments

1. Proof of Concept (PoC) File

   function execTransactionOnBehalf(
   bytes32 org,
   address superSafe, // can be root or super safe
   address targetSafe,
   address to,
   uint256 value,
   bytes calldata data,
   Enum.Operation operation,
   bytes memory signatures
   )
   external
   payable
   nonReentrant
   SafeRegistered(superSafe)
   SafeRegistered(targetSafe)
   Denied(org, to)
   returns (bool result)
   {
   address caller = _msgSender();
   // Caller is Safe Lead: bypass check of signatures
   // Caller is another kind of wallet: check if it has the corrects signatures of the root/super safe
   if (!isSafeLead(getSafeIdBySafe(org, targetSafe), caller)) {
       // Check if caller is a superSafe of the target safe (checking with isTreeMember because is the same method!!)
       if (hasNotPermissionOverTarget(superSafe, org, targetSafe)) {
           revert Errors.NotAuthorizedExecOnBehalf();
       }
       // Caller is a safe then check caller's safe signatures.
       bytes memory palmeraTxHashData = encodeTransactionData(
           org,
           superSafe,
           targetSafe,
           to,
           value,
           data,
           operation,
           nonce[org]
       );
       // Verify Collision of Nonce with multiple txs in the same range of time, study to use a nonce per org
   
       ISafe leadSafe = ISafe(superSafe);
       bytes memory sortedSignatures = processAndSortSignatures(
           keccak256(palmeraTxHashData), signatures, leadSafe.getOwners()
       );
       leadSafe.checkSignatures(
           keccak256(palmeraTxHashData),
           palmeraTxHashData,
           sortedSignatures
       );
   }
   // Increase nonce and execute transaction.
   nonce[org]++;
   // Execute transaction from target safe
   ISafe safeTarget = ISafe(targetSafe);
   result =
       safeTarget.execTransactionFromModule(to, value, data, operation);
   
   if (!result) revert Errors.TxOnBehalfExecutedFailed();
   emit Events.TxOnBehalfExecuted(
       org, caller, superSafe, targetSafe, result
   );
   }

2. Revised Code File (Optional)

   The current implementation of the execTransactionOnBehalf function can lead to situations where transactions cannot be executed if the super safe is removed. Implementing a fallback mechanism for the root safe can help ensure that legitimate transactions can still be processed, maintaining the smooth operation of the organization

[alfredolopez80]

is the attack scenario is the superSafe can't execute the executionOnBehalf, is not a issue is a expected behavior, becuase the root safe remove this Super Safe, not must be the ability to executio any transaction on Behalf any child Safe into the On chain organization!!

if i misunderstood the attack scenarios or missed something, pls clarify more!!

[AresAudits]

hey @alfredolopez80 ,u misunderstood the issue. The core problem is not about the removed super safe trying to execute transactions. Instead, the issue arises when legitimate entities (such as the root safe, safe lead, or other authorized safes) attempt to execute transactions after the super safe has been removed.

When a super safe is removed by the root safe, the execTransactionOnBehalf function will revert, preventing the transaction from being executed by the root safe, safe lead, or other authorized safes(even if these roles has permission to execute transaction on behalf). This is because the function relies on the superSafe being registered and having the necessary permissions, which is no longer the case once it is removed.