vaults: lack of validation `REBALANCER_ROLE` and `LIQUIDATOR_ROLE` exist

Title

Lack of validation to check whether or not the REBALANCER_ROLE and LIQUIDATOR_ROLE would be granted before the YieldVault/BorrowingVault is deployed, which results in losing the health of this protocol and decreased-performance of vaults due to failing to rebalance and liquidation.

Affected smart contract

Description

Within the CoreRoles, REBALANCER_ROLE and LIQUIDATOR_ROLE are defined like this: https://github.com/Fujicracy/fuji-v2/blob/6231dd1161602cc4b081fd2bab621fa6a3cb9394/packages/protocol/src/access/CoreRoles.sol#L13-L20

  bytes32 public constant HOUSE_KEEPER_ROLE = keccak256("HOUSE_KEEPER_ROLE");

  bytes32 public constant REBALANCER_ROLE = keccak256("REBALANCER_ROLE");   /// @audit 
  bytes32 public constant HARVESTER_ROLE = keccak256("HARVESTER_ROLE");
  bytes32 public constant LIQUIDATOR_ROLE = keccak256("LIQUIDATOR_ROLE");    /// @audit 

  bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
  bytes32 public constant UNPAUSER_ROLE = keccak256("UNPAUSER_ROLE");

Then, within the Chief# constructor(), some roles would be assigned via the _grantRole() like this: On the other hands, some roles like such as REBALANCER_ROLE and LIQUIDATOR_ROLE would not be granted here. https://github.com/Fujicracy/fuji-v2/blob/6231dd1161602cc4b081fd2bab621fa6a3cb9394/packages/protocol/src/Chief.sol#L119-L122

  constructor(bool deployTimelock, bool deployAddrMapper) {
    _grantRole(DEPLOYER_ROLE, msg.sender);
    _grantRole(HOUSE_KEEPER_ROLE, msg.sender);
    _grantRole(PAUSER_ROLE, address(this));
    _grantRole(UNPAUSER_ROLE, address(this));
    if (deployTimelock) _deployTimelockController();
    if (deployAddrMapper) _deployAddrMapper();
  }

Within the SystemAccessControl, the hasRole() modifier would be defined in order to check whether or not specified-address has proper role to call a function like this: https://github.com/Fujicracy/fuji-v2/blob/6231dd1161602cc4b081fd2bab621fa6a3cb9394/packages/protocol/src/access/SystemAccessControl.sol#L27-L32

  /**
   * @dev Modifier that checks `caller` has `role`.
   */
  modifier hasRole(address caller, bytes32 role) {
    if (!chief.hasRole(role, caller)) {
      revert SystemAccessControl__hasRole_missingRole(caller, role);
    }
    _;
  }

By being checked by the hasRole(msg.sender, REBALANCER_ROLE) modifier, both the YieldVault# rebalance() and the BorrowingVault# rebalance() can be called by only caller who has a REBALANCER_ROLE like this: https://github.com/Fujicracy/fuji-v2/blob/6231dd1161602cc4b081fd2bab621fa6a3cb9394/packages/protocol/src/vaults/borrowing/BorrowingVault.sol#L526 https://github.com/Fujicracy/fuji-v2/blob/6231dd1161602cc4b081fd2bab621fa6a3cb9394/packages/protocol/src/vaults/yield/YieldVault.sol#L179

  function rebalance(
    uint256 assets,
    uint256 debt,
    ILendingProvider from,
    ILendingProvider to,
    uint256 fee,
    bool setToAsActiveProvider
  )
    external
    hasRole(msg.sender, REBALANCER_ROLE) /// @audit - This caller must has "REBALANCER_ROLE"
    returns (bool)
  {
...

Also, by being checked by the hasRole(msg.sender, LIQUIDATOR_ROLE) modifier, both the BorrowingVault# liquidate() can be called by only caller who has a LIQUIDATOR_ROLE like this: https://github.com/Fujicracy/fuji-v2/blob/6231dd1161602cc4b081fd2bab621fa6a3cb9394/packages/protocol/src/vaults/borrowing/BorrowingVault.sol#L589

  function liquidate(
    address owner,
    address receiver
  )
    external
    hasRole(msg.sender, LIQUIDATOR_ROLE) /// @audit - This caller must has "LIQUIDATOR_ROLE"
    returns (uint256 gainedShares)
  {
...

Based on above, when both vaults (YieldVault and BorrowingVault) would be deployed via the vault factory (YieldVaultFactory and BorrowingVaultFactory), both the REBALANCER_ROLE and the LIQUIDATOR_ROLE must be granted to the proper addresses before the both vaults would be deployed.

However, within the constructor() of the YieldVault#constructor() and the BorrowingVault#constructor(), there is no validation to check whether or not both the REBALANCER_ROLE and the LIQUIDATOR_ROLE would be granted to the proper addresses.

If the owner forget or miss to grant both roles above to the proper address before both vaults above would be deployed, three functions defined in the both vaults below will always fail unless the owner realize it and grant each roles (REBALANCER_ROLE and LIQUIDATOR_ROLE) to the proper addresses.

the YieldVault# rebalance()
the BorrowingVault# rebalance()
the BorrowingVault# liquidate()

This lead to a bad situation that rebalance() and liquidate() above that was supposed to be occurred would fail repeatedly unless the owner realize it and grant each roles (REBALANCER_ROLE and LIQUIDATOR_ROLE) to the proper addresses. This results in losing the health of this protocol and decreased-performance of vaults due to failing to rebalance and liquidation.

Attack scenario

See the "Description" part above.

Recommendation

Consider adding a validation in order to check whether or not both roles (REBALANCER_ROLE and LIQUIDATOR_ROLE) would be granted to the proper addresses to the BorrowingVault#constructor() like this:

/// File: BorrowingVault.sol

  constructor(
    address asset_,
    address debtAsset_,
    address oracle_,
    address chief_,
    string memory name_,
    string memory symbol_,
    ILendingProvider[] memory providers_
  )
    BaseVault(asset_, chief_, name_, symbol_)
  {
+   require(REBALANCER_ROLE != bytes32(0), "The REBALANCER_ROLE must be granted to the proper address before the BorrowingVault would be deployed");
+   require(LIQUIDATOR_ROLE != bytes32(0), "The LIQUIDATOR_ROLE must be granted to the proper address before the BorrowingVault would be deployed");

...

Also, consider adding a validation in order to check whether or not the REBALANCER_ROLE would be granted to the proper addresses to the YieldVault#constructor() like this:

/// File: YieldVault.sol

  constructor(
    address asset_,
    address chief_,
    string memory name_,
    string memory symbol_,
    ILendingProvider[] memory providers_
  )
    BaseVault(asset_, chief_, name_, symbol_)
  {
+   require(REBALANCER_ROLE != bytes32(0), "The REBALANCER_ROLE must be granted to the proper address before the BorrowingVault would be deployed");

...

Fujicracy / fuji-v2