Merge pull request #786 from soad003/staticAnalysisAssign

remix-solidity/src/analysis/modules/assignAndCompare.js

+var name = 'Result not used: '
+var desc = 'The result of an operation was not used.'
+var categories = require('./categories')
+var common = require('./staticAnalysisCommon')
+
+function assignAndCompare () {
+  this.warningNodes = []
+}
+
+assignAndCompare.prototype.visit = function (node) {
+  if (common.isSubScopeWithTopLevelUnAssignedBinOp(node)) common.getUnAssignedTopLevelBinOps(node).forEach((n) => this.warningNodes.push(n))
+}
+
+assignAndCompare.prototype.report = function (compilationResults) {
+  return this.warningNodes.map(function (item, i) {
+    return {
+      warning: 'A binary operation yields a value that is not used in the following. This is often caused by confusing assignment (=) and comparison (==).',
+      location: item.src
+    }
+  })
+}
+
+module.exports = {
+  name: name,
+  description: desc,
+  category: categories.MISC,
+  Module: assignAndCompare
+}

remix-solidity/src/analysis/modules/intDivisionTruncate.js

+var name = 'Data Trucated: '
+var desc = 'Division on int/uint values truncates the result.'
+var categories = require('./categories')
+var common = require('./staticAnalysisCommon')
+
+function intDivitionTruncate () {
+  this.warningNodes = []
+}
+
+intDivitionTruncate.prototype.visit = function (node) {
+  if (common.isIntDivision(node)) this.warningNodes.push(node)
+}
+
+intDivitionTruncate.prototype.report = function (compilationResults) {
+  return this.warningNodes.map(function (item, i) {
+    return {
+      warning: 'Division of integer values yields an integer value again. That means eg. a / 100 = 0 instead of 0.a since the result is an integer again. This does not hold for division of (only) literal values since those yield rational constants.',
+      location: item.src
+    }
+  })
+}
+
+module.exports = {
+  name: name,
+  description: desc,
+  category: categories.MISC,
+  Module: intDivitionTruncate
+}

remix-solidity/src/analysis/modules/list.js

@@ -12,5 +12,6 @@ module.exports = [
   require('./noReturn'),
   require('./selfdestruct'),
   require('./guardConditions'),
-  require('./deleteDynamicArrays')
+  require('./deleteDynamicArrays'),
+  require('./assignAndCompare')
 ]

remix-solidity/src/analysis/modules/staticAnalysisCommon.js

