From 3c12e52ca41e5b857609c0fd6cf16535e92b84e7 Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Mon, 15 Apr 2024 15:10:32 +0200 Subject: [PATCH] Fix bulk register validator (#298) * bulk register: revert when empty public keys list * fix slither ci --- .github/workflows/slither.yml | 4 +- CHANGELOG.md | 7 +- contracts/interfaces/ISSVNetworkCore.sol | 1 + contracts/modules/SSVClusters.sol | 7 +- package.json | 2 +- test/helpers/gas-usage.ts | 2 +- test/validators/register.ts | 276 ++++++++++++++--------- 7 files changed, 186 insertions(+), 113 deletions(-) diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml index 2432e529..64b0d295 100644 --- a/.github/workflows/slither.yml +++ b/.github/workflows/slither.yml @@ -8,9 +8,9 @@ jobs: uses: actions/checkout@v3 - name: Run Slither - uses: crytic/slither-action@v0.3.0 + uses: crytic/slither-action@v0.3.2 id: slither with: node-version: 18 fail-on: high - slither-args: --exclude controlled-delegatecall \ No newline at end of file + slither-args: --exclude controlled-delegatecall,incorrect-return \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index f2b3d1cd..efd428c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### [v1.1.1] 2024-04-12 +- [7b61d4f](https://github.com/bloxapp/ssv-network/commit/7b61d4f) - [Fix] Revert when passing an empty public keys list. + +## [Released] + ### [v1.1.0] 2024-01-08 - [c80dc3b](https://github.com/bloxapp/ssv-network/commit/c80dc3b) - [Feature] Bulk exit of validators. - [6431a00](https://github.com/bloxapp/ssv-network/commit/6431a00) - [Feature] Bulk removal of validators. @@ -15,8 +20,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - [7564dfe](https://github.com/bloxapp/ssv-network/commit/7564dfe) - [Feature] Integration ssv-keys in ssv-network for generating keyshares. - [8647401](https://github.com/bloxapp/ssv-network/commit/8647401) - [Update]: Configuration for publishing npm package. -## [Released] - ### [v1.0.2] 2023-11-08 - [8f5df42](https://github.com/bloxapp/ssv-network/commit/8f5df42633d2b92c6bb70253a41e6afa80b9f111) - Change ValidatorExited signature: owner indexed. diff --git a/contracts/interfaces/ISSVNetworkCore.sol b/contracts/interfaces/ISSVNetworkCore.sol index 70990e72..af8b6b94 100644 --- a/contracts/interfaces/ISSVNetworkCore.sol +++ b/contracts/interfaces/ISSVNetworkCore.sol @@ -89,6 +89,7 @@ interface ISSVNetworkCore { error PublicKeysSharesLengthMismatch(); // 0x9ad467b8 error IncorrectValidatorStateWithData(bytes publicKey); // 0x89307938 error ValidatorAlreadyExistsWithData(bytes publicKey); // 0x388e7999 + error EmptyPublicKeysList(); // df83e679 // legacy errors error ValidatorAlreadyExists(); // 0x8d09a73e diff --git a/contracts/modules/SSVClusters.sol b/contracts/modules/SSVClusters.sol index f4b6fd4f..8d60a5d8 100644 --- a/contracts/modules/SSVClusters.sol +++ b/contracts/modules/SSVClusters.sol @@ -49,13 +49,14 @@ contract SSVClusters is ISSVClusters { uint256 amount, Cluster memory cluster ) external override { - if (publicKeys.length != sharesData.length) revert PublicKeysSharesLengthMismatch(); + uint256 validatorsLength = publicKeys.length; + + if (validatorsLength == 0) revert EmptyPublicKeysList(); + if (validatorsLength != sharesData.length) revert PublicKeysSharesLengthMismatch(); StorageData storage s = SSVStorage.load(); StorageProtocol storage sp = SSVStorageProtocol.load(); - uint256 validatorsLength = publicKeys.length; - ValidatorLib.validateOperatorsLength(operatorIds); for (uint i; i < validatorsLength; ++i) { diff --git a/package.json b/package.json index dbc0163d..a19656ad 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "ssv-network", - "version": "1.0.2", + "version": "1.1.1", "description": "Solidity smart contracts for the SSV Network", "author": "SSV.Network", "repository": { diff --git a/test/helpers/gas-usage.ts b/test/helpers/gas-usage.ts index 95678e9b..75e3e629 100644 --- a/test/helpers/gas-usage.ts +++ b/test/helpers/gas-usage.ts @@ -86,7 +86,7 @@ const MAX_GAS_PER_GROUP: any = { [GasGroup.REDUCE_OPERATOR_FEE]: 51900, [GasGroup.REGISTER_VALIDATOR_EXISTING_CLUSTER]: 202000, - [GasGroup.REGISTER_VALIDATOR_NEW_STATE]: 235500, + [GasGroup.REGISTER_VALIDATOR_NEW_STATE]: 236000, [GasGroup.REGISTER_VALIDATOR_WITHOUT_DEPOSIT]: 180600, [GasGroup.BULK_REGISTER_10_VALIDATOR_NEW_STATE_4]: 889900, diff --git a/test/validators/register.ts b/test/validators/register.ts index 1f8e95dd..6d1bc95c 100644 --- a/test/validators/register.ts +++ b/test/validators/register.ts @@ -185,57 +185,54 @@ describe('Register Validator Tests', () => { it('Bulk register 10 validators with 4 operators into the same cluster', async () => { await helpers.DB.ssvToken.connect(helpers.DB.owners[1]).approve(ssvNetworkContract.address, minDepositAmount); - const { eventsByName } = await trackGas(ssvNetworkContract.connect(helpers.DB.owners[1]).bulkRegisterValidator( - [helpers.DataGenerator.publicKey(11)], - [1, 2, 3, 4], - [helpers.DataGenerator.shares(1,11,4)], - minDepositAmount, - { - validatorCount: 0, - networkFeeIndex: 0, - index: 0, - balance: 0, - active: true - } - )); + const { eventsByName } = await trackGas( + ssvNetworkContract + .connect(helpers.DB.owners[1]) + .bulkRegisterValidator( + [helpers.DataGenerator.publicKey(11)], + [1, 2, 3, 4], + [helpers.DataGenerator.shares(1, 11, 4)], + minDepositAmount, + { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0, + active: true, + }, + ), + ); const args = eventsByName.ValidatorAdded[0].args; - await helpers.bulkRegisterValidators( - 1, - 10, - [1, 2, 3, 4], - minDepositAmount, - args.cluster, - [GasGroup.BULK_REGISTER_10_VALIDATOR_EXISTING_CLUSTER_4], - ); + await helpers.bulkRegisterValidators(1, 10, [1, 2, 3, 4], minDepositAmount, args.cluster, [ + GasGroup.BULK_REGISTER_10_VALIDATOR_EXISTING_CLUSTER_4, + ]); }); it('Bulk register 10 validators with 4 operators new cluster', async () => { - await helpers.bulkRegisterValidators( 1, 10, [1, 2, 3, 4], minDepositAmount, - { validatorCount: 0, networkFeeIndex: 0, index: 0, balance: 0, - active: true - + active: true, }, [GasGroup.BULK_REGISTER_10_VALIDATOR_EXISTING_CLUSTER_4], ); - }); // 7 operators it('Register validator with 7 operators gas limit', async () => { - await helpers.DB.ssvToken.connect(helpers.DB.owners[1]).approve(helpers.DB.ssvNetwork.contract.address, minDepositAmount); + await helpers.DB.ssvToken + .connect(helpers.DB.owners[1]) + .approve(helpers.DB.ssvNetwork.contract.address, minDepositAmount); await trackGas( ssvNetworkContract .connect(helpers.DB.owners[1]) @@ -364,30 +361,29 @@ describe('Register Validator Tests', () => { const operatorIds = [1, 2, 3, 4, 5, 6, 7]; await helpers.DB.ssvToken.connect(helpers.DB.owners[1]).approve(ssvNetworkContract.address, minDepositAmount); - const { eventsByName } = await trackGas(ssvNetworkContract.connect(helpers.DB.owners[1]).bulkRegisterValidator( - [helpers.DataGenerator.publicKey(11)], - operatorIds, - [helpers.DataGenerator.shares(1,11,operatorIds.length)], - minDepositAmount, - { - validatorCount: 0, - networkFeeIndex: 0, - index: 0, - balance: 0, - active: true - } - )); + const { eventsByName } = await trackGas( + ssvNetworkContract + .connect(helpers.DB.owners[1]) + .bulkRegisterValidator( + [helpers.DataGenerator.publicKey(11)], + operatorIds, + [helpers.DataGenerator.shares(1, 11, operatorIds.length)], + minDepositAmount, + { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0, + active: true, + }, + ), + ); const args = eventsByName.ValidatorAdded[0].args; - await helpers.bulkRegisterValidators( - 1, - 10, - operatorIds, - minDepositAmount, - args.cluster, - [GasGroup.BULK_REGISTER_10_VALIDATOR_EXISTING_CLUSTER_7], - ); + await helpers.bulkRegisterValidators(1, 10, operatorIds, minDepositAmount, args.cluster, [ + GasGroup.BULK_REGISTER_10_VALIDATOR_EXISTING_CLUSTER_7, + ]); }); it('Bulk register 10 validators with 7 operators new cluster', async () => { @@ -402,11 +398,10 @@ describe('Register Validator Tests', () => { networkFeeIndex: 0, index: 0, balance: 0, - active: true + active: true, }, [GasGroup.BULK_REGISTER_10_VALIDATOR_NEW_STATE_7], ); - }); // 10 operators @@ -541,31 +536,29 @@ describe('Register Validator Tests', () => { const operatorIds = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]; await helpers.DB.ssvToken.connect(helpers.DB.owners[1]).approve(ssvNetworkContract.address, minDepositAmount); - const { eventsByName } = await trackGas(ssvNetworkContract.connect(helpers.DB.owners[1]).bulkRegisterValidator( - [helpers.DataGenerator.publicKey(11)], - operatorIds, - [helpers.DataGenerator.shares(1,10,operatorIds.length)], - minDepositAmount, - { - validatorCount: 0, - networkFeeIndex: 0, - index: 0, - balance: 0, - active: true - } - )); + const { eventsByName } = await trackGas( + ssvNetworkContract + .connect(helpers.DB.owners[1]) + .bulkRegisterValidator( + [helpers.DataGenerator.publicKey(11)], + operatorIds, + [helpers.DataGenerator.shares(1, 10, operatorIds.length)], + minDepositAmount, + { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0, + active: true, + }, + ), + ); const args = eventsByName.ValidatorAdded[0].args; - - await helpers.bulkRegisterValidators( - 1, - 10, - operatorIds, - minDepositAmount, - args.cluster, - [GasGroup.BULK_REGISTER_10_VALIDATOR_EXISTING_CLUSTER_10], - ); + await helpers.bulkRegisterValidators(1, 10, operatorIds, minDepositAmount, args.cluster, [ + GasGroup.BULK_REGISTER_10_VALIDATOR_EXISTING_CLUSTER_10, + ]); }); it('Bulk register 10 validators with 10 operators new cluster', async () => { @@ -580,12 +573,10 @@ describe('Register Validator Tests', () => { networkFeeIndex: 0, index: 0, balance: 0, - active: true - + active: true, }, [GasGroup.BULK_REGISTER_10_VALIDATOR_NEW_STATE_10], ); - }); // 13 operators @@ -718,31 +709,29 @@ describe('Register Validator Tests', () => { const operatorIds = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]; await helpers.DB.ssvToken.connect(helpers.DB.owners[1]).approve(ssvNetworkContract.address, minDepositAmount); - const { eventsByName } = await trackGas(ssvNetworkContract.connect(helpers.DB.owners[1]).bulkRegisterValidator( - [helpers.DataGenerator.publicKey(11)], - operatorIds, - [helpers.DataGenerator.shares(1, 11, operatorIds.length)], - minDepositAmount, - { - validatorCount: 0, - networkFeeIndex: 0, - index: 0, - balance: 0, - active: true - } - )); + const { eventsByName } = await trackGas( + ssvNetworkContract + .connect(helpers.DB.owners[1]) + .bulkRegisterValidator( + [helpers.DataGenerator.publicKey(11)], + operatorIds, + [helpers.DataGenerator.shares(1, 11, operatorIds.length)], + minDepositAmount, + { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0, + active: true, + }, + ), + ); const args = eventsByName.ValidatorAdded[0].args; - - await helpers.bulkRegisterValidators( - 1, - 10, - operatorIds, - minDepositAmount, - args.cluster, - [GasGroup.BULK_REGISTER_10_VALIDATOR_EXISTING_CLUSTER_13], - ); + await helpers.bulkRegisterValidators(1, 10, operatorIds, minDepositAmount, args.cluster, [ + GasGroup.BULK_REGISTER_10_VALIDATOR_EXISTING_CLUSTER_13, + ]); }); it('Bulk register 10 validators with 13 operators new cluster', async () => { @@ -757,12 +746,10 @@ describe('Register Validator Tests', () => { networkFeeIndex: 0, index: 0, balance: 0, - active: true - + active: true, }, [GasGroup.BULK_REGISTER_10_VALIDATOR_NEW_STATE_13], ); - }); it('Get cluster burn rate', async () => { @@ -1012,8 +999,9 @@ describe('Register Validator Tests', () => { minDepositAmount, helpers.getClusterForValidator(0, 0, 0, 0, true), ), - ).to.be.revertedWithCustomError(ssvNetworkContract, 'ValidatorAlreadyExistsWithData') - .withArgs(helpers.DataGenerator.publicKey(1)); + ) + .to.be.revertedWithCustomError(ssvNetworkContract, 'ValidatorAlreadyExistsWithData') + .withArgs(helpers.DataGenerator.publicKey(1)); }); it('Register an existing validator with different operators setup reverts "ValidatorAlreadyExistsWithData"', async () => { @@ -1030,8 +1018,88 @@ describe('Register Validator Tests', () => { minDepositAmount, helpers.getClusterForValidator(0, 0, 0, 0, true), ), - ).to.be.revertedWithCustomError(ssvNetworkContract, 'ValidatorAlreadyExistsWithData') - .withArgs(helpers.DataGenerator.publicKey(1)); + ) + .to.be.revertedWithCustomError(ssvNetworkContract, 'ValidatorAlreadyExistsWithData') + .withArgs(helpers.DataGenerator.publicKey(1)); + }); + + it('Register validator with an empty public key reverts "InvalidPublicKeyLength"', async () => { + await expect( + ssvNetworkContract.registerValidator( + '0x', + [1, 2, 3, 4], + helpers.DataGenerator.shares(0, 4, 4), + minDepositAmount, + { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0, + active: true, + }, + ), + ).to.be.revertedWithCustomError(ssvNetworkContract, 'InvalidPublicKeyLength'); + }); + + it('Bulk register 10 validators with empty public keys list reverts "EmptyPublicKeysList"', async () => { + await expect( + ssvNetworkContract.connect(helpers.DB.owners[1]).bulkRegisterValidator([], [1, 2, 3, 4], [], minDepositAmount, { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0, + active: true, + }), + ).to.be.revertedWithCustomError(ssvNetworkContract, 'EmptyPublicKeysList'); + }); + + it('Bulk register 10 validators with different pks/shares lenght reverts "PublicKeysSharesLengthMismatch"', async () => { + const pks = Array.from({ length: 10 }, (_, index) => helpers.DataGenerator.publicKey(index + 1)); + const shares = Array.from({ length: 8 }, (_, index) => helpers.DataGenerator.shares(1, index, 4)); + + await expect( + ssvNetworkContract + .connect(helpers.DB.owners[1]) + .bulkRegisterValidator(pks, [1, 2, 3, 4], shares, minDepositAmount, { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0, + active: true, + }), + ).to.be.revertedWithCustomError(ssvNetworkContract, 'PublicKeysSharesLengthMismatch'); + }); + + it('Bulk register 10 validators with wrong operators length reverts "InvalidOperatorIdsLength"', async () => { + const pks = Array.from({ length: 10 }, (_, index) => helpers.DataGenerator.publicKey(index + 1)); + const shares = Array.from({ length: 10 }, (_, index) => helpers.DataGenerator.shares(1, index, 4)); + + await expect( + ssvNetworkContract + .connect(helpers.DB.owners[1]) + .bulkRegisterValidator(pks, [1, 2, 3, 4, 5], shares, minDepositAmount, { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0, + active: true, + }), + ).to.be.revertedWithCustomError(ssvNetworkContract, 'InvalidOperatorIdsLength'); + }); + + it('Bulk register 10 validators with empty operators list reverts "InvalidOperatorIdsLength"', async () => { + const pks = Array.from({ length: 10 }, (_, index) => helpers.DataGenerator.publicKey(index + 1)); + const shares = Array.from({ length: 10 }, (_, index) => helpers.DataGenerator.shares(1, index, 4)); + + await expect( + ssvNetworkContract.connect(helpers.DB.owners[1]).bulkRegisterValidator(pks, [], shares, minDepositAmount, { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0, + active: true, + }), + ).to.be.revertedWithCustomError(ssvNetworkContract, 'InvalidOperatorIdsLength'); }); it('Register whitelisted validator in 1 operator with 4 operators emits "ValidatorAdded"', async () => {