Add Manager permissions for protected modules and extensions #71

cgewecke · 2021-08-13T03:40:46Z

PR implements

Summary

Makes fee, fee split and fee recipient updates mutualUpgrade in existing fee extension contracts
Makes fee extension address the default feeRecipient to firewall fees from other extensions.
Adds methods to guarantee that operator cannot unilaterally change fee arrangements.

New Permissions

The permissions model proposed here has the following properties:

supports a registry of protected modules and defines which extensions may call them
protected modules are configured after deployment before the contract is enabled
the methodologist must enable the manager contract before it can begin operation
updates to protected module arrangements can only be performed by mutual upgrade, except under emergency conditions
protected modules cannot be unilaterally removed, except under emergency circumstances
extensions authorized for protected modules cannot be unilaterally removed, except under emergency conditions
the operator can protect new modules at their discretion, on behalf of the methodologist
the methodologist can un-protect modules at their discretion, on behalf of the operator

TODO

Debug remaining skipped tests
Events tests
Update spec with new getters
Add method to allow operator to set an address which will receive its fees.
- IIP-72
- (StreamingFeeSplitExtension) fe959c5
- (FeeSplitAdapter) aa126c4
Resolve accrual protection design questions
Format as new manager contract, renaming adapter as extension.
Fixes for second review sweep

Open Questions

Have retained the "adapter" naming convention to reduce noise in the diff. This PR might be the right time to rename everything extension?

There's an asymmetry in the preconditions for replacing modules vs. protecting them.

Replacing a module adds and removes from the SetToken, meaning the replacement cannot already be added
Protecting a module (operator only) requires that the module already be added.

The idea behind this is that in the "replacement" scenario, we don't really want the operator to double-up fee modules - it's cleaner to model as an atomic swap. In the "protect" scenario, the operator is adding new features and should have more flexibility. Does this make sense? Or should behavior in these two scenarios be standardized?

bweick

Couple logic things I think we wanna clean up and then let's try to xfer everything to Extension terminology at this point, have a feeling we'll be using this new manager going forward (hence wanna have it in a new file).

One note that may be good to add is that Extensions can be added through:
addAdapter (Extension)
protectModule
replaceProtectedModule
emergencyReplaceProtectedModule
authorizeAdapter (Extension...and if we make the logic update I proposed)

Extensions can be removed (from adapters array):
removeAdapter (Extension)

Extensions can be authorized:
protectModule
replaceProtectedModule
emergencyReplaceProtectedModule
authorizeAdapter (Extension...and if we make the logic update I proposed)

Extensions can be deauthorized:
replaceProtectedModule
emergencyRemoveProtectedModule
unProtectModule

Want to make sure that's your understanding as well. Think it's important to have those mapped out, that's the part I found the most confusing about it is the different Extension states.

bweick · 2021-08-15T18:20:49Z

contracts/adapters/FeeSplitAdapter.sol



 /**
 * @title FeeSplitAdapter


Ah let's take this opportunity to rename the all the leftover "adapters" to "extensions"

These renamings done from ea81801 to cbb6b7c

bweick · 2021-08-15T18:23:33Z

contracts/adapters/FeeSplitAdapter.sol

        streamingFeeModule.accrueFee(setToken);
-        
+
        uint256 totalFees = setToken.balanceOf(address(manager));


To ensure that all fees can't be poached we actually want to send fees to this extension now. So we should be reading the balance on this contract

This change is in feaf6a2.

This approach requires that any time a fee extension contract is added or replaced the fee recipient is updated (by mutual upgrade) as well.

Have added a distribute method to the extensions that lets distribution happen whether the extension is authorized or not. This might be necessary if accrue is called on the streaming fee module during a transition from one extension to another, or fees went undistributed for other reasons.

[EDIT - distribute is unnecessary because there's no routing through the manager. Have added a behavioral test that makes sure we can sweep accrued fees from a de-authorized and removed extension since this is something that might be necessary]

contracts/adapters/FeeSplitAdapter.sol

contracts/adapters/StreamingFeeSplitExtension.sol

bweick · 2021-08-15T18:27:48Z

contracts/manager/BaseManager.sol

 */

 pragma solidity 0.6.10;
