From c5ba0cb97479f9088c3643163f7218f8d9a19885 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Wed, 20 Sep 2023 15:46:50 +0200 Subject: [PATCH 1/2] fix(roles): single risk manager --- src/MetaMorpho.sol | 30 +++++++++--------------------- src/interfaces/IMetaMorpho.sol | 4 ++-- src/libraries/ConstantsLib.sol | 6 ------ src/libraries/EventsLib.sol | 4 ++++ test/forge/RoleTest.sol | 9 ++++++--- test/forge/helpers/BaseTest.sol | 2 +- test/hardhat/MetaMorpho.spec.ts | 2 +- 7 files changed, 23 insertions(+), 34 deletions(-) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index 97cdf6e9..b6baad94 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -41,7 +41,8 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho { /* STORAGE */ - mapping(address => uint256) internal _roleOf; + address public riskManager; + mapping(address => bool) internal _isAllocator; mapping(Id => MarketConfig) public config; mapping(Id => Pending) public pendingCap; @@ -85,7 +86,7 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho { /* MODIFIERS */ modifier onlyRiskManager() { - require(isRiskManager(_msgSender()), ErrorsLib.NOT_RISK_MANAGER); + require(_msgSender() == riskManager || _msgSender() == owner(), ErrorsLib.NOT_RISK_MANAGER); _; } @@ -105,12 +106,14 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho { /* ONLY OWNER FUNCTIONS */ - function setIsRiskManager(address newRiskManager, bool newIsRiskManager) external onlyOwner { - _setRole(newRiskManager, RISK_MANAGER_ROLE, newIsRiskManager); + function setRiskManager(address newRiskManager) external onlyOwner { + riskManager = newRiskManager; } function setIsAllocator(address newAllocator, bool newIsAllocator) external onlyOwner { - _setRole(newAllocator, ALLOCATOR_ROLE, newIsAllocator); + _isAllocator[newAllocator] = newIsAllocator; + + emit EventsLib.SetIsAllocator(newAllocator, newIsAllocator); } function submitTimelock(uint256 newTimelock) external onlyOwner { @@ -247,12 +250,8 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho { /* PUBLIC */ - function isRiskManager(address target) public view returns (bool) { - return _hasRole(target, RISK_MANAGER_ROLE); - } - function isAllocator(address target) public view returns (bool) { - return _hasRole(target, ALLOCATOR_ROLE); + return _isAllocator[target] || _msgSender() == riskManager || _msgSender() == owner(); } /* ERC4626 (PUBLIC) */ @@ -603,15 +602,4 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho { ); } } - - function _hasRole(address target, uint256 role) internal view returns (bool) { - return _roleOf[target] >= role || _msgSender() == owner(); - } - - function _setRole(address target, uint256 role, bool hasRole) internal { - if (hasRole) _roleOf[target] = role; - else delete _roleOf[target]; - - emit EventsLib.SetRole(target, role); - } } diff --git a/src/interfaces/IMetaMorpho.sol b/src/interfaces/IMetaMorpho.sol index bbf6e165..9078ac6b 100644 --- a/src/interfaces/IMetaMorpho.sol +++ b/src/interfaces/IMetaMorpho.sol @@ -22,8 +22,8 @@ struct MarketAllocation { interface IMetaMorpho is IERC4626 { function MORPHO() external view returns (IMorpho); + function riskManager() external view returns (address); function isAllocator(address target) external view returns (bool); - function isRiskManager(address target) external view returns (bool); function fee() external view returns (uint96); function feeRecipient() external view returns (address); @@ -48,7 +48,7 @@ interface IMetaMorpho is IERC4626 { function pendingFee() external view returns (uint192 value, uint64 submittedAt); function setIsAllocator(address newAllocator, bool newIsAllocator) external; - function setIsRiskManager(address newRiskManager, bool newIsRiskManager) external; + function setRiskManager(address newRiskManager) external; function setFeeRecipient(address newFeeRecipient) external; function setSupplyQueue(Id[] calldata newSupplyQueue) external; diff --git a/src/libraries/ConstantsLib.sol b/src/libraries/ConstantsLib.sol index abdd78fe..51438831 100644 --- a/src/libraries/ConstantsLib.sol +++ b/src/libraries/ConstantsLib.sol @@ -11,12 +11,6 @@ uint256 constant MAX_TIMELOCK = 2 weeks; /// @dev OpenZeppelin's decimals offset used in MetaMorpho's ERC4626 implementation. uint8 constant DECIMALS_OFFSET = 6; -/// @dev The role assigned to risk managers. Must be greater than the allocator role. -uint256 constant RISK_MANAGER_ROLE = 2; - -/// @dev The role assigned to allocators. -uint256 constant ALLOCATOR_ROLE = 1; - /// @dev The maximum supply/withdraw queue size ensuring the cost of depositing/withdrawing from the vault fits in a /// block. uint256 constant MAX_QUEUE_SIZE = 64; diff --git a/src/libraries/EventsLib.sol b/src/libraries/EventsLib.sol index 7c816940..a4c97ce8 100644 --- a/src/libraries/EventsLib.sol +++ b/src/libraries/EventsLib.sol @@ -27,4 +27,8 @@ library EventsLib { /// @notice Emitted when the vault's last total assets is updated. /// @param totalAssets The total amount of assets this vault manages. event UpdateLastTotalAssets(uint256 totalAssets); + + event SetRiskManager(address indexed riskManager); + + event SetIsAllocator(address indexed allocator, bool isAllocator); } diff --git a/test/forge/RoleTest.sol b/test/forge/RoleTest.sol index 3612b5f4..76e3ff35 100644 --- a/test/forge/RoleTest.sol +++ b/test/forge/RoleTest.sol @@ -8,6 +8,7 @@ contract RoleTest is BaseTest { function testOwnerFunctionsShouldRevertWhenNotOwner(address caller) public { vm.assume(caller != vault.owner()); + vm.startPrank(caller); vm.expectRevert("Ownable: caller is not the owner"); @@ -17,7 +18,7 @@ contract RoleTest is BaseTest { vault.acceptTimelock(); vm.expectRevert("Ownable: caller is not the owner"); - vault.setIsRiskManager(caller, true); + vault.setRiskManager(caller); vm.expectRevert("Ownable: caller is not the owner"); vault.setIsAllocator(caller, true); @@ -35,7 +36,8 @@ contract RoleTest is BaseTest { } function testRiskManagerFunctionsShouldRevertWhenNotRiskManagerAndNotOwner(address caller) public { - vm.assume(caller != vault.owner() && !vault.isRiskManager(caller)); + vm.assume(caller != vault.owner() && caller != vault.riskManager()); + vm.startPrank(caller); vm.expectRevert(bytes(ErrorsLib.NOT_RISK_MANAGER)); @@ -48,7 +50,8 @@ contract RoleTest is BaseTest { } function testAllocatorFunctionsShouldRevertWhenNotAllocatorAndNotRiskManagerAndNotOwner(address caller) public { - vm.assume(caller != vault.owner() && !vault.isRiskManager(caller) && !vault.isAllocator(caller)); + vm.assume(!vault.isAllocator(caller)); + vm.startPrank(caller); Id[] memory supplyQueue; diff --git a/test/forge/helpers/BaseTest.sol b/test/forge/helpers/BaseTest.sol index 3b805dc9..c99fa99f 100644 --- a/test/forge/helpers/BaseTest.sol +++ b/test/forge/helpers/BaseTest.sol @@ -92,7 +92,7 @@ contract BaseTest is Test { vm.startPrank(OWNER); vault = new MetaMorpho(address(morpho), TIMELOCK, address(borrowableToken), "MetaMorpho Vault", "MMV"); - vault.setIsRiskManager(RISK_MANAGER, true); + vault.setRiskManager(RISK_MANAGER); vault.setIsAllocator(ALLOCATOR, true); vm.stopPrank(); diff --git a/test/hardhat/MetaMorpho.spec.ts b/test/hardhat/MetaMorpho.spec.ts index a60981d2..fdd09a8d 100644 --- a/test/hardhat/MetaMorpho.spec.ts +++ b/test/hardhat/MetaMorpho.spec.ts @@ -125,7 +125,7 @@ describe("MetaMorpho", () => { await collateral.connect(user).approve(morphoAddress, MaxUint256); } - await metaMorpho.setIsRiskManager(riskManager.address, true); + await metaMorpho.setRiskManager(riskManager.address); await metaMorpho.setIsAllocator(allocator.address, true); await metaMorpho.submitTimelock(0); From aaf95bcb833415eccce6d18b4cdf3679eaa44bb8 Mon Sep 17 00:00:00 2001 From: Rubilmax Date: Wed, 20 Sep 2023 17:03:05 +0200 Subject: [PATCH 2/2] fix(setter): log event --- src/MetaMorpho.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol index b6baad94..48d3cd40 100644 --- a/src/MetaMorpho.sol +++ b/src/MetaMorpho.sol @@ -108,6 +108,8 @@ contract MetaMorpho is ERC4626, Ownable2Step, IMetaMorpho { function setRiskManager(address newRiskManager) external onlyOwner { riskManager = newRiskManager; + + emit EventsLib.SetRiskManager(newRiskManager); } function setIsAllocator(address newAllocator, bool newIsAllocator) external onlyOwner {