Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve comments in the contract's code. #872

Open
wants to merge 4 commits into
base: feat/sr-1.5
Choose a base branch
from
Open

Improve comments in the contract's code. #872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7f8411b
refactor: update comments in AccountingOracle contract
mkurayan Nov 12, 2024
2d1c5fc
refactor: add a short description for the DepositSecurityModule contract
mkurayan Nov 13, 2024
0f565e0
refactor: rename ZERO_HASH to ZERO_BYTES32
mkurayan Nov 13, 2024
508cd74
refactor: move unvet payload validation to dedicated function
mkurayan Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 25 additions & 14 deletions contracts/0.8.9/DepositSecurityModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
/**
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
}
}
53 changes: 27 additions & 26 deletions contracts/0.8.9/oracle/AccountingOracle.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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;

Expand All @@ -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;

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
Expand All @@ -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();
}
}
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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);
}
Expand Down
Loading