Reentrancy Preparation: Managed Pool

pkg/interfaces/contracts/pool-utils/IManagedPool.sol

@@ -54,6 +54,13 @@ interface IManagedPool is IBasePool {
      * they will effectively be included in any Pool operation that involves BPT.
      *
      * In the vast majority of cases, this function should be used instead of `totalSupply()`.
+     *
+     * WARNING: since this function reads balances directly from the Vault, it is potentially subject to manipulation
+     * via reentrancy. See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
+     *
+     * To call this function safely, attempt to trigger the reentrancy guard in the Vault by calling a non-reentrant
+     * function before calling `getActualSupply`. That will make the transaction revert in an unsafe context.
+     * (See `whenNotInVaultContext` in `ManagedPoolSettings`).
      */
     function getActualSupply() external view returns (uint256);
 
@@ -221,6 +228,13 @@ interface IManagedPool is IBasePool {
      * @dev This can be called by anyone to collect accrued AUM fees - and will be called automatically
      * whenever the supply changes (e.g., joins and exits, add and remove token), and before the fee
      * percentage is changed by the manager, to prevent fees from being applied retroactively.
+     *
+     * Correct behavior depends on the current supply, which is potentially manipulable if the pool
+     * is reentered during execution of a Vault hook. This is protected where overridden in ManagedPoolSettings,
+     * and so is safe to call on ManagedPool.
+     *
+     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
+     *
      * @return The amount of BPT minted to the manager.
      */
     function collectAumManagementFees() external returns (uint256);
@@ -230,6 +244,14 @@ interface IManagedPool is IBasePool {
      * @dev Attempting to collect AUM fees in excess of the maximum permitted percentage will revert.
      * To avoid retroactive fee increases, we force collection at the current fee percentage before processing
      * the update. Emits the ManagementAumFeePercentageChanged event. This is a permissioned function.
+     *
+     * To prevent changing management fees retroactively, this triggers payment of protocol fees before applying
+     * the change. Correct behavior depends on the current supply, which is potentially manipulable if the pool
+     * is reentered during execution of a Vault hook. This is protected where overridden in ManagedPoolSettings,
+     * and so is safe to call on ManagedPool.
+     *
+     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
+     *
      * @param managementAumFeePercentage - The new management AUM fee percentage.
      * @return amount - The amount of BPT minted to the manager before the update, if any.
      */
@@ -292,7 +314,13 @@ interface IManagedPool is IBasePool {
      * The caller may additionally pass a non-zero `mintAmount` to have some BPT be minted for them, which might be
      * useful in some scenarios to account for the fact that the Pool will have more tokens.
      *
-     * Emits the TokenAdded event.
+     * Emits the TokenAdded event. This is a permissioned function.
+     *
+     * Correct behavior depends on the token balances from the Vault, which may be out of sync with the state of
+     * the pool during execution of a Vault hook. This is protected where overridden in ManagedPoolSettings,
+     * and so is safe to call on ManagedPool.
+     *
+     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
      *
      * @param tokenToAdd - The ERC20 token to be added to the Pool.
      * @param assetManager - The Asset Manager for the token.
@@ -315,6 +343,11 @@ interface IManagedPool is IBasePool {
      * the future.
      *
      * Emits the TokenRemoved event. This is a permissioned function.
+     * Correct behavior depends on the token balances from the Vault, which may be out of sync with the state of
+     * the pool during execution of a Vault hook. This is protected where overridden in ManagedPoolSettings,
+     * and so is safe to call on ManagedPool.
+     *
+     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
      *
      * The caller may additionally pass a non-zero `burnAmount` to burn some of their BPT, which might be useful
      * in some scenarios to account for the fact that the Pool now has fewer tokens. This is a permissioned function.

pkg/interfaces/contracts/pool-utils/IProtocolFeeCache.sol

@@ -18,6 +18,12 @@ interface IProtocolFeeCache {
     /**
      * @notice Updates the cache to the latest value set by governance.
      * @dev Can be called by anyone to update the cached fee percentages.
+     *
+     * Correct behavior depends on the token balances from the Vault, which may be out of sync with the state of
+     * the pool during execution of a Vault hook. This is protected by a call to `ensureNotInVaultContext` in
+     * VaultReentrancyLib where overridden in `ProtocolFeeCache`, and so is safe to call on ManagedPool.
+     *
+     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
      */
     function updateProtocolFeePercentageCache() external;
 }

pkg/pool-utils/contracts/external-fees/ProtocolFeeCache.sol

@@ -22,6 +22,7 @@ import "@balancer-labs/v2-interfaces/contracts/standalone-utils/IProtocolFeePerc
 import "@balancer-labs/v2-solidity-utils/contracts/helpers/WordCodec.sol";
 import "@balancer-labs/v2-solidity-utils/contracts/openzeppelin/SafeCast.sol";
 
+import "../lib/VaultReentrancyLib.sol";
 import "../RecoveryMode.sol";
 
 /**
@@ -134,6 +135,8 @@ abstract contract ProtocolFeeCache is IProtocolFeeCache, RecoveryMode {
 
     /// @inheritdoc IProtocolFeeCache
     function updateProtocolFeePercentageCache() external override {
+        VaultReentrancyLib.ensureNotInVaultContext(_getVault());
+
         _beforeProtocolFeeCacheUpdate();
 
         _updateProtocolFeeCache(_protocolFeeProvider, _feeIds);

pkg/pool-weighted/contracts/managed/ManagedPoolSettings.sol

@@ -27,6 +27,7 @@ import "@balancer-labs/v2-pool-utils/contracts/lib/PoolRegistrationLib.sol";
 import "@balancer-labs/v2-pool-utils/contracts/external-fees/InvariantGrowthProtocolSwapFees.sol";
 import "@balancer-labs/v2-pool-utils/contracts/external-fees/ProtocolFeeCache.sol";
 import "@balancer-labs/v2-pool-utils/contracts/external-fees/ExternalAUMFees.sol";
+import "@balancer-labs/v2-pool-utils/contracts/lib/VaultReentrancyLib.sol";
 import "@balancer-labs/v2-pool-utils/contracts/NewBasePool.sol";
 
 import "../lib/GradualValueChange.sol";
@@ -101,6 +102,27 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
         uint256 aumFeeId;
     }
 
+    /**
+     * @dev Ensure we are not in a Vault context when this function is called, by attempting a no-op internal
+     * balance operation. If we are already in a Vault transaction (e.g., a swap, join, or exit), the Vault's
+     * reentrancy protection will cause this function to revert.
+     *
+     * Use this modifier with any function that can cause a state change in a pool and is either public itself,
+     * or called by a public function *outside* a Vault operation (e.g., join, exit, or swap).
+     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
+     */
+    modifier whenNotInVaultContext() {
+        _ensureNotInVaultContext();
+        _;
+    }
+
+    /**
+     * @dev Reverts if called in the middle of a Vault operation; has no effect otherwise.
+     */
+    function _ensureNotInVaultContext() private {
+        VaultReentrancyLib.ensureNotInVaultContext(getVault());
+    }
+
     constructor(ManagedPoolSettingsParams memory params, IProtocolFeePercentagesProvider protocolFeeProvider)
         ProtocolFeeCache(
             protocolFeeProvider,
@@ -173,6 +195,7 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
 
     // Actual Supply
 
+    /// @inheritdoc IManagedPool
     function getActualSupply() external view override returns (uint256) {
         return _getActualSupply(_getVirtualSupply());
     }
@@ -199,6 +222,7 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
         return ManagedPoolStorageLib.getSwapFeePercentage(_poolState);
     }
 
+    /// @inheritdoc IManagedPool
     function getGradualSwapFeeUpdateParams()
         external
         view
@@ -213,6 +237,7 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
         return ManagedPoolStorageLib.getSwapFeeFields(_poolState);
     }
 
+    /// @inheritdoc IManagedPool
     function updateSwapFeeGradually(
         uint256 startTime,
         uint256 endTime,
@@ -278,6 +303,7 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
         }
     }
 
+    /// @inheritdoc IManagedPool
     function getNormalizedWeights() external view override returns (uint256[] memory) {
         (IERC20[] memory tokens, ) = _getPoolTokens();
         return _getNormalizedWeights(tokens);
@@ -294,6 +320,7 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
             );
     }
 
+    /// @inheritdoc IManagedPool
     function getGradualWeightUpdateParams()
         external
         view
@@ -319,6 +346,7 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
         }
     }
 
