From d82c022ce97c8784d95dc2472349782c8d266e8d Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 30 Sep 2020 14:40:58 +0200 Subject: [PATCH 1/4] Added operators banning to FullyBackedSortitionPool Added a 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. 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. --- contracts/FullyBackedSortitionPool.sol | 26 ++++++++ test/fullyBackedSortitionPoolTest.js | 87 ++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) diff --git a/contracts/FullyBackedSortitionPool.sol b/contracts/FullyBackedSortitionPool.sol index 18d7f49b..251c18d7 100644 --- a/contracts/FullyBackedSortitionPool.sol +++ b/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, @@ -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"); + _; + } } diff --git a/test/fullyBackedSortitionPoolTest.js b/test/fullyBackedSortitionPoolTest.js index dc0743bc..88d7d671 100644 --- a/test/fullyBackedSortitionPoolTest.js +++ b/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 () => { @@ -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,51 @@ 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 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, {from: owner}), + "Caller is not the owner", + ) + }) }) }) From f9070b515fb00fe8c95f3569e599398679087ca4 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 30 Sep 2020 14:47:57 +0200 Subject: [PATCH 2/4] Use onlyOwner modifier in selectSetGroup of FullyBackedSortitionPool We use an modifier that is already defined to check if a function is called by a pool owner. The change results in a revert message change from `Only owner may select groups` to `Caller is not the owner`. --- contracts/FullyBackedSortitionPool.sol | 4 ++-- test/fullyBackedSortitionPoolTest.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/FullyBackedSortitionPool.sol b/contracts/FullyBackedSortitionPool.sol index 251c18d7..a26d7c84 100644 --- a/contracts/FullyBackedSortitionPool.sol +++ b/contracts/FullyBackedSortitionPool.sol @@ -91,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 { diff --git a/test/fullyBackedSortitionPoolTest.js b/test/fullyBackedSortitionPoolTest.js index 88d7d671..05a606fa 100644 --- a/test/fullyBackedSortitionPoolTest.js +++ b/test/fullyBackedSortitionPoolTest.js @@ -173,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", ) }) From 9cf7a173b34589692a996844d29e6f77f1e9e8cf Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Wed, 30 Sep 2020 15:01:02 +0200 Subject: [PATCH 3/4] Fixed failing owner test --- test/fullyBackedSortitionPoolTest.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/fullyBackedSortitionPoolTest.js b/test/fullyBackedSortitionPoolTest.js index 05a606fa..97365d3f 100644 --- a/test/fullyBackedSortitionPoolTest.js +++ b/test/fullyBackedSortitionPoolTest.js @@ -173,7 +173,7 @@ contract("FullyBackedSortitionPool", (accounts) => { await expectRevert( pool.selectSetGroup(3, seed, minimumBondableValue, {from: alice}), - "Caller is not the owner", + "Only owner may select groups", ) }) @@ -464,10 +464,7 @@ contract("FullyBackedSortitionPool", (accounts) => { }) it("reverts when called by non-owner", async () => { - await expectRevert( - pool.ban(alice, {from: owner}), - "Caller is not the owner", - ) + await expectRevert(pool.ban(alice), "Caller is not the owner") }) }) }) From 0364f52d4d3515712bf1615954917514b99f4367 Mon Sep 17 00:00:00 2001 From: Jakub Nowakowski Date: Fri, 2 Oct 2020 09:36:56 +0200 Subject: [PATCH 4/4] Updated expected error message for non-owner test --- test/fullyBackedSortitionPoolTest.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/fullyBackedSortitionPoolTest.js b/test/fullyBackedSortitionPoolTest.js index 97365d3f..b926b9aa 100644 --- a/test/fullyBackedSortitionPoolTest.js +++ b/test/fullyBackedSortitionPoolTest.js @@ -173,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", ) }) @@ -443,6 +443,15 @@ contract("FullyBackedSortitionPool", (accounts) => { 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