diff --git a/solidity/core/contracts/test/TestMultisigValidatorManager.sol b/solidity/core/contracts/test/TestMultisigValidatorManager.sol new file mode 100644 index 000000000..c7f7f0a9e --- /dev/null +++ b/solidity/core/contracts/test/TestMultisigValidatorManager.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +pragma solidity >=0.6.11; +pragma abicoder v2; + +import {MultisigValidatorManager} from "../validator-manager/MultisigValidatorManager.sol"; + +/** + * This contract exists to test MultisigValidatorManager.sol, which is abstract + * and cannot be deployed directly. + */ +contract TestMultisigValidatorManager is MultisigValidatorManager { + // solhint-disable-next-line no-empty-blocks + constructor( + uint32 _outboxDomain, + address[] memory _validatorSet, + uint256 _quorumThreshold + ) + MultisigValidatorManager(_outboxDomain, _validatorSet, _quorumThreshold) + {} +} diff --git a/solidity/core/contracts/validator-manager/InboxMultisigValidatorManager.sol b/solidity/core/contracts/validator-manager/InboxMultisigValidatorManager.sol index 5fa72104d..f0eecfcad 100644 --- a/solidity/core/contracts/validator-manager/InboxMultisigValidatorManager.sol +++ b/solidity/core/contracts/validator-manager/InboxMultisigValidatorManager.sol @@ -13,7 +13,13 @@ contract InboxMultisigValidatorManager is MultisigValidatorManager { * @param _remoteDomain The remote domain of the outbox chain. */ // solhint-disable-next-line no-empty-blocks - constructor(uint32 _remoteDomain) MultisigValidatorManager(_remoteDomain) {} + constructor( + uint32 _remoteDomain, + address[] memory _validatorSet, + uint256 _quorumThreshold + ) + MultisigValidatorManager(_remoteDomain, _validatorSet, _quorumThreshold) + {} // ============ External Functions ============ diff --git a/solidity/core/contracts/validator-manager/MultisigValidatorManager.sol b/solidity/core/contracts/validator-manager/MultisigValidatorManager.sol index bf17d59cf..6d91e09e8 100644 --- a/solidity/core/contracts/validator-manager/MultisigValidatorManager.sol +++ b/solidity/core/contracts/validator-manager/MultisigValidatorManager.sol @@ -58,9 +58,22 @@ abstract contract MultisigValidatorManager is Ownable { * @param _outboxDomain The domain of the outbox this validator manager * tracks the validator set for. */ - constructor(uint32 _outboxDomain) Ownable() { + constructor( + uint32 _outboxDomain, + address[] memory _validatorSet, + uint256 _quorumThreshold + ) Ownable() { + // Set immutables. outboxDomain = _outboxDomain; outboxDomainHash = keccak256(abi.encodePacked(_outboxDomain, "ABACUS")); + + // Enroll validators. Reverts if there are any duplicates. + uint256 _validatorSetLength = _validatorSet.length; + for (uint256 i = 0; i < _validatorSetLength; i++) { + _enrollValidator(_validatorSet[i]); + } + + _setQuorumThreshold(_quorumThreshold); } // ============ External Functions ============ @@ -71,8 +84,7 @@ abstract contract MultisigValidatorManager is Ownable { * @param _validator The validator to add to the validator set. */ function enrollValidator(address _validator) external onlyOwner { - require(validators.add(_validator), "enrolled"); - emit EnrollValidator(_validator); + _enrollValidator(_validator); } /** @@ -81,8 +93,7 @@ abstract contract MultisigValidatorManager is Ownable { * @param _validator The validator to remove from the validator set. */ function unenrollValidator(address _validator) external onlyOwner { - require(validators.remove(_validator), "!enrolled"); - emit UnenrollValidator(_validator); + _unenrollValidator(_validator); } /** @@ -90,12 +101,21 @@ abstract contract MultisigValidatorManager is Ownable { * @param _quorumThreshold The new quorum threshold. */ function setQuorumThreshold(uint256 _quorumThreshold) external onlyOwner { - require( - _quorumThreshold > 0 && _quorumThreshold <= validators.length(), - "!range" - ); - quorumThreshold = _quorumThreshold; - emit SetQuorumThreshold(_quorumThreshold); + _setQuorumThreshold(_quorumThreshold); + } + + /** + * @notice Gets the addresses of the current validator set. + * @dev There is no ordering guarantee. + * @return The addresses of the validator set. + */ + function validatorSet() external view returns (address[] memory) { + uint256 _length = validators.length(); + address[] memory _validatorSet = new address[](_length); + for (uint256 i = 0; i < _length; i++) { + _validatorSet[i] = validators.at(i); + } + return _validatorSet; } // ============ Public Functions ============ @@ -130,7 +150,7 @@ abstract contract MultisigValidatorManager is Ownable { address _previousSigner = address(0); uint256 _validatorSignatureCount = 0; for (uint256 i = 0; i < _signaturesLength; i++) { - address _signer = recoverCheckpointSigner( + address _signer = _recoverCheckpointSigner( _root, _index, _signatures[i] @@ -155,7 +175,7 @@ abstract contract MultisigValidatorManager is Ownable { * @param _signature Signature on the the checkpoint. * @return The signer of the checkpoint signature. **/ - function recoverCheckpointSigner( + function _recoverCheckpointSigner( bytes32 _root, uint256 _index, bytes calldata _signature @@ -166,4 +186,37 @@ abstract contract MultisigValidatorManager is Ownable { _digest = ECDSA.toEthSignedMessageHash(_digest); return ECDSA.recover(_digest, _signature); } + + /** + * @notice Enrolls a validator into the validator set. + * @dev Reverts if _validator is already in the validator set. + * @param _validator The validator to add to the validator set. + */ + function _enrollValidator(address _validator) internal { + require(validators.add(_validator), "enrolled"); + emit EnrollValidator(_validator); + } + + /** + * @notice Unenrolls a validator from the validator set. + * @dev Reverts if _validator is not in the validator set. + * @param _validator The validator to remove from the validator set. + */ + function _unenrollValidator(address _validator) internal { + require(validators.remove(_validator), "!enrolled"); + emit UnenrollValidator(_validator); + } + + /** + * @notice Sets the quorum threshold. + * @param _quorumThreshold The new quorum threshold. + */ + function _setQuorumThreshold(uint256 _quorumThreshold) internal { + require( + _quorumThreshold > 0 && _quorumThreshold <= validators.length(), + "!range" + ); + quorumThreshold = _quorumThreshold; + emit SetQuorumThreshold(_quorumThreshold); + } } diff --git a/solidity/core/contracts/validator-manager/OutboxMultisigValidatorManager.sol b/solidity/core/contracts/validator-manager/OutboxMultisigValidatorManager.sol index d7bba02a6..9f3437b05 100644 --- a/solidity/core/contracts/validator-manager/OutboxMultisigValidatorManager.sol +++ b/solidity/core/contracts/validator-manager/OutboxMultisigValidatorManager.sol @@ -28,7 +28,11 @@ contract OutboxMultisigValidatorManager is MultisigValidatorManager { * @param _localDomain The local domain. */ // solhint-disable-next-line no-empty-blocks - constructor(uint32 _localDomain) MultisigValidatorManager(_localDomain) {} + constructor( + uint32 _localDomain, + address[] memory _validatorSet, + uint256 _quorumThreshold + ) MultisigValidatorManager(_localDomain, _validatorSet, _quorumThreshold) {} // ============ External Functions ============ diff --git a/solidity/core/test/inbox.test.ts b/solidity/core/test/inbox.test.ts index 462208eb3..5266c3c4e 100644 --- a/solidity/core/test/inbox.test.ts +++ b/solidity/core/test/inbox.test.ts @@ -21,7 +21,7 @@ const proveAndProcessTestCases = require('../../../vectors/proveAndProcess.json' const localDomain = 2000; const remoteDomain = 1000; -describe.only('Inbox', async () => { +describe('Inbox', async () => { const badRecipientFactories = [ BadRecipient1__factory, BadRecipient3__factory, @@ -39,7 +39,9 @@ describe.only('Inbox', async () => { // Inbox.initialize will ensure the validator manager is a contract. // TestValidatorManager doesn't have any special logic, it just submits // checkpoints without any signature verification. - const testValidatorManagerFactory = new TestValidatorManager__factory(signer); + const testValidatorManagerFactory = new TestValidatorManager__factory( + signer, + ); validatorManager = await testValidatorManagerFactory.deploy(); }); diff --git a/solidity/core/test/validator-manager/multisigValidatorManager.test.ts b/solidity/core/test/validator-manager/multisigValidatorManager.test.ts new file mode 100644 index 000000000..1019428e7 --- /dev/null +++ b/solidity/core/test/validator-manager/multisigValidatorManager.test.ts @@ -0,0 +1,265 @@ +import { ethers } from 'hardhat'; +import { expect } from 'chai'; +import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; +import { types } from '@abacus-network/utils'; + +import { + TestMultisigValidatorManager, + TestMultisigValidatorManager__factory, +} from '../../types'; +import { Validator } from '../lib/core'; + +const OUTBOX_DOMAIN = 1234; +const OUTBOX_DOMAIN_HASH = ethers.utils.keccak256( + ethers.utils.solidityPack(['uint32', 'string'], [OUTBOX_DOMAIN, 'ABACUS']), +); +const QUORUM_THRESHOLD = 1; + +// const VALIDATOR = '0xdeadbeef00000000000000000000000000000000'; + +// Signs a checkpoint with the provided validators, +// sorts +async function getCheckpointSignatures( + root: types.HexString, + index: number, + unsortedValidators: Validator[], +): Promise { + const validators = unsortedValidators.sort((a, b) => { + const aAddress = a.address.toLowerCase(); + const bAddress = b.address.toLowerCase(); + + if (aAddress < bAddress) { + return -1; + } else if (aAddress > bAddress) { + return 1; + } else { + return 0; + } + }); + + const signedCheckpoints = await Promise.all( + validators.map((validator) => validator.signCheckpoint(root, index)), + ); + return signedCheckpoints.map( + (signedCheckpoint) => signedCheckpoint.signature, + ); +} + +describe.only('MultisigValidatorManager', async () => { + let validatorManager: TestMultisigValidatorManager, + signer: SignerWithAddress, + nonOwner: SignerWithAddress, + validator0: Validator, + validator1: Validator, + validator2: Validator, + validator3: Validator; + + before(async () => { + const signers = await ethers.getSigners(); + [signer, nonOwner] = signers; + const [ + , + , + validatorSigner0, + validatorSigner1, + validatorSigner2, + validatorSigner3, + ] = signers; + validator0 = await Validator.fromSigner(validatorSigner0, OUTBOX_DOMAIN); + validator1 = await Validator.fromSigner(validatorSigner1, OUTBOX_DOMAIN); + validator2 = await Validator.fromSigner(validatorSigner2, OUTBOX_DOMAIN); + validator3 = await Validator.fromSigner(validatorSigner3, OUTBOX_DOMAIN); + }); + + beforeEach(async () => { + const validatorManagerFactory = new TestMultisigValidatorManager__factory( + signer, + ); + validatorManager = await validatorManagerFactory.deploy( + OUTBOX_DOMAIN, + [validator0.address], + QUORUM_THRESHOLD, + ); + }); + + describe('#constructor', () => { + it('sets the outboxDomain', async () => { + expect(await validatorManager.outboxDomain()).to.equal(OUTBOX_DOMAIN); + }); + + it('sets the outboxDomainHash', async () => { + expect(await validatorManager.outboxDomainHash()).to.equal( + OUTBOX_DOMAIN_HASH, + ); + }); + + it('enrolls the validator set', async () => { + expect(await validatorManager.validatorSet()).to.deep.equal([ + validator0.address, + ]); + }); + + it('sets the quorum threshold', async () => { + expect(await validatorManager.quorumThreshold()).to.equal([ + QUORUM_THRESHOLD, + ]); + }); + }); + + describe('#enrollValidator', () => { + it('enrolls a validator into the validator set', async () => { + await validatorManager.enrollValidator(validator1.address); + + expect(await validatorManager.validatorSet()).to.deep.equal([ + validator0.address, + validator1.address, + ]); + }); + + it('emits the EnrollValidator event', async () => { + expect(await validatorManager.enrollValidator(validator1.address)) + .to.emit(validatorManager, 'EnrollValidator') + .withArgs(validator1.address); + }); + + it('reverts if the validator is already enrolled', async () => { + await expect( + validatorManager.enrollValidator(validator0.address), + ).to.be.revertedWith('enrolled'); + }); + + it('reverts when called by a non-owner', async () => { + await expect( + validatorManager.connect(nonOwner).enrollValidator(validator1.address), + ).to.be.revertedWith('Ownable: caller is not the owner'); + }); + }); + + describe('#unenrollValidator', () => { + it('unenrolls a validator from the validator set', async () => { + await validatorManager.unenrollValidator(validator0.address); + + expect(await validatorManager.validatorSet()).to.deep.equal([]); + }); + + it('emits the UnenrollValidator event', async () => { + expect(await validatorManager.unenrollValidator(validator0.address)) + .to.emit(validatorManager, 'UnenrollValidator') + .withArgs(validator0.address); + }); + + it('reverts if the validator is not already enrolled', async () => { + await expect( + validatorManager.unenrollValidator(validator1.address), + ).to.be.revertedWith('!enrolled'); + }); + + it('reverts when called by a non-owner', async () => { + await expect( + validatorManager + .connect(nonOwner) + .unenrollValidator(validator0.address), + ).to.be.revertedWith('Ownable: caller is not the owner'); + }); + }); + + describe('#setQuorumThreshold', () => { + beforeEach(async () => { + // Have 2 validators to allow us to have more than 1 valid + // quorum threshold + await validatorManager.enrollValidator(validator1.address); + }); + + it('sets the quorum threshold', async () => { + await validatorManager.setQuorumThreshold(2); + + expect(await validatorManager.quorumThreshold()).to.equal(2); + }); + + it('emits the SetQuorumThreshold event', async () => { + expect(await validatorManager.setQuorumThreshold(2)) + .to.emit(validatorManager, 'SetQuorumThreshold') + .withArgs(2); + }); + + it('reverts if the new quorum threshold is zero', async () => { + await expect(validatorManager.setQuorumThreshold(0)).to.be.revertedWith( + '!range', + ); + }); + + it('reverts if the new quorum threshold is > the validator set size', async () => { + await expect(validatorManager.setQuorumThreshold(3)).to.be.revertedWith( + '!range', + ); + }); + + it('reverts when called by a non-owner', async () => { + await expect( + validatorManager.connect(nonOwner).setQuorumThreshold(2), + ).to.be.revertedWith('Ownable: caller is not the owner'); + }); + }); + + describe('#isQuorum', () => { + const root = ethers.utils.formatBytes32String('test root'); + const index = 1; + + beforeEach(async () => { + // Have 3 validators and a quorum of 2 + await validatorManager.enrollValidator(validator1.address); + await validatorManager.enrollValidator(validator2.address); + + await validatorManager.setQuorumThreshold(2); + }); + + it('returns true when there is a quorum', async () => { + const signatures = await getCheckpointSignatures(root, index, [ + validator0, + validator1, + ]); + expect(await validatorManager.isQuorum(root, index, signatures)).to.be + .true; + }); + + it('returns true when a quorum exists even if provided with non-validator signatures', async () => { + const signatures = await getCheckpointSignatures( + root, + index, + [validator0, validator1, validator3], // validator 3 is not enrolled + ); + expect(await validatorManager.isQuorum(root, index, signatures)).to.be + .true; + }); + + it('returns false when the signature count is < the quorum threshold', async () => { + const signatures = await getCheckpointSignatures(root, index, [ + validator0, + ]); + expect(await validatorManager.isQuorum(root, index, signatures)).to.be + .false; + }); + + it('returns false when some signatures are not from enrolled validators', async () => { + const signatures = await getCheckpointSignatures( + root, + index, + [validator0, validator3], // validator 3 is not enrolled + ); + expect(await validatorManager.isQuorum(root, index, signatures)).to.be + .false; + }); + + it('reverts when signatures are not ordered by their signer', async () => { + // Reverse the signature order, purposely messing up the + // ascending sort + const signatures = ( + await getCheckpointSignatures(root, index, [validator0, validator1]) + ).reverse(); + + await expect( + validatorManager.isQuorum(root, index, signatures), + ).to.be.revertedWith('!sorted signers'); + }); + }); +});