`setRole` Function Incorrectly Assigns `_safe.lead` without Validating `enabled` Parameter

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xbf10b39a27d651c17d6761180193456a8268a6128535dc785370130b083e2c69 Severity: high

Description: Description: The setRole function assigns the _safe.lead attribute to a user if the role is related to safe leadership (SAFE_LEAD, SAFE_LEAD_EXEC_ON_BEHALF_ONLY, SAFE_LEAD_MODIFY_OWNERS_ONLY). However, the function fails to check the enabled boolean parameter before updating _safe.lead. This can lead to unauthorized access control issues where users might be improperly assigned as safe leads.

Impact: This oversight can result in unauthorized access control, allowing users to be incorrectly recognized as safe leads even if their role was meant to be disabled. This compromises the security and integrity of the system.

Proof of Concept (PoC): Consider the current implementation of the setRole function:

function setRole(uint256 safeId, address user, uint8 role, bool enabled)
    external
    IsRootSafe(_msgSender())
    requiresAuth
{
    bytes32 org = getOrgBySafe(safeId);
    DataTypes.Safe storage _safe = safes[org][safeId];

    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;
    }
}

In the above code, _safe.lead is assigned to user without checking if enabled is true. This means users could inadvertently be granted lead roles.

Mitigation:

1. Validate the enabled Parameter: Ensure the enabled parameter is checked before updating _safe.lead.
2. Revoke Lead Role Appropriately: If enabled is false, and the user does not have any other lead roles, _safe.lead should be set to address(0).

Updated setRole function:

function setRole(uint256 safeId, address user, uint8 role, bool enabled)
    external
    IsRootSafe(_msgSender())
    requiresAuth
{
    RolesAuthority _authority = RolesAuthority(rolesAuthority);
    bytes32 org = getOrgBySafe(safeId);
    DataTypes.Safe storage _safe = safes[org][safeId];

    if (enabled) {
        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;
        }
    } else {
        // Disable the role and check if the user has any other lead roles
        _authority.setUserRole(user, role, false);

        // If the user has no other lead roles, revoke the lead status
        if (
            !_authority.doesUserHaveRole(user, uint8(DataTypes.Role.SAFE_LEAD)) &&
            !_authority.doesUserHaveRole(user, uint8(DataTypes.Role.SAFE_LEAD_EXEC_ON_BEHALF_ONLY)) &&
            !_authority.doesUserHaveRole(user, uint8(DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY))
        ) {
            _safe.lead = address(0);
        }
    }
}

By including the enabled parameter check and ensuring that lead roles are properly revoked, this updated function maintains the integrity of access control, preventing unauthorized role assignments.