Merge pull request #58 from lidofinance/feature/review

Review

Author: avsetsin

src/StvPool.sol

@@ -26,6 +26,7 @@ contract StvPool is Initializable, ERC20Upgradeable, AllowList, FeaturePausable
     error NotEnoughToRebalance();
     error UnassignedLiabilityOnVault();
     error VaultInBadDebt();
+    error VaultReportStale();
 
     bytes32 public constant DEPOSITS_FEATURE = keccak256("DEPOSITS_FEATURE");
     bytes32 public constant DEPOSITS_PAUSE_ROLE = keccak256("DEPOSITS_PAUSE_ROLE");
@@ -208,6 +209,7 @@ contract StvPool is Initializable, ERC20Upgradeable, AllowList, FeaturePausable
      * @param _recipient Address to receive the minted shares
      * @param _referral Address of the referral (if any)
      * @return stv Amount of stv minted
+     * @dev Requires fresh oracle report to price stv accurately
      */
     function depositETH(address _recipient, address _referral) public payable returns (uint256 stv) {
         stv = _deposit(_recipient, _referral);
@@ -218,6 +220,7 @@ contract StvPool is Initializable, ERC20Upgradeable, AllowList, FeaturePausable
         if (_recipient == address(0)) revert InvalidRecipient();
         _checkFeatureNotPaused(DEPOSITS_FEATURE);
         _checkAllowList();
+        _checkFreshReport();
 
         stv = previewDeposit(msg.value);
         _mint(_recipient, stv);
@@ -260,7 +263,7 @@ contract StvPool is Initializable, ERC20Upgradeable, AllowList, FeaturePausable
      * @param _stethShares Amount of stETH shares to rebalance (18 decimals)
      * @dev Only unassigned liability can be rebalanced with this method, not individual liability
      * @dev Can be called by anyone if there is any unassigned liability
-     * @dev Required fresh oracle report before calling
+     * @dev Requires fresh oracle report before calling (check is performed in VaultHub)
      */
     function rebalanceUnassignedLiability(uint256 _stethShares) external {
         _checkOnlyUnassignedLiabilityRebalance(_stethShares);
@@ -274,7 +277,7 @@ contract StvPool is Initializable, ERC20Upgradeable, AllowList, FeaturePausable
      * @dev Only unassigned liability can be rebalanced with this method, not individual liability
      * @dev Can be called by anyone if there is any unassigned liability
      * @dev This function accepts ETH and uses it to rebalance unassigned liability
-     * @dev Required fresh oracle report before calling
+     * @dev Requires fresh oracle report before calling (check is performed in VaultHub)
      */
     function rebalanceUnassignedLiabilityWithEther() external payable {
         uint256 stethShares = _getSharesByPooledEth(msg.value);
@@ -406,4 +409,12 @@ contract StvPool is Initializable, ERC20Upgradeable, AllowList, FeaturePausable
         _checkRole(DEPOSITS_RESUME_ROLE, msg.sender);
         _resumeFeature(DEPOSITS_FEATURE);
     }
+
+    // =================================================================================
+    // ORACLE FRESHNESS CHECK
+    // =================================================================================
+
+    function _checkFreshReport() internal view {
+        if (!VAULT_HUB.isReportFresh(address(VAULT))) revert VaultReportStale();
+    }
 }

src/StvStETHPool.sol

@@ -29,7 +29,6 @@ contract StvStETHPool is StvPool {
     error InsufficientStv();
     error ZeroArgument();
     error ArraysLengthMismatch(uint256 firstArrayLength, uint256 secondArrayLength);
-    error VaultReportStale();
     error UndercollateralizedAccount();
     error CollateralizedAccount();
     error ExcessiveLossSocialization();
@@ -543,7 +542,7 @@ contract StvStETHPool is StvPool {
      * @dev Second, if there are remaining liability shares, rebalances Staking Vault
      * @dev Requires fresh oracle report, which is checked in the Withdrawal Queue
      */
-    function rebalanceMintedStethShares(uint256 _stethShares, uint256 _maxStvToBurn)
+    function rebalanceMintedStethSharesForWithdrawalQueue(uint256 _stethShares, uint256 _maxStvToBurn)
         public
         returns (uint256 stvBurned)
     {
@@ -559,6 +558,8 @@ contract StvStETHPool is StvPool {
      * @dev Requires fresh oracle report to price stv accurately
      */
     function forceRebalance(address _account) public returns (uint256 stvBurned) {
+        _checkFreshReport();
+
         (uint256 stethShares, uint256 stv, bool isUndercollateralized) = previewForceRebalance(_account);
         if (isUndercollateralized) revert UndercollateralizedAccount();
 
@@ -573,6 +574,7 @@ contract StvStETHPool is StvPool {
      */
     function forceRebalanceAndSocializeLoss(address _account) public returns (uint256 stvBurned) {
         _checkRole(LOSS_SOCIALIZER_ROLE, msg.sender);
+        _checkFreshReport();
 
         (uint256 stethShares, uint256 stv, bool isUndercollateralized) = previewForceRebalance(_account);
         if (!isUndercollateralized) revert CollateralizedAccount();
@@ -586,15 +588,13 @@ contract StvStETHPool is StvPool {
      * @return stethShares The amount of stETH shares to rebalance, limited by available assets
      * @return stv The amount of stv needed to burn in exchange for the stETH shares, limited by user's stv balance
      * @return isUndercollateralized True if the user's assets are insufficient to cover the liability
-     * @dev Requires fresh oracle report to price stv accurately
+     * @dev Requires fresh oracle report to price stv accurately (not enforced in this method, so caller must ensure it)
      */
     function previewForceRebalance(address _account)
         public
         view
         returns (uint256 stethShares, uint256 stv, bool isUndercollateralized)
     {
-        _checkFreshReport();
-
         uint256 stethSharesLiability = mintedStethSharesOf(_account);
         uint256 stvBalance = balanceOf(_account);
         uint256 assets = assetsOf(_account);
@@ -706,10 +706,6 @@ contract StvStETHPool is StvPool {
         }
     }
 
-    function _checkFreshReport() internal view {
-        if (!VAULT_HUB.isReportFresh(address(VAULT))) revert VaultReportStale();
-    }
-
     // =================================================================================
     // LOSS SOCIALIZATION LIMITER
     // =================================================================================

src/WithdrawalQueue.sol

@@ -303,6 +303,8 @@ contract WithdrawalQueue is AccessControlEnumerableUpgradeable, FeaturePausable
      * @param _stvToWithdraw Array of amounts of stv to withdraw
      * @param _stethSharesToRebalance Array of amounts of stETH shares to rebalance if supported by the pool, array of 0 otherwise
      * @return requestIds the created withdrawal request ids
+     * @dev Transfers stv and liability shares from the requester to the withdrawal queue
+     * @dev Requires fresh oracle report to price stv accurately
      */
     function requestWithdrawalBatch(
         address _owner,
@@ -311,6 +313,7 @@ contract WithdrawalQueue is AccessControlEnumerableUpgradeable, FeaturePausable
     ) external returns (uint256[] memory requestIds) {
         _checkFeatureNotPaused(WITHDRAWALS_FEATURE);
         _checkArrayLength(_stvToWithdraw.length, _stethSharesToRebalance.length);
+        _checkFreshReport();
 
         requestIds = new uint256[](_stvToWithdraw.length);
         for (uint256 i = 0; i < _stvToWithdraw.length; ++i) {
@@ -325,12 +328,15 @@ contract WithdrawalQueue is AccessControlEnumerableUpgradeable, FeaturePausable
      * @param _stethSharesToRebalance Amount of steth shares to rebalance if supported by the pool, 0 otherwise
      * @return requestId The created withdrawal request id
      * @dev Transfers stv and liability shares from the requester to the withdrawal queue
+     * @dev Requires fresh oracle report to price stv accurately
      */
     function requestWithdrawal(address _owner, uint256 _stvToWithdraw, uint256 _stethSharesToRebalance)
         external
         returns (uint256 requestId)
     {
         _checkFeatureNotPaused(WITHDRAWALS_FEATURE);
+        _checkFreshReport();
+
         requestId = _requestWithdrawal(_owner, _stvToWithdraw, _stethSharesToRebalance);
     }
 
@@ -562,15 +568,15 @@ contract WithdrawalQueue is AccessControlEnumerableUpgradeable, FeaturePausable
 
             // Stv burning is limited at this point by maxStvToRebalance calculated above
             // to make sure that only stv of finalized requests is used for rebalancing
-            totalStvRebalanced = POOL.rebalanceMintedStethShares(totalStethShares, maxStvToRebalance);
+            totalStvRebalanced = POOL.rebalanceMintedStethSharesForWithdrawalQueue(totalStethShares, maxStvToRebalance);
         }
 
         // 3. Burn any remaining stv that was not used for rebalancing
         // The rebalancing may burn less stv than maxStvToRebalance because of:
         //   - the changed stv rate after the first step
         //   - accumulated rounding errors in maxStvToRebalance
         //
-        // It's guaranteed by POOL.rebalanceMintedStethShares() that maxStvToRebalance >= totalStvRebalanced
+        // It's guaranteed by POOL.rebalanceMintedStethSharesForWithdrawalQueue() that maxStvToRebalance >= totalStvRebalanced
         uint256 remainingStvForRebalance = maxStvToRebalance - totalStvRebalanced;
         if (remainingStvForRebalance > 0) {
             POOL.burnStvForWithdrawalQueue(remainingStvForRebalance);
@@ -959,7 +965,7 @@ contract WithdrawalQueue is AccessControlEnumerableUpgradeable, FeaturePausable
     function _calcRequestAmounts(
         WithdrawalRequest memory _prevRequest,
         WithdrawalRequest memory _request,
-        Checkpoint memory checkpoint
+        Checkpoint memory _checkpoint
     )
         internal
         pure
@@ -979,21 +985,21 @@ contract WithdrawalQueue is AccessControlEnumerableUpgradeable, FeaturePausable
         uint256 requestStvRate = (assetsToClaim * E36_PRECISION_BASE) / stv;
 
         // Apply discount if the request stv rate is above the finalization stv rate
-        if (requestStvRate > checkpoint.stvRate) {
-            assetsToClaim = Math.mulDiv(stv, checkpoint.stvRate, E36_PRECISION_BASE, Math.Rounding.Floor);
+        if (requestStvRate > _checkpoint.stvRate) {
+            assetsToClaim = Math.mulDiv(stv, _checkpoint.stvRate, E36_PRECISION_BASE, Math.Rounding.Floor);
         }
 
         if (stethSharesToRebalance > 0) {
             assetsToRebalance =
-                Math.mulDiv(stethSharesToRebalance, checkpoint.stethShareRate, E27_PRECISION_BASE, Math.Rounding.Ceil);
+                Math.mulDiv(stethSharesToRebalance, _checkpoint.stethShareRate, E27_PRECISION_BASE, Math.Rounding.Ceil);
 
             // Decrease assets to claim by the amount of assets to rebalance
             assetsToClaim = Math.saturatingSub(assetsToClaim, assetsToRebalance);
         }
 
         // Apply request finalization gas cost coverage
-        if (checkpoint.gasCostCoverage > 0) {
-            gasCostCoverage = Math.min(assetsToClaim, checkpoint.gasCostCoverage);
+        if (_checkpoint.gasCostCoverage > 0) {
+            gasCostCoverage = Math.min(assetsToClaim, _checkpoint.gasCostCoverage);
             assetsToClaim -= gasCostCoverage;
         }
     }
@@ -1064,8 +1070,10 @@ contract WithdrawalQueue is AccessControlEnumerableUpgradeable, FeaturePausable
     // CHECKS
     // =================================================================================
 
-    function _checkArrayLength(uint256 firstArrayLength, uint256 secondArrayLength) internal pure {
-        if (firstArrayLength != secondArrayLength) revert ArraysLengthMismatch(firstArrayLength, secondArrayLength);
+    function _checkArrayLength(uint256 _firstArrayLength, uint256 _secondArrayLength) internal pure {
+        if (_firstArrayLength != _secondArrayLength) {
+            revert ArraysLengthMismatch(_firstArrayLength, _secondArrayLength);
+        }
     }
 
     function _checkFreshReport() internal view {

src/interfaces/IStvStETHPool.sol

@@ -7,7 +7,7 @@ import {IVaultHub} from "./core/IVaultHub.sol";
 
 interface IStvStETHPool is IBasePool {
     function totalExceedingMintedSteth() external view returns (uint256);
-    function rebalanceMintedStethShares(uint256 _stethShares, uint256 _maxStvToBurn)
+    function rebalanceMintedStethSharesForWithdrawalQueue(uint256 _stethShares, uint256 _maxStvToBurn)
         external
         returns (uint256 stvBurned);
     function transferFromWithLiabilityForWithdrawalQueue(address _from, uint256 _stv, uint256 _stethShares) external;