[VAULTS][POC] a very raw bug-ridden proof-of-concept for PredepositGuardian #932
+264
−39
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}
Comment on lines
71
to
93
| function withdrawAsVaultOwner(address stakingVault, bytes[] calldata validatorPubkeys) external { | ||
| if (msg.sender != StakingVault(payable(stakingVault)).owner()) revert MustBeVaultOwner(); | ||
|
|
||
| for (uint256 i = 0; i < validatorPubkeys.length; i++) { | ||
| bytes32 validatorPubkeyHash = keccak256(validatorPubkeys[i]); | ||
|
|
||
| if (validatorStatuses[validatorPubkeyHash] != ValidatorStatus.RESOLVED) { | ||
| revert MustBeResolvedValidatorPubkey(); | ||
| } | ||
|
|
||
| if (validatorStatuses[validatorPubkeyHash] == ValidatorStatus.WITHDRAWN) { | ||
| revert ValidatorAlreadyWithdrawn(); | ||
| } | ||
|
|
||
| if (wcRecords[validatorPubkeyHash] == StakingVault(payable(stakingVault)).withdrawalCredentials()) { | ||
| revert ValidatorWithdrawalCredentialsMatchVaultWithdrawalCredentials(); | ||
| } | ||
|
|
||
| msg.sender.call{value: 1 ether}(""); | ||
|
|
||
| validatorStatuses[validatorPubkeyHash] = ValidatorStatus.WITHDRAWN; | ||
| } | ||
| } |
Check failure
Code scanning / Slither
Reentrancy vulnerabilities High
Comment on lines
71
to
93
| function withdrawAsVaultOwner(address stakingVault, bytes[] calldata validatorPubkeys) external { | ||
| if (msg.sender != StakingVault(payable(stakingVault)).owner()) revert MustBeVaultOwner(); | ||
|
|
||
| for (uint256 i = 0; i < validatorPubkeys.length; i++) { | ||
| bytes32 validatorPubkeyHash = keccak256(validatorPubkeys[i]); | ||
|
|
||
| if (validatorStatuses[validatorPubkeyHash] != ValidatorStatus.RESOLVED) { | ||
| revert MustBeResolvedValidatorPubkey(); | ||
| } | ||
|
|
||
| if (validatorStatuses[validatorPubkeyHash] == ValidatorStatus.WITHDRAWN) { | ||
| revert ValidatorAlreadyWithdrawn(); | ||
| } | ||
|
|
||
| if (wcRecords[validatorPubkeyHash] == StakingVault(payable(stakingVault)).withdrawalCredentials()) { | ||
| revert ValidatorWithdrawalCredentialsMatchVaultWithdrawalCredentials(); | ||
| } | ||
|
|
||
| msg.sender.call{value: 1 ether}(""); | ||
|
|
||
| validatorStatuses[validatorPubkeyHash] = ValidatorStatus.WITHDRAWN; | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unchecked low-level calls Medium
Comment on lines
95
to
115
| function withdrawAsNodeOperator(bytes[] calldata validatorPubkeys) external { | ||
| for (uint256 i = 0; i < validatorPubkeys.length; i++) { | ||
| bytes32 validatorPubkeyHash = keccak256(validatorPubkeys[i]); | ||
|
|
||
| if (validatorStatuses[validatorPubkeyHash] != ValidatorStatus.RESOLVED) { | ||
| revert MustBeResolvedValidatorPubkey(); | ||
| } | ||
|
|
||
| if (validatorStatuses[validatorPubkeyHash] == ValidatorStatus.WITHDRAWN) { | ||
| revert ValidatorAlreadyWithdrawn(); | ||
| } | ||
|
|
||
| if (nodeOperatorToValidators[msg.sender] != validatorPubkeyHash) { | ||
| revert ValidatorMustBelongToSender(); | ||
| } | ||
|
|
||
| msg.sender.call{value: 1 ether}(""); | ||
|
|
||
| validatorStatuses[validatorPubkeyHash] = ValidatorStatus.WITHDRAWN; | ||
| } | ||
| } |
Check failure
Code scanning / Slither
Reentrancy vulnerabilities High
Comment on lines
95
to
115
| function withdrawAsNodeOperator(bytes[] calldata validatorPubkeys) external { | ||
| for (uint256 i = 0; i < validatorPubkeys.length; i++) { | ||
| bytes32 validatorPubkeyHash = keccak256(validatorPubkeys[i]); | ||
|
|
||
| if (validatorStatuses[validatorPubkeyHash] != ValidatorStatus.RESOLVED) { | ||
| revert MustBeResolvedValidatorPubkey(); | ||
| } | ||
|
|
||
| if (validatorStatuses[validatorPubkeyHash] == ValidatorStatus.WITHDRAWN) { | ||
| revert ValidatorAlreadyWithdrawn(); | ||
| } | ||
|
|
||
| if (nodeOperatorToValidators[msg.sender] != validatorPubkeyHash) { | ||
| revert ValidatorMustBelongToSender(); | ||
| } | ||
|
|
||
| msg.sender.call{value: 1 ether}(""); | ||
|
|
||
| validatorStatuses[validatorPubkeyHash] = ValidatorStatus.WITHDRAWN; | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unchecked low-level calls Medium
TheDZhon
reviewed
Jan 30, 2025
| bytes32 _expectedGlobalDepositRoot, | ||
| bytes calldata _signature | ||
| ) external { | ||
| function depositToBeaconChain(Deposit[] calldata _deposits) external { |
There was a problem hiding this comment.
strawman sanity checks would be needed for the input data
| @@ -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
|
|
||
| // 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.
| for (uint256 i = 0; i < validatorsLength; i++) { | ||
| bytes32 validatorId = _validatorIds[i]; | ||
|
|
||
| if (msg.sender != _stakingVault.nodeOperator()) { |
| stakingVault.depositToBeaconChain(deposits); | ||
| } | ||
|
|
||
| function proveValidatorWithdrawalCredentials( |
There was a problem hiding this comment.
should combine prove + deposit
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 IS A POC, NOT READY FOR PRODUCTION, PRACTICALLY PSEUDOCODE