feat: triggerable withdrawals vebo part (val-1456) #927
Conversation
…bo-legacy-tests-WC-lib
…appyPath and triggerExitHashVerify tests
tamtamchik
changed the title
tamtamchik
added
solidity
Amuhar
changed the base branch from
feat/val-1456-triggerable-withdrawals-vebo
to
feat/val-1556-move-vebo-legacy-tests-WC-lib
Amuhar
changed the base branch from
feat/val-1556-move-vebo-legacy-tests-WC-lib
to
feat/withdrawal-credentials
|
Hardhat Unit Tests Coverage SummaryDiff against masterResults for commit: 3e732bf Minimum allowed coverage is ♻️ This comment has been updated with latest results |
| if (keyIndexes[i] > lastDeliveredKeyIndex) { | ||
| revert KeyWasNotUnpacked(keyIndexes[i], lastDeliveredKeyIndex); | ||
| } | ||
| uint256 requestOffset = keyIndexes[i] * PACKED_REQUEST_LENGTH + 16; |
There was a problem hiding this comment.
A minor suggestion to check requestOffset is not out of range.
if (requestOffset + PUBLIC_KEY_LENGTH > data.length) {
revert OutOfRangeDataAccess();
}
There was a problem hiding this comment.
Maybe better this way
https://github.com/lidofinance/core/pull/927/files#r1966406996
There was a problem hiding this comment.
Let's define what is 16.
MSB <---------------------------------------------------- LSB
| 24 bits: moduleId | 40 bits: nodeOpId | 64 bits: valIndex |
Or even define and replace it with constants.
Like keyIndexes[i] * PACKED_REQUEST_LENGTH + PACKED_MODULE_ID_LENGHT + PACKED_NODE_OP_ID_LENGTH + PACKED_VALIDATOR_INDEX_SIZE
| // TODO: timestamp | ||
| } | ||
| // TODO: make type optimization | ||
| struct RequestStatus { |
There was a problem hiding this comment.
Should we optimize storage?
Current:
mapping(bytes32 => RequestStatus) requests;
struct DeliveryHistory {
uint256 blockNumber;
uint256 lastDeliveredKeyIndex;
}
struct RequestStatus {
uint256 totalItemsCount;
uint256 deliveredItemsCount;
uint256 contractVersion;
DeliveryHistory[] deliverHistory;
}
Suggested:
mapping(bytes32 => RequestStatusPacked) requestsBasic;
mapping(bytes32 => DeliveryHistoryPacked[]) deliveryHistories;
struct RequestStatusPacked {
uint16 contractVersion;
uint16 totalItemsCount;
uint16 deliveredItemsCount;
uint16 deliverHistoryLength;
uint32 latestDeliveredBlockNumber;
uint32 latestDeliveredTimestamp;
}
struct DeliveryHistoryPacked {
uint16 lastDeliveredKeyIndex;
uint32 blockNumber;
uint32 blockTimestamp;
}
I suspect that our reports will typically be processed in a single pass, so storing an array of DeliveryHistory with one entry per report in storage seems excessive. Instead, we store deliverHistoryLength and information about the last report processing (which is usually the only one) in RequestStatusPacked. This way, in 99% of cases, we don't need to keep an additional history array in storage.
There was a problem hiding this comment.
Contract version should be uint256. It's defined in contract Versioned.
Also I'd prefer do such optimization in last iteration. I don't have strong expiriens in solidity, but seems it would make logic more complex
There was a problem hiding this comment.
IMHO it will not overcomplicate implementation but save a lot of gas.
|
I found it a bit inconvenient to jump between ValidatorsExitBus.sol and ValidatorsExitBusOracle.sol. For example, the description of the report data format ( IMHO, the logic in these files seems tightly coupled, and separating it into two files does not simplify things. I suggest combining ValidatorsExitBus.sol and ValidatorsExitBusOracle.sol into a single file. |
| require(success, "Refund failed"); | ||
| revert TriggerableWithdrawalRefundFailed(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Small suggestion.
1. Save the initial balance before processing withdrawal requests.
uint256 prevBalance = address(this).balance - msg.value;
2. After finishing processing and making a refund, check invariant
assert(address(this).balance == prevBalance);
| @@ -109,6 +111,10 @@ contract ValidatorsExitBusOracle is BaseOracle, PausableUntil { | |||
| _initialize(consensusContract, consensusVersion, lastProcessingRefSlot); | |||
| } | |||
|
|
|||
| function finalizeUpgrade_v2() external { | |||
There was a problem hiding this comment.
This method should be part of VEB
|
|
||
| bytes memory pubkeys = new bytes(keyIndexes.length * PUBLIC_KEY_LENGTH); | ||
|
|
||
| for (uint256 i = 0; i < keyIndexes.length; i++) { |
There was a problem hiding this comment.
I propose to add here check that
require(keyIndexes[i] < requestStatus.totalItemsCount)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function triggerExits(ExitRequestData calldata request, uint256[] calldata keyIndexes) external payable { | ||
| uint256 prevBalance = address(this).balance - msg.value; | ||
| RequestStatus storage requestStatus = _storageExitRequestsHashes()[keccak256(abi.encode(request.data, request.dataFormat))]; | ||
| bytes calldata data = request.data; | ||
|
|
||
| if (requestStatus.contractVersion == 0) { | ||
| revert ExitHashWasNotSubmitted(); | ||
| } | ||
|
|
||
| address locatorAddr = LOCATOR_CONTRACT_POSITION.getStorageAddress(); | ||
| address withdrawalVaultAddr = ILidoLocator(locatorAddr).withdrawalVault(); | ||
| uint256 withdrawalFee = IWithdrawalVault(withdrawalVaultAddr).getWithdrawalRequestFee(); | ||
|
|
||
| if (msg.value < keyIndexes.length * withdrawalFee ) { | ||
| revert InsufficientPayment(withdrawalFee, keyIndexes.length, msg.value); | ||
| } | ||
|
|
||
| uint256 lastDeliveredKeyIndex = requestStatus.deliveredItemsCount - 1; | ||
|
|
||
| bytes memory pubkeys = new bytes(keyIndexes.length * PUBLIC_KEY_LENGTH); | ||
|
|
||
| for (uint256 i = 0; i < keyIndexes.length; i++) { | ||
| if (keyIndexes[i] >= requestStatus.totalItemsCount) { | ||
| revert KeyIndexOutOfRange(keyIndexes[i], requestStatus.totalItemsCount); | ||
| } | ||
|
|
||
| if (keyIndexes[i] > lastDeliveredKeyIndex) { | ||
| revert KeyWasNotDelivered(keyIndexes[i], lastDeliveredKeyIndex); | ||
| } | ||
|
|
||
| /// | ||
| /// | 3 bytes | 5 bytes | 8 bytes | 48 bytes | | ||
| /// | moduleId | nodeOpId | validatorIndex | validatorPubkey | | ||
| /// 16 bytes - part without pubkey | ||
| uint256 requestPublicKeyOffset = keyIndexes[i] * PACKED_REQUEST_LENGTH + 16; | ||
| uint256 destOffset = i * PUBLIC_KEY_LENGTH; | ||
|
|
||
| assembly { | ||
| let dest := add(pubkeys, add(32, destOffset)) | ||
| calldatacopy( | ||
| dest, | ||
| add(data.offset, requestPublicKeyOffset), | ||
| PUBLIC_KEY_LENGTH | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| IWithdrawalVault(withdrawalVaultAddr).addFullWithdrawalRequests{value: keyIndexes.length * withdrawalFee}(pubkeys); | ||
|
|
||
| uint256 refund = msg.value - keyIndexes.length * withdrawalFee; | ||
|
|
||
| if (refund > 0) { | ||
| (bool success, ) = msg.sender.call{value: refund}(""); | ||
|
|
||
| if (!success) { | ||
| revert TriggerableWithdrawalRefundFailed(); | ||
| } | ||
|
|
||
| emit MadeRefund(msg.sender, refund); | ||
| } | ||
|
|
||
| assert(address(this).balance == prevBalance); | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
A short summary of the changes.
Context
What the reviewer needs to know
Problem
What problem this PR solves, link relevant issue if it exists
Solution
Your proposed solution