+pragma experimental ABIEncoderV2;


Sorry I should have clarified this...I think there's enough changes here that it warrants a different manager contract. I want to preserve the old contract so maybe we make this PermissionedManager or something to that affect. There's probably a better name...

Also given we're trying to move towards the Extension nomenclature and we're changing a lot about this BaseManager can we change the adapter to extension. I should have specified that in the spec, that's my bad.

Ah, I overlooked the suggestion to rename the manager descriptively and did V2.....

bweick · 2021-08-15T18:49:51Z

contracts/manager/BaseManager.sol

+     * OPERATOR ONLY: Marks a currently protected module as unprotected and deletes its authorized
+     * adapter registries. Removes module from the SetToken. Increments the `emergencies` counter,
+     * prohibiting any operator-only module or extension additions until `emergencyReplaceProtectedModule`
+     * is executed.


ro resolveEmergency is called by the methodologist

bweick · 2021-08-15T18:53:32Z

contracts/manager/BaseManager.sol

+            protectedModules[_module].authorizedAdapters[adapter] = false;
+        }
+
+        delete protectedModules[_module].authorizedAdaptersList;


Do we need to delete this if we're deleting protectedModules[_module]

You're right, this is unnecessary. Removed in 7bdb9c9.

(Aside, this case is tested here)

bweick · 2021-08-15T19:06:18Z

