Fix handling of rebate leftover (for a reserve that is accounted fee but not entitled for rebate)

ilanDoron · Anyhowclick · web-flow · commit d7a026dbcff2 · 2020-07-01T11:27:28.000+03:00
* For split trade 50:50 with 2 reserves fee entitled. out of them one is entitled rebate.
	current: 1 entitled rebate reserve will receive all rebate amount
	fix: entitled rebate reserve will receive rebate amount according to its split in the trade

Co-authored-by: Ilan Doron &lt;ilan.on.the.road@gmail.com&gt;

* Update handling of rebate leftover. use leftover for extra rewards instead of burning

* if 2 reserves are fee accounted but only 1 reserve entitled for rebate, there is some leftover of rebate amount.

current: leftover rebate will be burnt
fix: leftover rebate will be added to reward amount

Co-authored-by: Desmond &lt;howeilun@hotmail.com&gt;
diff --git a/contracts/sol6/Dao/KyberFeeHandler.sol b/contracts/sol6/Dao/KyberFeeHandler.sol
@@ -61,7 +61,8 @@ contract KyberFeeHandler is IKyberFeeHandler, Utils5, DaoOperator, ReentrancyGua
 
     struct BRRWei {
         uint256 rewardWei;
-        uint256 rebateWei;
+        uint256 fullRebateWei;
+        uint256 paidRebateWei;
         uint256 burnWei;
     }
 
@@ -203,22 +204,29 @@ contract KyberFeeHandler is IKyberFeeHandler, Utils5, DaoOperator, ReentrancyGua
         uint256 epoch;
 
         // Decoding BRR data
-        (brrAmounts.rewardWei, brrAmounts.rebateWei, epoch) = getRRWeiValues(networkFee);
+        (brrAmounts.rewardWei, brrAmounts.fullRebateWei, epoch) = getRRWeiValues(networkFee);
 
-        brrAmounts.rebateWei = updateRebateValues(brrAmounts.rebateWei, rebateWallets, rebateBpsPerWallet);
+        brrAmounts.paidRebateWei = updateRebateValues(
+            brrAmounts.fullRebateWei, rebateWallets, rebateBpsPerWallet
+        );
+        brrAmounts.rewardWei = brrAmounts.rewardWei.add(
+            brrAmounts.fullRebateWei.sub(brrAmounts.paidRebateWei)
+        );
 
         rewardsPerEpoch[epoch] = rewardsPerEpoch[epoch].add(brrAmounts.rewardWei);
 
         // update total balance of rewards, rebates, fee
-        totalPayoutBalance = totalPayoutBalance.add(platformFee).add(brrAmounts.rewardWei).add(brrAmounts.rebateWei);
+        totalPayoutBalance = totalPayoutBalance.add(
+            platformFee).add(brrAmounts.rewardWei).add(brrAmounts.paidRebateWei
+        );
 
-        brrAmounts.burnWei = networkFee.sub(brrAmounts.rewardWei).sub(brrAmounts.rebateWei);
+        brrAmounts.burnWei = networkFee.sub(brrAmounts.rewardWei).sub(brrAmounts.paidRebateWei);
         emit FeeDistributed(
             ETH_TOKEN_ADDRESS,
             platformWallet,
             platformFee,
             brrAmounts.rewardWei,
-            brrAmounts.rebateWei,
+            brrAmounts.paidRebateWei,
             rebateWallets,
             rebateBpsPerWallet,
             brrAmounts.burnWei
diff --git a/contracts/sol6/KyberNetwork.sol b/contracts/sol6/KyberNetwork.sol
@@ -62,8 +62,6 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
     /// @param networkFeeBps Network fee bps determined by kyberDao, or default value
     /// @param numEntitledRebateReserves No. of reserves that are eligible for rebates
     /// @param feeAccountedBps Proportion of this trade that fee is accounted to, in BPS. Up to 2 * BPS
-    /// @param entitledRebateBps Proportion of reserves entitled for rebate, in BPS. Up to 2 * BPS
-    /// @param rateWithNetworkFee src -> dest token rate, after accounting for only network fee
     struct TradeData {
         TradeInput input;
         ReservesData tokenToEth;
@@ -74,7 +72,6 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
         uint256 networkFeeBps;
         uint256 numEntitledRebateReserves;
         uint256 feeAccountedBps; // what part of this trade is fee paying. for token -> token - up to 200%
-        uint256 entitledRebateBps;
     }
 
     struct TradeInput {
@@ -979,7 +976,7 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
             rebatePercentBps,
             tradeData.tokenToEth,
             index,
-            tradeData.entitledRebateBps
+            tradeData.feeAccountedBps
         );
 
         // eth -> token
@@ -988,7 +985,7 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
             rebatePercentBps,
             tradeData.ethToToken,
             index,
-            tradeData.entitledRebateBps
+            tradeData.feeAccountedBps
         );
 
         rebateWallets = kyberStorage.getRebateWalletsFromIds(rebateReserveIds);
