Ineffective Revocation of Multiple Roles in `disableSafeLeadRoles` Function

[hats-bug-reporter[bot]]

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

Description: Description: The disableSafeLeadRoles function is designed to revoke specific Safe Lead roles from a user. However, the current implementation only revokes the first role it encounters and skips the rest. This means if a user has multiple roles, only the first role in the conditional checks is revoked, leaving the other roles intact.

Impact: If a user has more than one Safe Lead role, such as SAFE_LEAD_EXEC_ON_BEHALF_ONLY and SAFE_LEAD_MODIFY_OWNERS_ONLY, the function will only revoke the SAFE_LEAD_EXEC_ON_BEHALF_ONLY role and will not revoke the SAFE_LEAD_MODIFY_OWNERS_ONLY role, leading to insufficient role revocation.

Proof of Concept (PoC):

1. Assume a user has the following roles:
   • SAFE_LEAD_EXEC_ON_BEHALF_ONLY
   • SAFE_LEAD_MODIFY_OWNERS_ONLY
2. The disableSafeLeadRoles function is called for this user:

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

3. Only the first role, SAFE_LEAD_EXEC_ON_BEHALF_ONLY, is revoked, leaving the SAFE_LEAD_MODIFY_OWNERS_ONLY role active.

Mitigation: Modify the disableSafeLeadRoles function to check and revoke all Safe Lead roles independently, regardless of their position in the conditional checks.

Updated disableSafeLeadRoles function:

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);
    }
    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);
    }
    if (_authority.doesUserHaveRole(user, uint8(DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY))) {
        _authority.setUserRole(user, uint8(DataTypes.Role.SAFE_LEAD_MODIFY_OWNERS_ONLY), false);
    }
}

[0xRizwan]

Agreed with this issue, would leave on @alfredolopez80 on final severity of this issue.