Delegated Manager System audit fixes and upgrades [SIM-55] [SIM-156] (#21)

* add mutual upgrade to owner fee split update

* add changelog to MutualUpgradeV2, add test case to delegatedManager

* audit fix 5.3, add controller and checks for setTokens

* audit fix 5.4.1, make setTokenFactory and managerCore immutable, add address zero check to setMethodologist

Author: pblivin0x

contracts/factories/DelegatedManagerFactory.sol

@@ -24,6 +24,7 @@ import { ISetToken } from "@setprotocol/set-protocol-v2/contracts/interfaces/ISe
 
 import { AddressArrayUtils } from "../lib/AddressArrayUtils.sol";
 import { DelegatedManager } from "../manager/DelegatedManager.sol";
+import { IController } from "../interfaces/IController.sol";
 import { IDelegatedManager } from "../interfaces/IDelegatedManager.sol";
 import { IManagerCore } from "../interfaces/IManagerCore.sol";
 import { ISetTokenCreator } from "../interfaces/ISetTokenCreator.sol";
@@ -46,6 +47,7 @@ contract DelegatedManagerFactory {
     struct InitializeParams{
         address deployer;
         address owner;
+        address methodologist;
         IDelegatedManager manager;
         bool isPending;
     }
@@ -79,8 +81,11 @@ contract DelegatedManagerFactory {
     // ManagerCore address
     IManagerCore public immutable managerCore;
 
+    // Controller address
+    IController public immutable controller;
+
     // SetTokenFactory address
-    ISetTokenCreator public setTokenFactory;
+    ISetTokenCreator public immutable setTokenFactory;
 
     // Mapping which stores manager creation metadata between creation and initialization steps
     mapping(ISetToken=>InitializeParams) public initializeState;
@@ -90,15 +95,18 @@ contract DelegatedManagerFactory {
     /**
      * @dev Sets managerCore and setTokenFactory address.
      * @param _managerCore                      Address of ManagerCore protocol contract
+     * @param _controller                       Address of Controller protocol contract
      * @param _setTokenFactory                  Address of SetTokenFactory protocol contract
      */
     constructor(
         IManagerCore _managerCore,
+        IController _controller,
         ISetTokenCreator _setTokenFactory
-    ) 
-        public 
+    )
+        public
     {
         managerCore = _managerCore;
+        controller = _controller;
         setTokenFactory = _setTokenFactory;
     }
 
@@ -149,13 +157,12 @@ contract DelegatedManagerFactory {
 
         DelegatedManager manager = _deployManager(
             setToken,
-            _methodologist,
             _extensions,
             _operators,
             _assets
         );
 
-        _setInitializationState(setToken, address(manager), _owner);
+        _setInitializationState(setToken, address(manager), _owner, _methodologist);
 
         return (setToken, address(manager));
     }
@@ -189,19 +196,19 @@ contract DelegatedManagerFactory {
         external
         returns (address)
     {
+        require(controller.isSet(address(_setToken)), "Must be controller-enabled SetToken");
         require(msg.sender == _setToken.manager(), "Must be manager");
 
         _validateManagerParameters(_setToken.getComponents(), _extensions, _assets);
 
         DelegatedManager manager = _deployManager(
             _setToken,
-            _methodologist,
             _extensions,
             _operators,
             _assets
         );
 
-        _setInitializationState(_setToken, address(manager), _owner);
+        _setInitializationState(_setToken, address(manager), _owner, _methodologist);
 
         return address(manager);
     }
@@ -234,23 +241,27 @@ contract DelegatedManagerFactory {
         require(msg.sender == initializeState[_setToken].deployer, "Only deployer can initialize manager");
         _initializeTargets.validatePairsWithArray(_initializeBytecode);
 
-        IDelegatedManager manager = initializeState[_setToken].manager;
-        manager.updateOwnerFeeSplit(_ownerFeeSplit);
-        manager.updateOwnerFeeRecipient(_ownerFeeRecipient);
-
         for (uint256 i = 0; i < _initializeTargets.length; i++) {
+            address target = _initializeTargets[i];
+            require(!controller.isSet(target), "Target must not be SetToken");
+
             // Because we validate uniqueness of _initializeTargets only one transaction can be sent to each module or extension during this
             // transaction. Due to this no modules/extension can be used for any SetToken transactions other than initializing these contracts
-            _initializeTargets[i].functionCallWithValue(_initializeBytecode[i], 0);
+            target.functionCallWithValue(_initializeBytecode[i], 0);
         }
 
+        IDelegatedManager manager = initializeState[_setToken].manager;
+        manager.updateOwnerFeeSplit(_ownerFeeSplit);
+        manager.updateOwnerFeeRecipient(_ownerFeeRecipient);
+
         // If the SetToken was factory-deployed & factory is its current `manager`, transfer
         // managership to the new DelegatedManager
         if (_setToken.manager() == address(this)) {
             _setToken.setManager(address(manager));
         }
 
         manager.transferOwnership(initializeState[_setToken].owner);
+        manager.setMethodologist(initializeState[_setToken].methodologist);
 
         delete initializeState[_setToken];
 
@@ -297,7 +308,6 @@ contract DelegatedManagerFactory {
      * Deploys a DelegatedManager
      *
      * @param  _setToken         Instance of SetToken to migrate to the DelegatedManager system
-     * @param  _methodologist    Address to set as the DelegateManager's methodologist role
      * @param  _extensions       List of extensions authorized for the DelegateManager
      * @param  _operators        List of operators authorized for the DelegateManager
      * @param  _assets           List of assets DelegateManager can trade. When empty, asset allow list is not enforced
@@ -306,7 +316,6 @@ contract DelegatedManagerFactory {
      */
     function _deployManager(
         ISetToken _setToken,
-        address _methodologist,
         address[] memory _extensions,
         address[] memory _operators,
         address[] memory _assets
@@ -321,7 +330,7 @@ contract DelegatedManagerFactory {
         DelegatedManager newManager = new DelegatedManager(
             _setToken,
             address(this),
-            _methodologist,
+            address(this),
             _extensions,
             _operators,
             _assets,
@@ -347,15 +356,18 @@ contract DelegatedManagerFactory {
      * @param  _setToken         Instance of SetToken
      * @param  _manager          Address of DelegatedManager created for SetToken
      * @param  _owner            Address that will be given the `owner` DelegatedManager's role on initialization
+     * @param  _methodologist    Address that will be given the `methodologist` DelegatedManager's role on initialization
      */
     function _setInitializationState(
         ISetToken _setToken,
         address _manager,
-        address _owner
+        address _owner,
+        address _methodologist
     ) internal {
         initializeState[_setToken] = InitializeParams({
             deployer: msg.sender,
             owner: _owner,
+            methodologist: _methodologist,
             manager: IDelegatedManager(_manager),
             isPending: true
         });

contracts/interfaces/IDelegatedManager.sol

@@ -31,6 +31,8 @@ interface IDelegatedManager {
     function updateOwnerFeeSplit(uint256 _newFeeSplit) external;
 
     function updateOwnerFeeRecipient(address _newFeeRecipient) external;
+
+    function setMethodologist(address _newMethodologist) external;
 
     function transferOwnership(address _owner) external;
 

contracts/lib/BaseGlobalExtension.sol

@@ -44,9 +44,9 @@ abstract contract BaseGlobalExtension {
     /* ============ State Variables ============ */
 
     // Address of the ManagerCore
-    IManagerCore public managerCore;
+    IManagerCore public immutable managerCore;
 
-    // Mapping from Set Token to DelegatedManager 
+    // Mapping from Set Token to DelegatedManager
     mapping(ISetToken => IDelegatedManager) public setManagers;
 
     /* ============ Modifiers ============ */

contracts/lib/MutualUpgradeV2.sol

@@ -0,0 +1,82 @@
+/*
+    Copyright 2022 Set Labs Inc.
+
+    Licensed under the Apache License, Version 2.0 (the "License");
+    you may not use this file except in compliance with the License.
+    You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+    Unless required by applicable law or agreed to in writing, software
+    distributed under the License is distributed on an "AS IS" BASIS,
+    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+    See the License for the specific language governing permissions and
+    limitations under the License.
+
+    SPDX-License-Identifier: Apache License, Version 2.0
+*/
+
+pragma solidity 0.6.10;
+
+/**
+ * @title MutualUpgradeV2
+ * @author Set Protocol
+ *
+ * The MutualUpgradeV2 contract contains a modifier for handling mutual upgrades between two parties
+ *
+ * CHANGELOG:
+ * - Update mutualUpgrade to allow single transaction execution if the two signing addresses are the same
+ */
+contract MutualUpgradeV2 {
+    /* ============ State Variables ============ */
+
+    // Mapping of upgradable units and if upgrade has been initialized by other party
+    mapping(bytes32 => bool) public mutualUpgrades;
+
+    /* ============ Events ============ */
+
+    event MutualUpgradeRegistered(
+        bytes32 _upgradeHash
+    );
+
+    /* ============ Modifiers ============ */
+
+    modifier mutualUpgrade(address _signerOne, address _signerTwo) {
+        require(
+            msg.sender == _signerOne || msg.sender == _signerTwo,
+            "Must be authorized address"
+        );
+
+        // If the two signing addresses are the same, skip upgrade hash step
+        if (_signerOne == _signerTwo) {
+            _;
+        }
+
+        address nonCaller = _getNonCaller(_signerOne, _signerTwo);
+
+        // The upgrade hash is defined by the hash of the transaction call data and sender of msg,
+        // which uniquely identifies the function, arguments, and sender.
+        bytes32 expectedHash = keccak256(abi.encodePacked(msg.data, nonCaller));
+
+        if (!mutualUpgrades[expectedHash]) {
+            bytes32 newHash = keccak256(abi.encodePacked(msg.data, msg.sender));
+
+            mutualUpgrades[newHash] = true;
+
+            emit MutualUpgradeRegistered(newHash);
+
+            return;
+        }
+
+        delete mutualUpgrades[expectedHash];
+
+        // Run the rest of the upgrades
+        _;
+    }
+
+    /* ============ Internal Functions ============ */
+
+    function _getNonCaller(address _signerOne, address _signerTwo) internal view returns(address) {
+        return msg.sender == _signerOne ? _signerTwo : _signerOne;
+    }
+}

contracts/manager/DelegatedManager.sol

@@ -28,6 +28,7 @@ import { PreciseUnitMath } from "@setprotocol/set-protocol-v2/contracts/lib/Prec
 
 import { AddressArrayUtils } from "../lib/AddressArrayUtils.sol";
 import { IGlobalExtension } from "../interfaces/IGlobalExtension.sol";
+import { MutualUpgradeV2 } from "../lib/MutualUpgradeV2.sol";
 
 
 /**
@@ -42,7 +43,7 @@ import { IGlobalExtension } from "../interfaces/IGlobalExtension.sol";
  * a part of the asset whitelist, hence they are a semi-trusted party. It is recommended that the owner address
  * be managed by a multi-sig or some form of permissioning system.
  */
-contract DelegatedManager is Ownable {
+contract DelegatedManager is Ownable, MutualUpgradeV2 {
     using Address for address;
     using AddressArrayUtils for address[];
     using SafeERC20 for IERC20;
@@ -327,7 +328,7 @@ contract DelegatedManager is Ownable {
      *
      * @param _newFeeSplit           Percent in precise units (100% = 10**18) of fees that accrue to owner
      */
-    function updateOwnerFeeSplit(uint256 _newFeeSplit) external onlyOwner {
+    function updateOwnerFeeSplit(uint256 _newFeeSplit) external mutualUpgrade(owner(), methodologist) {
         require(_newFeeSplit <= PreciseUnitMath.preciseUnit(), "Invalid fee split");
 
         ownerFeeSplit = _newFeeSplit;
@@ -354,6 +355,8 @@ contract DelegatedManager is Ownable {
      * @param _newMethodologist           New methodologist address
      */
     function setMethodologist(address _newMethodologist) external onlyMethodologist {
+        require(_newMethodologist != address(0), "Null address passed");
+
         methodologist = _newMethodologist;
 
         emit MethodologistChanged(_newMethodologist);

contracts/mocks/MutualUpgradeV2Mock.sol

@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: Apache License, Version 2.0
+pragma solidity 0.6.10;
+
+import { MutualUpgradeV2 } from "../lib/MutualUpgradeV2.sol";
+
+
+// Mock contract implementation of MutualUpgradeV2 functions
+contract MutualUpgradeV2Mock is
+    MutualUpgradeV2
+{
+    uint256 public testUint;
+    address public owner;
+    address public methodologist;
+
+    constructor(address _owner, address _methodologist) public {
+        owner = _owner;
+        methodologist = _methodologist;
+    }
+
+    function testMutualUpgrade(
+        uint256 _testUint
+    )
+        external
+        mutualUpgrade(owner, methodologist)
+    {
+        testUint = _testUint;
+    }
+}

test/extensions/issuanceExtension.spec.ts

@@ -91,9 +91,10 @@ describe("IssuanceExtension", () => {
     );
 
     ownerFeeSplit = ether(0.1);
-    await delegatedManager.updateOwnerFeeSplit(ownerFeeSplit);
+    await delegatedManager.connect(owner.wallet).updateOwnerFeeSplit(ownerFeeSplit);
+    await delegatedManager.connect(methodologist.wallet).updateOwnerFeeSplit(ownerFeeSplit);
     ownerFeeRecipient = owner.address;
-    await delegatedManager.updateOwnerFeeRecipient(ownerFeeRecipient);
+    await delegatedManager.connect(owner.wallet).updateOwnerFeeRecipient(ownerFeeRecipient);
 
     await setToken.setManager(delegatedManager.address);
 
@@ -695,6 +696,7 @@ describe("IssuanceExtension", () => {
     describe("when methodologist fees are 0", async () => {
       beforeEach(async () => {
         await delegatedManager.connect(owner.wallet).updateOwnerFeeSplit(ether(1));
+        await delegatedManager.connect(methodologist.wallet).updateOwnerFeeSplit(ether(1));
       });
 
       it("should not send fees to methodologist", async () => {
@@ -710,6 +712,7 @@ describe("IssuanceExtension", () => {
     describe("when owner fees are 0", async () => {
       beforeEach(async () => {
         await delegatedManager.connect(owner.wallet).updateOwnerFeeSplit(ZERO);
+        await delegatedManager.connect(methodologist.wallet).updateOwnerFeeSplit(ZERO);
       });
 
       it("should not send fees to owner fee recipient", async () => {

test/extensions/streamingFeeSplitExtension.spec.ts

@@ -95,9 +95,10 @@ describe("StreamingFeeSplitExtension", () => {
     );
 
     ownerFeeSplit = ether(0.1);
-    await delegatedManager.updateOwnerFeeSplit(ownerFeeSplit);
+    await delegatedManager.connect(owner.wallet).updateOwnerFeeSplit(ownerFeeSplit);
+    await delegatedManager.connect(methodologist.wallet).updateOwnerFeeSplit(ownerFeeSplit);
     ownerFeeRecipient = owner.address;
-    await delegatedManager.updateOwnerFeeRecipient(ownerFeeRecipient);
+    await delegatedManager.connect(owner.wallet).updateOwnerFeeRecipient(ownerFeeRecipient);
 
     await setToken.setManager(delegatedManager.address);
 
@@ -630,6 +631,7 @@ describe("StreamingFeeSplitExtension", () => {
     describe("when methodologist fees are 0", async () => {
       beforeEach(async () => {
         await delegatedManager.connect(owner.wallet).updateOwnerFeeSplit(ether(1));
+        await delegatedManager.connect(methodologist.wallet).updateOwnerFeeSplit(ether(1));
       });
 
       it("should not send fees to methodologist", async () => {
@@ -645,6 +647,7 @@ describe("StreamingFeeSplitExtension", () => {
     describe("when owner fees are 0", async () => {
       beforeEach(async () => {
         await delegatedManager.connect(owner.wallet).updateOwnerFeeSplit(ZERO);
+        await delegatedManager.connect(methodologist.wallet).updateOwnerFeeSplit(ZERO);
       });
 
       it("should not send fees to owner fee recipient", async () => {