@@ -999,14 +996,14 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
         uint256[] memory rebatePercentBps,
         ReservesData memory reservesData,
         uint256 index,
-        uint256 entitledRebateBps
+        uint256 feeAccountedBps
     ) internal pure returns (uint256) {
         uint256 _index = index;
 
         for (uint256 i = 0; i < reservesData.isEntitledRebateFlags.length; i++) {
             if (reservesData.isEntitledRebateFlags[i]) {
                 rebateReserveIds[_index] = reservesData.ids[i];
-                rebatePercentBps[_index] = (reservesData.splitsBps[i] * BPS) / entitledRebateBps;
+                rebatePercentBps[_index] = (reservesData.splitsBps[i] * BPS) / feeAccountedBps;
                 _index++;
             }
         }
@@ -1130,7 +1127,6 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
                 tradeData.feeAccountedBps += reservesData.splitsBps[i];
 
                 if (reservesData.isEntitledRebateFlags[i]) {
-                    tradeData.entitledRebateBps += reservesData.splitsBps[i];
                     tradeData.numEntitledRebateReserves++;
                 }
             }
diff --git a/solcOptimiserSettings.js b/solcOptimiserSettings.js
@@ -1,4 +1,4 @@
 module.exports = {
     enabled: true,
-    runs: 410
+    runs: 430
 }
