Vulnerability Details

When we remove a strategy from the MultiStrategyVault we undeploy the total assets that this strategy holds and then allocate them towards the other strategies. However the allocated amount is incorrect. We can see the remove functionality below:

    function removeStrategy(uint256 index) external onlyRole(VAULT_MANAGER_ROLE) {
        // Validate the index to ensure it is within bounds
        if (index >= _strategies.length) revert InvalidStrategyIndex(index);

        // Retrieve the total assets managed by the strategy to be removed
        uint256 strategyAssets = _strategies[index].totalAssets();

        // Update the total weight and mark the weight of the removed strategy as zero
        _totalWeight -= _weights[index];
        _weights[index] = 0;

        // If the strategy has assets, undeploy them and allocate accordingly
        if (strategyAssets > 0) {
@->            IStrategy(_strategies[index]).undeploy(strategyAssets);
@->            _allocateAssets(strategyAssets);
        }
    }

After undeploying from the strategy we call _allocateAssets(strategyAssets) and strategyAssets = _strategies[index].totalAssets() so we are basically allocating the totalAssets() of the strategy instead of what the .undeploy() call returned. If the transferred amount from the undeploy is lower than what totalAsstes() shows the call will revert because we won't actually have in balance all of the totalAsstes().

For example if we look at StrategySupplyERC4626::totalAssets() would return the vault shares and not the actual amount will be undeployed from the vault. So even if shares:assets = 1:1 the call would still revert if for example the vault takes a withdrawal fee as in our Vault contract. That is because say that we have 100 tokens dedicated to a strategy and totalAsstes() = 100 tokens. Now we undeploy from that strategy and say the ERC4626 vault takes 1% withdrawal fee, the funds transferred to the MultiStrategyVault will be 100 - (1/100*100) = 99 and we would try to allocate 100 tokens to other strategies instead of 99 tokens.

Impact

Unable to remove a strategy

Recommended mitigation steps

Change to the following

        if (strategyAssets > 0) {
-           IStrategy(_strategies[index]).undeploy(strategyAssets);
-            _allocateAssets(strategyAssets);
+            uint256 withdrawnAssets = IStrategy(_strategies[index]).undeploy(strategyAssets);
+            _allocateAssets(withdrawnAssets);
        }