[EPIC] stVaults #874
Draft
[EPIC] stVaults #874
Conversation
This file contains hidden or 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
There was a problem hiding this comment.
Slither found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Hardhat Unit Tests Coverage SummaryDiff against masterResults for commit: fb4f7e5 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
TheDZhon
reviewed
Nov 28, 2024
contracts/0.8.25/Accounting.sol
Outdated
| } | ||
|
|
||
| // Distribute protocol fee (treasury & node operators) | ||
| if (_update.sharesToMintAsFees > 0) { |
There was a problem hiding this comment.
this name might be misleading, should be named smth 'sharesToMintAsLidoV2ProtocolFees'
TheDZhon
reviewed
Nov 29, 2024
contracts/0.8.25/vaults/VaultHub.sol
Outdated
| import {AccessControlEnumerableUpgradeable} from "contracts/openzeppelin/5.0.2/upgradeable/access/extensions/AccessControlEnumerableUpgradeable.sol"; | ||
| import {IHubVault} from "./interfaces/IHubVault.sol"; | ||
| import {Math256} from "contracts/common/lib/Math256.sol"; | ||
| import {ILido as StETH} from "contracts/0.8.25/interfaces/ILido.sol"; |
There was a problem hiding this comment.
why this iface not in common?
There was a problem hiding this comment.
should revise iface structure in the future
contracts/0.8.25/vaults/VaultHub.sol
Outdated
|
|
||
| /// @notice disconnects a vault from the hub | ||
| /// @dev can be called by vaults only | ||
| function disconnectVault(address _vault) external { |
There was a problem hiding this comment.
let's add emergency flow
There was a problem hiding this comment.
let's consider the following cases:
- voluntary disconnect (this one)
- stop new minting
- eject and withdraw
- disconnect forcibly
There was a problem hiding this comment.
- disconnect and dust issue
contracts/0.8.25/vaults/VaultHub.sol
Outdated
| /// @param _reserveRatio minimum Reserve ratio in basis points | ||
| /// @param _reserveRatioThreshold reserve ratio that makes possible to force rebalance on the vault (in basis points) | ||
| /// @param _treasuryFeeBP treasury fee in basis points | ||
| function connectVault( |
There was a problem hiding this comment.
for permissionless addition:
- there could be a scenario when we adding a vault under slashing
folkyatina
reviewed
Nov 29, 2024
folkyatina
reviewed
Nov 29, 2024
folkyatina
reviewed
Nov 29, 2024
folkyatina
reviewed
Nov 29, 2024
contracts/0.8.25/vaults/VaultHub.sol
Outdated
| /// @notice force rebalance of the vault to have sufficient reserve ratio | ||
| /// @param _vault vault address | ||
| /// @dev can be used permissionlessly if the vault's min reserve ratio is broken | ||
| function forceRebalance(IHubVault _vault) external { |
There was a problem hiding this comment.
Embed auto-disconnect if resulting valuation is too small
folkyatina
reviewed
Nov 29, 2024
...acts/openzeppelin/5.0.2/upgradeable/access/extensions/AccessControlEnumerableUpgradeable.sol
Fixed
Show fixed
Hide fixed
...acts/openzeppelin/5.0.2/upgradeable/access/extensions/AccessControlEnumerableUpgradeable.sol
Fixed
Show fixed
Hide fixed
folkyatina
reviewed
Dec 3, 2024
folkyatina
reviewed
Dec 3, 2024
folkyatina
reviewed
Dec 3, 2024
TheDZhon
reviewed
Dec 4, 2024
contracts/0.8.25/vaults/VaultHub.sol
Outdated
|
|
||
| stETH.mintExternalShares(_recipient, sharesToMint); | ||
|
|
||
| emit MintedStETHOnVault(_vault, _tokens); |
There was a problem hiding this comment.
need to disentangle events for each core operation later 🧠
There was a problem hiding this comment.
backward compatibility is needed for Subgraph
<
F438
summary role="button" data-target="details-collapsible.summaryElement details-toggle.summaryTarget" data-action="click:details-collapsible#toggle click:details-toggle#toggle" data-aria-label-closed="Expand comment" data-aria-label-open="Collapse comment" aria-expanded="false" aria-label="Collapse comment" data-view-component="true" class="js-toggle-outdated-comments py-2 px-3 rounded-2 color-bg-subtle">
Jeday
reviewed
Dec 5, 2024
Jeday
reviewed
Dec 5, 2024
Jeday
reviewed
Dec 5, 2024
TheDZhon
reviewed
Dec 5, 2024
contracts/0.8.25/vaults/VaultHub.sol
Outdated
| // Lido curated validators could earn if vault's ETH was staked in Lido | ||
| // itself and minted as stETH shares | ||
| // | ||
| // treasuryFeeShares = value * lidoGrossAPR * treasuryFeeRate / preShareRate |
There was a problem hiding this comment.
Suggested change
| // treasuryFeeShares = value * lidoGrossAPR * treasuryFeeRate / preShareRate | |
| // treasuryFeeShares = chargeableValue * lidoGrossAPR * treasuryFeeRate / (_preTotalPooledEther / _preTotalShares) |
contracts/0.8.25/vaults/VaultHub.sol
Outdated
| } | ||
| } | ||
|
|
||
| function _calculateLidoFees( |
There was a problem hiding this comment.
// @dev impossible to invoke this method under negative rebase
contracts/0.8.25/vaults/VaultHub.sol
Outdated
| // | ||
| // treasuryFeeShares = value * lidoGrossAPR * treasuryFeeRate / preShareRate | ||
| // lidoGrossAPR = postShareRateWithoutFees / preShareRate - 1 | ||
| // = value * (postShareRateWithoutFees / preShareRate - 1) * treasuryFeeRate / preShareRate |
There was a problem hiding this comment.
Suggested change
| // = value * (postShareRateWithoutFees / preShareRate - 1) * treasuryFeeRate / preShareRate | |
| // treasuryFeeShares = value * (postShareRateWithoutFees / preShareRate - 1) * treasuryFeeRate / preShareRate |
contracts/0.8.25/vaults/VaultHub.sol
Outdated
| chargeableValue); | ||
| uint256 treasuryFee = (potentialRewards * _socket.treasuryFeeBP) / BPS_BASE; | ||
|
|
||
| treasuryFeeShares = (treasuryFee * _preTotalShares) / _preTotalPooledEther; |
| if (msg.value == 0) revert ZeroArgument("msg.value"); | ||
|
|
||
| VaultStorage storage $ = _getVaultStorage(); | ||
| $.inOutDelta += SafeCast.toInt128(int256(msg.value)); |
| VaultStorage storage $ = _getVaultStorage(); | ||
| $.inOutDelta -= SafeCast.toInt128(int256(_ether)); | ||
|
|
||
| (bool success, ) = _recipient.call{value: _ether}(""); |
| * @param _numberOfDeposits Number of 32 ETH deposits to make | ||
| * @param _pubkeys Validator public keys | ||
| * @param _signatures Validator signatures | ||
| * @dev Ensures vault is healthy and handles deposit logistics |
There was a problem hiding this comment.
Suggested change
| * @dev Ensures vault is healthy and handles deposit logistics | |
| * @dev Ensures vault is balanced and handles deposit logistics |
| * @param _validatorPublicKey Public key of validator to exit | ||
| */ | ||
| function requestValidatorExit(bytes calldata _validatorPublicKey) external onlyOwner { | ||
| emit ValidatorsExitRequest(msg.sender, _validatorPublicKey); |
There was a problem hiding this comment.
maybe make it compatible with ValidatorsExitBus and an ejector service
| VaultStorage storage $ = _getVaultStorage(); | ||
| $.inOutDelta -= SafeCast.toInt128(int256(_ether)); | ||
|
|
||
| emit Withdrawn(msg.sender, msg.sender, _ether); |
There was a problem hiding this comment.
Suggested change
| emit Withdrawn(msg.sender, msg.sender, _ether); | |
| emit Withdrawn(msg.sender, VAULT_HUB, _ether); |
TheDZhon
reviewed
Dec 11, 2024
| return address(VAULT_HUB); | ||
| } | ||
|
|
||
| receive() external payable { |
There was a problem hiding this comment.
This func accepts ether without updating valuation (and inOutDelta) and it's not possible to withdraw it back.
The more important thing is that while calling rebalance() it gets used to reduce inOutDelta allowing to underflow valuation()
| $.locked = SafeCast.toUint128(_locked); | ||
|
|
||
| try IReportReceiver(owner()).onReport(_valuation, _inOutDelta, _locked) {} catch (bytes memory reason) { | ||
| emit OnReportFailed(address(this), reason); |
|
|
||
| event Funded(address indexed sender, uint256 amount); | ||
| event Withdrawn(address indexed sender, address indexed recipient, uint256 amount); | ||
| event DepositedToBeaconChain(address indexed sender, uint256 deposits, uint256 amount); |
There was a problem hiding this comment.
maybe should target pectra (array of amounts)
Comment on lines
357
to
359
| event Locked(uint256 locked); | ||
| event Reported(address indexed vault, uint256 valuation, int256 inOutDelta, uint256 locked); | ||
| event OnReportFailed(address vault, bytes reason); |
There was a problem hiding this comment.
here have three different approaches:
- do not include the emitter contract (and it's fine indeed)
- include the address but indexed
- include non-indexed addr
let's use the first approach 🧠
| /** | ||
| * @dev Modifier to fund the staking vault if msg.value > 0 | ||
| */ | ||
| modifier fundAndProceed() { |
There was a problem hiding this comment.
Suggested change
| modifier fundAndProceed() { | |
| modifier prefundable() { |
| * @dev Transfers ownership of the staking vault to a new owner | ||
| * @param _newOwner Address of the new owner | 17AE||
| */ | ||
| function _transferStVaultOwnership(address _newOwner) internal { |
There was a problem hiding this comment.
see the rename proposal above
| * @dev Requests the exit of a validator from the staking vault | ||
| * @param _validatorPublicKey Public key of the validator to exit | ||
| */ | ||
| function _requestValidatorExit(bytes calldata _validatorPublicKey) internal { |
|
|
||
| // ==================== Errors ==================== | ||
|
|
||
| /// @notice Error for zero address arguments |
There was a problem hiding this comment.
Suggested change
| /// @notice Error for zero address arguments | |
| /// @notice Error for zero value arguments |
| /// @notice Error when the withdrawable amount is insufficient. | ||
| /// @param withdrawable The amount that is withdrawable | ||
| /// @param requested The amount requested to withdraw | ||
| error InsufficientWithdrawableAmount(uint256 withdrawable, uint256 requested); |
There was a problem hiding this comment.
should have a view func to show withdrawable amount
for rebalance we also need a view to show what would happen
There was a problem hiding this comment.
this error should be moved to Delegation not being used here at all (it seems)
TheDZhon
reviewed
Dec 12, 2024
| // ==================== Constants ==================== | ||
|
|
||
| uint256 private constant BP_BASE = 10000; // Basis points base (100%) | ||
| uint256 private constant MAX_FEE = BP_BASE; // Maximum fee in basis points (100%) |
Comment on lines
43
to
44
| * - vote on ownership transfer | ||
| * - vote on performance fee changes |
There was a problem hiding this comment.
maybe veto instead of approval (ET dynamics)
later
| * @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 |
| * Lido DAO can: | ||
| * - set the operator role | ||
| * - vote on ownership transfer | ||
| */ |
There was a problem hiding this comment.
what about VaultHub.connect?
There was a problem hiding this comment.
another issue: vault should express a WILL to connect (two-step)
There was a problem hiding this comment.
these comments might be not relevant to Delegation
| * Key master can: | ||
| * - deposit validators to the beacon chain | ||
| */ | ||
| bytes32 public constant KEY_MASTER_ROLE = keccak256("Vault.Delegation.KeyMasterRole"); |
There was a problem hiding this comment.
BEACON_CHAIN_DEPOSITOR_ROLE?
| function onReport(uint256 _valuation, int256 _inOutDelta, uint256 _locked) external { | ||
| if (msg.sender != address(stakingVault)) revert OnlyStVaultCanCallOnReportHook(); | ||
|
|
||
| managementDue += (_valuation * managementFee) / 365 / BP_BASE; |
There was a problem hiding this comment.
timeElapsed (in case of missing reports e.g.)
Comment on lines
387
to
388
| int256 unlocked = int256(stakingVault.valuation()) - int256(stakingVault.locked()); | ||
| uint256 unreserved = unlocked >= 0 ? uint256(unlocked) : 0; |
There was a problem hiding this comment.
can query stakingVault.unlocked() instead
| modifier onlyIfVotedBy(bytes32[] memory _committee, uint256 _votingPeriod) { | ||
| bytes32 callId = keccak256(msg.data); | ||
| uint256 committeeSize = _committee.length; | ||
| uint256 votingStart = block.timestamp - _votingPeriod; |
There was a problem hiding this comment.
Suggested change
| uint256 votingStart = block.timestamp - _votingPeriod; | |
| uint256 votesVaildSince = block.timestamp - _votingPeriod; |
| // ==================== Events ==================== | ||
|
|
||
| /// @notice Emitted when a role member votes on a function requiring committee approval. | ||
| event RoleMemberVoted(address member, bytes32 role, uint256 timestamp, bytes data); |
| /// @notice Emitted when a role member votes on a function requiring committee approval. | ||
| event RoleMemberVoted(address member, bytes32 role, uint256 timestamp, bytes data); | ||
|
|
||
| // ==================== Errors ==================== |
There was a problem hiding this comment.
fees not covered with event changes
TheDZhon
reviewed
Dec 13, 2024
Comment on lines
14
to
15
| uint256 managementFee; | ||
| uint256 performanceFee; |
There was a problem hiding this comment.
Suggested change
| uint256 managementFee; | |
| uint256 performanceFee; | |
| uint256 managementFeeBP; | |
| uint256 performanceFeeBP; |
|
|
||
| pragma solidity 0.8.25; | ||
|
|
||
| interface IDelegation { |
There was a problem hiding this comment.
why we don't import this interface though?
There was a problem hiding this comment.
let's add the comment that it's intended (to hook up the particular interface strictly)
| } | ||
|
|
||
| contract VaultFactory is UpgradeableBeacon { | ||
| address public immutable delegationImpl; |
There was a problem hiding this comment.
Suggested change
| address public immutable delegationImpl; | |
| address public immutable DELEGATION_IMPL; |
There was a problem hiding this comment.
should think about disentangling the beacon and factory (from Azat)
There was a problem hiding this comment.
@loga4 (the problem is that it's impossible to upgrade the delegation for the factory)
| delegation.grantRole(delegation.LIDO_DAO_ROLE(), _lidoAgent); | ||
| delegation.grantRole(delegation.MANAGER_ROLE(), _initializationParams.manager); | ||
| delegation.grantRole(delegation.OPERATOR_ROLE(), _initializationParams.operator); | ||
| delegation.grantRole(delegation.DEFAULT_ADMIN_ROLE(), msg.sender); |
There was a problem hiding this comment.
scary though, let's add a comment that those roles above have their admins
There was a problem hiding this comment.
msg.sender and use input parameters instead to make the deployments automated without pain (suggested defaultAdmin above to replace msg.sender)
|
|
||
| delegation.initialize(address(this), address(vault)); | ||
|
|
||
| delegation.grantRole(delegation.LIDO_DAO_ROLE(), _lidoAgent); |
There was a problem hiding this comment.
maybe LIDO_AGENT should be immutable for the factory
| /// @notice Creates a new StakingVault and Delegation contracts | ||
| /// @param _stakingVaultParams The params of vault initialization | ||
| /// @param _initializationParams The params of vault initialization | ||
| function createVault( |
There was a problem hiding this comment.
Suggested change
| function createVault( | |
| function createVaultWithDelegation( |
| * @notice The term "fee" is used to express the fee percentage as basis points, e.g. 5%, | ||
| * while "due" is the actual amount of the fee, e.g. 1 ether | ||
| */ | ||
| contract Delegation is Dashboard, IReportReceiver { |
There was a problem hiding this comment.
Suggested change
| contract Delegation is Dashboard, IReportReceiver { | |
| contract Delegation is Dashboard, IReportReceiver, AssetRecoverer { |
|
|
||
| emit VaultCreated(address(delegation), address(vault)); | ||
| emit DelegationCreated(msg.sender, address(delegation)); | ||
| } |
There was a problem hiding this comment.
🧠 role setup has dependencies on the Delegation internals which is not perfect
feat(V3Template): check start and finish in same tx (transient storage)
feat: move Accounting to 0.8.9 and refactor interfaces
Comment on lines
+240
to
+268
| function _calculateLidoProtocolFeeShares( | ||
| ReportValues calldata _report, | ||
| CalculatedValues memory _update, | ||
| uint256 _internalSharesBeforeFees, | ||
| uint256 _internalEther | ||
| ) internal pure returns (uint256 sharesToMintAsFees) { | ||
| // we are calculating the share rate equal to the post-rebase share rate | ||
| // but with fees taken as ether deduction instead of minting shares | ||
| // to learn the amount of shares we need to mint to compensate for this fee | ||
|
|
||
| uint256 unifiedClBalance = _report.clBalance + _update.withdrawals; | ||
| // Don't mint/distribute any protocol fee on the non-profitable Lido oracle report | ||
| // (when consensus layer balance delta is zero or negative). | ||
| // See LIP-12 for details: | ||
| // https://research.lido.fi/t/lip-12-on-chain-part-of-the-rewards-distribution-after-the-merge/1625 | ||
| if (unifiedClBalance > _update.principalClBalance) { | ||
| uint256 totalRewards = unifiedClBalance - _update.principalClBalance + _update.elRewards; | ||
| uint256 totalFee = _update.rewardDistribution.totalFee; | ||
| uint256 precision = _update.rewardDistribution.precisionPoints; | ||
| // amount of fees in ether | ||
| uint256 feeEther = (totalRewards * totalFee) / precision; | ||
| // but we won't pay fees in ether, so we need to calculate how many shares we need to mint as fees | ||
| // using the share rate that takes fees into account | ||
| // the share rate is the same as the post-rebase share rate | ||
| // but with fees taken as ether deduction instead of minting shares | ||
| // to learn the amount of shares we need to mint to compensate for this fee | ||
| sharesToMintAsFees = (feeEther * _internalSharesBeforeFees) / (_internalEther - feeEther); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Divide before multiply Medium
| ) internal { | ||
| _checkAccountingOracleReport(_contracts, _report, _pre, _update); | ||
|
|
||
| uint256 lastWithdrawalRequestToFinalize; |
Check warning
Code scanning / Slither
Uninitialized local variables Medium
Feat/rebase on tw e2e
fix: failing integration tests
feat: testnet-2
test: complete Burner.sol coverage
# Conflicts: # test/0.8.25/vaults/vaulthub/contracts/VaultFactory__MockForVaultHub.sol
tests: restore unit tests for vaults
# Conflicts: # test/0.8.25/vaults/vaultFactory.test.ts
fix: added og/factory/beacon
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.
❗ 🚧 Under construction 🚧 ❗
Staking Vaults
New way of isolated staking, through separate vaults, with optional stETH liquidity