Feat: JBDirectory controller whitelist (jbx-protocol#37)

* Initial implementation, no tests

* Whoops

* feat: whitelist mapping

* gitignore update: vscode

* wip: override needs to be in knownControllers

* fix: changed isKnownController in IJBDirectory

* Fix: I swear I compiled it

* WIP: Review access logic for setControllerOf(...)

* feat: override in setControllerOf(...)

* feat: isKnownController in test for 100% coverage

* fix: unused var

* fix: test wording

* Feat: setControllerAllowList

* wip

* feat: allowedToSetController+tests

* Update .gitignore

* fix: review comment implem

* Fix: camel case + other format

Co-authored-by: Exekias <[email protected]>

.gitignore

@@ -13,4 +13,7 @@ deployments/localhost/**
 coverage/
 coverage.json
 
-*.txt
+*.txt
+
+# VSCode
+workspace.code-workspace

contracts/JBDirectory.sol

@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: MIT
 pragma solidity 0.8.6;
 
+import '@openzeppelin/contracts/access/Ownable.sol';
+
 import './interfaces/IJBTerminal.sol';
 import './interfaces/IJBDirectory.sol';
 import './abstract/JBOperatable.sol';
@@ -10,7 +12,7 @@ import './libraries/JBOperations.sol';
   @notice
   Keeps a reference of which terminal contracts each project is currently accepting funds through, and which controller contract is managing each project's tokens and funding cycles.
 */
-contract JBDirectory is IJBDirectory, JBOperatable {
+contract JBDirectory is IJBDirectory, JBOperatable, Ownable {
   //*********************************************************************//
   // --------------------- private stored properties ------------------- //
   //*********************************************************************//
@@ -32,6 +34,12 @@ contract JBDirectory is IJBDirectory, JBOperatable {
   */
   mapping(uint256 => mapping(address => IJBTerminal)) private _primaryTerminalOf;
 
+  /**
+    @notice
+    Addresses that can set a project's controller. These addresses/contracts have been vetted and verified by Juicebox owners.
+   */
+  mapping(address => bool) private _setControllerAllowlist;
+
   //*********************************************************************//
   // ---------------- public immutable stored properties --------------- //
   //*********************************************************************//
@@ -142,6 +150,18 @@ contract JBDirectory is IJBDirectory, JBOperatable {
     return IJBTerminal(address(0));
   }
 
+  /**
+    @notice
+    Whether or not a specified address is allowed to set controllers.
+
+    @param _address the address to check
+
+    @return A flag indicating whether or not the specified address can change controllers.
+  */
+  function isAllowedToSetController(address _address) public view override returns (bool) {
+    return _setControllerAllowlist[_address];
+  }
+
   //*********************************************************************//
   // -------------------------- constructor ---------------------------- //
   //*********************************************************************//
@@ -154,43 +174,31 @@ contract JBDirectory is IJBDirectory, JBOperatable {
     projects = _projects;
   }
 
-  //*********************************************************************//
-  // ---------------------- external transactions ---------------------- //
-  //*********************************************************************//
-
   /**
     @notice
     Update the controller that manages how terminals interact with the ecosystem.
-
     @dev 
     A controller can be set if:
-    - the message sender is the project owner or an operator is changing the controller.
-    - or, the controller hasn't been set yet and the message sender is the controller being set.
-    - or, the current controller is setting a new controller.
-
+    - the message sender is the project owner or an operator having the correct authorization.
+    - or, an allowed address is setting a new controller.
     @param _projectId The ID of the project to set a new controller for.
     @param _controller The new controller to set.
   */
-  // TODO(odd-amphora): Revisit access pattern with allowlist.
   function setControllerOf(uint256 _projectId, IJBController _controller)
     external
     override
     requirePermissionAllowingOverride(
       projects.ownerOf(_projectId),
       _projectId,
       JBOperations.SET_CONTROLLER,
-      (address(controllerOf[_projectId]) == address(0) && msg.sender == address(_controller)) ||
-        address(controllerOf[_projectId]) == msg.sender
+      (_setControllerAllowlist[address(_controller)] && _setControllerAllowlist[msg.sender])
     )
   {
     // Can't set the zero address.
     require(_controller != IJBController(address(0)), '0x2b: ZERO_ADDRESS');
 
-    // Get a reference to the current controller being used.
-    IJBController _currentController = controllerOf[_projectId];
-
     // If the controller is already set, nothing to do.
-    if (_currentController == _controller) return;
+    if (controllerOf[_projectId] == _controller) return;
 
     // The project must exist.
     require(projects.count() >= _projectId, '0x2c: NOT_FOUND');
@@ -319,4 +327,42 @@ contract JBDirectory is IJBDirectory, JBOperatable {
 
     emit SetPrimaryTerminal(_projectId, _token, _terminal, msg.sender);
   }
+
+  /** 
+    @notice
+    The owner (Juicebox multisig) can add addresses which are allowed to change
+    a project's controller. Those addresses are known and vetted controllers as well as
+    contracts designed to launch new projects. This is not a requirement for all controllers.
+    However, unknown controllers may require additional transactions to perform certain operations.
+
+    @dev
+    If you would like an address/contract allowlisted, please reach out to the Juicebox dev team.
+
+    @param _address the allowed address to be added.
+  */
+  function addToSetControllerAllowlist(address _address) external override onlyOwner {
+    // Check that the controller has not already been added.
+    require(!_setControllerAllowlist[_address], '0x30: ALREADY_ADDED');
+
+    // Add the controller to the list of known controllers.
+    _setControllerAllowlist[_address] = true;
+
+    emit AddToSetControllerAllowlist(_address, msg.sender);
+  }
+
+  /** 
+    @notice
+    See `addKnownController(...)` for context. Removes an address from the allowlist.
+
+    @param _address The address to be removed.
+  */
+  function removeFromSetControllerAllowlist(address _address) external override onlyOwner {
+    // Not in the known controllers list
+    require(_setControllerAllowlist[_address], '0x31: NOT_FOUND');
+
+    // Remove the controller from the list of known controllers.
+    delete _setControllerAllowlist[_address];
+
+    emit RemoveFromSetControllerAllowlist(_address, msg.sender);
+  }
 }

contracts/interfaces/IJBDirectory.sol

@@ -19,10 +19,16 @@ interface IJBDirectory {
     address caller
   );
 
+  event AddToSetControllerAllowlist(address indexed _address, address caller);
+
+  event RemoveFromSetControllerAllowlist(address indexed _address, address caller);
+
   function projects() external view returns (IJBProjects);
 
   function controllerOf(uint256 _projectId) external view returns (IJBController);
 
+  function isAllowedToSetController(address _address) external view returns (bool);
+
   function primaryTerminalOf(uint256 _projectId, address _token)
     external
     view
@@ -41,4 +47,8 @@ interface IJBDirectory {
   function setControllerOf(uint256 _projectId, IJBController _controller) external;
 
   function setPrimaryTerminalOf(uint256 _projectId, IJBTerminal _terminal) external;
+
+  function addToSetControllerAllowlist(address _address) external;
+
+  function removeFromSetControllerAllowlist(address _address) external;
 }

deploy/deploy.js

@@ -1,3 +1,5 @@
+const { ethers } = require("hardhat");
+
 /**
  * Deploys the Juice V2 contracts.
  *
@@ -73,7 +75,7 @@ module.exports = async ({ getNamedAccounts, deployments, getChainId }) => {
     log: true,
   });
 
-  await deploy('JBController', {
+  const JBController = await deploy('JBController', {
     from: deployer,
     args: [
       JBOperatorStore.address,
@@ -86,6 +88,11 @@ module.exports = async ({ getNamedAccounts, deployments, getChainId }) => {
     log: true,
   });
 
+  // Add the deployed JBController as a known controller.
+  const [signer, ..._] = await ethers.getSigners()
+  const jbDirectoryContract = new ethers.Contract(JBDirectory.address, JBDirectory.abi);
+  await jbDirectoryContract.connect(signer).addToSetControllerAllowlist(JBController.address)
+
   const JBETHPaymentTerminalStore = await deploy('JBETHPaymentTerminalStore', {
     from: deployer,
     args: [

test/jb_directory/add_set_controller_allowed_list.test.js

@@ -0,0 +1,64 @@
+import { expect } from 'chai';
+import { ethers } from 'hardhat';
+
+import { deployMockContract } from '@ethereum-waffle/mock-contract';
+
+import jbController from '../../artifacts/contracts/interfaces/IJBController.sol/IJBController.json';
+import jbOperatoreStore from '../../artifacts/contracts/JBOperatorStore.sol/JBOperatorStore.json';
+import jbProjects from '../../artifacts/contracts/JBProjects.sol/JBProjects.json';
+
+describe('JBDirectory::addToSetControllerAllowlist(...)', function () {
+
+  async function setup() {
+    let [deployer, ...addrs] = await ethers.getSigners();
+
+    let mockJbOperatorStore = await deployMockContract(deployer, jbOperatoreStore.abi);
+    let mockJbProjects = await deployMockContract(deployer, jbProjects.abi);
+    let mockJbController = await deployMockContract(deployer, jbController.abi);
+
+    let jbDirectoryFactory = await ethers.getContractFactory('JBDirectory');
+    let jbDirectory = await jbDirectoryFactory.deploy(
+      mockJbOperatorStore.address,
+      mockJbProjects.address,
+    );
+
+    return {
+      deployer,
+      addrs,
+      jbDirectory,
+      mockJbController
+    };
+  }
+
+  it('Should add known controller and emit events if caller is JBDirectory owner', async function () {
+    const { deployer, jbDirectory, mockJbController } = await setup();
+
+    await expect(
+      jbDirectory.connect(deployer).addToSetControllerAllowlist(mockJbController.address)
+    ).to.emit(jbDirectory, 'AddToSetControllerAllowlist')
+      .withArgs(mockJbController.address, deployer.address);
+
+    expect(
+      await jbDirectory.isAllowedToSetController(mockJbController.address)
+    ).to.be.true;
+  });
+
+  it('Can\'t add known controller if caller is not JBDirectory owner', async function () {
+    const { addrs, jbDirectory, mockJbController } = await setup();
+
+    await expect(
+      jbDirectory.connect(addrs[0]).addToSetControllerAllowlist(mockJbController.address)
+    ).to.revertedWith('Ownable: caller is not the owner');
+  });
+
+  it('Can\'t add the same known controller twice', async function () {
+    const { deployer, jbDirectory, mockJbController } = await setup();
+
+    await jbDirectory.connect(deployer).addToSetControllerAllowlist(mockJbController.address)
+
+    await expect(
+      jbDirectory.connect(deployer).addToSetControllerAllowlist(mockJbController.address)
+    ).to.revertedWith('0x30: ALREADY_ADDED');
+  });
+
+});

test/jb_directory/add_terminals_of.test.js

@@ -115,7 +115,7 @@ describe('JBDirectory::addTerminalsOf(...)', function () {
       .be.reverted;
   });
 
-  it('Should reject if caller does not have permission', async function () {
+  it('Can\'t add if caller does not have permission', async function () {
     const { addrs, projectOwner, jbDirectory, mockJbProjects, mockJbOperatorStore, terminal1 } =
       await setup();
     const caller = addrs[1];
@@ -130,7 +130,7 @@ describe('JBDirectory::addTerminalsOf(...)', function () {
       .reverted;
   });
 
-  it('Should reject terminals with address(0)', async function () {
+  it('Can\'t add terminals with address(0)', async function () {
     const { projectOwner, jbDirectory, terminal1, terminal2 } = await setup();
 
     let terminals = [terminal1.address, ethers.constants.AddressZero, terminal2.address];
@@ -140,7 +140,7 @@ describe('JBDirectory::addTerminalsOf(...)', function () {
     ).to.be.revertedWith('0x2d: ZERO_ADDRESS');
   });
 
-  it('Should not add terminals more than once', async function () {
+  it('Can\'t add terminals more than once', async function () {
     const { projectOwner, jbDirectory, terminal1, terminal2 } = await setup();
 
     await jbDirectory

test/jb_directory/is_terminal_of.test.js

@@ -52,7 +52,7 @@ describe('JBDirectory::isTerminalOf(...)', function () {
     return { projectOwner, deployer, addrs, jbDirectory, terminal1, terminal2 };
   }
 
-  it('Returns true if the terminal belongs to the project', async function () {
+  it('Should return true if the terminal belongs to the project', async function () {
     const { projectOwner, jbDirectory, terminal1, terminal2 } = await setup();
 
     expect(await jbDirectory.connect(projectOwner).isTerminalOf(PROJECT_ID, terminal1.address)).to
@@ -62,7 +62,7 @@ describe('JBDirectory::isTerminalOf(...)', function () {
       .be.true;
   });
 
-  it(`Returns false if the terminal doesn't belong to the project`, async function () {
+  it(`Should return false if the terminal doesn't belong to the project`, async function () {
     const { projectOwner, jbDirectory } = await setup();
 
     expect(
@@ -72,7 +72,7 @@ describe('JBDirectory::isTerminalOf(...)', function () {
     ).to.be.false;
   });
 
-  it(`Returns false if the project does not exist`, async function () {
+  it(`Should return false if the project does not exist`, async function () {
     const { projectOwner, jbDirectory } = await setup();
 
     expect(

test/jb_directory/primary_terminal_of.test.js

@@ -62,7 +62,7 @@ describe('JBDirectory::primaryTerminalOf(...)', function () {
     return { projectOwner, deployer, addrs, jbDirectory, terminal1, terminal2 };
   }
 
-  it('Returns primary terminal if set', async function () {
+  it('Should return primary terminal if set', async function () {
     const { projectOwner, jbDirectory, terminal1 } = await setup();
 
     let token = ethers.Wallet.createRandom().address;
@@ -75,7 +75,7 @@ describe('JBDirectory::primaryTerminalOf(...)', function () {
     );
   });
 
-  it('Returns terminal with matching token if set', async function () {
+  it('Should return terminal with matching token if set', async function () {
     const { projectOwner, jbDirectory, terminal1, terminal2 } = await setup();
 
     await terminal1.mock.token.returns(ethers.Wallet.createRandom().address);