feat: Collector revision-6 Upgrade. (#82)

### Context

The Collector contract (Revision 5) has the limitation of only allowing one address to call its functions, the `_fundsAdmin`, which currently is the Governance Executor contract.

To enable the possibility of the [FinanceSteward](https://governance.aave.com/t/arfc-aave-finance-steward/17570) contract to be allowed to call the Collector alongside the Executor, it is recommended to use the `ACL_MANAGER` to manage its access control. 
A new role is going to be created named `FUNDS_ADMIN`

### Changelog

Collector

* Initialize function changes to only populate the `nextStreamId`
* Created constructor to setup the `ACL_MANAGER` contract address as immutable variable
* deprecated `_fundsAdmin` variable
* removed getter and setter functions for `_fundsAdmin` and relevant errors and events.
* introduced new function to check if an address has the `FUNDS_ADMIN` role named `IsFundsAdmin`
* New tests for the Collector

Author: Luigy-Lemon

src/contracts/treasury/Collector.sol

@@ -2,6 +2,7 @@
 pragma solidity ^0.8.0;
 
 import {ICollector} from './ICollector.sol';
+import {IAccessControl} from '../dependencies/openzeppelin/contracts/IAccessControl.sol';
 import {ReentrancyGuard} from '../dependencies/openzeppelin/ReentrancyGuard.sol';
 import {VersionedInitializable} from '../misc/aave-upgradeability/VersionedInitializable.sol';
 import {IERC20} from '../dependencies/openzeppelin/contracts/IERC20.sol';
@@ -28,14 +29,23 @@ contract Collector is VersionedInitializable, ICollector, ReentrancyGuard {
   /*** Storage Properties ***/
 
   /**
-   * @notice Address of the current funds admin.
+   * @notice Current revision of the contract.
    */
-  address internal _fundsAdmin;
+  uint256 public constant REVISION = 6;
+
+  /// @inheritdoc ICollector
+  address public constant ETH_MOCK_ADDRESS = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
+
+  /// @inheritdoc ICollector
+  bytes32 public constant FUNDS_ADMIN_ROLE = 'FUNDS_ADMIN';
+
+  /// @inheritdoc ICollector
+  address public immutable ACL_MANAGER;
 
   /**
-   * @notice Current revision of the contract.
+   * @notice [DEPRECATED] Use `isFundsAdmin()` to check address.
    */
-  uint256 public constant REVISION = 5;
+  address internal _fundsAdmin_deprecated;
 
   /**
    * @notice Counter for new stream ids.
@@ -47,16 +57,15 @@ contract Collector is VersionedInitializable, ICollector, ReentrancyGuard {
    */
   mapping(uint256 => Stream) private _streams;
 
-  /// @inheritdoc ICollector
-  address public constant ETH_MOCK_ADDRESS = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
-
   /*** Modifiers ***/
 
   /**
-   * @dev Throws if the caller is not the funds admin.
+   * @dev Throws if the caller does not have the FUNDS_ADMIN role
    */
   modifier onlyFundsAdmin() {
-    require(msg.sender == _fundsAdmin, 'ONLY_BY_FUNDS_ADMIN');
+    if (_onlyFundsAdmin() == false) {
+      revert OnlyFundsAdmin();
+    }
     _;
   }
 
@@ -65,30 +74,32 @@ contract Collector is VersionedInitializable, ICollector, ReentrancyGuard {
    * @param streamId The id of the stream to query.
    */
   modifier onlyAdminOrRecipient(uint256 streamId) {
-    require(
-      msg.sender == _fundsAdmin || msg.sender == _streams[streamId].recipient,
-      'caller is not the funds admin or the recipient of the stream'
-    );
+    if (_onlyFundsAdmin() == false && msg.sender != _streams[streamId].recipient) {
+      revert OnlyFundsAdminOrRceipient();
+    }
     _;
   }
 
   /**
    * @dev Throws if the provided id does not point to a valid stream.
    */
   modifier streamExists(uint256 streamId) {
-    require(_streams[streamId].isEntity, 'stream does not exist');
+    if (!_streams[streamId].isEntity) revert StreamDoesNotExist();
     _;
   }
 
+  constructor(address aclManager) {
+    if (aclManager == address(0)) revert InvalidZeroAddress();
+    ACL_MANAGER = aclManager;
+  }
+
   /*** Contract Logic Starts Here */
 
   /// @inheritdoc ICollector
-  function initialize(address fundsAdmin, uint256 nextStreamId) external initializer {
+  function initialize(uint256 nextStreamId) external virtual initializer {
     if (nextStreamId != 0) {
       _nextStreamId = nextStreamId;
     }
-
-    _setFundsAdmin(fundsAdmin);
   }
 
   /*** View Functions ***/
@@ -99,8 +110,8 @@ contract Collector is VersionedInitializable, ICollector, ReentrancyGuard {
   }
 
   /// @inheritdoc ICollector
-  function getFundsAdmin() external view returns (address) {
-    return _fundsAdmin;
+  function isFundsAdmin(address admin) external view returns (bool) {
+    return IAccessControl(ACL_MANAGER).hasRole(FUNDS_ADMIN_ROLE, admin);
   }
 
   /// @inheritdoc ICollector
@@ -195,7 +206,7 @@ contract Collector is VersionedInitializable, ICollector, ReentrancyGuard {
 
   /// @inheritdoc ICollector
   function transfer(IERC20 token, address recipient, uint256 amount) external onlyFundsAdmin {
-    require(recipient != address(0), 'INVALID_0X_RECIPIENT');
+    if (recipient == address(0)) revert InvalidZeroAddress();
 
     if (address(token) == ETH_MOCK_ADDRESS) {
       payable(recipient).sendValue(amount);
@@ -204,18 +215,8 @@ contract Collector is VersionedInitializable, ICollector, ReentrancyGuard {
     }
   }
 
-  /// @inheritdoc ICollector
-  function setFundsAdmin(address admin) external onlyFundsAdmin {
-    _setFundsAdmin(admin);
-  }
-
-  /**
-   * @dev Transfer the ownership of the funds administrator role.
-   * @param admin The address of the new funds administrator
-   */
-  function _setFundsAdmin(address admin) internal {
-    _fundsAdmin = admin;
-    emit NewFundsAdmin(admin);
+  function _onlyFundsAdmin() internal view returns (bool) {
+    return IAccessControl(ACL_MANAGER).hasRole(FUNDS_ADMIN_ROLE, msg.sender);
   }
 
   struct CreateStreamLocalVars {
@@ -244,21 +245,21 @@ contract Collector is VersionedInitializable, ICollector, ReentrancyGuard {
     uint256 startTime,
     uint256 stopTime
   ) external onlyFundsAdmin returns (uint256) {
-    require(recipient != address(0), 'stream to the zero address');
-    require(recipient != address(this), 'stream to the contract itself');
-    require(recipient != msg.sender, 'stream to the caller');
-    require(deposit > 0, 'deposit is zero');
-    require(startTime >= block.timestamp, 'start time before block.timestamp');
-    require(stopTime > startTime, 'stop time before the start time');
+    if (recipient == address(0)) revert InvalidZeroAddress();
+    if (recipient == address(this)) revert InvalidRecipient();
+    if (recipient == msg.sender) revert InvalidRecipient();
+    if (deposit == 0) revert InvalidZeroAmount();
+    if (startTime < block.timestamp) revert InvalidStartTime();
+    if (stopTime <= startTime) revert InvalidStopTime();
 
     CreateStreamLocalVars memory vars;
     vars.duration = stopTime - startTime;
 
     /* Without this, the rate per second would be zero. */
-    require(deposit >= vars.duration, 'deposit smaller than time delta');
+    if (deposit < vars.duration) revert DepositSmallerTimeDelta();
 
     /* This condition avoids dealing with remainders */
-    require(deposit % vars.duration == 0, 'deposit not multiple of time delta');
+    if (deposit % vars.duration > 0) revert DepositNotMultipleTimeDelta();
 
     vars.ratePerSecond = deposit / vars.duration;
 
@@ -302,11 +303,11 @@ contract Collector is VersionedInitializable, ICollector, ReentrancyGuard {
     uint256 streamId,
     uint256 amount
   ) external nonReentrant streamExists(streamId) onlyAdminOrRecipient(streamId) returns (bool) {
-    require(amount > 0, 'amount is zero');
+    if (amount == 0) revert InvalidZeroAmount();
     Stream memory stream = _streams[streamId];
 
     uint256 balance = balanceOf(streamId, stream.recipient);
-    require(balance >= amount, 'amount exceeds the available balance');
+    if (balance < amount) revert BalanceExceeded();
 
     _streams[streamId].remainingBalance = stream.remainingBalance - amount;
 

src/contracts/treasury/ICollector.sol

@@ -16,10 +16,60 @@ interface ICollector {
     bool isEntity;
   }
 
-  /** @notice Emitted when the funds admin changes
-   * @param fundsAdmin The new funds admin.
-   **/
-  event NewFundsAdmin(address indexed fundsAdmin);
+  /**
+   * @dev Withdraw amount exceeds available balance
+   */
+  error BalanceExceeded();
+
+  /**
+   * @dev Deposit smaller than time delta
+   */
+  error DepositSmallerTimeDelta();
+
+  /**
+   * @dev Deposit not multiple of time delta
+   */
+  error DepositNotMultipleTimeDelta();
+
+  /**
+   * @dev Recipient cannot be the contract itself or msg.sender
+   */
+  error InvalidRecipient();
+
+  /**
+   * @dev Start time cannot be before block.timestamp
+   */
+  error InvalidStartTime();
+
+  /**
+   * @dev Stop time must be greater than startTime
+   */
+  error InvalidStopTime();
+
+  /**
+   * @dev Provided address cannot be the zero-address
+   */
+  error InvalidZeroAddress();
+
+  /**
+   * @dev Amount cannot be zero
+   */
+  error InvalidZeroAmount();
+
+  /**
+   * @dev Only caller with FUNDS_ADMIN role can call
+   */
+  error OnlyFundsAdmin();
+
+  /**
+   * @dev Only caller with FUNDS_ADMIN role or stream recipient can call
+   */
+  error OnlyFundsAdminOrRceipient();
+
+  /**
+   * @dev The provided ID does not belong to an existing stream
+   */
+  error StreamDoesNotExist();
 
   /** @notice Emitted when the new stream is created
    * @param streamId The identifier of the stream.
@@ -64,29 +114,38 @@ interface ICollector {
     uint256 recipientBalance
   );
 
+  /**
+   * @notice FUNDS_ADMIN role granted by ACL Manager
+   **/
+  function FUNDS_ADMIN_ROLE() external view returns (bytes32);
+
+  /**
+   * @notice Address of the current ACL Manager.
+   **/
+  function ACL_MANAGER() external view returns (address);
+
   /** @notice Returns the mock ETH reference address
    * @return address The address
    **/
   function ETH_MOCK_ADDRESS() external pure returns (address);
 
   /** @notice Initializes the contracts
-   * @param fundsAdmin Funds admin address
    * @param nextStreamId StreamId to set, applied if greater than 0
    **/
-  function initialize(address fundsAdmin, uint256 nextStreamId) external;
+  function initialize(uint256 nextStreamId) external;
 
   /**
-   * @notice Return the funds admin, only entity to be able to interact with this contract (controller of reserve)
-   * @return address The address of the funds admin
+   * @notice Checks if address is funds admin
+   * @return bool If the address has the funds admin role
    **/
-  function getFundsAdmin() external view returns (address);
+  function isFundsAdmin(address admin) external view returns (bool);
 
   /**
    * @notice Returns the available funds for the given stream id and address.
    * @param streamId The id of the stream for which to query the balance.
    * @param who The address for which to query the balance.
    * @notice Returns the total funds allocated to `who` as uint256.
-   */
+   **/
   function balanceOf(uint256 streamId, address who) external view returns (uint256 balance);
 
   /**
@@ -105,13 +164,6 @@ interface ICollector {
    **/
   function transfer(IERC20 token, address recipient, uint256 amount) external;
 
-  /**
-   * @dev Transfer the ownership of the funds administrator role.
-          This function should only be callable by the current funds administrator.
-   * @param admin The address of the new funds administrator
-   */
-  function setFundsAdmin(address admin) external;
-
   /**
    * @notice Creates a new stream funded by this contracts itself and paid towards `recipient`.
    * @param recipient The address towards which the money is streamed.

src/deployments/contracts/procedures/AaveV3TreasuryProcedure.sol

@@ -15,42 +15,35 @@ contract AaveV3TreasuryProcedure {
   function _deployAaveV3Treasury(
     address poolAdmin,
     address deployedProxyAdmin,
+    address aclManager,
     bytes32 collectorSalt
   ) internal returns (TreasuryReport memory) {
     TreasuryReport memory treasuryReport;
     bytes32 salt = collectorSalt;
     address treasuryOwner = poolAdmin;
 
     if (salt != '') {
-      Collector treasuryImplementation = new Collector{salt: salt}();
-      treasuryImplementation.initialize(address(0), 0);
+      Collector treasuryImplementation = new Collector{salt: salt}(aclManager);
+      treasuryImplementation.initialize(0);
       treasuryReport.treasuryImplementation = address(treasuryImplementation);
 
       treasuryReport.treasury = address(
         new TransparentUpgradeableProxy{salt: salt}(
           treasuryReport.treasuryImplementation,
           ProxyAdmin(deployedProxyAdmin),
-          abi.encodeWithSelector(
-            treasuryImplementation.initialize.selector,
-            address(treasuryOwner),
-            0
-          )
+          abi.encodeWithSelector(treasuryImplementation.initialize.selector, 100_000)
         )
       );
     } else {
-      Collector treasuryImplementation = new Collector();
-      treasuryImplementation.initialize(address(0), 0);
+      Collector treasuryImplementation = new Collector(aclManager);
+      treasuryImplementation.initialize(0);
       treasuryReport.treasuryImplementation = address(treasuryImplementation);
 
       treasuryReport.treasury = address(
         new TransparentUpgradeableProxy(
           treasuryReport.treasuryImplementation,
           ProxyAdmin(deployedProxyAdmin),
-          abi.encodeWithSelector(
-            treasuryImplementation.initialize.selector,
-            address(treasuryOwner),
-            100_000
-          )
+          abi.encodeWithSelector(treasuryImplementation.initialize.selector, 100_000)
         )
       );
     }

src/deployments/projects/aave-v3-batched/batches/AaveV3PeripheryBatch.sol

@@ -31,10 +31,13 @@ contract AaveV3PeripheryBatch is
 
     _report.aaveOracle = _deployAaveOracle(config.oracleDecimals, poolAddressesProvider);
 
+    address aclManager = address(1); // temp-to-run-tests
+
     if (config.treasury == address(0)) {
       TreasuryReport memory treasuryReport = _deployAaveV3Treasury(
         poolAdmin,
         _report.proxyAdmin,
+        aclManager,
         config.salt
       );
 

tests/deployments/AaveV3PermissionsTest.t.sol

@@ -305,6 +305,7 @@ contract AaveV3PermissionsTest is BatchTestProcedures {
         report.proxyAdmin,
         'Treasury proxy admin does not match with report.proxyAdmin'
       );
+      assertEq(ICollector(report.treasury).ACL_MANAGER(), report.aclManager);
     }
     {
       address proxyAdminOwner = Ownable(report.proxyAdmin).owner();