From 5be15b9d0b791279daf947388a53e050c826a03b Mon Sep 17 00:00:00 2001 From: Jayesh Yadav Date: Fri, 29 May 2026 16:59:37 +0530 Subject: [PATCH] feat(roles): separate curator role from owner --- README.md | 6 +- src/facets/AllocatorFacet.sol | 14 +- src/facets/HarvestFacet.sol | 6 +- src/facets/RolesFacet.sol | 34 ++++ src/libraries/LibRoles.sol | 45 +++++ test/unit/Roles.t.sol | 348 ++++++++++++++++++++++++++++++++++ 6 files changed, 445 insertions(+), 8 deletions(-) create mode 100644 src/facets/RolesFacet.sol create mode 100644 src/libraries/LibRoles.sol create mode 100644 test/unit/Roles.t.sol diff --git a/README.md b/README.md index 8c154b1..5c29a6c 100644 --- a/README.md +++ b/README.md @@ -32,10 +32,12 @@ All facet state is isolated using EIP-7201 namespaced storage — no storage col | Role | Permissions | |------|-------------| -| **Owner** | Add / remove / replace facets via `diamondCut` | -| **Curator** | Register strategies, set allocations, trigger rebalance and harvest | +| **Owner** | Add / remove / replace facets via `diamondCut`, register / remove strategies, set caps + idle floor, configure fees, appoint / revoke curators | +| **Curator** | Set allocations, trigger rebalance and harvest — **within** the bounds the owner sets | | **User** | Deposit / withdraw underlying asset | +The curator is a deliberately low-privilege operational key: it can move capital only between owner-allow-listed strategies and only within owner-set caps and the idle floor — it can never upgrade facets, change fees, or withdraw funds. This separation is what makes it safe to delegate day-to-day rebalancing to an automated operator (e.g. an off-chain agent) without exposing the vault to it. Owners are implicitly curators, so governance can always operate the vault directly. + ## Strategies | Strategy | Protocol | Yield Source | diff --git a/src/facets/AllocatorFacet.sol b/src/facets/AllocatorFacet.sol index 94bf1b8..73056f4 100644 --- a/src/facets/AllocatorFacet.sol +++ b/src/facets/AllocatorFacet.sol @@ -5,6 +5,7 @@ import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import { IERC4626 } from "@openzeppelin/contracts/interfaces/IERC4626.sol"; import { LibDiamond } from "../libraries/LibDiamond.sol"; +import { LibRoles } from "../libraries/LibRoles.sol"; import { LibAllocator } from "../libraries/LibAllocator.sol"; /// @title AllocatorFacet @@ -35,8 +36,11 @@ contract AllocatorFacet { event Rebalanced(uint256 totalAssets, uint256 idleAfter); // ----------------------------------------------------------------------- - // Curator-gated setters + // Owner-gated governance / risk bounds // ----------------------------------------------------------------------- + // Registering strategies and setting caps / idle floor define the bounds the + // curator must operate within, so they stay owner-only. `setAllocation` (a + // policy choice within those bounds) and `rebalance` are curator-gated below. function registerStrategy(bytes32 strategyId, LibAllocator.StrategyConfig calldata config) external { LibDiamond.enforceIsContractOwner(); @@ -77,8 +81,12 @@ contract AllocatorFacet { emit StrategyRemoved(strategyId); } + // ----------------------------------------------------------------------- + // Curator-gated operations (allocation policy within owner-set bounds) + // ----------------------------------------------------------------------- + function setAllocation(bytes32[] calldata strategyIds, uint16[] calldata bps) external { - LibDiamond.enforceIsContractOwner(); + LibRoles.enforceIsCurator(); if (strategyIds.length != bps.length) revert AllocationLengthMismatch(strategyIds.length, bps.length); LibAllocator.AllocatorStorage storage s = LibAllocator.allocatorStorage(); @@ -139,7 +147,7 @@ contract AllocatorFacet { /// cap is enforced upstream in `setAllocation`; the idle-reserve floor /// follows automatically from `total + idleReserveBps ≤ 10_000`. function rebalance() external { - LibDiamond.enforceIsContractOwner(); + LibRoles.enforceIsCurator(); LibAllocator.AllocatorStorage storage s = LibAllocator.allocatorStorage(); if (block.number <= uint256(s.lastRebalanceBlock)) { revert RebalanceTooSoon(uint256(s.lastRebalanceBlock), block.number); diff --git a/src/facets/HarvestFacet.sol b/src/facets/HarvestFacet.sol index aa579bb..4f533a9 100644 --- a/src/facets/HarvestFacet.sol +++ b/src/facets/HarvestFacet.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; -import { LibDiamond } from "../libraries/LibDiamond.sol"; +import { LibRoles } from "../libraries/LibRoles.sol"; import { LibAllocator } from "../libraries/LibAllocator.sol"; /// @title HarvestFacet @@ -23,7 +23,7 @@ contract HarvestFacet { /// `harvestSelector` — useful for protocols where rewards auto-accrue /// (Aave aTokens, Morpho lender shares) and no explicit claim is needed. function harvest(bytes32 strategyId) external { - LibDiamond.enforceIsContractOwner(); + LibRoles.enforceIsCurator(); LibAllocator.StrategyConfig memory cfg = LibAllocator.allocatorStorage().configs[strategyId]; if (!cfg.active) revert StrategyNotRegistered(strategyId); if (cfg.harvestSelector != bytes4(0)) { @@ -42,7 +42,7 @@ contract HarvestFacet { /// @notice Harvest every registered strategy in registration order. function harvestAll() external { - LibDiamond.enforceIsContractOwner(); + LibRoles.enforceIsCurator(); LibAllocator.AllocatorStorage storage s = LibAllocator.allocatorStorage(); uint256 n = s.strategyIds.length; for (uint256 i; i < n; i++) { diff --git a/src/facets/RolesFacet.sol b/src/facets/RolesFacet.sol new file mode 100644 index 0000000..2af1227 --- /dev/null +++ b/src/facets/RolesFacet.sol @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import { LibDiamond } from "../libraries/LibDiamond.sol"; +import { LibRoles } from "../libraries/LibRoles.sol"; + +/// @title RolesFacet +/// @notice Owner-gated management of the curator role. The owner appoints or +/// revokes curators; curators get day-to-day operational authority +/// (allocation + rebalance + harvest) bounded by the risk parameters the +/// owner controls. Curators can never upgrade facets, change fees, or +/// move funds outside the allow-listed strategies. +contract RolesFacet { + error ZeroAddress(); + + event CuratorSet(address indexed account, bool enabled); + + /// @notice Grant or revoke the curator role for `account`. + /// @dev Owner-only. The owner is always implicitly a curator (see + /// LibRoles.isCurator), so this controls *additional* operational keys — + /// for example an off-chain agent that autonomously rebalances. + function setCurator(address account, bool enabled) external { + LibDiamond.enforceIsContractOwner(); + if (account == address(0)) revert ZeroAddress(); + LibRoles.rolesStorage().isCurator[account] = enabled; + emit CuratorSet(account, enabled); + } + + /// @notice True if `account` may perform curator-gated operations. + /// @dev Returns true for the owner as well, since owner ≥ curator. + function isCurator(address account) external view returns (bool) { + return LibRoles.isCurator(account); + } +} diff --git a/src/libraries/LibRoles.sol b/src/libraries/LibRoles.sol new file mode 100644 index 0000000..2168823 --- /dev/null +++ b/src/libraries/LibRoles.sol @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import { LibDiamond } from "./LibDiamond.sol"; + +/// @title LibRoles +/// @notice Namespaced storage for the vault's operational role layer. Separates +/// the low-privilege **curator** seat (day-to-day allocation + harvest) +/// from the high-privilege **owner** seat (facet upgrades, risk bounds, +/// fee config). The curator key can operate the vault within bounds the +/// owner sets but can never drain it, upgrade it, or change fees — which +/// is what makes it safe to hand to an automated (e.g. AI) operator. +/// @dev Roles state lives in an EIP-7201 namespaced slot so it cannot collide +/// with the ERC-4626 surface storage on Vault.sol, LibDiamond's selector +/// table, or any other facet's namespace. +/// keccak256(abi.encode(uint256(keccak256("vaultrouter.storage.roles")) - 1)) & ~bytes32(uint256(0xff)) +library LibRoles { + bytes32 internal constant ROLES_STORAGE_SLOT = 0x72812988d549c1f62ecdf8218c688f5047bef5695066f17d8d1060ecc0962300; + + error NotCurator(address caller); + + /// @custom:storage-location erc7201:vaultrouter.storage.roles + struct RolesStorage { + mapping(address => bool) isCurator; + } + + function rolesStorage() internal pure returns (RolesStorage storage s) { + bytes32 slot = ROLES_STORAGE_SLOT; + assembly { + s.slot := slot + } + } + + /// @notice True if `account` may perform curator-gated operations. + /// @dev The owner is implicitly a curator (owner ≥ curator), so governance + /// can always operate the vault even before any curator is appointed. + function isCurator(address account) internal view returns (bool) { + return account == LibDiamond.contractOwner() || rolesStorage().isCurator[account]; + } + + /// @notice Reverts unless `msg.sender` is the owner or an appointed curator. + function enforceIsCurator() internal view { + if (!isCurator(msg.sender)) revert NotCurator(msg.sender); + } +} diff --git a/test/unit/Roles.t.sol b/test/unit/Roles.t.sol new file mode 100644 index 0000000..3e39bd1 --- /dev/null +++ b/test/unit/Roles.t.sol @@ -0,0 +1,348 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import { Test } from "forge-std/Test.sol"; +import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; + +import { Vault } from "../../src/Vault.sol"; +import { IDiamond } from "../../src/interfaces/IDiamond.sol"; +import { IDiamondCut } from "../../src/interfaces/IDiamondCut.sol"; +import { IDiamondLoupe } from "../../src/interfaces/IDiamondLoupe.sol"; +import { IERC173 } from "../../src/interfaces/IERC173.sol"; +import { DiamondCutFacet } from "../../src/facets/DiamondCutFacet.sol"; +import { DiamondLoupeFacet } from "../../src/facets/DiamondLoupeFacet.sol"; +import { OwnershipFacet } from "../../src/facets/OwnershipFacet.sol"; +import { RolesFacet } from "../../src/facets/RolesFacet.sol"; +import { IdleStrategyFacet } from "../../src/facets/strategies/IdleStrategyFacet.sol"; +import { AllocatorFacet } from "../../src/facets/AllocatorFacet.sol"; +import { HarvestFacet } from "../../src/facets/HarvestFacet.sol"; +import { LibAllocator } from "../../src/libraries/LibAllocator.sol"; +import { LibRoles } from "../../src/libraries/LibRoles.sol"; +import { LibDiamond } from "../../src/libraries/LibDiamond.sol"; + +import { MockProtocol } from "../mocks/MockProtocol.sol"; +import { MockStrategyFacet } from "../mocks/MockStrategyFacet.sol"; + +contract MockUSDC is ERC20 { + constructor() ERC20("USD Coin", "USDC") { } + + function mint(address to, uint256 amount) external { + _mint(to, amount); + } + + function decimals() public pure override returns (uint8) { + return 6; + } +} + +/// @notice Exercises the owner/curator role split: owners govern risk bounds and +/// appoint curators; curators run allocation/rebalance/harvest within +/// those bounds but cannot govern or upgrade. +contract RolesTest is Test { + MockUSDC internal usdc; + Vault internal vault; + MockProtocol internal mockProtocol; + + address internal owner = makeAddr("owner"); + address internal curator = makeAddr("curator"); + address internal stranger = makeAddr("stranger"); + address internal alice = makeAddr("alice"); + + bytes32 internal constant MOCK_ID = bytes32("mock"); + + event CuratorSet(address indexed account, bool enabled); + + function setUp() public { + usdc = new MockUSDC(); + mockProtocol = new MockProtocol(IERC20(address(usdc))); + vault = _deployVault(); + + vm.prank(owner); + MockStrategyFacet(address(vault)).mockSetProtocol(mockProtocol); + vm.prank(owner); + AllocatorFacet(address(vault)).registerStrategy(MOCK_ID, _mockStrategyConfig(0)); + } + + // ----------------------------------------------------------------------- + // setCurator + // ----------------------------------------------------------------------- + + function test_Owner_AppointsCurator() public { + assertFalse(RolesFacet(address(vault)).isCurator(curator)); + + vm.expectEmit(true, false, false, true, address(vault)); + emit CuratorSet(curator, true); + vm.prank(owner); + RolesFacet(address(vault)).setCurator(curator, true); + + assertTrue(RolesFacet(address(vault)).isCurator(curator)); + } + + function test_Owner_RevokesCurator() public { + vm.prank(owner); + RolesFacet(address(vault)).setCurator(curator, true); + assertTrue(RolesFacet(address(vault)).isCurator(curator)); + + vm.prank(owner); + RolesFacet(address(vault)).setCurator(curator, false); + assertFalse(RolesFacet(address(vault)).isCurator(curator)); + } + + function test_SetCurator_RevertsForNonOwner() public { + vm.prank(stranger); + vm.expectRevert(abi.encodeWithSelector(LibDiamond.NotContractOwner.selector, stranger, owner)); + RolesFacet(address(vault)).setCurator(curator, true); + } + + function test_SetCurator_RevertsOnZeroAddress() public { + vm.prank(owner); + vm.expectRevert(RolesFacet.ZeroAddress.selector); + RolesFacet(address(vault)).setCurator(address(0), true); + } + + function test_OwnerIsImplicitlyCurator() public view { + assertTrue(RolesFacet(address(vault)).isCurator(owner)); + } + + // ----------------------------------------------------------------------- + // Curator can operate within bounds + // ----------------------------------------------------------------------- + + function test_Curator_CanSetAllocationAndRebalance() public { + _depositToVault(alice, 1000 * 1e6); + vm.prank(owner); + RolesFacet(address(vault)).setCurator(curator, true); + + bytes32[] memory ids = new bytes32[](1); + uint16[] memory bps = new uint16[](1); + ids[0] = MOCK_ID; + bps[0] = 8000; + + vm.prank(curator); + AllocatorFacet(address(vault)).setAllocation(ids, bps); + + vm.roll(block.number + 1); + vm.prank(curator); + AllocatorFacet(address(vault)).rebalance(); + + assertEq(mockProtocol.balanceOf(address(vault)), 800 * 1e6, "curator deployed 80%"); + } + + function test_Curator_CanHarvest() public { + vm.prank(owner); + RolesFacet(address(vault)).setCurator(curator, true); + + vm.prank(curator); + HarvestFacet(address(vault)).harvest(MOCK_ID); // no-op-ish but must not revert on auth + } + + // ----------------------------------------------------------------------- + // Non-curators are rejected on operations + // ----------------------------------------------------------------------- + + function test_Stranger_CannotSetAllocation() public { + bytes32[] memory ids = new bytes32[](1); + uint16[] memory bps = new uint16[](1); + ids[0] = MOCK_ID; + bps[0] = 5000; + + vm.prank(stranger); + vm.expectRevert(abi.encodeWithSelector(LibRoles.NotCurator.selector, stranger)); + AllocatorFacet(address(vault)).setAllocation(ids, bps); + } + + function test_Stranger_CannotRebalance() public { + vm.prank(stranger); + vm.expectRevert(abi.encodeWithSelector(LibRoles.NotCurator.selector, stranger)); + AllocatorFacet(address(vault)).rebalance(); + } + + function test_Stranger_CannotHarvest() public { + vm.prank(stranger); + vm.expectRevert(abi.encodeWithSelector(LibRoles.NotCurator.selector, stranger)); + HarvestFacet(address(vault)).harvest(MOCK_ID); + } + + function test_RevokedCurator_CannotRebalance() public { + vm.startPrank(owner); + RolesFacet(address(vault)).setCurator(curator, true); + RolesFacet(address(vault)).setCurator(curator, false); + vm.stopPrank(); + + vm.prank(curator); + vm.expectRevert(abi.encodeWithSelector(LibRoles.NotCurator.selector, curator)); + AllocatorFacet(address(vault)).rebalance(); + } + + // ----------------------------------------------------------------------- + // Curator cannot cross into owner-only governance + // ----------------------------------------------------------------------- + + function test_Curator_CannotRegisterStrategy() public { + vm.prank(owner); + RolesFacet(address(vault)).setCurator(curator, true); + + vm.prank(curator); + vm.expectRevert(abi.encodeWithSelector(LibDiamond.NotContractOwner.selector, curator, owner)); + AllocatorFacet(address(vault)).registerStrategy(bytes32("x"), _mockStrategyConfig(0)); + } + + function test_Curator_CannotSetStrategyCap() public { + vm.prank(owner); + RolesFacet(address(vault)).setCurator(curator, true); + + vm.prank(curator); + vm.expectRevert(abi.encodeWithSelector(LibDiamond.NotContractOwner.selector, curator, owner)); + AllocatorFacet(address(vault)).setStrategyCap(MOCK_ID, 5000); + } + + function test_Curator_CannotSetIdleReserve() public { + vm.prank(owner); + RolesFacet(address(vault)).setCurator(curator, true); + + vm.prank(curator); + vm.expectRevert(abi.encodeWithSelector(LibDiamond.NotContractOwner.selector, curator, owner)); + AllocatorFacet(address(vault)).setIdleReserve(1000); + } + + function test_Curator_CannotAppointCurator() public { + vm.prank(owner); + RolesFacet(address(vault)).setCurator(curator, true); + + vm.prank(curator); + vm.expectRevert(abi.encodeWithSelector(LibDiamond.NotContractOwner.selector, curator, owner)); + RolesFacet(address(vault)).setCurator(stranger, true); + } + + // ----------------------------------------------------------------------- + // helpers + // ----------------------------------------------------------------------- + + function _deployVault() internal returns (Vault) { + DiamondCutFacet cut = new DiamondCutFacet(); + DiamondLoupeFacet loupe = new DiamondLoupeFacet(); + OwnershipFacet ownership = new OwnershipFacet(); + RolesFacet roles = new RolesFacet(); + IdleStrategyFacet idle = new IdleStrategyFacet(); + AllocatorFacet allocator = new AllocatorFacet(); + HarvestFacet harvest = new HarvestFacet(); + MockStrategyFacet mock = new MockStrategyFacet(); + + IDiamond.FacetCut[] memory cuts = new IDiamond.FacetCut[](8); + cuts[0] = IDiamond.FacetCut({ + facetAddress: address(cut), action: IDiamond.FacetCutAction.Add, functionSelectors: _diamondCutSelectors() + }); + cuts[1] = IDiamond.FacetCut({ + facetAddress: address(loupe), + action: IDiamond.FacetCutAction.Add, + functionSelectors: _diamondLoupeSelectors() + }); + cuts[2] = IDiamond.FacetCut({ + facetAddress: address(ownership), + action: IDiamond.FacetCutAction.Add, + functionSelectors: _ownershipSelectors() + }); + cuts[3] = IDiamond.FacetCut({ + facetAddress: address(roles), action: IDiamond.FacetCutAction.Add, functionSelectors: _rolesSelectors() + }); + cuts[4] = IDiamond.FacetCut({ + facetAddress: address(idle), action: IDiamond.FacetCutAction.Add, functionSelectors: _idleSelectors() + }); + cuts[5] = IDiamond.FacetCut({ + facetAddress: address(allocator), + action: IDiamond.FacetCutAction.Add, + functionSelectors: _allocatorSelectors() + }); + cuts[6] = IDiamond.FacetCut({ + facetAddress: address(harvest), action: IDiamond.FacetCutAction.Add, functionSelectors: _harvestSelectors() + }); + cuts[7] = IDiamond.FacetCut({ + facetAddress: address(mock), action: IDiamond.FacetCutAction.Add, functionSelectors: _mockSelectors() + }); + + return new Vault(IERC20(address(usdc)), "Vault Router", "vUSDC", owner, cuts, address(0), ""); + } + + function _depositToVault(address from, uint256 amount) internal { + usdc.mint(from, amount); + vm.startPrank(from); + usdc.approve(address(vault), amount); + vault.deposit(amount, from); + vm.stopPrank(); + } + + function _mockStrategyConfig(uint16 capBps) internal pure returns (LibAllocator.StrategyConfig memory) { + return LibAllocator.StrategyConfig({ + totalAssetsSelector: MockStrategyFacet.mockTotalAssets.selector, + depositSelector: MockStrategyFacet.mockDeposit.selector, + withdrawSelector: MockStrategyFacet.mockWithdraw.selector, + harvestSelector: MockStrategyFacet.mockHarvest.selector, + capBps: capBps, + active: false + }); + } + + function _diamondCutSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](1); + s[0] = IDiamondCut.diamondCut.selector; + } + + function _diamondLoupeSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](4); + s[0] = IDiamondLoupe.facets.selector; + s[1] = IDiamondLoupe.facetFunctionSelectors.selector; + s[2] = IDiamondLoupe.facetAddresses.selector; + s[3] = IDiamondLoupe.facetAddress.selector; + } + + function _ownershipSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](2); + s[0] = IERC173.owner.selector; + s[1] = IERC173.transferOwnership.selector; + } + + function _rolesSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](2); + s[0] = RolesFacet.setCurator.selector; + s[1] = RolesFacet.isCurator.selector; + } + + function _idleSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](1); + s[0] = IdleStrategyFacet.idleTotalAssets.selector; + } + + function _allocatorSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](13); + s[0] = AllocatorFacet.registerStrategy.selector; + s[1] = AllocatorFacet.removeStrategy.selector; + s[2] = AllocatorFacet.setAllocation.selector; + s[3] = AllocatorFacet.setIdleReserve.selector; + s[4] = AllocatorFacet.setStrategyCap.selector; + s[5] = AllocatorFacet.setGlobalStrategyCap.selector; + s[6] = AllocatorFacet.rebalance.selector; + s[7] = AllocatorFacet.strategies.selector; + s[8] = AllocatorFacet.strategyConfig.selector; + s[9] = AllocatorFacet.targetAllocation.selector; + s[10] = AllocatorFacet.idleReserveBps.selector; + s[11] = AllocatorFacet.strategyTotalAssets.selector; + s[12] = AllocatorFacet.idleAssets.selector; + } + + function _harvestSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](2); + s[0] = HarvestFacet.harvest.selector; + s[1] = HarvestFacet.harvestAll.selector; + } + + function _mockSelectors() internal pure returns (bytes4[] memory s) { + s = new bytes4[](6); + s[0] = MockStrategyFacet.mockSetProtocol.selector; + s[1] = MockStrategyFacet.mockProtocol.selector; + s[2] = MockStrategyFacet.mockTotalAssets.selector; + s[3] = MockStrategyFacet.mockDeposit.selector; + s[4] = MockStrategyFacet.mockWithdraw.selector; + s[5] = MockStrategyFacet.mockHarvest.selector; + } +}