Delegated Manager System internal audit fixes [SIM-179] (#23)

Author: pblivin0x

contracts/ManagerCore.sol

@@ -26,13 +26,15 @@ import { AddressArrayUtils } from "./lib/AddressArrayUtils.sol";
  * @title ManagerCore
  * @author Set Protocol
  *
- *  Registry for governance approved DelegatedManagerFactories and DelegatedManagers.
+ *  Registry for governance approved GlobalExtensions, DelegatedManagerFactories, and DelegatedManagers.
  */
 contract ManagerCore is Ownable {
     using AddressArrayUtils for address[];
 
     /* ============ Events ============ */
 
+    event ExtensionAdded(address indexed _extension);
+    event ExtensionRemoved(address indexed _extension);
     event FactoryAdded(address indexed _factory);
     event FactoryRemoved(address indexed _factory);
     event ManagerAdded(address indexed _manager, address indexed _factory);
@@ -55,14 +57,18 @@ contract ManagerCore is Ownable {
 
     /* ============ State Variables ============ */
 
-    // List of enabled managers
-    address[] public managers;
+    // List of enabled extensions
+    address[] public extensions;
     // List of enabled factories of managers
     address[] public factories;
+    // List of enabled managers
+    address[] public managers;
 
-    // Mapping to check whether address is valid Manager or Factory
-    mapping(address => bool) public isManager;
+    // Mapping to check whether address is valid Extension, Factory, or Manager
+    mapping(address => bool) public isExtension;
     mapping(address => bool) public isFactory;
+    mapping(address => bool) public isManager;
+
 
     // Return true if the ManagerCore is initialized
     bool public isInitialized;
@@ -73,57 +79,59 @@ contract ManagerCore is Ownable {
      * Initializes any predeployed factories. Note: This function can only be called by
      * the owner once to batch initialize the initial system contracts.
      *
+     * @param _extensions            List of extensions to add
      * @param _factories             List of factories to add
      */
     function initialize(
+        address[] memory _extensions,
         address[] memory _factories
     )
         external
         onlyOwner
     {
         require(!isInitialized, "ManagerCore is already initialized");
 
+        extensions = _extensions;
         factories = _factories;
 
-        // Loop through and initialize isFactory mapping
+        // Loop through and initialize isExtension and isFactory mapping
+        for (uint256 i = 0; i < _extensions.length; i++) {
+            _addExtension(_extensions[i]);
+        }
         for (uint256 i = 0; i < _factories.length; i++) {
-            address factory = _factories[i];
-            require(factory != address(0), "Zero address submitted.");
-            isFactory[factory] = true;
+            _addFactory(_factories[i]);
         }
 
         // Set to true to only allow initialization once
         isInitialized = true;
     }
 
     /**
-     * PRIVILEGED FACTORY FUNCTION. Adds a newly deployed manager as an enabled manager.
+     * PRIVILEGED GOVERNANCE FUNCTION. Allows governance to add an extension
      *
-     * @param _manager               Address of the manager contract to add
+     * @param _extension               Address of the extension contract to add
      */
-    function addManager(address _manager) external onlyInitialized onlyFactory {
-        require(!isManager[_manager], "Manager already exists");
+    function addExtension(address _extension) external onlyInitialized onlyOwner {
+        require(!isExtension[_extension], "Extension already exists");
 
-        isManager[_manager] = true;
-
-        managers.push(_manager);
+        _addExtension(_extension);
 
-        emit ManagerAdded(_manager, msg.sender);
+        extensions.push(_extension);
     }
 
     /**
-     * PRIVILEGED GOVERNANCE FUNCTION. Allows governance to remove a manager
+     * PRIVILEGED GOVERNANCE FUNCTION. Allows governance to remove an extension
      *
-     * @param _manager               Address of the manager contract to remove
+     * @param _extension               Address of the extension contract to remove
      */
-    function removeManager(address _manager) external onlyInitialized onlyOwner {
-        require(isManager[_manager], "Manager does not exist");
+    function removeExtension(address _extension) external onlyInitialized onlyOwner {
+        require(isExtension[_extension], "Extension does not exist");
 
-        managers.removeStorage(_manager);
+        extensions.removeStorage(_extension);
 
-        isManager[_manager] = false;
+        isExtension[_extension] = false;
 
-        emit ManagerRemoved(_manager);
+        emit ExtensionRemoved(_extension);
     }
 
     /**
@@ -134,11 +142,9 @@ contract ManagerCore is Ownable {
     function addFactory(address _factory) external onlyInitialized onlyOwner {
         require(!isFactory[_factory], "Factory already exists");
 
-        isFactory[_factory] = true;
+        _addFactory(_factory);
 
         factories.push(_factory);
-
-        emit FactoryAdded(_factory);
     }
 
     /**
@@ -156,13 +162,75 @@ contract ManagerCore is Ownable {
         emit FactoryRemoved(_factory);
     }
 
+    /**
+     * PRIVILEGED FACTORY FUNCTION. Adds a newly deployed manager as an enabled manager.
+     *
+     * @param _manager               Address of the manager contract to add
+     */
+    function addManager(address _manager) external onlyInitialized onlyFactory {
+        require(!isManager[_manager], "Manager already exists");
+
+        isManager[_manager] = true;
+
+        managers.push(_manager);
+
+        emit ManagerAdded(_manager, msg.sender);
+    }
+
+    /**
+     * PRIVILEGED GOVERNANCE FUNCTION. Allows governance to remove a manager
+     *
+     * @param _manager               Address of the manager contract to remove
+     */
+    function removeManager(address _manager) external onlyInitialized onlyOwner {
+        require(isManager[_manager], "Manager does not exist");
+
+        managers.removeStorage(_manager);
+
+        isManager[_manager] = false;
+
+        emit ManagerRemoved(_manager);
+    }
+
     /* ============ External Getter Functions ============ */
 
-    function getManagers() external view returns (address[] memory) {
-        return managers;
+    function getExtensions() external view returns (address[] memory) {
+        return extensions;
     }
 
     function getFactories() external view returns (address[] memory) {
         return factories;
     }
+
+    function getManagers() external view returns (address[] memory) {
+        return managers;
+    }
+
+    /* ============ Internal Functions ============ */
+
+    /**
+     * Add an extension tracked on the ManagerCore
+     *
+     * @param _extension               Address of the extension contract to add
+     */
+    function _addExtension(address _extension) internal {
+        require(_extension != address(0), "Zero address submitted.");
+
+        isExtension[_extension] = true;
+
+        emit ExtensionAdded(_extension);
+    }
+
+    /**
+     * Add a factory tracked on the ManagerCore
+     *
+     * @param _factory               Address of the factory contract to add
+     */
+    function _addFactory(address _factory) internal {
+        require(_factory != address(0), "Zero address submitted.");
+
+        isFactory[_factory] = true;
+
+        emit FactoryAdded(_factory);
+    }
 }

contracts/extensions/IssuanceExtension.sol

@@ -34,8 +34,12 @@ import { IManagerCore } from "../interfaces/IManagerCore.sol";
  * @title IssuanceExtension
  * @author Set Protocol
  *
- * Smart contract global extension which provides DelegatedManager owner and methodologist the ability to accrue and split 
+ * Smart contract global extension which provides DelegatedManager owner and methodologist the ability to accrue and split
  * issuance and redemption fees. Owner may configure the fee split percentages.
+ *
+ * Notes
+ * - the fee split is set on the Delegated Manager contract
+ * - when fees distributed via this contract will be inclusive of all fee types that have already been accrued
  */
 contract IssuanceExtension is BaseGlobalExtension {
     using Address for address;
@@ -119,16 +123,16 @@ contract IssuanceExtension is BaseGlobalExtension {
         uint256 _managerRedeemFee,
         address _feeRecipient,
         address _managerIssuanceHook
-    ) 
-        external 
-        onlyOwnerAndValidManager(_delegatedManager) 
+    )
+        external
+        onlyOwnerAndValidManager(_delegatedManager)
     {
         require(_delegatedManager.isInitializedExtension(address(this)), "Extension must be initialized");
 
         _initializeModule(
             _delegatedManager.setToken(),
             _delegatedManager,
-            _maxManagerFee, 
+            _maxManagerFee,
             _managerIssueFee,
             _managerRedeemFee,
             _feeRecipient,
@@ -168,7 +172,7 @@ contract IssuanceExtension is BaseGlobalExtension {
         uint256 _managerRedeemFee,
         address _feeRecipient,
         address _managerIssuanceHook
-    ) 
+    )
         external
         onlyOwnerAndValidManager(_delegatedManager)
     {
@@ -180,7 +184,7 @@ contract IssuanceExtension is BaseGlobalExtension {
         _initializeModule(
             setToken,
             _delegatedManager,
-            _maxManagerFee, 
+            _maxManagerFee,
             _managerIssueFee,
             _managerRedeemFee,
             _feeRecipient,
@@ -191,10 +195,13 @@ contract IssuanceExtension is BaseGlobalExtension {
     }
 
     /**
-     * ONLY MANAGER: Remove an existing SetToken and DelegatedManager tracked by the IssuanceExtension 
+     * ONLY MANAGER: Remove an existing SetToken and DelegatedManager tracked by the IssuanceExtension
      */
     function removeExtension() external override {
-        _removeExtension();
+        IDelegatedManager delegatedManager = IDelegatedManager(msg.sender);
+        ISetToken setToken = delegatedManager.setToken();
+
+        _removeExtension(setToken, delegatedManager);
     }
 
     /**
@@ -260,11 +267,11 @@ contract IssuanceExtension is BaseGlobalExtension {
         uint256 _managerRedeemFee,
         address _feeRecipient,
         address _managerIssuanceHook
-    ) 
+    )
         internal
     {
         bytes memory callData = abi.encodeWithSignature(
-            "initialize(address,uint256,uint256,uint256,address,address)", 
+            "initialize(address,uint256,uint256,uint256,address,address)",
             _setToken,
             _maxManagerFee,
             _managerIssueFee,

contracts/extensions/StreamingFeeSplitExtension.sol

@@ -12,7 +12,7 @@
     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
 */
 
@@ -34,8 +34,12 @@ import { IStreamingFeeModule } from "../interfaces/IStreamingFeeModuleV2.sol";
  * @title StreamingFeeSplitExtension
  * @author Set Protocol
  *
- * Smart contract global extension which provides DelegatedManager owner and methodologist the ability to accrue and split 
+ * Smart contract global extension which provides DelegatedManager owner and methodologist the ability to accrue and split
  * streaming fees. Owner may configure the fee split percentages.
+ *
+ * Notes
+ * - the fee split is set on the Delegated Manager contract
+ * - when fees distributed via this contract will be inclusive of all fee types
  */
 contract StreamingFeeSplitExtension is BaseGlobalExtension {
     using Address for address;
@@ -114,9 +118,9 @@ contract StreamingFeeSplitExtension is BaseGlobalExtension {
     function initializeModule(
         IDelegatedManager _delegatedManager,
         IStreamingFeeModule.FeeState memory _settings
-    ) 
-        external 
-        onlyOwnerAndValidManager(_delegatedManager) 
+    )
+        external
+        onlyOwnerAndValidManager(_delegatedManager)
     {
         require(_delegatedManager.isInitializedExtension(address(this)), "Extension must be initialized");
 
@@ -147,8 +151,8 @@ contract StreamingFeeSplitExtension is BaseGlobalExtension {
     function initializeModuleAndExtension(
         IDelegatedManager _delegatedManager,
         IStreamingFeeModule.FeeState memory _settings
-    ) 
-        external 
+    )
+        external
         onlyOwnerAndValidManager(_delegatedManager)
     {
         require(_delegatedManager.isPendingExtension(address(this)), "Extension must be pending");
@@ -162,10 +166,13 @@ contract StreamingFeeSplitExtension is BaseGlobalExtension {
     }
 
     /**
-     * ONLY MANAGER: Remove an existing SetToken and DelegatedManager tracked by the StreamingFeeSplitExtension 
+     * ONLY MANAGER: Remove an existing SetToken and DelegatedManager tracked by the StreamingFeeSplitExtension
      */
     function removeExtension() external override {
-        _removeExtension();
+        IDelegatedManager delegatedManager = IDelegatedManager(msg.sender);
+        ISetToken setToken = delegatedManager.setToken();
+
+        _removeExtension(setToken, delegatedManager);
     }
 
     /**
@@ -211,11 +218,11 @@ contract StreamingFeeSplitExtension is BaseGlobalExtension {
         ISetToken _setToken,
         IDelegatedManager _delegatedManager,
         IStreamingFeeModule.FeeState memory _settings
-    ) 
+    )
         internal
     {
         bytes memory callData = abi.encodeWithSignature(
-            "initialize(address,(address,uint256,uint256,uint256))", 
+            "initialize(address,(address,uint256,uint256,uint256))",
             _setToken,
             _settings);
         _invokeManager(_delegatedManager, address(streamingFeeModule), callData);

contracts/extensions/TradeExtension.sol

@@ -99,10 +99,13 @@ contract TradeExtension is BaseGlobalExtension {
     }
 
     /**
-     * ONLY MANAGER: Remove an existing SetToken and DelegatedManager tracked by the TradeExtension 
+     * ONLY MANAGER: Remove an existing SetToken and DelegatedManager tracked by the TradeExtension
      */
     function removeExtension() external override {
-        _removeExtension();
+        IDelegatedManager delegatedManager = IDelegatedManager(msg.sender);
+        ISetToken setToken = delegatedManager.setToken();
+
+        _removeExtension(setToken, delegatedManager);
     }
 
     /**
@@ -132,7 +135,7 @@ contract TradeExtension is BaseGlobalExtension {
         onlyAllowedAsset(_setToken, _receiveToken)
     {
         bytes memory callData = abi.encodeWithSignature(
-            "trade(address,string,address,uint256,address,uint256,bytes)", 
+            "trade(address,string,address,uint256,address,uint256,bytes)",
             _setToken,
             _exchangeName,
             _sendToken,