diff --git a/test/sol6/kyberFeeHandler.js b/test/sol6/kyberFeeHandler.js
@@ -540,11 +540,16 @@ contract('KyberFeeHandler', function(accounts) {
 
             expectedTotalReward = expectedTotalReward.add(sendVal.mul(currentRewardBps).div(BPS));
             expectedRebate = sendVal.mul(currentRebateBps).div(BPS);
+            let totalPaidRebate = new BN(0);
 
             for(let i = 0; i < rebateBpsPerWallet.length; i++) {
-                expectedTotalRebate.iadd((new BN(rebateBpsPerWallet[i])).mul(expectedRebate).div(BPS));
+                let paidRebate = (new BN(rebateBpsPerWallet[i])).mul(expectedRebate).div(BPS);
+                expectedTotalRebate.iadd(paidRebate);
+                totalPaidRebate = totalPaidRebate.add(paidRebate);
             }
 
+            //whatever wasn't paid for rebates to wallets due to rounding errors, will be added to reward amount
+            expectedTotalReward = expectedTotalReward.add(expectedRebate.sub(totalPaidRebate));
             expectedTotalPayOut = expectedTotalReward.add(expectedTotalRebate);
             totalPayOutBalance = await feeHandler.totalPayoutBalance();
             Helper.assertEqual(expectedTotalPayOut, totalPayOutBalance);
@@ -744,9 +749,7 @@ contract('KyberFeeHandler', function(accounts) {
                         rebateWallets, rebateBpsPerWallet
                     );
 
-                expectedRewardPerEpoch = expectedRewardPerEpoch.add(sendVal.mul(currentRewardBps).div(BPS));
-                rewardPerEpoch = await feeHandler.rewardsPerEpoch(currentEpoch);
-                Helper.assertEqual(expectedRewardPerEpoch, rewardPerEpoch);
+                //above function also validates reward per eopch.
             });
 
             it("test reward per eopch updated when epoch advances", async() => {
@@ -770,14 +773,9 @@ contract('KyberFeeHandler', function(accounts) {
                 Helper.assertEqual(0, rewardPerEpoch);
 
                 sendVal = oneEth.div(new BN(333));
-                expectedRebates = await callHandleFeeAndVerifyValues(
-                    sendVal, zeroAddress, 0, currentRebateBps, currentRewardBps, currentEpoch,
-                        rebateWallets, rebateBpsPerWallet
-                    );
+                expectedRebates = await callHandleFeeAndVerifyValues(sendVal, zeroAddress, 0, currentRebateBps, currentRewardBps, currentEpoch,rebateWallets, rebateBpsPerWallet);
 
-                expectedRewardPerEpoch = sendVal.mul(currentRewardBps).div(BPS);
-                rewardPerEpoch = await feeHandler.rewardsPerEpoch(currentEpoch);
-                Helper.assertEqual(expectedRewardPerEpoch, rewardPerEpoch);
+                //above function also validates reward per eopch.
             });
 
             it("claim reward and see total payout balance updated.", async() => {
@@ -2027,12 +2025,18 @@ async function callHandleFeeAndVerifyValues(sendValWei, platformWallet, platFeeW
 
     //validate values
     let expectedRebates = [];
+    let totalRebateWei = feeAmountBRR.mul(rebateBps).div(BPS);
+    let paidRebatesWei = new BN(0);
     for (let i = 0; i < rebateWalletArr.length; i++) {
-        expectedRebates[i] = currentRebatesArr[i].add(feeAmountBRR.mul(rebateBps).div(BPS).mul(rebateBpsArr[i]).div(BPS));
+        let walletRebateWei = totalRebateWei.mul(rebateBpsArr[i]).div(BPS)
+        expectedRebates[i] = currentRebatesArr[i].add(walletRebateWei);
+        paidRebatesWei = paidRebatesWei.add(walletRebateWei);
         let actualRebate = await feeHandler.rebatePerWallet(rebateWalletArr[i]);
         Helper.assertEqual(actualRebate, expectedRebates[i]);
     }
 
+    const extraRewardFromUnpaidRebate = totalRebateWei.sub(paidRebatesWei);
+    expectedRewardForEpoch = expectedRewardForEpoch.add(extraRewardFromUnpaidRebate);
     const actualFeeWallet = await feeHandler.feePerPlatformWallet(platformWallet);
     Helper.assertEqual(actualFeeWallet, expectedPlatWalletFee);
 
diff --git a/test/sol6/kyberNetwork.js b/test/sol6/kyberNetwork.js
@@ -2399,7 +2399,7 @@ contract('KyberNetwork', function(accounts) {
         });
     });
 
-    describe("test fee handler integrations with 1 mock and 1 apr", async() => {
+    describe("test fee handler integrations with 1 mock and 1 fpr", async() => {
         let platformFee = new BN(200);
         let reserveIdToWallet = [];
         let rebateWallets;
@@ -2457,26 +2457,24 @@ contract('KyberNetwork', function(accounts) {
             reserveInstances = {};
         });
 
-        async function assertFeeHandlerUpdate(tradeWei, platfromFeeBps, feeAccountedBps, rebatePerWallet){
+        async function assertFeeHandlerUpdate(tradeWei, platfromFeeBps, feeAccountedBps, rebateEntitledBps, rebatePerWallet){
             platformFeeWei = tradeWei.mul(platfromFeeBps).div(BPS);
             Helper.assertEqual(await feeHandler.feePerPlatformWallet(platformWallet), beforePlatformFee.add(platformFeeWei), "unexpected rebate value");
             networkFeeWei = tradeWei.mul(networkFeeBps).div(BPS).mul(feeAccountedBps).div(BPS);
-            let hasRebate = false;
+            rebateWei = zeroBN;
             for (const [rebateWallet, beforeBalance] of Object.entries(beforeRebate)) {
                 if (rebateWallet in rebatePerWallet) {
+                    rebateWei = rebateWei.add(rebatePerWallet[rebateWallet]);
                     Helper.assertApproximate(await feeHandler.rebatePerWallet(rebateWallet), beforeBalance.add(rebatePerWallet[rebateWallet]), "unexpected rebate value");
-                    hasRebate = true;
                 }else {
                     Helper.assertApproximate(await feeHandler.rebatePerWallet(rebateWallet), beforeBalance, "unexpected rebate value");
                 }
             }
-            if (hasRebate) {
-                totalPayout = platformFeeWei.add(networkFeeWei.mul(rewardInBPS).div(BPS)).add(networkFeeWei.mul(rebateInBPS).div(BPS));
-            } else {
-                totalPayout = platformFeeWei.add(networkFeeWei.mul(rewardInBPS).div(BPS));
-            }
+
+            let rebateLeftOver = feeAccountedBps == zeroBN? zeroBN : networkFeeWei.mul(feeAccountedBps.sub(rebateEntitledBps)).div(feeAccountedBps).mul(rebateInBPS).div(BPS); 
+            let rewardWei = (networkFeeWei.mul(rewardInBPS).div(BPS)).add(rebateLeftOver);
+            totalPayout = platformFeeWei.add(rewardWei.add(rebateWei));
             Helper.assertApproximate(await feeHandler.totalPayoutBalance(), beforeTotalBalancePayout.add(totalPayout), "unexpected payout balance");
-            
         }
 
         it("e2t trade. see fee updated in fee handler.", async() => {
@@ -2494,7 +2492,7 @@ contract('KyberNetwork', function(accounts) {
             let expectedRebate = new BN(tradeEventArgs.ethWeiValue).mul(networkFeeBps).div(BPS).mul(rebateInBPS).div(BPS);
             let rebatePerWallet = {}
             rebatePerWallet[rebateWallet] = expectedRebate;
-            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, platformFee, BPS, rebatePerWallet);
+            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, platformFee, BPS, BPS, rebatePerWallet);
         });
 
         it("t2e trade. see fee in fee handler.", async() => {
@@ -2513,10 +2511,10 @@ contract('KyberNetwork', function(accounts) {
             let expectedRebate = new BN(tradeEventArgs.ethWeiValue).mul(networkFeeBps).div(BPS).mul(rebateInBPS).div(BPS);
             let rebatePerWallet = {}
             rebatePerWallet[rebateWallet] = expectedRebate;
-            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, platformFee, BPS, rebatePerWallet);
+            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, platformFee, BPS, BPS, rebatePerWallet);
         });
 
-        it("should have rebate given only to rebate entitled reserve.", async() => {
+        it("check that reserve rebate amount is correct", async() => {
             await storage.setFeeAccountedPerReserveType(true, true, true, true, true, true, {from: admin});
             // set rebate entitled true for FPR
             await storage.setEntitledRebatePerReserveType(true, false, false, false, false, false, {from: admin});
@@ -2530,14 +2528,22 @@ contract('KyberNetwork', function(accounts) {
             await srcToken.transfer(network.address, srcQty);
             hint = await nwHelper.getHint(rateHelper, matchingEngine, reserveInstances, SPLIT_HINTTYPE, numReserves, 
                 srcToken.address, destToken.address, srcQty);
+
             txResult = await network.tradeWithHintAndFee(networkProxy, srcToken.address, srcQty, destToken.address, taker,
                 maxDestAmt, minConversionRate, platformWallet, platformFee, hint, {from: networkProxy});
+            
             // assert first rebateWallet is received enitled rebate value
             let tradeEventArgs = nwHelper.getTradeEventArgs(txResult);
-            let rebatePerWallet = {}
-            let expectedRebate = new BN(tradeEventArgs.ethWeiValue).mul(networkFeeBps).div(BPS).mul(new BN(2)).mul(rebateInBPS).div(BPS);
-            rebatePerWallet[rebateWallets[0]] = expectedRebate
-            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, platformFee, BPS.mul(new BN(2)), rebatePerWallet);
+            let rebatePerWallet = {};
+            let feeAccountedBps = new BN(2).mul(BPS);
+            let entitledRebateBps = BPS;
+            let networkFeeWei = new BN(tradeEventArgs.ethWeiValue).mul(networkFeeBps).div(BPS).mul(new BN(2));
+            let expectedRebateWei = networkFeeWei.mul(entitledRebateBps).div(feeAccountedBps).mul(rebateInBPS).div(BPS);
+
+            rebatePerWallet[rebateWallets[0]] = expectedRebateWei;
+            rebatePerWallet[rebateWallets[1]] = zeroBN;
+            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, platformFee, feeAccountedBps, 
+                entitledRebateBps, rebatePerWallet);
             // revert changes
             await storage.setEntitledRebatePerReserveType(true, true, true, true, true, true, {from: admin});
         });
@@ -2551,27 +2557,25 @@ contract('KyberNetwork', function(accounts) {
             let txResult = await network.tradeWithHintAndFee(networkProxy, ethAddress, srcQty, srcToken.address, taker,
                 maxDestAmt, minConversionRate, platformWallet, zeroBN, '0x', {from: networkProxy, value: srcQty});
             let tradeEventArgs = nwHelper.getTradeEventArgs(txResult);
-            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, zeroBN, zeroBN, {});
+            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, zeroBN, zeroBN, zeroBN, {});
 
             // revert changes
             await storage.setFeeAccountedPerReserveType(true, true, true, true, true, true, { from: admin });
             await storage.setEntitledRebatePerReserveType(true, true, true, true, true, true, {from: admin});
         });
 
-        it("should have no rebate fee if entitled rebate data is set to false", async() => {
+        it("should have no rebate if entitled rebate data is set to false", async() => {
             await storage.setFeeAccountedPerReserveType(true, true, true, true, true, true, {from: admin});
             // set entitled rebate data to false
             await storage.setEntitledRebatePerReserveType(false, false, false, false, false, false, {from: admin});
-            
-            // let payoutBalance0 = await feeHandler.totalPayoutBalance();
+
             let srcQty = oneEth;
 
             let txResult = await network.tradeWithHintAndFee(networkProxy, ethAddress, srcQty, srcToken.address, taker,
                 maxDestAmt, minConversionRate, platformWallet, zeroBN, '0x', {from: networkProxy, value: srcQty});
-            // let payoutBalance1 = await feeHandler.totalPayoutBalance();
-            // Helper.assertEqual(payoutBalance0, payoutBalance1);
+
             let tradeEventArgs = nwHelper.getTradeEventArgs(txResult);
-            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, zeroBN, BPS, {});
+            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, zeroBN, BPS, zeroBN, {});
 
             // reset fees
             await storage.setFeeAccountedPerReserveType(true, true, true, true, true, true, { from: admin });
@@ -2587,7 +2591,7 @@ contract('KyberNetwork', function(accounts) {
             let txResult = await network.tradeWithHintAndFee(networkProxy, ethAddress, srcQty, srcToken.address, taker,
                 maxDestAmt, minConversionRate, platformWallet, platformFee, '0x', {from: networkProxy, value: srcQty});
             let tradeEventArgs = nwHelper.getTradeEventArgs(txResult);
-            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, platformFee, zeroBN, {});
+            await assertFeeHandlerUpdate(tradeEventArgs.ethWeiValue, platformFee, zeroBN, zeroBN, {});
 
             // reset fees
             await storage.setFeeAccountedPerReserveType(true, true, true, true, true, true, { from: admin });

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`	`1`	`module.exports = {`
`2`	`2`	`enabled: true,`
`3`		`- runs: 410`
	`3`	`+ runs: 430`
`4`	`4`	`}`