AP-680 Limit what vault config update can change (#374)

* Create new VaultConfigUpdate struct and switch to Ownable pattern for access control

* Fix dummy vault

* Adjusted tests to reflect new config structs

* Fixes to scripts/tasks according to new deployment pattern

* lint

contracts/core/vault/APVault_V1.sol

@@ -5,14 +5,16 @@ pragma solidity ^0.8.19;
 import {IVault} from "./interfaces/IVault.sol";
 import {IVaultEmitter} from "./interfaces/IVaultEmitter.sol";
 import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
+import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 import {ERC4626AP} from "./ERC4626AP.sol";
 import {IStrategy} from "../strategy/IStrategy.sol";
 import {IRegistrar} from "../registrar/interfaces/IRegistrar.sol";
 import {LocalRegistrarLib} from "../registrar/lib/LocalRegistrarLib.sol";
 import {LibAccounts} from "../accounts/lib/LibAccounts.sol";
 import {FixedPointMathLib} from "../../lib/FixedPointMathLib.sol";
+import {Validator} from "../validator.sol";
 
-contract APVault_V1 is IVault, ERC4626AP {
+contract APVault_V1 is IVault, ERC4626AP, Ownable {
   using FixedPointMathLib for uint256;
 
   address public immutable EMITTER_ADDRESS;
@@ -22,23 +24,18 @@ contract APVault_V1 is IVault, ERC4626AP {
 
   constructor(
     VaultConfig memory _config,
-    address _emitter
+    address _emitter,
+    address _admin
   ) ERC4626AP(IERC20Metadata(_config.yieldToken), _config.apTokenName, _config.apTokenSymbol) {
     EMITTER_ADDRESS = _emitter;
     vaultConfig = _config;
+    transferOwnership(_admin);
   }
 
   /*//////////////////////////////////////////////////////////////
                                 MODIFIERS
   //////////////////////////////////////////////////////////////*/
 
-  modifier onlyAdmin() {
-    if (!(_msgSender() == vaultConfig.admin)) {
-      revert OnlyAdmin();
-    }
-    _;
-  }
-
   modifier onlyApprovedRouter() {
     if (!_isApprovedRouter()) {
       revert OnlyRouter();
@@ -71,8 +68,9 @@ contract APVault_V1 is IVault, ERC4626AP {
                                 CONFIG
   //////////////////////////////////////////////////////////////*/
 
-  function setVaultConfig(VaultConfig memory _newConfig) external virtual override onlyAdmin {
-    vaultConfig = _newConfig;
+  function setVaultConfig(VaultConfigUpdate memory _newConfig) external virtual override onlyOwner {
+    vaultConfig.strategy = _newConfig.strategy;
+    vaultConfig.registrar = _newConfig.registrar;
     IVaultEmitter(EMITTER_ADDRESS).vaultConfigUpdated(address(this), _newConfig);
   }
 

contracts/core/vault/VaultEmitter.sol

@@ -58,7 +58,10 @@ contract VaultEmitter is IVaultEmitter, Initializable, OwnableUpgradeable {
   /// @notice emits the VaultConfigUpdated event
   /// @param vault Address of the Vault
   /// @param config New Vault config values
-  function vaultConfigUpdated(address vault, IVault.VaultConfig memory config) public isEmitter {
+  function vaultConfigUpdated(
+    address vault,
+    IVault.VaultConfigUpdate memory config
+  ) public isEmitter {
     emit VaultConfigUpdated(vault, config);
   }
 

contracts/core/vault/interfaces/IVault.sol

@@ -18,6 +18,15 @@ abstract contract IVault {
     LIQUID
   }
 
+  /// @notice A config struct is stored on each deployed Vault.
+  /// @param vaultType one of LOCKED | LIQUID
+  /// @param strategyId the unique identifier for the strategy
+  /// @param strategy the address of the strategy-specific impl.
+  /// @param registrar the address of the local registrar
+  /// @param baseToken the address of the expected base token
+  /// @param yieldToken the address of the expected yield token
+  /// @param apTokenName the name of the this vaults ERC20AP token
+  /// @param apTokenSymbol the symbol of this vaults ERC20AP token
   struct VaultConfig {
     VaultType vaultType;
     bytes4 strategyId;
@@ -27,7 +36,17 @@ abstract contract IVault {
     address yieldToken;
     string apTokenName;
     string apTokenSymbol;
-    address admin;
+  }
+
+  /// @notice The struct that can be passed for updating config fields
+  /// @dev Vault type, strategyId and token params are explicitly ommitted from being
+  /// @dev updateable since changing these could lead to the loss of funds
+  /// @dev or are immutables in parent contracts
+  /// @param strategy the new address of the strategy implementation
+  /// @param registrar the address of the new registrar
+  struct VaultConfigUpdate {
+    address strategy;
+    address registrar;
   }
 
   /// @notice Gerneric AP Vault action data
@@ -65,6 +84,10 @@ abstract contract IVault {
     FAIL_TOKENS_FALLBACK // Tokens failed to be returned to accounts contract
   }
 
+  /// @notice Structure returned upon redemption
+  /// @param token The address of the token being redeemed
+  /// @param amount The qty of tokens being redeemed
+  /// @param status The status of the redemption request's processing
   struct RedemptionResponse {
     address token;
     uint256 amount;
@@ -74,7 +97,6 @@ abstract contract IVault {
   /*////////////////////////////////////////////////
                         ERRORS
   */ ////////////////////////////////////////////////
-  error OnlyAdmin();
   error OnlyRouter();
   error OnlyApproved();
   error OnlyBaseToken();
@@ -90,7 +112,7 @@ abstract contract IVault {
   function getVaultConfig() external view virtual returns (VaultConfig memory);
 
   /// @notice set the vault config
-  function setVaultConfig(VaultConfig memory _newConfig) external virtual;
+  function setVaultConfig(VaultConfigUpdate memory _newConfig) external virtual;
 
   /// @notice deposit tokens into vault position of specified Account
   /// @dev the deposit method allows the Vault contract to create or add to an existing

contracts/core/vault/interfaces/IVaultEmitter.sol

@@ -27,7 +27,7 @@ interface IVaultEmitter {
   /// @notice Event emited on each Harvest call
   /// @param vault Address of the Vault
   /// @param config New Vault config values
-  event VaultConfigUpdated(address vault, IVault.VaultConfig config);
+  event VaultConfigUpdated(address vault, IVault.VaultConfigUpdate config);
 
   /// @notice Event emited on each Harvest call
   /// @param vault Address of the new Vault
@@ -51,7 +51,7 @@ interface IVaultEmitter {
   /// @notice emits the VaultConfigUpdated event
   /// @param vault Address of the Vault
   /// @param config New Vault config values
-  function vaultConfigUpdated(address vault, IVault.VaultConfig memory config) external;
+  function vaultConfigUpdated(address vault, IVault.VaultConfigUpdate memory config) external;
 
   /// @notice emits the VaultCreated event
   /// @param vault Address of the new Vault

contracts/test/DummyVault.sol

@@ -25,11 +25,11 @@ contract DummyVault is IVault {
   /// Vault impl
   constructor(VaultConfig memory _config) {
     vaultConfig = _config;
-    IVaultEmitter(emitterAddress).vaultConfigUpdated(address(this), _config);
   }
 
-  function setVaultConfig(VaultConfig memory _newConfig) external override {
-    vaultConfig = _newConfig;
+  function setVaultConfig(VaultConfigUpdate memory _newConfig) external override {
+    vaultConfig.strategy = _newConfig.strategy;
+    vaultConfig.registrar = _newConfig.registrar;
     IVaultEmitter(emitterAddress).vaultConfigUpdated(address(this), _newConfig);
   }
 

tasks/deploy/integrations/dummyIntegration.ts

@@ -66,9 +66,8 @@ task("Deploy:dummyIntegration", "Will deploy a set of vaults and a dummy strateg
         yieldToken: yieldToken.address,
         apTokenName: "LockedTestVault",
         apTokenSymbol: "LockTV",
-        admin: admin,
       };
-      let lockVault = await Vault.deploy(lockedConfig, addresses.vaultEmitter.proxy);
+      let lockVault = await Vault.deploy(lockedConfig, addresses.vaultEmitter.proxy, admin);
       logger.pad(30, "Locked Vault deployed to", lockVault.address);
 
       let liquidConfig = {
@@ -80,9 +79,8 @@ task("Deploy:dummyIntegration", "Will deploy a set of vaults and a dummy strateg
         yieldToken: yieldToken.address,
         apTokenName: "LiquidTestVault",
         apTokenSymbol: "LiqTV",
-        admin: admin,
       };
-      let liqVault = await Vault.deploy(liquidConfig, addresses.vaultEmitter.proxy);
+      let liqVault = await Vault.deploy(liquidConfig, addresses.vaultEmitter.proxy, admin);
       logger.pad(30, "Liquid Vault deployed to", liqVault.address);
 
       const data: StrategyObject = {

tasks/deploy/integrations/genericVault.ts

@@ -9,6 +9,7 @@ import {deploy, getAddresses, getSigners, logger, verify} from "utils";
 
 task("Deploy:genericVault", "Will deploy a generic vault with the provided params")
   .addOptionalParam("yieldtoken", "The address of the yield token")
+  .addOptionalParam("admin", "The address of the admin, will default to the deployer's address")
   .addFlag("skipVerify", "Skip contract verification")
   .setAction(async (taskArgs, hre) => {
     try {
@@ -43,10 +44,13 @@ task("Deploy:genericVault", "Will deploy a generic vault with the provided param
         yieldToken: yieldTokenAddress,
         apTokenName: "TestVault",
         apTokenSymbol: "TV",
-        admin: deployer.address,
       };
       // deploy
-      const deployment = await deploy(APVault_V1, [vaultConfig, addresses.vaultEmitter.proxy]);
+      const deployment = await deploy(APVault_V1, [
+        vaultConfig,
+        addresses.vaultEmitter.proxy,
+        deployer.address,
+      ]);
 
       logger.out("Emitting `VaultCreated` event...");
       const vaultEmitter = IVaultEmitter__factory.connect(addresses.vaultEmitter.proxy, deployer);

tasks/helpers/submitMultiSigTx.ts

@@ -2,7 +2,7 @@ import {SignerWithAddress} from "@nomiclabs/hardhat-ethers/signers";
 import {BytesLike} from "ethers";
 import {IMultiSigGeneric__factory} from "typechain-types";
 import {filterEvents, logger} from "utils";
-import { Wallet } from "ethers";
+import {Wallet} from "ethers";
 
 /**
  * Submits a transaction to the designated Multisig contract and executes it if possible.
@@ -20,7 +20,9 @@ export async function submitMultiSigTx(
 ): Promise<boolean> {
   logger.out(`Submitting transaction to Multisig at address: ${msAddress}...`);
   const multisig = IMultiSigGeneric__factory.connect(msAddress, owner);
-  const tx = await multisig.submitTransaction(destination, 0, data, "0x", {gasPrice: 120*10**9});
+  const tx = await multisig.submitTransaction(destination, 0, data, "0x", {
+    gasPrice: 120 * 10 ** 9,
+  });
   logger.out(`Tx hash: ${tx.hash}`);
   const receipt = await tx.wait();
 

test/core/vault/Vault.ts

@@ -75,9 +75,8 @@ describe("Vault", function () {
       yieldToken: yieldToken,
       apTokenName: apTokenName,
       apTokenSymbol: apTokenSymbol,
-      admin: admin,
     };
-    const vault = await Vault.deploy(vaultInitConfig, vaultEmitter);
+    const vault = await Vault.deploy(vaultInitConfig, vaultEmitter, admin);
     await vault.deployed();
     return vault;
   }
@@ -149,6 +148,7 @@ describe("Vault", function () {
     });
     it("should set the config as specified on deployment", async function () {
       let config = await vault.getVaultConfig();
+      let admin = await vault.owner();
       expect(config.vaultType).to.equal(0);
       expect(config.strategyId).to.equal(DEFAULT_STRATEGY_ID);
       expect(config.strategy).to.equal(ethers.constants.AddressZero);
@@ -157,48 +157,24 @@ describe("Vault", function () {
       expect(config.yieldToken).to.equal(token.address);
       expect(config.apTokenName).to.equal(DEFAULT_VAULT_NAME);
       expect(config.apTokenSymbol).to.equal(DEFAULT_VAULT_SYMBOL);
-      expect(config.admin).to.equal(owner.address);
+      expect(admin).to.equal(owner.address);
     });
     it("should accept new config values", async function () {
       let newConfig = {
-        vaultType: 1,
-        strategyId: "0x87654321",
         strategy: user.address,
         registrar: user.address,
-        baseToken: token.address,
-        yieldToken: token.address,
-        apTokenName: "NewName",
-        apTokenSymbol: "NN",
-        admin: user.address,
       } as IVault.VaultConfigStruct;
       await vault.setVaultConfig(newConfig);
       let queriedConfig = await vault.getVaultConfig();
-      expect(queriedConfig.vaultType).to.equal(newConfig.vaultType);
-      expect(queriedConfig.strategyId).to.equal(newConfig.strategyId);
       expect(queriedConfig.strategy).to.equal(newConfig.strategy);
       expect(queriedConfig.registrar).to.equal(newConfig.registrar);
-      expect(queriedConfig.baseToken).to.equal(newConfig.baseToken);
-      expect(queriedConfig.yieldToken).to.equal(newConfig.yieldToken);
-      expect(queriedConfig.apTokenName).to.equal(newConfig.apTokenName);
-      expect(queriedConfig.apTokenSymbol).to.equal(newConfig.apTokenSymbol);
-      expect(queriedConfig.admin).to.equal(newConfig.admin);
     });
     it("should revert when a non-admin calls the set method", async function () {
       let newConfig = {
-        vaultType: 1,
-        strategyId: "0x87654321",
         strategy: user.address,
         registrar: user.address,
-        baseToken: token.address,
-        yieldToken: token.address,
-        apTokenName: "NewName",
-        apTokenSymbol: "NN",
-        admin: user.address,
       } as IVault.VaultConfigStruct;
-      await expect(vault.connect(user).setVaultConfig(newConfig)).to.be.revertedWithCustomError(
-        vault,
-        "OnlyAdmin"
-      );
+      await expect(vault.connect(user).setVaultConfig(newConfig)).to.be.reverted;
     });
   });
 

test/utils/dummyVault.ts

@@ -36,7 +36,6 @@ export async function deployDummyVault(
     yieldToken: yieldToken,
     apTokenName: apTokenName,
     apTokenSymbol: apTokenSymbol,
-    admin: deployer.address,
   };
   const vault = await Vault.deploy(vaultInitConfig);
   await vault.deployed();

utils/deploy.ts

@@ -23,7 +23,7 @@ export async function deploy<T extends ContractFactory>(
   try {
     const contract = await factory.deploy(...(constructorArguments ?? []));
     await contract.deployed();
-    await delay(1000)
+    await delay(1000);
     logger.out(`Address: ${contract.address}`);
     return {
       constructorArguments,
@@ -64,5 +64,5 @@ export async function deployBehindProxy<T extends ContractFactory>(
 }
 
 function delay(ms: number) {
-  return new Promise( resolve => setTimeout(resolve, ms) );
+  return new Promise((resolve) => setTimeout(resolve, ms));
 }

utils/signers/getAPTeamOwner.ts

@@ -2,12 +2,12 @@ import {SignerWithAddress} from "@nomiclabs/hardhat-ethers/signers";
 import {HardhatRuntimeEnvironment} from "hardhat/types";
 import {connectSignerFromPkey} from "./connectSignerFromPkey";
 import {getSigners} from "./getSigners";
-import { Wallet } from "ethers";
+import {Wallet} from "ethers";
 
 export async function getAPTeamOwner(
   hre: HardhatRuntimeEnvironment,
   pkey?: string
-): Promise<SignerWithAddress|Wallet> {
+): Promise<SignerWithAddress | Wallet> {
   if (pkey) {
     return await connectSignerFromPkey(pkey, hre);
   }