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

feat: stVaults #874

Draft
wants to merge 786 commits into
base: develop
Choose a base branch
from
Draft

feat: stVaults #874

wants to merge 786 commits into from

Conversation

tamtamchik
Copy link
Member

@tamtamchik tamtamchik commented Nov 17, 2024

❗ 🚧 Under construction 🚧 ❗

Staking Vaults

New way of isolated staking, through separate vaults, with optional stETH liquidity

  • Support minting external backed stETH in Lido contract
  • Extract protocol accounting to separate contract (two-step, better precision)
  • StakingVault + VaultHub
  • Dashboard and Delegation for Vaults
  • VaultFactory

@folkyatina folkyatina changed the base branch from master to develop November 27, 2024 07:48
@folkyatina folkyatina changed the base branch from develop to master November 27, 2024 07:48
@TheDZhon TheDZhon self-requested a review November 27, 2024 08:05
Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

πŸ‘€

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Slither found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link

github-actions bot commented Nov 27, 2024

badge

Hardhat Unit Tests Coverage Summary

Filename                                                       Stmts    Miss  Cover    Missing
-----------------------------------------------------------  -------  ------  -------  -----------------------------------------------------------------------------------
contracts/0.4.24/Lido.sol                                        201       6  97.01%   741, 746, 787-789, 946-947
contracts/0.4.24/StETH.sol                                        79       0  100.00%
contracts/0.4.24/StETHPermit.sol                                  15       0  100.00%
contracts/0.4.24/lib/Packed64x4.sol                                5       0  100.00%
contracts/0.4.24/lib/SigningKeys.sol                              36       0  100.00%
contracts/0.4.24/lib/StakeLimitUtils.sol                          37       0  100.00%
contracts/0.4.24/nos/NodeOperatorsRegistry.sol                   512       0  100.00%
contracts/0.4.24/oracle/LegacyOracle.sol                          72       0  100.00%
contracts/0.4.24/utils/Pausable.sol                                9       0  100.00%
contracts/0.4.24/utils/Versioned.sol                               5       0  100.00%
contracts/0.6.12/WstETH.sol                                       17       0  100.00%
contracts/0.8.25/Accounting.sol                                   90       5  94.44%   117-120, 346, 372
contracts/0.8.25/interfaces/IDepositContract.sol                   0       0  100.00%
contracts/0.8.25/interfaces/ILido.sol                              0       0  100.00%
contracts/0.8.25/interfaces/IOracleReportSanityChecker.sol         0       0  100.00%
contracts/0.8.25/interfaces/IPostTokenRebaseReceiver.sol           0       0  100.00%
contracts/0.8.25/interfaces/IStakingRouter.sol                     0       0  100.00%
contracts/0.8.25/interfaces/IWithdrawalQueue.sol                   0       0  100.00%
contracts/0.8.25/utils/AccessControlConfirmable.sol               30       0  100.00%
contracts/0.8.25/utils/PausableUntilWithRoles.sol                  3       0  100.00%
contracts/0.8.25/vaults/Dashboard.sol                             84       0  100.00%
contracts/0.8.25/vaults/Delegation.sol                            40       0  100.00%
contracts/0.8.25/vaults/Permissions.sol                           31       0  100.00%
contracts/0.8.25/vaults/StakingVault.sol                         102       0  100.00%
contracts/0.8.25/vaults/VaultFactory.sol                          43       0  100.00%
contracts/0.8.25/vaults/VaultHub.sol                             144     102  29.17%   111-124, 180-269, 284-350, 382-431, 443-451, 457-487, 501
contracts/0.8.25/vaults/interfaces/IStakingVault.sol               0       0  100.00%
contracts/0.8.4/WithdrawalsManagerProxy.sol                       61       0  100.00%
contracts/0.8.9/BeaconChainDepositor.sol                          21       2  90.48%   48, 51
contracts/0.8.9/Burner.sol                                        72       0  100.00%
contracts/0.8.9/DepositSecurityModule.sol                        128       0  100.00%
contracts/0.8.9/EIP712StETH.sol                                   16       0  100.00%
contracts/0.8.9/LidoExecutionLayerRewardsVault.sol                16       0  100.00%
contracts/0.8.9/LidoLocator.sol                                   20       0  100.00%
contracts/0.8.9/OracleDaemonConfig.sol                            28       0  100.00%
contracts/0.8.9/StakingRouter.sol                                316       0  100.00%
contracts/0.8.9/WithdrawalQueue.sol                               88       0  100.00%
contracts/0.8.9/WithdrawalQueueBase.sol                          146       0  100.00%
contracts/0.8.9/WithdrawalQueueERC721.sol                         89       0  100.00%
contracts/0.8.9/WithdrawalVault.sol                               21       0  100.00%
contracts/0.8.9/lib/Math.sol                                       4       0  100.00%
contracts/0.8.9/lib/PositiveTokenRebaseLimiter.sol                22      22  0.00%    88-172
contracts/0.8.9/lib/UnstructuredRefStorage.sol                     2       0  100.00%
contracts/0.8.9/oracle/AccountingOracle.sol                      190       2  98.95%   154-155
contracts/0.8.9/oracle/BaseOracle.sol                             89       1  98.88%   397
contracts/0.8.9/oracle/HashConsensus.sol                         263       1  99.62%   1005
contracts/0.8.9/oracle/ValidatorsExitBusOracle.sol                91       2  97.80%   138, 315
contracts/0.8.9/proxy/OssifiableProxy.sol                         17       0  100.00%
contracts/0.8.9/sanity_checks/OracleReportSanityChecker.sol      218      50  77.06%   195, 232, 273-286, 306-324, 413-441, 495, 558-561, 569, 578, 586, 697, 702-747, 802
contracts/0.8.9/utils/DummyEmptyContract.sol                       0       0  100.00%
contracts/0.8.9/utils/PausableUntil.sol                           31       0  100.00%
contracts/0.8.9/utils/Versioned.sol                               11       0  100.00%
contracts/0.8.9/utils/access/AccessControl.sol                    23       0  100.00%
contracts/0.8.9/utils/access/AccessControlEnumerable.sol           9       0  100.00%
contracts/common/utils/PausableUntil.sol                          29       1  96.55%   33
contracts/testnets/sepolia/SepoliaDepositAdapter.sol              21      21  0.00%    49-100
TOTAL                                                           3597     215  94.02%

