Merge pull request lidofinance#419 from lidofinance/review/mev-lips-r…

…ound2

Fix: review/mev lips round2

contracts/0.4.24/StETH.sol

@@ -97,15 +97,17 @@ contract StETH is IERC20, Pausable {
      *
      * @dev Reports simultaneously burnt shares amount
      * and corresponding stETH amount.
-     * The stETH amount is calculated just before the burning incurred rebase.
+     * The stETH amount is calculated twice: before and after the burning incurred rebase.
      *
      * @param account holder of the burnt shares
-     * @param amount amount of burnt shares (expressed in stETH just before burning invocation)
-     * @param sharesAmount amount of burnt shares (expressed in shares themselves)
+     * @param preRebaseTokenAmount amount of stETH the burnt shares corresponded to before the burn
+     * @param postRebaseTokenAmount amount of stETH the burnt shares corresponded to after the burn
+     * @param sharesAmount amount of burnt shares
      */
     event SharesBurnt(
         address indexed account,
-        uint256 amount,
+        uint256 preRebaseTokenAmount,
+        uint256 postRebaseTokenAmount,
         uint256 sharesAmount
     );
 
@@ -458,14 +460,17 @@ contract StETH is IERC20, Pausable {
         uint256 accountShares = shares[_account];
         require(_sharesAmount <= accountShares, "BURN_AMOUNT_EXCEEDS_BALANCE");
 
-        uint256 amount = getPooledEthByShares(_sharesAmount);
-        emit SharesBurnt(_account, amount, _sharesAmount);
+        uint256 preRebaseTokenAmount = getPooledEthByShares(_sharesAmount);
 
         newTotalShares = _getTotalShares().sub(_sharesAmount);
         TOTAL_SHARES_POSITION.setStorageUint256(newTotalShares);
 
         shares[_account] = accountShares.sub(_sharesAmount);
 
+        uint256 postRebaseTokenAmount = getPooledEthByShares(_sharesAmount);
+
+        emit SharesBurnt(_account, preRebaseTokenAmount, postRebaseTokenAmount, _sharesAmount);
+
         // Notice: we're not emitting a Transfer event to the zero address here since shares burn
         // works by redistributing the amount of tokens corresponding to the burned shares between
         // all other token holders. The total supply of the token doesn't change as the result.

contracts/0.4.24/interfaces/ILido.sol

@@ -13,6 +13,10 @@ pragma solidity 0.4.24;
   * and stakes it via the deposit_contract.sol contract. It doesn't hold ether on it's balance,
   * only a small portion (buffer) of it.
   * It also mints new tokens for rewards generated at the ETH 2.0 side.
+  *
+  * At the moment withdrawals are not possible in the beacon chain and there's no workaround.
+  * Pool will be upgraded to an actual implementation when withdrawals are enabled
+  * (Phase 1.5 or 2 of Eth2 launch, likely late 2022 or 2023).
   */
 interface ILido {
     function totalSupply() external view returns (uint256);
@@ -29,7 +33,7 @@ interface ILido {
     function resume() external;
 
     /**
-      * @notice Stops accepting new Ether to the protocol.
+      * @notice Stops accepting new Ether to the protocol
       *
       * @dev While accepting new Ether is stopped, calls to the `submit` function,
       * as well as to the default payable function, will revert.
@@ -40,12 +44,37 @@ interface ILido {
 
     /**
       * @notice Resumes accepting new Ether to the protocol (if `pauseStaking` was called previously)
-      * and updates the staking rate limit.
-      * NB: To resume without limits pass zero arg values.
+      * NB: Staking could be rate-limited by imposing a limit on the stake amount
+      * at each moment in time, see `setStakingLimit()` and `removeStakingLimit()`
+      *
+      * @dev Preserves staking limit if it was set previously
+      *
+      * Emits `StakingResumed` event
+      */
+    function resumeStaking() external;
+
+    /**
+      * @notice Sets the staking rate limit
+      *
+      * @dev Reverts if:
+      * - `_maxStakeLimit` == 0
+      * - `_maxStakeLimit` >= 2^96
+      * - `_maxStakeLimit` < `_stakeLimitIncreasePerBlock`
+      * - `_maxStakeLimit` / `_stakeLimitIncreasePerBlock` >= 2^32 (only if `_stakeLimitIncreasePerBlock` != 0)
+      *
+      * Emits `StakingLimitSet` event
+      *
       * @param _maxStakeLimit max stake limit value
       * @param _stakeLimitIncreasePerBlock stake limit increase per single block
       */
-    function resumeStaking(uint256 _maxStakeLimit, uint256 _stakeLimitIncreasePerBlock) external;
+    function setStakingLimit(uint256 _maxStakeLimit, uint256 _stakeLimitIncreasePerBlock) external;
+
+    /**
+      * @notice Removes the staking rate limit
+      *
+      * Emits `StakingLimitRemoved` event
+      */
+    function removeStakingLimit() external;
 
     /**
       * @notice Check staking state: whether it's paused or not
@@ -61,15 +90,20 @@ interface ILido {
     function getCurrentStakeLimit() external view returns (uint256);
 
     /**
-      * @notice Returns internal info about stake limit
+      * @notice Returns full info about current stake limit params and state
       * @dev Might be used for the advanced integration requests.
-      * @return
-      * `maxStakeLimit` max stake limit
-      * `maxStakeLimitGrowthBlocks` blocks needed to restore max stake limit from the fully exhausted state
-      * `prevStakeLimit` previously reached stake limit
-      * `prevStakeBlockNumber` previously seen block number
+      * @return isStakingPaused staking pause state (equivalent to return of isStakingPaused())
+      * @return isStakingLimitSet whether the stake limit is set
+      * @return currentStakeLimit current stake limit (equivalent to return of getCurrentStakeLimit())
+      * @return maxStakeLimit max stake limit
+      * @return maxStakeLimitGrowthBlocks blocks needed to restore max stake limit from the fully exhausted state
+      * @return prevStakeLimit previously reached stake limit
+      * @return prevStakeBlockNumber previously seen block number
       */
-    function getStakeLimitInternalInfo() external view returns (
+    function getStakeLimitFullInfo() external view returns (
+        bool isStakingPaused,
+        bool isStakingLimitSet,
+        uint256 currentStakeLimit,
         uint256 maxStakeLimit,
         uint256 maxStakeLimitGrowthBlocks,
         uint256 prevStakeLimit,
@@ -78,14 +112,17 @@ interface ILido {
 
     event Stopped();
     event Resumed();
+
     event StakingPaused();
-    event StakingResumed(uint256 maxStakeLimit, uint256 stakeLimitIncreasePerBlock);
+    event StakingResumed();
+    event StakingLimitSet(uint256 maxStakeLimit, uint256 stakeLimitIncreasePerBlock);
+    event StakingLimitRemoved();
 
     /**
       * @notice Set Lido protocol contracts (oracle, treasury, insurance fund).
       * @param _oracle oracle contract
-      * @param _treasury treasury contract which accumulates treasury fee
-      * @param _insuranceFund insurance fund contract which accumulates insurance fee
+      * @param _treasury treasury contract
+      * @param _insuranceFund insurance fund contract
       */
     function setProtocolContracts(
         address _oracle,
@@ -137,16 +174,16 @@ interface ILido {
 
     /**
       * @notice A payable function supposed to be called only by LidoExecutionLayerRewardsVault contract
-      * @dev We need a separate function because funds received by default payable function
-      * are considered as funds submitted by a user for staking
+      * @dev We need a dedicated function because funds received by the default payable function
+      * are treated as a user deposit
       */
     function receiveELRewards() external payable;
 
     // The amount of ETH withdrawn from LidoExecutionLayerRewardsVault contract to Lido contract
     event ELRewardsReceived(uint256 amount);
 
     /**
-      * @dev Sets limit to amount of ETH to withdraw from execution layer rewards vault per LidoOracle report
+      * @dev Sets limit on amount of ETH to withdraw from execution layer rewards vault per LidoOracle report
       * @param _limitPoints limit in basis points to amount of ETH to withdraw per LidoOracle report
       */
     function setELRewardsWithdrawalLimit(uint16 _limitPoints) external;
@@ -157,8 +194,7 @@ interface ILido {
     /**
       * @notice Set credentials to withdraw ETH on ETH 2.0 side after the phase 2 is launched to `_withdrawalCredentials`
       * @dev Note that setWithdrawalCredentials discards all unused signing keys as the signatures are invalidated.
-      * @param _withdrawalCredentials hash of withdrawal multisignature key as accepted by
-      *        the deposit_contract.deposit function
+      * @param _withdrawalCredentials withdrawal credentials field as defined in the Ethereum PoS consensus specs
       */
     function setWithdrawalCredentials(bytes32 _withdrawalCredentials) external;
 
@@ -170,9 +206,9 @@ interface ILido {
     event WithdrawalCredentialsSet(bytes32 withdrawalCredentials);
 
     /**
-    * @dev Sets the address of LidoExecutionLayerRewardsVault contract
-    * @param _executionLayerRewardsVault Execution layer rewards vault contract address
-    */
+      * @dev Sets the address of LidoExecutionLayerRewardsVault contract
+      * @param _executionLayerRewardsVault Execution layer rewards vault contract address
+      */
     function setELRewardsVault(address _executionLayerRewardsVault) external;
 
     // The `executionLayerRewardsVault` was set as the execution layer rewards vault for Lido
@@ -200,13 +236,6 @@ interface ILido {
     // The `amount` of ether was sent to the deposit_contract.deposit function
     event Unbuffered(uint256 amount);
 
-    /**
-      * @notice Issues withdrawal request. Large withdrawals will be processed only after the phase 2 launch.
-      * @param _amount Amount of StETH to burn
-      * @param _pubkeyHash Receiving address
-      */
-    function withdraw(uint256 _amount, bytes32 _pubkeyHash) external;
-
     // Requested withdrawal of `etherAmount` to `pubkeyHash` on the ETH 2.0 side, `tokenAmount` burned by `sender`,
     // `sentFromBuffer` was sent on the current Ethereum side.
     event Withdrawal(address indexed sender, uint256 tokenAmount, uint256 sentFromBuffer,
@@ -232,7 +261,4 @@ interface ILido {
       * @return beaconBalance - total amount of Beacon-side Ether (sum of all the balances of Lido validators)
       */
     function getBeaconStat() external view returns (uint256 depositedValidators, uint256 beaconValidators, uint256 beaconBalance);
-
-    // Requested ERC721 recovery from the `Lido` to the designated `recoveryVault` vault.
-    event RecoverERC721ToVault(address indexed vault, address indexed token, uint256 tokenId);
 }

contracts/0.4.24/interfaces/ILidoExecutionLayerRewardsVault.sol

@@ -10,7 +10,7 @@ interface ILidoExecutionLayerRewardsVault {
     /**
     * @notice Withdraw all accumulated execution layer rewards to Lido contract
     * @param _maxAmount Max amount of ETH to withdraw
-    * @return amount uint256 of funds received as execution layer rewards (in wei)
+    * @return amount of funds received as execution layer rewards (in wei)
     */
     function withdrawRewards(uint256 _maxAmount) external returns (uint256 amount);
 }

contracts/0.4.24/lib/StakeLimitUtils.sol

@@ -27,7 +27,7 @@ import "@aragon/os/contracts/common/UnstructuredStorage.sol";
 //           32 bits                 96 bits               96 bits
 //
 //
-// - the "staking paused" state is encoded by all fields being zero,
+// - the "staking paused" state is encoded by `prevStakeBlockNumber` being zero,
 // - the "staking unlimited" state is encoded by `maxStakeLimit` being zero and `prevStakeBlockNumber` being non-zero.
 //
 
@@ -64,13 +64,13 @@ library StakeLimitUnstructuredStorage {
     * @dev Read stake limit state from the unstructured storage position
     * @param _position storage offset
     */
-    function getStorageStakeLimitStruct(bytes32 _position) internal view returns (StakeLimitState.Data memory ret) {
+    function getStorageStakeLimitStruct(bytes32 _position) internal view returns (StakeLimitState.Data memory stakeLimit) {
         uint256 slotValue = _position.getStorageUint256();
 
-        ret.prevStakeBlockNumber = uint32(slotValue >> PREV_STAKE_BLOCK_NUMBER_OFFSET);
-        ret.prevStakeLimit = uint96(slotValue >> PREV_STAKE_LIMIT_OFFSET);
-        ret.maxStakeLimitGrowthBlocks = uint32(slotValue >> MAX_STAKE_LIMIT_GROWTH_BLOCKS_OFFSET);
-        ret.maxStakeLimit = uint96(slotValue >> MAX_STAKE_LIMIT_OFFSET);
+        stakeLimit.prevStakeBlockNumber = uint32(slotValue >> PREV_STAKE_BLOCK_NUMBER_OFFSET);
+        stakeLimit.prevStakeLimit = uint96(slotValue >> PREV_STAKE_LIMIT_OFFSET);
+        stakeLimit.maxStakeLimitGrowthBlocks = uint32(slotValue >> MAX_STAKE_LIMIT_GROWTH_BLOCKS_OFFSET);
+        stakeLimit.maxStakeLimit = uint96(slotValue >> MAX_STAKE_LIMIT_OFFSET);
     }
 
      /**
@@ -108,34 +108,32 @@ library StakeLimitUtils {
     }
 
     /**
-    * @notice check if staking is on pause (i.e. every byte in the slot has a zero value)
+    * @notice check if staking is on pause
     */
     function isStakingPaused(StakeLimitState.Data memory _data) internal pure returns(bool) {
-        return (
-            _data.maxStakeLimit
-            | _data.maxStakeLimitGrowthBlocks
-            | _data.prevStakeBlockNumber
-            | _data.prevStakeLimit
-        ) == 0;
+        return _data.prevStakeBlockNumber == 0;
     }
 
     /**
-    * @notice check if staking limit is applied (otherwise staking is unlimited)
+    * @notice check if staking limit is set (otherwise staking is unlimited)
     */
-    function isStakingLimitApplied(StakeLimitState.Data memory _data) internal pure returns(bool) {
+    function isStakingLimitSet(StakeLimitState.Data memory _data) internal pure returns(bool) {
         return _data.maxStakeLimit != 0;
     }
 
     /**
-    * @notice prepare stake limit repr to resume staking with the desired limits
+    * @notice update stake limit repr with the desired limits
+    * @dev input `_data` param is mutated and the func returns effectively the same pointer
+    * @param _data stake limit state struct
     * @param _maxStakeLimit stake limit max value
     * @param _stakeLimitIncreasePerBlock stake limit increase (restoration) per block
     */
-    function resumeStakingWithNewLimit(
+    function setStakingLimit(
         StakeLimitState.Data memory _data,
         uint256 _maxStakeLimit,
         uint256 _stakeLimitIncreasePerBlock
     ) internal view returns (StakeLimitState.Data memory) {
+        require(_maxStakeLimit != 0, "ZERO_MAX_STAKE_LIMIT");
         require(_maxStakeLimit <= uint96(-1), "TOO_LARGE_MAX_STAKE_LIMIT");
         require(_maxStakeLimit >= _stakeLimitIncreasePerBlock, "TOO_LARGE_LIMIT_INCREASE");
         require(
@@ -153,23 +151,58 @@ library StakeLimitUtils {
         _data.maxStakeLimitGrowthBlocks = _stakeLimitIncreasePerBlock != 0 ? uint32(_maxStakeLimit / _stakeLimitIncreasePerBlock) : 0;
 
         _data.maxStakeLimit = uint96(_maxStakeLimit);
-        _data.prevStakeBlockNumber = uint32(block.number);
+
+        if (_data.prevStakeBlockNumber != 0) {
+            _data.prevStakeBlockNumber = uint32(block.number);
+        }
+
+        return _data;
+    }
+
+    /**
+    * @notice update stake limit repr to remove the limit
+    * @dev input `_data` param is mutated and the func returns effectively the same pointer
+    * @param _data stake limit state struct
+    */
+    function removeStakingLimit(
+        StakeLimitState.Data memory _data
+    ) internal view returns (StakeLimitState.Data memory) {
+        _data.maxStakeLimit = 0;
 
         return _data;
     }
 
     /**
     * @notice update stake limit repr after submitting user's eth
+    * @dev input `_data` param is mutated and the func returns effectively the same pointer
+    * @param _data stake limit state struct
+    * @param _newPrevStakeLimit new value for the `prevStakeLimit` field
     */
     function updatePrevStakeLimit(
         StakeLimitState.Data memory _data,
-        uint256 _newPrevLimit
+        uint256 _newPrevStakeLimit
     ) internal view returns (StakeLimitState.Data memory) {
-        assert(_newPrevLimit <= uint96(-1));
+        assert(_newPrevStakeLimit <= uint96(-1));
+        assert(_data.prevStakeBlockNumber != 0);
 
-        _data.prevStakeLimit = uint96(_newPrevLimit);
+        _data.prevStakeLimit = uint96(_newPrevStakeLimit);
         _data.prevStakeBlockNumber = uint32(block.number);
 
         return _data;
     }
+
+    /**
+    * @notice set stake limit pause state (on or off)
+    * @dev input `_data` param is mutated and the func returns effectively the same pointer
+    * @param _data stake limit state struct
+    * @param _isPaused pause state flag
+    */
+    function setStakeLimitPauseState(
+        StakeLimitState.Data memory _data,
+        bool _isPaused
+    ) internal view returns (StakeLimitState.Data memory) {
+        _data.prevStakeBlockNumber = uint32(_isPaused ? 0 : block.number);
+
+        return _data;
+    }
 }

contracts/0.4.24/template/LidoTemplate.sol

@@ -656,7 +656,7 @@ contract LidoTemplate is IsContract {
         perms[4] = _state.lido.BURN_ROLE();
         perms[5] = _state.lido.RESUME_ROLE();
         perms[6] = _state.lido.STAKING_PAUSE_ROLE();
-        perms[7] = _state.lido.STAKING_RESUME_ROLE();
+        perms[7] = _state.lido.STAKING_CONTROL_ROLE();
         perms[8] = _state.lido.SET_EL_REWARDS_VAULT_ROLE();
         perms[9] = _state.lido.SET_EL_REWARDS_WITHDRAWAL_LIMIT_ROLE();
 

contracts/0.4.24/test_helpers/LidoMock.sol

@@ -28,6 +28,7 @@ contract LidoMock is Lido {
         );
 
         _resume();
+        _resumeStaking();
     }
 
     /**