diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index deda8ee..798bb18 100644 --- a/.github/workflows/tests.yaml +++ b/.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 diff --git a/README.md b/README.md index e9f4bbd..bc07bb6 100644 --- a/README.md +++ b/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. diff --git a/contracts/AaveV3InvestStrategy.sol b/contracts/AaveV3InvestStrategy.sol index b67899d..1855128 100644 --- a/contracts/AaveV3InvestStrategy.sol +++ b/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 security@ensuro.co @@ -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(); diff --git a/contracts/AccessManagedMSV.sol b/contracts/AccessManagedMSV.sol index a3ce387..82dd77b 100644 --- a/contracts/AccessManagedMSV.sol +++ b/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, diff --git a/contracts/AccessManagedProxy.sol b/contracts/AccessManagedProxy.sol index 19b8154..6170a3c 100644 --- a/contracts/AccessManagedProxy.sol +++ b/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 security@ensuro.co + * @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])); diff --git a/contracts/CompoundV3ERC4626.sol b/contracts/CompoundV3ERC4626.sol index a99d1c6..87c000b 100644 --- a/contracts/CompoundV3ERC4626.sol +++ b/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 security@ensuro.co * @author Ensuro diff --git a/contracts/CompoundV3InvestStrategy.sol b/contracts/CompoundV3InvestStrategy.sol index cdca427..6dbdab5 100644 --- a/contracts/CompoundV3InvestStrategy.sol +++ b/contracts/CompoundV3InvestStrategy.sol @@ -12,8 +12,14 @@ 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 security@ensuro.co * @author Ensuro @@ -21,9 +27,6 @@ import {InvestStrategyClient} from "./InvestStrategyClient.sol"; 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); @@ -31,14 +34,33 @@ contract CompoundV3InvestStrategy is IInvestStrategy { 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_); } diff --git a/contracts/InvestStrategyClient.sol b/contracts/InvestStrategyClient.sol index a8419e4..c4af150 100644 --- a/contracts/InvestStrategyClient.sol +++ b/contracts/InvestStrategyClient.sol @@ -7,7 +7,10 @@ import {IInvestStrategy} from "./interfaces/IInvestStrategy.sol"; /** * @title InvestStrategyClient - * @dev Library to simplify the interaction with IInvestStrategy objects + * + * @dev Library to simplify the interaction with IInvestStrategy objects. Abstract away the delegate calls and + * other gotchas of the communication with the strategies. + * * @custom:security-contact security@ensuro.co * @author Ensuro */ @@ -21,10 +24,25 @@ library InvestStrategyClient { error InvalidStrategyAsset(); + /** + * @dev Performs a connection with the given strategy. See {IInvestStrategy.connect} + * + * @param strategy Investment strategy to connect. + * @param initStrategyData Initialization data required for the strategy to connect. + */ function dcConnect(IInvestStrategy strategy, bytes memory initStrategyData) internal { address(strategy).functionDelegateCall(abi.encodeCall(IInvestStrategy.connect, initStrategyData)); } + /** + * @dev Disconnects from the given strategy. This diconnection is done using a delegatecall to the strategy. + * + * See {IInvestStrategy.disconnect} + * + * @param strategy Investment strategy to connect. + * @param force Bool value to force disconnection, when `true` it will just emit DisconnectFailed if it fails, + * otherwise it will revert when it's false and fails. + */ function dcDisconnect(IInvestStrategy strategy, bool force) internal { if (force) { // solhint-disable-next-line avoid-low-level-calls @@ -37,6 +55,17 @@ library InvestStrategyClient { } } + /** + * @dev Delegate call to withdraw assets from a given strategy. + * + * See {IInvestStrategy.withdraw} + * + * @param strategy Strategy to withdraw assets from. + * @param assets Amount of assets to be withdrawn. + * @param ignoreError When true, the error will be caught and event WithdrawFailed will be emitted, + otherwise it will revert when it's false and fails. + * @return Returns true if it was successful, otherwise returns false (only when ignoreError = true, otherwise reverts) + */ function dcWithdraw(IInvestStrategy strategy, uint256 assets, bool ignoreError) internal returns (bool) { if (ignoreError) { // solhint-disable-next-line avoid-low-level-calls @@ -51,6 +80,17 @@ library InvestStrategyClient { } } + /** + * @dev Delegate call to deposit assets from a given strategy. + * + * See {IInvestStrategy.deposit} + * + * @param strategy Strategy to deposit assets from. + * @param assets Amount of assets to be deposited. + * @param ignoreError When true, the error will be caught and event DepositFailed will be emitted, + otherwise it will revert when it's false and fails. + * @return Returns true if it was successful, otherwise returns false (only when ignoreError = true, otherwise reverts) + */ function dcDeposit(IInvestStrategy strategy, uint256 assets, bool ignoreError) internal returns (bool) { if (ignoreError) { // solhint-disable-next-line avoid-low-level-calls @@ -65,15 +105,40 @@ library InvestStrategyClient { } } + /** + * @dev Delegate call to forward a custom method of the given strategy. + * + * See {IInvestStrategy.forwardEntryPoint} + * + * @param strategy Strategy to forward the custom method. + * @param method Method to be forwarded. + * @param extraData Additional params required by the method + * @return Returns the result of {IInvestStrategy.forwardEntryPoint} + */ function dcForward(IInvestStrategy strategy, uint8 method, bytes memory extraData) internal returns (bytes memory) { return address(strategy).functionDelegateCall(abi.encodeCall(IInvestStrategy.forwardEntryPoint, (method, extraData))); } + /** + * @dev Checks the strategy asset() to ensure it is the same as the asset of the vault. + * @param strategy Strategy to be checked. + * @param asset Asset of the vault. + */ function checkAsset(IInvestStrategy strategy, address asset) internal view { if (strategy.asset(address(this)) != asset) revert InvalidStrategyAsset(); } + /** + * @dev Replaces one strategy with another. + * + * @param oldStrategy The strategy to be replaced + * @param newStrategy The new strategy to connect + * @param newStrategyInitData The initialization data that will be send to the `newStrategy` on connect + * @param asset Asset of the vault (the newStrategy has to have the same asset) + * @param force When false, it reverts if withdrawal of assets or disconnection or deposit into the new strategy + * fails. When true, it doesn't revert on any of those errors, it just emits events. + */ function strategyChange( IInvestStrategy oldStrategy, IInvestStrategy newStrategy, @@ -95,7 +160,8 @@ library InvestStrategyClient { /** * @dev Returns the slot where the specific data of the strategy is stored. - * Warning! This assumes the same strategy (deployed code in a given address) isn't used twice inside a given + * + * WARNING: This assumes the same strategy (deployed code in a given address) isn't used twice inside a given * contract. If that happens, the storage of one can collide with the other. * Also, be aware if you unplug and the re-plug a given strategy into a contract, you might be reading a state * that is not clean @@ -104,14 +170,29 @@ library InvestStrategyClient { return keccak256(abi.encode("co.ensuro.InvestStrategyClient", strategy)); } + /** + * @dev Returns the total assets in the strategy given. + * + * See {IInvestStrategy.totalAssets()} + */ function totalAssets(IInvestStrategy strategy) internal view returns (uint256) { return strategy.totalAssets(address(this)); } + /** + * @dev Returns the maximum amount of assets that can be deposited in the strategy. + * + * See {IInvestStrategy.maxDeposit} + */ function maxDeposit(IInvestStrategy strategy) internal view returns (uint256) { return strategy.maxDeposit(address(this)); } + /** + * @dev Returns the maximum amount of assets that can be withdrawn from the strategy. + * + * See {IInvestStrategy.maxWithdraw} + */ function maxWithdraw(IInvestStrategy strategy) internal view returns (uint256) { return strategy.maxWithdraw(address(this)); } diff --git a/contracts/MSVBase.sol b/contracts/MSVBase.sol index e3d1ffa..24a93c7 100644 --- a/contracts/MSVBase.sol +++ b/contracts/MSVBase.sol @@ -8,6 +8,29 @@ import {IInvestStrategy} from "./interfaces/IInvestStrategy.sol"; import {InvestStrategyClient} from "./InvestStrategyClient.sol"; import {IExposeStorage} from "./interfaces/IExposeStorage.sol"; +/** + * @title MSVBase + * @dev Base vault contract that manages multiple investment strategies. + * Allows deposits/withdraws from each strategy, and also permit rebalances between them. + * + * Funds that enter the vault, will be deposited into the strategies following _depositQueue (only tries the + * next strategy if the current one doesn't accept more deposits). + * + * Funds that exit the vault, will be withdrawn into the strategies following _withdrawQueue (only tries the + * next strategy if the current one doesn't accept more withdrawals). + * + * It doesn't have any allocation strategy besides that. Rebalance is done externally by calling `rebalance` + * method. + * + * This is a base contract, intended to be inherited by implementations of the ERC4626 standard, that will + * handle access control, deposits and other stuff. + * + * WARNING: this contract uses storage gaps (https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps) + * to manage upgradeability potential issues, NOT namespaced storage + * + * @custom:security-contact security@ensuro.co + * @author Ensuro + */ abstract contract MSVBase is IExposeStorage { using InvestStrategyClient for IInvestStrategy; @@ -94,6 +117,10 @@ abstract contract MSVBase is IExposeStorage { return ret; } + /** + * @dev For each strategy in the deposit queue, calculates the max deposit and sum it up to finally return the + * total assets could be deposited. + */ function _maxDepositable() internal view returns (uint256 ret) { for (uint256 i; address(_strategies[i]) != address(0) && i < MAX_STRATEGIES; i++) { uint256 maxDep = _strategies[i].maxDeposit(); @@ -103,12 +130,20 @@ abstract contract MSVBase is IExposeStorage { return ret; } + /** + * @dev Sum up the total assets of each strategy in the vault and returns the total value. + */ function _totalAssets() internal view returns (uint256 assets) { for (uint256 i; address(_strategies[i]) != address(0) && i < MAX_STRATEGIES; i++) { assets += _strategies[i].totalAssets(); } } + /** + * @dev Withdraw assets from the strategies in the withdraw queue order until zero assets remains to be withdrawn. + * After finishing the withdraw, left must be zero, otherwise reverts, and should never happen. + * @param assets The amount of assets to be withdrawn from the strategies. + */ function _withdrawFromStrategies(uint256 assets) internal { uint256 left = assets; for (uint256 i; left != 0 && _withdrawQueue[i] != 0 && i < MAX_STRATEGIES; i++) { @@ -121,6 +156,11 @@ abstract contract MSVBase is IExposeStorage { if (left != 0) revert WithdrawError(); // This shouldn't happen, since assets must be <= maxWithdraw(owner) } + /** + * @dev Deposit assets to the strategies in the deposit queue order until zero assets remains to be deposited. + * After finishing the deposit, left must be zero, otherwise reverts, and should never happen. + * @param assets The amount of assets to be deposited to the strategies. + */ function _depositToStrategies(uint256 assets) internal { // Transfers the assets from the caller and supplies to compound uint256 left = assets; @@ -149,7 +189,9 @@ abstract contract MSVBase is IExposeStorage { } /** - * @dev Checks the caller can execute this forwardToStrategy call, otherwise reverts + * @dev Checks the caller can execute this forwardToStrategy call, otherwise reverts. + * + * This method MUST be implemented by the inheriting contracts with the specific access control mechanism * * @param strategyIndex The index of the strategy in the _strategies array * @param method Id of the method to call. Is recommended that the strategy defines an enum with the methods that @@ -159,8 +201,9 @@ abstract contract MSVBase is IExposeStorage { function _checkForwardToStrategy(uint8 strategyIndex, uint8 method, bytes memory extraData) internal view virtual; /** - * @dev Used to call specific methods on the strategies. Anyone can call this method, is responsability of the - * IInvestStrategy to check access permissions when needed. + * @dev Used to call specific methods on the strategies. The specific vault implementation will define the access + * control mechanism to validate who can execute these calls. + * * @param strategyIndex The index of the strategy in the _strategies array * @param method Id of the method to call. Is recommended that the strategy defines an enum with the methods that * can be called externally and validates this value. @@ -281,7 +324,14 @@ abstract contract MSVBase is IExposeStorage { strategy.dcDisconnect(force); emit StrategyRemoved(strategy, strategyIndex); } - + /** + * @dev Updates the deposit queue with a new one. + * + * @notice Emits DepositQueueChanged(uint8[]) when updating the deposit queue. + * + * @param newDepositQueue_ New deposit queue, the lenght must be the same of the installed strategies without + * repeated indexes + */ function changeDepositQueue(uint8[] memory newDepositQueue_) public virtual { bool[MAX_STRATEGIES] memory seen; uint256 i = 0; @@ -297,6 +347,14 @@ abstract contract MSVBase is IExposeStorage { emit DepositQueueChanged(newDepositQueue_); } + /** + * @dev Updates the withdraw queue with a new one. + * + * @notice Emits WithdrawQueueChanged(uint8[]) when updating the deposit queue. + * + * @param newWithdrawQueue_ New withdrawal queue, the lenght must be the same of the installed strategies without + * repeated indexes + */ function changeWithdrawQueue(uint8[] memory newWithdrawQueue_) public virtual { bool[MAX_STRATEGIES] memory seen; uint8 i = 0; @@ -313,7 +371,10 @@ abstract contract MSVBase is IExposeStorage { } /** - * @dev Moves funds from one strategy to the other. + * @dev Moves funds from one strategy to another. + * + * @notice Emits {Rebalance(strategyFrom, strategyTo, amount)} + * * @param strategyFromIdx The index of the strategy that will provide the funds in the _strategies array * @param strategyToIdx The index of the strategy that will receive the funds in the _strategies array * @param amount The amount to transfer from one strategy to the other. type(uint256).max to move all the assets. @@ -333,6 +394,10 @@ abstract contract MSVBase is IExposeStorage { return amount; } + /** + * @dev Returns the list of strategies in the vault in order. + * The array is filled with zero addresses after the first one that is address(0). + */ function strategies() external view returns (IInvestStrategy[MAX_STRATEGIES] memory) { return _strategies; } diff --git a/contracts/OutflowLimitedAMMSV.sol b/contracts/OutflowLimitedAMMSV.sol index cb7009b..9db8203 100644 --- a/contracts/OutflowLimitedAMMSV.sol +++ b/contracts/OutflowLimitedAMMSV.sol @@ -15,7 +15,10 @@ import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; * withdraw/redeem/mint/deposit methods. * * The limit is applied for TWO `slotSize` periods. So for example if slotSize=1 day and limit=100K, this means - * that up to 100K of outflows every two calendar days are acceptable. + * that up to 100K of outflows every two consecutive calendar days are acceptable. + + * 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. * * @custom:security-contact security@ensuro.co * @author Ensuro @@ -32,12 +35,6 @@ contract OutflowLimitedAMMSV is AccessManagedMSV { mapping(SlotIndex => int256) assetsDelta; // Variation in assets in a given slot } - enum MethodType { - other, - enter, - exit - } - event LimitChanged(uint256 slotSize, uint256 newLimit); event DeltaManuallySet(SlotIndex slot, int256 oldDelta, int256 newDelta); @@ -56,9 +53,6 @@ contract OutflowLimitedAMMSV is AccessManagedMSV { /** * @dev Changes the limit and the timeframe used to track it. * - * @notice This method doesn't have built-in access control. The access control validation is supposed to be - * implemented by the proxy. But this SHOULDN'T be publicly available. - * * @param slotSize The duration in seconds of the timeframe used to limit the amount of outflows. * @param limit The max amount of outflows that will be allowed in a given time slot. */ @@ -69,18 +63,39 @@ contract OutflowLimitedAMMSV is AccessManagedMSV { emit LimitChanged(slotSize, limit); } + /** + * @dev Returns the current time slot size in seconds. + */ function getOutflowLimitSlotSize() external view returns (uint256) { return _getLOMStorage().slotSize; } + /** + * @dev Returns the net outflow limit that will be applied on two consecutive time slots + */ function getOutflowLimit() external view returns (uint256) { return _getLOMStorage().limit; } + /** + * @dev The current delta variation in assets for the given slot. + * Calculated as the sum of limit + deposits - withdrawals. + * @param slot The given slot to check the delta. Compatible with the slot calculated by makeOutflowSlot. + * @return The net flows in a slot (positive for inflows, more deposits than withdrawals, negative otherwise) + */ function getAssetsDelta(SlotIndex slot) external view returns (int256) { return _getLOMStorage().assetsDelta[slot]; } + /** + * @dev Computes the SlotIndex datatype comining both the slotSize and the index in which the timestamp is in + * a line of time that starts at epoch, with slots of slotSize + * + * @param slotSize The size of the slot in seconds that splits the timeline + * @param timestamp The time for which we want to calculate the slot. + * @return Returns a SlotIndex datatype that's the combination of the slotSize and the index in which the timestamp + * falls + */ function makeOutflowSlot(uint256 slotSize, uint40 timestamp) external pure returns (SlotIndex) { return SlotIndex.wrap((slotSize << 128) + timestamp / slotSize); } @@ -89,14 +104,12 @@ contract OutflowLimitedAMMSV is AccessManagedMSV { * @dev Manually changes the delta in a given slot. Used to exceptionally allow or disallow limits different than * the configured ones or to reset the limit when a valid operation is verified. * - * @notice This method doesn't have built-in access control. The access control validation is supposed to be - * implemented by the proxy. But this SHOULDN'T be publicly available. - * * @param slot Identification of the slot to modify. - * The slot is computed as `slotSize << 128 + block.timestamp / slotSize` - * @param newDelta The delta in assets to store in a given slot + * The slot is computed as `slotSize << 128 + block.timestamp / slotSize` (See {makeOutflowSlot}) + * @param deltaChange The modification to apply to the registered inflows in a given slot. Positive to increase the + * inflows, negative to decrease the outflows. + * @return newDelta The resulting delta in the slot after applying the change */ - // solhint-disable-next-line func-name-mixedcase function changeDelta(SlotIndex slot, int256 deltaChange) external returns (int256 newDelta) { int256 oldDelta = _getLOMStorage().assetsDelta[slot]; newDelta = _getLOMStorage().assetsDelta[slot] += deltaChange; diff --git a/contracts/PermissionedERC4626.sol b/contracts/PermissionedERC4626.sol index 5112316..bc05f36 100644 --- a/contracts/PermissionedERC4626.sol +++ b/contracts/PermissionedERC4626.sol @@ -8,6 +8,15 @@ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; +/** + * @title PermissionedERC4626 + * @dev Base class for permissioned ERC-4626 that use AccessControl for the access permissions. + * + * Not used at the moment, since we started to use AccessManagedProxy contracts. + * + * @custom:security-contact security@ensuro.co + * @author Ensuro + */ contract PermissionedERC4626 is AccessControlUpgradeable, UUPSUpgradeable, ERC4626Upgradeable { using SafeERC20 for IERC20Metadata; diff --git a/contracts/SwapStableAaveV3InvestStrategy.sol b/contracts/SwapStableAaveV3InvestStrategy.sol index 3c8a92a..1699379 100644 --- a/contracts/SwapStableAaveV3InvestStrategy.sol +++ b/contracts/SwapStableAaveV3InvestStrategy.sol @@ -26,8 +26,9 @@ contract SwapStableAaveV3InvestStrategy is SwapStableInvestStrategy { * @dev Constructor of the strategy * * @param asset_ The address of the underlying token used for accounting, depositing, and withdrawing. - * @param investAsset_ The address of the tokens hold by the strategy. Typically a rebasing yield bearing token + * @param investAsset_ The address of the tokens that are later supplied to AAVE * @param price_ Approximate amount of units of _asset required to acquire a unit of _investAsset + * @param aave_ Address of AAVE Pool contract */ constructor( IERC20Metadata asset_, @@ -69,13 +70,15 @@ contract SwapStableAaveV3InvestStrategy is SwapStableInvestStrategy { if (assets == 0) return; // Withdraw everything then deposit the remainder _aave.withdraw(address(_investAsset), type(uint256).max, address(this)); + // This call will convert investAssets to assets super.withdraw(assets); // Supply the remaining balance again to AAVE - Ignore errors to avoid reverting in case this deposit fails + // In the worst case we will have some funds not invested _supply(_investAsset.balanceOf(address(this)), true); } function deposit(uint256 assets) public virtual override onlyDelegCall { - super.deposit(assets); + super.deposit(assets); // Converts assets to investAssets _supply(_investAsset.balanceOf(address(this)), false); } diff --git a/contracts/SwapStableInvestStrategy.sol b/contracts/SwapStableInvestStrategy.sol index 5d69722..de68870 100644 --- a/contracts/SwapStableInvestStrategy.sol +++ b/contracts/SwapStableInvestStrategy.sol @@ -62,30 +62,47 @@ contract SwapStableInvestStrategy is IInvestStrategy { return 10 ** (18 - token.decimals()); } + /// @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 && _investAsset.balanceOf(address(this)) != 0) revert CannotDisconnectWithAssets(); } + /// @inheritdoc IInvestStrategy function maxWithdraw(address contract_) public view virtual override returns (uint256) { return totalAssets(contract_); // TODO: check how much can be swapped without breaking the slippage } + /// @inheritdoc IInvestStrategy function maxDeposit(address /*contract_*/) public view virtual override returns (uint256) { return type(uint256).max; // TODO: check how much can be swapped without breaking the slippage } + /// @inheritdoc IInvestStrategy function asset(address) public view virtual override returns (address) { return address(_asset); } + /** + * @dev Returns the address of the asset invested in the strategy. + */ function investAsset(address) public view returns (address) { return address(_investAsset); } + /** + * @dev Converts a given amount of investAssets into assets, considering the difference in decimals and the + * maxSlippage accepted + * + * @param investAssets Amount in investAssets + * @param contract_ The address of the vault, not used in the implementation, but it might be required by + * inheriting contracts. + * @return assets The minimum amount in assets that will result from swapping `investAssets` + */ function _convertAssets(uint256 investAssets, address contract_) internal view virtual returns (uint256 assets) { return Math.mulDiv( @@ -95,10 +112,17 @@ contract SwapStableInvestStrategy is IInvestStrategy { ) / _toWadFactor(_asset); } + /// @inheritdoc IInvestStrategy function totalAssets(address contract_) public view virtual override returns (uint256 assets) { return _convertAssets(_investAsset.balanceOf(contract_), contract_); } + /// @inheritdoc IInvestStrategy + /** + * @dev Withdraws the amount of assets given from the strategy swapping _investAsset to _asset + * + * @param assets Amount of assets to be withdrawn. + */ function withdraw(uint256 assets) public virtual override onlyDelegCall { if (assets == 0) return; SwapLibrary.SwapConfig memory swapConfig = abi.decode( @@ -116,6 +140,12 @@ contract SwapStableInvestStrategy is IInvestStrategy { } } + /// @inheritdoc IInvestStrategy + /** + * @dev Deposit the amount of assets given into the strategy by swapping _asset to _investAsset + * + * @param assets Amount of assets to be deposited. + */ function deposit(uint256 assets) public virtual override onlyDelegCall { if (assets == 0) return; SwapLibrary.SwapConfig memory swapConfig = abi.decode( @@ -134,9 +164,13 @@ contract SwapStableInvestStrategy 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.setSwapConfig) { + // The change of the swap config, that involves both the DEX to use and the maxSlippage is a critical operation + // that should be access controlled, probably imposing timelocks, because it can produce a conversion of the + // assets at a non-fair price _setSwapConfig(_getSwapConfig(address(this)), params); } // Should never reach to this revert, since method should be one of the enum values but leave it in case @@ -152,6 +186,11 @@ contract SwapStableInvestStrategy is IInvestStrategy { return abi.decode(swapConfigAsBytes, (SwapLibrary.SwapConfig)); } + /** + * @dev Returns the swap configuration of the given contract. + * + * @param contract_ Address of the vault contract + */ function getSwapConfig(address contract_) public view returns (SwapLibrary.SwapConfig memory) { return _getSwapConfig(contract_); } diff --git a/contracts/interfaces/IInvestStrategy.sol b/contracts/interfaces/IInvestStrategy.sol index 064924a..02dc482 100644 --- a/contracts/interfaces/IInvestStrategy.sol +++ b/contracts/interfaces/IInvestStrategy.sol @@ -7,19 +7,20 @@ pragma solidity ^0.8.0; * @dev Interface that must follow investment strategies to be plugged into ERC4626 vaults. All the non-view methods * MUST be called using delegatecall, thus executed in the context of the calling vault. * - * The strategy can use the storage of the calling contract, but ONLY in the received storageSlot. + * The strategy can use the storage of the calling contract, but ONLY in the storageSlot computed calling + * {InvestStrategyClient.makeStorageSlot} * - * The calling contract should implement IExposeStorage to give access to the storage to the views. + * The calling contract should implement IExposeStorage to give access to the storage to the strategy views * * @custom:security-contact security@ensuro.co * @author Ensuro */ interface IInvestStrategy { /** - * @dev Called when the strategy is plugged into the vault. Initializes the storage and can do validations (for - * example, checking the asset is the same) + * @dev Called when the strategy is plugged into the vault. Initializes the strategy storage and can do validations * - * @param initData Initialization data for the strategy. Must be parsed by the strategy. + * @param initData Initialization data for the strategy. Must be parsed by the strategy, since the format is + * strategy specific */ function connect(bytes memory initData) external; @@ -50,8 +51,8 @@ interface IInvestStrategy { /** * @dev Receives an external call to execute a custom method of action in the strategy. Can be used for harvesting - * rewards or other tasks. It's called with delegatecall from the calling vault, but the calling vault usually - * don't do any access validation, so if the method requires a permission, it must be checked by the strategy. + * rewards or other tasks. It's called with delegatecall from the calling vault. The calling vault SHOULD + * do the access control validation, since the strategy normally won't do it. * * @param method An id of the method or action to call/execute. It's recommended to define an enum and convert the * the uint8 value into the enum. @@ -59,38 +60,45 @@ interface IInvestStrategy { */ function forwardEntryPoint(uint8 method, bytes memory params) external returns (bytes memory); - // Views + /** + * VIEWS + * + * All the views receive as a parameter the contract (the vault) of which they should return the information, + * since they won't be called with delegatecall (because EVM doesn't have staticdelegatecall) + */ /** - * @dev The address of the underlying token used for accounting, depositing, and withdrawing. + * @dev The address of the underlying token used for accounting, depositing, and withdrawing. It must be the same + * as `IERC4626(contract_).asset()` when dealing with vaults. * - * @param contract_ The address of the contract that owns the assets. + * @param contract_ The address of the vault contract that owns the assets. */ function asset(address contract_) external view returns (address); /** * @dev Returns the number of assets under management of the investment strategy for a given contract. * - * @param contract_ The address of the contract that owns the assets. + * @param contract_ The address of the vault contract that owns the assets and where the strategy is plugged */ function totalAssets(address contract_) external view returns (uint256 totalManagedAssets); /** * @dev Returns the max amount that can be deposited into the strategy. * - * @param contract_ The address of the contract that owns the assets. + * @param contract_ The address of the vault contract that owns the assets and where the strategy is plugged */ function maxDeposit(address contract_) external view returns (uint256 maxAssets); /** * @dev Returns the max amount that can be withdrawn from the strategy. * - * @param contract_ The address of the contract that owns the assets. + * @param contract_ The address of the vault contract that owns the assets and where the strategy is plugged */ function maxWithdraw(address contract_) external view returns (uint256 maxAssets); /** * @dev Returns the slot where the data of the strategy can be stored. + * Typically it would return `InvestStrategyClient.makeStorageSlot()` */ function storageSlot() external view returns (bytes32); } diff --git a/test/test-limit-outflow.js b/test/test-limit-outflow.js index 5eeb2a3..4496afb 100644 --- a/test/test-limit-outflow.js +++ b/test/test-limit-outflow.js @@ -1,5 +1,5 @@ const { expect } = require("chai"); -const { amountFunction, getRole } = require("@ensuro/utils/js/utils"); +const { amountFunction } = require("@ensuro/utils/js/utils"); const { WEEK, DAY } = require("@ensuro/utils/js/constants"); const { initCurrency } = require("@ensuro/utils/js/test-utils"); const { encodeDummyStorage, tagit, makeAllViewsPublic, mergeFragments, setupAMRole } = require("./utils");