Change forwardToStrategy access control: strategies -> vault

.solhint.json

@@ -10,6 +10,7 @@
     "func-visibility": ["warn", { "ignoreConstructors": true }],
     "not-rely-on-time": "off",
     "max-states-count": "off",
+    "gas-custom-errors": "error",
     "immutable-vars-naming": "off"
   },
   "plugins": ["prettier"]

contracts/AaveV3InvestStrategy.sol

@@ -24,8 +24,6 @@ contract AaveV3InvestStrategy is IInvestStrategy {
   IPool internal immutable _aave;
   IERC20 internal immutable _asset;
 
-  // From OZ v5
-  error AccessControlUnauthorizedAccount(address account, bytes32 neededRole);
   error CanBeCalledOnlyThroughDelegateCall();
   error CannotDisconnectWithAssets();
   error NoExtraDataAllowed();

contracts/AccessManagedMSV.sol

@@ -3,11 +3,13 @@ pragma solidity ^0.8.0;
 
 import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import {IERC4626} from "@openzeppelin/contracts/interfaces/IERC4626.sol";
+import {IAccessManager} from "@openzeppelin/contracts/access/manager/IAccessManager.sol";
 import {ERC4626Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol";
 import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
 import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
 import {MSVBase} from "./MSVBase.sol";
 import {IInvestStrategy} from "./interfaces/IInvestStrategy.sol";
+import {AccessManagedProxy} from "./AccessManagedProxy.sol";
 
 /**
  * @title AccessManagedMSV
@@ -122,4 +124,32 @@ contract AccessManagedMSV is MSVBase, UUPSUpgradeable, ERC4626Upgradeable {
     super._deposit(caller, receiver, assets, shares);
     _depositToStrategies(assets);
   }
+
+  /**
+   * @dev Returns the selector used to define the role required to call forwardToStrategy on a given strategy and
+   *      method
+   *
+   * @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.
+   * @return selector The bytes4 selector required to execute the call (will be used with target=address(this))
+   */
+  function getForwardToStrategySelector(uint8 strategyIndex, uint8 method) public view returns (bytes4 selector) {
+    // I assemble a fake selector combining the address of the strategy, the index, and the method called
+    address strategy = address(_strategies[strategyIndex]);
+    return (bytes4(bytes1(strategyIndex)) >> 8) ^ (bytes4(bytes1(method))) ^ bytes4(bytes20(strategy));
+  }
+
+  /// @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()
+    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,
+    // only on the forwardToStrategy call.
+    // In the future we might use consumeScheduledOp flow to implement specific delays for specific
+    // forward calls
+    if (!immediate) revert AccessManagedProxy.AccessManagedUnauthorized(msg.sender);
+  }
 }

contracts/CompoundV3InvestStrategy.sol

@@ -4,7 +4,6 @@ pragma solidity ^0.8.0;
 import {ICompoundV3} from "./interfaces/ICompoundV3.sol";
 import {ICometRewards} from "./interfaces/ICometRewards.sol";
 import {IERC20} from "@openzeppelin/contracts/interfaces/IERC20.sol";
-import {IAccessControl} from "@openzeppelin/contracts/access/IAccessControl.sol";
 import {SwapLibrary} from "@ensuro/swaplibrary/contracts/SwapLibrary.sol";
 import {IInvestStrategy} from "./interfaces/IInvestStrategy.sol";
 import {StorageSlot} from "@openzeppelin/contracts/utils/StorageSlot.sol";
