Should we introduce FlowAdminHandler or will that be an overkill?

[smol-ninja]

Given that now we have two admin related functions: recover and collectProtocolRevenue, should we have invariants that make sure that any of the admin functions don't lead to an unexpected behaviour? Or will it be an overkill given they are only callable by admin?

It would require us to introduce the following changes:

1. contract FlowAdminHandler
2. functions collectRevenue and recover in FlowAdminHandler
3. variables mapping (IERC20 token => uint256) protocolRevenue and mapping (IERC20 token => uint256) aggregateBalance in FlowStore
4. functions token.transfer so that invariant can make random deposits to Flow contract

And we can then introduce the following invariants:

1. $\text{all stream balance sum} + \text{revenue} = \text{aggregate balance}$
2. $\text{aggregate balance} \le \text{token balance}$

cc @sablier-labs/solidity

[PaulRBerg]

Yes, we should, and no, it wouldn't be overkill. A bug that is only accessible via an admin entry point is still a bug; it would create an incentive for hackers to break into our multisigs.

The invariants make sense to me.

[andreivladbrg]

We should, I don't think it's an overkill, that's what I've also wanted to recommend in the PR #222 (comment)

Now, with the ERC20 recover mechanism (more admin related functions) it is even more useful.

[smol-ninja]

Great. Thank you for the feedback.

• Issue: Implement FlowAdminHandler & invariants to test against admin functions #241