+    /// @inheritdoc IManagedPool
     function updateWeightsGradually(
         uint256 startTime,
         uint256 endTime,
@@ -378,10 +406,12 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
 
     // Join / Exit Enabled
 
+    /// @inheritdoc IManagedPool
     function getJoinExitEnabled() external view override returns (bool) {
         return ManagedPoolStorageLib.getJoinExitEnabled(_poolState);
     }
 
+    /// @inheritdoc IManagedPool
     function setJoinExitEnabled(bool joinExitEnabled) external override authenticate whenNotPaused {
         _setJoinExitEnabled(joinExitEnabled);
     }
@@ -394,10 +424,12 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
 
     // Swap Enabled
 
+    /// @inheritdoc IManagedPool
     function getSwapEnabled() external view override returns (bool) {
         return ManagedPoolStorageLib.getSwapEnabled(_poolState);
     }
 
+    /// @inheritdoc IManagedPool
     function setSwapEnabled(bool swapEnabled) external override authenticate whenNotPaused {
         _setSwapEnabled(swapEnabled);
     }
@@ -410,17 +442,12 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
 
     // LP Allowlist
 
+    /// @inheritdoc IManagedPool
     function getMustAllowlistLPs() external view override returns (bool) {
         return ManagedPoolStorageLib.getLPAllowlistEnabled(_poolState);
     }
 
-    /**
-     * @notice Check whether an LP address is on the allowlist.
-     * @dev This simply checks the list, regardless of whether the allowlist feature is enabled, so that the allowlist
-     * can be inspected at any time.
-     * @param member - The address to check against the allowlist.
-     * @return true if the given address is on the allowlist.
-     */
+    /// @inheritdoc IManagedPool
     function isAddressOnAllowlist(address member) public view override returns (bool) {
         return _allowedAddresses[member];
     }
@@ -436,20 +463,23 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
         return !ManagedPoolStorageLib.getLPAllowlistEnabled(poolState) || isAddressOnAllowlist(member);
     }
 
