Fix reward distribution algorithm

contracts/0.4.24/Lido.sol

@@ -448,15 +448,12 @@ contract Lido is ILido, IsContract, StETH, AragonApp {
         if (sharesAmount == 0) {
             // totalControlledEther is 0: either the first-ever deposit or complete slashing
             // assume that shares correspond to Ether 1-to-1
-            _mintShares(sender, deposit);
-            _emitTransferAfterMintingShares(sender, deposit);
-        } else {
-            _mintShares(sender, sharesAmount);
-            _emitTransferAfterMintingShares(sender, sharesAmount);
+            sharesAmount = deposit;
         }
 
+        _mintShares(sender, sharesAmount);
         _submitted(sender, deposit, _referral);
-
+        _emitTransferAfterMintingShares(sender, sharesAmount);
         return sharesAmount;
     }
 
@@ -554,10 +551,10 @@ contract Lido is ILido, IsContract, StETH, AragonApp {
     * @param _totalRewards Total rewards accrued on the Ethereum 2.0 side in wei
     */
     function distributeRewards(uint256 _totalRewards) internal {
-        uint256 feeInEther = _totalRewards.mul(_getFee()).div(10000);
         // 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).
+        // 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%).
         //
         // Since we've increased totalPooledEther by _totalRewards (which is already
         // performed by the time this function is called), the combined cost of all holders'
@@ -567,48 +564,65 @@ 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 = feeInEther
+        // shares2mint * newShareCost = (_totalRewards * feeBasis) / 10000
         // newShareCost = newTotalPooledEther / (prevTotalShares + shares2mint)
         //
         // which follows to:
         //
-        //                    feeInEther * prevTotalShares
-        // shares2mint = --------------------------------------
-        //                 newTotalPooledEther - feeInEther
+        //                        _totalRewards * feeBasis * prevTotalShares
+        // shares2mint = --------------------------------------------------------------
+        //                 (newTotalPooledEther * 10000) - (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
         // token shares.
-        //
+        uint256 feeBasis = _getFee();
         uint256 shares2mint = (
-            feeInEther
-            .mul(_getTotalShares())
-            .div(_getTotalPooledEther().sub(feeInEther))
+            _totalRewards.mul(feeBasis).mul(_getTotalShares())
+            .div(
+                _getTotalPooledEther().mul(10000)
+                .sub(feeBasis.mul(_totalRewards))
+            )
         );
 
         // Mint the calculated amount of shares to this contract address. This will reduce the
         // balances of the holders, as if the fee was taken in parts from each of them.
         _mintShares(address(this), shares2mint);
 
-        (uint16 treasuryFeeBasisPoints, uint16 insuranceFeeBasisPoints, ) = _getFeeDistribution();
-        uint256 toTreasury = shares2mint.mul(treasuryFeeBasisPoints).div(10000);
+        (,uint16 insuranceFeeBasisPoints, uint16 operatorsFeeBasisPoints) = _getFeeDistribution();
+
         uint256 toInsuranceFund = shares2mint.mul(insuranceFeeBasisPoints).div(10000);
-        // Transfer the rest of the fee to operators
-        uint256 toOperatorsRegistry = shares2mint.sub(toTreasury).sub(toInsuranceFund);
+        address insuranceFund = getInsuranceFund();
+        _transferShares(address(this), insuranceFund, toInsuranceFund);
+        _emitTransferAfterMintingShares(insuranceFund, toInsuranceFund);
+
+        uint256 distributedToOperatorsShares = _distributeNodeOperatorsReward(
+            shares2mint.mul(operatorsFeeBasisPoints).div(10000)
+        );
+
+        // Transfer the rest of the fee to treasury
+        uint256 toTreasury = shares2mint.sub(toInsuranceFund).sub(distributedToOperatorsShares);
 
         address treasury = getTreasury();
-        _transferShares(address(this), getTreasury(), toTreasury);
+        _transferShares(address(this), treasury, toTreasury);
         _emitTransferAfterMintingShares(treasury, toTreasury);
+    }
 
-        address insuranceFund = getInsuranceFund();
-        _transferShares(address(this), getInsuranceFund(), toInsuranceFund);
-        _emitTransferAfterMintingShares(insuranceFund, toInsuranceFund);
+    function _distributeNodeOperatorsReward(uint256 _sharesToDistribute) internal returns (uint256 distributed) {
+        (address[] memory recipients, uint256[] memory shares) = getOperators().getRewardsDistribution(_sharesToDistribute);
 
-        INodeOperatorsRegistry operatorsRegistry = getOperators();
-        _transferShares(address(this), operatorsRegistry, toOperatorsRegistry);
-        _emitTransferAfterMintingShares(operatorsRegistry, toOperatorsRegistry);
+        assert(recipients.length == shares.length);
 
-        operatorsRegistry.distributeRewards(address(this), getPooledEthByShares(toOperatorsRegistry));
+        distributed = 0;
+        for (uint256 idx = 0; idx < recipients.length; ++idx) {
+            _transferShares(
+                address(this),
+                recipients[idx],
+                shares[idx]
+            );
+            _emitTransferAfterMintingShares(recipients[idx], shares[idx]);
+            distributed = distributed.add(shares[idx]);
+        }
     }
 
     /**

contracts/0.4.24/interfaces/INodeOperatorsRegistry.sol

@@ -76,21 +76,22 @@ interface INodeOperatorsRegistry {
         uint64 totalSigningKeys,
         uint64 usedSigningKeys);
 
+    /**
+      * @notice Returns the rewards distribution proportional to the effective stake for each node operator.
+      * @param _totalRewardShares Total amount of reward shares to distribute.
+      */
+    function getRewardsDistribution(uint256 _totalRewardShares) external view returns (
+        address[] memory recipients,
+        uint256[] memory shares
+    );
+
     event NodeOperatorAdded(uint256 id, string name, address rewardAddress, uint64 stakingLimit);
     event NodeOperatorActiveSet(uint256 indexed id, bool active);
     event NodeOperatorNameSet(uint256 indexed id, string name);
     event NodeOperatorRewardAddressSet(uint256 indexed id, address rewardAddress);
     event NodeOperatorStakingLimitSet(uint256 indexed id, uint64 stakingLimit);
     event NodeOperatorTotalStoppedValidatorsReported(uint256 indexed id, uint64 totalStopped);
 
-    /**
-      * @notice Distributes rewards among node operators.
-      * @dev Function is used by the pool
-      * @param _token Reward token (must be ERC20-compatible)
-      * @param _totalReward Total amount to distribute (must be transferred to this contract beforehand)
-      */
-    function distributeRewards(address _token, uint256 _totalReward) external;
-
     /**
      * @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.

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

@@ -9,7 +9,6 @@ import "@aragon/os/contracts/apps/AragonApp.sol";
 import "@aragon/os/contracts/common/IsContract.sol";
 import "@aragon/os/contracts/lib/math/SafeMath.sol";
 import "@aragon/os/contracts/lib/math/SafeMath64.sol";
-import "openzeppelin-solidity/contracts/token/ERC20/IERC20.sol";
 import "solidity-bytes-utils/contracts/BytesLib.sol";
 
 import "../interfaces/INodeOperatorsRegistry.sol";
@@ -363,35 +362,47 @@ contract NodeOperatorsRegistry is INodeOperatorsRegistry, IsContract, AragonApp
     }
 
     /**
-      * @notice Distributes rewards among node operators.
-      * @dev Function is used by the pool
-      * @param _token Reward token (must be ERC20-compatible)
-      * @param _totalReward Total amount to distribute (must be transferred to this contract beforehand)
+      * @notice Returns the rewards distribution proportional to the effective stake for each node operator.
+      * @param _totalRewardShares Total amount of reward shares to distribute.
       */
-    function distributeRewards(address _token, uint256 _totalReward) external onlyPool {
-        uint256 length = getNodeOperatorsCount();
-        uint64 effectiveStakeTotal;
-        for (uint256 operatorId = 0; operatorId < length; ++operatorId) {
+    function getRewardsDistribution(uint256 _totalRewardShares) external view
+        returns (
+            address[] memory recipients,
+            uint256[] memory shares
+        )
+    {
+        uint256 nodeOperatorCount = getNodeOperatorsCount();
+
+        uint256 activeCount = getActiveNodeOperatorsCount();
+        recipients = new address[](activeCount);
+        shares = new uint256[](activeCount);
+        uint256 idx = 0;
+
+        uint256 effectiveStakeTotal = 0;
+        for (uint256 operatorId = 0; operatorId < nodeOperatorCount; ++operatorId) {
             NodeOperator storage operator = operators[operatorId];
             if (!operator.active)
                 continue;
 
-            uint64 effectiveStake = operator.usedSigningKeys.sub(operator.stoppedValidators);
+            uint256 effectiveStake = operator.usedSigningKeys.sub(operator.stoppedValidators);
             effectiveStakeTotal = effectiveStakeTotal.add(effectiveStake);
+
+            recipients[idx] = operator.rewardAddress;
+            shares[idx] = effectiveStake;
+
+            ++idx;
         }
 
-        if (0 == effectiveStakeTotal)
-            revert("NO_STAKE");
+        if (effectiveStakeTotal == 0)
+            return (recipients, shares);
 
-        for (operatorId = 0; operatorId < length; ++operatorId) {
-            operator = operators[operatorId];
-            if (!operator.active)
-                continue;
+        uint256 perValidatorReward = _totalRewardShares.div(effectiveStakeTotal);
 
-            effectiveStake = operator.usedSigningKeys.sub(operator.stoppedValidators);
-            uint256 reward = uint256(effectiveStake).mul(_totalReward).div(uint256(effectiveStakeTotal));
-            require(IERC20(_token).transfer(operator.rewardAddress, reward), "TRANSFER_FAILED");
+        for (idx = 0; idx < activeCount; ++idx) {
+            shares[idx] = shares[idx].mul(perValidatorReward);
         }
+
+        return (recipients, shares);
     }
 
     /**

contracts/0.4.24/nos/test_helpers/PoolMock.sol

@@ -23,8 +23,4 @@ contract PoolMock {
     function trimUnusedKeys() external {
         operators.trimUnusedKeys();
     }
-
-    function distributeRewards(address _token, uint256 _totalReward) external {
-        operators.distributeRewards(_token, _totalReward);
-    }
 }

test/0.4.24/lido.test.js

@@ -32,13 +32,14 @@ const hexConcat = (first, ...rest) => {
 }
 
 // Divides a BN by 1e15
-const div15 = (bn) => bn.div(new BN(1000000)).div(new BN(1000000)).div(new BN(1000))
+
+const div15 = (bn) => bn.div(new BN('1000000000000000'))
 
 const ETH = (value) => web3.utils.toWei(value + '', 'ether')
 const tokens = ETH
 
 contract('Lido', ([appManager, voting, user1, user2, user3, nobody]) => {
-  let appBase, nodeOperatorsRegistryBase, app, token, oracle, validatorRegistration, operators
+  let appBase, nodeOperatorsRegistryBase, app, oracle, validatorRegistration, operators
   let treasuryAddr, insuranceAddr
 
   before('deploy base app', async () => {
@@ -218,7 +219,10 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody]) => {
     assertBn(await app.totalSupply(), tokens(1))
 
     // +2 ETH
-    await app.submit(ZERO_ADDRESS, { from: user2, value: ETH(2) }) // another form of a deposit call
+    const receipt = await app.submit(ZERO_ADDRESS, { from: user2, value: ETH(2) }) // another form of a deposit call
+
+    assertEvent(receipt, 'Transfer', { expectedArgs: { from: ZERO_ADDRESS, to: user2, value: ETH(2) } })
+
     await checkStat({ depositedValidators: 0, beaconValidators: 0, beaconBalance: ETH(0) })
     assertBn(await validatorRegistration.totalCalls(), 0)
     assertBn(await app.getTotalPooledEther(), ETH(3))
@@ -596,7 +600,7 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody]) => {
     await oracle.reportBeacon(300, 1, ETH(36))
     await checkStat({ depositedValidators: 1, beaconValidators: 1, beaconBalance: ETH(36) })
     assertBn(await app.totalSupply(), tokens(38)) // remote + buffered
-    await checkRewards({ treasury: 599, insurance: 399, operator: 1000 })
+    await checkRewards({ treasury: 600, insurance: 399, operator: 999 })
   })
 
   it('rewards distribution works', async () => {
@@ -659,7 +663,7 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody]) => {
     await oracle.reportBeacon(300, 1, ETH(36))
     await checkStat({ depositedValidators: 1, beaconValidators: 1, beaconBalance: ETH(36) })
     assertBn(await app.totalSupply(), tokens(68))
-    await checkRewards({ treasury: 599, insurance: 399, operator: 1000 })
+    await checkRewards({ treasury: 600, insurance: 399, operator: 999 })
   })
 
   it('Node Operators filtering during deposit works when doing a huge deposit', async () => {
@@ -972,11 +976,11 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody]) => {
 
   context('treasury', () => {
     it('treasury adddress has been set after init', async () => {
-      assert.notEqual(await app.getTreasury(), ZERO_ADDRESS);
+      assert.notEqual(await app.getTreasury(), ZERO_ADDRESS)
     })
 
     it(`treasury can't be set by an arbitary address`, async () => {
-      await assertRevert(app.setTreasury(user1, { from: nobody  }))
+      await assertRevert(app.setTreasury(user1, { from: nobody }))
       await assertRevert(app.setTreasury(user1, { from: user1 }))
     })
 
@@ -986,20 +990,17 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody]) => {
     })
 
     it('reverts when treasury is zero address', async () => {
-      await assertRevert(
-        app.setTreasury(ZERO_ADDRESS, { from: voting }),
-        "SET_TREASURY_ZERO_ADDRESS"
-      )
+      await assertRevert(app.setTreasury(ZERO_ADDRESS, { from: voting }), 'SET_TREASURY_ZERO_ADDRESS')
     })
   })
 
   context('insurance fund', () => {
     it('insurance fund adddress has been set after init', async () => {
-      assert.notEqual(await app.getInsuranceFund(), ZERO_ADDRESS);
+      assert.notEqual(await app.getInsuranceFund(), ZERO_ADDRESS)
     })
 
     it(`insurance fund can't be set by an arbitary address`, async () => {
-      await assertRevert(app.setInsuranceFund(user1, { from: nobody   }))
+      await assertRevert(app.setInsuranceFund(user1, { from: nobody }))
       await assertRevert(app.setInsuranceFund(user1, { from: user1 }))
     })
 
@@ -1009,10 +1010,7 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody]) => {
     })
 
     it('reverts when insurance fund is zero address', async () => {
-      await assertRevert(
-        app.setInsuranceFund(ZERO_ADDRESS, { from: voting }),
-        "SET_INSURANCE_FUND_ZERO_ADDRESS"
-      )
+      await assertRevert(app.setInsuranceFund(ZERO_ADDRESS, { from: voting }), 'SET_INSURANCE_FUND_ZERO_ADDRESS')
     })
   })
 })

test/0.4.24/node-operators-registry.test.js

@@ -613,7 +613,7 @@ contract('NodeOperatorsRegistry', ([appManager, voting, user1, user2, user3, nob
     await assertRevert(app.getSigningKey(1, 0, { from: nobody }), 'KEY_NOT_FOUND')
   })
 
-  it('distributeRewards works', async () => {
+  it('getRewardsDistribution works', async () => {
     await app.addNodeOperator('fo o', ADDRESS_1, 10, { from: voting })
     await app.addNodeOperator(' bar', ADDRESS_2, UNLIMITED, { from: voting })
     await app.addNodeOperator('3', ADDRESS_3, UNLIMITED, { from: voting })
@@ -636,13 +636,9 @@ contract('NodeOperatorsRegistry', ([appManager, voting, user1, user2, user3, nob
     await app.reportStoppedValidators(0, 1, { from: voting })
     await app.setNodeOperatorActive(2, false, { from: voting })
 
-    const token = await ERC20Mock.new()
-    await token.mint(app.address, tokens(900))
-    await pool.distributeRewards(token.address, tokens(900))
+    const {recipients, shares} = await app.getRewardsDistribution(tokens(900));
 
-    assertBn(await token.balanceOf(ADDRESS_1, { from: nobody }), tokens(300))
-    assertBn(await token.balanceOf(ADDRESS_2, { from: nobody }), tokens(600))
-    assertBn(await token.balanceOf(ADDRESS_3, { from: nobody }), 0)
-    assertBn(await token.balanceOf(app.address, { from: nobody }), 0)
+    assert.sameOrderedMembers(recipients, [ADDRESS_1, ADDRESS_2], 'recipients')
+    assert.sameOrderedMembers(shares.map(x => String(x)), [tokens(300), tokens(600)], 'shares')
   })
 })

test/scenario/lido_happy_path.js

@@ -330,8 +330,7 @@ contract('Lido: happy path', (addresses) => {
     // and node operators
     // treasuryTokenBalance ~= mintedAmount * treasuryFeePoints / 10000
     // insuranceTokenBalance ~= mintedAmount * insuranceFeePoints / 10000
-
-    assertBn(await token.balanceOf(treasuryAddr), new BN('95999999999999998'), 'treasury tokens')
+    assertBn(await token.balanceOf(treasuryAddr), new BN('96000000000000001'), 'treasury tokens')
     assertBn(await token.balanceOf(insuranceAddr), new BN('63999999999999998'), 'insurance tokens')
 
     // The node operators' fee is distributed between all active node operators,