Weighted Pool: ongoing reentrancy protection

pkg/pool-stable/contracts/test/MockComposableStablePoolRates.sol

@@ -13,6 +13,7 @@
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 pragma solidity ^0.7.0;
+pragma experimental ABIEncoderV2;
 
 import "@balancer-labs/v2-solidity-utils/contracts/helpers/ERC20Helpers.sol";
 

pkg/pool-utils/contracts/RecoveryMode.sol

@@ -22,6 +22,7 @@ import "@balancer-labs/v2-interfaces/contracts/vault/IVault.sol";
 import "@balancer-labs/v2-solidity-utils/contracts/math/FixedPoint.sol";
 
 import "./BasePoolAuthorization.sol";
+import "./lib/VaultReentrancyLib.sol";
 
 /**
  * @notice Handle storage and state changes for pools that support "Recovery Mode".
@@ -85,13 +86,24 @@ abstract contract RecoveryMode is IRecoveryMode, BasePoolAuthorization {
      * @notice Disable recovery mode, which disables the special safe exit path for LPs.
      * @dev Protocol fees are not paid while in Recovery Mode, so it should only remain active for as long as strictly
      * necessary.
+     *
+     * This function will revert when called within a Vault context (i.e. in the middle of a join or an exit).
+     *
+     * Disabling recovery mode in derived contracts often triggers code that depends on the invariant or total supply,
+     * which may be calculated incorrectly in the middle of a join or an exit, because the state of the pool could be
+     * out of sync with the state of the Vault. The call to `ensureNotInVaultContext` in `VaultReentrancyLib` ensures
+     * this is safe to call.
+     *
+     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
      */
     function disableRecoveryMode() external override authenticate {
         // Some derived contracts respond to disabling recovery mode with state changes (e.g., related to protocol fees,
         // or otherwise ensuring that enabling and disabling recovery mode has no ill effects on LPs). When called
         // outside of recovery mode, these state changes might lead to unexpected behavior.
         _ensureInRecoveryMode();
 
+        VaultReentrancyLib.ensureNotInVaultContext(_getVault());
+
         _setRecoveryMode(false);
 
         emit RecoveryModeStateChanged(false);

pkg/pool-weighted/contracts/BaseWeightedPool.sol

@@ -84,6 +84,17 @@ abstract contract BaseWeightedPool is BaseMinimalSwapInfoPool {
 
     /**
      * @dev Returns the current value of the invariant.
+     * **IMPORTANT NOTE**: calling this function within a Vault context (i.e. in the middle of a join or an exit) is
+     * potentially unsafe, since the returned value is manipulable. It is up to the caller to ensure safety.
+     *
+     * Calculating the invariant requires the state of the pool to be in sync with the state of the Vault.
+     * That condition may not be true in the middle of a join or an exit.
+     *
+     * To call this function safely, attempt to trigger the reentrancy guard in the Vault by calling a non-reentrant
+     * function before calling `getInvariant`. That will make the transaction revert in an unsafe context.
+     * (See `VaultReentrancyLib.ensureNotInVaultContext` in pool-utils.)
+     *
+     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
      */
     function getInvariant() public view returns (uint256) {
         (, uint256[] memory balances, ) = getVault().getPoolTokens(getPoolId());

pkg/pool-weighted/contracts/WeightedPool.sol

@@ -330,8 +330,20 @@ contract WeightedPool is BaseWeightedPool, WeightedPoolProtocolFees {
      * even if they don't yet exist, 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()`.
+     *
+     * **IMPORTANT NOTE**: calling this function within a Vault context (i.e. in the middle of a join or an exit) is
+     * potentially unsafe, since the returned value is manipulable. It is up to the caller to ensure safety.
+     *
+     * This is because this function calculates the invariant, which requires the state of the pool to be in sync
+     * with the state of the Vault. That condition may not be true in the middle of a join or an exit.
+     *
+     * 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 `VaultReentrancyLib.ensureNotInVaultContext` in pool-utils.)
+     *
+     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
      */
-    function getActualSupply() public view returns (uint256) {
+    function getActualSupply() external view returns (uint256) {
         uint256 supply = totalSupply();
 
         (uint256 protocolFeesToBeMinted, ) = _getPreJoinExitProtocolFees(

pkg/standalone-utils/contracts/test/MockRecoveryRateProviderPool.sol

@@ -13,6 +13,7 @@
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 pragma solidity ^0.7.0;
+pragma experimental ABIEncoderV2;
 
 import "@balancer-labs/v2-interfaces/contracts/pool-utils/IRateProviderPool.sol";
 import "@balancer-labs/v2-interfaces/contracts/vault/IVault.sol";