Verify Network balances of src & dst tokens after each reserve trade (#928)

manhlx3006 · ilanDoron · commit c3f83e0d9869 · 2020-05-29T14:50:44.000+03:00
* Verify Network balances of src &amp; dst tokens after each reserve trade

* Update revert messages to make it more readable
diff --git a/contracts/sol6/KyberNetwork.sol b/contracts/sol6/KyberNetwork.sol
@@ -551,8 +551,10 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
         IERC20 src,
         IERC20 dest,
         address payable destAddress,
-        TradeData memory tradeData,
-        uint256 expectedDestAmount
+        ReservesData memory reservesData,
+        uint256 expectedDestAmount,
+        uint256 srcDecimals,
+        uint256 destDecimals
     ) internal virtual returns (bool) {
 
         if (src == dest) {
@@ -564,25 +566,15 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
             return true;
         }
 
-        ReservesData memory reservesData = (src == ETH_TOKEN_ADDRESS)
-            ? tradeData.ethToToken
-            : tradeData.tokenToEth;
-        uint256 callValue;
-
         for (uint256 i = 0; i < reservesData.addresses.length; i++) {
-            callValue = (src == ETH_TOKEN_ADDRESS) ? reservesData.srcAmounts[i] : 0;
-
-            // reserve sends tokens/eth to network. network sends it to destination
-            require(
-                reservesData.addresses[i].trade{value: callValue}(
-                    src,
-                    reservesData.srcAmounts[i],
-                    dest,
-                    address(this),
-                    reservesData.rates[i],
-                    true
-                ),
-                "trade failed"
+            tradeAndVerifyNetworkBalance(
+                reservesData.addresses[i],
+                src,
+                reservesData.srcAmounts[i],
+                dest,
+                reservesData.rates[i],
+                srcDecimals,
+                destDecimals
             );
         }
 
@@ -594,6 +586,76 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
         return true;
     }
 
+    /// @dev call trade from reserve and verify balances
+    ///      return actual dest amount received from reserve
+    /// @param reserve reserve address to trade with
+    /// @param src src token of the trade
+    /// @param srcAmount amount of src token to trade
+    /// @param dest dest token of the trade
+    /// @param conversionRate conversion rate of the trade
+    /// @param srcDecimals src token decimals
+    /// @param destDecimals dest token decimals
+    function tradeAndVerifyNetworkBalance(
+        IKyberReserve reserve,
+        IERC20 src,
+        uint256 srcAmount,
+        IERC20 dest,
+        uint256 conversionRate,
+        uint256 srcDecimals,
+        uint256 destDecimals
+    ) internal
+    {
+        uint256 destBalanceBefore = getBalance(dest, address(this));
+
+        uint256 srcBalanceBefore;
+        uint256 callValue;
+
+        if (src == ETH_TOKEN_ADDRESS) {
+            srcBalanceBefore = 0; // no need to verify src balance
+            callValue = srcAmount;
+        } else {
+            srcBalanceBefore = getBalance(src, address(this));
+            callValue = 0;
+        }
+
+        // reserve sends tokens/eth to network. network sends it to destination
+        uint256 expectedDestAmount = calcDstQty(
+            srcAmount,
+            srcDecimals,
+            destDecimals,
+            conversionRate
+        );
+
+        require(
+            reserve.trade{value: callValue}(
+                src,
+                srcAmount,
+                dest,
+                address(this),
+                conversionRate,
+                true
+            ),
+            "trade failed"
+        );
+
+        uint256 balanceAfter;
+        if (src != ETH_TOKEN_ADDRESS) {
+            // verify src balance only if it is not ETH
+            balanceAfter = getBalance(src, address(this));
+            // verify correct src amount is taken
+            if (srcBalanceBefore >= balanceAfter && srcBalanceBefore - balanceAfter > srcAmount) {
+                revert("reserve takes high amount");
+            }
+        }
+        // verify correct dest amount is received
+        balanceAfter = getBalance(dest, address(this));
+        if (balanceAfter < destBalanceBefore || balanceAfter - destBalanceBefore < expectedDestAmount) {
+            revert("reserve returns low amount");
+        }
+    }
+
+    /* solhint-disable function-max-lines */
+    //  Most of the lines here are functions calls spread over multiple lines. We find this function readable enough
     /// @notice Use token address ETH_TOKEN_ADDRESS for ether
     /// @dev Trade API for kyber network
     /// @param tradeData Main trade data object for trade info to be stored
@@ -630,8 +692,10 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
                 tradeData.input.src,
                 ETH_TOKEN_ADDRESS,
                 address(this),
-                tradeData,
-                tradeData.tradeWei
+                tradeData.tokenToEth,
+                tradeData.tradeWei,
+                tradeData.tokenToEth.decimals,
+                ETH_DECIMALS
             )
         ); // tradeData.tradeWei (expectedDestAmount) not used if destAddress == address(this)
 
@@ -641,8 +705,10 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
                 ETH_TOKEN_ADDRESS,
                 tradeData.input.dest,
                 tradeData.input.destAddress,