contracts/manager/BaseManager.sol

     * @param _module           Module to remove
     */
    function removeModule(address _module) external onlyOperator {
+        require(!protectedModules[_module].isProtected, "Module protected");


Let's add a note that any extensions associated with the module need to be removed in a separate transaction

bweick · 2021-08-15T19:10:57Z

contracts/manager/BaseManager.sol

+     * @param  _module          Module to protect
+     * @param  _adapters        Array of adapters to authorize for protected module
+     */
+    function protectModule(address _module, address[] memory _adapters)


Seems like there's an inconsistency in extension authorization between this and authorizeAdapter. Here there's no check to see if the extension has already been added (not authorized!) whereas the authorizeAdapter there's an explicit check to see if it has been added:

See Line 259

I would tend towards not requiring the extension to have been "added" before being "authorized"

As noted above - authorizeAdapter and protectModule now both automatically add the adapter if missing with ebd8676

bweick · 2021-08-15T19:13:21Z

contracts/manager/BaseManager.sol

+
+    /**
+     * MUTUAL UPGRADE**: Revokes adapter authorization for a protected module. Operator and Methodologist
+     * must each call this function to execute the update.


Would note here that in order to remove the extension completely from the contract removeAdapter must be called by the operator

bweick

Couple small things but looks really good to go to me. The tests especially are amazing, super well done and thorough. Those tests help inspire confidence.

bweick · 2021-08-18T05:00:55Z

contracts/adapters/FeeSplitExtension.sol

     * ANYONE CALLABLE: Accrues fees from streaming fee module. Gets resulting balance after fee accrual, calculates fees for
-     * operator and methodologist, and sends to each. NOTE: mint/redeem fees will automatically be sent to this address so reading
-     * the balance of the SetToken in the contract after accrual is sufficient for accounting for all collected fees.
+     * operator and methodologist, and sends to operator and methodologist, and sends to operatorFeeRecipient and methodlogist


Redundant statement here. Also methodologist

bweick · 2021-08-18T05:02:03Z

contracts/adapters/FeeSplitExtension.sol


-    event FeesAccrued(address indexed _operator, address indexed _methodologist, uint256 _operatorTake, uint256 _methodologistTake);
-
+    event FeesDistributed(


Don't know if IC analytics is looking for the FeesAccrued event but should let them know this is changed if they are. Think it's definitely better naming

bweick · 2021-08-18T05:03:10Z

contracts/adapters/FeeSplitExtension.sol

    /**
-     * ONLY OPERATOR: Updates streaming fee on StreamingFeeModule. NOTE: This will accrue streaming fees though not send to operator
-     * and methodologist.
+     * MUTUAL UPGRADE: Updates streaming fee on StreamingFeeModule. NOTE: This will accrue streaming


Maybe we should make a note for all of these mutualUpgrade and timeLockUpgrade combo functions that it will require each party submitting two txns. One to set the timelock and the next to execute the function

bweick · 2021-08-18T05:27:23Z

contracts/manager/BaseManager.sol

-        require(isAdapter[_adapter], "Adapter does not exist");
        require(!protectedModules[_module].authorizedAdapters[_adapter], "Adapter already authorized");

+        if (!isAdapter[_adapter]) {


Bit of a nit: But we can maybe DRY this up a bit more by making an internal function that does the state updates. That can be used here and in protectModule.

bweick · 2021-08-18T05:33:02Z

test/manager/baseManagerV2.spec.ts

+        subjectProtectedModules = [subjectModule];
+      });
+
+      // This test is borked... module initialized in before block..


what's wrong with this test?

Whoah! Glad you flagged this. The constructor does not add modules to the SetToken. I don't this this is possible - the setToken.addModule call reverts with Manager only because the manager contract doesn't technically exist yet.

So ... this situation is a little weird. You could protect a module in the Manager deployment which isn't on the token.

Have removed the bad test and replaced with one where non-added module is protected in the constructor and operator adds module after deployment.

62c226a

Yeah I think this is ok

bweick · 2021-08-18T05:36:01Z

test/manager/baseManagerV2.spec.ts

+      });
+    });
+
+    // This test for the coverage report - hits an alternate branch condition the authorized


good to note

bweick · 2021-08-18T05:41:31Z

test/manager/baseManagerV2.spec.ts

@@ -0,0 +1,1710 @@
+import "module-alias/register";


These tests are amazing and so thorough. Well laid out and even checking to make sure all critical pieces of state remain unchanged within different flows. This is chef's kiss.

bweick · 2021-08-18T16:04:13Z

contracts/manager/BaseManagerV2.sol

+
+        _protectModule(_newModule, _newExtensions);
+
+        ReplacedProtectedModule(_oldModule, _newModule, _newExtensions);


Do we not need the emit here?

Apparently it just works. However added the missing ones for consistency's sake in 87c8fcf

cgewecke · 2021-08-18T23:35:31Z

Have added some new changes to address bugs / issues discovered this morning:

The constructor logic that protects modules had a circular dependency with the Manager. Have removed that and added a test for the flow where:
- manager is deployed
- modules are protected as necessary
- methodologist reviews the config and calls authorizeInitialization.

The replaceProtectedModule -type methods add modules to the SetToken. These need to be initialized via the manager before they can be used. initialize methods for the relevant modules are added to the extensions in

edb15b9 (StreamingFeeSplitExtension)
5bbf4f0 (FeeSplitExtension)

Updated javadocs for GeneralIndexModule.

cgewecke added 13 commits August 10, 2021 18:36

Make fee extension setters use mutualUpgrade

8e5eb4b

Distribute fees when updating streaming fee

1f51df9

BaseManager initial draft

a41eb81

Add protect, unprotect and resolveEmergency methods. Add events

d699ca9

Update tests for new BaseManager constructor / initialization

12fcc40

Test skeleton for initialization, protected modules states

80f268b

Test skeleton for authorized extension, emergency states

92f6747

Add constructor / initialization tests

c3f26fa

Add test for extension authorization

01ba0a8

Add protection, emergency, and replacement tests

b41c2f5

Additional emergency / replacement tests

995d811

Add event tests

a6633a0

Clean up comments in BaseManager.sol

ef4bd8d

cgewecke marked this pull request as ready for review August 13, 2021 14:27

cgewecke added 3 commits August 13, 2021 07:45

Add missing tests for resolveEmergency, revokeAuth

4d2bd77

Add test for alternative branch in auth adapters search fn

07d2c1c

Fix coverage report

fa71e12

cgewecke changed the title ~~[WIP] Add Manager permissions for protected modules and extensions~~ Add Manager permissions for protected modules and extensions Aug 13, 2021

Add onlyEmergency modifier

acf1928

bweick reviewed Aug 15, 2021

View reviewed changes

cgewecke added 8 commits August 16, 2021 08:32

Add explanatory note about emergencies counter

459230a

Make authorizeInitialization single use

537cab7

Additional comment clarifications & cleanup

1392513

Remove redundant list deletion in _unProtectModule

7bdb9c9

#authorizeAdapter: automatically add adapter if missing

ebd8676

StreamingFeeSplitExtension: add configurable operaterFeeRecipient

fe959c5

FeeSplitAdapter: add configurable operatorFeeRecipient

aa126c4

Send fees to fee extension

feaf6a2

cgewecke force-pushed the chris/fee-permissions branch from 63062bd to feaf6a2 Compare August 17, 2021 15:44

Add BaseManagerV2

ea81801

cgewecke added 3 commits August 17, 2021 09:46

Update specs to use BaseManagerV2

b73bece

Rename FeeSplitAdapter --> FeeSplitExtension

6acceb1

Fix extension spec setups for BaseManagerV2

cbe93be

cgewecke force-pushed the chris/fee-permissions branch from 5c16b6a to cbe93be Compare August 17, 2021 18:29

Additional adapter to extension renamings

cbb6b7c

cgewecke force-pushed the chris/fee-permissions branch from e151766 to cbb6b7c Compare August 17, 2021 21:40

bweick reviewed Aug 18, 2021

View reviewed changes

cgewecke added 4 commits August 18, 2021 06:07

Fix FeeSplitAdapter#accrueFeesAndDistribute comment

7754f6b

DRY authorize extension

331a399

Add test for non-added modules on deployment

62c226a

Add comments for mutualUpgrade & timeLockUpgrade modifier combinations

f19d7ea

bweick reviewed Aug 18, 2021

View reviewed changes

cgewecke added 5 commits August 18, 2021 12:29

Remove protection logic from constructor (due to circular dep)

cfbc739

Add missing emit keywords in BaseManagerV2

87c8fcf

Add initialize method to StreamingFeeSplitExtension contract

edb15b9

Add initialize methods to FeeSplitExtension contract

5bbf4f0

Fix import capitalization problem

02b9862

bweick approved these changes Aug 18, 2021

View reviewed changes

cgewecke merged commit 17909eb into master Aug 19, 2021

cgewecke deleted the chris/fee-permissions branch August 19, 2021 00:04

ace3 pushed a commit to ace3/index-coop-smart-contracts that referenced this pull request Feb 2, 2024

Updated javadocs for GeneralIndexModule. (IndexCoop#71)

d4a532d

Updated javadocs for GeneralIndexModule.

		streamingFeeModule.accrueFee(setToken);


		uint256 totalFees = setToken.balanceOf(address(manager));


		event FeesAccrued(address indexed _operator, address indexed _methodologist, uint256 _operatorTake, uint256 _methodologistTake);

		event FeesDistributed(


		_protectModule(_newModule, _newExtensions);

		ReplacedProtectedModule(_oldModule, _newModule, _newExtensions);

Add Manager permissions for protected modules and extensions #71

Add Manager permissions for protected modules and extensions #71

Uh oh!

Conversation

cgewecke commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bweick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgewecke Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bweick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgewecke Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cgewecke commented Aug 13, 2021 •

edited

Loading

cgewecke Aug 17, 2021 •

edited

Loading

cgewecke Aug 18, 2021 •

edited

Loading