@@ -36,8 +35,6 @@ contract CompoundV3InvestStrategy is IInvestStrategy {
 
   event SwapConfigChanged(SwapLibrary.SwapConfig oldConfig, SwapLibrary.SwapConfig newConfig);
 
-  // From OZ v5
-  error AccessControlUnauthorizedAccount(address account, bytes32 neededRole);
   error CanBeCalledOnlyThroughDelegateCall();
   error CannotDisconnectWithAssets();
   error NoExtraDataAllowed();
@@ -52,20 +49,14 @@ contract CompoundV3InvestStrategy is IInvestStrategy {
     _;
   }
 
-  modifier onlyRole(bytes32 role) {
-    if (!IAccessControl(address(this)).hasRole(role, msg.sender))
-      revert AccessControlUnauthorizedAccount(msg.sender, role);
-    _;
-  }
-
   constructor(ICompoundV3 cToken_, ICometRewards rewardsManager_) {
     _cToken = cToken_;
     _rewardsManager = rewardsManager_;
     _baseToken = cToken_.baseToken();
   }
 
   function connect(bytes memory initData) external virtual override onlyDelegCall {
-    _setSwapConfigNoCheck(SwapLibrary.SwapConfig(SwapLibrary.SwapProtocol.undefined, 0, bytes("")), initData);
+    _setSwapConfig(SwapLibrary.SwapConfig(SwapLibrary.SwapProtocol.undefined, 0, bytes("")), initData);
   }
 
   function disconnect(bool force) external virtual override onlyDelegCall {
@@ -103,7 +94,7 @@ contract CompoundV3InvestStrategy is IInvestStrategy {
     _cToken.supply(_baseToken, assets);
   }
 
-  function _harvestRewards(uint256 price) internal onlyRole(HARVEST_ROLE) {
+  function _harvestRewards(uint256 price) internal {
     (address reward, , ) = _rewardsManager.rewardConfig(address(_cToken));
     if (reward == address(0)) return;
     _rewardsManager.claim(address(_cToken), address(this), true);
@@ -119,14 +110,7 @@ contract CompoundV3InvestStrategy is IInvestStrategy {
     emit RewardsClaimed(reward, earned, reinvestAmount);
   }
 
-  function _setSwapConfig(bytes memory newSwapConfigAsBytes) internal onlyRole(SWAP_ADMIN_ROLE) {
-    _setSwapConfigNoCheck(_getSwapConfig(address(this)), newSwapConfigAsBytes);
-  }
-
-  function _setSwapConfigNoCheck(
-    SwapLibrary.SwapConfig memory oldSwapConfig,
-    bytes memory newSwapConfigAsBytes
-  ) internal {
+  function _setSwapConfig(SwapLibrary.SwapConfig memory oldSwapConfig, bytes memory newSwapConfigAsBytes) internal {
     SwapLibrary.SwapConfig memory swapConfig = abi.decode(newSwapConfigAsBytes, (SwapLibrary.SwapConfig));
     swapConfig.validate();
     if (abi.encode(swapConfig).length != newSwapConfigAsBytes.length) revert NoExtraDataAllowed();
@@ -140,7 +124,7 @@ contract CompoundV3InvestStrategy is IInvestStrategy {
       uint256 price = abi.decode(params, (uint256));
       _harvestRewards(price);
     } else if (checkedMethod == ForwardMethods.setSwapConfig) {
-      _setSwapConfig(params);
+      _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
     // we add new values in the enum and we forgot to add them here

contracts/MSVBase.sol

@@ -148,6 +148,16 @@ abstract contract MSVBase is IExposeStorage {
     revert OnlyStrategyStorageExposed();
   }
 
+  /**
+   * @dev Checks the caller can execute this forwardToStrategy call, otherwise reverts
+   *
+   * @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.
+   * @param extraData Additional parameters sent to the method.
+   */
+  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.
@@ -162,6 +172,7 @@ abstract contract MSVBase is IExposeStorage {
     uint8 method,
     bytes memory extraData
   ) external virtual returns (bytes memory) {
+    _checkForwardToStrategy(strategyIndex, method, extraData);
     IInvestStrategy strategy = _strategies[strategyIndex];
     if (address(strategy) == address(0)) revert InvalidStrategy();
     return _strategies[strategyIndex].dcForward(method, extraData);

contracts/MultiStrategyERC4626.sol

@@ -26,6 +26,7 @@ contract MultiStrategyERC4626 is MSVBase, PermissionedERC4626 {
   bytes32 public constant STRATEGY_ADMIN_ROLE = keccak256("STRATEGY_ADMIN_ROLE");
   bytes32 public constant QUEUE_ADMIN_ROLE = keccak256("QUEUE_ADMIN_ROLE");
   bytes32 public constant REBALANCER_ROLE = keccak256("REBALANCER_ROLE");
+  bytes32 public constant FORWARD_TO_STRATEGY_ROLE = keccak256("FORWARD_TO_STRATEGY_ROLE");
 
   /// @custom:oz-upgrades-unsafe-allow constructor
   constructor() {
@@ -177,4 +178,35 @@ contract MultiStrategyERC4626 is MSVBase, PermissionedERC4626 {
   ) public override onlyRole(REBALANCER_ROLE) returns (uint256) {
     return super.rebalance(strategyFromIdx, strategyToIdx, amount);
   }
+
+  /**
+   * @dev Returns the AccessControl role required to call forwardToStrategy on a given strategy and method
+   *
+   * @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.
+   * @return role The bytes32 role required to execute the call
+   */
+  function getForwardToStrategyRole(uint8 strategyIndex, uint8 method) public view returns (bytes32 role) {
+    address strategy = address(_strategies[strategyIndex]);
+    return
+      bytes32(bytes20(strategy)) ^
+      (bytes32(bytes1(method)) >> 160) ^
+      (bytes32(bytes1(strategyIndex)) >> 168) ^
+      FORWARD_TO_STRATEGY_ROLE;
+  }
+
+  /// @inheritdoc MSVBase
+  // solhint-disable no-empty-blocks
+  function _checkForwardToStrategy(
+    uint8 strategyIndex,
+    uint8 method,
+    bytes memory
+  )
+    internal
+    view
+    override
+    onlyRole(FORWARD_TO_STRATEGY_ROLE)
+    onlyRole(getForwardToStrategyRole(strategyIndex, method))
+  {}
 }

contracts/SwapStableInvestStrategy.sol

@@ -2,7 +2,6 @@
 pragma solidity ^0.8.0;
 
 import {IERC20Metadata} from "@openzeppelin/contracts/interfaces/IERC20Metadata.sol";
-import {IAccessControl} from "@openzeppelin/contracts/access/IAccessControl.sol";
 import {SwapLibrary} from "@ensuro/swaplibrary/contracts/SwapLibrary.sol";
 import {IInvestStrategy} from "./interfaces/IInvestStrategy.sol";
 import {StorageSlot} from "@openzeppelin/contracts/utils/StorageSlot.sol";
@@ -33,8 +32,6 @@ contract SwapStableInvestStrategy is IInvestStrategy {
 
   event SwapConfigChanged(SwapLibrary.SwapConfig oldConfig, SwapLibrary.SwapConfig newConfig);
 
-  // From OZ v5
-  error AccessControlUnauthorizedAccount(address account, bytes32 neededRole);
   error CanBeCalledOnlyThroughDelegateCall();
   error CannotDisconnectWithAssets();
   error NoExtraDataAllowed();
@@ -48,12 +45,6 @@ contract SwapStableInvestStrategy is IInvestStrategy {
     _;
   }
 
-  modifier onlyRole(bytes32 role) {
-    if (!IAccessControl(address(this)).hasRole(role, msg.sender))
-      revert AccessControlUnauthorizedAccount(msg.sender, role);
-    _;
-  }
-
   /**
    * @dev Constructor of the strategy
    *
@@ -72,7 +63,7 @@ contract SwapStableInvestStrategy is IInvestStrategy {
   }
 
   function connect(bytes memory initData) external virtual override onlyDelegCall {
-    _setSwapConfigNoCheck(SwapLibrary.SwapConfig(SwapLibrary.SwapProtocol.undefined, 0, bytes("")), initData);
+    _setSwapConfig(SwapLibrary.SwapConfig(SwapLibrary.SwapProtocol.undefined, 0, bytes("")), initData);
   }
 
   function disconnect(bool force) external virtual override onlyDelegCall {
@@ -135,14 +126,7 @@ contract SwapStableInvestStrategy is IInvestStrategy {
     swapConfig.exactInput(address(_asset), address(_investAsset), assets, _price);
   }
 
-  function _setSwapConfig(bytes memory newSwapConfigAsBytes) internal onlyRole(SWAP_ADMIN_ROLE) {
-    _setSwapConfigNoCheck(_getSwapConfig(address(this)), newSwapConfigAsBytes);
-  }
-
-  function _setSwapConfigNoCheck(
-    SwapLibrary.SwapConfig memory oldSwapConfig,
-    bytes memory newSwapConfigAsBytes
-  ) internal {
+  function _setSwapConfig(SwapLibrary.SwapConfig memory oldSwapConfig, bytes memory newSwapConfigAsBytes) internal {
     SwapLibrary.SwapConfig memory swapConfig = abi.decode(newSwapConfigAsBytes, (SwapLibrary.SwapConfig));
     swapConfig.validate();
     if (abi.encode(swapConfig).length != newSwapConfigAsBytes.length) revert NoExtraDataAllowed();
@@ -153,7 +137,7 @@ contract SwapStableInvestStrategy is IInvestStrategy {
   function forwardEntryPoint(uint8 method, bytes memory params) external onlyDelegCall returns (bytes memory) {
     ForwardMethods checkedMethod = ForwardMethods(method);
     if (checkedMethod == ForwardMethods.setSwapConfig) {
-      _setSwapConfig(params);
+      _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
     // we add new values in the enum and we forgot to add them here

contracts/SingleStrategyERC4626.sol → contracts/mock/SingleStrategyERC4626.sol

@@ -7,10 +7,10 @@ import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 import {StorageSlot} from "@openzeppelin/contracts/utils/StorageSlot.sol";
 import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
-import {PermissionedERC4626} from "./PermissionedERC4626.sol";
-import {IInvestStrategy} from "./interfaces/IInvestStrategy.sol";
-import {IExposeStorage} from "./interfaces/IExposeStorage.sol";
-import {InvestStrategyClient} from "./InvestStrategyClient.sol";
+import {PermissionedERC4626} from "../PermissionedERC4626.sol";
+import {IInvestStrategy} from "../interfaces/IInvestStrategy.sol";
+import {IExposeStorage} from "../interfaces/IExposeStorage.sol";
+import {InvestStrategyClient} from "../InvestStrategyClient.sol";
 
 /**
  * @title SingleStrategyERC4626
@@ -22,6 +22,9 @@ import {InvestStrategyClient} from "./InvestStrategyClient.sol";
  *      The code of the IInvestStrategy is called using delegatecall, so it has full control over the assets and
  *      storage of this contract, so you must be very careful the kind of IInvestStrategy is plugged.
  *
+ * WARNING: this contract isn't fully tested and has known errors (lack of access validation on forwardToStrategy),
+ *          so this is not intended to be used in production. Is just for the purpose of testing strategies.
+ *
  * @custom:security-contact [email protected]
  * @author Ensuro
  */

hardhat.config.js

@@ -15,6 +15,7 @@ module.exports = {
         enabled: true,
         runs: 200,
       },
+      evmVersion: "cancun",
     },
   },
   networks: {

package.json

@@ -14,7 +14,7 @@
     "hardhat": "^2.22.17",
     "hardhat-contract-sizer": "^2.10.0",
     "prettier": "^3.4.2",
-    "solhint": "^5.0.3",
+    "solhint": "^5.0.4",
     "solhint-plugin-prettier": "0.1.0"
   },
   "dependencies": {