Staking Router 1.5 #78
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Code Coverage SummaryDiff against masterResults for commit: fafa232 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Depending on the size of the third-phase report, it may be split into multiple transactions.
Distribute reward separately using the permissionless method in each staking module.
…-third-phase-fix Feat/multi transaction oracle third phase fix
Update contract version and reward distribution state.
Ackee I3: Typo in node operator registry function name
Sr 1.5 sync
docs: description for sr deploy params
| */ | ||
| function setMaxDeposits(uint256 newValue) external onlyOwner { | ||
| _setMaxDeposits(newValue); | ||
| function setMaxOperatorsPerUnvetting(uint256 newValue) external onlyOwner { |
There was a problem hiding this comment.
I suggest to add note that this is really technical constraint to avoid unbound loop issue and meaningful value for this param would be ~200 or so...
|
|
||
| uint256 nodeOperatorsCount = nodeOperatorIds.length / 8; | ||
|
|
||
| if ( |
There was a problem hiding this comment.
it's unclear from first glance that the question is in the way how the line might be cut in pieces
There was a problem hiding this comment.
yeah, would suggest extracting a helper func to validate the input calldata
| uint256 stakingModuleId, | ||
| uint256 depositsValue | ||
| ) internal { | ||
| stakingModule.lastDepositAt = uint64(block.timestamp); |
There was a problem hiding this comment.
Do we need this counter cause we have this _setLastDepositBlock(block.number); within deposit security module.
| /// as the next transaction hash. | ||
| /// | ||
| /// | 32 bytes | array of items | ||
| /// | nextHash | ... |
There was a problem hiding this comment.
please update the comment within line 361, cause bytes32 extraDataHash; is now hash of first item in the chain but not hash of the whole extra data
There was a problem hiding this comment.
based on the code, it's in a different order: nextHash and then data chunk and not the other way
| assembly { | ||
| dataHash := calldataload(data.offset) | ||
| } | ||
|
|
There was a problem hiding this comment.
why so hacky?
bytes32(msg.data[data.offset:data.offset+32])
We usually try to avoid inline assembly when possbile.
…1.5-deploy-scripts
…ocator Use steth on optimism locator impl to fetch addresses for SR2 deploy
SR 1.5 deploy scripts
…limit Update CL_BALANCE_ORACLES_ERROR_UPPER_BP_LIMIT
Deployed mainnet SR 1.5
| @@ -772,7 +803,7 @@ contract AccountingOracle is BaseOracle { | |||
| dataOffset := add(dataOffset, 5) | |||
| } | |||
|
|
|||
| if (iter.itemType == 0) { | |||
| if (iter.lastSortingKey == 0) { | |||
There was a problem hiding this comment.
what? looks like it's overcomplicated logic, or even worse might be wrong for some edge cases
There was a problem hiding this comment.
double checked - it should work but it's very ugly
There was a problem hiding this comment.
if this is just checking the increase and sequence of item indices between transactions in the third phase, then please make it more semantic
There was a problem hiding this comment.
if (index != iter.nextIndex) (see my comment for line 750)
| ExtraDataIterState memory iter = ExtraDataIterState({ | ||
| index: 0, | ||
| index: procState.itemsProcessed > 0 ? procState.itemsProcessed - 1 : 0, |
There was a problem hiding this comment.
i'd simplify by renaming iter.index to iter.nextIndex, passing procState.itemsProcessed unconditionally here, and changing the check on lines 806-811 to if (index != iter.nextIndex)
| /// 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. |
There was a problem hiding this comment.
i'd avoid using the term "transaction hash" since it means a specific and different thing within the blockchain context
There was a problem hiding this comment.
also, the extra data format hasn't changed per see (it's still a list of items of the same format); what has changed is its transport format, LIST in this case: now it is not just an array of items but something more involved, having multiple chunks linked using hashes
so i'd ask to revert the changes in lines 278-286 and move the description of the transport format to line 388 where it actually belongs (see my comment there)
| @@ -357,13 +375,15 @@ 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 transaction. | |||
| /// fits into a single or multiple transactions. | |||
There was a problem hiding this comment.
no need to mention single/multiple here imo, i'd just say "Used when the oracle report contains extra data"
| /// | ||
| /// Extra data is passed within a single transaction as a bytearray containing all data 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 |
There was a problem hiding this comment.
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).
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.
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] |)
| @@ -400,11 +420,11 @@ contract AccountingOracle is BaseOracle { | |||
|
|
|||
| /// @notice Submits report extra data in the EXTRA_DATA_FORMAT_LIST format for processing. | |||
| /// | |||
| /// @param items The extra data items list. See docs for the `EXTRA_DATA_FORMAT_LIST` | |||
| /// @param data The extra data chunk with items list. See docs for the `EXTRA_DATA_FORMAT_LIST` | |||
There was a problem hiding this comment.
id remove "with items list", it misleads more than helps
| @@ -133,6 +132,8 @@ contract AccountingOracle is BaseOracle { | |||
| bytes32 internal constant EXTRA_DATA_PROCESSING_STATE_POSITION = | |||
| keccak256("lido.AccountingOracle.extraDataProcessingState"); | |||
|
|
|||
| bytes32 internal constant ZERO_HASH = bytes32(0); | |||
There was a problem hiding this comment.
i'd call it ZERO_BYTES32 instead since "zero hash" usually means "hash of a zero data" which is not zero bytes
| /// as the next transaction hash. | ||
| /// | ||
| /// | 32 bytes | array of items | ||
| /// | nextHash | ... |
There was a problem hiding this comment.
based on the code, it's in a different order: nextHash and then data chunk and not the other way
| ExtraDataIterState memory iter = ExtraDataIterState({ | ||
| index: 0, | ||
| index: procState.itemsProcessed > 0 ? procState.itemsProcessed - 1 : 0, |
There was a problem hiding this comment.
i'd simplify by renaming iter.index to iter.nextIndex, passing procState.itemsProcessed unconditionally here, and changing the check on lines 806-811 to if (index != iter.nextIndex)
| @@ -772,7 +803,7 @@ contract AccountingOracle is BaseOracle { | |||
| dataOffset := add(dataOffset, 5) | |||
| } | |||
|
|
|||
| if (iter.itemType == 0) { | |||
| if (iter.lastSortingKey == 0) { | |||
There was a problem hiding this comment.
if (index != iter.nextIndex) (see my comment for line 750)
|
|
||
| while (dataOffset < data.length) { | ||
| uint256 index; | ||
| uint256 itemType; |
There was a problem hiding this comment.
what's the reason for moving these two variables to a broader scope?
| @@ -93,8 +102,8 @@ contract NodeOperatorsRegistry is AragonApp, Versioned { | |||
| uint8 internal constant TOTAL_DEPOSITED_KEYS_COUNT_OFFSET = 3; | |||
|
|
|||
| // TargetValidatorsStats | |||
| /// @dev Flag enable/disable limiting target active validators count for operator | |||
| uint8 internal constant IS_TARGET_LIMIT_ACTIVE_OFFSET = 0; | |||
| /// @dev Target limit mode, allows limiting target active validators count for operator (0 = disabled, 1 = soft mode, 2 = forced mode) | |||
There was a problem hiding this comment.
docs should clearly outline that 'forced' = 'boosted'
| @@ -280,6 +295,22 @@ contract NodeOperatorsRegistry is AragonApp, Versioned { | |||
| emit StakingModuleTypeSet(_type); | |||
| } | |||
|
|
|||
| function finalizeUpgrade_v3() external { | |||
There was a problem hiding this comment.
Why we haven't removed finalizeUpgrade_v2?
There was a problem hiding this comment.
Good point, will put it in the backlog
| @@ -1254,6 +1355,9 @@ contract NodeOperatorsRegistry is AragonApp, Versioned { | |||
| _saveOperatorStuckPenaltyStats(_nodeOperatorId, stuckPenaltyStats); | |||
| _updateSummaryMaxValidatorsCount(_nodeOperatorId); | |||
| _increaseValidatorsKeysNonce(); | |||
|
|
|||
| emit NodeOperatorPenaltyCleared(_nodeOperatorId); | |||
| return true; | |||
There was a problem hiding this comment.
why do we need return bool though?
There was a problem hiding this comment.
This is a fix for a technical debt. The method interface assumed return bool, but always returned false because there was no return statement. This was found during a security audit. We preferred to add return: true and keep the method interface
|
|
||
| /** | ||
| * @title DepositSecurityModule | ||
| * @dev The contract represents a security module for handling deposits. |
There was a problem hiding this comment.
now we mention 'unvetting' in the methods below, would be great to give an insight on this term too
| _addGuardian(addresses[i]); | ||
|
|
||
| unchecked { |
There was a problem hiding this comment.
I am not sure that this optimization is worth doing tbh
This methods gets called barely once a year, but readability is a bit worse than it was
| bytes32 public constant REPORT_EXITED_VALIDATORS_ROLE = keccak256("REPORT_EXITED_VALIDATORS_ROLE"); | ||
| bytes32 public constant UNSAFE_SET_EXITED_VALIDATORS_ROLE = keccak256("UNSAFE_SET_EXITED_VALIDATORS_ROLE"); | ||
| bytes32 public constant REPORT_REWARDS_MINTED_ROLE = keccak256("REPORT_REWARDS_MINTED_ROLE"); | ||
|
|
||
| bytes32 internal constant LIDO_POSITION = keccak256("lido.StakingRouter.lido"); | ||
|
|
||
| /// @dev Credentials which allows the DAO to withdraw Ether on the 2.0 side | ||
| /// @dev Credentials to withdraw ETH on Consensus Layer side. |
| newStakingModule.lastDepositAt = uint64(block.timestamp); | ||
| newStakingModule.lastDepositBlock = block.number; | ||
| emit StakingRouterETHDeposited(newStakingModuleId, 0); | ||
| /// @dev Simulate zero value deposit to prevent real deposits into the new StakingModule via |
There was a problem hiding this comment.
Why it's called 'simulate'? maybe 'initialize' or 'pretend'?
There was a problem hiding this comment.
Why should it be prevented, though?
There was a problem hiding this comment.
To make sure that the minimum distance is kept between 2 sequential deposits in rare edge cases. For example, if we add a module and update DSM contract, as for example in SR+CSM update.
| revert ExtraDataHashCannotBeZeroForNonEmptyData(); | ||
| } | ||
| } | ||
|
|
||
| IOracleReportSanityChecker(LOCATOR.oracleReportSanityChecker()) | ||
| .checkAccountingExtraDataListItemsCount(data.extraDataItemsCount); | ||
|
|
||
| ILegacyOracle(LEGACY_ORACLE).handleConsensusLayerReport( |
There was a problem hiding this comment.
It would have been great having LEGACY_ORACLE removed completely 😢
Let's schedule it for the next upgrade
| @@ -105,13 +102,14 @@ struct LimitsList { | |||
|
|
|||
| /// @dev The packed version of the LimitsList struct to be effectively persisted in storage | |||
| struct LimitsListPacked { | |||
There was a problem hiding this comment.
cool, still fits 256bits 🍷
| @@ -31,7 +31,7 @@ library MinFirstAllocationStrategy { | |||
| uint256[] memory buckets, | |||
| uint256[] memory capacities, | |||
| uint256 allocationSize | |||
| ) internal pure returns (uint256 allocated) { | |||
| ) public pure returns (uint256 allocated, uint256[] memory) { | |||
There was a problem hiding this comment.
Please don't forget to add docs and artifacts bc now it's a separately deploying thing
No description provided.