Diff against master

Filename                                                       Stmts    Miss  Cover
-----------------------------------------------------------  -------  ------  --------
contracts/0.4.24/Lido.sol                                        -11      +6  -2.99%
contracts/0.4.24/StETH.sol                                        +7       0  +100.00%
contracts/0.8.25/Accounting.sol                                  +90      +5  +94.44%
contracts/0.8.25/interfaces/IDepositContract.sol                   0       0  +100.00%
contracts/0.8.25/interfaces/ILido.sol                              0       0  +100.00%
contracts/0.8.25/interfaces/IOracleReportSanityChecker.sol         0       0  +100.00%
contracts/0.8.25/interfaces/IPostTokenRebaseReceiver.sol           0       0  +100.00%
contracts/0.8.25/interfaces/IStakingRouter.sol                     0       0  +100.00%
contracts/0.8.25/interfaces/IWithdrawalQueue.sol                   0       0  +100.00%
contracts/0.8.25/utils/AccessControlConfirmable.sol              +30       0  +100.00%
contracts/0.8.25/utils/PausableUntilWithRoles.sol                 +3       0  +100.00%
contracts/0.8.25/vaults/Dashboard.sol                            +84       0  +100.00%
contracts/0.8.25/vaults/Delegation.sol                           +40       0  +100.00%
contracts/0.8.25/vaults/Permissions.sol                          +31       0  +100.00%
contracts/0.8.25/vaults/StakingVault.sol                        +102       0  +100.00%
contracts/0.8.25/vaults/VaultFactory.sol                         +43       0  +100.00%
contracts/0.8.25/vaults/VaultHub.sol                            +144    +102  +29.17%
contracts/0.8.25/vaults/interfaces/IStakingVault.sol               0       0  +100.00%
contracts/0.8.9/Burner.sol                                        +1       0  +100.00%
contracts/0.8.9/LidoLocator.sol                                   +2       0  +100.00%
contracts/0.8.9/lib/PositiveTokenRebaseLimiter.sol                 0     +22  -100.00%
contracts/0.8.9/oracle/ValidatorsExitBusOracle.sol                 0     -89  +97.80%
contracts/0.8.9/sanity_checks/OracleReportSanityChecker.sol      -14     +50  -22.94%
contracts/common/utils/PausableUntil.sol                         +29      +1  +96.55%
TOTAL                                                           +581     +97  -2.07%

