[VAULTS] PredepositGuarantee #932
Open
+2,614
−186
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
TheDZhon
reviewed
Jan 30, 2025
| @@ -68,6 +68,7 @@ contract StakingVault is IStakingVault, OwnableUpgradeable { | |||
| uint128 locked; | |||
| int128 inOutDelta; | |||
| address nodeOperator; | |||
| // depositGuardian becomes the depositor, instead of just guardian, perhaps a renaming is needed 🌚 | |||
There was a problem hiding this comment.
maybe make depositGuardian immutable
There was a problem hiding this comment.
Changable but only for unconnceted vaults
|
|
||
| // TODO: think about naming. It's not a deposit guardian, it's the depositor itself | ||
| // TODO: minor UX improvement: perhaps there's way to reuse predeposits for a different validator without withdrawing | ||
| contract PredepositGuardian { |
There was a problem hiding this comment.
Suggested change
| contract PredepositGuardian { | |
| contract PredepositGuarantee { |
Comment on lines
14
to
15
| mapping(bytes32 validatorId => bool isPreDeposited) public validatorPredeposits; | ||
| mapping(bytes32 validatorId => bytes32 withdrawalCredentials) public validatorWithdrawalCredentials; |
There was a problem hiding this comment.
Suggested change
| mapping(bytes32 validatorId => bool isPreDeposited) public validatorPredeposits; | |
| mapping(bytes32 validatorId => bytes32 withdrawalCredentials) public validatorWithdrawalCredentials; | |
| mapping(address nodeOperator => uint256) public nodeOperatorGuarantee; | |
| mapping(bytes validatorPubkey => bytes32 withdrawalCredentials) public validatorWithdrawalCredentials; |
| mapping(bytes32 validatorId => bytes32 withdrawalCredentials) public validatorWithdrawalCredentials; | ||
|
|
||
| // Question: predeposit is permissionless, i.e. the msg.sender doesn't have to be the node operator, | ||
| // however, the deposit will still revert if it wasn't signed with the validator private key |
There was a problem hiding this comment.
this function can't be permissionless
There was a problem hiding this comment.
- let's include accounting and 'outsourcing' for funding
|
|
||
| // called by the staking vault owner if the predeposited validator has a different withdrawal credentials than the vault's withdrawal credentials, | ||
| // i.e. node operator was malicious | ||
| function withdrawDisprovenPredeposits( |
There was a problem hiding this comment.
not sure what should be done here
There was a problem hiding this comment.
let's implement optionality in Dashboard (either fund stVault or withdraw to the outer address)
| // set flag to false to prevent double withdrawal | ||
| validatorPredeposits[validatorId] = false; | ||
|
|
||
| (bool success, ) = _recipient.call{value: 1 ether}(""); |
There was a problem hiding this comment.
| stakingVault.depositToBeaconChain(deposits); | ||
| } | ||
|
|
||
| function proveValidatorWithdrawalCredentials( |
There was a problem hiding this comment.
should combine prove + deposit
Jeday
changed the title
TheDZhon
reviewed
Feb 12, 2025
| // Zero the remaining 16 bytes to form a 64‐byte block. | ||
| // (0x30 = 48, so mstore at 0x30 will zero 32 bytes covering addresses 48–79; | ||
| // only bytes 48–63 matter for our 64-byte input.) | ||
| mstore(0x30, 0) |
There was a problem hiding this comment.
it corrupts the free memory slot
contracts/0.8.25/lib/SSZ.sol
Outdated
| calldatacopy(0x00, pubkey.offset, 48) | ||
|
|
||
| // Zero the remaining 16 bytes to form a 64-byte input block | ||
| mstore(0x30, 0) |
There was a problem hiding this comment.
contracts/0.8.25/vaults/predeposit_guarantee/CLProofVerifier.sol
Outdated
Show resolved
Hide resolved
contracts/0.8.25/vaults/predeposit_guarantee/PredepositGuarantee.sol
Outdated
Show resolved
Hide resolved
contracts/0.8.25/vaults/predeposit_guarantee/PredepositGuarantee.sol
Outdated
Show resolved
Hide resolved
contracts/0.8.25/vaults/predeposit_guarantee/PredepositGuarantee.sol
Outdated
Show resolved
Hide resolved
failingtwice
commented
Feb 13, 2025
| * @custom:inOutDelta Net difference between ether funded and withdrawn from StakingVault | ||
| * @custom:nodeOperator Address of the node operator | ||
| * @custom:beaconChainDepositsPaused Whether beacon deposits are paused by the vault owner | ||
| * @custom: report Latest report containing valuation and inOutDelta |
There was a problem hiding this comment.
i think the space breaks the natspec custom tag directive
https://docs.soliditylang.org/en/latest/natspec-format.html#tags
failingtwice
commented
Feb 13, 2025
| */ | ||
| struct ERC7201Storage { | ||
| Report report; | ||
| uint128 locked; | ||
| int128 inOutDelta; | ||
| address nodeOperator; | ||
| address depositGuardian; |
There was a problem hiding this comment.
let's rename to trustedDepositor, or something
There was a problem hiding this comment.
just depositor is also ok, trusted kind of excessive
failingtwice
commented
Feb 13, 2025
| * @param _pubkey of validator that was proven invalid in PDG | ||
| * @param _recipient address to receive the `PREDEPOSIT_AMOUNT` | ||
| */ | ||
| function withdrawDisputedValidator(bytes calldata _pubkey, address _recipient) external { |
There was a problem hiding this comment.
Why is it called "disputed"? It can either be proven of disproven?
failingtwice
commented
Feb 13, 2025
| * Staking Vault does not have to be connected to Lido or any other system to be compatible with PDG | ||
| * but a reverse constraint can be AND are applied. | ||
| */ | ||
| contract PredepositGuarantee is CLProofVerifier, PausableUntilWithRoles { |
There was a problem hiding this comment.
Why do need to pause it?
There was a problem hiding this comment.
emeregency breaks by the dao
|
Hardhat Unit Tests Coverage SummaryDiff against masterResults for commit: b098888 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
DiRaiks
approved these changes
Mar 3, 2025
TheDZhon
reviewed
Mar 3, 2025
contracts/0.8.25/Accounting.sol
Outdated
contracts/0.8.25/lib/SSZ.sol
Outdated
Comment on lines
138
to
145
| let result := staticcall( | ||
| gas(), | ||
| 0x02, | ||
| 0x00, | ||
| 0x40, | ||
| 0x00, | ||
| 0x20 | ||
| ) |
There was a problem hiding this comment.
Suggested change
| let result := staticcall( | |
| gas(), | |
| 0x02, | |
| 0x00, | |
| 0x40, | |
| 0x00, | |
| 0x20 | |
| ) | |
| let result := staticcall( | |
| gas(), | |
| 0x02, /* precompile */ | |
| 0x00, /* input memory offset */ | |
| 0x40, /* input length */ | |
| 0x00, /* output memory offset */ | |
| 0x20 /* output length */ | |
| ) |
contracts/0.8.25/lib/SSZ.sol
Outdated
Comment on lines
133
to
136
| // Store elements to hash contiguously in scratch space. | ||
| // Scratch space is 64 bytes (0x00 - 0x3f) and both elements are 32 bytes. | ||
| mstore(scratch, leaf) | ||
| mstore(xor(scratch, 0x20), calldataload(offset)) |
There was a problem hiding this comment.
it's basically to store to 0x00 and 0x20 not using the if condition
| * @notice funds vault with ether of disproven validator from PDG | ||
| * @param _pubkey of validator that was proven invalid in PDG | ||
| */ | ||
| function refundDisputedValidatorToVault(bytes calldata _pubkey) external { |
There was a problem hiding this comment.
Suggested change
| function refundDisputedValidatorToVault(bytes calldata _pubkey) external { | |
| function refundDisprovenValidatorToVault(bytes calldata _pubkey) external { |
| @@ -75,6 +76,16 @@ abstract contract Permissions is AccessControlConfirmable { | |||
| */ | |||
| bytes32 public constant VOLUNTARY_DISCONNECT_ROLE = keccak256("vaults.Permissions.VoluntaryDisconnect"); | |||
|
|
|||
| /** | |||
| * @notice Permission for recover assets from Delegate contracts | |||
There was a problem hiding this comment.
the comment is misleading (copypasta)
| * Node operator address is set in the initialization and can never be changed. | ||
| */ | ||
| function nodeOperator() external view returns (address) { | ||
| return _getStorage().nodeOperator; | ||
| } | ||
|
|
||
| /** | ||
| * @notice Returns the address of the depositor | ||
| * Trusted party responsible for securely depositing validators to the beacon chain. |
There was a problem hiding this comment.
it's not clear what it is about (deposit front-run vuln)
| * In the context of this contract, the depositor performs deposits through `depositToBeaconChain()`. | ||
| * Depositor address is set in the initialization and can be changed by the owner with `setDepositor` | ||
| * only on the condition that the vault is not connected to the VaultHub. | ||
| * @return Address of the deposit guardian |
There was a problem hiding this comment.
Suggested change
| * @return Address of the deposit guardian | |
| * @return Address of the depositor |
| IStakingVault.Deposit calldata deposit = _deposits[i]; | ||
| Deposit calldata deposit = _deposits[i]; | ||
|
|
||
| //TODO: check BLS signature |
There was a problem hiding this comment.
Suggested change
| //TODO: check BLS signature |
| revert DepositorAlreadySet(); | ||
| } | ||
|
|
||
| VaultHub.VaultSocket memory socket = VaultHub(VAULT_HUB).vaultSocket(address(this)); |
There was a problem hiding this comment.
unfortunately, it's better to avoid using VaultHub here
| * @dev It can only be changed when vault is not connected to the VaultHub | ||
| * | ||
| */ | ||
| function setDepositor(address _depositor) external onlyOwner { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request introduces PredepositGuarantee contract that acts as deposit security layer for all Lido connected vaults.