From 7f8411b0348249ba7f84d91317a54bca3536a9e8 Mon Sep 17 00:00:00 2001 From: maksim Date: Tue, 12 Nov 2024 20:55:22 +0100 Subject: [PATCH 1/5] refactor: update comments in AccountingOracle contract --- contracts/0.8.9/oracle/AccountingOracle.sol | 40 +++++++++++---------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/contracts/0.8.9/oracle/AccountingOracle.sol b/contracts/0.8.9/oracle/AccountingOracle.sol index 8225667b2..0ac8f3dcb 100644 --- a/contracts/0.8.9/oracle/AccountingOracle.sol +++ b/contracts/0.8.9/oracle/AccountingOracle.sol @@ -132,6 +132,7 @@ contract AccountingOracle is BaseOracle { bytes32 internal constant EXTRA_DATA_PROCESSING_STATE_POSITION = keccak256("lido.AccountingOracle.extraDataProcessingState"); + /// @dev will be renamed to ZERO_BYTES32 bytes32 internal constant ZERO_HASH = bytes32(0); address public immutable LIDO; @@ -275,15 +276,7 @@ contract AccountingOracle is BaseOracle { /// data for a report is possible after its processing deadline passes or a new data report /// arrives. /// - /// Depending on the size of the extra data, the processing might need to be split into - /// multiple transactions. Each transaction contains a chunk of report data (an array of items) - /// and the hash of the next transaction. The last transaction will contain ZERO_HASH - /// as the next transaction hash. - /// - /// | 32 bytes | array of items - /// | nextHash | ... - /// - /// Each item being encoded as follows: + /// Extra data is an array of items, each item being encoded as follows: /// /// 3 bytes 2 bytes X bytes /// | itemIndex | itemType | itemPayload | @@ -356,7 +349,7 @@ contract AccountingOracle is BaseOracle { /// @dev Hash of the extra data. See the constant defining a specific extra data /// format for the info on how to calculate the hash. /// - /// Must be set to a zero hash if the oracle report contains no extra data. + /// Must be set to a `ZERO_BYTES32` if the oracle report contains no extra data. /// bytes32 extraDataHash; @@ -374,16 +367,25 @@ contract AccountingOracle is BaseOracle { /// uint256 public constant EXTRA_DATA_FORMAT_EMPTY = 0; - /// @notice The list format for the extra data array. Used when all extra data processing - /// fits into a single or multiple transactions. + /// @notice The list format for the extra data array. Used when the oracle reports contains extra data. + /// + /// Depending on the extra data size, it's passed within a single or multiple transactions. + /// Each transaction contains data consisting of 1) the keccak256 hash of the next + /// transaction's data or `ZERO_BYTES32` if there are no more data chunks, and 2) a chunk + /// of report data (an array of items). + /// + /// | 32 bytes | X bytes | + /// | Next transaction's data hash or `ZERO_BYTES32` | array of items | /// - /// Depend on the extra data size it passed within a single or multiple transactions. - /// Each transaction contains next transaction hash and a bytearray containing data items - /// packed tightly. + /// The `extraDataHash` field of the `ReportData` struct is calculated as a keccak256 hash + /// over the first transaction's data, i.e. over the first data chunk with the second + /// transaction's data hash (or `ZERO_BYTES32`) prepended. /// - /// Hash is a keccak256 hash calculated over the transaction data (next transaction hash and bytearray items). - /// The Solidity equivalent of the hash calculation code would be `keccak256(data)`, - /// where `data` has the `bytes` type. + /// ReportData.extraDataHash := hash0 + /// hash0 := keccak256(| hash1 | extraData[0], ... extraData[n] |) + /// hash1 := keccak256(| hash2 | extraData[n + 1], ... extraData[m] |) + /// ... + /// hashK := keccak256(| ZERO_BYTES32 | extraData[x + 1], ... extraData[extraDataItemsCount] |) /// uint256 public constant EXTRA_DATA_FORMAT_LIST = 1; @@ -420,7 +422,7 @@ contract AccountingOracle is BaseOracle { /// @notice Submits report extra data in the EXTRA_DATA_FORMAT_LIST format for processing. /// - /// @param data The extra data chunk with items list. See docs for the `EXTRA_DATA_FORMAT_LIST` + /// @param data The extra data chunk. See docs for the `EXTRA_DATA_FORMAT_LIST` /// constant for details. /// function submitReportExtraDataList(bytes calldata data) external { From 2d1c5fcb27d9859a6c66c7b95ebfae18908a3b90 Mon Sep 17 00:00:00 2001 From: maksim Date: Wed, 13 Nov 2024 12:47:44 +0100 Subject: [PATCH 2/5] refactor: add a short description for the DepositSecurityModule contract --- contracts/0.8.9/DepositSecurityModule.sol | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/contracts/0.8.9/DepositSecurityModule.sol b/contracts/0.8.9/DepositSecurityModule.sol index b39ef28bb..9fc77d971 100644 --- a/contracts/0.8.9/DepositSecurityModule.sol +++ b/contracts/0.8.9/DepositSecurityModule.sol @@ -32,6 +32,11 @@ interface IStakingRouter { /** * @title DepositSecurityModule * @dev The contract represents a security module for handling deposits. + * + * The contract allows pausing deposits in response to potential security incidents and + * requires a quorum of guardians to authorize deposit operations. It also provides a mechanism + * to unvet signing keys (a vetted key is a validator key approved for receiving ether deposits) + * in case of any issues. */ contract DepositSecurityModule { /** From 0f565e004459c0aaaf63b56ca60831ec8f6de98d Mon Sep 17 00:00:00 2001 From: maksim Date: Wed, 13 Nov 2024 17:17:39 +0100 Subject: [PATCH 3/5] refactor: rename ZERO_HASH to ZERO_BYTES32 --- contracts/0.8.9/oracle/AccountingOracle.sol | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/contracts/0.8.9/oracle/AccountingOracle.sol b/contracts/0.8.9/oracle/AccountingOracle.sol index 0ac8f3dcb..c9794876e 100644 --- a/contracts/0.8.9/oracle/AccountingOracle.sol +++ b/contracts/0.8.9/oracle/AccountingOracle.sol @@ -132,8 +132,7 @@ contract AccountingOracle is BaseOracle { bytes32 internal constant EXTRA_DATA_PROCESSING_STATE_POSITION = keccak256("lido.AccountingOracle.extraDataProcessingState"); - /// @dev will be renamed to ZERO_BYTES32 - bytes32 internal constant ZERO_HASH = bytes32(0); + bytes32 internal constant ZERO_BYTES32 = bytes32(0); address public immutable LIDO; ILidoLocator public immutable LOCATOR; @@ -463,7 +462,7 @@ contract AccountingOracle is BaseOracle { ConsensusReport memory report = _storageConsensusReport().value; result.currentFrameRefSlot = _getCurrentRefSlot(); - if (report.hash == ZERO_HASH || result.currentFrameRefSlot != report.refSlot) { + if (report.hash == ZERO_BYTES32 || result.currentFrameRefSlot != report.refSlot) { return result; } @@ -587,8 +586,8 @@ contract AccountingOracle is BaseOracle { function _handleConsensusReportData(ReportData calldata data, uint256 prevRefSlot) internal { if (data.extraDataFormat == EXTRA_DATA_FORMAT_EMPTY) { - if (data.extraDataHash != ZERO_HASH) { - revert UnexpectedExtraDataHash(ZERO_HASH, data.extraDataHash); + if (data.extraDataHash != ZERO_BYTES32) { + revert UnexpectedExtraDataHash(ZERO_BYTES32, data.extraDataHash); } if (data.extraDataItemsCount != 0) { revert UnexpectedExtraDataItemsCount(0, data.extraDataItemsCount); @@ -600,7 +599,7 @@ contract AccountingOracle is BaseOracle { if (data.extraDataItemsCount == 0) { revert ExtraDataItemsCountCannotBeZeroForNonEmptyData(); } - if (data.extraDataHash == ZERO_HASH) { + if (data.extraDataHash == ZERO_BYTES32) { revert ExtraDataHashCannotBeZeroForNonEmptyData(); } } @@ -710,7 +709,7 @@ contract AccountingOracle is BaseOracle { ConsensusReport memory report = _storageConsensusReport().value; - if (report.hash == ZERO_HASH || procState.refSlot != report.refSlot) { + if (report.hash == ZERO_BYTES32 || procState.refSlot != report.refSlot) { revert CannotSubmitExtraDataBeforeMainData(); } @@ -759,7 +758,7 @@ contract AccountingOracle is BaseOracle { _processExtraDataItems(data, iter); uint256 itemsProcessed = iter.index + 1; - if (dataHash == ZERO_HASH) { + if (dataHash == ZERO_BYTES32) { if (itemsProcessed != procState.itemsCount) { revert UnexpectedExtraDataItemsCount(procState.itemsCount, itemsProcessed); } From 508cd74a2889c641b6fbe4982ea5ae7dd003aaca Mon Sep 17 00:00:00 2001 From: maksim Date: Mon, 18 Nov 2024 11:02:59 +0700 Subject: [PATCH 4/5] refactor: move unvet payload validation to dedicated function --- contracts/0.8.9/DepositSecurityModule.sol | 34 +++++++++++++---------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/contracts/0.8.9/DepositSecurityModule.sol b/contracts/0.8.9/DepositSecurityModule.sol index 9fc77d971..2464f172a 100644 --- a/contracts/0.8.9/DepositSecurityModule.sol +++ b/contracts/0.8.9/DepositSecurityModule.sol @@ -584,20 +584,7 @@ contract DepositSecurityModule { bytes calldata vettedSigningKeysCounts, Signature calldata sig ) external { - /// @dev The most likely reason for the signature to go stale - uint256 onchainNonce = STAKING_ROUTER.getStakingModuleNonce(stakingModuleId); - if (nonce != onchainNonce) revert ModuleNonceChanged(); - - uint256 nodeOperatorsCount = nodeOperatorIds.length / 8; - - if ( - nodeOperatorIds.length % 8 != 0 || - vettedSigningKeysCounts.length % 16 != 0 || - vettedSigningKeysCounts.length / 16 != nodeOperatorsCount || - nodeOperatorsCount > maxOperatorsPerUnvetting - ) { - revert UnvetPayloadInvalid(); - } + _checkIfUnvetPayloadValid(nodeOperatorIds, vettedSigningKeysCounts); address guardianAddr = msg.sender; int256 guardianIndex = _getGuardianIndex(msg.sender); @@ -630,4 +617,23 @@ contract DepositSecurityModule { vettedSigningKeysCounts ); } + + function _checkIfUnvetPayloadValid( + uint256 stakingModuleId, + uint256 nonce, + bytes calldata nodeOperatorIds, + bytes calldata vettedSigningKeysCounts + ) internal view { + /// @dev The most likely reason for the signature to go stale + uint256 onchainNonce = STAKING_ROUTER.getStakingModuleNonce(stakingModuleId); + if (nonce != onchainNonce) revert ModuleNonceChanged(); + + uint256 nodeOperatorsCount = nodeOperatorIds.length / 8; + + if ( + nodeOperatorIds.length % 8 != 0 || + vettedSigningKeysCounts.length != nodeOperatorsCount * 16 || + nodeOperatorsCount > maxOperatorsPerUnvetting + ) revert UnvetPayloadInvalid(); + } } From f7c7632c715d73a6ca6ed242c1cb85b0ef68a05d Mon Sep 17 00:00:00 2001 From: maksim Date: Mon, 25 Nov 2024 11:07:13 +0100 Subject: [PATCH 5/5] refactor: rename forced to boosted use 'boosted' instead of 'forced' when we describe a mode when validators to exit are prioritized --- contracts/0.4.24/nos/NodeOperatorsRegistry.sol | 4 ++-- contracts/0.8.9/interfaces/IStakingModule.sol | 2 +- test/0.8.9/stakingRouter/stakingRouter.module-sync.test.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol index 1e5d30f3a..e7866e751 100644 --- a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol +++ b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol @@ -102,7 +102,7 @@ contract NodeOperatorsRegistry is AragonApp, Versioned { uint8 internal constant TOTAL_DEPOSITED_KEYS_COUNT_OFFSET = 3; // TargetValidatorsStats - /// @dev Target limit mode, allows limiting target active validators count for operator (0 = disabled, 1 = soft mode, 2 = forced mode) + /// @dev Target limit mode, allows limiting target active validators count for operator (0 = disabled, 1 = soft mode, 2 = boosted mode) uint8 internal constant TARGET_LIMIT_MODE_OFFSET = 0; /// @dev relative target active validators limit for operator, set by DAO /// @notice used to check how many keys should go to exit, 0 - means all deposited keys would be exited @@ -689,7 +689,7 @@ contract NodeOperatorsRegistry is AragonApp, Versioned { /// @notice Updates the limit of the validators that can be used for deposit by DAO /// @param _nodeOperatorId Id of the node operator - /// @param _targetLimitMode target limit mode (0 = disabled, 1 = soft mode, 2 = forced mode) + /// @param _targetLimitMode target limit mode (0 = disabled, 1 = soft mode, 2 = boosted mode) /// @param _targetLimit Target limit of the node operator function updateTargetValidatorsLimits(uint256 _nodeOperatorId, uint256 _targetLimitMode, uint256 _targetLimit) public { _onlyExistedNodeOperator(_nodeOperatorId); diff --git a/contracts/0.8.9/interfaces/IStakingModule.sol b/contracts/0.8.9/interfaces/IStakingModule.sol index 82d55cf05..bf7056ee3 100644 --- a/contracts/0.8.9/interfaces/IStakingModule.sol +++ b/contracts/0.8.9/interfaces/IStakingModule.sol @@ -23,7 +23,7 @@ interface IStakingModule { /// @notice Returns all-validators summary belonging to the node operator with the given id /// @param _nodeOperatorId id of the operator to return report for - /// @return targetLimitMode shows whether the current target limit applied to the node operator (0 = disabled, 1 = soft mode, 2 = forced mode) + /// @return targetLimitMode shows whether the current target limit applied to the node operator (0 = disabled, 1 = soft mode, 2 = boosted mode) /// @return targetValidatorsCount relative target active validators limit for operator /// @return stuckValidatorsCount number of validators with an expired request to exit time /// @return refundedValidatorsCount number of validators that can't be withdrawn, but deposit diff --git a/test/0.8.9/stakingRouter/stakingRouter.module-sync.test.ts b/test/0.8.9/stakingRouter/stakingRouter.module-sync.test.ts index b7bb09878..ead4a584b 100644 --- a/test/0.8.9/stakingRouter/stakingRouter.module-sync.test.ts +++ b/test/0.8.9/stakingRouter/stakingRouter.module-sync.test.ts @@ -340,7 +340,7 @@ describe("StakingRouter:module-sync", () => { context("updateTargetValidatorsLimits", () => { const NODE_OPERATOR_ID = 0n; - const TARGET_LIMIT_MODE = 1; // 1 - soft, i.e. on WQ request; 2 - forced + const TARGET_LIMIT_MODE = 1; // 1 - soft, i.e. on WQ request; 2 - boosted const TARGET_LIMIT = 100n; it("Reverts if the caller does not have the role", async () => {