Merge pull request #94 from morpho-labs/fix/single-risk-manager

fix(roles): single risk manager
morpho-org · Sep 20, 2023 · fb52a32 · fb52a32
2 parents 78af18a + aaf95bc
commit fb52a32
Show file tree

Hide file tree

Showing 7 changed files with 25 additions and 34 deletions.
diff --git a/src/MetaMorpho.sol b/src/MetaMorpho.sol
@@ -42,7 +42,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, 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;
@@ -87,7 +88,7 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, IMetaMorpho {
     /* MODIFIERS */
 
     modifier onlyRiskManager() {
-        require(isRiskManager(_msgSender()), ErrorsLib.NOT_RISK_MANAGER);
+        require(_msgSender() == riskManager || _msgSender() == owner(), ErrorsLib.NOT_RISK_MANAGER);
 
         _;
     }
@@ -107,12 +108,16 @@ contract MetaMorpho is ERC4626, ERC20Permit, 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;
+
+        emit EventsLib.SetRiskManager(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 {
@@ -250,12 +255,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, 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) */
@@ -612,15 +613,4 @@ contract MetaMorpho is ERC4626, ERC20Permit, 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -91,7 +91,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
@@ -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);