Ensure validator set unenrolling does not violate a quorum threshold

pull/334/head
Trevor Porter 3 years ago
parent 8e76439a2f
commit 61d8e53ad2
  1. 3
      solidity/core/contracts/validator-manager/MultisigValidatorManager.sol
  2. 31
      solidity/core/test/validator-manager/multisigValidatorManager.test.ts

@ -201,7 +201,7 @@ abstract contract MultisigValidatorManager is Ownable {
* @param _validator The validator to add to the validator set. * @param _validator The validator to add to the validator set.
*/ */
function _enrollValidator(address _validator) internal { function _enrollValidator(address _validator) internal {
require(validators.add(_validator), "enrolled"); require(validators.add(_validator), "already enrolled");
emit EnrollValidator(_validator); emit EnrollValidator(_validator);
} }
@ -211,6 +211,7 @@ abstract contract MultisigValidatorManager is Ownable {
* @param _validator The validator to remove from the validator set. * @param _validator The validator to remove from the validator set.
*/ */
function _unenrollValidator(address _validator) internal { function _unenrollValidator(address _validator) internal {
require(validators.length() > quorumThreshold, "violates quorum threshold");
require(validators.remove(_validator), "!enrolled"); require(validators.remove(_validator), "!enrolled");
emit UnenrollValidator(_validator); emit UnenrollValidator(_validator);
} }

@ -15,7 +15,7 @@ const OUTBOX_DOMAIN_HASH = ethers.utils.keccak256(
); );
const QUORUM_THRESHOLD = 1; const QUORUM_THRESHOLD = 1;
const domainHashTestCases = require('../../../vectors/domainHash.json'); const domainHashTestCases = require('../../../../vectors/domainHash.json');
describe('MultisigValidatorManager', async () => { describe('MultisigValidatorManager', async () => {
let validatorManager: TestMultisigValidatorManager, let validatorManager: TestMultisigValidatorManager,
@ -97,7 +97,7 @@ describe('MultisigValidatorManager', async () => {
it('reverts if the validator is already enrolled', async () => { it('reverts if the validator is already enrolled', async () => {
await expect( await expect(
validatorManager.enrollValidator(validator0.address), validatorManager.enrollValidator(validator0.address),
).to.be.revertedWith('enrolled'); ).to.be.revertedWith('already enrolled');
}); });
it('reverts when called by a non-owner', async () => { it('reverts when called by a non-owner', async () => {
@ -108,21 +108,36 @@ describe('MultisigValidatorManager', async () => {
}); });
describe('#unenrollValidator', () => { describe('#unenrollValidator', () => {
beforeEach(async () => {
// Enroll a second validator
await validatorManager.enrollValidator(validator1.address);
});
it('unenrolls a validator from the validator set', async () => { it('unenrolls a validator from the validator set', async () => {
await validatorManager.unenrollValidator(validator0.address); await validatorManager.unenrollValidator(validator1.address);
expect(await validatorManager.validatorSet()).to.deep.equal([]); expect(await validatorManager.validatorSet()).to.deep.equal([
validator0.address,
]);
}); });
it('emits the UnenrollValidator event', async () => { it('emits the UnenrollValidator event', async () => {
expect(await validatorManager.unenrollValidator(validator0.address)) expect(await validatorManager.unenrollValidator(validator1.address))
.to.emit(validatorManager, 'UnenrollValidator') .to.emit(validatorManager, 'UnenrollValidator')
.withArgs(validator0.address); .withArgs(validator1.address);
});
it('reverts if the resulting validator set size will be less than the quorum threshold', async () => {
await validatorManager.setQuorumThreshold(2);
await expect(
validatorManager.unenrollValidator(validator1.address)
).to.be.revertedWith("violates quorum threshold");
}); });
it('reverts if the validator is not already enrolled', async () => { it('reverts if the validator is not already enrolled', async () => {
await expect( await expect(
validatorManager.unenrollValidator(validator1.address), validatorManager.unenrollValidator(validator2.address),
).to.be.revertedWith('!enrolled'); ).to.be.revertedWith('!enrolled');
}); });
@ -130,7 +145,7 @@ describe('MultisigValidatorManager', async () => {
await expect( await expect(
validatorManager validatorManager
.connect(nonOwner) .connect(nonOwner)
.unenrollValidator(validator0.address), .unenrollValidator(validator1.address),
).to.be.revertedWith('Ownable: caller is not the owner'); ).to.be.revertedWith('Ownable: caller is not the owner');
}); });
}); });

Loading…
Cancel
Save