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/DepositSecurityModule.sol b/contracts/0.8.9/DepositSecurityModule.sol index b39ef28bb..2464f172a 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 { /** @@ -579,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); @@ -625,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(); + } } 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/contracts/0.8.9/oracle/AccountingOracle.sol b/contracts/0.8.9/oracle/AccountingOracle.sol index 8225667b2..c9794876e 100644 --- a/contracts/0.8.9/oracle/AccountingOracle.sol +++ b/contracts/0.8.9/oracle/AccountingOracle.sol @@ -132,7 +132,7 @@ contract AccountingOracle is BaseOracle { bytes32 internal constant EXTRA_DATA_PROCESSING_STATE_POSITION = keccak256("lido.AccountingOracle.extraDataProcessingState"); - bytes32 internal constant ZERO_HASH = bytes32(0); + bytes32 internal constant ZERO_BYTES32 = bytes32(0); address public immutable LIDO; ILidoLocator public immutable LOCATOR; @@ -275,15 +275,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 +348,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 +366,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 +421,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 { @@ -461,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; } @@ -585,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); @@ -598,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(); } } @@ -708,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(); } @@ -757,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); } 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 () => {