Zero Src Qty Handling

contracts/ExpectedRate.sol

@@ -55,10 +55,27 @@ contract ExpectedRate is Withdrawable, ExpectedRateInterface, Utils2 {
 
         if (srcQty == 0) srcQty = 1;
 
-        bool didRevert = false;
+        //not needed
+        uint bestReserve;
 
-        (didRevert, expectedRate, slippageRate) = safeFindBestRate(src, dest, srcQty, usePermissionless);
-        if (didRevert) return (0, 0);
+        if (usePermissionless) {
+            (bestReserve, expectedRate) = kyberNetwork.findBestRate(src, dest, srcQty);
+
+            if (quantityFactor != 1) {
+                (bestReserve, slippageRate) = kyberNetwork.findBestRate(src, dest, (srcQty * quantityFactor));
+            } else {
+                slippageRate = expectedRate;
+            }
+        } else {
+            (bestReserve, expectedRate) = kyberNetwork.findBestRateOnlyPermission(src, dest, srcQty);
+
+            if (quantityFactor != 1) {
+                (bestReserve, slippageRate) = kyberNetwork.findBestRateOnlyPermission(src, dest,
+                    (srcQty * quantityFactor));
+            } else {
+                slippageRate = expectedRate;
+            }
+        }
 
         if (expectedRate == 0) {
             expectedRate = expectedRateSmallQty(src, dest, srcQty, usePermissionless);
@@ -104,59 +121,9 @@ contract ExpectedRate is Withdrawable, ExpectedRateInterface, Utils2 {
         (reserve, rateSrcToEth) = kyberNetwork.searchBestRate(src, ETH_TOKEN_ADDRESS, srcQty, usePermissionless);
 
         uint ethQty = calcDestAmount(src, ETH_TOKEN_ADDRESS, srcQty, rateSrcToEth);
+        if (ethQty == 0) ethQty = 1;
 
         (reserve, rateEthToDest) = kyberNetwork.searchBestRate(ETH_TOKEN_ADDRESS, dest, ethQty, usePermissionless);
         return rateSrcToEth * rateEthToDest / PRECISION;
     }
-
-    function safeFindBestRate(ERC20 src, ERC20 dest, uint srcQty, bool usePermissionless)
-        internal view
-        returns (bool didRevert, uint expectedRate, uint slippageRate)
-    {
-        bytes4 sig = usePermissionless ?
-            bytes4(keccak256("findBestRate(address,address,uint256)")) :
-            bytes4(keccak256("findBestRateOnlyPermission(address,address,uint256)")); //Function signatures
-
-        (didRevert, expectedRate) = assemblyFindBestRate(src, dest, srcQty, sig);
-
-        if (didRevert) return (true, 0, 0);
-
-        if (quantityFactor != 1) {
-            (didRevert, slippageRate) = assemblyFindBestRate(src, dest, (srcQty * quantityFactor), sig);
-        } else {
-            slippageRate = expectedRate;
-        }
-    }
-
-    function assemblyFindBestRate(ERC20 src, ERC20 dest, uint srcQty, bytes4 sig)
-        internal view
-        returns (bool didRevert, uint rate)
-    {
-        address addr = address(kyberNetwork);  // kyber address
-        uint success;
-
-        assembly {
-            let x := mload(0x40)        // "free memory pointer"
-            mstore(x,sig)               // function signature
-            mstore(add(x,0x04),src)     // src address padded to 32 bytes
-            mstore(add(x,0x24),dest)    // dest padded to 32 bytes
-            mstore(add(x,0x44),srcQty)  // uint 32 bytes
-            mstore(0x40,add(x,0xa4))    // set free storage pointer to empty space after output
-
-            // input size = sig + ERC20 (address) + ERC20 + uint
-            // = 4 + 32 + 32 + 32 = 100 = 0x64
-            success := staticcall(
-                gas,  // gas
-                addr, // Kyber addr
-                x,    // Inputs at location x
-                0x64, // Inputs size bytes
-                add(x, 0x64),    // output storage after input
-                0x40) // Output size are (uint, uint) = 64 bytes
-
-            rate := mload(add(x,0x84))  //Assign 2nd output to rate, first output not used,
-            mstore(0x40,x)    // Set empty storage pointer back to start position
-        }
-
-        if (success != 1) didRevert = true;
-    }
 }

contracts/KyberNetwork.sol

@@ -436,7 +436,12 @@ contract KyberNetwork is Withdrawable, Utils3, KyberNetworkInterface, Reentrancy
             searchBestRate(src, ETH_TOKEN_ADDRESS, srcAmount, usePermissionless);
 
         result.weiAmount = calcDestAmountWithDecimals(srcDecimals, ETH_DECIMALS, srcAmount, result.rateSrcToEth);
-
+        //if weiAmount is zero, return zero rate to avoid revert in ETH -> token call
+        if (result.weiAmount == 0) {
+            result.rate = 0;
+            return;
+        }
+
         (result.reserve2, result.rateEthToDest) =
             searchBestRate(ETH_TOKEN_ADDRESS, dest, result.weiAmount, usePermissionless);
 

contracts/mock/MaliciousReserve.sol

@@ -73,5 +73,36 @@ contract MaliciousReserve is KyberReserve {
     function setNumRecursive(uint num)  public {
         numRecursive = num;
     }
+
+    function getConversionRate(ERC20 src, ERC20 dest, uint srcQty, uint blockNumber) public view returns(uint) {
+        //reverts if srcAmount is zero
+        require(srcQty > 0);
+        ERC20 token;
+        bool  isBuy;
+
+        if (!tradeEnabled) return 0;
+
+        if (ETH_TOKEN_ADDRESS == src) {
+            isBuy = true;
+            token = dest;
+        } else if (ETH_TOKEN_ADDRESS == dest) {
+            isBuy = false;
+            token = src;
+        } else {
+            return 0; // pair is not listed
+        }
+
+        uint rate = conversionRatesContract.getRate(token, blockNumber, isBuy, srcQty);
+        uint destQty = getDestQty(src, dest, srcQty, rate);
+
+        if (getBalance(dest) < destQty) return 0;
+
+        if (sanityRatesContract != address(0)) {
+            uint sanityRate = sanityRatesContract.getSanityRate(src, dest);
+            if (rate > sanityRate) return 0;
+        }
+
+        return rate;
+    }
 }
 

test/expectedRate.js

@@ -2,9 +2,9 @@ const ConversionRates = artifacts.require("ConversionRates.sol");
 const EnhancedStepFunctions = artifacts.require("./EnhancedStepFunctions.sol");
 const TestToken = artifacts.require("mockContracts/TestToken.sol");
 const Reserve = artifacts.require("KyberReserve.sol");
+const MaliciousReserve = artifacts.require("MaliciousReserve.sol");
 const Network = artifacts.require("KyberNetwork.sol");
 const MockNetwork = artifacts.require("MockKyberNetwork.sol");
-const NetworkFailingGetRate = artifacts.require("NetworkFailingGetRate.sol");
 const WhiteList = artifacts.require("WhiteList.sol");
 const ExpectedRate = artifacts.require("ExpectedRate.sol");
 const FeeBurner = artifacts.require("FeeBurner.sol");
@@ -522,6 +522,40 @@ contract('ExpectedRate', function(accounts) {
         assert.equal(rates[1].valueOf(), 0, "unexpected rate");
     });
 
+    it("should verify when src qty 0, findBestRate, findBestRateOnlyPermission and getExpectedRate don't revert", async function() {
+        let tokenSrcInd = 2;
+        let tokenDestInd = 1;
+        let qty = 0;
+
+        //create bad reserve that reverts for zero src qty rate queries
+        let badReserve = await MaliciousReserve.new(network.address, pricing1.address, admin);
+
+        //try to get rate with zero src qty, should revert
+        try {
+            await badReserve.getConversionRate(tokenAdd[tokenSrcInd],tokenAdd[tokenDestInd],qty,0);
+            assert(false, "throw was expected in line above.")
+        } catch(e) {
+            assert(Helper.isRevertErrorMessage(e), "expected throw but got: " + e);
+        }
+
+        //add malicious reserve to network for ETH -> tokenDest
+        await network.addReserve(badReserve.address, false, {from: operator});
+        await network.listPairForReserve(badReserve.address, tokenAdd[tokenDestInd], true, true, true, {from: operator});
+
+        //try to get rate
+        rate = await network.findBestRate(tokenAdd[tokenSrcInd], tokenAdd[tokenDestInd], qty);
+        assert.equal(rate[1].valueOf(), 0, "did not return zero rate");
+
+        rate = await network.findBestRateOnlyPermission(tokenAdd[tokenSrcInd], tokenAdd[tokenDestInd], qty);
+        assert.equal(rate[1].valueOf(), 0, "did not return zero rate");
+
+        rate = await network.getExpectedRate(tokenAdd[tokenSrcInd], tokenAdd[tokenDestInd], qty);
+        assert(rate[0].valueOf() > 0, "unexpected rate");
+
+        //unlist and remove bad reserve
+        await network.listPairForReserve(badReserve.address, tokenAdd[tokenDestInd], true, true, false, {from: operator});    
+    });
+
     it("should disable the first reserve and add the second one with new conversion rate", async function() {
         await reserve1.disableTrade({from: alerter});
         await network.addReserve(reserve2.address, false, {from: operator});