M-01-late Balancer Review: more robust withdraw from strategies (#1928)

* make VaultAdmin not fail when withdrawing from all of the strategis  when withdrawAll from a strategy fails

* add a separate withdrawal function

* remove only

* fix fork tests

contracts/contracts/interfaces/IVault.sol

@@ -124,6 +124,8 @@ interface IVault {
 
     function withdrawAllFromStrategies() external;
 
+    function faultTolerantWithdrawAllFromStrategies() external;
+
     function withdrawFromStrategy(
         address _strategyFromAddress,
         address[] calldata _assets,

contracts/contracts/mocks/BalancerComposablePoolBrokenTestStrategy.sol

@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+/**
+ * @title OETH Balancer BalancerComposablePoolTest Strategy
+ * This one is required to expose internal functions to our test suite. In fixtures
+ * we replace the mainnet byte code with the one from this contract.
+ *
+ * @author Origin Protocol Inc
+ */
+import { BalancerComposablePoolStrategy } from "../strategies/balancer/BalancerComposablePoolStrategy.sol";
+
+contract BalancerComposablePoolBrokenTestStrategy is
+    BalancerComposablePoolStrategy
+{
+    constructor(
+        BaseStrategyConfig memory _stratConfig,
+        BaseBalancerConfig memory _balancerConfig,
+        address _auraRewardPoolAddress,
+        uint256 _bptTokenPoolPosition
+    )
+        BalancerComposablePoolStrategy(
+            _stratConfig,
+            _balancerConfig,
+            _auraRewardPoolAddress,
+            _bptTokenPoolPosition
+        )
+    {}
+
+    function withdrawAll() external override onlyVaultOrGovernor nonReentrant {
+        require(false, "Simulating a failed call");
+    }
+}

contracts/contracts/strategies/balancer/BalancerMetaPoolStrategy.sol

@@ -160,7 +160,7 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
                 /* This check is triggered when the _deposit is called with
                  * a duplicate asset in the _strategyAssets array
                  *
-                 * A duplicate asset supplied with 0 amount or an amount close to 
+                 * A duplicate asset supplied with 0 amount or an amount close to
                  * 0 that wraps to a 0 amount will still pass this check.
                  */
                 require(
@@ -441,7 +441,13 @@ contract BalancerMetaPoolStrategy is BaseAuraStrategy {
      *
      * Is only executable by the OToken's Vault or the Governor.
      */
-    function withdrawAll() external override onlyVaultOrGovernor nonReentrant {
+    function withdrawAll()
+        external
+        virtual
+        override
+        onlyVaultOrGovernor
+        nonReentrant
+    {
         _withdrawAll(false);
     }
 

contracts/contracts/vault/VaultAdmin.sol

@@ -622,6 +622,22 @@ contract VaultAdmin is VaultStorage {
         }
     }
 
+    /**
+     * @notice Withdraws all assets from all the strategies and sends assets to the Vault.
+     * and doesn't revert event when `withdrawAll` of a strategy reverts.
+     */
+    function faultTolerantWithdrawAllFromStrategies()
+        external
+        onlyGovernorOrStrategist
+    {
+        uint256 stratCount = allStrategies.length;
+        for (uint256 i = 0; i < stratCount; ++i) {
+            try IStrategy(allStrategies[i]).withdrawAll() {} catch {
+                emit WithdrawFromStrategyFailed(allStrategies[i]);
+            }
+        }
+    }
+
     /***************************************
                     Utils
     ****************************************/

contracts/contracts/vault/VaultStorage.sol

@@ -52,6 +52,7 @@ contract VaultStorage is Initializable, Governable {
         uint256 _fromAssetAmount,
         uint256 _toAssetAmount
     );
+    event WithdrawFromStrategyFailed(address _strategyAddress);
 
     // Assets supported by the Vault, i.e. Stablecoins
     enum UnitConversion {

contracts/test/abi/IVault.json

@@ -991,6 +991,13 @@
       "stateMutability": "nonpayable",
       "type": "function"
     },
+    {
+      "inputs": [],
+      "name": "faultTolerantWithdrawAllFromStrategies",
+      "outputs": [],
+      "stateMutability": "nonpayable",
+      "type": "function"
+    },
     {
       "inputs": [
         {

contracts/test/fixture/_fixture.js

@@ -320,12 +320,12 @@ const defaultFixture = deployments.createFixture(async () => {
       fraxEthStrategyProxy.address
     );
 
-    const balancerRethStrategyProxy = await ethers.getContract(
+    const balancerREthStrategyProxy = await ethers.getContract(
       "OETHBalancerMetaPoolrEthStrategyProxy"
     );
     balancerREthStrategy = await ethers.getContractAt(
       "BalancerMetaPoolStrategy",
-      balancerRethStrategyProxy.address
+      balancerREthStrategyProxy.address
     );
 
     const balancerSfrxWstRETHStrategyProxy = await ethers.getContract(
@@ -905,6 +905,7 @@ async function convexVaultFixture() {
  */
 async function balancerREthFixture(config = { defaultStrategy: true }) {
   const fixture = await defaultFixture();
+
   await hotDeployOption(fixture, "balancerREthFixture", {
     isOethFixture: true,
   });
@@ -1035,8 +1036,9 @@ async function balancerFrxETHwstETHeETHFixture(
  */
 async function balancerRethWETHExposeFunctionFixture() {
   const fixture = await balancerREthFixture();
-  await hotDeployOption(fixture, "balancerRethWETHExposeFunctionFixture");
-
+  await hotDeployOption(fixture, "balancerRethWETHExposeFunctionFixture", {
+    forceDeployStrategy: true,
+  });
   return fixture;
 }
 
@@ -1055,7 +1057,30 @@ async function balancerSfrxETHRETHWstETHExposeFunctionFixture() {
   const fixture = await balancerFrxETHwstETHeETHFixture();
   await hotDeployOption(
     fixture,
-    "balancerSfrxETHRETHWstETHExposeFunctionFixture"
+    "balancerSfrxETHRETHWstETHExposeFunctionFixture",
+    {
+      forceDeployStrategy: true,
+    }
+  );
+  return fixture;
+}
+
+/**
+ * Configure a Vault with the Balancer strategy for frxEth/Reth/wstEth pool and
+ * replace the byte code with the one that fails on a withdrawAll call
+ */
+async function balancerSfrxETHRETHWstETHBrokenWithdrawalFixture() {
+  const fixture = await balancerFrxETHwstETHeETHFixture();
+  await hotDeployOption(
+    fixture,
+    "balancerSfrxETHRETHWstETHBrokenWithdrawalFixture",
+    {
+      isOethFixture: true,
+      /* force deploy strategy (+ vaultAdmin in this case) for test suite
+       * to be able to run a VaultAdmin test that hasn't been deployed yet.
+       */
+      forceDeployStrategy: true,
+    }
   );
   return fixture;
 }
@@ -2083,6 +2108,7 @@ module.exports = {
   balancerRethWETHExposeFunctionFixture,
   balancerSfrxETHRETHWstETHExposeFunctionFixture,
   balancerSfrxETHRETHWstETHMissConfiguredStrategy,
+  balancerSfrxETHRETHWstETHBrokenWithdrawalFixture,
   fluxStrategyFixture,
   buybackFixture,
   nodeSnapshot,

contracts/test/fixture/_hot-deploy.js

@@ -39,7 +39,12 @@ async function constructNewContract(fixture, implContractName) {
         ],
         addresses.mainnet.rETH_WETH_AuraRewards, // Address of the Aura rewards contract
       ];
-    } else if (implContractName === "BalancerComposablePoolTestStrategy") {
+    } else if (
+      [
+        "BalancerComposablePoolBrokenTestStrategy",
+        "BalancerComposablePoolTestStrategy",
+      ].includes(implContractName)
+    ) {
       return [
         [
           addresses.mainnet.wstETH_sfrxETH_rETH_BPT,
@@ -101,22 +106,44 @@ async function constructNewContract(fixture, implContractName) {
 async function hotDeployOption(
   fixture,
   fixtureName,
-  config = { isOethFixture: false }
+  config = { isOethFixture: false, forceDeployStrategy: false }
 ) {
   if (!isFork) return;
 
   const hotDeployOptions = (process.env.HOT_DEPLOY || "")
     .split(",")
     .map((item) => item.trim());
-  const { isOethFixture } = config;
-  const deployStrat = hotDeployOptions.includes("strategy");
+  const { isOethFixture, forceDeployStrategy } = config;
+  const deployStrat =
+    hotDeployOptions.includes("strategy") || forceDeployStrategy;
   const deployVaultCore = hotDeployOptions.includes("vaultCore");
   const deployVaultAdmin = hotDeployOptions.includes("vaultAdmin");
   const deployHarvester = hotDeployOptions.includes("harvester");
 
-  log(`Running fixture hot deployment w/ config; isOethFixture:${isOethFixture} strategy:${!!deployStrat} 
+  log(`Running fixture [${fixtureName}] hot deployment w/ config; isOethFixture:${isOethFixture} strategy:${!!deployStrat} 
     vaultCore:${!!deployVaultCore} vaultAdmin:${!!deployVaultAdmin} harvester:${!!deployHarvester}`);
 
+  const cacheAssetsAndProviders = async (strategyVarName) => {
+    const proxy = await ethers.getContractAt(
+      "InitializeGovernedUpgradeabilityProxy",
+      fixture[strategyVarName].address
+    );
+
+    const implAddress = await proxy.implementation();
+    log(`Current impl address: ${implAddress}`);
+    // new version with cached rate providers / assets has not been deployed yet
+    if (implAddress === "0xAaA1d497fdff9a88048743Db31d3173a2E442A3D") {
+      log(`Caching rate providers and pool assets`);
+      await fixture[strategyVarName].connect(fixture.josh).cachePoolAssets();
+
+      await fixture[strategyVarName].connect(fixture.josh).cacheRateProviders();
+    } else {
+      throw new Error(
+        `New ${strategyVarName} deployed, removed manual caching!`
+      );
+    }
+  };
+
   if (deployStrat) {
     if (fixtureName === "balancerREthFixture") {
       await hotDeployFixture(
@@ -125,14 +152,7 @@ async function hotDeployOption(
         "BalancerMetaPoolStrategy" // implContractName
       );
 
-      // IMPORTANT: remove once rETH/WETH is redeployed with the new code base
-      await fixture.balancerREthStrategy
-        .connect(fixture.josh)
-        .cachePoolAssets();
-      // IMPORTANT also remove this one
-      await fixture.balancerREthStrategy
-        .connect(fixture.josh)
-        .cacheRateProviders();
+      await cacheAssetsAndProviders("balancerREthStrategy");
     } else if (fixtureName === "morphoCompoundFixture") {
       await hotDeployFixture(
         fixture, // fixture
@@ -157,16 +177,50 @@ async function hotDeployOption(
         "balancerREthStrategy", // fixtureStrategyVarName
         "BalancerMetaPoolTestStrategy" // implContractName
       );
+
+      await cacheAssetsAndProviders("balancerREthStrategy");
     } else if (
-      fixtureName === "balancerSfrxETHRETHWstETHExposeFunctionFixture"
+      fixtureName === "balancerSfrxETHRETHWstETHBrokenWithdrawalFixture"
     ) {
       await hotDeployFixture(
         fixture, // fixture
         "balancerSfrxWstRETHStrategy", // fixtureStrategyVarName
-        "BalancerComposablePoolTestStrategy" // implContractName
+        "BalancerComposablePoolBrokenTestStrategy" // implContractName
+      );
+
+      /*
+       * Delete this piece of code once the new VaultAdmin implementation is deployed.
+       */
+      const oethVaultAdminImplAddress =
+        "0x" +
+        (
+          await ethers.provider.send("eth_getStorageAt", [
+            fixture.oethVault.address,
+            "0xa2bd3d3cf188a41358c8b401076eb59066b09dec5775650c0de4c55187d17bd9", // Vault admin implementation position
+            "latest", // block
+          ])
+        ).substring(26);
+
+      if (
+        oethVaultAdminImplAddress !==
+        "0x31a91336414d3b955e494e7d485a6b06b55fc8fb"
+      ) {
+        throw Error(
+          "OETHVaultAdmin has been re-deployed. Hot-deploy shouldn't be re-deploying it anymore."
+        );
+      }
+
+      await hotDeployVaultAdmin(
+        fixture,
+        true, // deploy VaultAdmin
+        false, // deploy VaultCore
+        true // isOethFixture
       );
     } else if (
-      fixtureName === "deployBalancerFrxEethRethWstEThStrategyMissConfigured"
+      [
+        "balancerSfrxETHRETHWstETHExposeFunctionFixture",
+        "deployBalancerFrxEethRethWstEThStrategyMissConfigured",
+      ].includes(fixtureName)
     ) {
       await hotDeployFixture(
         fixture, // fixture

contracts/test/strategies/BalancerComposableStablePool.fork-test.js

@@ -12,6 +12,7 @@ const {
   createFixtureLoader,
   balancerSfrxETHRETHWstETHExposeFunctionFixture,
   balancerSfrxETHRETHWstETHMissConfiguredStrategy,
+  balancerSfrxETHRETHWstETHBrokenWithdrawalFixture,
 } = require("../fixture/_fixture");
 
 const { tiltPool, unTiltPool } = require("../fixture/_pool_tilt");
@@ -433,6 +434,37 @@ describe("ForkTest: Balancer ComposableStablePool sfrxETH/wstETH/rETH Strategy",
       });
     }
 
+    it("Should be able to call faultTolerantWithdrawAllFromStrategies even when one errors out", async function () {
+      const fixture = await balancerSfrxETHRETHWstETHBrokenWithdrawalFixture();
+      const { timelock, oethVault, balancerSfrxWstRETHStrategy } = fixture;
+      const strategyAddress = balancerSfrxWstRETHStrategy.address.toLowerCase();
+
+      // Withdraw all from strategies
+      const tx = await oethVault
+        .connect(timelock)
+        .faultTolerantWithdrawAllFromStrategies();
+      const events = (await tx.wait()).events || [];
+      /*
+       * If OethVaultAdmin has triggered the "WithdrawFromStrategyFailed" with the
+       * balancerSfrxWstRETHStrategy address the below variable will equal that address,
+       * otherwise it will be `undefined`
+       */
+      const balancerStrategyAddressOption = events
+        // 0xbf15... -> "WithdrawFromStrategyFailed" event
+        .filter(
+          (e) =>
+            e.topics[0] ===
+            "0xbf15aee350c3b4fb830dccbe5255de03252ad7eea9c1e5e721c2280704ef5b37"
+        )
+        .map((e) => "0x" + e.data.substring(26).toLowerCase())
+        .find((evtStrategyAddress) => evtStrategyAddress === strategyAddress);
+
+      expect(balancerStrategyAddressOption).to.not.be.undefined;
+
+      await expect(oethVault.connect(timelock).withdrawAllFromStrategies()).to
+        .be.reverted;
+    });
+
     it("Should be able to withdraw all of pool liquidity", async function () {
       const { oethVault, stETH, frxETH, reth, balancerSfrxWstRETHStrategy } =
         fixture;

contracts/test/strategies/balancerMetaStablePool.fork-test.js

@@ -534,6 +534,11 @@ describe("ForkTest: Balancer MetaStablePool rETH/WETH Strategy", function () {
     });
 
     it(`Should fail when duplicating an asset in withdrawal call`, async function () {
+      /* using expose function so that the code is force-deployed - unit needed,
+       * that is until we deploy updated implementation to the mainnet
+       */
+      fixture = await balancerRethWETHExposeFunctionFixture();
+
       const { balancerREthStrategy, oethVault, weth } = fixture;
 
       const oethVaultSigner = await impersonateAndFund(oethVault.address);