-                tradeData,
-                destAmount
+                tradeData.ethToToken,
+                destAmount,
+                ETH_DECIMALS,
+                tradeData.ethToToken.decimals
             )
         );
 
@@ -748,7 +814,7 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
         view
         returns (uint256 destAmount, uint256 rateWithNetworkFee)
     {
-        // token to ether: find best reserve match and calculate wei amount
+        // token to ether: find best reserves match and calculate wei amount
         tradeData.tradeWei = calcDestQtyAndMatchReserves(
             tradeData.input.src,
             ETH_TOKEN_ADDRESS,
@@ -762,18 +828,17 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
             return (0, 0);
         }
 
-        // platform fee
+        // calculate fees
         tradeData.platformFeeWei = (tradeData.tradeWei * tradeData.input.platformFeeBps) / BPS;
         tradeData.networkFeeWei =
             (((tradeData.tradeWei * tradeData.networkFeeBps) / BPS) * tradeData.feeAccountedBps) /
             BPS;
-        // set networkFeeWei in stack. since we set it again after full flow done.
         require(
             tradeData.tradeWei >= (tradeData.networkFeeWei + tradeData.platformFeeWei),
             "fees exceed trade"
         );
 
-        // ether to token: find best reserve match and calculate trade dest amount
+        // ether to token: find best reserves match and calculate trade dest amount
         uint256 actualSrcWei = tradeData.tradeWei -
             tradeData.networkFeeWei -
             tradeData.platformFeeWei;
@@ -1172,8 +1237,7 @@ contract KyberNetwork is WithdrawableNoModifiers, Utils5, IKyberNetwork, Reentra
             );
             if (newSrcAmounts[i] > currentSrcAmount) {
                 // revert back to use current src amounts
-                newSrcAmount = srcAmount;
-                return newSrcAmount;
+                return srcAmount;
             }
 
             newSrcAmount += newSrcAmounts[i];
diff --git a/contracts/sol6/mock/GenerousKyberNetwork.sol b/contracts/sol6/mock/GenerousKyberNetwork.sol
@@ -90,8 +90,10 @@ contract GenerousKyberNetwork is KyberNetwork {
                 tData.input.src,
                 ETH_TOKEN_ADDRESS,
                 address(this),
-                tData,
-                tData.tradeWei
+                tData.tokenToEth,
+                tData.tradeWei,
+                tData.tokenToEth.decimals,
+                ETH_DECIMALS
             )
         ); //tData.tradeWei (expectedDestAmount) not used if destAddress == address(this)
 
@@ -100,8 +102,10 @@ contract GenerousKyberNetwork is KyberNetwork {
                 ETH_TOKEN_ADDRESS,
                 tData.input.dest,
                 tData.input.destAddress,
-                tData,
-                destAmount
+                tData.ethToToken,
+                destAmount,
+                ETH_DECIMALS,
+                tData.ethToToken.decimals
             )
         );
 
diff --git a/contracts/sol6/mock/GenerousKyberNetwork2.sol b/contracts/sol6/mock/GenerousKyberNetwork2.sol
@@ -75,8 +75,10 @@ contract GenerousKyberNetwork2 is KyberNetwork {
                 tData.input.src,
                 ETH_TOKEN_ADDRESS,
                 address(this),
-                tData,
-                tData.tradeWei
+                tData.tokenToEth,
+                tData.tradeWei,
+                tData.tokenToEth.decimals,
+                ETH_DECIMALS
             )
         ); //tData.tradeWei (expectedDestAmount) not used if destAddress == address(this)
 
@@ -85,8 +87,10 @@ contract GenerousKyberNetwork2 is KyberNetwork {
                 ETH_TOKEN_ADDRESS,
                 tData.input.dest,
                 tData.input.destAddress,
-                tData,
-                destAmount
+                tData.ethToToken,
+                destAmount,
+                ETH_DECIMALS,
+                tData.ethToToken.decimals
             )
         );
 
diff --git a/contracts/sol6/mock/MaliciousKyberNetwork.sol b/contracts/sol6/mock/MaliciousKyberNetwork.sol
@@ -72,8 +72,10 @@ contract MaliciousKyberNetwork is KyberNetwork {
                 actualSrcAmount,
                 ETH_TOKEN_ADDRESS,
                 address(this),
-                tData,
-                tData.tradeWei
+                tData.tokenToEth,
+                tData.tradeWei,
+                tData.tokenToEth.decimals,
+                ETH_DECIMALS
             )
         ); //tData.tradeWei (expectedDestAmount) not used if destAddress == address(this)
 
@@ -83,8 +85,10 @@ contract MaliciousKyberNetwork is KyberNetwork {
                 tData.tradeWei - tData.networkFeeWei - tData.platformFeeWei,
                 tData.input.dest,
                 tData.input.destAddress,
-                tData,
-                destAmount
+                tData.ethToToken,
+                destAmount,
+                ETH_DECIMALS,
+                tData.ethToToken.decimals
             )
         );
         require(handleFees(tData));
