M-01-late Balancer Review: more robust withdraw from strategies

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

@@ -448,7 +448,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/_custom-deploys.js

@@ -32,7 +32,7 @@ const deployBalancerFrxEethRethWstEThStrategyMissConfigured = async () => {
   });
 
   const strategy = await ethers.getContract("BalancerComposablePoolStrategy");
-  log("Strategy deployed, calling initialize")
+  log("Strategy deployed, calling initialize");
   // prettier-ignore
   await strategy
     .connect(sTimelock)["initialize(address[],address[],address[])"](

contracts/test/fixture/_fixture.js

@@ -1034,12 +1034,8 @@ async function balancerFrxETHwstETHeETHFixture(
  * replace the byte code with the one that exposes internal functions
  */
 async function balancerRethWETHExposeFunctionFixture() {
-
   const fixture = await balancerREthFixture();
   await hotDeployOption(fixture, "balancerRethWETHExposeFunctionFixture");
-
-  const { balancerREthStrategy, josh } = fixture;
-
   return fixture;
 }
 
@@ -1056,7 +1052,31 @@ async function balancerSfrxETHRETHWstETHMissConfiguredStrategy() {
  */
 async function balancerSfrxETHRETHWstETHExposeFunctionFixture() {
   const fixture = await balancerFrxETHwstETHeETHFixture();
-  await hotDeployOption(fixture, "balancerSfrxETHRETHWstETHExposeFunctionFixture");
+  await hotDeployOption(
+    fixture,
+    "balancerSfrxETHRETHWstETHExposeFunctionFixture"
+  );
+  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 +2103,7 @@ module.exports = {
   balancerRethWETHExposeFunctionFixture,
   balancerSfrxETHRETHWstETHExposeFunctionFixture,
   balancerSfrxETHRETHWstETHMissConfiguredStrategy,
+  balancerSfrxETHRETHWstETHBrokenWithdrawalFixture,
   fluxStrategyFixture,
   buybackFixture,
   nodeSnapshot,

contracts/test/fixture/_hot-deploy.js

@@ -39,8 +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,
@@ -102,15 +106,16 @@ 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");
@@ -127,10 +132,13 @@ async function hotDeployOption(
       );
 
       // IMPORTANT: remove once rETH/WETH is redeployed with the new code base
-      await fixture.balancerREthStrategy.connect(fixture.josh).cachePoolAssets();
+      await fixture.balancerREthStrategy
+        .connect(fixture.josh)
+        .cachePoolAssets();
       // IMPORTANT also remove this one
-      await fixture.balancerREthStrategy.connect(fixture.josh).cacheRateProviders();
-
+      await fixture.balancerREthStrategy
+        .connect(fixture.josh)
+        .cacheRateProviders();
     } else if (fixtureName === "morphoCompoundFixture") {
       await hotDeployFixture(
         fixture, // fixture
@@ -155,19 +163,48 @@ async function hotDeployOption(
         "balancerREthStrategy", // fixtureStrategyVarName
         "BalancerMetaPoolTestStrategy" // implContractName
       );
-    } else if (fixtureName === "balancerRethWETHExposeFunctionFixture") {
-      await hotDeployFixture(
-        fixture, // fixture
-        "balancerREthStrategy", // fixtureStrategyVarName
-        "BalancerMetaPoolTestStrategy" // implContractName
-      );
-    } else if (fixtureName === "balancerSfrxETHRETHWstETHExposeFunctionFixture") {
+    } else if (
+      fixtureName === "balancerSfrxETHRETHWstETHBrokenWithdrawalFixture"
+    ) {
       await hotDeployFixture(
         fixture, // fixture
         "balancerSfrxWstRETHStrategy", // fixtureStrategyVarName
-        "BalancerComposablePoolTestStrategy" // implContractName
+        "BalancerComposablePoolBrokenTestStrategy" // implContractName
       );
-    } else if (fixtureName === "deployBalancerFrxEethRethWstEThStrategyMissConfigured") {
+      /*
+       * 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 (
+      [
+        "balancerSfrxETHRETHWstETHExposeFunctionFixture",
+        "deployBalancerFrxEethRethWstEThStrategyMissConfigured",
+      ].includes(fixtureName)
+    ) {
       await hotDeployFixture(
         fixture, // fixture
         "balancerSfrxWstRETHStrategy", // fixtureStrategyVarName

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");
@@ -289,13 +290,18 @@ describe("ForkTest: Balancer ComposableStablePool sfrxETH/wstETH/rETH Strategy",
         "checkBalance()"
       ]();
 
-      expect(rethInVaultBefore.sub(rethInVaultAfter)).to.approxEqualTolerance(rethUnits, 0.5);
+      expect(rethInVaultBefore.sub(rethInVaultAfter)).to.approxEqualTolerance(
+        rethUnits,
+        0.5
+      );
       // stETH has rounding issues
       expect(stETHInVaultBefore.sub(stETHInVaultAfter)).to.approxEqualTolerance(
         stethUnits,
         0.01
       );
-      expect(frxETHInVaultBefore.sub(frxETHInVaultAfter)).to.approxEqualTolerance(frxethUnits, 0.5);
+      expect(
+        frxETHInVaultBefore.sub(frxETHInVaultAfter)
+      ).to.approxEqualTolerance(frxethUnits, 0.5);
       expect(
         strategyValueAfter.sub(strategyValueBefore)
       ).to.approxEqualTolerance(
@@ -428,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/balancerPoolReentrancy.fork-test.js

@@ -10,7 +10,7 @@ const { deployWithConfirmation } = require("../../utils/deploy");
 const addresses = require("../../utils/addresses");
 const { setERC20TokenBalance } = require("../_fund");
 
-describe.only("ForkTest: Balancer MetaStablePool - Read-only Reentrancy", function () {
+describe("ForkTest: Balancer MetaStablePool - Read-only Reentrancy", function () {
   this.timeout(0);
   // Retry up to 3 times on CI
   this.retries(isCI ? 3 : 0);