Feat/withdrawal credentials #904
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hardhat Unit Tests Coverage SummaryDiff against masterResults for commit: 66ccbcf Minimum allowed coverage is ♻️ This comment has been updated with latest results |
| revert NoWithdrawalRequests(); | ||
| } | ||
|
|
||
| uint256 minFeePerRequest = getWithdrawalRequestFee(); |
There was a problem hiding this comment.
Doesn't fee increase with each request?
There was a problem hiding this comment.
No, the fee will not increase with each request. Inside the transaction, all requests will have the same fee.
EIP 7002 uses block-by-block behavior.
If block N processes X requests, then at the end of block N the number of withdrawal requests that the chain has processed relative to the “targeted” number increases by X - TARGET_WITHDRAWAL_REQUESTS_PER_BLOCK, and so the fee in block N+1 increases by a factor of e**((X - TARGET_WITHDRAWAL_REQUESTS_PER_BLOCK) / WITHDRAWAL_REQUEST_FEE_UPDATE_FRACTION).
| */ | ||
| function addFullWithdrawalRequests( | ||
| bytes[] calldata pubkeys, | ||
| uint256 totalWithdrawalFee |
There was a problem hiding this comment.
It think that this parameter is not required at all. We can safely assume that all required ether is on the balance of the contract and we'll revert if it's not true and if we need some additional constraints (like, msg.value == fee), we can add it in the contract that use that lib.
There was a problem hiding this comment.
This parameter was introduced to decouple the fee allocation strategy from the withdrawals library, discussion. It enables contracts to employ different allocation strategies.
The proposed Validator Exitt Bus Triggerable Withdrawal implementation assumes that the withdrawal fee is provided by the actor who triggers the withdrawals. In this approach, the WithdrawalVault.sol balance remains unaffected, preventing issues with the Oracle’s accounting. Consequently, the entire msg.value sent is used as the fee for withdrawal requests:
// Simplified pseudo-code
function addWithdrawalRequests(
bytes[] calldata pubkeys,
uint64[] calldata amounts
) external payable {
// Use the entire sent amount (msg.value) as the total fee for withdrawal requests
uint256 totalWithdrawalFee = msg.value;
WithdrawalRequests.addWithdrawalRequests(pubkeys, amounts, totalWithdrawalFee);
}Other vaults could employ the strategy you mentioned, assuming all required Ether is already in the contract’s balance:
// Simplified pseudo-code
function addWithdrawalRequests(
bytes[] calldata pubkeys,
uint64[] calldata amounts
) external {
// Use the minimum required fee per request
uint256 minFeePerRequest = WithdrawalRequests.getWithdrawalRequestFee();
uint256 totalWithdrawalFee = minFeePerRequest * pubkeys.length;
WithdrawalRequests.addWithdrawalRequests(pubkeys, amounts, totalWithdrawalFee);
}When the withdrawal fee is specified explicitly, any fee allocation strategy can be used. The library ensures that the provided fee sufficiently covers all requests and that the exact fee amount is spent.
There was a problem hiding this comment.
When the withdrawal fee is specified explicitly, any fee allocation strategy can be used. The library ensures that the provided fee sufficiently covers all requests and that the exact fee amount is spent.
See no reason to pass it inside the function when it can definitely be checked before and after the function call in the WithdrawalVault itself. It's kinda alien constraint for the raw withdrawal request creation library.
E.g. in the vaults we don't care where the funds for the gas will come from and we don't need to check it at all.
There was a problem hiding this comment.
The EIP-7002 specification does not impose an upper limit on the withdrawal request fee; this was intentionally designed for flexibility. The library implementation proposed in this PR follows the EIP-7002 specification and does not add any extra restrictions on withdrawal requests.
If we do not want the general purpose library, we can remove control over the request fee, and narrow the library's functionality to allow only requests with minimal fees. This would simplify the code slightly, but also limit the library's potential use cases.
I am not confident that we will not need control over the request fee in the future, @folkyatina what do you think?
There was a problem hiding this comment.
After an internal discussion, we agreed to follow the eip 7002 specification and keep control over the request fee amount, but pass the request fee instead of the total withdrawal fee to simplify library implementation.
| */ | ||
| function addFullWithdrawalRequests( | ||
| bytes[] calldata pubkeys, | ||
| uint256 totalWithdrawalFee |
There was a problem hiding this comment.
When the withdrawal fee is specified explicitly, any fee allocation strategy can be used. The library ensures that the provided fee sufficiently covers all requests and that the exact fee amount is spent.
See no reason to pass it inside the function when it can definitely be checked before and after the function call in the WithdrawalVault itself. It's kinda alien constraint for the raw withdrawal request creation library.
E.g. in the vaults we don't care where the funds for the gas will come from and we don't need to check it at all.
Add role ADD_FULL_WITHDRAWAL_REQUEST_ROLE for full withdrawal requests.
…hdrawal-credentials
Access pubkeys and amounts directly instead of copying them to memory.
pass pubkeys as array of bytes
| function addFullWithdrawalRequests( | ||
| bytes calldata pubkeys | ||
| ) external payable onlyRole(ADD_FULL_WITHDRAWAL_REQUEST_ROLE) { | ||
| uint256 prevBalance = address(this).balance - msg.value; | ||
|
|
||
| uint256 minFeePerRequest = TriggerableWithdrawals.getWithdrawalRequestFee(); | ||
| uint256 totalFee = pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH * minFeePerRequest; | ||
|
|
||
| if(totalFee > msg.value) { | ||
| revert InsufficientTriggerableWithdrawalFee( | ||
| msg.value, | ||
| totalFee, | ||
| pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH | ||
| ); | ||
| } | ||
|
|
||
| TriggerableWithdrawals.addFullWithdrawalRequests(pubkeys, minFeePerRequest); | ||
|
|
||
| uint256 refund = msg.value - totalFee; | ||
| if (refund > 0) { | ||
| (bool success, ) = msg.sender.call{value: refund}(""); | ||
|
|
||
| if (!success) { | ||
| revert TriggerableWithdrawalRefundFailed(); | ||
| } | ||
| } | ||
|
|
||
| assert(address(this).balance == prevBalance); | ||
| } |
Check warning
Code scanning / Slither
Divide before multiply Medium
| function addFullWithdrawalRequests( | ||
| bytes calldata pubkeys | ||
| ) external payable onlyRole(ADD_FULL_WITHDRAWAL_REQUEST_ROLE) { | ||
| uint256 prevBalance = address(this).balance - msg.value; | ||
|
|
||
| uint256 minFeePerRequest = TriggerableWithdrawals.getWithdrawalRequestFee(); | ||
| uint256 totalFee = pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH * minFeePerRequest; | ||
|
|
||
| if(totalFee > msg.value) { | ||
| revert InsufficientTriggerableWithdrawalFee( | ||
| msg.value, | ||
| totalFee, | ||
| pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH | ||
| ); | ||
| } | ||
|
|
||
| TriggerableWithdrawals.addFullWithdrawalRequests(pubkeys, minFeePerRequest); | ||
|
|
||
| uint256 refund = msg.value - totalFee; | ||
| if (refund > 0) { | ||
| (bool success, ) = msg.sender.call{value: refund}(""); | ||
|
|
||
| if (!success) { | ||
| revert TriggerableWithdrawalRefundFailed(); | ||
| } | ||
| } | ||
|
|
||
| assert(address(this).balance == prevBalance); | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
| revert TreasuryZeroAddress(); | ||
| } | ||
| constructor(address _lido, address _treasury) { | ||
| _requireNonZero(_lido); |
There was a problem hiding this comment.
| _requireNonZero(_lido); | |
| _onlyNonZeroAddress(_lido); |
| @@ -0,0 +1,163 @@ | |||
| // SPDX-FileCopyrightText: 2023 Lido <info@lido.fi> | |||
There was a problem hiding this comment.
| // SPDX-FileCopyrightText: 2023 Lido <info@lido.fi> | |
| // SPDX-FileCopyrightText: 2025 Lido <info@lido.fi> |
|
|
||
| pragma solidity 0.8.9; | ||
| library TriggerableWithdrawals { | ||
| address constant WITHDRAWAL_REQUEST = 0x0c15F14308530b7CDB8460094BbB9cC28b9AaaAA; |
There was a problem hiding this comment.
Add some comment with "validation" link?
| address constant WITHDRAWAL_REQUEST = 0x0c15F14308530b7CDB8460094BbB9cC28b9AaaAA; | |
| /// @dev https://eips.ethereum.org/EIPS/eip-7002#configuration | |
| address constant WITHDRAWAL_REQUEST = 0x0c15F14308530b7CDB8460094BbB9cC28b9AaaAA; |
There was a problem hiding this comment.
May this contract address be changed on testnets to something else (as, for example, with the deposit contract on Holesky)? We'll have to use a separate library for it?
| // SPDX-FileCopyrightText: 2023 Lido <info@lido.fi> | ||
| // SPDX-License-Identifier: GPL-3.0 | ||
|
|
||
| pragma solidity 0.8.9; |
There was a problem hiding this comment.
| pragma solidity 0.8.9; | |
| // solhint-disable-next-line lido/fixed-compiler-version | |
| pragma solidity >=0.8.9 <0.9.0; |
Also, consider moving to common libs?
| if (!success) { | ||
| revert WithdrawalRequestAdditionFailed(callData); | ||
| } | ||
| } |
There was a problem hiding this comment.
emit WithdrawalRequestAdded(...) ? Or is this done on the WITHDRAWAL_REQUEST contract side, or should it be done on the side of the contract that uses this library?
| @@ -9,6 +9,9 @@ import "@openzeppelin/contracts-v4.4/token/ERC721/IERC721.sol"; | |||
| import "@openzeppelin/contracts-v4.4/token/ERC20/utils/SafeERC20.sol"; | |||
|
|
|||
| import {Versioned} from "./utils/Versioned.sol"; | |||
| import {AccessControlEnumerable} from "./utils/access/AccessControlEnumerable.sol"; | |||
| import {TriggerableWithdrawals} from "./lib/TriggerableWithdrawals.sol"; | |||
| import { ILidoLocator } from "../common/interfaces/ILidoLocator.sol"; | |||
There was a problem hiding this comment.
| import { ILidoLocator } from "../common/interfaces/ILidoLocator.sol"; | |
| import {ILidoLocator} from "../common/interfaces/ILidoLocator.sol"; |
| uint256 minFeePerRequest = TriggerableWithdrawals.getWithdrawalRequestFee(); | ||
| uint256 totalFee = pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH * minFeePerRequest; | ||
|
|
||
| if(totalFee > msg.value) { |
There was a problem hiding this comment.
| if(totalFee > msg.value) { | |
| if (totalFee > msg.value) { |
| } | ||
|
|
||
| function _validateAndCountPubkeys(bytes calldata pubkeys) private pure returns (uint256) { | ||
| if(pubkeys.length % PUBLIC_KEY_LENGTH != 0) { |
There was a problem hiding this comment.
| if(pubkeys.length % PUBLIC_KEY_LENGTH != 0) { | |
| if (pubkeys.length % PUBLIC_KEY_LENGTH != 0) { |
| revert InsufficientRequestFee(feePerRequest, minFeePerRequest); | ||
| } | ||
|
|
||
| if(address(this).balance < feePerRequest * keysCount) { |
There was a problem hiding this comment.
| if(address(this).balance < feePerRequest * keysCount) { | |
| if (address(this).balance < feePerRequest * keysCount) { |
| uint256 prevBalance = address(this).balance - msg.value; | ||
|
|
||
| uint256 minFeePerRequest = TriggerableWithdrawals.getWithdrawalRequestFee(); | ||
| uint256 totalFee = pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH * minFeePerRequest; |
There was a problem hiding this comment.
| uint256 totalFee = pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH * minFeePerRequest; | |
| uint256 totalFee = pubkeys.length / TriggerableWithdrawals.PUBLIC_KEY_LENGTH * minFeePerRequest; |
Summary
This PR introduces an early-stage prototype for a potential implementation of EIP-7685: General Purpose Execution Layer Requests. Specifically, it implements EIP-7002: Execution Layer Triggerable Withdrawals within the Lido WithdrawalVault contract, which serves as the withdrawal credentials address for Lido validators.
Approach
The implementation follows the first approach outlined in the ADR for Withdrawal Credentials Contract. This approach was selected due to the following advantages:
Implementation Details
In this iteration, the
WithdrawalVaultcontract supports only full withdrawal requests. Partial withdrawals are not included because they are not part of the proposed Triggerable Withdrawal V1 implementation within the Lido protocol.The following pseudo-code demonstrates the approach:
Key Considerations
The modular design of separate libraries for different withdrawal request types ensures that withdrawal credentials contracts can import and use only the minimal functionality required. For example:
Notes
This PR is an initial prototype and is subject to further iterations based on feedback and evolving requirements.