From 071851dd277308f5565b7b549881373ece98ee2f Mon Sep 17 00:00:00 2001 From: Artyom Veremeenko Date: Tue, 18 Jan 2022 11:42:35 +0300 Subject: [PATCH 01/15] DepositSecurityModule, _addGuardian: add check for zero address --- contracts/0.8.9/DepositSecurityModule.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/0.8.9/DepositSecurityModule.sol b/contracts/0.8.9/DepositSecurityModule.sol index dc6ad889f..b29489c93 100644 --- a/contracts/0.8.9/DepositSecurityModule.sol +++ b/contracts/0.8.9/DepositSecurityModule.sol @@ -280,6 +280,7 @@ contract DepositSecurityModule { } function _addGuardian(address addr) internal { + require(addr != address(0), "guardian zero address"); require(!_isGuardian(addr), "duplicate address"); guardians.push(addr); guardianIndicesOneBased[addr] = guardians.length; From 6dea01fd491c574a930b6ae22e94be63e5b26c74 Mon Sep 17 00:00:00 2001 From: Artyom Veremeenko Date: Tue, 18 Jan 2022 12:04:31 +0300 Subject: [PATCH 02/15] extract hardcoded `10000` into TOTAL_BASIS_POINTS internal constant --- contracts/0.4.24/Lido.sol | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/contracts/0.4.24/Lido.sol b/contracts/0.4.24/Lido.sol index 60b8d7cd1..23e9215a4 100644 --- a/contracts/0.4.24/Lido.sol +++ b/contracts/0.4.24/Lido.sol @@ -55,6 +55,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp { uint256 constant public DEPOSIT_SIZE = 32 ether; uint256 internal constant DEPOSIT_AMOUNT_UNIT = 1000000000 wei; + uint256 internal constant TOTAL_BASIS_POINTS = 10000; /// @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 = 150; @@ -186,7 +187,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp { external auth(MANAGE_FEE) { require( - 10000 == uint256(_treasuryFeeBasisPoints) + TOTAL_BASIS_POINTS == uint256(_treasuryFeeBasisPoints) .add(uint256(_insuranceFeeBasisPoints)) .add(uint256(_operatorsFeeBasisPoints)), "FEES_DONT_ADD_UP" @@ -554,7 +555,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp { // We need to take a defined percentage of the reported reward as a fee, and we do // this by minting new token shares and assigning them to the fee recipients (see // StETH docs for the explanation of the shares mechanics). The staking rewards fee - // is defined in basis points (1 basis point is equal to 0.01%, 10000 is 100%). + // is defined in basis points (1 basis point is equal to 0.01%, 10000 (TOTAL_BASIS_POINTS) is 100%). // // Since we've increased totalPooledEther by _totalRewards (which is already // performed by the time this function is called), the combined cost of all holders' @@ -564,14 +565,14 @@ contract Lido is ILido, IsContract, StETH, AragonApp { // Now we want to mint new shares to the fee recipient, so that the total cost of the // newly-minted shares exactly corresponds to the fee taken: // - // shares2mint * newShareCost = (_totalRewards * feeBasis) / 10000 + // shares2mint * newShareCost = (_totalRewards * feeBasis) / TOTAL_BASIS_POINTS // newShareCost = newTotalPooledEther / (prevTotalShares + shares2mint) // // which follows to: // // _totalRewards * feeBasis * prevTotalShares // shares2mint = -------------------------------------------------------------- - // (newTotalPooledEther * 10000) - (feeBasis * _totalRewards) + // (newTotalPooledEther * TOTAL_BASIS_POINTS) - (feeBasis * _totalRewards) // // The effect is that the given percentage of the reward goes to the fee recipient, and // the rest of the reward is distributed between token holders proportionally to their @@ -580,7 +581,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp { uint256 shares2mint = ( _totalRewards.mul(feeBasis).mul(_getTotalShares()) .div( - _getTotalPooledEther().mul(10000) + _getTotalPooledEther().mul(TOTAL_BASIS_POINTS) .sub(feeBasis.mul(_totalRewards)) ) ); @@ -591,13 +592,13 @@ contract Lido is ILido, IsContract, StETH, AragonApp { (,uint16 insuranceFeeBasisPoints, uint16 operatorsFeeBasisPoints) = _getFeeDistribution(); - uint256 toInsuranceFund = shares2mint.mul(insuranceFeeBasisPoints).div(10000); + uint256 toInsuranceFund = shares2mint.mul(insuranceFeeBasisPoints).div(TOTAL_BASIS_POINTS); address insuranceFund = getInsuranceFund(); _transferShares(address(this), insuranceFund, toInsuranceFund); _emitTransferAfterMintingShares(insuranceFund, toInsuranceFund); uint256 distributedToOperatorsShares = _distributeNodeOperatorsReward( - shares2mint.mul(operatorsFeeBasisPoints).div(10000) + shares2mint.mul(operatorsFeeBasisPoints).div(TOTAL_BASIS_POINTS) ); // Transfer the rest of the fee to treasury @@ -652,7 +653,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp { * @dev Write a value nominated in basis points */ function _setBPValue(bytes32 _slot, uint16 _value) internal { - require(_value <= 10000, "VALUE_OVER_100_PERCENT"); + require(_value <= TOTAL_BASIS_POINTS, "VALUE_OVER_100_PERCENT"); _slot.setStorageUint256(uint256(_value)); } @@ -679,7 +680,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp { */ function _readBPValue(bytes32 _slot) internal view returns (uint16) { uint256 v = _slot.getStorageUint256(); - assert(v <= 10000); + assert(v <= TOTAL_BASIS_POINTS); return uint16(v); } From 3b11426286aa3605cb26acca1cb6e26e0f7e8fa6 Mon Sep 17 00:00:00 2001 From: Alexey Potapkin Date: Tue, 18 Jan 2022 19:11:03 +0300 Subject: [PATCH 03/15] Use UINT64_MAX constant. Fix #382 --- contracts/0.4.24/nos/NodeOperatorsRegistry.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol index bc12ca570..a26755d82 100644 --- a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol +++ b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol @@ -551,7 +551,7 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp } function to64(uint256 v) internal pure returns (uint64) { - assert(v <= uint256(uint64(-1))); + assert(v <= UINT64_MAX); return uint64(v); } From a6d264aef7e2ee22441ec3ef0afb378babecfbf7 Mon Sep 17 00:00:00 2001 From: Alexey Potapkin Date: Wed, 19 Jan 2022 09:55:56 +0300 Subject: [PATCH 04/15] Remove extra defensive aserts for constants. Fix #383 --- contracts/0.4.24/nos/NodeOperatorsRegistry.sol | 9 --------- 1 file changed, 9 deletions(-) diff --git a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol index a26755d82..f7b2fdd6e 100644 --- a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol +++ b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol @@ -537,8 +537,6 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp function _isEmptySigningKey(bytes memory _key) internal pure returns (bool) { assert(_key.length == PUBKEY_LENGTH); - // algorithm applicability constraint - assert(PUBKEY_LENGTH >= 32 && PUBKEY_LENGTH <= 64); uint256 k1; uint256 k2; @@ -562,9 +560,6 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp function _storeSigningKey(uint256 _operator_id, uint256 _keyIndex, bytes memory _key, bytes memory _signature) internal { assert(_key.length == PUBKEY_LENGTH); assert(_signature.length == SIGNATURE_LENGTH); - // algorithm applicability constraints - assert(PUBKEY_LENGTH >= 32 && PUBKEY_LENGTH <= 64); - assert(0 == SIGNATURE_LENGTH % 32); // key uint256 offset = _signingKeyOffset(_operator_id, _keyIndex); @@ -642,10 +637,6 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp } function _loadSigningKey(uint256 _operator_id, uint256 _keyIndex) internal view returns (bytes memory key, bytes memory signature) { - // algorithm applicability constraints - assert(PUBKEY_LENGTH >= 32 && PUBKEY_LENGTH <= 64); - assert(0 == SIGNATURE_LENGTH % 32); - uint256 offset = _signingKeyOffset(_operator_id, _keyIndex); // key From 9372b0dd1c558d33c0e7c50450a691751defe14f Mon Sep 17 00:00:00 2001 From: Alexey Potapkin Date: Wed, 19 Jan 2022 10:03:14 +0300 Subject: [PATCH 05/15] Move storage assigning under if-branch. Fix #390 --- contracts/0.4.24/nos/NodeOperatorsRegistry.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol index f7b2fdd6e..ee5eca780 100644 --- a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol +++ b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol @@ -149,9 +149,9 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp ACTIVE_OPERATORS_COUNT_POSITION.setStorageUint256(activeOperatorsCount.add(1)); else ACTIVE_OPERATORS_COUNT_POSITION.setStorageUint256(activeOperatorsCount.sub(1)); - } - operators[_id].active = _active; + operators[_id].active = _active; + } emit NodeOperatorActiveSet(_id, _active); } From f03fce328d92810027486c1c73797a22b8f932b0 Mon Sep 17 00:00:00 2001 From: Alexey Potapkin Date: Wed, 19 Jan 2022 10:06:40 +0300 Subject: [PATCH 06/15] Rename a variable. Fix #387 --- contracts/0.4.24/nos/NodeOperatorsRegistry.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol index ee5eca780..1e9dbf1da 100644 --- a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol +++ b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol @@ -438,10 +438,10 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp if (effectiveStakeTotal == 0) return (recipients, shares); - uint256 perValidatorReward = _totalRewardShares.div(effectiveStakeTotal); + uint256 perStakeReward = _totalRewardShares.div(effectiveStakeTotal); for (idx = 0; idx < activeCount; ++idx) { - shares[idx] = shares[idx].mul(perValidatorReward); + shares[idx] = shares[idx].mul(perStakeReward); } return (recipients, shares); From d17b86d118206687bcf2a701ebc5a9f2ee85121e Mon Sep 17 00:00:00 2001 From: Alexey Potapkin Date: Wed, 19 Jan 2022 11:03:34 +0300 Subject: [PATCH 07/15] Add comments to internal functions. Fix #388 --- contracts/0.4.24/Lido.sol | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/contracts/0.4.24/Lido.sol b/contracts/0.4.24/Lido.sol index 23e9215a4..a036225f8 100644 --- a/contracts/0.4.24/Lido.sol +++ b/contracts/0.4.24/Lido.sol @@ -299,7 +299,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp { uint256 balance; if (_token == ETH) { balance = _getUnaccountedEther(); - // Transfer replaced by call to prevent transfer gas amount issue + // Transfer replaced by call to prevent transfer gas amount issue require(vault.call.value(balance)(), "RECOVER_TRANSFER_FAILED"); } else { ERC20 token = ERC20(_token); @@ -425,11 +425,19 @@ contract Lido is ILido, IsContract, StETH, AragonApp { NODE_OPERATORS_REGISTRY_POSITION.setStorageAddress(_r); } + /** + * @dev Internal function to set treasury address + * @param _treasury treasury address + */ function _setTreasury(address _treasury) internal { require(_treasury != address(0), "SET_TREASURY_ZERO_ADDRESS"); TREASURY_POSITION.setStorageAddress(_treasury); } + /** + * @dev Internal function to set insurance fund address + * @param _insuranceFund insurance fund address + */ function _setInsuranceFund(address _insuranceFund) internal { require(_insuranceFund != address(0), "SET_INSURANCE_FUND_ZERO_ADDRESS"); INSURANCE_FUND_POSITION.setStorageAddress(_insuranceFund); @@ -609,6 +617,11 @@ contract Lido is ILido, IsContract, StETH, AragonApp { _emitTransferAfterMintingShares(treasury, toTreasury); } + /** + * @dev Internal function to distribute reward to node operators + * @param _sharesToDistribute amount of shares to distribute + * @return actual amount of shares that was transferred to node operators as a reward + */ function _distributeNodeOperatorsReward(uint256 _sharesToDistribute) internal returns (uint256 distributed) { (address[] memory recipients, uint256[] memory shares) = getOperators().getRewardsDistribution(_sharesToDistribute); From 517d379ebbe08d31ae8decbde5751390e957d8a0 Mon Sep 17 00:00:00 2001 From: Alexey Potapkin Date: Wed, 19 Jan 2022 11:51:44 +0300 Subject: [PATCH 08/15] Remove explicit variables initialization. Fix #384 --- contracts/0.8.9/DepositSecurityModule.sol | 4 ---- 1 file changed, 4 deletions(-) diff --git a/contracts/0.8.9/DepositSecurityModule.sol b/contracts/0.8.9/DepositSecurityModule.sol index b29489c93..25b49a492 100644 --- a/contracts/0.8.9/DepositSecurityModule.sol +++ b/contracts/0.8.9/DepositSecurityModule.sol @@ -94,12 +94,8 @@ contract DepositSecurityModule { _setMaxDeposits(_maxDepositsPerBlock); _setMinDepositBlockDistance(_minDepositBlockDistance); _setPauseIntentValidityPeriodBlocks(_pauseIntentValidityPeriodBlocks); - - paused = false; - lastDepositBlock = 0; } - /** * Returns the owner address. */ From 638a9c20b63ff8dddead2e882bf4909b38708596 Mon Sep 17 00:00:00 2001 From: Alexey Potapkin Date: Wed, 19 Jan 2022 16:21:58 +0300 Subject: [PATCH 09/15] Fix comments not to mention non-existent constants. Fix #385 --- contracts/0.8.9/DepositSecurityModule.sol | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/contracts/0.8.9/DepositSecurityModule.sol b/contracts/0.8.9/DepositSecurityModule.sol index 25b49a492..a8572fceb 100644 --- a/contracts/0.8.9/DepositSecurityModule.sol +++ b/contracts/0.8.9/DepositSecurityModule.sol @@ -143,14 +143,14 @@ contract DepositSecurityModule { /** - * Returns `PAUSE_INTENT_VALIDITY_PERIOD_BLOCKS` (see `pauseDeposits`). + * Returns current `pauseIntentValidityPeriodBlocks` contract parameter (see `pauseDeposits`). */ function getPauseIntentValidityPeriodBlocks() external view returns (uint256) { return pauseIntentValidityPeriodBlocks; } /** - * Sets `PAUSE_INTENT_VALIDITY_PERIOD_BLOCKS`. Only callable by the owner. + * Sets `pauseIntentValidityPeriodBlocks`. Only callable by the owner. */ function setPauseIntentValidityPeriodBlocks(uint256 newValue) external onlyOwner { _setPauseIntentValidityPeriodBlocks(newValue); @@ -164,14 +164,14 @@ contract DepositSecurityModule { /** - * Returns `MAX_DEPOSITS_PER_BLOCK` (see `depositBufferedEther`). + * Returns `maxDepositsPerBlock` (see `depositBufferedEther`). */ function getMaxDeposits() external view returns (uint256) { return maxDepositsPerBlock; } /** - * Sets `MAX_DEPOSITS_PER_BLOCK`. Only callable by the owner. + * Sets `maxDepositsPerBlock`. Only callable by the owner. */ function setMaxDeposits(uint256 newValue) external onlyOwner { _setMaxDeposits(newValue); @@ -184,14 +184,14 @@ contract DepositSecurityModule { /** - * Returns `MIN_DEPOSIT_BLOCK_DISTANCE` (see `depositBufferedEther`). + * Returns `minDepositBlockDistance` (see `depositBufferedEther`). */ function getMinDepositBlockDistance() external view returns (uint256) { return minDepositBlockDistance; } /** - * Sets `MIN_DEPOSIT_BLOCK_DISTANCE`. Only callable by the owner. + * Sets `minDepositBlockDistance`. Only callable by the owner. */ function setMinDepositBlockDistance(uint256 newValue) external onlyOwner { _setMinDepositBlockDistance(newValue); @@ -324,7 +324,7 @@ contract DepositSecurityModule { * is a valid signature by the guardian with index guardianIndex of the data * defined below. * - * 2. block.number - blockNumber <= PAUSE_INTENT_VALIDITY_PERIOD_BLOCKS + * 2. block.number - blockNumber <= pauseIntentValidityPeriodBlocks * * The signature, if present, must be produced for keccak256 hash of the following * message (each component taking 32 bytes): @@ -387,14 +387,14 @@ contract DepositSecurityModule { /** - * Calls Lido.depositBufferedEther(MAX_DEPOSITS_PER_BLOCK). + * Calls Lido.depositBufferedEther(maxDepositsPerBlock). * * Reverts if any of the following is true: * 1. IDepositContract.get_deposit_root() != depositRoot. * 2. INodeOperatorsRegistry.getKeysOpIndex() != keysOpIndex. * 3. The number of guardian signatures is less than getGuardianQuorum(). * 4. An invalid or non-guardian signature received. - * 5. block.number - getLastDepositBlock() < MIN_DEPOSIT_BLOCK_DISTANCE. + * 5. block.number - getLastDepositBlock() < minDepositBlockDistance. * 6. blockhash(blockNumber) != blockHash. * * Signatures must be sorted in ascending order by index of the guardian. Each signature must From b9bbc48702cf24475c3f1e8cb8259e7175e86783 Mon Sep 17 00:00:00 2001 From: Artyom Veremeenko Date: Wed, 19 Jan 2022 21:56:26 +0300 Subject: [PATCH 10/15] NodeOperatorsRegistry: add requires to 4 set functions --- contracts/0.4.24/nos/NodeOperatorsRegistry.sol | 18 +++++++++++------- test/0.4.24/node-operators-registry.test.js | 9 +++++++-- test/scenario/lido_penalties_slashing.js | 5 ++++- .../scenario/lido_rewards_distribution_math.js | 7 +++++-- 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol index bc12ca570..dca71a7e0 100644 --- a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol +++ b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol @@ -142,14 +142,15 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp authP(SET_NODE_OPERATOR_ACTIVE_ROLE, arr(_id, _active ? uint256(1) : uint256(0))) operatorExists(_id) { + require(operators[_id].active != _active, "NODE_OPERATOR_ACTIVITY_ALREADY_SET"); + _increaseKeysOpIndex(); - if (operators[_id].active != _active) { - uint256 activeOperatorsCount = getActiveNodeOperatorsCount(); - if (_active) - ACTIVE_OPERATORS_COUNT_POSITION.setStorageUint256(activeOperatorsCount.add(1)); - else - ACTIVE_OPERATORS_COUNT_POSITION.setStorageUint256(activeOperatorsCount.sub(1)); - } + + uint256 activeOperatorsCount = getActiveNodeOperatorsCount(); + if (_active) + ACTIVE_OPERATORS_COUNT_POSITION.setStorageUint256(activeOperatorsCount.add(1)); + else + ACTIVE_OPERATORS_COUNT_POSITION.setStorageUint256(activeOperatorsCount.sub(1)); operators[_id].active = _active; @@ -163,6 +164,7 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp authP(SET_NODE_OPERATOR_NAME_ROLE, arr(_id)) operatorExists(_id) { + require(keccak256(operators[_id].name) != keccak256(_name), "NODE_OPERATOR_NAME_IS_THE_SAME"); operators[_id].name = _name; emit NodeOperatorNameSet(_id, _name); } @@ -175,6 +177,7 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp operatorExists(_id) validAddress(_rewardAddress) { + require(operators[_id].rewardAddress != _rewardAddress, "NODE_OPERATOR_ADDRESS_IS_THE_SAME"); operators[_id].rewardAddress = _rewardAddress; emit NodeOperatorRewardAddressSet(_id, _rewardAddress); } @@ -186,6 +189,7 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp authP(SET_NODE_OPERATOR_LIMIT_ROLE, arr(_id, uint256(_stakingLimit))) operatorExists(_id) { + require(operators[_id].stakingLimit != _stakingLimit, "NODE_OPERATOR_STAKING_LIMIT_IS_THE_SAME"); _increaseKeysOpIndex(); operators[_id].stakingLimit = _stakingLimit; emit NodeOperatorStakingLimitSet(_id, _stakingLimit); diff --git a/test/0.4.24/node-operators-registry.test.js b/test/0.4.24/node-operators-registry.test.js index bb60450ba..05ddb4028 100644 --- a/test/0.4.24/node-operators-registry.test.js +++ b/test/0.4.24/node-operators-registry.test.js @@ -138,7 +138,7 @@ contract('NodeOperatorsRegistry', ([appManager, voting, user1, user2, user3, nob assertBn(await app.getNodeOperatorsCount({ from: nobody }), 2) assertBn(await app.getActiveNodeOperatorsCount({ from: nobody }), 1) - await app.setNodeOperatorActive(0, false, { from: voting }) + await assertRevert(app.setNodeOperatorActive(0, false, { from: voting }), 'NODE_OPERATOR_ACTIVITY_ALREADY_SET') assert.equal((await app.getNodeOperator(0, false)).active, false) assertBn(await app.getActiveNodeOperatorsCount({ from: nobody }), 1) @@ -156,7 +156,7 @@ contract('NodeOperatorsRegistry', ([appManager, voting, user1, user2, user3, nob assertBn(await app.getNodeOperatorsCount({ from: nobody }), 2) assertBn(await app.getActiveNodeOperatorsCount({ from: nobody }), 1) - await app.setNodeOperatorActive(0, true, { from: voting }) + await assertRevert(app.setNodeOperatorActive(0, true, { from: voting }), 'NODE_OPERATOR_ACTIVITY_ALREADY_SET') assert.equal((await app.getNodeOperator(0, false)).active, true) assertBn(await app.getActiveNodeOperatorsCount({ from: nobody }), 1) @@ -174,6 +174,8 @@ contract('NodeOperatorsRegistry', ([appManager, voting, user1, user2, user3, nob assert.equal((await app.getNodeOperator(1, true)).name, ' bar') await app.setNodeOperatorName(0, 'zzz', { from: voting }) + await assertRevert(app.setNodeOperatorName(0, 'zzz', { from: voting }), 'NODE_OPERATOR_NAME_IS_THE_SAME') + assert.equal((await app.getNodeOperator(0, true)).name, 'zzz') assert.equal((await app.getNodeOperator(1, true)).name, ' bar') @@ -191,6 +193,7 @@ contract('NodeOperatorsRegistry', ([appManager, voting, user1, user2, user3, nob assert.equal((await app.getNodeOperator(1, false)).rewardAddress, ADDRESS_2) await app.setNodeOperatorRewardAddress(0, ADDRESS_4, { from: voting }) + await assertRevert(app.setNodeOperatorRewardAddress(0, ADDRESS_4, { from: voting }), 'NODE_OPERATOR_ADDRESS_IS_THE_SAME') assert.equal((await app.getNodeOperator(0, false)).rewardAddress, ADDRESS_4) assert.equal((await app.getNodeOperator(1, false)).rewardAddress, ADDRESS_2) @@ -209,6 +212,7 @@ contract('NodeOperatorsRegistry', ([appManager, voting, user1, user2, user3, nob assertBn((await app.getNodeOperator(1, false)).stakingLimit, 0) await app.setNodeOperatorStakingLimit(0, 40, { from: voting }) + await assertRevert(app.setNodeOperatorStakingLimit(0, 40, { from: voting }), 'NODE_OPERATOR_STAKING_LIMIT_IS_THE_SAME') assertBn((await app.getNodeOperator(0, false)).stakingLimit, 40) assertBn((await app.getNodeOperator(1, false)).stakingLimit, 0) @@ -310,6 +314,7 @@ contract('NodeOperatorsRegistry', ([appManager, voting, user1, user2, user3, nob assert.sameMembers(hexSplit(keysAssignedEvt.signatures, SIGNATURE_LENGTH_BYTES), [op0.sigs[0], op1.sigs[0]], 'assignment 1: signatures') await app.setNodeOperatorActive(0, false, { from: voting }) + await assertRevert(app.setNodeOperatorActive(0, false, { from: voting }), 'NODE_OPERATOR_ACTIVITY_ALREADY_SET') result = await pool.assignNextSigningKeys(2) keysAssignedEvt = getEventAt(result, 'KeysAssigned').args diff --git a/test/scenario/lido_penalties_slashing.js b/test/scenario/lido_penalties_slashing.js index d21b2539c..79c651542 100644 --- a/test/scenario/lido_penalties_slashing.js +++ b/test/scenario/lido_penalties_slashing.js @@ -117,7 +117,10 @@ contract('Lido: penalties, slashing, operator stops', (addresses) => { const validatorsLimit = 0 const txn = await nodeOperatorRegistry.addNodeOperator(nodeOperator1.name, nodeOperator1.address, { from: voting }) - await nodeOperatorRegistry.setNodeOperatorStakingLimit(0, validatorsLimit, { from: voting }) + await assertRevert( + nodeOperatorRegistry.setNodeOperatorStakingLimit(0, validatorsLimit, { from: voting }), + 'NODE_OPERATOR_STAKING_LIMIT_IS_THE_SAME' + ) // Some Truffle versions fail to decode logs here, so we're decoding them explicitly using a helper nodeOperator1.id = getEventArgument(txn, 'NodeOperatorAdded', 'id', { decodeForAbi: NodeOperatorsRegistry._json.abi }) diff --git a/test/scenario/lido_rewards_distribution_math.js b/test/scenario/lido_rewards_distribution_math.js index 232cd6c60..c8e65be97 100644 --- a/test/scenario/lido_rewards_distribution_math.js +++ b/test/scenario/lido_rewards_distribution_math.js @@ -1,6 +1,6 @@ const { assert } = require('chai') const { BN } = require('bn.js') -const { assertBn, assertEvent } = require('@aragon/contract-helpers-test/src/asserts') +const { assertBn, assertEvent, assertRevert } = require('@aragon/contract-helpers-test/src/asserts') const { getEventArgument, ZERO_ADDRESS } = require('@aragon/contract-helpers-test') const { pad, ETH } = require('../helpers/utils') @@ -120,7 +120,10 @@ contract('Lido: rewards distribution math', (addresses) => { const validatorsLimit = 0 const txn = await nodeOperatorRegistry.addNodeOperator(nodeOperator1.name, nodeOperator1.address, { from: voting }) - await nodeOperatorRegistry.setNodeOperatorStakingLimit(0, validatorsLimit, { from: voting }) + await assertRevert( + nodeOperatorRegistry.setNodeOperatorStakingLimit(0, validatorsLimit, { from: voting }), + 'NODE_OPERATOR_STAKING_LIMIT_IS_THE_SAME' + ) // Some Truffle versions fail to decode logs here, so we're decoding them explicitly using a helper nodeOperator1.id = getEventArgument(txn, 'NodeOperatorAdded', 'id', { decodeForAbi: NodeOperatorsRegistry._json.abi }) From 0b1e52e15e33fce914c2cd0e235da5a77dde4743 Mon Sep 17 00:00:00 2001 From: Artyom Veremeenko Date: Wed, 19 Jan 2022 22:36:11 +0300 Subject: [PATCH 11/15] dep-sec-module: add 2 more usages of SafeMath: _index.add(_amount) --- contracts/0.4.24/nos/NodeOperatorsRegistry.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol index dca71a7e0..1425f683c 100644 --- a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol +++ b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol @@ -288,7 +288,7 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp 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) { + for (uint256 i = _index.add(_amount); i > _index; --i) { _removeSigningKey(_operator_id, i - 1); } } @@ -312,7 +312,7 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp 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) { + for (uint256 i = _index.add(_amount); i > _index; --i) { _removeSigningKey(_operator_id, i - 1); } } From bf3c2611f086c891ad8b02dcc21670abb2021533 Mon Sep 17 00:00:00 2001 From: Artyom Veremeenko Date: Wed, 19 Jan 2022 22:40:55 +0300 Subject: [PATCH 12/15] dep-sec-module: add checks for zero addresses in constructor --- contracts/0.8.9/DepositSecurityModule.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/0.8.9/DepositSecurityModule.sol b/contracts/0.8.9/DepositSecurityModule.sol index b29489c93..f64898876 100644 --- a/contracts/0.8.9/DepositSecurityModule.sol +++ b/contracts/0.8.9/DepositSecurityModule.sol @@ -74,6 +74,8 @@ contract DepositSecurityModule { uint256 _minDepositBlockDistance, uint256 _pauseIntentValidityPeriodBlocks ) { + require(_lido != address(0x0), "LIDO_ZERO_ADDRESS"); + require(_depositContract != address(0x0), "DEPOSIT_CONTRACT_ZERO_ADDRESS"); LIDO = _lido; DEPOSIT_CONTRACT = _depositContract; From 0ce2dc2c8e9f7aea92edb6db8a5dccabbba4fb9b Mon Sep 17 00:00:00 2001 From: Artyom Veremeenko Date: Fri, 21 Jan 2022 10:31:34 +0300 Subject: [PATCH 13/15] wrn-13: fix omitted events --- contracts/0.4.24/Lido.sol | 35 ++++++++-------------- contracts/0.4.24/interfaces/ILido.sol | 29 ++++++++++++++++++ contracts/0.4.24/test_helpers/LidoMock.sol | 20 ++----------- test/0.4.24/lido.test.js | 21 +++++++------ 4 files changed, 53 insertions(+), 52 deletions(-) diff --git a/contracts/0.4.24/Lido.sol b/contracts/0.4.24/Lido.sol index a036225f8..f2361f711 100644 --- a/contracts/0.4.24/Lido.sol +++ b/contracts/0.4.24/Lido.sol @@ -85,12 +85,12 @@ contract Lido is ILido, IsContract, StETH, AragonApp { /** * @dev As AragonApp, Lido contract must be initialized with following variables: - * @param depositContract official ETH2 Deposit contract + * @param _depositContract official ETH2 Deposit contract * @param _oracle oracle contract * @param _operators instance of Node Operators Registry */ function initialize( - IDepositContract depositContract, + IDepositContract _depositContract, address _oracle, INodeOperatorsRegistry _operators, address _treasury, @@ -98,9 +98,13 @@ contract Lido is ILido, IsContract, StETH, AragonApp { ) public onlyInit { - _setDepositContract(depositContract); + require(isContract(address(_operators)), "NOT_A_CONTRACT"); + require(isContract(address(_depositContract)), "NOT_A_CONTRACT"); + + NODE_OPERATORS_REGISTRY_POSITION.setStorageAddress(_operators); + DEPOSIT_CONTRACT_POSITION.setStorageAddress(address(_depositContract)); + _setOracle(_oracle); - _setOperators(_operators); _setTreasury(_treasury); _setInsuranceFund(_insuranceFund); @@ -208,6 +212,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp { */ function setOracle(address _oracle) external auth(SET_ORACLE) { _setOracle(_oracle); + emit OracleSet(_oracle); } /** @@ -217,6 +222,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp { */ function setTreasury(address _treasury) external auth(SET_TREASURY) { _setTreasury(_treasury); + emit TreasurySet(_treasury); } /** @@ -226,6 +232,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp { */ function setInsuranceFund(address _insuranceFund) external auth(SET_INSURANCE_FUND) { _setInsuranceFund(_insuranceFund); + emit InsuranceFundSet(_insuranceFund); } /** @@ -398,15 +405,6 @@ contract Lido is ILido, IsContract, StETH, AragonApp { beaconBalance = BEACON_BALANCE_POSITION.getStorageUint256(); } - /** - * @dev Sets the address of Deposit contract - * @param _contract the address of Deposit contract - */ - function _setDepositContract(IDepositContract _contract) internal { - require(isContract(address(_contract)), "NOT_A_CONTRACT"); - DEPOSIT_CONTRACT_POSITION.setStorageAddress(address(_contract)); - } - /** * @dev Internal function to set authorized oracle address * @param _oracle oracle contract @@ -416,15 +414,6 @@ contract Lido is ILido, IsContract, StETH, AragonApp { ORACLE_POSITION.setStorageAddress(_oracle); } - /** - * @dev Internal function to set node operator registry address - * @param _r registry of node operators - */ - function _setOperators(INodeOperatorsRegistry _r) internal { - require(isContract(_r), "NOT_A_CONTRACT"); - NODE_OPERATORS_REGISTRY_POSITION.setStorageAddress(_r); - } - /** * @dev Internal function to set treasury address * @param _treasury treasury address @@ -557,7 +546,7 @@ contract Lido is ILido, IsContract, StETH, AragonApp { /** * @dev Distributes rewards by minting and distributing corresponding amount of liquid tokens. - * @param _totalRewards Total rewards accrued on the Ethereum 2.0 side in wei + * @param _totalRewards Total rewards accured on the Ethereum 2.0 side in wei */ function distributeRewards(uint256 _totalRewards) internal { // We need to take a defined percentage of the reported reward as a fee, and we do diff --git a/contracts/0.4.24/interfaces/ILido.sol b/contracts/0.4.24/interfaces/ILido.sol index 647fa3cb1..9c2224de2 100644 --- a/contracts/0.4.24/interfaces/ILido.sol +++ b/contracts/0.4.24/interfaces/ILido.sol @@ -35,6 +35,35 @@ interface ILido { event Resumed(); + /** + * @notice Set authorized oracle contract address to `_oracle` + * @dev Contract specified here is allowed to make periodical updates of beacon states + * by calling pushBeacon. + * @param _oracle oracle contract + */ + function setOracle(address _oracle) external; + + event OracleSet(address oracle); + + /** + * @notice Set treasury contract address to `_treasury` + * @dev Contract specified here is used to accumulate the protocol treasury fee. + * @param _treasury contract which accumulates treasury fee. + */ + function setTreasury(address _treasury) external; + + event TreasurySet(address treasury); + + /** + * @notice Set insuranceFund contract address to `_insuranceFund` + * @dev Contract specified here is used to accumulate the protocol insurance fee. + * @param _insuranceFund contract which accumulates insurance fee. + */ + function setInsuranceFund(address _insuranceFund) external; + + event InsuranceFundSet(address insuranceFund); + + /** * @notice Set fee rate to `_feeBasisPoints` basis points. The fees are accrued when oracles report staking results * @param _feeBasisPoints Fee rate, in basis points diff --git a/contracts/0.4.24/test_helpers/LidoMock.sol b/contracts/0.4.24/test_helpers/LidoMock.sol index 8ae94289a..8aa191157 100644 --- a/contracts/0.4.24/test_helpers/LidoMock.sol +++ b/contracts/0.4.24/test_helpers/LidoMock.sol @@ -13,14 +13,14 @@ import "./VaultMock.sol"; */ contract LidoMock is Lido { function initialize( - IDepositContract depositContract, + IDepositContract _depositContract, address _oracle, INodeOperatorsRegistry _operators ) public { super.initialize( - depositContract, + _depositContract, _oracle, _operators, new VaultMock(), @@ -53,22 +53,6 @@ contract LidoMock is Lido { return _toLittleEndian64(_value); } - /** - * @dev Public wrapper of internal fun. Internal function sets the address of Deposit contract - * @param _contract the address of Deposit contract - */ - function setDepositContract(IDepositContract _contract) public { - _setDepositContract(_contract); - } - - /** - * @dev Public wrapper of internal fun. Internal function sets node operator registry address - * @param _r registry of node operators - */ - function setOperators(INodeOperatorsRegistry _r) public { - _setOperators(_r); - } - /** * @dev Only for testing recovery vault */ diff --git a/test/0.4.24/lido.test.js b/test/0.4.24/lido.test.js index ec52c1efe..b71b9fe7f 100644 --- a/test/0.4.24/lido.test.js +++ b/test/0.4.24/lido.test.js @@ -92,7 +92,10 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody, depositor]) await acl.createPermission(depositor, app.address, await app.DEPOSIT_ROLE(), appManager, { from: appManager }) // Initialize the app's proxy. + await assertRevert(app.initialize(user1, oracle.address, operators.address), 'NOT_A_CONTRACT') + await assertRevert(app.initialize(depositContract.address, oracle.address, user1), 'NOT_A_CONTRACT') await app.initialize(depositContract.address, oracle.address, operators.address) + treasuryAddr = await app.getTreasury() insuranceAddr = await app.getInsuranceFund() @@ -157,15 +160,9 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody, depositor]) it('setOracle works', async () => { await assertRevert(app.setOracle(user1, { from: voting }), 'NOT_A_CONTRACT') - await app.setOracle(yetAnotherOracle.address, { from: voting }) - }) - - it('_setDepositContract reverts with invalid arg', async () => { - await assertRevert(app.setDepositContract(user1, { from: voting }), 'NOT_A_CONTRACT') - }) - - it('_setOperators reverts with invalid arg', async () => { - await assertRevert(app.setOperators(user1, { from: voting }), 'NOT_A_CONTRACT') + const receipt = await app.setOracle(yetAnotherOracle.address, { from: voting }) + assertEvent(receipt, 'OracleSet', { expectedArgs: { oracle: yetAnotherOracle.address } }) + assert.equal(await app.getOracle(), yetAnotherOracle.address) }) it('setWithdrawalCredentials resets unused keys', async () => { @@ -1082,7 +1079,8 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody, depositor]) }) it('voting can set treasury', async () => { - await app.setTreasury(user1, { from: voting }) + const receipt = await app.setTreasury(user1, { from: voting }) + assertEvent(receipt, 'TreasurySet', { expectedArgs: { treasury: user1 } }) assert.equal(await app.getTreasury(), user1) }) @@ -1102,7 +1100,8 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody, depositor]) }) it('voting can set insurance fund', async () => { - await app.setInsuranceFund(user1, { from: voting }) + const receipt = await app.setInsuranceFund(user1, { from: voting }) + assertEvent(receipt, 'InsuranceFundSet', { expectedArgs: { insuranceFund: user1 } }) assert.equal(await app.getInsuranceFund(), user1) }) From 7dcf3b4358818f6e64d51c1e49fd3e887f04f064 Mon Sep 17 00:00:00 2001 From: Artyom Veremeenko Date: Fri, 21 Jan 2022 10:32:51 +0300 Subject: [PATCH 14/15] wrn-5: add clarifying comment to `if (paused)` in `pauseDeposits` --- contracts/0.8.9/DepositSecurityModule.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/0.8.9/DepositSecurityModule.sol b/contracts/0.8.9/DepositSecurityModule.sol index 0906bc71f..724fc4e73 100644 --- a/contracts/0.8.9/DepositSecurityModule.sol +++ b/contracts/0.8.9/DepositSecurityModule.sol @@ -334,6 +334,10 @@ contract DepositSecurityModule { * | PAUSE_MESSAGE_PREFIX | blockNumber */ function pauseDeposits(uint256 blockNumber, Signature memory sig) external { + // In case of an emergency function `pauseDeposits` is supposed to be called + // by all guardians. Thus only the first call will do the actual change. But + // the other calls would be OK operations from the point of view of protocol’s logic. + // Thus we prefer not to use “error” semantics which is implied by `require`. if (paused) { return; } From bcae4249a0d9cf1e92858f92cf9fe5154dff8c21 Mon Sep 17 00:00:00 2001 From: Aleksei Potapkin Date: Sat, 22 Jan 2022 23:16:19 +0300 Subject: [PATCH 15/15] Fix naming in getRewardsDistribution (#395) --- contracts/0.4.24/nos/NodeOperatorsRegistry.sol | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol index 8af7a550a..e87898775 100644 --- a/contracts/0.4.24/nos/NodeOperatorsRegistry.sol +++ b/contracts/0.4.24/nos/NodeOperatorsRegistry.sol @@ -424,28 +424,28 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp shares = new uint256[](activeCount); uint256 idx = 0; - uint256 effectiveStakeTotal = 0; + uint256 activeValidatorsTotal = 0; for (uint256 operatorId = 0; operatorId < nodeOperatorCount; ++operatorId) { NodeOperator storage operator = operators[operatorId]; if (!operator.active) continue; - uint256 effectiveStake = operator.usedSigningKeys.sub(operator.stoppedValidators); - effectiveStakeTotal = effectiveStakeTotal.add(effectiveStake); + uint256 activeValidators = operator.usedSigningKeys.sub(operator.stoppedValidators); + activeValidatorsTotal = activeValidatorsTotal.add(activeValidators); recipients[idx] = operator.rewardAddress; - shares[idx] = effectiveStake; + shares[idx] = activeValidators; ++idx; } - if (effectiveStakeTotal == 0) + if (activeValidatorsTotal == 0) return (recipients, shares); - uint256 perStakeReward = _totalRewardShares.div(effectiveStakeTotal); + uint256 perValidatorReward = _totalRewardShares.div(activeValidatorsTotal); for (idx = 0; idx < activeCount; ++idx) { - shares[idx] = shares[idx].mul(perStakeReward); + shares[idx] = shares[idx].mul(perValidatorReward); } return (recipients, shares);