Results for commit: 80d2e49

Minimum allowed coverage is 90%

♻️ This comment has been updated with latest results

Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

πŸ‘€

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe restore later

Comment on lines +9 to +16
import {ILidoLocator} from "../common/interfaces/ILidoLocator.sol";
import {IBurner} from "../common/interfaces/IBurner.sol";
import {IPostTokenRebaseReceiver} from "./interfaces/IPostTokenRebaseReceiver.sol";
import {IStakingRouter} from "./interfaces/IStakingRouter.sol";
import {IOracleReportSanityChecker} from "./interfaces/IOracleReportSanityChecker.sol";
import {IWithdrawalQueue} from "./interfaces/IWithdrawalQueue.sol";
import {ILido} from "./interfaces/ILido.sol";
import {ReportValues} from "contracts/common/interfaces/ReportValues.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not embedded in the file here? (Lido contract has embedded ones)

Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of consistency: let's use one way or another

}

// Distribute protocol fee (treasury & node operators)
if (_update.sharesToMintAsFees > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this name might be misleading, should be named smth 'sharesToMintAsLidoV2ProtocolFees'


_updateVaults(
_report.vaultValues,
_report.netCashFlows,
Copy link
Contributor

Choose a reason for hiding this comment

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

wdim 'cashflows'

}
}

/// @dev mints treasury rewards
Copy link
Contributor

Choose a reason for hiding this comment

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

only for Lido V2 protocol fee though

Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

πŸ‘€

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";
Copy link
Contributor

Choose a reason for hiding this comment

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

why this iface not in common?

Copy link
Contributor

Choose a reason for hiding this comment

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

should revise iface structure in the future

/// @notice role that allows to connect vaults to the hub
bytes32 public constant VAULT_MASTER_ROLE = keccak256("Vaults.VaultHub.VaultMasterRole");
/// @dev basis points base
uint256 internal constant BPS_BASE = 100_00;
Copy link
Contributor

Choose a reason for hiding this comment

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

has a different name everywhere outside this contract TOTAL_BASIS_POINT

/// @dev basis points base
uint256 internal constant BPS_BASE = 100_00;
/// @dev maximum number of vaults that can be connected to the hub
uint256 internal constant MAX_VAULTS_COUNT = 500;
Copy link
Contributor

Choose a reason for hiding this comment

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

test for reports should be able to digest this count

revert ExternalBalanceCapReached(address(_vault), capVaultBalance, maxExternalBalance);
}

