Possible `OUT OF GAS` on `pauseForceAllVaults`, `unpauseForceAllVaults`, `pauseActionInAllVaults` and `upauseActionInAllVaults` functions

Git branch: M03

Affected smart contract

https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/Chief.sol#L289-L298 https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/Chief.sol#L300-L309 https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/Chief.sol#L311-L326 https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/Chief.sol#L328-L343 https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/Chief.sol#L362-L375 https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/Chief.sol#L399-L419

Description

In the Chief contract, the setPermissionlessDeployments function allows any user to deploy vaults. If this is allowed, a malicious user could create a large number of vaults, breaking the functionality of the pauseForceAllVaults, unpauseForceAllVaults, pauseActionInAllVaults, and unpauseActionInAllVaults functions, as these functions iterate over the array of vaults using a for loop ending in out of gas error

File: packages/protocol/src/Chief.sol

395:  function _changePauseState(address[] memory vaults, bytes memory callData) internal {
396:    uint256 alength = vaults.length;
397:    for (uint256 i; i < alength;) {
398:      vaults[i].functionCall(callData, ": pause call failed");
399:      unchecked {
400:        ++i;
401:      }
402:    }
403:  }

430:    function _checkValidVault(address vault) internal view {
431:    _checkInputIsNotZeroAddress(vault);
432:    uint256 len = _vaults.length;
433:    bool isInVaultList;
434:    for (uint256 i = 0; i < len;) {
435:      if (vault == _vaults[i]) {
436:        isInVaultList = true;
437:      }
438:      unchecked {
439:        ++i;
440:      }
441:    }
442:    if (!isInVaultList) {
443:      revert Chief__checkValidVault_notValidVault();
444:    }
445:  }

Attack scenario

With the permissionlessDeployments allowed, the attacker use the deployVault function to deploy a large number of vaults. The pauseForceAllVaults, unpauseForceAllVaults, pauseActionInAllVaults, and unpauseActionInAllVaults functions can't execute the transaction because they would run out of gas

The timelock could use the setVaults to fix this bug but it's laborious and expensive

Recommendation

Add functions to pause an array of vaults:

@@ -294,7 +294,12 @@ contract Chief is CoreRoles, AccessControl, IChief {
    */
   function pauseForceAllVaults() external onlyRole(PAUSER_ROLE) {
     bytes memory callData = abi.encodeWithSelector(IPausableVault.pauseForceAll.selector);
-    _changePauseState(callData);
+    _changePauseState(_vaults, callData);
+  }
+
+  function pauseForceVaults(address[] calldata vaults) external onlyRole(PAUSER_ROLE) {
+    bytes memory callData = abi.encodeWithSelector(IPausableVault.pauseForceAll.selector);
+    _changePauseState(vaults, callData);
   }

   /**
@@ -305,7 +310,12 @@ contract Chief is CoreRoles, AccessControl, IChief {
    */
   function unpauseForceAllVaults() external onlyRole(UNPAUSER_ROLE) {
     bytes memory callData = abi.encodeWithSelector(IPausableVault.unpauseForceAll.selector);
-    _changePauseState(callData);
+    _changePauseState(_vaults, callData);
+  }
+
+  function unpauseForceVaults(address[] calldata vaults) external onlyRole(PAUSER_ROLE) {
+    bytes memory callData = abi.encodeWithSelector(IPausableVault.unpauseForceAll.selector);
+    _changePauseState(vaults, callData);
   }

   /**
@@ -322,7 +332,15 @@ contract Chief is CoreRoles, AccessControl, IChief {
     onlyRole(PAUSER_ROLE)
   {
     bytes memory callData = abi.encodeWithSelector(IPausableVault.pause.selector, action);
-    _changePauseState(callData);
+    _changePauseState(_vaults, callData);
+  }
+
+  function pauseActionInVaults(address[] calldata vaults, IPausableVault.VaultActions action)
+    external
+    onlyRole(PAUSER_ROLE)
+  {
+    bytes memory callData = abi.encodeWithSelector(IPausableVault.pause.selector, action);
+    _changePauseState(vaults, callData);
   }

   /**
@@ -339,7 +357,15 @@ contract Chief is CoreRoles, AccessControl, IChief {
     onlyRole(UNPAUSER_ROLE)
   {
     bytes memory callData = abi.encodeWithSelector(IPausableVault.unpause.selector, uint8(action));
-    _changePauseState(callData);
+    _changePauseState(_vaults, callData);
+  }
+
+  function upauseActionInVaults(address[] calldata vaults, IPausableVault.VaultActions action)
+    external
+    onlyRole(UNPAUSER_ROLE)
+  {
+    bytes memory callData = abi.encodeWithSelector(IPausableVault.unpause.selector, uint8(action));
+    _changePauseState(vaults, callData);
   }

   /**
@@ -364,10 +390,10 @@ contract Chief is CoreRoles, AccessControl, IChief {
    *
    * @param callData encoded data containing pause or unpause commands.
    */
-  function _changePauseState(bytes memory callData) internal {
-    uint256 alength = _vaults.length;
+  function _changePauseState(address[] memory vaults, bytes memory callData) internal {
+    uint256 alength = vaults.length;
     for (uint256 i; i < alength;) {
-      address(_vaults[i]).functionCall(callData, ": pause call failed");
+      vaults[i].functionCall(callData, ": pause call failed");
       unchecked {
         ++i;
       }

Fujicracy / fuji-v2