Merge pull request #96 from keep-network/fully-backed-ban

Ban operator in FullyBackedSortitionPool

Added functionality of operators banning in 
FullyBackedSortitionPool contract.

A pool owner can call a function to ban an operator. 
Once an operator gets banned it is removed from the pool. 
The operator won't get selected to new groups.

An operator can be banned even if it has never been 
registered or is not currently registered in the pool.

A banned operator cannot join a pool.

Author: pdyraga

contracts/FullyBackedSortitionPool.sol

@@ -56,6 +56,10 @@ contract FullyBackedSortitionPool is AbstractSortitionPool {
 
   PoolParams poolParams;
 
+  // Banned operator addresses. Banned operators are removed from the pool; they
+  // won't be selected to groups and won't be able to join the pool.
+  mapping(address => bool) public bannedOperators;
+
   constructor(
     IFullyBackedBonding _bondingContract,
     uint256 _initialMinimumBondableValue,
@@ -87,9 +91,9 @@ contract FullyBackedSortitionPool is AbstractSortitionPool {
     uint256 groupSize,
     bytes32 seed,
     uint256 bondValue
-  ) public returns (address[] memory) {
+  ) public onlyOwner() returns (address[] memory) {
     PoolParams memory params = initializeSelectionParams(bondValue);
-    require(msg.sender == params.owner, "Only owner may select groups");
+
     uint256 paramsPtr;
     // solium-disable-next-line security/no-inline-assembly
     assembly {
@@ -119,6 +123,18 @@ contract FullyBackedSortitionPool is AbstractSortitionPool {
     return poolParams.minimumBondableValue;
   }
 
+  /// @notice Bans an operator in the pool. The operator is added to banned
+  /// operators list. If the operator is already registered in the pool it gets
+  /// removed. Only onwer of the pool can call this function.
+  /// @param operator An operator address.
+  function ban(address operator) public onlyOwner() {
+    bannedOperators[operator] = true;
+
+    if (isOperatorRegistered(operator)) {
+      removeOperator(operator);
+    }
+  }
+
   function initializeSelectionParams(uint256 bondValue)
     internal
     returns (PoolParams memory params)
@@ -136,6 +152,11 @@ contract FullyBackedSortitionPool is AbstractSortitionPool {
   // which may differ from the weight in the pool.
   // Return 0 if ineligible.
   function getEligibleWeight(address operator) internal view returns (uint256) {
+    // Check if the operator has been banned.
+    if (bannedOperators[operator]) {
+      return 0;
+    }
+
     address ownerAddress = poolParams.owner;
     // Get the amount of bondable value available for this pool.
     // We only care that this covers one single bond
@@ -234,4 +255,9 @@ contract FullyBackedSortitionPool is AbstractSortitionPool {
     // but reasonable to begin with.
     return Fate(Decision.UpdateSelect, postWeight);
   }
+
+  modifier onlyOwner() {
+    require(msg.sender == poolParams.owner, "Caller is not the owner");
+    _;
+  }
 }

test/fullyBackedSortitionPoolTest.js

@@ -94,6 +94,19 @@ contract("FullyBackedSortitionPool", (accounts) => {
 
       assert.isTrue(await pool.isOperatorInitialized(alice))
     })
+
+    it("reverts when operator gets banned in the pool", async () => {
+      await prepareOperator(alice, bond)
+
+      await mineBlocks(operatorInitBlocks.addn(1))
+
+      await pool.ban(alice, {from: owner})
+
+      expectRevert(
+        pool.isOperatorInitialized(alice),
+        "Operator is not in the pool",
+      )
+    })
   })
 
   describe("selectSetGroup", async () => {
@@ -160,7 +173,7 @@ contract("FullyBackedSortitionPool", (accounts) => {
 
       await expectRevert(
         pool.selectSetGroup(3, seed, minimumBondableValue, {from: alice}),
-        "Only owner may select groups",
+        "Caller is not the owner",
       )
     })
 
@@ -221,6 +234,34 @@ contract("FullyBackedSortitionPool", (accounts) => {
       await pool.selectSetGroup(3, seed, minimumBondableValue, {from: owner})
     })
 
+    it("reverts when operator gets banned in the sortition pool", async () => {
+      // Register three operators.
+      await prepareOperator(alice, bond)
+      await prepareOperator(bob, bond)
+      await prepareOperator(carol, bond)
+
+      assert.equal(await pool.operatorsInPool(), 3)
+
+      // Initialization period passed.
+      await mineBlocks(operatorInitBlocks.addn(1))
+
+      await pool.selectSetGroup(2, seed, minimumBondableValue, {from: owner})
+
+      // Ban an operator.
+      await pool.ban(carol, {from: owner})
+
+      assert.equal(
+        await pool.operatorsInPool(),
+        2,
+        "incorrect number of operators after operator ban",
+      )
+
+      await expectRevert(
+        pool.selectSetGroup(3, seed, minimumBondableValue, {from: owner}),
+        "Not enough operators in pool",
+      )
+    })
+
     it("removes minimum-bond-ineligible operators and still works afterwards", async () => {
       await prepareOperator(alice, bond)
       await prepareOperator(bob, bond)
@@ -382,5 +423,57 @@ contract("FullyBackedSortitionPool", (accounts) => {
         "Operator is already registered in the pool",
       )
     })
+
+    it("fails for banned operator", async () => {
+      await pool.ban(alice, {from: owner})
+
+      await bonding.setBondableValue(alice, minimumBondableValue)
+      await bonding.setInitialized(alice, true)
+
+      await expectRevert(pool.joinPool(alice), "Operator not eligible")
+    })
+  })
+
+  describe("ban", async () => {
+    it("adds operator to banned operators", async () => {
+      expect(await pool.bannedOperators(alice)).to.be.false
+
+      await pool.ban(alice, {from: owner})
+
+      expect(await pool.bannedOperators(alice)).to.be.true
+    })
+
+    it("does not revert when called multiple times", async () => {
+      expect(await pool.bannedOperators(alice)).to.be.false
+
+      await pool.ban(alice, {from: owner})
+      await pool.ban(alice, {from: owner})
+
+      expect(await pool.bannedOperators(alice)).to.be.true
+    })
+
+    it("does not revert when operator is not registered", async () => {
+      expect(await pool.isOperatorRegistered(alice)).to.be.false
+
+      await pool.ban(alice, {from: owner})
+    })
+
+    it("removes operator from the pool", async () => {
+      await bonding.setBondableValue(alice, minimumBondableValue)
+      await bonding.setInitialized(alice, true)
+      await pool.joinPool(alice)
+
+      expect(await pool.isOperatorRegistered(alice)).to.be.true
+
+      await pool.ban(alice, {from: owner})
+
+      expect(await pool.isOperatorRegistered(alice)).to.be.false
+      expect(await pool.isOperatorInPool(alice)).to.be.false
+      expect(await pool.getPoolWeight(alice)).to.eq.BN(0)
+    })
+
+    it("reverts when called by non-owner", async () => {
+      await expectRevert(pool.ban(alice), "Caller is not the owner")
+    })
   })
 })