VaultSocket memory vr = VaultSocket(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
VaultSocket memory vr = VaultSocket(
VaultSocket memory vsocket = VaultSocket(


/// @notice disconnects a vault from the hub
/// @dev can be called by vaults only
function disconnectVault(address _vault) external {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add emergency flow ⚠️

Copy link
Contributor

Choose a reason for hiding this comment

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

let's consider the following cases:

  • voluntary disconnect (this one)
  • stop new minting
  • eject and withdraw
  • disconnect forcibly

Copy link
Contributor

Choose a reason for hiding this comment

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

  • disconnect and dust issue

/// @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(
Copy link
Contributor

Choose a reason for hiding this comment

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

for permissionless addition:

  • there could be a scenario when we adding a vault under slashing

@tamtamchik tamtamchik changed the base branch from master to develop November 29, 2024 11:56
vaultToDisconnect.rebalance(stethToBurn);
}

vaultToDisconnect.report(vaultToDisconnect.valuation(), vaultToDisconnect.inOutDelta(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

Move report lower to reduce reentrance potential

/// @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 {
Copy link
Member

Choose a reason for hiding this comment

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

Embed auto-disconnect if resulting valuation is too small

) internal {
_checkAccountingOracleReport(_contracts, _report, _pre, _update);

uint256 lastWithdrawalRequestToFinalize;

Check warning

Code scanning / Slither

Uninitialized local variables Medium

Comment on lines 393 to 422
function _calculateLidoFees(
VaultSocket memory _socket,
uint256 _postTotalSharesNoFees,
uint256 _postTotalPooledEther,
uint256 _preTotalShares,
uint256 _preTotalPooledEther
) internal view returns (uint256 treasuryFeeShares) {
IHubVault vault_ = _socket.vault;

uint256 chargeableValue = Math256.min(
vault_.valuation(),
(_socket.shareLimit * _preTotalPooledEther) / _preTotalShares
);

// treasury fee is calculated as a share of potential rewards that
// Lido curated validators could earn if vault's ETH was staked in Lido
// itself and minted as stETH shares
//
// treasuryFeeShares = value * lidoGrossAPR * treasuryFeeRate / preShareRate
// lidoGrossAPR = postShareRateWithoutFees / preShareRate - 1
// = value * (postShareRateWithoutFees / preShareRate - 1) * treasuryFeeRate / preShareRate

// TODO: optimize potential rewards calculation
uint256 potentialRewards = ((chargeableValue * (_postTotalPooledEther * _preTotalShares)) /
(_postTotalSharesNoFees * _preTotalPooledEther) -
chargeableValue);
uint256 treasuryFee = (potentialRewards * _socket.treasuryFeeBP) / BPS_BASE;

treasuryFeeShares = (treasuryFee * _preTotalShares) / _preTotalPooledEther;
}
Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

πŸ‘€


stETH.mintExternalShares(_recipient, sharesToMint);

emit MintedStETHOnVault(_vault, _tokens);
Copy link
Contributor

Choose a reason for hiding this comment

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

need to disentangle events for each core operation later 🧠

Copy link
Contributor

Choose a reason for hiding this comment

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

backward compatibility is needed for Subgraph

for (uint256 i = 0; i < length; ++i) {
VaultSocket memory socket = $.sockets[i + 1];

// if there is no fee in Lido, then no fee in vaults
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// if there is no fee in Lido, then no fee in vaults
// if there is no fee in Lido, then no treasury fee in vaults

Comment on lines 388 to 389
uint256 mintedStETH = (totalMintedShares * _postTotalPooledEther) / _postTotalShares; //TODO: check rounding
lockedEther[i] = (mintedStETH * BPS_BASE) / (BPS_BASE - socket.reserveRatio);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe combine this to improve rounding

}
}

function _calculateLidoFees(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function _calculateLidoFees(
function _calculateTreasuryFees(


uint256 chargeableValue = Math256.min(
vault_.valuation(),
(_socket.shareLimit * _preTotalPooledEther) / _preTotalShares
Copy link
Contributor

Choose a reason for hiding this comment

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

won't work for non-mintable vaults

// itself and minted as stETH shares
//
// treasuryFeeShares = value * lidoGrossAPR * treasuryFeeRate / preShareRate
// lidoGrossAPR = postShareRateWithoutFees / preShareRate - 1
Copy link
Contributor

Choose a reason for hiding this comment

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

???

Copy link
Contributor

Choose a reason for hiding this comment

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

postShareRateWithoutFeesAndExternalEther (post share rate of Lido V2 without fees)

* @notice Rebalances the vault by transferring ether
* @param _ether Amount of ether to rebalance
*/
function rebalanceVault(uint256 _ether) external payable virtual onlyRole(DEFAULT_ADMIN_ROLE) fundAndProceed {
Copy link
Contributor

@Jeday Jeday Dec 5, 2024

Choose a reason for hiding this comment

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

Could we add views for ether amount required to fund vault back to health. And other related ratio views?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+++

* @notice Burns stETH tokens from the sender backed by the vault
* @param _tokens Amount of tokens to burn
*/
function burn(uint256 _tokens) external virtual onlyRole(DEFAULT_ADMIN_ROLE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

burnPermit?

Copy link
Contributor

Choose a reason for hiding this comment

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

burnWstethPermit

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better in delegate

Copy link
Contributor

Choose a reason for hiding this comment

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

/**
* @notice Mints stETH tokens backed by the vault to a recipient.
* @param _recipient Address of the recipient
* @param _tokens Amount of tokens to mint
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add comment about value because of fundAndProceed

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe let's make this more explicit, not a modifier (hard to follow)

Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

πŸ‘€ πŸ‘€ πŸ‘€

// Lido curated validators could earn if vault's ETH was staked in Lido
// itself and minted as stETH shares
//
// treasuryFeeShares = value * lidoGrossAPR * treasuryFeeRate / preShareRate
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// treasuryFeeShares = value * lidoGrossAPR * treasuryFeeRate / preShareRate
// treasuryFeeShares = chargeableValue * lidoGrossAPR * treasuryFeeRate / (_preTotalPooledEther / _preTotalShares)

// itself and minted as stETH shares
//
// treasuryFeeShares = value * lidoGrossAPR * treasuryFeeRate / preShareRate
// lidoGrossAPR = postShareRateWithoutFees / preShareRate - 1
Copy link
Contributor

Choose a reason for hiding this comment

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

postShareRateWithoutFeesAndExternalEther (post share rate of Lido V2 without fees)

}
}

function _calculateLidoFees(
Copy link
Contributor

Choose a reason for hiding this comment

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

// @dev impossible to invoke this method under negative rebase

//
// treasuryFeeShares = value * lidoGrossAPR * treasuryFeeRate / preShareRate
// lidoGrossAPR = postShareRateWithoutFees / preShareRate - 1
// = value * (postShareRateWithoutFees / preShareRate - 1) * treasuryFeeRate / preShareRate
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// = value * (postShareRateWithoutFees / preShareRate - 1) * treasuryFeeRate / preShareRate
// treasuryFeeShares = value * (postShareRateWithoutFees / preShareRate - 1) * treasuryFeeRate / preShareRate

chargeableValue);
uint256 treasuryFee = (potentialRewards * _socket.treasuryFeeBP) / BPS_BASE;

treasuryFeeShares = (treasuryFee * _preTotalShares) / _preTotalPooledEther;
Copy link
Contributor

Choose a reason for hiding this comment

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

might be precision issues

if (msg.value == 0) revert ZeroArgument("msg.value");

VaultStorage storage $ = _getVaultStorage();
$.inOutDelta += SafeCast.toInt128(int256(msg.value));
Copy link
Contributor

Choose a reason for hiding this comment

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

:)

VaultStorage storage $ = _getVaultStorage();
$.inOutDelta -= SafeCast.toInt128(int256(_ether));

(bool success, ) = _recipient.call{value: _ether}("");
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit scary

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe Address.sendValue

* @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
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
emit Withdrawn(msg.sender, msg.sender, _ether);
emit Withdrawn(msg.sender, VAULT_HUB, _ether);

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe 'Rebalanced'

Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

πŸš›

return address(VAULT_HUB);
}

receive() external payable {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Here we have an attack scenario

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe rename this event


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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
modifier fundAndProceed() {
modifier prefundable() {

* @dev Transfers ownership of the staking vault to a new owner
* @param _newOwner Address of the new owner
*/
function _transferStVaultOwnership(address _newOwner) internal {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe plural for keys


// ==================== Errors ====================

/// @notice Error for zero address arguments
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

should have a view func to show withdrawable amount
for rebalance we also need a view to show what would happen

Copy link
Contributor

Choose a reason for hiding this comment

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

this error should be moved to Delegation not being used here at all (it seems)

Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

πŸš› πŸš› πŸš›

// ==================== Constants ====================

uint256 private constant BP_BASE = 10000; // Basis points base (100%)
uint256 private constant MAX_FEE = BP_BASE; // Maximum fee in basis points (100%)
Copy link
Contributor

Choose a reason for hiding this comment

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

TOTAL_BASIS_POINTS

Comment on lines 43 to 44
* - vote on ownership transfer
* - vote on performance fee changes
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure

* Lido DAO can:
* - set the operator role
* - vote on ownership transfer
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

what about VaultHub.connect?

Copy link
Contributor

Choose a reason for hiding this comment

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

another issue: vault should express a WILL to connect (two-step)

Copy link
Contributor

Choose a reason for hiding this comment

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

🧠 🧠 🧠 LDO staking

Copy link
Contributor

Choose a reason for hiding this comment

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

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");
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ there is no timeElapsed (in case of missing reports e.g.)

Copy link
Contributor

Choose a reason for hiding this comment

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

and 365.25

Comment on lines 387 to 388
int256 unlocked = int256(stakingVault.valuation()) - int256(stakingVault.locked());
uint256 unreserved = unlocked >= 0 ? uint256(unlocked) : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

indexed fields?

/// @notice Emitted when a role member votes on a function requiring committee approval.
event RoleMemberVoted(address member, bytes32 role, uint256 timestamp, bytes data);

// ==================== Errors ====================
Copy link
Contributor

Choose a reason for hiding this comment

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

fees not covered with event changes

@tamtamchik tamtamchik added solidity issues/tasks related to smart contract code next upgrade Things to pickup for the next protocol upgrade vaults labels Dec 12, 2024
Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

First lap finished πŸ‘


import {IStakingVault} from "./interfaces/IStakingVault.sol";

pragma solidity 0.8.25;
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸš“ pragma should go before the imports

Comment on lines 14 to 15
uint256 managementFee;
uint256 performanceFee;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint256 managementFee;
uint256 performanceFee;
uint256 managementFeeBP;
uint256 performanceFeeBP;


pragma solidity 0.8.25;

interface IDelegation {
Copy link
Contributor

Choose a reason for hiding this comment

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

why we don't import this interface though?

Copy link
Contributor

Choose a reason for hiding this comment

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

let's add the comment that it's intended (to hook up the particular interface strictly)

}

contract VaultFactory is UpgradeableBeacon {
address public immutable delegationImpl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
address public immutable delegationImpl;
address public immutable DELEGATION_IMPL;

Copy link
Contributor

Choose a reason for hiding this comment

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

Why it's immutable?

Copy link
Contributor

Choose a reason for hiding this comment

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

should think about disentangling the beacon and factory (from Azat)

Copy link
Contributor

Choose a reason for hiding this comment

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

@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);
Copy link
Contributor

Choose a reason for hiding this comment

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

scary though, let's add a comment that those roles above have their admins

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ let's not rely on 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function createVault(
function createVaultWithDelegation(

Copy link
Contributor

Choose a reason for hiding this comment

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

🧠 let's extract voting modifier to a library or extension for AC

* @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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🧠 role setup has dependencies on the Delegation internals which is not perfect

* @notice Disconnects the staking vault from the vault hub.
*/
function voluntaryDisconnect() external payable fundAndProceed {
uint256 shares = vaultHub.vaultSocket(address(stakingVault())).sharesMinted;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint256 shares = vaultHub.vaultSocket(address(stakingVault())).sharesMinted;
uint256 shares = vaultSocket().sharesMinted;

Comment on lines +94 to +100
function initialize(address _defaultAdmin, uint256 _confirmExpiry) external virtual {
// reduces gas cost for `mintWsteth`
// invariant: dashboard does not hold stETH on its balance
STETH.approve(address(WSTETH), type(uint256).max);

_initialize(_defaultAdmin, _confirmExpiry);
}

Check warning

Code scanning / Slither

Unused return Medium

Comment on lines +84 to +93
function initialize(address _defaultAdmin, uint256 _confirmExpiry) external override {
_initialize(_defaultAdmin, _confirmExpiry);

// the next line implies that the msg.sender is an operator
// however, the msg.sender is the VaultFactory, and the role will be revoked
// at the end of the initialization
_grantRole(NODE_OPERATOR_MANAGER_ROLE, _defaultAdmin);
_setRoleAdmin(NODE_OPERATOR_MANAGER_ROLE, NODE_OPERATOR_MANAGER_ROLE);
_setRoleAdmin(NODE_OPERATOR_FEE_CLAIM_ROLE, NODE_OPERATOR_MANAGER_ROLE);
}

Check failure

Code scanning / Slither

Unprotected Initialize High

Function Delegation.initialize(address,uint256) is an unprotected initializer.
Comment on lines +274 to +278
function transferAndBurnShares(address _vault, uint256 _amountOfShares) external {
STETH.transferSharesFrom(msg.sender, address(this), _amountOfShares);

burnShares(_vault, _amountOfShares);
}

Check warning

Code scanning / Slither

Unused return Medium

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next upgrade Things to pickup for the next protocol upgrade solidity issues/tasks related to smart contract code tests When it comes to testing the code vaults
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants