Redundant check of owner() != address(0)

[code423n4]

Handle

WatchPug

Vulnerability details

By the time MixinRoles._initializeMixinRoles() is called, there are no roles configured yet, therefore the check of !isLockManager(sender) and !isKeyGranter(sender) is redundant.

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/mixins/MixinRoles.sol#L22-L36

  function _initializeMixinRoles(address sender) internal {

    // for admin mamangers to add other lock admins
    _setRoleAdmin(LOCK_MANAGER_ROLE, LOCK_MANAGER_ROLE);

    // for lock managers to add/remove key granters
    _setRoleAdmin(KEY_GRANTER_ROLE, LOCK_MANAGER_ROLE);

    if (!isLockManager(sender)) {
      _setupRole(LOCK_MANAGER_ROLE, sender);  
    }
    if (!isKeyGranter(sender)) {
      _setupRole(KEY_GRANTER_ROLE, sender);
    }
  }

Recommendation

Change to:

  function _initializeMixinRoles(address sender) internal {

    // for admin mamangers to add other lock admins
    _setRoleAdmin(LOCK_MANAGER_ROLE, LOCK_MANAGER_ROLE);

    // for lock managers to add/remove key granters
    _setRoleAdmin(KEY_GRANTER_ROLE, LOCK_MANAGER_ROLE);

    _setupRole(LOCK_MANAGER_ROLE, sender);

    _setupRole(KEY_GRANTER_ROLE, sender);
  }

[0xleastwood]

This seems right