Stable Pool: ongoing reentrancy protection

pkg/interfaces/contracts/pool-stable/IComposableStablePoolRates.sol

@@ -20,12 +20,35 @@ interface IComposableStablePoolRates {
     /**
      * @dev Forces a rate cache hit for a token.
      * It will revert if the requested token does not have an associated rate provider.
+     *
+     * This function will revert when called within a Vault context (i.e. in the middle of a join or an exit).
+     *
+     * This function depends on `getRate` via the rate provider, 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. It is protected
+     * by a call to `VaultReentrancyLib.ensureNotInVaultContext` where overridden in `ComposableStablePoolRates`, and so
+     * is safe to call on ComposableStablePool.
+     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
+     *
+     * It will also revert if there was no rate provider set initially.
+     *
+     * @param token The token whose rate cache will be updated.
      */
     function updateTokenRateCache(IERC20 token) external;
 
     /**
-     * @dev Sets a new duration for a token rate cache. It reverts if there was no rate provider set initially.
+     * @dev Sets a new duration for a token rate cache.
      * Note this function also updates the current cached value.
+     *
+     * This function will revert when called within a Vault context (i.e. in the middle of a join or an exit).
+     *
+     * This function depends on `getRate` via the rate provider, 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. It is protected
+     * by a call to `VaultReentrancyLib.ensureNotInVaultContext` where overridden in `ComposableStablePoolRates`, and so
+     * is safe to call on ComposableStablePool.
+     * See https://forum.balancer.fi/t/reentrancy-vulnerability-scope-expanded/4345 for reference.
+     *
+     * It will also revert if there was no rate provider set initially.
+     *
      * @param duration Number of seconds until the current token rate is fetched again.
      */
     function setTokenRateCacheDuration(IERC20 token, uint256 duration) external;

pkg/pool-stable/contracts/ComposableStablePool.sol

@@ -1034,9 +1034,19 @@ contract ComposableStablePool is
      * the token rates increase). Therefore, the rate is a monotonically increasing function.
      *
      * WARNING: since this function reads balances directly from the Vault, it is potentially subject to manipulation
-     * via reentrancy. However, this can only happen if one of the tokens in the Pool contains some form of callback
-     * behavior in the `transferFrom` function (like ERC777 tokens do). These tokens are strictly incompatible with the
+     * via reentrancy if called within a Vault context (i.e. in the middle of a join or an exit). It is up to the
+     * caller to ensure that the function is safe to call.
+     *
+     * This may happen e.g. if one of the tokens in the Pool contains some form of callback behavior in the
+     * `transferFrom` function (like ERC777 tokens do). These tokens are strictly incompatible with the
      * Vault and Pool design, and are not safe to be used.
+     *
+     * There are also other situations where calling this function is unsafe. 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 `getRate`. That will make the transaction revert in an unsafe context.
+     * (See `VaultReentrancyLib.ensureNotInVaultContext` in pool-utils.)
      */
     function getRate() external view virtual override returns (uint256) {
         // We need to compute the current invariant and actual total supply. The latter includes protocol fees that have
@@ -1083,6 +1093,18 @@ contract ComposableStablePool is
      *    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() external view returns (uint256) {
         (, uint256 virtualSupply, uint256 protocolFeeAmount, , ) = _getSupplyAndFeesData();

pkg/pool-stable/contracts/ComposableStablePoolRates.sol

@@ -21,6 +21,7 @@ import "@balancer-labs/v2-interfaces/contracts/pool-utils/IRateProvider.sol";
 import "@balancer-labs/v2-solidity-utils/contracts/helpers/ERC20Helpers.sol";
 import "@balancer-labs/v2-solidity-utils/contracts/helpers/InputHelpers.sol";
 import "@balancer-labs/v2-pool-utils/contracts/rates/PriceRateCache.sol";
+import "@balancer-labs/v2-pool-utils/contracts/lib/VaultReentrancyLib.sol";
 
 import "./ComposableStablePoolStorage.sol";
 
@@ -50,6 +51,19 @@ abstract contract ComposableStablePoolRates is IComposableStablePoolRates, Compo
     event TokenRateCacheUpdated(uint256 indexed tokenIndex, uint256 rate);
     event TokenRateProviderSet(uint256 indexed tokenIndex, IRateProvider indexed provider, uint256 cacheDuration);
 
+    /**
+     * @dev Ensure we are not in a Vault context when this function is called, by calling a library function that
+     * reverts in that case.
+     *
+     * 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() {
+        VaultReentrancyLib.ensureNotInVaultContext(_getVault());
+        _;
+    }
+
     constructor(RatesParams memory rateParams) {
         InputHelpers.ensureInputLengthMatch(
             rateParams.tokens.length,
@@ -140,7 +154,12 @@ abstract contract ComposableStablePoolRates is IComposableStablePoolRates, Compo
     }
 
     /// @inheritdoc IComposableStablePoolRates
-    function setTokenRateCacheDuration(IERC20 token, uint256 duration) external override authenticate {
+    function setTokenRateCacheDuration(IERC20 token, uint256 duration)
+        external
+        override
+        whenNotInVaultContext
+        authenticate
+    {
         uint256 index = _getTokenIndex(token);
         IRateProvider provider = _getRateProvider(index);
         _require(address(provider) != address(0), Errors.TOKEN_DOES_NOT_HAVE_RATE_PROVIDER);
@@ -149,7 +168,7 @@ abstract contract ComposableStablePoolRates is IComposableStablePoolRates, Compo
     }
 
     /// @inheritdoc IComposableStablePoolRates
-    function updateTokenRateCache(IERC20 token) external override {
+    function updateTokenRateCache(IERC20 token) external override whenNotInVaultContext {
         uint256 index = _getTokenIndex(token);
 
         IRateProvider provider = _getRateProvider(index);

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";