feat: Collector Upgrade. (#82) (#84)

### 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

Co-authored-by: Luigy-Lemon <[email protected]>
Co-authored-by: Harsh Pandey <[email protected]>

src/contracts/treasury/Collector.sol

@@ -1,12 +1,12 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.0;
 
+import {AccessControlUpgradeable} from 'openzeppelin-contracts-upgradeable/contracts/access/AccessControlUpgradeable.sol';
+import {ReentrancyGuardUpgradeable} from 'openzeppelin-contracts-upgradeable/contracts/utils/ReentrancyGuardUpgradeable.sol';
+import {IERC20} from 'openzeppelin-contracts/contracts/token/ERC20/IERC20.sol';
+import {SafeERC20} from 'openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol';
+import {Address} from 'openzeppelin-contracts/contracts/utils/Address.sol';
 import {ICollector} from './ICollector.sol';
-import {ReentrancyGuard} from '../dependencies/openzeppelin/ReentrancyGuard.sol';
-import {VersionedInitializable} from '../misc/aave-upgradeability/VersionedInitializable.sol';
-import {IERC20} from '../dependencies/openzeppelin/contracts/IERC20.sol';
-import {SafeERC20} from '../dependencies/openzeppelin/contracts/SafeERC20.sol';
-import {Address} from '../dependencies/openzeppelin/contracts/Address.sol';
 
 /**
  * @title Collector
@@ -21,21 +21,25 @@ import {Address} from '../dependencies/openzeppelin/contracts/Address.sol';
  * - Same as with creation, on Sablier the `sender` and `recipient` can cancel a stream. Here, only fund admin and recipient
  * @author BGD Labs
  **/
-contract Collector is VersionedInitializable, ICollector, ReentrancyGuard {
+contract Collector is AccessControlUpgradeable, ReentrancyGuardUpgradeable, ICollector {
   using SafeERC20 for IERC20;
   using Address for address payable;
 
   /*** Storage Properties ***/
+  /// @inheritdoc ICollector
+  address public constant ETH_MOCK_ADDRESS = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
 
-  /**
-   * @notice Address of the current funds admin.
-   */
-  address internal _fundsAdmin;
+  /// @inheritdoc ICollector
+  bytes32 public constant FUNDS_ADMIN_ROLE = 'FUNDS_ADMIN';
 
-  /**
-   * @notice Current revision of the contract.
-   */
-  uint256 public constant REVISION = 5;
+  // Reserved storage space to account for deprecated inherited storage
+  // 0 was lastInitializedRevision
+  // 1-50 were the ____gap
+  // 51 was the reentrancy guard _status
+  // 52 was the _fundsAdmin
+  // On some networks the layout was shifted by 1 due to `initializing` being on slot 1
+  // The upgrade proposal would in this case manually shift the storage layout to properly align the networks
+  uint256[53] private ______gap;
 
   /**
    * @notice Counter for new stream ids.
@@ -47,16 +51,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,43 +68,43 @@ 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 OnlyFundsAdminOrRecipient();
+    }
     _;
   }
 
   /**
    * @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() {
+    _disableInitializers();
+  }
+
   /*** Contract Logic Starts Here */
 
-  /// @inheritdoc ICollector
-  function initialize(address fundsAdmin, uint256 nextStreamId) external initializer {
+  /** @notice Initializes the contracts
+   * @param nextStreamId StreamId to set, applied if greater than 0
+   * @param admin The default admin managing the FundsAdmins
+   **/
+  function initialize(uint256 nextStreamId, address admin) external virtual initializer {
+    __AccessControl_init();
+    __ReentrancyGuard_init();
+    _grantRole(DEFAULT_ADMIN_ROLE, admin);
     if (nextStreamId != 0) {
       _nextStreamId = nextStreamId;
     }
-
-    _initGuard();
-    _setFundsAdmin(fundsAdmin);
   }
 
   /*** View Functions ***/
-
-  /// @inheritdoc VersionedInitializable
-  function getRevision() internal pure override returns (uint256) {
-    return REVISION;
-  }
-
   /// @inheritdoc ICollector
-  function getFundsAdmin() external view returns (address) {
-    return _fundsAdmin;
+  function isFundsAdmin(address admin) external view returns (bool) {
+    return hasRole(FUNDS_ADMIN_ROLE, admin);
   }
 
   /// @inheritdoc ICollector
@@ -191,12 +194,12 @@ contract Collector is VersionedInitializable, ICollector, ReentrancyGuard {
 
   /// @inheritdoc ICollector
   function approve(IERC20 token, address recipient, uint256 amount) external onlyFundsAdmin {
-    token.safeApprove(recipient, amount);
+    token.forceApprove(recipient, amount);
   }
 
   /// @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);
@@ -205,18 +208,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 hasRole(FUNDS_ADMIN_ROLE, msg.sender);
   }
 
   struct CreateStreamLocalVars {
@@ -245,21 +238,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;
 
@@ -303,11 +296,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;
 
@@ -339,4 +332,7 @@ contract Collector is VersionedInitializable, ICollector, ReentrancyGuard {
     emit CancelStream(streamId, stream.sender, stream.recipient, senderBalance, recipientBalance);
     return true;
   }
+
+  /// @dev needed in order to receive ETH from the Aave v1 ecosystem reserve
+  receive() external payable {}
 }

src/contracts/treasury/ICollector.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.0;
 
-import {IERC20} from '../dependencies/openzeppelin/contracts/IERC20.sol';
+import {IERC20} from 'openzeppelin-contracts/contracts/token/ERC20/IERC20.sol';
 
 interface ICollector {
   struct Stream {
@@ -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 OnlyFundsAdminOrRecipient();
+
+  /**
+   * @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,28 @@ interface ICollector {
     uint256 recipientBalance
   );
 
+  /**
+   * @notice FUNDS_ADMIN role granted by ACL Manager
+   **/
+  function FUNDS_ADMIN_ROLE() external view returns (bytes32);
+
   /** @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;
-
   /**
-   * @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 +154,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

@@ -17,38 +17,27 @@ contract AaveV3TreasuryProcedure {
   ) 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);
       treasuryReport.treasuryImplementation = address(treasuryImplementation);
 
       treasuryReport.treasury = address(
         new TransparentUpgradeableProxy{salt: salt}(
           treasuryReport.treasuryImplementation,
           poolAdmin,
-          abi.encodeWithSelector(
-            treasuryImplementation.initialize.selector,
-            address(treasuryOwner),
-            0
-          )
+          abi.encodeWithSelector(treasuryImplementation.initialize.selector, 100_000, poolAdmin)
         )
       );
     } else {
       Collector treasuryImplementation = new Collector();
-      treasuryImplementation.initialize(address(0), 0);
       treasuryReport.treasuryImplementation = address(treasuryImplementation);
 
       treasuryReport.treasury = address(
         new TransparentUpgradeableProxy(
           treasuryReport.treasuryImplementation,
           poolAdmin,
-          abi.encodeWithSelector(
-            treasuryImplementation.initialize.selector,
-            address(treasuryOwner),
-            100_000
-          )
+          abi.encodeWithSelector(treasuryImplementation.initialize.selector, 100_000, poolAdmin)
         )
       );
     }