Merge pull request #357 from lidofinance/feature/deposit-frontrun-pro…

…tection-upgrade

Protect from deposit front-running vulnerability

apps/node-operators-registry/app/src/App.js

@@ -50,8 +50,7 @@ function App() {
     []
   )
   const addNodeOperatorApi = useCallback(
-    (name, address, limit) =>
-      api.addNodeOperator(name, address, limit).toPromise(),
+    (name, address) => api.addNodeOperator(name, address).toPromise(),
     [api]
   )
 

apps/node-operators-registry/app/src/components/AddNodeOperatorSidePanel.js

@@ -18,8 +18,8 @@ const validationSchema = yup.object().shape({
 
 function PanelContent({ addNodeOperatorApi, onClose }) {
   const onSubmit = useCallback(
-    ({ name, address, limit }) => {
-      addNodeOperatorApi(name, address, limit)
+    ({ name, address }) => {
+      addNodeOperatorApi(name, address)
         .catch(console.error)
         .finally(() => {
           onClose()

contracts/0.4.24/Lido.sol

@@ -46,6 +46,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp {
     bytes32 constant public BURN_ROLE = keccak256("BURN_ROLE");
     bytes32 constant public SET_TREASURY = keccak256("SET_TREASURY");
     bytes32 constant public SET_INSURANCE_FUND = keccak256("SET_INSURANCE_FUND");
+    bytes32 constant public DEPOSIT_ROLE = keccak256("DEPOSIT_ROLE");
 
     uint256 constant public PUBKEY_LENGTH = 48;
     uint256 constant public WITHDRAWAL_CREDENTIALS_LENGTH = 32;
@@ -56,7 +57,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp {
     uint256 internal constant DEPOSIT_AMOUNT_UNIT = 1000000000 wei;
 
     /// @dev default value for maximum number of Ethereum 2.0 validators registered in a single depositBufferedEther call
-    uint256 internal constant DEFAULT_MAX_DEPOSITS_PER_CALL = 16;
+    uint256 internal constant DEFAULT_MAX_DEPOSITS_PER_CALL = 150;
 
     bytes32 internal constant FEE_POSITION = keccak256("lido.Lido.fee");
     bytes32 internal constant TREASURY_FEE_POSITION = keccak256("lido.Lido.treasuryFee");
@@ -131,15 +132,15 @@ contract Lido is ILido, IsContract, StETH, AragonApp {
     * @notice Deposits buffered ethers to the official DepositContract.
     * @dev This function is separated from submit() to reduce the cost of sending funds.
     */
-    function depositBufferedEther() external {
+    function depositBufferedEther() external auth(DEPOSIT_ROLE) {
         return _depositBufferedEther(DEFAULT_MAX_DEPOSITS_PER_CALL);
     }
 
     /**
       * @notice Deposits buffered ethers to the official DepositContract, making no more than `_maxDeposits` deposit calls.
       * @dev This function is separated from submit() to reduce the cost of sending funds.
       */
-    function depositBufferedEther(uint256 _maxDeposits) external {
+    function depositBufferedEther(uint256 _maxDeposits) external auth(DEPOSIT_ROLE) {
         return _depositBufferedEther(_maxDeposits);
     }
 
@@ -297,7 +298,8 @@ contract Lido is ILido, IsContract, StETH, AragonApp {
         uint256 balance;
         if (_token == ETH) {
             balance = _getUnaccountedEther();
-            vault.transfer(balance);
+            // Transfer replaced by call to prevent transfer gas amount issue    
+            require(vault.call.value(balance)(), "RECOVER_TRANSFER_FAILED");
         } else {
             ERC20 token = ERC20(_token);
             balance = token.staticBalanceOf(this);

contracts/0.4.24/interfaces/INodeOperatorsRegistry.sol

@@ -13,13 +13,12 @@ pragma solidity 0.4.24;
   */
 interface INodeOperatorsRegistry {
     /**
-      * @notice Add node operator named `name` with reward address `rewardAddress` and staking limit `stakingLimit` validators
+      * @notice Add node operator named `name` with reward address `rewardAddress` and staking limit = 0 validators
       * @param _name Human-readable name
       * @param _rewardAddress Ethereum 1 address which receives stETH rewards for this operator
-      * @param _stakingLimit the maximum number of validators to stake for this operator
       * @return a unique key of the added operator
       */
-    function addNodeOperator(string _name, address _rewardAddress, uint64 _stakingLimit) external returns (uint256 id);
+    function addNodeOperator(string _name, address _rewardAddress) external returns (uint256 id);
 
     /**
       * @notice `_active ? 'Enable' : 'Disable'` the node operator #`_id`
@@ -91,6 +90,7 @@ interface INodeOperatorsRegistry {
     event NodeOperatorRewardAddressSet(uint256 indexed id, address rewardAddress);
     event NodeOperatorStakingLimitSet(uint256 indexed id, uint64 stakingLimit);
     event NodeOperatorTotalStoppedValidatorsReported(uint256 indexed id, uint64 totalStopped);
+    event NodeOperatorTotalKeysTrimmed(uint256 indexed id, uint64 totalKeysTrimmed);
 
     /**
      * @notice Selects and returns at most `_numKeys` signing keys (as well as the corresponding
@@ -115,13 +115,49 @@ interface INodeOperatorsRegistry {
       */
     function addSigningKeys(uint256 _operator_id, uint256 _quantity, bytes _pubkeys, bytes _signatures) external;
 
+    /**
+      * @notice Add `_quantity` validator signing keys of operator #`_id` to the set of usable keys. Concatenated keys are: `_pubkeys`. Can be done by node operator in question by using the designated rewards address.
+      * @dev Along with each key the DAO has to provide a signatures for the
+      *      (pubkey, withdrawal_credentials, 32000000000) message.
+      *      Given that information, the contract'll be able to call
+      *      deposit_contract.deposit on-chain.
+      * @param _operator_id Node Operator id
+      * @param _quantity Number of signing keys provided
+      * @param _pubkeys Several concatenated validator signing keys
+      * @param _signatures Several concatenated signatures for (pubkey, withdrawal_credentials, 32000000000) messages
+      */
+    function addSigningKeysOperatorBH(uint256 _operator_id, uint256 _quantity, bytes _pubkeys, bytes _signatures) external;
+
     /**
       * @notice Removes a validator signing key #`_index` from the keys of the node operator #`_operator_id`
       * @param _operator_id Node Operator id
       * @param _index Index of the key, starting with 0
       */
     function removeSigningKey(uint256 _operator_id, uint256 _index) external;
 
+    /**
+      * @notice Removes a validator signing key #`_index` of operator #`_id` from the set of usable keys. Executed on behalf of Node Operator.
+      * @param _operator_id Node Operator id
+      * @param _index Index of the key, starting with 0
+      */
+    function removeSigningKeyOperatorBH(uint256 _operator_id, uint256 _index) external;
+
+    /**
+      * @notice Removes an #`_amount` of validator signing keys starting from #`_index` of operator #`_id` usable keys. Executed on behalf of DAO.
+      * @param _operator_id Node Operator id
+      * @param _index Index of the key, starting with 0
+      * @param _amount Number of keys to remove
+      */
+    function removeSigningKeys(uint256 _operator_id, uint256 _index, uint256 _amount) external;
+
+    /**
+      * @notice Removes an #`_amount` of validator signing keys starting from #`_index` of operator #`_id` usable keys. Executed on behalf of Node Operator.
+      * @param _operator_id Node Operator id
+      * @param _index Index of the key, starting with 0
+      * @param _amount Number of keys to remove
+      */
+    function removeSigningKeysOperatorBH(uint256 _operator_id, uint256 _index, uint256 _amount) external;
+
     /**
       * @notice Returns total number of signing keys of the node operator #`_operator_id`
       */
@@ -143,6 +179,18 @@ interface INodeOperatorsRegistry {
     function getSigningKey(uint256 _operator_id, uint256 _index) external view returns
             (bytes key, bytes depositSignature, bool used);
 
+
+    /**
+     * @notice Returns a monotonically increasing counter that gets incremented when any of the following happens:
+     *   1. a node operator's key(s) is added;
+     *   2. a node operator's key(s) is removed;
+     *   3. a node operator's approved keys limit is changed.
+     *   4. a node operator was activated/deactivated. Activation or deactivation of node operator
+     *      might lead to usage of unvalidated keys in the assignNextSigningKeys method.
+     */
+    function getKeysOpIndex() external view returns (uint256);
+
     event SigningKeyAdded(uint256 indexed operatorId, bytes pubkey);
     event SigningKeyRemoved(uint256 indexed operatorId, bytes pubkey);
+    event KeysOpIndexSet(uint256 keysOpIndex);
 }

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

@@ -79,6 +79,9 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
     /// @dev link to the Lido contract
     bytes32 internal constant LIDO_POSITION = keccak256("lido.NodeOperatorsRegistry.lido");
 
+    /// @dev link to the index of operations with keys
+    bytes32 internal constant KEYS_OP_INDEX_POSITION = keccak256("lido.NodeOperatorsRegistry.keysOpIndex");
+
 
     modifier onlyLido() {
         require(msg.sender == LIDO_POSITION.getStorageAddress(), "APP_AUTH_FAILED");
@@ -98,18 +101,18 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
     function initialize(address _lido) public onlyInit {
         TOTAL_OPERATORS_COUNT_POSITION.setStorageUint256(0);
         ACTIVE_OPERATORS_COUNT_POSITION.setStorageUint256(0);
+        KEYS_OP_INDEX_POSITION.setStorageUint256(0);
         LIDO_POSITION.setStorageAddress(_lido);
         initialized();
     }
 
     /**
-      * @notice Add node operator named `_name` with reward address `_rewardAddress` and staking limit `_stakingLimit`
+      * @notice Add node operator named `_name` with reward address `_rewardAddress` and staking limit = 0
       * @param _name Human-readable name
       * @param _rewardAddress Ethereum 1 address which receives stETH rewards for this operator
-      * @param _stakingLimit the maximum number of validators to stake for this operator
       * @return a unique key of the added operator
       */
-    function addNodeOperator(string _name, address _rewardAddress, uint64 _stakingLimit) external
+    function addNodeOperator(string _name, address _rewardAddress) external
         auth(ADD_NODE_OPERATOR_ROLE)
         validAddress(_rewardAddress)
         returns (uint256 id)
@@ -125,9 +128,9 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
         operator.active = true;
         operator.name = _name;
         operator.rewardAddress = _rewardAddress;
-        operator.stakingLimit = _stakingLimit;
+        operator.stakingLimit = 0;
 
-        emit NodeOperatorAdded(id, _name, _rewardAddress, _stakingLimit);
+        emit NodeOperatorAdded(id, _name, _rewardAddress, 0);
 
         return id;
     }
@@ -139,6 +142,7 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
         authP(SET_NODE_OPERATOR_ACTIVE_ROLE, arr(_id, _active ? uint256(1) : uint256(0)))
         operatorExists(_id)
     {
+        _increaseKeysOpIndex();
         if (operators[_id].active != _active) {
             uint256 activeOperatorsCount = getActiveNodeOperatorsCount();
             if (_active)
@@ -182,6 +186,7 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
         authP(SET_NODE_OPERATOR_LIMIT_ROLE, arr(_id, uint256(_stakingLimit)))
         operatorExists(_id)
     {
+        _increaseKeysOpIndex();
         operators[_id].stakingLimit = _stakingLimit;
         emit NodeOperatorStakingLimitSet(_id, _stakingLimit);
     }
@@ -207,8 +212,12 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
     function trimUnusedKeys() external onlyLido {
         uint256 length = getNodeOperatorsCount();
         for (uint256 operatorId = 0; operatorId < length; ++operatorId) {
-            if (operators[operatorId].totalSigningKeys != operators[operatorId].usedSigningKeys)  // write only if update is needed
-                operators[operatorId].totalSigningKeys = operators[operatorId].usedSigningKeys;  // discard unused keys
+            uint64 totalSigningKeys = operators[operatorId].totalSigningKeys;
+            uint64 usedSigningKeys = operators[operatorId].usedSigningKeys;
+            if (totalSigningKeys != usedSigningKeys) { // write only if update is needed
+                operators[operatorId].totalSigningKeys = usedSigningKeys;  // discard unused keys
+                emit NodeOperatorTotalKeysTrimmed(operatorId, totalSigningKeys - usedSigningKeys);
+            }
         }
     }
 
@@ -264,6 +273,22 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
         _removeSigningKey(_operator_id, _index);
     }
 
+    /**
+      * @notice Removes an #`_amount` of validator signing keys starting from #`_index` of operator #`_id` usable keys. Executed on behalf of DAO.
+      * @param _operator_id Node Operator id
+      * @param _index Index of the key, starting with 0
+      * @param _amount Number of keys to remove
+      */
+    function removeSigningKeys(uint256 _operator_id, uint256 _index, uint256 _amount)
+        external
+        authP(MANAGE_SIGNING_KEYS, arr(_operator_id))
+    {
+        // removing from the last index to the highest one, so we won't get outside the array
+        for (uint256 i = _index + _amount; i > _index ; --i) {
+            _removeSigningKey(_operator_id, i - 1);
+        }
+    }
+
     /**
       * @notice Removes a validator signing key #`_index` of operator #`_id` from the set of usable keys. Executed on behalf of Node Operator.
       * @param _operator_id Node Operator id
@@ -274,6 +299,20 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
         _removeSigningKey(_operator_id, _index);
     }
 
+    /**
+      * @notice Removes an #`_amount` of validator signing keys starting from #`_index` of operator #`_id` usable keys. Executed on behalf of Node Operator.
+      * @param _operator_id Node Operator id
+      * @param _index Index of the key, starting with 0
+      * @param _amount Number of keys to remove
+      */
+    function removeSigningKeysOperatorBH(uint256 _operator_id, uint256 _index, uint256 _amount) external {
+        require(msg.sender == operators[_operator_id].rewardAddress, "APP_AUTH_FAILED");
+        // removing from the last index to the highest one, so we won't get outside the array
+        for (uint256 i = _index + _amount; i > _index ; --i) {
+            _removeSigningKey(_operator_id, i - 1);
+        }
+    }
+
     /**
      * @notice Selects and returns at most `_numKeys` signing keys (as well as the corresponding
      *         signatures) from the set of active keys and marks the selected keys as used.
@@ -484,6 +523,18 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
         return TOTAL_OPERATORS_COUNT_POSITION.getStorageUint256();
     }
 
+    /**
+     * @notice Returns a monotonically increasing counter that gets incremented when any of the following happens:
+     *   1. a node operator's key(s) is added;
+     *   2. a node operator's key(s) is removed;
+     *   3. a node operator's approved keys limit is changed.
+     *   4. a node operator was activated/deactivated. Activation or deactivation of node operator
+     *      might lead to usage of unvalidated keys in the assignNextSigningKeys method.
+     */
+    function getKeysOpIndex() public view returns (uint256) {
+        return KEYS_OP_INDEX_POSITION.getStorageUint256();
+    }
+
     function _isEmptySigningKey(bytes memory _key) internal pure returns (bool) {
         assert(_key.length == PUBKEY_LENGTH);
         // algorithm applicability constraint
@@ -540,6 +591,8 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
         require(_pubkeys.length == _quantity.mul(PUBKEY_LENGTH), "INVALID_LENGTH");
         require(_signatures.length == _quantity.mul(SIGNATURE_LENGTH), "INVALID_LENGTH");
 
+        _increaseKeysOpIndex();
+
         for (uint256 i = 0; i < _quantity; ++i) {
             bytes memory key = BytesLib.slice(_pubkeys, i * PUBKEY_LENGTH, PUBKEY_LENGTH);
             require(!_isEmptySigningKey(key), "EMPTY_KEY");
@@ -558,6 +611,8 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
         require(_index < operators[_operator_id].totalSigningKeys, "KEY_NOT_FOUND");
         require(_index >= operators[_operator_id].usedSigningKeys, "KEY_WAS_USED");
 
+        _increaseKeysOpIndex();
+
         (bytes memory removedKey, ) = _loadSigningKey(_operator_id, _index);
 
         uint256 lastIndex = operators[_operator_id].totalSigningKeys.sub(1);
@@ -569,6 +624,11 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
         _deleteSigningKey(_operator_id, lastIndex);
         operators[_operator_id].totalSigningKeys = operators[_operator_id].totalSigningKeys.sub(1);
 
+        if (_index < operators[_operator_id].stakingLimit) {
+            // decreasing the staking limit so the key at _index can't be used anymore
+            operators[_operator_id].stakingLimit = uint64(_index);
+        }
+
         emit SigningKeyRemoved(_operator_id, removedKey);
     }
 
@@ -634,4 +694,10 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
 
         return cache;
     }
+
+    function _increaseKeysOpIndex() internal {
+        uint256 keysOpIndex = getKeysOpIndex();
+        KEYS_OP_INDEX_POSITION.setStorageUint256(keysOpIndex + 1);
+        emit KeysOpIndexSet(keysOpIndex + 1);
+    }
 }

contracts/0.4.24/test_helpers/DepositContractMock.sol

@@ -20,6 +20,7 @@ contract DepositContractMock is IDepositContract {
     }
 
     Call[] public calls;
+    bytes32 internal depositRoot;
 
     function deposit(
         bytes /* 48 */ pubkey,
@@ -40,4 +41,12 @@ contract DepositContractMock is IDepositContract {
     function reset() external {
         calls.length = 0;
     }
+
+    function get_deposit_root() external view returns (bytes32) {
+        return depositRoot;
+    }
+
+    function set_deposit_root(bytes32 _newRoot) external {
+        depositRoot = _newRoot;
+    }
 }