New Documentation for vaults contracts

.github/workflows/tests.yaml

@@ -12,7 +12,7 @@ jobs:
         uses: actions/checkout@v2
       - uses: actions/setup-node@v3
         with:
-          node-version: "20"
+          node-version: "22"
           cache: "npm"
       - run: npm ci
       - run: npx hardhat compile

README.md

@@ -10,13 +10,44 @@ npx hardhat test
 GAS_REPORT=true npx hardhat test
 ```
 
+## Multi Strategy Vaults (MSV)
+
+These contracts are ERC4626 vaults where several investment strategies are plugged and the funds are allocated across
+them.
+
+This allows diverfisication of the assets and the risks in a flexible manner.
+
+The contract doesn't implement any rebalance strategy, that needs to be called from the outside. It is just a queue
+of strategies for deposits and another for withdrawals and uses that order to serve the enter or exit request as
+fast as possible.
+
+For efficiency and transparency reasons, the strategies are contracts called with delegatecall, meaning that they
+execute in the context of the vault, managing the vault assets. Only trusted strategies must be plugged.
+
+### MSV Alternatives
+
+The repositoy includes three MultiStrategyVault alternatives, all inheriting from MSVBase, difering on how they manage
+access control or other features:
+
+- **MultiStrategyERC4626**: uses OZ's AccessControl contract for managing the permissions.
+- **AccessManagedMSV**: this one is intented to be deployed behing an AccessManagedProxy, a modified ERC1967
+  proxy that checks with an AccessManager (OZ 5.x) contract for each method called. The contract itself doesn't
+  implement any access control policy.
+- **OutflowLimitedMSV**: this one is a variation of AccessManagedMSV that also tracks the net inflows by slots
+  of time and rejects withdrawals when a given outflow limit is exceeded.
+
 ## Invest Strategies
 
 The invest Strategies (those who implement) IInvestStrategy are implementation contrats that manage the investment in
 a particular protocol, like AAVEv3 or CompoundV3. These implementation contracts are meant to be used with delegate
-calls from a containter contract (typically a 4626 vault), that contains one or several strategies.
+calls from a containter contract (typically a MSV), that contains one or several strategies.
 
-## MultiStrategyERC4626
+The current implemented strategies are:
 
-This vault supports several pluggable strategies to invest the funds diversified in several protocols, with resilience
-of these protocols being paused or not accepting deposits, and spreading the risk and generating diversified yields.
+- **AaveV3InvestStrategy**: invests the funds received in an AAVE pool.
+- **CompoundV3InvestStrategy**: invest the funds received in a Compound pool. Has support for claiming rewards that
+  are reinvested.
+- **SwapStableInvestStrategy**: the strategy consist in swapping the asset to an investment assets that typically
+  has a 1:1 equivalence with the asset. Useful for yield bearing assets like USDM or Lido ETH.
+- **SwapStableAaveV3InvestStrategy**: it swaps the asset and invests it into AAVE. Useful for equivalent assets that
+  have different returns on AAVE like Bridged USDC vs Native USDC.

contracts/AaveV3InvestStrategy.sol

@@ -10,6 +10,7 @@ import {InvestStrategyClient} from "./InvestStrategyClient.sol";
 
 /**
  * @title AaveV3InvestStrategy
+ *
  * @dev Strategy that invests/deinvests into AaveV3 on each deposit/withdraw.
  *
  * @custom:security-contact [email protected]
@@ -44,21 +45,25 @@ contract AaveV3InvestStrategy is IInvestStrategy {
     return _aave.getReserveData(address(_asset));
   }
 
+  /// @inheritdoc IInvestStrategy
   function connect(bytes memory initData) external virtual override onlyDelegCall {
     if (initData.length != 0) revert NoExtraDataAllowed();
   }
 
+  /// @inheritdoc IInvestStrategy
   function disconnect(bool force) external virtual override onlyDelegCall {
     IERC20 aToken = IERC20(_reserveData().aTokenAddress);
     if (!force && aToken.balanceOf(address(this)) != 0) revert CannotDisconnectWithAssets();
   }
 
+  /// @inheritdoc IInvestStrategy
   function maxWithdraw(address contract_) public view virtual override returns (uint256) {
     DataTypes.ReserveData memory reserve = _reserveData();
     if (!reserve.configuration.getActive() || reserve.configuration.getPaused()) return 0;
     return IERC20(reserve.aTokenAddress).balanceOf(contract_);
   }
 
+  /// @inheritdoc IInvestStrategy
   function maxDeposit(address /*contract_*/) public view virtual override returns (uint256) {
     DataTypes.ReserveData memory reserve = _reserveData();
     if (!reserve.configuration.getActive() || reserve.configuration.getPaused() || reserve.configuration.getFrozen())
@@ -67,18 +72,22 @@ contract AaveV3InvestStrategy is IInvestStrategy {
     return type(uint256).max;
   }
 
+  /// @inheritdoc IInvestStrategy
   function asset(address) public view virtual override returns (address) {
     return address(_asset);
   }
 
+  /// @inheritdoc IInvestStrategy
   function totalAssets(address contract_) public view virtual override returns (uint256 assets) {
     return IERC20(_reserveData().aTokenAddress).balanceOf(contract_);
   }
 
+  /// @inheritdoc IInvestStrategy
   function withdraw(uint256 assets) external virtual override onlyDelegCall {
     if (assets != 0) _aave.withdraw(address(_asset), assets, address(this));
   }
 
+  /// @inheritdoc IInvestStrategy
   function deposit(uint256 assets) external virtual override onlyDelegCall {
     if (assets != 0) _supply(assets);
   }
@@ -88,6 +97,7 @@ contract AaveV3InvestStrategy is IInvestStrategy {
     _aave.supply(address(_asset), assets, address(this), 0);
   }
 
+  /// @inheritdoc IInvestStrategy
   function forwardEntryPoint(uint8, bytes memory) external view onlyDelegCall returns (bytes memory) {
     // solhint-disable-next-line gas-custom-errors,reason-string
     revert();

contracts/AccessManagedMSV.sol

@@ -14,7 +14,8 @@ import {AccessManagedProxy} from "./AccessManagedProxy.sol";
 /**
  * @title AccessManagedMSV
  *
- * @dev Vault that invests/deinvests using a pluggable IInvestStrategy on each deposit/withdraw.
+ * @dev Vault that invests/deinvests using pluggable IInvestStrategy contracts on each deposit/withdraw.
+ *
  *      The vault MUST be deployed behind an AccessManagedProxy that controls the access to the critical methods
  *      Since this contract DOESN'T DO ANY ACCESS CONTROL.
  *
@@ -120,8 +121,9 @@ contract AccessManagedMSV is MSVBase, UUPSUpgradeable, ERC4626Upgradeable {
 
   /// @inheritdoc ERC4626Upgradeable
   function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal virtual override {
-    // Transfers the assets from the caller and supplies to compound
+    // super._deposit(...) the assets from the caller to this contract
     super._deposit(caller, receiver, assets, shares);
+    // Then it deposits to the strategies
     _depositToStrategies(assets);
   }
 
@@ -142,8 +144,9 @@ contract AccessManagedMSV is MSVBase, UUPSUpgradeable, ERC4626Upgradeable {
 
   /// @inheritdoc MSVBase
   function _checkForwardToStrategy(uint8 strategyIndex, uint8 method, bytes memory) internal view override {
-    // To call forwardToStrategy, besides the access the method, we will check with the ACCESS_MANAGER we
-    // canCall()
+    // To call forwardToStrategy, besides the access the method, we will check with the ACCESS_MANAGER the
+    // msg.sender canCall this contract with a fake selector generated by
+    // `getForwardToStrategySelector(strategyIndex, method)`
     IAccessManager acMgr = AccessManagedProxy(payable(address(this))).ACCESS_MANAGER();
     (bool immediate, ) = acMgr.canCall(msg.sender, address(this), getForwardToStrategySelector(strategyIndex, method));
     // This only works when immediate == true, so timelocks can't be applied on this extra permission,

contracts/AccessManagedProxy.sol

@@ -4,6 +4,25 @@ pragma solidity ^0.8.0;
 import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol";
 
+/**
+ * @title AccessManagedProxy
+ * @dev Proxy contract using IAccessManager to manage access control before delegating calls.
+ *
+ *      It's a variant of ERC1967Proxy.
+ *
+ *      Currently the check is executed on any call received by the proxy contract even calls to view methods
+ *      (staticcall). In the setup of the ACCESS_MANAGER permissions you would want to make all the views and pure
+ *      functions enabled for the PUBLIC_ROLE.
+ *
+ *      For gas efficiency, the ACCESS_MANAGER is immutable, so take care you don't lose control of it, otherwise
+ *      it will make your contract inaccesible or other bad things will happen.
+ *
+ *      Check https://forum.openzeppelin.com/t/accessmanagedproxy-is-a-good-idea/41917 for a discussion on the
+ *      advantages and disadvantages of using it.
+ *
+ * @custom:security-contact [email protected]
+ * @author Ensuro
+ */
 contract AccessManagedProxy is ERC1967Proxy {
   IAccessManager public immutable ACCESS_MANAGER;
 
@@ -20,9 +39,15 @@ contract AccessManagedProxy is ERC1967Proxy {
 
   /**
    * @dev Checks with the ACCESS_MANAGER if msg.sender is authorized to call the current call's function,
-   * and if so, delegates the current call to `implementation`.
+   *      and if so, delegates the current call to `implementation`.
    *
-   * This function does not return to its internal call site, it will return directly to the external caller.
+   *      This function does not return to its internal call site, it will return directly to the external caller.
+   *
+   *      It uses `msg.sender`, so it will ignore any metatx like ERC-2771 or other ways of changing the sender.
+   *
+   *      Only let's the call go throught for immediate access, but scheduled calls can be made throught the
+   *      ACCESS_MANAGER. It doesn't support the `.consumeScheduledOp(...)` flow that other access managed contracts
+   *      support.
    */
   function _delegate(address implementation) internal virtual override {
     (bool immediate, ) = ACCESS_MANAGER.canCall(msg.sender, address(this), bytes4(msg.data[0:4]));

contracts/CompoundV3ERC4626.sol

@@ -13,7 +13,10 @@ import {PermissionedERC4626} from "./PermissionedERC4626.sol";
  * @title CompoundV3ERC4626
  * @dev Vault that invests/deinvests into CompoundV3 on each deposit/withdraw. Also, has a method to claim the rewards,
  *      swap them, and reinvests the result into CompoundV3.
- *      Entering or exiting the vault is permissioned, requires LP_ROLE
+ *      Entering or exiting the vault is permissioned, requires LP_ROLE.
+ *
+ *      Use it at your own risk, this is a proof of concept, but it's not used by the authors, we prefer using the
+ *      pluggable strategies (See {CompoundV3InvestStrategy})
  *
  * @custom:security-contact [email protected]
  * @author Ensuro

contracts/CompoundV3InvestStrategy.sol

@@ -12,33 +12,55 @@ import {InvestStrategyClient} from "./InvestStrategyClient.sol";
 
 /**
  * @title CompoundV3InvestStrategy
- * @dev Strategy that invests/deinvests into CompoundV3 on each deposit/withdraw. Also, has a method to claim the rewards,
- *      swap them, and reinvests the result into CompoundV3.
+ * @dev Strategy that invests/deinvests into CompoundV3 on each deposit/withdraw. Also, has a method to claim the
+ *      rewards, swap them, and reinvests the result into CompoundV3.
+ *
+ *      The rewards are not accounted in the totalAssets() until they are claimed. It's advised to claim the rewards
+ *      frequently, to avoid discrete variations on the returns.
+ *
+ *      This strategy as the other IInvestStrategy are supposed to be called with delegateCall by a vault, managing
+ *      the assets on behalf of the vault.
  *
  * @custom:security-contact [email protected]
  * @author Ensuro
  */
 contract CompoundV3InvestStrategy is IInvestStrategy {
   using SwapLibrary for SwapLibrary.SwapConfig;
 
-  bytes32 public constant HARVEST_ROLE = keccak256("HARVEST_ROLE");
-  bytes32 public constant SWAP_ADMIN_ROLE = keccak256("SWAP_ADMIN_ROLE");
-
   address private immutable __self = address(this);
   bytes32 public immutable storageSlot = InvestStrategyClient.makeStorageSlot(this);
 
   ICompoundV3 internal immutable _cToken;
   ICometRewards internal immutable _rewardsManager;
   address internal immutable _baseToken;
 
+  /**
+   * @dev Emitted when the rewards are claimed
+   *
+   * @param token The token in which the rewards are denominated
+   * @param rewards Amount of rewards received (in units of token)
+   * @param receivedInAsset Amount of `asset()` received in exchange of the rewards sold
+   */
   event RewardsClaimed(address token, uint256 rewards, uint256 receivedInAsset);
 
+  /**
+   * @dev Emitted when the swap config is changed. This swap config is used to swap the rewards for assets``
+   *
+   * @param oldConfig The swap configuration before the change
+   * @param newConfig The swap configuration after the change
+   */
   event SwapConfigChanged(SwapLibrary.SwapConfig oldConfig, SwapLibrary.SwapConfig newConfig);
 
   error CanBeCalledOnlyThroughDelegateCall();
   error CannotDisconnectWithAssets();
   error NoExtraDataAllowed();
 
+  /**
+   * @dev "Methods" called from the vault to execute different operations on the strategy
+   *
+   * @enum harvestRewards Used to trigger the claim of rewards and the swap of them for `asset`
+   * @enum setSwapConfig Used to change the swap configuration, used for selling the rewards
+   */
   enum ForwardMethods {
     harvestRewards,
     setSwapConfig
@@ -49,42 +71,57 @@ contract CompoundV3InvestStrategy is IInvestStrategy {
     _;
   }
 
+  /**
+   * @dev Constructor of the strategy.
+   *
+   * @param cToken_ The address of the cToken (compound pool) where funds will be supplied. The strategy asset()
+   *                will be `cToken_.baseToken()`.
+   * @param rewardsManager_ The address of the rewards manager contract that will be used to claim the rewards
+   */
   constructor(ICompoundV3 cToken_, ICometRewards rewardsManager_) {
     _cToken = cToken_;
     _rewardsManager = rewardsManager_;
     _baseToken = cToken_.baseToken();
   }
 
+  /// @inheritdoc IInvestStrategy
   function connect(bytes memory initData) external virtual override onlyDelegCall {
     _setSwapConfig(SwapLibrary.SwapConfig(SwapLibrary.SwapProtocol.undefined, 0, bytes("")), initData);
   }
 
+  /// @inheritdoc IInvestStrategy
   function disconnect(bool force) external virtual override onlyDelegCall {
     if (!force && _cToken.balanceOf(address(this)) != 0) revert CannotDisconnectWithAssets();
   }
 
+  /// @inheritdoc IInvestStrategy
   function maxWithdraw(address contract_) public view virtual override returns (uint256) {
     if (_cToken.isWithdrawPaused()) return 0;
     return _cToken.balanceOf(contract_);
   }
 
+  /// @inheritdoc IInvestStrategy
   function maxDeposit(address /*contract_*/) public view virtual override returns (uint256) {
     if (_cToken.isSupplyPaused()) return 0;
     return type(uint256).max;
   }
 
+  /// @inheritdoc IInvestStrategy
   function asset(address) public view virtual override returns (address) {
     return _baseToken;
   }
 
+  /// @inheritdoc IInvestStrategy
   function totalAssets(address contract_) public view virtual override returns (uint256 assets) {
     return _cToken.balanceOf(contract_);
   }
 
+  /// @inheritdoc IInvestStrategy
   function withdraw(uint256 assets) external virtual override onlyDelegCall {
     _cToken.withdraw(_baseToken, assets);
   }
 
+  /// @inheritdoc IInvestStrategy
   function deposit(uint256 assets) external virtual override onlyDelegCall {
     _supply(assets);
   }
@@ -118,12 +155,22 @@ contract CompoundV3InvestStrategy is IInvestStrategy {
     StorageSlot.getBytesSlot(storageSlot).value = newSwapConfigAsBytes;
   }
 
+  /// @inheritdoc IInvestStrategy
   function forwardEntryPoint(uint8 method, bytes memory params) external onlyDelegCall returns (bytes memory) {
     ForwardMethods checkedMethod = ForwardMethods(method);
     if (checkedMethod == ForwardMethods.harvestRewards) {
+      // The harvestRewards receives the price as an input, expressed in wad as the units of the reward token
+      // required to by one unit of `asset()`.
+      // For example if reward token is COMP and asset is USDC, and price of COMP is $ 100,
+      // Then we should receive 0.01 (in wad).
+      // This is a permissioned call, and someone giving a wrong price can make the strategy sell the rewards at
+      // a zero price. So you should be carefull regarding who can call this method, if rewards are a relevant part
+      // of the returns
       uint256 price = abi.decode(params, (uint256));
       _harvestRewards(price);
     } else if (checkedMethod == ForwardMethods.setSwapConfig) {
+      // This method receives the new swap config to be used when swapping rewards for asset().
+      // A wrong swap config, with high slippage, might affect the conversion rate of the rewards into assets
       _setSwapConfig(_getSwapConfig(address(this)), params);
     }
     // Show never reach to this revert, since method should be one of the enum values but leave it in case
@@ -139,6 +186,11 @@ contract CompoundV3InvestStrategy is IInvestStrategy {
     return abi.decode(swapConfigAsBytes, (SwapLibrary.SwapConfig));
   }
 
+  /**
+   * @dev Returns the swap configuration of the given contract. It uses the internal function _getSwapConfig that returns the decoded swap configuration structure.
+   *
+   * @param contract_ Address of the contract configuration being requested.
+   */
   function getSwapConfig(address contract_) public view returns (SwapLibrary.SwapConfig memory) {
     return _getSwapConfig(contract_);
   }