@@ -12,6 +12,7 @@ var nodeTypes = {
   ASSIGNMENT: 'Assignment',
   CONTRACTDEFINITION: 'ContractDefinition',
   UNARYOPERATION: 'UnaryOperation',
+  BINARYOPERATION: 'BinaryOperation',
   EXPRESSIONSTATEMENT: 'ExpressionStatement',
   MODIFIERDEFINITION: 'ModifierDefinition',
   MODIFIERINVOCATION: 'ModifierInvocation',
@@ -20,7 +21,11 @@ var nodeTypes = {
   INLINEASSEMBLY: 'InlineAssembly',
   BLOCK: 'Block',
   NEWEXPRESSION: 'NewExpression',
-  RETURN: 'Return'
+  RETURN: 'Return',
+  IFSTATEMENT: 'IfStatement',
+  FORSTATEMENT: 'ForStatement',
+  WHILESTATEMENT: 'WhileStatement',
+  DOWHILESTATEMENT: 'DoWhileStatement'
 }
 
 var basicTypes = {
@@ -145,7 +150,7 @@ function getEffectedVariableName (effectNode) {
  * @return {string} name of the function called
  */
 function getLocalCallName (localCallNode) {
-  if (!isLocalCall(localCallNode)) throw new Error('staticAnalysisCommon.js: not an local call Node')
+  if (!isLocalCall(localCallNode) && !isAbiNamespaceCall(localCallNode)) throw new Error('staticAnalysisCommon.js: not an local call Node')
   return localCallNode.children[0].attributes.value
 }
 
@@ -381,6 +386,10 @@ function getFullQuallyfiedFuncDefinitionIdent (contract, func, paramTypes) {
   return getContractName(contract) + '.' + getFunctionDefinitionName(func) + '(' + util.concatWithSeperator(paramTypes, ',') + ')'
 }
 
+function getUnAssignedTopLevelBinOps (subScope) {
+  return subScope.children.filter(isBinaryOpInExpression)
+}
+
 // #################### Trivial Node Identification
 
 function isFunctionDefinition (node) {
@@ -423,6 +432,24 @@ function isNewExpression (node) {
   return nodeType(node, exactMatch(nodeTypes.NEWEXPRESSION))
 }
 
+/**
+ * True if is Expression
+ * @node {ASTNode} some AstNode
+ * @return {bool}
+ */
+function isExpressionStatement (node) {
+  return nodeType(node, exactMatch(nodeTypes.EXPRESSIONSTATEMENT))
+}
+
+/**
+ * True if is binaryop
+ * @node {ASTNode} some AstNode
+ * @return {bool}
+ */
+function isBinaryOperation (node) {
+  return nodeType(node, exactMatch(nodeTypes.BINARYOPERATION))
+}
+
 // #################### Complex Node Identification
 
 /**
@@ -589,6 +616,42 @@ function isConstructor (node) {
 }
 
 /**
+ * True if node is integer division that truncates (not only int literals since those yield a rational value)
+ * @node {ASTNode} some AstNode
+ * @return {bool}
+ */
+function isIntDivision (node) {
+  return isBinaryOperation(node) && operator(node, exactMatch(util.escapeRegExp('/'))) && expressionType(node, util.escapeRegExp('int'))
+}
+
+/**
+ * True if is block / SubScope has top level binops (e.g. that are not assigned to anything, most of the time confused compare instead of assign)
+ * @node {ASTNode} some AstNode
+ * @return {bool}
+ */
+function isSubScopeWithTopLevelUnAssignedBinOp (node) {
+  return nodeType(node, exactMatch(nodeTypes.BLOCK)) && node.children && node.children.some(isBinaryOpInExpression) ||
+          isSubScopeStatement(node) && node.children && node.children.some(isBinaryOpInExpression) // Second Case for if without braces
+}
+
+function isSubScopeStatement (node) {
+  return (nodeType(node, exactMatch(nodeTypes.IFSTATEMENT)) ||
+            nodeType(node, exactMatch(nodeTypes.FORSTATEMENT)) ||
+            nodeType(node, exactMatch(nodeTypes.WHILESTATEMENT)) ||
+            nodeType(node, exactMatch(nodeTypes.DOWHILESTATEMENT))) &&
+          minNrOfChildren(node, 2) && !nodeType(node.children[1], exactMatch(nodeTypes.BLOCK))
+}
+
+/**
+ * True if binary operation inside of expression statement
+ * @node {ASTNode} some AstNode
+ * @return {bool}
+ */
+function isBinaryOpInExpression (node) {
+  return isExpressionStatement(node) && nrOfChildren(node, 1) && isBinaryOperation(node.children[0])
+}
+
+/**
  * True if unary increment operation
  * @node {ASTNode} some AstNode
  * @return {bool}
@@ -891,12 +954,14 @@ module.exports = {
   getStateVariableDeclarationsFormContractNode: getStateVariableDeclarationsFormContractNode,
   getFunctionOrModifierDefinitionParameterPart: getFunctionOrModifierDefinitionParameterPart,
   getFunctionOrModifierDefinitionReturnParameterPart: getFunctionOrModifierDefinitionReturnParameterPart,
+  getUnAssignedTopLevelBinOps: getUnAssignedTopLevelBinOps,
 
   // #################### Complex Node Identification
   isDeleteOfDynamicArray: isDeleteOfDynamicArray,
   isAbiNamespaceCall: isAbiNamespaceCall,
   isSpecialVariableAccess: isSpecialVariableAccess,
   isDynamicArrayAccess: isDynamicArrayAccess,
+  isSubScopeWithTopLevelUnAssignedBinOp: isSubScopeWithTopLevelUnAssignedBinOp,
   hasFunctionBody: hasFunctionBody,
   isInteraction: isInteraction,
   isEffect: isEffect,
@@ -926,6 +991,7 @@ module.exports = {
   isSelfdestructCall: isSelfdestructCall,
   isAssertCall: isAssertCall,
   isRequireCall: isRequireCall,
+  isIntDivision: isIntDivision,
 
   // #################### Trivial Node Identification
   isDeleteUnaryOperation: isDeleteUnaryOperation,

remix-solidity/src/analysis/staticAnalysisRunner.js

@@ -27,7 +27,7 @@ staticAnalysisRunner.prototype.runWithModuleList = function (compilationResult,
             item.mod.visit(node)
           } catch (e) {
             reports.push({
-              name: item.name, report: [{ warning: 'INTERNAL ERROR in module ' + item.name + ' ' + e.message }]
+              name: item.name, report: [{ warning: 'INTERNAL ERROR in module ' + item.name + ' ' + e.message, error: e.stack }]
             })
           }
         }
@@ -43,7 +43,7 @@ staticAnalysisRunner.prototype.runWithModuleList = function (compilationResult,
     try {
       report = item.mod.report(compilationResult)
     } catch (e) {
-      report = [{ warning: 'INTERNAL ERROR in module ' + item.name + ' ' + e.message }]
+      report = [{ warning: 'INTERNAL ERROR in module ' + item.name + ' ' + e.message, error: e.stack }]
     }
     return { name: item.name, report: report }
   }))

remix-solidity/test/analysis/staticAnalysisIntegration-test.js

@@ -28,7 +28,9 @@ var testFiles = [
   'ctor.sol',
   'forgottenReturn.sol',
   'selfdestruct.sol',
-  'deleteDynamicArray.sol'
+  'deleteDynamicArray.sol',
+  'blockLevelCompare.sol',
+  'intDivisionTruncate.sol'
 ]
 
 var testFileAsts = {}
@@ -62,7 +64,9 @@ test('Integration test thisLocal.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 0
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -94,7 +98,9 @@ test('Integration test checksEffectsInteraction.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 0
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -126,7 +132,9 @@ test('Integration test constantFunctions.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 1,
-    'deleteDynamicArray.sol': 0
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -158,7 +166,9 @@ test('Integration test inlineAssembly.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 0
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -190,7 +200,9 @@ test('Integration test txOrigin.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 0
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -222,7 +234,9 @@ test('Integration test gasCosts.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 3,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 2
+    'deleteDynamicArray.sol': 2,
+    'blockLevelCompare.sol': 1,
+    'intDivisionTruncate.sol': 1
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -254,7 +268,9 @@ test('Integration test similarVariableNames.js', function (t) {
     'ctor.sol': 1,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 1
+    'deleteDynamicArray.sol': 1,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -286,7 +302,9 @@ test('Integration test inlineAssembly.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 0
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -318,7 +336,9 @@ test('Integration test blockTimestamp.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 0
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -350,7 +370,9 @@ test('Integration test lowLevelCalls.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 0
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -382,7 +404,9 @@ test('Integration test blockBlockhash.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 0
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -414,7 +438,9 @@ test('Integration test noReturn.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 1,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 0
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -446,7 +472,9 @@ test('Integration test selfdestruct.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 2,
-    'deleteDynamicArray.sol': 0
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 1
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -478,7 +506,9 @@ test('Integration test guardConditions.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 1
+    'deleteDynamicArray.sol': 1,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 1
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -510,7 +540,9 @@ test('Integration test deleteDynamicArrays.js', function (t) {
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
     'selfdestruct.sol': 0,
-    'deleteDynamicArray.sol': 2
+    'deleteDynamicArray.sol': 2,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -518,13 +550,85 @@ test('Integration test deleteDynamicArrays.js', function (t) {
   })
 })
 
+test('Integration test assignAndCompare.js', function (t) {
+  t.plan(testFiles.length)
+
+  var module = require('../../src/analysis/modules/assignAndCompare')
+
+  var lengthCheck = {
+    'KingOfTheEtherThrone.sol': 0,
+    'assembly.sol': 0,
+    'ballot.sol': 0,
+    'ballot_reentrant.sol': 0,
+    'ballot_withoutWarnings.sol': 0,
+    'cross_contract.sol': 0,
+    'inheritance.sol': 0,
+    'modifier1.sol': 0,
+    'modifier2.sol': 0,
+    'notReentrant.sol': 0,
+    'structReentrant.sol': 0,
+    'thisLocal.sol': 0,
+    'globals.sol': 0,
+    'library.sol': 0,
+    'transfer.sol': 0,
+    'ctor.sol': 0,
+    'forgottenReturn.sol': 0,
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 8,
+    'intDivisionTruncate.sol': 0
+  }
+
+  runModuleOnFiles(module, t, (file, report) => {
+    t.equal(report.length, lengthCheck[file], `${file} has right amount of assignAndCompare warnings`)
+  })
+})
+
+test('Integration test intDivisionTruncate.js', function (t) {
+  t.plan(testFiles.length)
+
+  var module = require('../../src/analysis/modules/intDivisionTruncate')
+
+  var lengthCheck = {
+    'KingOfTheEtherThrone.sol': 0,
+    'assembly.sol': 0,
+    'ballot.sol': 0,
+    'ballot_reentrant.sol': 0,
+    'ballot_withoutWarnings.sol': 0,
+    'cross_contract.sol': 0,
+    'inheritance.sol': 0,
+    'modifier1.sol': 0,
+    'modifier2.sol': 0,
+    'notReentrant.sol': 0,
+    'structReentrant.sol': 0,
+    'thisLocal.sol': 0,
+    'globals.sol': 0,
+    'library.sol': 0,
+    'transfer.sol': 0,
+    'ctor.sol': 0,
+    'forgottenReturn.sol': 0,
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 0,
+    'blockLevelCompare.sol': 0,
+    'intDivisionTruncate.sol': 2
+  }
+
+  runModuleOnFiles(module, t, (file, report) => {
+    t.equal(report.length, lengthCheck[file], `${file} has right amount of intDivisionTruncate warnings`)
+  })
+})
+
 // #################### Helpers
 function runModuleOnFiles (module, t, cb) {
   var statRunner = new StatRunner()
 
   testFiles.forEach((fileName) => {
     statRunner.runWithModuleList(testFileAsts[fileName], [{ name: module.name, mod: new module.Module() }], (reports) => {
-      cb(fileName, reports[0].report)
+      let report = reports[0].report
+      if (report.some((x) => x['warning'].includes('INTERNAL ERROR'))) {
+        t.comment('Error while executing Module: ' + JSON.stringify(report))
+      }
+      cb(fileName, report)
     })
   })
 }

remix-solidity/test/analysis/test-contracts/blockLevelCompare.sol

+pragma solidity ^0.4.22;
+contract grr {
+    bool breaker;
+    
+    function() public {
+        uint a = 1;
+        string memory sig = "withdraw()";
+        uint b = 3;
+        
+        bytes4 selector = bytes4(keccak256(sig));
+        
+        abi.encode(a,b);
+        
+        abi.encodePacked(a,b);
+        
+        a = -b;
+        
+        a == b;
+
+        if(a == b) {
+            abi.encodeWithSelector(selector, a, b);
+            abi.encodeWithSignature(sig, a, b);        
+        }
+
+        if(b < 4) { a == b; }
+
+        if(b > 4) b == a;
+
+        while(true) a == b;
+
+        for(int i = 0; i < 3; i++) b == a;
+
+        while(false) {
+            int c = 3;
+            uint(c) + a;
+
+            c == 5;
+
+        }
+
+        a + b;
+        breaker = false;
+    }
+    
+}
\ No newline at end of file

remix-solidity/test/analysis/test-contracts/intDivisionTruncate.sol

+pragma solidity ^0.4.19;
+
+contract CharityCampaign {
+    mapping (address => uint) contributions; 
+    int128 feePercentage;
+    uint p2;
+    address processor;
+    address beneficiary;
+
+    function CharityCampaign(address _beneficiary, int128 _feePercentage) public {
+        processor = msg.sender;
+        beneficiary = _beneficiary;
+        feePercentage = _feePercentage;
+    }
+
+    function contribute() payable public returns (uint feeCollected) {
+        uint fee = msg.value * uint256(feePercentage / 100);
+        fee = msg.value * (p2 / 100);
+        contributions[msg.sender] = msg.value - fee;
+        processor.transfer(fee);
+        return fee;
+    }
+
+    function endCampaign() public {
+        require(msg.sender == processor || msg.sender == beneficiary);
+        selfdestruct(beneficiary);
+    }
+}
\ No newline at end of file