Removed safe can bypass security checks in execTransactionOnBehalf

[hats-bug-reporter[bot]]

Github username: @lanrebayode Twitter username: lanrebayode1 Submission hash (on-chain): 0x9533a8977c3fabcfca99939a8dd8e06d1e95bdba7594e0af784ded8cf710f2d7 Severity: high

Description: Description\ Describe the context and the effect of the vulnerability.

Attack Scenario\ After a safe was removed from org, the safe exercises lead role on target safe to call execTransactionOnBehalf() with any malicious data because the lead role on target still persist after safe removal.

It was observed that when role was set, user/safe was granted lead role

     // Update safe/org lead
            _safe.lead = user;
        }
        RolesAuthority _authority = RolesAuthority(rolesAuthority);
        _authority.setUserRole(user, uint8(role), enabled);

And when safe was removed, all roles granted was revoked

 // Disable safe lead role
        disableSafeLeadRoles(_safe.safe);

But the isLead() check does not confirm if the address still has the role

    function isSafeLead(uint256 safeId, address user)
        public
        view
        returns (bool)
    {
        bytes32 org = getOrgBySafe(safeId);
        DataTypes.Safe memory _safe = safes[org][safeId];
        if (_safe.safe == address(0)) return false;
   @>    if (_safe.lead == user) { //this still holds
            return true;
        }
        return false;
    }

Since the target safe struct was not updated in the removal process, targetSafe.lead == safe_A(the caller)!

Attachments

Proof of Concept (PoC) File

1. safeRoot gives safe_A lead role of safe_Target by calling setRole()

   function setRole(
       DataTypes.Role role,
       address user,
       uint256 safeId,
       bool enabled
   ) external validAddress(user) IsRootSafe(_msgSender()) requiresAuth {
       address caller = _msgSender();
       if (
           role == DataTypes.Role.ROOT_SAFE
               || role == DataTypes.Role.SUPER_SAFE
       ) {
           revert Errors.SetRoleForbidden(role);
       }
       if (!isRootSafeOf(caller, safeId)) {
           revert Errors.NotAuthorizedSetRoleAnotherTree();
       }
       DataTypes.Safe storage _safe = safes[getOrgHashBySafe(caller)][safeId];
       // Check if safe is part of the caller org
       if (
           role == DataTypes.Role.SAFE_LEAD
               || role == DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY
               || role == DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY
       ) {
           // Update safe/org lead
       @>    _safe.lead = user; //user = safe_A
       }
       RolesAuthority _authority = RolesAuthority(rolesAuthority);
       _authority.setUserRole(user, uint8(role), enabled);
   }

2. safeRoot of an Org calls removeSafe() to remove a safe(safe_A) and clears all role given to safe_A.

3. But safe_A can still call execTransactionOnBehalf() and bypass all signatures check with any malicious data.

4. Revised Code File (Optional) Include a check if the caller is a safe

   if (safes[org][caller].tier == DataTypes.Tier.REMOVED) {
         other  verification & signature check!
       }

Another fix will be to return false in the isSafeLead() if the user role has been revoked!

[0xmahdirostami]

Duplicates of #38 #31