Description

The changeAllocations function in the ConcreteMultiStrategyVault contract does not check the vaultIdle status before redistributing assets:

function changeAllocations(
	Strategy[] storage strategies,
	Allocation[] calldata allocations_,
	bool redistribute,
	address asset
) external {
	// [...]
	if (redistribute) {
		pullFundsFromStrategies(strategies);
		distributeAssetsToStrategies(strategies, IERC20(asset).balanceOf(address(this)));
	}
	// [...]
}

According to the contract's design, setting vaultIdle to true should prevent all deposits into underlying strategies. However, when changeAllocations is called with the redistribute parameter set to true, the function proceeds to call distributeAssetsToStrategies without this validation.

Impact

The changeAllocations function allows deposits into strategies even when vaultIdle is true, bypassing the intended idle vault restrictions.

Recommendations

We recommend adding the vaultIdle check within the changeAllocations function before assets are redistributed.

function changeAllocations(
	Strategy[] storage strategies,
	Allocation[] calldata allocations_,
	bool redistribute,
	address asset
) external {
	// [...]
	if (redistribute) {
		pullFundsFromStrategies(strategies);
+       if (vaultIdle) revert("vaultIdle");
		distributeAssetsToStrategies(strategies, IERC20(asset).balanceOf(address(this)));
	}
	// [...]
}

Remediation

Blueprint Finance provided the following response to this finding:

We haven’t used the vaultIdle flag so far and it appears to be redundant in our current logic. The changeAllocations() function isn't intended to interact with vaultIdle, so we’re choosing to skip this fix.