Removing a safe with LEAD role do not remove all LEAD roles

[hats-bug-reporter[bot]]

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

Description: Description\ Safe with more than one LEAD role, will still have LEAD role, if if one is removed.

Attack Scenario\ When safe is removed, removeSafe() calls disableSafeLeadRoles() to remove the LEAD roles of the SAFE.

function removeSafe(uint256 safeId)
        public
        SafeRegistered(_msgSender())
        requiresAuth
    {
        address caller = _msgSender();
        bytes32 org = getOrgHashBySafe(caller);
        uint256 callerSafe = getSafeIdBySafe(org, caller);
        uint256 rootSafe = getRootSafe(safeId);
        /// Avoid Replay attack
        ... MORE CODE
        /// we guarantee the child was moving to another SuperSafe in the Org
        /// and validate after in the Disconnect Safe method
        _safe.child = new uint256[](0);

        // Revoke roles to safe
        RolesAuthority _authority = RolesAuthority(rolesAuthority);
        _authority.setUserRole(
            _safe.safe, uint8(DataTypes.Role.SUPER_SAFE), false
        );
        // Disable safe lead role
        disableSafeLeadRoles(_safe.safe);

        ....
    }

    function disableSafeLeadRoles(address user) private {
        RolesAuthority _authority = RolesAuthority(rolesAuthority);
        if (_authority.doesUserHaveRole(user, uint8(DataTypes.Role.SAFE_LEAD)))
        {
            _authority.setUserRole(user, uint8(DataTypes.Role.SAFE_LEAD), false);
        } else if (
            _authority.doesUserHaveRole(
                user, uint8(DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY)
            )
        ) {
            _authority.setUserRole(
                user, uint8(DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY), false
            );
        } else if (
            _authority.doesUserHaveRole(
                user, uint8(DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY)
            )
        ) {
            _authority.setUserRole(
                user, uint8(DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY), false
            );
        }
    }

As you can see if the SAFE has SAFE_LEAD and any other LEAD role (ex. SAFE_LEAD_EXEC_ON_BEHALF_ONLY, SAFE_LEAD_MODIFY_OWNERS_ONLY)

Only the first that match will be removed.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional) else if statements should be changed with if, since the roles are different and it should check for all of them, and thus remove if the SAFE have it.

[alfredolopez80]

duplicate #37 @0xmahdirostami