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.

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

[Vaults] Immutably fix the node operator in the basic StakingVault #897

Open
wants to merge 40 commits into
base: feat/vaults
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
7e7edee
fix: update stvault interface
mymphe Dec 3, 2024
2de4da5
fix: check balance before unlocked
mymphe Dec 3, 2024
1bcd4fd
fix: eoa owner should not revert
mymphe Dec 4, 2024
9030b5c
fix: check before sstore
mymphe Dec 5, 2024
03a84a8
fix: use precise error for locking
mymphe Dec 5, 2024
1395555
fix: set withdraw recipient to vaulthub on rebalance
mymphe Dec 6, 2024
751c77e
fix: update action name for error
mymphe Dec 6, 2024
17b88bf
fix: handle report hook for different account types
mymphe Dec 6, 2024
a50851f
chore: bump hh
mymphe Dec 6, 2024
91d432e
test: staking vault full coverage*
mymphe Dec 6, 2024
212ec13
fix: remove safecast
mymphe Dec 9, 2024
50b04f6
fix: some vault renaming and cleanup
mymphe Dec 9, 2024
9faf18a
chore: add q about recovery
mymphe Dec 10, 2024
06340ca
test: dashboard
mymphe Dec 10, 2024
20d7db8
fix: consistent naming
mymphe Dec 10, 2024
1a843a9
fix: remove only
mymphe Dec 11, 2024
7a7e622
test: delegation tests (wip)
mymphe Dec 11, 2024
e137105
fix: rename beacon chain depositor to logistics
mymphe Dec 12, 2024
cfc013b
feat: fix operator in the vault
mymphe Dec 13, 2024
5185b04
fix: catch run out of gas error in report hook
mymphe Dec 16, 2024
ce7ccb8
Merge branch 'feat/vaults' into feat-immutable-operator-in-vault
tamtamchik Dec 17, 2024
496e6f2
test: fix .only in tests
tamtamchik Dec 17, 2024
79653dd
ci: use hardhat 2.22.17
tamtamchik Dec 17, 2024
bb43b2e
fix: ensure rebalance amount doesn't exceed valuation
mymphe Dec 18, 2024
afcb7d3
Merge branch 'feat-immutable-operator-in-vault' of https://github.com…
mymphe Dec 18, 2024
1463ad3
fix: update eip7201 location
mymphe Dec 18, 2024
732dbf4
feat: update comments
mymphe Dec 19, 2024
6256c16
fix: revert report if onReport ran out of gas
mymphe Dec 19, 2024
34261a9
Merge branch 'feat/vaults' of https://github.com/lidofinance/core int…
mymphe Dec 20, 2024
bc86331
feat: use Ownable interface for owner()
mymphe Dec 20, 2024
21425d0
fix: dashboard tests
mymphe Dec 20, 2024
7899756
fix: use consistent naming for bp
mymphe Dec 20, 2024
8d7a6af
fix: sort imports
mymphe Dec 20, 2024
2f0ec60
fix: add comment for codesize check
mymphe Dec 20, 2024
d9888f0
fix: take operator from vault
mymphe Dec 20, 2024
c34ebe1
test(integration): fix and update vaults happy path
tamtamchik Dec 20, 2024
4897271
test: disable negative rebase integration test
tamtamchik Dec 20, 2024
d0b1d4c
fix: remove onReport hook 👏
mymphe Dec 20, 2024
f7086b5
Merge branch 'feat-immutable-operator-in-vault' of https://github.com…
mymphe Dec 20, 2024
887fec3
fix: enable all the tests
tamtamchik Dec 20, 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
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// See contracts/COMPILERS.md
pragma solidity 0.8.25;

import {MemUtils} from "../../common/lib/MemUtils.sol";
import {MemUtils} from "contracts/common/lib/MemUtils.sol";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what the process is here, do we need to import our contracts always under "contracts/" namespace or use a relative path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think imports relative to root are much more readable. You can clearly see where the file is imported from