+    /// @inheritdoc IManagedPool
     function addAllowedAddress(address member) external override authenticate whenNotPaused {
         _require(!isAddressOnAllowlist(member), Errors.ADDRESS_ALREADY_ALLOWLISTED);
 
         _allowedAddresses[member] = true;
         emit AllowlistAddressAdded(member);
     }
 
+    /// @inheritdoc IManagedPool
     function removeAllowedAddress(address member) external override authenticate whenNotPaused {
         _require(isAddressOnAllowlist(member), Errors.ADDRESS_NOT_ALLOWLISTED);
 
         delete _allowedAddresses[member];
         emit AllowlistAddressRemoved(member);
     }
 
+    /// @inheritdoc IManagedPool
     function setMustAllowlistLPs(bool mustAllowlistLPs) external override authenticate whenNotPaused {
         _setMustAllowlistLPs(mustAllowlistLPs);
     }
@@ -462,6 +492,7 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
 
     // AUM management fees
 
+    /// @inheritdoc IManagedPool
     function getManagementAumFeeParams()
         public
         view
@@ -477,11 +508,13 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
         }
     }
 
+    /// @inheritdoc IManagedPool
     function setManagementAumFeePercentage(uint256 managementAumFeePercentage)
         external
         override
         authenticate
         whenNotPaused
+        whenNotInVaultContext
         returns (uint256 amount)
     {
         // We want to prevent the pool manager from retroactively increasing the amount of AUM fees payable.
@@ -513,7 +546,8 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
         _aumState = ManagedPoolAumStorageLib.setLastCollectionTimestamp(_aumState, block.timestamp);
     }
 
-    function collectAumManagementFees() external override whenNotPaused returns (uint256) {
+    /// @inheritdoc IManagedPool
+    function collectAumManagementFees() external override whenNotPaused whenNotInVaultContext returns (uint256) {
         // It only makes sense to collect AUM fees after the pool is initialized (as before then the AUM is zero).
         // We can query if the pool is initialized by checking for a nonzero total supply.
         // Reverting here prevents zero value AUM fee collections causing bogus events.
@@ -570,13 +604,14 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
 
     // Add/Remove tokens
 
+    /// @inheritdoc IManagedPool
     function addToken(
         IERC20 tokenToAdd,
         address assetManager,
         uint256 tokenToAddNormalizedWeight,
         uint256 mintAmount,
         address recipient
-    ) external override authenticate whenNotPaused {
+    ) external override authenticate whenNotPaused whenNotInVaultContext {
         {
             // This complex operation might mint BPT, altering the supply. For simplicity, we forbid adding tokens
             // before initialization (i.e. before BPT is first minted). We must also collect AUM fees every time the
@@ -626,11 +661,12 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
         emit TokenAdded(tokenToAdd, tokenToAddNormalizedWeight);
     }
 
+    /// @inheritdoc IManagedPool
     function removeToken(
         IERC20 tokenToRemove,
         uint256 burnAmount,
         address sender
-    ) external override authenticate whenNotPaused {
+    ) external override authenticate whenNotPaused whenNotInVaultContext {
         {
             // Add new scope to avoid stack too deep.
 
@@ -692,6 +728,7 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
 
     // Scaling Factors
 
+    /// @inheritdoc IBasePool
     function getScalingFactors() external view override returns (uint256[] memory) {
         (IERC20[] memory tokens, ) = _getPoolTokens();
         return _scalingFactors(tokens);
@@ -728,9 +765,7 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
 
     // Recovery Mode
 
-    /**
-     * @notice Returns whether the pool is in Recovery Mode.
-     */
+    /// @inheritdoc IRecoveryMode
     function inRecoveryMode() public view override returns (bool) {
         return ManagedPoolStorageLib.getRecoveryModeEnabled(_poolState);
     }
@@ -755,6 +790,7 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
 
     // Circuit Breakers
 
+    /// @inheritdoc IManagedPool
     function getCircuitBreakerState(IERC20 token)
         external
         view
@@ -788,6 +824,7 @@ abstract contract ManagedPoolSettings is NewBasePool, ProtocolFeeCache, IManaged
         upperBptPriceBound = _upscale(upperBptPriceBound, tokenScalingFactor);
     }
 
+    /// @inheritdoc IManagedPool
     function setCircuitBreakers(
         IERC20[] memory tokens,
         uint256[] memory bptPrices,