BaseManager IIP-64/72 Internal audit fixes (#74)

* Prohibit calling SetToken with #interactManager / add #transferTokens

* Fix javadoc for BaseManagerV2#emergencyReplaceProtectedModule

* Remove unused param in FeeSplitExtension#initializeIssuanceModule

* Add BaseMangerV2 EmergencyResolved event

* Add missing parameter documentation for FeeSplitExtension / StreamingFeeSplitExtension

* Add explanatory notes about timelocking in extension methods

Author: cgewecke

contracts/adapters/FeeSplitExtension.sol

@@ -117,9 +117,14 @@ contract FeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpgrade {
      *
      * This method is called after invoking `replaceProtectedModule` or `emergencyReplaceProtectedModule`
      * to configure the replacement streaming fee module's fee settings.
+     *
+     * @param _maxManagerFee            Max size of issuance and redeem fees in precise units (10^16 = 1%).
+     * @param _managerIssueFee          Manager issuance fees in precise units (10^16 = 1%)
+     * @param _managerRedeemFee         Manager redeem fees in precise units (10^16 = 1%)
+     * @param _feeRecipient             Address that receives all manager issue and redeem fees
+     * @param _managerIssuanceHook      Address of manager defined hook contract
      */
     function initializeIssuanceModule(
-        ISetToken _setToken,
         uint256 _maxManagerFee,
         uint256 _managerIssueFee,
         uint256 _managerRedeemFee,
@@ -148,6 +153,17 @@ contract FeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpgrade {
      *
      * This method is called after invoking `replaceProtectedModule` or `emergencyReplaceProtectedModule`
      * to configure the replacement streaming fee module's fee settings.
+     *
+     * @dev FeeState settings encode the following struct
+     * ```
+     * struct FeeState {
+     *   address feeRecipient;                // Address to accrue fees to
+     *   uint256 maxStreamingFeePercentage;   // Max streaming fee maanager commits to using (1% = 1e16, 100% = 1e18)
+     *   uint256 streamingFeePercentage;      // Percent of Set accruing to manager annually (1% = 1e16, 100% = 1e18)
+     *   uint256 lastStreamingFeeTimestamp;   // Timestamp last streaming fee was accrued
+     *}
+     *```
+     * @param _settings     FeeModule.FeeState settings
      */
     function initializeStreamingFeeModule(IStreamingFeeModule.FeeState memory _settings)
         external
@@ -167,7 +183,12 @@ contract FeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpgrade {
      * this function to execute the update. Because the method is timelocked, each party must call it twice:
      * once to set the lock and once to execute.
      *
+     * Method is timelocked to protect token owners from sudden changes in fee structure which
+     * they would rather not bear. The delay gives them a chance to exit their positions without penalty.
+     *
      * NOTE: This will accrue streaming fees though not send to operator fee recipient and methodologist.
+     *
+     * @param _newFee       Percent of Set accruing to fee extension annually (1% = 1e16, 100% = 1e18)
      */
     function updateStreamingFee(uint256 _newFee)
         external
@@ -182,6 +203,11 @@ contract FeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpgrade {
      * MUTUAL UPGRADE: Updates issue fee on IssuanceModule. Only is executed once time lock has passed.
      * Operator and Methodologist must each call this function to execute the update. Because the method
      * is timelocked, each party must call it twice: once to set the lock and once to execute.
+     *
+     * Method is timelocked to protect token owners from sudden changes in fee structure which
+     * they would rather not bear. The delay gives them a chance to exit their positions without penalty.
+     *
+     * @param _newFee           New issue fee percentage in precise units (1% = 1e16, 100% = 1e18)
      */
     function updateIssueFee(uint256 _newFee)
         external
@@ -196,6 +222,11 @@ contract FeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpgrade {
      * MUTUAL UPGRADE: Updates redeem fee on IssuanceModule. Only is executed once time lock has passed.
      * Operator and Methodologist must each call this function to execute the update. Because the method is
      * timelocked, each party must call it twice: once to set the lock and once to execute.
+     *
+     * Method is timelocked to protect token owners from sudden changes in fee structure which
+     * they would rather not bear. The delay gives them a chance to exit their positions without penalty.
+     *
+     * @param _newFee           New redeem fee percentage in precise units (1% = 1e16, 100% = 1e18)
      */
     function updateRedeemFee(uint256 _newFee)
         external
@@ -208,6 +239,8 @@ contract FeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpgrade {
 
     /**
      * MUTUAL UPGRADE: Updates fee recipient on both streaming fee and issuance modules.
+     *
+     * @param _newFeeRecipient  Address of new fee recipient. This should be the address of the fee extension itself.
      */
     function updateFeeRecipient(address _newFeeRecipient)
         external
@@ -220,6 +253,8 @@ contract FeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpgrade {
 
     /**
      * MUTUAL UPGRADE: Updates fee split between operator and methodologist. Split defined in precise units (1% = 10^16).
+     *
+     * @param _newFeeSplit      Percent of fees in precise units (10^16 = 1%) sent to operator, (rest go to the methodologist).
      */
     function updateFeeSplit(uint256 _newFeeSplit)
         external
@@ -232,6 +267,8 @@ contract FeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpgrade {
 
     /**
      * OPERATOR ONLY: Updates the address that receives the operator's share of the fees (see IIP-72)
+     *
+     * @param _newOperatorFeeRecipient  Address to send operator's fees to.
      */
     function updateOperatorFeeRecipient(address _newOperatorFeeRecipient)
         external

contracts/adapters/StreamingFeeSplitExtension.sol

@@ -113,6 +113,17 @@ contract StreamingFeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpg
      *
      * This method is called after invoking `replaceProtectedModule` or `emergencyReplaceProtectedModule`
      * to configure the replacement streaming fee module's fee settings.
+     *
+     * @dev FeeState settings encode the following struct
+     * ```
+     * struct FeeState {
+     *   address feeRecipient;                // Address to accrue fees to
+     *   uint256 maxStreamingFeePercentage;   // Max streaming fee maanager commits to using (1% = 1e16, 100% = 1e18)
+     *   uint256 streamingFeePercentage;      // Percent of Set accruing to manager annually (1% = 1e16, 100% = 1e18)
+     *   uint256 lastStreamingFeeTimestamp;   // Timestamp last streaming fee was accrued
+     *}
+     *```
+     * @param _settings     FeeModule.FeeState settings
      */
     function initializeModule(IStreamingFeeModule.FeeState memory _settings)
         external
@@ -132,7 +143,12 @@ contract StreamingFeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpg
      * each call this function to execute the update. Because the method is timelocked, each party
      * must call it twice: once to set the lock and once to execute.
      *
+     * Method is timelocked to protect token owners from sudden changes in fee structure which
+     * they would rather not bear. The delay gives them a chance to exit their positions without penalty.
+     *
      * NOTE: This will accrue streaming fees though not send to operator fee recipient and methodologist.
+     *
+     * @param _newFee       Percent of Set accruing to fee extension annually (1% = 1e16, 100% = 1e18)
      */
     function updateStreamingFee(uint256 _newFee)
         external
@@ -150,6 +166,8 @@ contract StreamingFeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpg
 
     /**
      * MUTUAL UPGRADE: Updates fee recipient on streaming fee module.
+     *
+     * @param _newFeeRecipient  Address of new fee recipient. This should be the address of the fee extension itself.
      */
     function updateFeeRecipient(address _newFeeRecipient)
         external
@@ -165,8 +183,10 @@ contract StreamingFeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpg
     }
 
     /**
-     * MUTUAL UPGRADE: Updates fee split between operator and methodologist. Split defined in precise units (1% = 10^16). Fees will be
-     * accrued and distributed before the new split goes into effect.
+     * MUTUAL UPGRADE: Updates fee split between operator and methodologist. Split defined in precise units (1% = 10^16).
+     * Fees will be accrued and distributed before the new split goes into effect.
+     *
+     * @param _newFeeSplit      Percent of fees in precise units (10^16 = 1%) sent to operator, (rest go to the methodologist).
      */
     function updateFeeSplit(uint256 _newFeeSplit)
         external
@@ -179,6 +199,8 @@ contract StreamingFeeSplitExtension is BaseExtension, TimeLockUpgrade, MutualUpg
 
     /**
      * OPERATOR ONLY: Updates the address that receives the operator's share of the fees (see IIP-72)
+     *
+     * @param _newOperatorFeeRecipient  Address to send operator's fees to.
      */
     function updateOperatorFeeRecipient(address _newOperatorFeeRecipient)
         external

contracts/interfaces/IBaseManager.sol

@@ -21,10 +21,12 @@ import { ISetToken } from "./ISetToken.sol";
 
 interface IBaseManager {
     function setToken() external returns(ISetToken);
-    
+
     function methodologist() external returns(address);
 
     function operator() external returns(address);
 
     function interactManager(address _module, bytes calldata _encoded) external;
+
+    function transferTokens(address _token, address _destination, uint256 _amount) external;
 }

contracts/lib/BaseExtension.sol

@@ -122,8 +122,7 @@ abstract contract BaseExtension {
      * @param _amount          Amount of token being transferred
      */
     function invokeManagerTransfer(address _token, address _destination, uint256 _amount) internal {
-        bytes memory callData = abi.encodeWithSignature("transfer(address,uint256)", _destination, _amount);
-        invokeManager(_token, callData);
+        manager.transferTokens(_token, _destination, _amount);
     }
 
     /**

contracts/manager/BaseManagerV2.sol

@@ -18,6 +18,8 @@ pragma solidity 0.6.10;
 pragma experimental ABIEncoderV2;
 
 import { Address } from "@openzeppelin/contracts/utils/Address.sol";
+import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";
 
 import { AddressArrayUtils } from "../lib/AddressArrayUtils.sol";
 import { IExtension } from "../interfaces/IExtension.sol";
@@ -35,6 +37,7 @@ import { MutualUpgrade } from "../lib/MutualUpgrade.sol";
 contract BaseManagerV2 is MutualUpgrade {
     using Address for address;
     using AddressArrayUtils for address[];
+    using SafeERC20 for IERC20;
 
     /* ============ Struct ========== */
 
@@ -98,6 +101,8 @@ contract BaseManagerV2 is MutualUpgrade {
         address _module
     );
 
+    event EmergencyResolved();
+
     /* ============ Modifiers ============ */
 
     /**
@@ -292,12 +297,26 @@ contract BaseManagerV2 is MutualUpgrade {
      */
     function interactManager(address _module, bytes memory _data) external onlyExtension {
         require(initialized, "Manager not initialized");
+        require(_module != address(setToken), "Extensions cannot call SetToken");
         require(_senderAuthorizedForModule(_module, msg.sender), "Extension not authorized for module");
 
         // Invoke call to module, assume value will always be 0
         _module.functionCallWithValue(_data, 0);
     }
 
+    /**
+     * OPERATOR ONLY: Transfers _tokens held by the manager to _destination. Can be used to
+     * recover anything sent here accidentally. In BaseManagerV2, extensions should
+     * be the only contracts designated as `feeRecipient` in fee modules.
+     *
+     * @param _token           ERC20 token to send
+     * @param _destination     Address receiving the tokens
+     * @param _amount          Quantity of tokens to send
+     */
+    function transferTokens(address _token, address _destination, uint256 _amount) external onlyExtension {
+        IERC20(_token).safeTransfer(_destination, _amount);
+    }
+
     /**
      * OPERATOR ONLY: Add a new module to the SetToken.
      *
@@ -341,8 +360,8 @@ contract BaseManagerV2 is MutualUpgrade {
     }
 
     /**
-     * OPERATOR ONLY: Marks an existing module as protected and authorizes existing extensions for
-     * it. Adds module to the protected modules list
+     * OPERATOR ONLY: Marks an existing module as protected and authorizes extensions for
+     * it, adding them if necessary. Adds module to the protected modules list
      *
      * The operator uses this when they're adding new features and want to assure the methodologist
      * they won't be unilaterally changed in the future. Cannot be called during an emergency because
@@ -431,9 +450,9 @@ contract BaseManagerV2 is MutualUpgrade {
      *
      * NOTE: If replacing a fee module, it's necessary to set the module `feeRecipient` to be
      * the new fee extension address after this call. Any fees remaining in the old module's
-     * de-authorized extensions can be distributed by calling `distribute()` on the old extension.
+     * de-authorized extensions can be distributed by calling `accrueFeesAndDistribute` on the old extension.
      *
-     * @param _module        Module to add in place of removed module
+     * @param _module          Module to add in place of removed module
      * @param _extensions      Array of extensions to authorize for replacement module
      */
     function emergencyReplaceProtectedModule(
@@ -461,6 +480,8 @@ contract BaseManagerV2 is MutualUpgrade {
      */
     function resolveEmergency() external onlyMethodologist onlyEmergency {
         emergencies -= 1;
+
+        emit EmergencyResolved();
     }
 
     /**

test/adapters/feeSplitExtension.spec.ts

@@ -387,7 +387,6 @@ describe("FeeSplitExtension", () => {
 
       async function subject(caller: Account): Promise<ContractTransaction> {
          return await subjectExtension.connect(caller.wallet).initializeIssuanceModule(
-           subjectSetToken,
            subjectMaxManagerFee,
            subjectManagerIssueFee,
            subjectManagerRedeemFee,

test/manager/baseManagerV2.spec.ts

@@ -1374,6 +1374,10 @@ describe("BaseManagerV2", () => {
         expect(initialEmergencies.toNumber()).equals(1);
         expect(finalEmergencies.toNumber()).equals(0);
       });
+
+      it("should emit the correct EmergencyResolved event", async () => {
+        await expect(subject()).to.emit(baseManager, "EmergencyResolved");
+      });
     });
 
     describe("when an emergency is *not* in progress", async () => {
@@ -1412,7 +1416,7 @@ describe("BaseManagerV2", () => {
     });
 
     async function subject(): Promise<any> {
-      return baseExtension.interactManager(subjectModule, subjectCallData);
+      return baseExtension.connect(operator.wallet).interactManager(subjectModule, subjectCallData);
     }
 
     context("when the manager is initialized", () => {
@@ -1435,11 +1439,21 @@ describe("BaseManagerV2", () => {
           await expect(subject()).to.be.revertedWith("Must be extension");
         });
       });
+
+      describe("when the extension tries to call the SetToken", async () => {
+        beforeEach(async () => {
+          subjectModule = setToken.address;
+        });
+
+        it("should revert", async () => {
+          await expect(subject()).to.be.revertedWith("Extensions cannot call SetToken");
+        });
+      });
     });
 
     context("when the manager is not initialized", () => {
       it("updateFeeRecipient should revert", async () => {
-        expect(subject()).to.be.revertedWith("Manager not initialized");
+        await expect(subject()).to.be.revertedWith("Manager not initialized");
       });
     });
 
@@ -1468,6 +1482,55 @@ describe("BaseManagerV2", () => {
     });
   });
 
+  describe("#transferTokens", async () => {
+    let subjectCaller: Account;
+    let subjectToken: Address;
+    let subjectDestination: Address;
+    let subjectAmount: BigNumber;
+
+    beforeEach(async () => {
+      await baseManager.connect(operator.wallet).addExtension(baseExtension.address);
+
+      subjectCaller = operator;
+      subjectToken = setV2Setup.weth.address;
+      subjectDestination = otherAccount.address;
+      subjectAmount = ether(1);
+
+      await setV2Setup.weth.transfer(baseManager.address, subjectAmount);
+    });
+
+    async function subject(): Promise<ContractTransaction> {
+      return baseExtension.connect(subjectCaller.wallet).testInvokeManagerTransfer(
+        subjectToken,
+        subjectDestination,
+        subjectAmount
+      );
+    }
+
+    it("should send the given amount from the manager to the address", async () => {
+      const preManagerAmount = await setV2Setup.weth.balanceOf(baseManager.address);
+      const preDestinationAmount = await setV2Setup.weth.balanceOf(subjectDestination);
+
+      await subject();
+
+      const postManagerAmount = await setV2Setup.weth.balanceOf(baseManager.address);
+      const postDestinationAmount = await setV2Setup.weth.balanceOf(subjectDestination);
+
+      expect(preManagerAmount.sub(postManagerAmount)).to.eq(subjectAmount);
+      expect(postDestinationAmount.sub(preDestinationAmount)).to.eq(subjectAmount);
+    });
+
+    describe("when the caller is not an extension", async () => {
+        beforeEach(async () => {
+          await baseManager.connect(operator.wallet).removeExtension(baseExtension.address);
+        });
+
+        it("should revert", async () => {
+          await expect(subject()).to.be.revertedWith("Must be extension");
+        });
+      });
+  });
+
   describe("#removeModule", async () => {
     let subjectModule: Address;
     let subjectExtension: Address;
@@ -1500,8 +1563,8 @@ describe("BaseManagerV2", () => {
     });
 
     describe("when the module is protected module", () => {
-      beforeEach(() => {
-        baseManager.connect(operator.wallet).protectModule(subjectModule, [subjectExtension]);
+      beforeEach(async () => {
+        await baseManager.connect(operator.wallet).protectModule(subjectModule, [subjectExtension]);
       });
 
       it("should revert", async() => {