From c9921f61fa9c9aba84e8076388b297edcb7f1e03 Mon Sep 17 00:00:00 2001 From: Lior Rutenberg Date: Wed, 12 Jul 2023 14:47:02 +0300 Subject: [PATCH 1/8] mainnet deployment config (#247) --- .openzeppelin/mainnet.json | 108 ++++++++++++++++++++++++++++++++ contracts/libraries/CoreLib.sol | 2 +- 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/.openzeppelin/mainnet.json b/.openzeppelin/mainnet.json index 4b712498..42b4216e 100644 --- a/.openzeppelin/mainnet.json +++ b/.openzeppelin/mainnet.json @@ -494,6 +494,114 @@ } } } + }, + "400b4079d3549b56702f7dfadb688c5395e64dd3211c072e6f584ae7ae02f79f": { + "address": "0xE2d1Cf93CD4D5E0EEEF1b33ca51Bb82c829A1b75", + "txHash": "0x5c1c55b3073cc2579fd426616636484a188f1205a4278dc37063bc891c397494", + "layout": { + "solcVersion": "0.8.18", + "storage": [ + { + "label": "_initialized", + "offset": 0, + "slot": "0", + "type": "t_uint8", + "contract": "Initializable", + "src": "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol:62", + "retypedFrom": "bool" + }, + { + "label": "_initializing", + "offset": 1, + "slot": "0", + "type": "t_bool", + "contract": "Initializable", + "src": "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol:67" + }, + { + "label": "__gap", + "offset": 0, + "slot": "1", + "type": "t_array(t_uint256)50_storage", + "contract": "ERC1967UpgradeUpgradeable", + "src": "@openzeppelin/contracts-upgradeable/proxy/ERC1967/ERC1967UpgradeUpgradeable.sol:197" + }, + { + "label": "__gap", + "offset": 0, + "slot": "51", + "type": "t_array(t_uint256)50_storage", + "contract": "UUPSUpgradeable", + "src": "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol:107" + }, + { + "label": "__gap", + "offset": 0, + "slot": "101", + "type": "t_array(t_uint256)50_storage", + "contract": "ContextUpgradeable", + "src": "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol:36" + }, + { + "label": "_owner", + "offset": 0, + "slot": "151", + "type": "t_address", + "contract": "OwnableUpgradeable", + "src": "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol:22" + }, + { + "label": "__gap", + "offset": 0, + "slot": "152", + "type": "t_array(t_uint256)49_storage", + "contract": "OwnableUpgradeable", + "src": "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol:94" + }, + { + "label": "_pendingOwner", + "offset": 0, + "slot": "201", + "type": "t_address", + "contract": "Ownable2StepUpgradeable", + "src": "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol:27" + }, + { + "label": "__gap", + "offset": 0, + "slot": "202", + "type": "t_array(t_uint256)49_storage", + "contract": "Ownable2StepUpgradeable", + "src": "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol:70" + } + ], + "types": { + "t_address": { + "label": "address", + "numberOfBytes": "20" + }, + "t_array(t_uint256)49_storage": { + "label": "uint256[49]", + "numberOfBytes": "1568" + }, + "t_array(t_uint256)50_storage": { + "label": "uint256[50]", + "numberOfBytes": "1600" + }, + "t_bool": { + "label": "bool", + "numberOfBytes": "1" + }, + "t_uint256": { + "label": "uint256", + "numberOfBytes": "32" + }, + "t_uint8": { + "label": "uint8", + "numberOfBytes": "1" + } + } + } } } } diff --git a/contracts/libraries/CoreLib.sol b/contracts/libraries/CoreLib.sol index 4439946f..adfcd0f2 100644 --- a/contracts/libraries/CoreLib.sol +++ b/contracts/libraries/CoreLib.sol @@ -7,7 +7,7 @@ library CoreLib { event ModuleUpgraded(SSVModules moduleId, address moduleAddress); function getVersion() internal pure returns (string memory) { - return "v1.0.0"; + return "v1.0.0.rc2"; } function transferBalance(address to, uint256 amount) internal { From 6ae5903a5c99c8d75b59fc0d35574d87f82e5861 Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Mon, 17 Jul 2023 10:31:19 +0200 Subject: [PATCH 2/8] Operators audit recommendations (#245) * Operators audit recommendations * explicit type definition --- contracts/SSVNetwork.sol | 7 ++----- contracts/interfaces/ISSVOperators.sol | 4 ++-- contracts/libraries/OperatorLib.sol | 2 +- contracts/modules/SSVOperators.sol | 21 +++++++++---------- contracts/test/SSVNetworkUpgrade.sol | 2 +- contracts/test/modules/SSVOperatorsUpdate.sol | 4 ++-- test/account/withdraw.ts | 8 +++---- test/helpers/gas-usage.ts | 2 +- test/operators/update-fee.ts | 4 ++-- 9 files changed, 25 insertions(+), 29 deletions(-) diff --git a/contracts/SSVNetwork.sol b/contracts/SSVNetwork.sol index a59d3f8e..a726f287 100644 --- a/contracts/SSVNetwork.sol +++ b/contracts/SSVNetwork.sol @@ -12,8 +12,6 @@ import "./libraries/Types.sol"; import "./libraries/CoreLib.sol"; import "./libraries/SSVStorage.sol"; import "./libraries/SSVStorageProtocol.sol"; -import "./libraries/OperatorLib.sol"; -import "./libraries/ClusterLib.sol"; import "./libraries/RegisterAuth.sol"; import "./SSVProxy.sol"; @@ -34,7 +32,6 @@ contract SSVNetwork is SSVProxy { using Types256 for uint256; - using ClusterLib for Cluster; /****************/ /* Initializers */ @@ -150,7 +147,7 @@ contract SSVNetwork is _delegate(SSVStorage.load().ssvContracts[SSVModules.SSV_OPERATORS]); } - function withdrawOperatorEarnings(uint64 operatorId) external override { + function withdrawAllOperatorEarnings(uint64 operatorId) external override { _delegate(SSVStorage.load().ssvContracts[SSVModules.SSV_OPERATORS]); } @@ -159,7 +156,7 @@ contract SSVNetwork is /*******************************/ function setFeeRecipientAddress(address recipientAddress) external override { - emit FeeRecipientAddressUpdated(msg.sender, recipientAddress); + _delegate(SSVStorage.load().ssvContracts[SSVModules.SSV_OPERATORS]); } /*******************************/ diff --git a/contracts/interfaces/ISSVOperators.sol b/contracts/interfaces/ISSVOperators.sol index b68d79d9..280d1237 100644 --- a/contracts/interfaces/ISSVOperators.sol +++ b/contracts/interfaces/ISSVOperators.sol @@ -47,7 +47,7 @@ interface ISSVOperators is ISSVNetworkCore { /// @notice Withdraws all operator earnings /// @param operatorId The ID of the operator - function withdrawOperatorEarnings(uint64 operatorId) external; + function withdrawAllOperatorEarnings(uint64 operatorId) external; /** * @dev Emitted when a new operator has been added. @@ -72,7 +72,7 @@ interface ISSVOperators is ISSVNetworkCore { event OperatorWhitelistUpdated(uint64 indexed operatorId, address whitelisted); event OperatorFeeDeclared(address indexed owner, uint64 indexed operatorId, uint256 blockNumber, uint256 fee); - event OperatorFeeCancellationDeclared(address indexed owner, uint64 indexed operatorId); + event OperatorFeeDeclarationCancelled(address indexed owner, uint64 indexed operatorId); /** * @dev Emitted when an operator's fee is updated. * @param owner Operator's owner. diff --git a/contracts/libraries/OperatorLib.sol b/contracts/libraries/OperatorLib.sol index 55fadfb6..ef3a6e01 100644 --- a/contracts/libraries/OperatorLib.sol +++ b/contracts/libraries/OperatorLib.sol @@ -36,7 +36,7 @@ library OperatorLib { uint32 deltaValidatorCount, StorageData storage s ) internal returns (uint64 clusterIndex, uint64 burnRate) { - for (uint i; i < operatorIds.length; ) { + for (uint256 i; i < operatorIds.length; ) { uint64 operatorId = operatorIds[i]; ISSVNetworkCore.Operator storage operator = s.operators[operatorId]; if (operator.snapshot.block != 0) { diff --git a/contracts/modules/SSVOperators.sol b/contracts/modules/SSVOperators.sol index 875484d5..05290dc8 100644 --- a/contracts/modules/SSVOperators.sol +++ b/contracts/modules/SSVOperators.sol @@ -72,9 +72,8 @@ contract SSVOperators is ISSVOperators { } function setOperatorWhitelist(uint64 operatorId, address whitelisted) external { - SSVStorage.load().operators[operatorId].checkOwner(); - StorageData storage s = SSVStorage.load(); + s.operators[operatorId].checkOwner(); if (whitelisted == address(0)) { s.operators[operatorId].whitelisted = false; @@ -87,9 +86,9 @@ contract SSVOperators is ISSVOperators { } function declareOperatorFee(uint64 operatorId, uint256 fee) external override { - SSVStorage.load().operators[operatorId].checkOwner(); - StorageData storage s = SSVStorage.load(); + s.operators[operatorId].checkOwner(); + StorageProtocol storage sp = SSVStorageProtocol.load(); if (fee != 0 && fee < MINIMAL_OPERATOR_FEE) revert FeeTooLow(); @@ -103,8 +102,7 @@ contract SSVOperators is ISSVOperators { } // @dev 100% = 10000, 10% = 1000 - using 10000 to represent 2 digit precision - uint64 maxAllowedFee = (operatorFee * (PRECISION_FACTOR + sp.operatorMaxFeeIncrease)) / - PRECISION_FACTOR; + uint64 maxAllowedFee = (operatorFee * (PRECISION_FACTOR + sp.operatorMaxFeeIncrease)) / PRECISION_FACTOR; if (shrunkFee > maxAllowedFee) revert FeeExceedsIncreaseLimit(); @@ -141,13 +139,14 @@ contract SSVOperators is ISSVOperators { } function cancelDeclaredOperatorFee(uint64 operatorId) external override { - SSVStorage.load().operators[operatorId].checkOwner(); + StorageData storage s = SSVStorage.load(); + s.operators[operatorId].checkOwner(); - if (SSVStorage.load().operatorFeeChangeRequests[operatorId].approvalBeginTime == 0) revert NoFeeDeclared(); + if (s.operatorFeeChangeRequests[operatorId].approvalBeginTime == 0) revert NoFeeDeclared(); - delete SSVStorage.load().operatorFeeChangeRequests[operatorId]; + delete s.operatorFeeChangeRequests[operatorId]; - emit OperatorFeeCancellationDeclared(msg.sender, operatorId); + emit OperatorFeeDeclarationCancelled(msg.sender, operatorId); } function reduceOperatorFee(uint64 operatorId, uint256 fee) external override { @@ -175,7 +174,7 @@ contract SSVOperators is ISSVOperators { _withdrawOperatorEarnings(operatorId, amount); } - function withdrawOperatorEarnings(uint64 operatorId) external override { + function withdrawAllOperatorEarnings(uint64 operatorId) external override { _withdrawOperatorEarnings(operatorId, 0); } diff --git a/contracts/test/SSVNetworkUpgrade.sol b/contracts/test/SSVNetworkUpgrade.sol index 52351028..acbbd995 100644 --- a/contracts/test/SSVNetworkUpgrade.sol +++ b/contracts/test/SSVNetworkUpgrade.sol @@ -181,7 +181,7 @@ contract SSVNetworkUpgrade is ); } - function withdrawOperatorEarnings(uint64 operatorId) external override { + function withdrawAllOperatorEarnings(uint64 operatorId) external override { _delegateCall( SSVStorage.load().ssvContracts[SSVModules.SSV_OPERATORS], abi.encodeWithSignature("withdrawOperatorEarnings(uint64)", operatorId) diff --git a/contracts/test/modules/SSVOperatorsUpdate.sol b/contracts/test/modules/SSVOperatorsUpdate.sol index 7ec4a3e0..99d93e0b 100644 --- a/contracts/test/modules/SSVOperatorsUpdate.sol +++ b/contracts/test/modules/SSVOperatorsUpdate.sol @@ -121,7 +121,7 @@ contract SSVOperatorsUpdate is ISSVOperators { delete SSVStorage.load().operatorFeeChangeRequests[operatorId]; - emit OperatorFeeCancellationDeclared(msg.sender, operatorId); + emit OperatorFeeDeclarationCancelled(msg.sender, operatorId); } function reduceOperatorFee(uint64 operatorId, uint256 fee) external override { @@ -149,7 +149,7 @@ contract SSVOperatorsUpdate is ISSVOperators { _withdrawOperatorEarnings(operatorId, amount); } - function withdrawOperatorEarnings(uint64 operatorId) external override { + function withdrawAllOperatorEarnings(uint64 operatorId) external override { _withdrawOperatorEarnings(operatorId, 0); } diff --git a/test/account/withdraw.ts b/test/account/withdraw.ts index 0e5af46a..6b4d5419 100644 --- a/test/account/withdraw.ts +++ b/test/account/withdraw.ts @@ -57,11 +57,11 @@ describe('Withdraw Tests', () => { }); it('Withdraw the total operator balance emits "OperatorWithdrawn"', async () => { - await expect(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawOperatorEarnings(uint64)'](1)).to.emit(ssvNetworkContract, 'OperatorWithdrawn'); + await expect(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawAllOperatorEarnings(uint64)'](1)).to.emit(ssvNetworkContract, 'OperatorWithdrawn'); }); it('Withdraw the total operator balance gas limits', async () => { - await trackGas(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawOperatorEarnings(uint64)'](1), [GasGroup.WITHDRAW_OPERATOR_BALANCE]); + await trackGas(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawAllOperatorEarnings(uint64)'](1), [GasGroup.WITHDRAW_OPERATOR_BALANCE]); }); it('Withdraw from a cluster that has a removed operator emits "ClusterWithdrawn"', async () => { @@ -98,11 +98,11 @@ describe('Withdraw Tests', () => { }); it('Withdraw the total balance from an operator I do not own reverts "CallerNotOwner"', async () => { - await expect(ssvNetworkContract.connect(helpers.DB.owners[2])['withdrawOperatorEarnings(uint64)'](12)).to.be.revertedWithCustomError(ssvNetworkContract, 'CallerNotOwner'); + await expect(ssvNetworkContract.connect(helpers.DB.owners[2])['withdrawAllOperatorEarnings(uint64)'](12)).to.be.revertedWithCustomError(ssvNetworkContract, 'CallerNotOwner'); }); it('Withdraw more than the operator total balance reverts "InsufficientBalance"', async () => { - await expect(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawOperatorEarnings(uint64)'](12)).to.be.revertedWithCustomError(ssvNetworkContract, 'InsufficientBalance'); + await expect(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawAllOperatorEarnings(uint64)'](12)).to.be.revertedWithCustomError(ssvNetworkContract, 'InsufficientBalance'); }); it('Withdraw from a cluster without validators', async () => { diff --git a/test/helpers/gas-usage.ts b/test/helpers/gas-usage.ts index 82da52b6..d9d67ebf 100644 --- a/test/helpers/gas-usage.ts +++ b/test/helpers/gas-usage.ts @@ -55,7 +55,7 @@ const MAX_GAS_PER_GROUP: any = { [GasGroup.REMOVE_OPERATOR_WITH_WITHDRAW]: 70200, [GasGroup.SET_OPERATOR_WHITELIST]: 84300, - [GasGroup.DECLARE_OPERATOR_FEE]: 70500, + [GasGroup.DECLARE_OPERATOR_FEE]: 70000, [GasGroup.CANCEL_OPERATOR_FEE]: 41900, [GasGroup.EXECUTE_OPERATOR_FEE]: 49900, [GasGroup.REDUCE_OPERATOR_FEE]: 51900, diff --git a/test/operators/update-fee.ts b/test/operators/update-fee.ts index 3ea86616..93b0cde1 100644 --- a/test/operators/update-fee.ts +++ b/test/operators/update-fee.ts @@ -40,10 +40,10 @@ describe('Operator Fee Tests', () => { await trackGas(ssvNetworkContract.connect(helpers.DB.owners[2]).declareOperatorFee(1, initialFee + initialFee / 10), [GasGroup.REGISTER_OPERATOR]); }); - it('Cancel declared fee emits "OperatorFeeCancellationDeclared"', async () => { + it('Cancel declared fee emits "OperatorFeeDeclarationCancelled"', async () => { await ssvNetworkContract.connect(helpers.DB.owners[2]).declareOperatorFee(1, initialFee + initialFee / 10); await expect(ssvNetworkContract.connect(helpers.DB.owners[2]).cancelDeclaredOperatorFee(1 - )).to.emit(ssvNetworkContract, 'OperatorFeeCancellationDeclared'); + )).to.emit(ssvNetworkContract, 'OperatorFeeDeclarationCancelled'); }); it('Cancel declared fee gas limits', async () => { From fe3b9b178344dd723b19792d01ab5010dfd2dcf9 Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Mon, 17 Jul 2023 10:34:05 +0200 Subject: [PATCH 3/8] audit recommendations (#246) --- contracts/SSVNetwork.sol | 4 +-- contracts/modules/SSVClusters.sol | 46 +++++++++++++++++-------------- test/helpers/gas-usage.ts | 4 +-- 3 files changed, 29 insertions(+), 25 deletions(-) diff --git a/contracts/SSVNetwork.sol b/contracts/SSVNetwork.sol index a726f287..866ce9cd 100644 --- a/contracts/SSVNetwork.sol +++ b/contracts/SSVNetwork.sol @@ -183,7 +183,7 @@ contract SSVNetwork is _delegate(SSVStorage.load().ssvContracts[SSVModules.SSV_CLUSTERS]); } - function liquidate(address owner, uint64[] calldata operatorIds, ISSVNetworkCore.Cluster memory cluster) external { + function liquidate(address clusterOwner, uint64[] calldata operatorIds, ISSVNetworkCore.Cluster memory cluster) external { _delegate(SSVStorage.load().ssvContracts[SSVModules.SSV_CLUSTERS]); } @@ -196,7 +196,7 @@ contract SSVNetwork is } function deposit( - address owner, + address clusterOwner, uint64[] calldata operatorIds, uint256 amount, ISSVNetworkCore.Cluster memory cluster diff --git a/contracts/modules/SSVClusters.sol b/contracts/modules/SSVClusters.sol index fb27f180..8d1df9aa 100644 --- a/contracts/modules/SSVClusters.sol +++ b/contracts/modules/SSVClusters.sol @@ -2,7 +2,6 @@ pragma solidity 0.8.18; import "../interfaces/ISSVClusters.sol"; -import "../libraries/Types.sol"; import "../libraries/ClusterLib.sol"; import "../libraries/OperatorLib.sol"; import "../libraries/ProtocolLib.sol"; @@ -29,7 +28,7 @@ contract SSVClusters is ISSVClusters { StorageData storage s = SSVStorage.load(); StorageProtocol storage sp = SSVStorageProtocol.load(); - uint operatorsLength = operatorIds.length; + uint256 operatorsLength = operatorIds.length; { if ( operatorsLength < MIN_OPERATORS_LENGTH || @@ -77,7 +76,7 @@ contract SSVClusters is ISSVClusters { if (cluster.active) { uint64 clusterIndex; - for (uint i; i < operatorsLength; ) { + for (uint256 i; i < operatorsLength; ) { uint64 operatorId = operatorIds[i]; { if (i + 1 < operatorsLength) { @@ -93,12 +92,11 @@ contract SSVClusters is ISSVClusters { if (operator.snapshot.block == 0) { revert OperatorDoesNotExist(); } - if ( - operator.whitelisted && - s.operatorsWhitelist[operatorId] != address(0) && - s.operatorsWhitelist[operatorId] != msg.sender - ) { - revert CallerNotWhitelisted(); + if (operator.whitelisted) { + address whitelisted = s.operatorsWhitelist[operatorId]; + if (whitelisted != address(0) && whitelisted != msg.sender) { + revert CallerNotWhitelisted(); + } } operator.updateSnapshot(); if (++operator.validatorCount > sp.validatorsPerOperatorLimit) { @@ -151,13 +149,14 @@ contract SSVClusters is ISSVClusters { bytes32 mask = ~bytes32(uint256(1)); // All bits set to 1 except LSB bytes32 validatorData = s.validatorPKs[hashedValidator]; - + if (validatorData == bytes32(0)) { revert ValidatorDoesNotExist(); } bytes32 hashedOperatorIds = keccak256(abi.encodePacked(operatorIds)) & mask; // Clear LSB of provided operator ids - if ((validatorData & mask) != hashedOperatorIds) { // Clear LSB of stored validator data and compare + if ((validatorData & mask) != hashedOperatorIds) { + // Clear LSB of stored validator data and compare revert IncorrectValidatorState(); } @@ -183,10 +182,10 @@ contract SSVClusters is ISSVClusters { emit ValidatorRemoved(msg.sender, operatorIds, publicKey, cluster); } - function liquidate(address owner, uint64[] memory operatorIds, Cluster memory cluster) external override { + function liquidate(address clusterOwner, uint64[] memory operatorIds, Cluster memory cluster) external override { StorageData storage s = SSVStorage.load(); - bytes32 hashedCluster = cluster.validateHashedCluster(owner, operatorIds, s); + bytes32 hashedCluster = cluster.validateHashedCluster(clusterOwner, operatorIds, s); cluster.validateClusterIsNotLiquidated(); StorageProtocol storage sp = SSVStorageProtocol.load(); @@ -203,7 +202,7 @@ contract SSVClusters is ISSVClusters { uint256 balanceLiquidatable; if ( - owner != msg.sender && + clusterOwner != msg.sender && !cluster.isLiquidatable( burnRate, sp.networkFee, @@ -230,7 +229,7 @@ contract SSVClusters is ISSVClusters { CoreLib.transferBalance(msg.sender, balanceLiquidatable); } - emit ClusterLiquidated(owner, operatorIds, cluster); + emit ClusterLiquidated(clusterOwner, operatorIds, cluster); } function reactivate(uint64[] calldata operatorIds, uint256 amount, Cluster memory cluster) external override { @@ -241,7 +240,12 @@ contract SSVClusters is ISSVClusters { StorageProtocol storage sp = SSVStorageProtocol.load(); - (uint64 clusterIndex, uint64 burnRate) = OperatorLib.updateOperators(operatorIds, true, cluster.validatorCount, s); + (uint64 clusterIndex, uint64 burnRate) = OperatorLib.updateOperators( + operatorIds, + true, + cluster.validatorCount, + s + ); cluster.balance += amount; cluster.active = true; @@ -271,14 +275,14 @@ contract SSVClusters is ISSVClusters { } function deposit( - address owner, + address clusterOwner, uint64[] calldata operatorIds, uint256 amount, Cluster memory cluster ) external override { StorageData storage s = SSVStorage.load(); - bytes32 hashedCluster = cluster.validateHashedCluster(owner, operatorIds, s); + bytes32 hashedCluster = cluster.validateHashedCluster(clusterOwner, operatorIds, s); cluster.balance += amount; @@ -286,7 +290,7 @@ contract SSVClusters is ISSVClusters { CoreLib.deposit(amount); - emit ClusterDeposited(owner, operatorIds, amount, cluster); + emit ClusterDeposited(clusterOwner, operatorIds, amount, cluster); } function withdraw(uint64[] calldata operatorIds, uint256 amount, Cluster memory cluster) external override { @@ -301,8 +305,8 @@ contract SSVClusters is ISSVClusters { if (cluster.active) { uint64 clusterIndex; { - uint operatorsLength = operatorIds.length; - for (uint i; i < operatorsLength; ) { + uint256 operatorsLength = operatorIds.length; + for (uint256 i; i < operatorsLength; ) { Operator storage operator = SSVStorage.load().operators[operatorIds[i]]; clusterIndex += operator.snapshot.index + diff --git a/test/helpers/gas-usage.ts b/test/helpers/gas-usage.ts index d9d67ebf..b5ba133e 100644 --- a/test/helpers/gas-usage.ts +++ b/test/helpers/gas-usage.ts @@ -61,7 +61,7 @@ const MAX_GAS_PER_GROUP: any = { [GasGroup.REDUCE_OPERATOR_FEE]: 51900, [GasGroup.REGISTER_VALIDATOR_EXISTING_CLUSTER]: 202000, - [GasGroup.REGISTER_VALIDATOR_NEW_STATE]: 221600, + [GasGroup.REGISTER_VALIDATOR_NEW_STATE]: 221400, [GasGroup.REGISTER_VALIDATOR_WITHOUT_DEPOSIT]: 181600, [GasGroup.REGISTER_VALIDATOR_EXISTING_CLUSTER_7]: 272900, @@ -78,7 +78,7 @@ const MAX_GAS_PER_GROUP: any = { [GasGroup.REMOVE_VALIDATOR]: 113500, [GasGroup.DEPOSIT]: 77500, - [GasGroup.WITHDRAW_CLUSTER_BALANCE]: 96800, + [GasGroup.WITHDRAW_CLUSTER_BALANCE]: 94500, [GasGroup.WITHDRAW_OPERATOR_BALANCE]: 64900, [GasGroup.LIQUIDATE_CLUSTER_4]: 129200, [GasGroup.LIQUIDATE_CLUSTER_7]: 170400, From 994287d5f5753f277d8ee3b0ca7fc112c3729be8 Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Mon, 17 Jul 2023 10:38:31 +0200 Subject: [PATCH 4/8] Views contract audit recommendations (#248) --- contracts/SSVNetworkViews.sol | 24 ++++++++-------- contracts/interfaces/ISSVViews.sol | 17 +++++------ contracts/modules/SSVViews.sol | 46 +++++++++++++----------------- contracts/test/SSVViewsT.sol | 23 ++++++--------- test/operators/update-fee.ts | 21 +++++++------- test/validators/register.ts | 2 +- 6 files changed, 61 insertions(+), 72 deletions(-) diff --git a/contracts/SSVNetworkViews.sol b/contracts/SSVNetworkViews.sol index 1e635b3f..393348a7 100644 --- a/contracts/SSVNetworkViews.sol +++ b/contracts/SSVNetworkViews.sol @@ -20,7 +20,7 @@ contract SSVNetworkViews is UUPSUpgradeable, Ownable2StepUpgradeable, ISSVViews // @dev reserve storage space for future new state variables in base contract // slither-disable-next-line shadowing-state - uint256[50] private__gap; + uint256[50] private __gap; function _authorizeUpgrade(address) internal override onlyOwner {} @@ -39,8 +39,8 @@ contract SSVNetworkViews is UUPSUpgradeable, Ownable2StepUpgradeable, ISSVViews /* Validator External View Functions */ /*************************************/ - function getValidator(address owner, bytes calldata publicKey) external view override returns (bool active) { - return ssvNetwork.getValidator(owner, publicKey); + function getValidator(address clusterOwner, bytes calldata publicKey) external view override returns (bool active) { + return ssvNetwork.getValidator(clusterOwner, publicKey); } /************************************/ @@ -51,7 +51,7 @@ contract SSVNetworkViews is UUPSUpgradeable, Ownable2StepUpgradeable, ISSVViews return ssvNetwork.getOperatorFee(operatorId); } - function getOperatorDeclaredFee(uint64 operatorId) external view override returns (uint256, uint64, uint64) { + function getOperatorDeclaredFee(uint64 operatorId) external view override returns (bool, uint256, uint64, uint64) { return ssvNetwork.getOperatorDeclaredFee(operatorId); } @@ -64,27 +64,27 @@ contract SSVNetworkViews is UUPSUpgradeable, Ownable2StepUpgradeable, ISSVViews /***********************************/ function isLiquidatable( - address owner, + address clusterOwner, uint64[] calldata operatorIds, Cluster memory cluster ) external view override returns (bool) { - return ssvNetwork.isLiquidatable(owner, operatorIds, cluster); + return ssvNetwork.isLiquidatable(clusterOwner, operatorIds, cluster); } function isLiquidated( - address owner, + address clusterOwner, uint64[] calldata operatorIds, Cluster memory cluster ) external view override returns (bool) { - return ssvNetwork.isLiquidated(owner, operatorIds, cluster); + return ssvNetwork.isLiquidated(clusterOwner, operatorIds, cluster); } function getBurnRate( - address owner, + address clusterOwner, uint64[] calldata operatorIds, Cluster memory cluster ) external view returns (uint256) { - return ssvNetwork.getBurnRate(owner, operatorIds, cluster); + return ssvNetwork.getBurnRate(clusterOwner, operatorIds, cluster); } /***********************************/ @@ -96,11 +96,11 @@ contract SSVNetworkViews is UUPSUpgradeable, Ownable2StepUpgradeable, ISSVViews } function getBalance( - address owner, + address clusterOwner, uint64[] calldata operatorIds, Cluster memory cluster ) external view override returns (uint256) { - return ssvNetwork.getBalance(owner, operatorIds, cluster); + return ssvNetwork.getBalance(clusterOwner, operatorIds, cluster); } /*******************************/ diff --git a/contracts/interfaces/ISSVViews.sol b/contracts/interfaces/ISSVViews.sol index 4bcad056..39cff14b 100644 --- a/contracts/interfaces/ISSVViews.sol +++ b/contracts/interfaces/ISSVViews.sol @@ -7,20 +7,21 @@ interface ISSVViews is ISSVNetworkCore { /// @notice Gets the validator status /// @param owner The address of the validator's owner /// @param publicKey The public key of the validator - /// @return A boolean indicating if the validator is active - function getValidator(address owner, bytes calldata publicKey) external view returns (bool); + /// @return active A boolean indicating if the validator is active. If it does not exist, returns false. + function getValidator(address owner, bytes calldata publicKey) external view returns (bool active); /// @notice Gets the operator fee /// @param operatorId The ID of the operator - /// @return The fee associated with the operator (SSV) - function getOperatorFee(uint64 operatorId) external view returns (uint256); - + /// @return fee The fee associated with the operator (SSV). If the operator does not exist, the returned value is 0. + function getOperatorFee(uint64 operatorId) external view returns (uint256 fee); + /// @notice Gets the declared operator fee /// @param operatorId The ID of the operator + /// @return isFeeDeclared A boolean indicating if the fee is declared /// @return fee The declared operator fee (SSV) /// @return approvalBeginTime The time when the fee approval process begins /// @return approvalEndTime The time when the fee approval process ends - function getOperatorDeclaredFee(uint64 operatorId) external view returns (uint256 fee, uint64 approvalBeginTime, uint64 approvalEndTime); + function getOperatorDeclaredFee(uint64 operatorId) external view returns (bool isFeeDeclared, uint256 fee, uint64 approvalBeginTime, uint64 approvalEndTime); /// @notice Gets operator details by ID /// @param operatorId The ID of the operator @@ -52,8 +53,8 @@ interface ISSVViews is ISSVNetworkCore { /// @notice Gets operator earnings /// @param operatorId The ID of the operator - /// @return The earnings associated with the operator (SSV) - function getOperatorEarnings(uint64 operatorId) external view returns (uint256); + /// @return earnings The earnings associated with the operator (SSV) + function getOperatorEarnings(uint64 operatorId) external view returns (uint256 earnings); /// @notice Gets the balance of the cluster /// @param owner The owner address of the cluster diff --git a/contracts/modules/SSVViews.sol b/contracts/modules/SSVViews.sol index 7a8e4322..a9acf885 100644 --- a/contracts/modules/SSVViews.sol +++ b/contracts/modules/SSVViews.sol @@ -21,10 +21,10 @@ contract SSVViews is ISSVViews { /* Validator External View Functions */ /*************************************/ - function getValidator(address owner, bytes calldata publicKey) external view override returns (bool active) { - bytes32 validatorData = SSVStorage.load().validatorPKs[keccak256(abi.encodePacked(publicKey, owner))]; + function getValidator(address clusterOwner, bytes calldata publicKey) external view override returns (bool active) { + bytes32 validatorData = SSVStorage.load().validatorPKs[keccak256(abi.encodePacked(publicKey, clusterOwner))]; - if (validatorData == bytes32(0)) revert ValidatorDoesNotExist(); + if (validatorData == bytes32(0)) return false; bytes32 activeFlag = validatorData & bytes32(uint256(1)); // Retrieve LSB of stored value return activeFlag == bytes32(uint256(1)); @@ -36,20 +36,14 @@ contract SSVViews is ISSVViews { /************************************/ function getOperatorFee(uint64 operatorId) external view override returns (uint256 fee) { - Operator memory operator = SSVStorage.load().operators[operatorId]; - if (operator.snapshot.block == 0) revert OperatorDoesNotExist(); - - fee = operator.fee.expand(); + return SSVStorage.load().operators[operatorId].fee.expand(); } - function getOperatorDeclaredFee(uint64 operatorId) external view override returns (uint256, uint64, uint64) { + function getOperatorDeclaredFee(uint64 operatorId) external view override returns (bool, uint256, uint64, uint64) { OperatorFeeChangeRequest memory opFeeChangeRequest = SSVStorage.load().operatorFeeChangeRequests[operatorId]; - if (opFeeChangeRequest.fee == 0) { - revert NoFeeDeclared(); - } - return ( + opFeeChangeRequest.approvalBeginTime != 0, opFeeChangeRequest.fee.expand(), opFeeChangeRequest.approvalBeginTime, opFeeChangeRequest.approvalEndTime @@ -70,11 +64,11 @@ contract SSVViews is ISSVViews { /***********************************/ function isLiquidatable( - address owner, + address clusterOwner, uint64[] calldata operatorIds, Cluster memory cluster ) external view override returns (bool) { - cluster.validateHashedCluster(owner, operatorIds, SSVStorage.load()); + cluster.validateHashedCluster(clusterOwner, operatorIds, SSVStorage.load()); if (!cluster.active) { return false; @@ -82,8 +76,8 @@ contract SSVViews is ISSVViews { uint64 clusterIndex; uint64 burnRate; - uint operatorsLength = operatorIds.length; - for (uint i; i < operatorsLength; ++i) { + uint256 operatorsLength = operatorIds.length; + for (uint256 i; i < operatorsLength; ++i) { Operator memory operator = SSVStorage.load().operators[operatorIds[i]]; clusterIndex += operator.snapshot.index + (uint64(block.number) - operator.snapshot.block) * operator.fee; burnRate += operator.fee; @@ -102,24 +96,24 @@ contract SSVViews is ISSVViews { } function isLiquidated( - address owner, + address clusterOwner, uint64[] calldata operatorIds, Cluster memory cluster ) external view override returns (bool) { - cluster.validateHashedCluster(owner, operatorIds, SSVStorage.load()); + cluster.validateHashedCluster(clusterOwner, operatorIds, SSVStorage.load()); return !cluster.active; } function getBurnRate( - address owner, + address clusterOwner, uint64[] calldata operatorIds, Cluster memory cluster ) external view returns (uint256) { - cluster.validateHashedCluster(owner, operatorIds, SSVStorage.load()); + cluster.validateHashedCluster(clusterOwner, operatorIds, SSVStorage.load()); uint64 aggregateFee; - uint operatorsLength = operatorIds.length; - for (uint i; i < operatorsLength; ++i) { + uint256 operatorsLength = operatorIds.length; + for (uint256 i; i < operatorsLength; ++i) { Operator memory operator = SSVStorage.load().operators[operatorIds[i]]; if (operator.owner != address(0)) { aggregateFee += operator.fee; @@ -142,17 +136,17 @@ contract SSVViews is ISSVViews { } function getBalance( - address owner, + address clusterOwner, uint64[] calldata operatorIds, Cluster memory cluster ) external view override returns (uint256) { - cluster.validateHashedCluster(owner, operatorIds, SSVStorage.load()); + cluster.validateHashedCluster(clusterOwner, operatorIds, SSVStorage.load()); cluster.validateClusterIsNotLiquidated(); uint64 clusterIndex; { - uint operatorsLength = operatorIds.length; - for (uint i; i < operatorsLength; ++i) { + uint256 operatorsLength = operatorIds.length; + for (uint256 i; i < operatorsLength; ++i) { Operator memory operator = SSVStorage.load().operators[operatorIds[i]]; clusterIndex += operator.snapshot.index + diff --git a/contracts/test/SSVViewsT.sol b/contracts/test/SSVViewsT.sol index 4b1f0b99..57c3a297 100644 --- a/contracts/test/SSVViewsT.sol +++ b/contracts/test/SSVViewsT.sol @@ -39,16 +39,11 @@ contract SSVViewsT is ISSVViews { fee = operator.fee.expand(); } - function getOperatorDeclaredFee(uint64 operatorId) external view override returns (uint256, uint64, uint64) { - OperatorFeeChangeRequest memory opFeeChangeRequest = SSVStorageUpgrade.load().operatorFeeChangeRequests[ - operatorId - ]; - - if (opFeeChangeRequest.fee == 0) { - revert NoFeeDeclared(); - } + function getOperatorDeclaredFee(uint64 operatorId) external view override returns (bool feeDeclared, uint256, uint64, uint64) { + OperatorFeeChangeRequest memory opFeeChangeRequest = SSVStorage.load().operatorFeeChangeRequests[operatorId]; return ( + opFeeChangeRequest.approvalBeginTime != 0, opFeeChangeRequest.fee.expand(), opFeeChangeRequest.approvalBeginTime, opFeeChangeRequest.approvalEndTime @@ -81,8 +76,8 @@ contract SSVViewsT is ISSVViews { uint64 clusterIndex; uint64 burnRate; - uint operatorsLength = operatorIds.length; - for (uint i; i < operatorsLength; ++i) { + uint256 operatorsLength = operatorIds.length; + for (uint256 i; i < operatorsLength; ++i) { Operator memory operator = SSVStorage.load().operators[operatorIds[i]]; clusterIndex += operator.snapshot.index + (uint64(block.number) - operator.snapshot.block) * operator.fee; burnRate += operator.fee; @@ -117,8 +112,8 @@ contract SSVViewsT is ISSVViews { cluster.validateHashedCluster(owner, operatorIds, SSVStorage.load()); uint64 aggregateFee; - uint operatorsLength = operatorIds.length; - for (uint i; i < operatorsLength; ++i) { + uint256 operatorsLength = operatorIds.length; + for (uint256 i; i < operatorsLength; ++i) { Operator memory operator = SSVStorageUpgrade.load().operators[operatorIds[i]]; if (operator.owner != address(0)) { aggregateFee += operator.fee; @@ -150,8 +145,8 @@ contract SSVViewsT is ISSVViews { uint64 clusterIndex; { - uint operatorsLength = operatorIds.length; - for (uint i; i < operatorsLength; ++i) { + uint256 operatorsLength = operatorIds.length; + for (uint256 i; i < operatorsLength; ++i) { Operator memory operator = SSVStorage.load().operators[operatorIds[i]]; clusterIndex += operator.snapshot.index + diff --git a/test/operators/update-fee.ts b/test/operators/update-fee.ts index 93b0cde1..b0d2de27 100644 --- a/test/operators/update-fee.ts +++ b/test/operators/update-fee.ts @@ -24,7 +24,7 @@ describe('Operator Fee Tests', () => { it('Declare fee gas limits"', async () => { await trackGas(ssvNetworkContract.connect(helpers.DB.owners[2]).declareOperatorFee(1, initialFee + initialFee / 10 - ),[GasGroup.DECLARE_OPERATOR_FEE]); + ), [GasGroup.DECLARE_OPERATOR_FEE]); }); it('Declare fee with zero value emits "OperatorFeeDeclared"', async () => { @@ -68,9 +68,8 @@ describe('Operator Fee Tests', () => { expect(await ssvViews.getOperatorFee(1)).to.equal(initialFee); }); - it('Get fee from operator that does not exist reverts "OperatorDoesNotExist"', async () => { - await expect(ssvViews.getOperatorFee(12 - )).to.be.revertedWithCustomError(ssvNetworkContract, 'OperatorDoesNotExist'); + it('Get fee from operator that does not exist returns 0', async () => { + expect(await ssvViews.getOperatorFee(12)).to.equal(0); }); it('Declare fee of operator I do not own reverts "CallerNotOwner"', async () => { @@ -170,8 +169,8 @@ describe('Operator Fee Tests', () => { expect(await ssvNetworkContract.connect(helpers.DB.owners[2]).reduceOperatorFee(1, initialFee / 2)).to.emit(ssvNetworkContract, 'OperatorFeeExecuted'); expect(await ssvViews.getOperatorFee(1)).to.equal(initialFee / 2); - await expect(ssvViews.getOperatorDeclaredFee(1 - )).to.be.revertedWithCustomError(ssvNetworkContract, 'NoFeeDeclared'); + const [isFeeDeclared] = await ssvViews.getOperatorDeclaredFee(1); + expect(isFeeDeclared).to.equal(false); }); //Dao @@ -192,7 +191,7 @@ describe('Operator Fee Tests', () => { it('DAO update the declare fee period gas limits"', async () => { await trackGas(ssvNetworkContract.updateDeclareOperatorFeePeriod(1200 - ), [GasGroup.OPERATOR_DECLARE_FEE_LIMIT]); + ), [GasGroup.OPERATOR_DECLARE_FEE_LIMIT]); }); it('DAO update the execute fee period emits "ExecuteOperatorFeePeriodUpdated"', async () => { @@ -202,7 +201,7 @@ describe('Operator Fee Tests', () => { it('DAO update the execute fee period gas limits', async () => { await trackGas(ssvNetworkContract.updateExecuteOperatorFeePeriod(1200 - ), [GasGroup.OPERATOR_EXECUTE_FEE_LIMIT]); + ), [GasGroup.OPERATOR_EXECUTE_FEE_LIMIT]); }); it('DAO get fee increase limit', async () => { @@ -212,7 +211,7 @@ describe('Operator Fee Tests', () => { it('DAO get declared fee', async () => { const newFee = initialFee + initialFee / 10; await trackGas(ssvNetworkContract.connect(helpers.DB.owners[2]).declareOperatorFee(1, newFee), [GasGroup.REGISTER_OPERATOR]); - const [feeDeclaredInContract] = await ssvViews.getOperatorDeclaredFee(1); + const [_, feeDeclaredInContract] = await ssvViews.getOperatorDeclaredFee(1); expect(feeDeclaredInContract).to.equal(newFee); }); @@ -237,7 +236,7 @@ describe('Operator Fee Tests', () => { it('DAO declared fee without a pending request reverts "NoFeeDeclared"', async () => { await trackGas(ssvNetworkContract.connect(helpers.DB.owners[2]).declareOperatorFee(1, initialFee + initialFee / 10), [GasGroup.REGISTER_OPERATOR]); - await expect(ssvViews.getOperatorDeclaredFee(2 - )).to.be.revertedWithCustomError(ssvNetworkContract, 'NoFeeDeclared'); + const [isFeeDeclared] = await ssvViews.getOperatorDeclaredFee(2); + expect(isFeeDeclared).to.equal(false); }); }); \ No newline at end of file diff --git a/test/validators/register.ts b/test/validators/register.ts index b7bec57b..83a151e8 100644 --- a/test/validators/register.ts +++ b/test/validators/register.ts @@ -843,6 +843,6 @@ describe('Register Validator Tests', () => { }); it('Retrieve a non-existing validator', async () => { - await expect(ssvViews.getValidator(helpers.DB.owners[2].address, helpers.DataGenerator.publicKey(90))).to.be.revertedWithCustomError(ssvNetworkContract, 'ValidatorDoesNotExist'); + expect(await ssvViews.getValidator(helpers.DB.owners[2].address, helpers.DataGenerator.publicKey(90))).to.equal(false); }); }); From 6a01a4013645814f3ad2572671001da059458373 Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Mon, 17 Jul 2023 10:40:05 +0200 Subject: [PATCH 5/8] DAO audit recommendations (#249) * audit recommendations * SSV11 Inconsistent Use of shrink() and expand() * SSV-11 add unit test --- contracts/modules/SSVDAO.sol | 13 +++++-------- contracts/test/libraries/CoreLibT.sol | 2 +- test/dao/network-fee-change.ts | 8 ++++++++ test/deployment/version.ts | 2 +- test/helpers/contract-helpers.ts | 2 +- 5 files changed, 16 insertions(+), 11 deletions(-) diff --git a/contracts/modules/SSVDAO.sol b/contracts/modules/SSVDAO.sol index 8612c6b6..8c5128ee 100644 --- a/contracts/modules/SSVDAO.sol +++ b/contracts/modules/SSVDAO.sol @@ -3,25 +3,22 @@ pragma solidity 0.8.18; import "../interfaces/ISSVDAO.sol"; import "../libraries/Types.sol"; -import "../libraries/OperatorLib.sol"; import "../libraries/ProtocolLib.sol"; -import "../libraries/CoreLib.sol"; -import "../libraries/SSVStorage.sol"; contract SSVDAO is ISSVDAO { using Types64 for uint64; using Types256 for uint256; using ProtocolLib for StorageProtocol; - + uint64 private constant MINIMAL_LIQUIDATION_THRESHOLD = 100_800; function updateNetworkFee(uint256 fee) external override { - uint64 previousFee = SSVStorageProtocol.load().networkFee; - - SSVStorageProtocol.load().updateNetworkFee(fee); + StorageProtocol storage sp = SSVStorageProtocol.load(); + uint64 previousFee = sp.networkFee; - emit NetworkFeeUpdated(previousFee, fee); + sp.updateNetworkFee(fee); + emit NetworkFeeUpdated(previousFee.expand(), fee); } function withdrawNetworkEarnings(uint256 amount) external override { diff --git a/contracts/test/libraries/CoreLibT.sol b/contracts/test/libraries/CoreLibT.sol index 0681eac8..929841e3 100644 --- a/contracts/test/libraries/CoreLibT.sol +++ b/contracts/test/libraries/CoreLibT.sol @@ -6,7 +6,7 @@ import "../../libraries/SSVStorage.sol"; library CoreLibT { function getVersion() internal pure returns (string memory) { - return "v1.0.0"; + return "v1.0.0.rc2"; } function transfer(address to, uint256 amount) internal { diff --git a/test/dao/network-fee-change.ts b/test/dao/network-fee-change.ts index 41ea8df0..ae67b6a0 100644 --- a/test/dao/network-fee-change.ts +++ b/test/dao/network-fee-change.ts @@ -22,6 +22,14 @@ describe('Network Fee Tests', () => { )).to.emit(ssvNetworkContract, 'NetworkFeeUpdated').withArgs(0, networkFee); }); + it('Change network fee when it was set emits "NetworkFeeUpdated"', async () => { + const initialNetworkFee = helpers.CONFIG.minimalOperatorFee; + await ssvNetworkContract.updateNetworkFee(initialNetworkFee); + + await expect(ssvNetworkContract.updateNetworkFee(networkFee + )).to.emit(ssvNetworkContract, 'NetworkFeeUpdated').withArgs(initialNetworkFee, networkFee); + }); + it('Change network fee gas limit', async () => { await trackGas(ssvNetworkContract.updateNetworkFee(networkFee), [GasGroup.NETWORK_FEE_CHANGE]); }); diff --git a/test/deployment/version.ts b/test/deployment/version.ts index 439dc7e8..b55c817a 100644 --- a/test/deployment/version.ts +++ b/test/deployment/version.ts @@ -20,7 +20,7 @@ describe('Version upgrade tests', () => { await ssvNetworkContract.updateModule(helpers.SSV_MODULES.SSV_VIEWS, viewsContract.address) - expect(await ssvNetworkViews.getVersion()).to.equal("v1.0.0"); + expect(await ssvNetworkViews.getVersion()).to.equal("v1.0.0.rc2"); }); }); \ No newline at end of file diff --git a/test/helpers/contract-helpers.ts b/test/helpers/contract-helpers.ts index 5c4aee6d..236e50e7 100644 --- a/test/helpers/contract-helpers.ts +++ b/test/helpers/contract-helpers.ts @@ -65,7 +65,7 @@ export const DataGenerator = { export const initializeContract = async () => { CONFIG = { - initialVersion: "v1.0.0", + initialVersion: "v1.0.0.rc2", operatorMaxFeeIncrease: 1000, declareOperatorFeePeriod: 3600, // HOUR executeOperatorFeePeriod: 86400, // DAY From 1e61c35736578d4b03bacbff9da2128ad12a5620 Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Mon, 17 Jul 2023 10:47:12 +0200 Subject: [PATCH 6/8] SSVNetwork audit recommendations (#250) * audit recommendations * _disableInitializers() --- contracts/SSVNetwork.sol | 9 +++++++++ contracts/interfaces/ISSVNetwork.sol | 2 ++ contracts/libraries/ClusterLib.sol | 1 - contracts/libraries/CoreLib.sol | 2 +- contracts/libraries/ProtocolLib.sol | 2 +- contracts/libraries/RegisterAuth.sol | 6 +----- contracts/libraries/SSVStorage.sol | 3 +-- contracts/libraries/SSVStorageProtocol.sol | 4 +--- test/deployment/deploy.ts | 2 +- 9 files changed, 17 insertions(+), 14 deletions(-) diff --git a/contracts/SSVNetwork.sol b/contracts/SSVNetwork.sol index 866ce9cd..f5d0c116 100644 --- a/contracts/SSVNetwork.sol +++ b/contracts/SSVNetwork.sol @@ -95,6 +95,11 @@ contract SSVNetwork is sp.operatorMaxFeeIncrease = operatorMaxFeeIncrease_; } + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + /*****************/ /* UUPS required */ /*****************/ @@ -240,6 +245,10 @@ contract SSVNetwork is _delegate(SSVStorage.load().ssvContracts[SSVModules.SSV_DAO]); } + function getVersion() external pure override returns (string memory version) { + return CoreLib.getVersion(); + } + /*******************************/ /* Upgrade Modules Function */ /*******************************/ diff --git a/contracts/interfaces/ISSVNetwork.sol b/contracts/interfaces/ISSVNetwork.sol index fc592ae8..273d476e 100644 --- a/contracts/interfaces/ISSVNetwork.sol +++ b/contracts/interfaces/ISSVNetwork.sol @@ -26,6 +26,8 @@ interface ISSVNetwork { uint64 operatorMaxFeeIncrease_ ) external; + function getVersion() external pure returns (string memory version); + function updateModule(SSVModules moduleId, address moduleAddress) external; function setRegisterAuth(address userAddress, bool authOperators, bool authValidators) external; diff --git a/contracts/libraries/ClusterLib.sol b/contracts/libraries/ClusterLib.sol index 75d8b2f6..ea0bb36f 100644 --- a/contracts/libraries/ClusterLib.sol +++ b/contracts/libraries/ClusterLib.sol @@ -3,7 +3,6 @@ pragma solidity 0.8.18; import "../interfaces/ISSVNetworkCore.sol"; import "./SSVStorage.sol"; -import "./SSVStorageProtocol.sol"; import "./Types.sol"; library ClusterLib { diff --git a/contracts/libraries/CoreLib.sol b/contracts/libraries/CoreLib.sol index adfcd0f2..6597a3b0 100644 --- a/contracts/libraries/CoreLib.sol +++ b/contracts/libraries/CoreLib.sol @@ -4,7 +4,7 @@ pragma solidity 0.8.18; import "./SSVStorage.sol"; library CoreLib { - event ModuleUpgraded(SSVModules moduleId, address moduleAddress); + event ModuleUpgraded(SSVModules indexed moduleId, address moduleAddress); function getVersion() internal pure returns (string memory) { return "v1.0.0.rc2"; diff --git a/contracts/libraries/ProtocolLib.sol b/contracts/libraries/ProtocolLib.sol index af8c18c7..bd34a404 100644 --- a/contracts/libraries/ProtocolLib.sol +++ b/contracts/libraries/ProtocolLib.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.18; import "../interfaces/ISSVNetworkCore.sol"; -import "../SSVNetwork.sol"; +import "./Types.sol"; import "./SSVStorageProtocol.sol"; library ProtocolLib { diff --git a/contracts/libraries/RegisterAuth.sol b/contracts/libraries/RegisterAuth.sol index cecad65c..351721b3 100644 --- a/contracts/libraries/RegisterAuth.sol +++ b/contracts/libraries/RegisterAuth.sol @@ -1,17 +1,13 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity 0.8.18; -import "../interfaces/ISSVNetworkCore.sol"; -import "./Types.sol"; -import "@openzeppelin/contracts/utils/Counters.sol"; - struct Authorization { bool registerOperator; bool registerValidator; } library RegisterAuth { - uint256 constant SSV_STORAGE_POSITION = uint256(keccak256("ssv.network.storage.auth")) - 1; + uint256 constant private SSV_STORAGE_POSITION = uint256(keccak256("ssv.network.storage.auth")) - 1; struct AuthData { mapping(address => Authorization) authorization; diff --git a/contracts/libraries/SSVStorage.sol b/contracts/libraries/SSVStorage.sol index 964e86b9..1ddd79aa 100644 --- a/contracts/libraries/SSVStorage.sol +++ b/contracts/libraries/SSVStorage.sol @@ -2,7 +2,6 @@ pragma solidity 0.8.18; import "../interfaces/ISSVNetworkCore.sol"; -import "./Types.sol"; import "@openzeppelin/contracts/utils/Counters.sol"; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; @@ -37,7 +36,7 @@ struct StorageData { } library SSVStorage { - uint256 constant SSV_STORAGE_POSITION = uint256(keccak256("ssv.network.storage.main")) - 1; + uint256 constant private SSV_STORAGE_POSITION = uint256(keccak256("ssv.network.storage.main")) - 1; function load() internal pure returns (StorageData storage sd) { uint256 position = SSV_STORAGE_POSITION; diff --git a/contracts/libraries/SSVStorageProtocol.sol b/contracts/libraries/SSVStorageProtocol.sol index 1f7d9276..81c2da7e 100644 --- a/contracts/libraries/SSVStorageProtocol.sol +++ b/contracts/libraries/SSVStorageProtocol.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity 0.8.18; -import "../interfaces/ISSVNetworkCore.sol"; - /// @title SSV Network Storage Protocol /// @notice Represents the operational settings and parameters required by the SSV Network struct StorageProtocol { @@ -33,7 +31,7 @@ struct StorageProtocol { } library SSVStorageProtocol { - uint256 constant SSV_STORAGE_POSITION = uint256(keccak256("ssv.network.storage.protocol")) - 1; + uint256 constant private SSV_STORAGE_POSITION = uint256(keccak256("ssv.network.storage.protocol")) - 1; function load() internal pure returns (StorageProtocol storage sd) { uint256 position = SSV_STORAGE_POSITION; diff --git a/test/deployment/deploy.ts b/test/deployment/deploy.ts index 863b259c..9e44de5e 100644 --- a/test/deployment/deploy.ts +++ b/test/deployment/deploy.ts @@ -74,7 +74,7 @@ describe('Deployment tests', () => { 2000000, 2000000, 2000000, - 2000)).to.be.revertedWith('Function must be called through delegatecall'); + 2000)).to.be.revertedWith('Initializable: contract is already initialized'); }); it('Upgrade SSVNetwork contract. Check state is only changed from proxy contract', async () => { From 4084fd3bac61bb33af922c945f3c50f63d23d3cb Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Mon, 17 Jul 2023 10:48:49 +0200 Subject: [PATCH 7/8] Audit recommendations - Update docs (#251) * SSV-13 Privileged Roles * SSV24 Missing Access Controls * sycn with main --- README.md | 3 ++- docs/architecture.md | 4 +++- docs/local-dev.md | 2 +- docs/roles.md | 41 +++++++++++++++++++++++++++++++++++++++++ docs/setup.md | 2 +- docs/tasks.md | 4 ++-- 6 files changed, 50 insertions(+), 6 deletions(-) create mode 100644 docs/roles.md diff --git a/README.md b/README.md index 30019d98..5a9e8ecb 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # SSV Network Smart Contracts -### Intro | [Architecture](./docs/architecture.md) | [Setup](./docs/setup.md) | [Tasks](./docs/tasks.md) | [Local development](./docs/local-dev.md) +### Intro | [Architecture](./docs/architecture.md) | [Setup](./docs/setup.md) | [Tasks](./docs/tasks.md) | [Local development](./docs/local-dev.md) | [Roles](./docs/roles.md) This repository contains the Solidity smart contracts for the SSV Network. The SSV Network is a decentralized network for the operation of Ethereum validators. It allows for secure, scalable, and decentralized staking on the Ethereum blockchain. The key elements of this system are represented through several Ethereum smart contracts, all of which are outlined below. @@ -10,6 +10,7 @@ The documentation is divided into different sections: - **Setup** The basic setup of the repository to be able to compile the contracts, run tests, etc. - **Tasks** Detailed instructions to run useful tools, deploy, and upgrade the contracts. - **Local development** Guide to setup the local environment to work with the contracts. +- **Roles** Detailed information about the privileged roles in the system. ## SSV Documentation diff --git a/docs/architecture.md b/docs/architecture.md index a909ad3d..5874a158 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1,6 +1,6 @@ # SSV Network -### [Intro](../README.md) | Architecture | [Setup](setup.md) | [Tasks](tasks.md) | [Local development](local-dev.md) +### [Intro](../README.md) | Architecture | [Setup](setup.md) | [Tasks](tasks.md) | [Local development](local-dev.md) | [Roles](roles.md) ## Contract Architecture @@ -25,6 +25,8 @@ It's the main contract for reading information about the network and its partici #### Modules Non-upgradeable, stateless contracts that contain the logic to support Clusters, Operators, and Protocol (DAO / Network) functionalities. +**Important**: Interacting directly with module contracts is not effective as you are not interacting with the correct state maintained by the main contract `SSVNetwork`. All interactions should be done via main contracts: `SSVNetwork` or `SSVNetworkViews`. + #### Libraries Libraries are a fundamental part of the architecture to support reusable pieces efficiently. Also, `SSVStorage` and `SSVStorageProtocol` implement the Diamond storage pattern. diff --git a/docs/local-dev.md b/docs/local-dev.md index 7b1cc674..94f529a3 100644 --- a/docs/local-dev.md +++ b/docs/local-dev.md @@ -1,6 +1,6 @@ # SSV Network -### [Intro](../README.md) | [Architecture](architecture.md) | [Setup](setup.md) | [Tasks](tasks.md) | Local development +### [Intro](../README.md) | [Architecture](architecture.md) | [Setup](setup.md) | [Tasks](tasks.md) | Local development | [Roles](roles.md) ## Running against a local node / testnet You can deploy and run these contracts in a local node like Hardhat's, Ganache, or public testnets. This guide will cover the process. diff --git a/docs/roles.md b/docs/roles.md new file mode 100644 index 00000000..f40417fb --- /dev/null +++ b/docs/roles.md @@ -0,0 +1,41 @@ +# SSV Network + +### [Intro](../README.md) | [Architecture](architecture.md) | [Setup](setup.md) | [Tasks](tasks.md) | [Local development](local-dev.md) | Roles + + +## Contract owner +The contract owner can perform operational actions over the contract and protocol updates. + + +### Contract operations +* Upgrade `SSVNetwork` and `SSVNetworkViews` +* `SSVNetwork.upgradeModule()` - Update any module + +### Protocol updates +* `SSVNetwork.updateNetworkFee()` - Updates the network fee +* `SSVNetwork.withdrawNetworkEarnings()` - Withdraws network earnings +* `SSVNetwork.updateOperatorFeeIncreaseLimit()` - Updates the limit on the percentage increase in operator fees +* `SSVNetwork.updateDeclareOperatorFeePeriod()` - Updates the period for declaring operator fees +* `SSVNetwork.updateExecuteOperatorFeePeriod()` - Updates the period for executing operator fees +* `SSVNetwork.updateLiquidationThresholdPeriod()` - Updates the liquidation threshold period +* `SSVNetwork.updateMinimumLiquidationCollateral()` - Updates the minimum collateral required to prevent liquidation + +## Operator owner +Only the owner of an operator can execute these functions: + +* `SSVNetwork.removeOperator` - Removes an existing operator +* `SSVNetwork.setOperatorWhitelist` - Sets the whitelist address for an operator +* `SSVNetwork.declareOperatorFee` - Declares the operator's fee change +* `SSVNetwork.executeOperatorFee` - Executes the operator's fee change +* `SSVNetwork.cancelDeclaredOperatorFee` - Cancels the declared operator's fee +* `SSVNetwork.reduceOperatorFee` - Reduces the operator's fee +* `SSVNetwork.withdrawOperatorEarnings` - Withdraws operator earnings +* `SSVNetwork.withdrawAllOperatorEarnings` - Withdraws all operator earnings + +## Cluster owner +Only the owner of a cluster can execute these functions: + +* `SSVNetwork.registerValidator` - Registers a new validator on the SSV Network +* `SSVNetwork.removeValidator` - Removes an existing validator from the SSV Network +* `SSVNetwork.reactivate` - Reactivates a cluster +* `SSVNetwork.withdraw` - Withdraws tokens from a cluster \ No newline at end of file diff --git a/docs/setup.md b/docs/setup.md index 05b6d51f..c6c6da37 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -1,6 +1,6 @@ # SSV Network -### [Intro](../README.md) | [Architecture](architecture.md) | Setup | [Tasks](tasks.md) | [Local development](local-dev.md) +### [Intro](../README.md) | [Architecture](architecture.md) | Setup | [Tasks](tasks.md) | [Local development](local-dev.md) | [Roles](roles.md) ## Developer Setup diff --git a/docs/tasks.md b/docs/tasks.md index d1de1956..5b0cf337 100644 --- a/docs/tasks.md +++ b/docs/tasks.md @@ -1,6 +1,6 @@ # SSV Network -### [Intro](../README.md) | [Architecture](architecture.md) | [Setup](setup.md) | Tasks | [Local development](local-dev.md) +### [Intro](../README.md) | [Architecture](architecture.md) | [Setup](setup.md) | Tasks | [Local development](local-dev.md) | [Roles](roles.md) ## Development scripts All scripts can be executed using `package.json` scripts. @@ -117,7 +117,7 @@ OPTIONS: --proxy-address Proxy address of SSVNetwork / SSVNetworkViews (default: null) Example: -npx hardhat --network goerli upgrade:prepare --proxyAddress 0x1234... --contract SSVNetworkViewsV2 +npx hardhat --network goerli upgrade:prepare --proxy-address 0x1234... --contract SSVNetworkViewsV2 ``` The task will return the new implementation address. After that, you can run `upgradeTo` or `upgradeToAndCall` in SSVNetwork / SSVNetworkViews proxy address, providing it as a parameter. From 470fc452e4e92f2bd9278d2f3c8cdd07b47b24f2 Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Mon, 17 Jul 2023 11:40:22 +0200 Subject: [PATCH 8/8] Add missed import (#252) * fix import * setFeeRecipientAddress in main contract, update version --- contracts/SSVNetwork.sol | 2 +- contracts/interfaces/ISSVNetwork.sol | 2 ++ contracts/interfaces/ISSVOperators.sol | 4 ---- contracts/libraries/CoreLib.sol | 2 +- contracts/modules/SSVDAO.sol | 1 + contracts/modules/SSVOperators.sol | 4 ---- contracts/test/interfaces/ISSVNetworkT.sol | 2 ++ contracts/test/libraries/CoreLibT.sol | 2 +- contracts/test/modules/SSVOperatorsUpdate.sol | 4 ---- test/deployment/version.ts | 2 +- test/helpers/contract-helpers.ts | 2 +- 11 files changed, 10 insertions(+), 17 deletions(-) diff --git a/contracts/SSVNetwork.sol b/contracts/SSVNetwork.sol index f5d0c116..976b63b5 100644 --- a/contracts/SSVNetwork.sol +++ b/contracts/SSVNetwork.sol @@ -161,7 +161,7 @@ contract SSVNetwork is /*******************************/ function setFeeRecipientAddress(address recipientAddress) external override { - _delegate(SSVStorage.load().ssvContracts[SSVModules.SSV_OPERATORS]); + emit FeeRecipientAddressUpdated(msg.sender, recipientAddress); } /*******************************/ diff --git a/contracts/interfaces/ISSVNetwork.sol b/contracts/interfaces/ISSVNetwork.sol index 273d476e..de6e5e73 100644 --- a/contracts/interfaces/ISSVNetwork.sol +++ b/contracts/interfaces/ISSVNetwork.sol @@ -28,6 +28,8 @@ interface ISSVNetwork { function getVersion() external pure returns (string memory version); + function setFeeRecipientAddress(address feeRecipientAddress) external; + function updateModule(SSVModules moduleId, address moduleAddress) external; function setRegisterAuth(address userAddress, bool authOperators, bool authValidators) external; diff --git a/contracts/interfaces/ISSVOperators.sol b/contracts/interfaces/ISSVOperators.sol index 280d1237..c77c2329 100644 --- a/contracts/interfaces/ISSVOperators.sol +++ b/contracts/interfaces/ISSVOperators.sol @@ -36,10 +36,6 @@ interface ISSVOperators is ISSVNetworkCore { /// @param fee The new Operator's fee (SSV) function reduceOperatorFee(uint64 operatorId, uint256 fee) external; - /// @notice Sets the fee recipient address - /// @param feeRecipientAddress The address to receive Operator's fee - function setFeeRecipientAddress(address feeRecipientAddress) external; - /// @notice Withdraws operator earnings /// @param operatorId The ID of the operator /// @param tokenAmount The amount of tokens to withdraw (SSV) diff --git a/contracts/libraries/CoreLib.sol b/contracts/libraries/CoreLib.sol index 6597a3b0..e8d31429 100644 --- a/contracts/libraries/CoreLib.sol +++ b/contracts/libraries/CoreLib.sol @@ -7,7 +7,7 @@ library CoreLib { event ModuleUpgraded(SSVModules indexed moduleId, address moduleAddress); function getVersion() internal pure returns (string memory) { - return "v1.0.0.rc2"; + return "v1.0.0.rc3"; } function transferBalance(address to, uint256 amount) internal { diff --git a/contracts/modules/SSVDAO.sol b/contracts/modules/SSVDAO.sol index 8c5128ee..b4c6a028 100644 --- a/contracts/modules/SSVDAO.sol +++ b/contracts/modules/SSVDAO.sol @@ -4,6 +4,7 @@ pragma solidity 0.8.18; import "../interfaces/ISSVDAO.sol"; import "../libraries/Types.sol"; import "../libraries/ProtocolLib.sol"; +import "../libraries/CoreLib.sol"; contract SSVDAO is ISSVDAO { using Types64 for uint64; diff --git a/contracts/modules/SSVOperators.sol b/contracts/modules/SSVOperators.sol index 05290dc8..38347aeb 100644 --- a/contracts/modules/SSVOperators.sol +++ b/contracts/modules/SSVOperators.sol @@ -166,10 +166,6 @@ contract SSVOperators is ISSVOperators { emit OperatorFeeExecuted(msg.sender, operatorId, block.number, fee); } - function setFeeRecipientAddress(address recipientAddress) external override { - emit FeeRecipientAddressUpdated(msg.sender, recipientAddress); - } - function withdrawOperatorEarnings(uint64 operatorId, uint256 amount) external override { _withdrawOperatorEarnings(operatorId, amount); } diff --git a/contracts/test/interfaces/ISSVNetworkT.sol b/contracts/test/interfaces/ISSVNetworkT.sol index 3a3cf357..eaa3d0e3 100644 --- a/contracts/test/interfaces/ISSVNetworkT.sol +++ b/contracts/test/interfaces/ISSVNetworkT.sol @@ -24,4 +24,6 @@ interface ISSVNetworkT { uint64 executeOperatorFeePeriod_, uint64 operatorMaxFeeIncrease_ ) external; + + function setFeeRecipientAddress(address feeRecipientAddress) external; } diff --git a/contracts/test/libraries/CoreLibT.sol b/contracts/test/libraries/CoreLibT.sol index 929841e3..9125938f 100644 --- a/contracts/test/libraries/CoreLibT.sol +++ b/contracts/test/libraries/CoreLibT.sol @@ -6,7 +6,7 @@ import "../../libraries/SSVStorage.sol"; library CoreLibT { function getVersion() internal pure returns (string memory) { - return "v1.0.0.rc2"; + return "v1.0.0.rc3"; } function transfer(address to, uint256 amount) internal { diff --git a/contracts/test/modules/SSVOperatorsUpdate.sol b/contracts/test/modules/SSVOperatorsUpdate.sol index 99d93e0b..1120a292 100644 --- a/contracts/test/modules/SSVOperatorsUpdate.sol +++ b/contracts/test/modules/SSVOperatorsUpdate.sol @@ -141,10 +141,6 @@ contract SSVOperatorsUpdate is ISSVOperators { emit OperatorFeeExecuted(msg.sender, operatorId, block.number, fee); } - function setFeeRecipientAddress(address recipientAddress) external override { - emit FeeRecipientAddressUpdated(msg.sender, recipientAddress); - } - function withdrawOperatorEarnings(uint64 operatorId, uint256 amount) external override { _withdrawOperatorEarnings(operatorId, amount); } diff --git a/test/deployment/version.ts b/test/deployment/version.ts index b55c817a..d13aa736 100644 --- a/test/deployment/version.ts +++ b/test/deployment/version.ts @@ -20,7 +20,7 @@ describe('Version upgrade tests', () => { await ssvNetworkContract.updateModule(helpers.SSV_MODULES.SSV_VIEWS, viewsContract.address) - expect(await ssvNetworkViews.getVersion()).to.equal("v1.0.0.rc2"); + expect(await ssvNetworkViews.getVersion()).to.equal("v1.0.0.rc3"); }); }); \ No newline at end of file diff --git a/test/helpers/contract-helpers.ts b/test/helpers/contract-helpers.ts index 236e50e7..eedb34cb 100644 --- a/test/helpers/contract-helpers.ts +++ b/test/helpers/contract-helpers.ts @@ -65,7 +65,7 @@ export const DataGenerator = { export const initializeContract = async () => { CONFIG = { - initialVersion: "v1.0.0.rc2", + initialVersion: "v1.0.0.rc3", operatorMaxFeeIncrease: 1000, declareOperatorFeePeriod: 3600, // HOUR executeOperatorFeePeriod: 86400, // DAY