@@ -109,18 +113,20 @@ contract MaliciousKyberNetwork is KyberNetwork {
         uint256 amount,
         IERC20 dest,
         address payable destAddress,
-        TradeData memory tData,
-        uint256 expectedDestAmount
+        ReservesData memory reservesData,
+        uint256 expectedDestAmount,
+        uint256 srcDecimals,
+        uint256 destDecimals
     ) internal virtual returns (bool) {
         if (src == dest) {
             //E2E, need not do anything except for T2E, transfer ETH to destAddress
             if (destAddress != (address(this))) destAddress.transfer(amount - myFeeWei);
             return true;
         }
 
-        ReservesData memory reservesData = src == ETH_TOKEN_ADDRESS
-            ? tData.ethToToken
-            : tData.tokenToEth;
+        srcDecimals;
+        destDecimals;
+
         uint256 callValue;
         uint256 srcAmountSoFar;
 
diff --git a/contracts/sol6/mock/MaliciousKyberNetwork2.sol b/contracts/sol6/mock/MaliciousKyberNetwork2.sol
@@ -20,18 +20,20 @@ contract MaliciousKyberNetwork2 is MaliciousKyberNetwork {
         uint256 amount,
         IERC20 dest,
         address payable destAddress,
-        TradeData memory tData,
-        uint256 expectedDestAmount
+        ReservesData memory reservesData,
+        uint256 expectedDestAmount,
+        uint256 srcDecimals,
+        uint256 destDecimals
     ) internal override returns (bool) {
         if (src == dest) {
             //E2E, need not do anything except for T2E, transfer ETH to destAddress
             if (destAddress != (address(this))) destAddress.transfer(amount - myFeeWei);
             return true;
         }
 
-        ReservesData memory reservesData = src == ETH_TOKEN_ADDRESS
-            ? tData.ethToToken
-            : tData.tokenToEth;
+        srcDecimals;
+        destDecimals;
+
         uint256 callValue;
         uint256 srcAmountSoFar;
 
diff --git a/contracts/sol6/mock/MaliciousReserve2.sol b/contracts/sol6/mock/MaliciousReserve2.sol
@@ -0,0 +1,53 @@
+pragma solidity 0.6.6;
+
+import "./MockReserve.sol";
+
+
+contract MaliciousReserve2 is MockReserve {
+    // extraSrcAmount > 0: take more src amount from network
+    // extraSrcAmount < 0: take less src amount from network
+    int256 public extraSrcAmount;
+    // extraDestAmount > 0: send more dest amount to network
+    // extraDestAmount < 0: send less dest amount to network
+    int256 public extraDestAmount;
+
+    function setExtraSrcAndDestAmounts(int256 _extraSrcAmount, int256 _extraDestAmount) public {
+        extraSrcAmount = _extraSrcAmount;
+        extraDestAmount = _extraDestAmount;
+    }
+
+    function trade(
+        IERC20 srcToken,
+        uint256 srcAmount,
+        IERC20 destToken,
+        address payable destAddress,
+        uint256 conversionRate,
+        bool validate
+    ) public payable override returns (bool) {
+        validate;
+        if (srcToken == ETH_TOKEN_ADDRESS) {
+            require(msg.value == srcAmount, "ETH sent != srcAmount");
+        } else {
+            require(msg.value == 0, "ETH was sent for token -> ETH trade");
+        }
+
+        uint256 srcDecimals = getDecimals(srcToken);
+        uint256 destDecimals = getDecimals(destToken);
+        uint256 destAmount = calcDstQty(srcAmount, srcDecimals, destDecimals, conversionRate);
+
+        // collect src tokens
+        if (srcToken != ETH_TOKEN_ADDRESS) {
+            srcToken.safeTransferFrom(msg.sender, address(this), uint256(int256(srcAmount) + extraSrcAmount));
+        }
+
+        // send dest tokens
+        uint256 actualDestAmount = uint256(int256(destAmount) + extraDestAmount);
+        if (destToken == ETH_TOKEN_ADDRESS) {
+            destAddress.transfer(actualDestAmount);
+        } else {
+            destToken.safeTransfer(destAddress, actualDestAmount);
+        }
+
+        return true;
+    }
+}
diff --git a/contracts/sol6/mock/MockNetwork.sol b/contracts/sol6/mock/MockNetwork.sol
@@ -62,14 +62,18 @@ contract MockNetwork is KyberNetwork {
         IERC20 src,
         IERC20 dest,
         address payable destAddress,
-        TradeData memory tradeData,
-        uint256 expectedDestAmount
+        ReservesData memory reservesData,
+        uint256 expectedDestAmount,
+        uint256 srcDecimals,
+        uint256 destDecimals
     ) internal override returns (bool) {
         src;
         dest;
         destAddress;
-        tradeData;
+        reservesData;
         expectedDestAmount;
+        srcDecimals;
+        destDecimals;
 
         revert("must use real network");
         // return true;
diff --git a/contracts/sol6/mock/MockReserve.sol b/contracts/sol6/mock/MockReserve.sol
diff --git a/solcOptimiserSettings.js b/solcOptimiserSettings.js
diff --git a/test/sol6/kyberNetwork.js b/test/sol6/kyberNetwork.js