interface IDepositContract {
function get_deposit_root() external view returns (bytes32 rootHash);
Expand All @@ -26,7 +26,7 @@ interface IDepositContract {
*
* This contract will be refactored to support custom deposit amounts for MAX_EB.
*/
contract VaultBeaconChainDepositor {
contract BeaconChainDepositLogistics {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this makes the naming better, tbh

uint256 internal constant PUBLIC_KEY_LENGTH = 48;
uint256 internal constant SIGNATURE_LENGTH = 96;
uint256 internal constant DEPOSIT_SIZE = 32 ether;
Expand Down
28 changes: 5 additions & 23 deletions contracts/0.8.25/vaults/Dashboard.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {VaultHub} from "./VaultHub.sol";
* in this single contract. It provides administrative functions for managing the staking vault,
* including funding, withdrawing, depositing to the beacon chain, minting, burning, and rebalancing operations.
* All these functions are only callable by the account with the DEFAULT_ADMIN_ROLE.
* TODO: need to add recover methods for ERC20, probably in a separate contract
*/
contract Dashboard is AccessControlEnumerable {
/// @notice Address of the implementation contract
Expand Down Expand Up @@ -48,30 +49,25 @@ contract Dashboard is AccessControlEnumerable {

/**
* @notice Initializes the contract with the default admin and `StakingVault` address.
* @param _defaultAdmin Address to be granted the `DEFAULT_ADMIN_ROLE`, i.e. the actual owner of the stVault
* @param _stakingVault Address of the `StakingVault` contract.
*/
function initialize(address _defaultAdmin, address _stakingVault) external virtual {
_initialize(_defaultAdmin, _stakingVault);
function initialize(address _stakingVault) external virtual {
_initialize(_stakingVault);
}

/**
* @dev Internal initialize function.
* @param _defaultAdmin Address to be granted the `DEFAULT_ADMIN_ROLE`
* @param _stakingVault Address of the `StakingVault` contract.
*/
function _initialize(address _defaultAdmin, address _stakingVault) internal {
if (_defaultAdmin == address(0)) revert ZeroArgument("_defaultAdmin");
function _initialize(address _stakingVault) internal {
if (_stakingVault == address(0)) revert ZeroArgument("_stakingVault");
if (isInitialized) revert AlreadyInitialized();
if (address(this) == _SELF) revert NonProxyCallsForbidden();

isInitialized = true;

_grantRole(DEFAULT_ADMIN_ROLE, _defaultAdmin);

stakingVault = IStakingVault(_stakingVault);
vaultHub = VaultHub(stakingVault.vaultHub());
_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);

emit Initialized();
}
Expand Down Expand Up @@ -167,20 +163,6 @@ contract Dashboard is AccessControlEnumerable {
_requestValidatorExit(_validatorPublicKey);
}

/**
* @notice Deposits validators to the beacon chain
* @param _numberOfDeposits Number of validator deposits
* @param _pubkeys Concatenated public keys of the validators
* @param _signatures Concatenated signatures of the validators
*/
function depositToBeaconChain(
uint256 _numberOfDeposits,
bytes calldata _pubkeys,
bytes calldata _signatures
) external virtual onlyRole(DEFAULT_ADMIN_ROLE) {
_depositToBeaconChain(_numberOfDeposits, _pubkeys, _signatures);
}

/**
* @notice Mints stETH tokens backed by the vault to a recipient.
* @param _recipient Address of the recipient
Expand Down
88 changes: 18 additions & 70 deletions contracts/0.8.25/vaults/Delegation.sol
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we have 3 different role hierarchy: independent operator in vault, operatir in delegation and default_admin in delegation.

But looks like it should be only two of them:
default_admin and operator(that is the same address that is in the basic vault)

Original file line number Diff line number Diff line change
Expand Up @@ -53,23 +53,15 @@ contract Delegation is Dashboard, IReportReceiver {
*/
bytes32 public constant STAKER_ROLE = keccak256("Vault.Delegation.StakerRole");

/**
* @notice Role for the operator
* Operator can:
/**
* @notice Role for the node operator
* Node operator rewards claimer can:
mymphe marked this conversation as resolved.
Show resolved Hide resolved
* - claim the performance due
* - vote on performance fee changes
* - vote on ownership transfer
* - set the Key Master role
*/
bytes32 public constant OPERATOR_ROLE = keccak256("Vault.Delegation.OperatorRole");

/**
* @notice Role for the key master.
* Key master can:
* - deposit validators to the beacon chain
*/
bytes32 public constant KEY_MASTER_ROLE = keccak256("Vault.Delegation.KeyMasterRole");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be preliminary to get rid of this role.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Keymaster does not have any duties in the delegation contract, since they use the base vault for depositing validator keys.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still can be a useful to have a role that can only deposit, but have no access to fees or role setup.


/**
* @notice Role for the token master.
* Token master can:
Expand All @@ -78,15 +70,6 @@ contract Delegation is Dashboard, IReportReceiver {
*/
bytes32 public constant TOKEN_MASTER_ROLE = keccak256("Vault.Delegation.TokenMasterRole");

/**
* @notice Role for the Lido DAO.
* This can be the Lido DAO agent, EasyTrack or any other DAO decision-making system.
* Lido DAO can:
* - set the operator role
* - vote on ownership transfer
*/
bytes32 public constant LIDO_DAO_ROLE = keccak256("Vault.Delegation.LidoDAORole");

// ==================== State Variables ====================

/// @notice The last report for which the performance due was claimed
Expand Down Expand Up @@ -121,36 +104,16 @@ contract Delegation is Dashboard, IReportReceiver {
/**
* @notice Initializes the contract with the default admin and `StakingVault` address.
* Sets up roles and role administrators.
* @param _defaultAdmin Address to be granted the `DEFAULT_ADMIN_ROLE`.
* @param _stakingVault Address of the `StakingVault` contract.
* @dev This function is called by the `VaultFactory` contract
*/
function initialize(address _defaultAdmin, address _stakingVault) external override {
_initialize(_defaultAdmin, _stakingVault);

/**
* Granting `LIDO_DAO_ROLE` to the default admin is needed to set the initial Lido DAO address
* in the `createVault` function in the vault factory, so that we don't have to pass it
* to this initialize function and break the inherited function signature.
* This role will be revoked in the `createVault` function in the vault factory and
* will only remain on the Lido DAO address
*/
_grantRole(LIDO_DAO_ROLE, _defaultAdmin);

/**
* Only Lido DAO can assign the Lido DAO role.
*/
_setRoleAdmin(LIDO_DAO_ROLE, LIDO_DAO_ROLE);

/**
* The node operator in the vault must be approved by Lido DAO.
* The vault owner (`DEFAULT_ADMIN_ROLE`) cannot change the node operator.
*/
_setRoleAdmin(OPERATOR_ROLE, LIDO_DAO_ROLE);

/**
* The operator role can change the key master role.
*/
_setRoleAdmin(KEY_MASTER_ROLE, OPERATOR_ROLE);
function initialize(address _stakingVault) external override {
_initialize(_stakingVault);

// `OPERATOR_ROLE` is set to `msg.sender` to allow the `VaultFactory` to set the initial operator fee
// the role will be revoked from `VaultFactory`
_grantRole(OPERATOR_ROLE, msg.sender);
_setRoleAdmin(OPERATOR_ROLE, OPERATOR_ROLE);
}

// ==================== View Functions ====================
Expand All @@ -163,13 +126,13 @@ contract Delegation is Dashboard, IReportReceiver {
function withdrawable() public view returns (uint256) {
// Question: shouldn't we reserve both locked + dues, not max(locked, dues)?
uint256 reserved = Math256.max(stakingVault.locked(), managementDue + performanceDue());
uint256 value = stakingVault.valuation();
uint256 valuation = stakingVault.valuation();

if (reserved > value) {
if (reserved > valuation) {
return 0;
}

return value - reserved;
return valuation - reserved;
}

/**
Expand All @@ -194,10 +157,9 @@ contract Delegation is Dashboard, IReportReceiver {
* @return An array of role identifiers.
*/
function ownershipTransferCommittee() public pure returns (bytes32[] memory) {
bytes32[] memory roles = new bytes32[](3);
bytes32[] memory roles = new bytes32[](2);
roles[0] = MANAGER_ROLE;
roles[1] = OPERATOR_ROLE;
roles[2] = LIDO_DAO_ROLE;
return roles;
}

Expand Down Expand Up @@ -240,7 +202,7 @@ contract Delegation is Dashboard, IReportReceiver {
*/
function claimManagementDue(address _recipient, bool _liquid) external onlyRole(MANAGER_ROLE) {
if (_recipient == address(0)) revert ZeroArgument("_recipient");
if (!stakingVault.isHealthy()) revert VaultNotHealthy();
if (!stakingVault.isBalanced()) revert VaultUnbalanced();
tamtamchik marked this conversation as resolved.
Show resolved Hide resolved

uint256 due = managementDue;

Expand Down Expand Up @@ -298,20 +260,6 @@ contract Delegation is Dashboard, IReportReceiver {
_withdraw(_recipient, _ether);
}

/**
* @notice Deposits validators to the beacon chain.
* @param _numberOfDeposits Number of validator deposits.
* @param _pubkeys Concatenated public keys of the validators.
* @param _signatures Concatenated signatures of the validators.
*/
function depositToBeaconChain(
uint256 _numberOfDeposits,
bytes calldata _pubkeys,
bytes calldata _signatures
) external override onlyRole(KEY_MASTER_ROLE) {
_depositToBeaconChain(_numberOfDeposits, _pubkeys, _signatures);
}

/**
* @notice Claims the performance fee due.
* @param _recipient Address of the recipient.
Expand Down Expand Up @@ -491,8 +439,8 @@ contract Delegation is Dashboard, IReportReceiver {
/// @param requested The amount requested to withdraw.
error InsufficientUnlockedAmount(uint256 unlocked, uint256 requested);

/// @notice Error when the vault is not healthy.
error VaultNotHealthy();
/// @notice Error when the vault is not balanced.
error VaultUnbalanced();
tamtamchik marked this conversation as resolved.
Show resolved Hide resolved

/// @notice Hook can only be called by the staking vault.
error OnlyStVaultCanCallOnReportHook();
Expand Down
Loading
Loading