Static Analysis: Compare instead of assign

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

@@ -0,0 +1,29 @@
+var name = 'Assign or Compare: '
+var desc = 'Assign and compare operators can be confusing.'
+var categories = require('./categories')
+var common = require('./staticAnalysisCommon')
+
+function assignAndCompare () {
+  this.warningNodes = []
+}
+
+assignAndCompare.prototype.visit = function (node) {
+  if (common.isBlockWithTopLevelUnAssignedBinOp(node)) this.warningNodes.push(node)
+}
+
+assignAndCompare.prototype.report = function (compilationResults) {
+  return this.warningNodes.map(function (item, i) {
+    return {
+      warning: 'Use of "this" for local functions: Never use this to call functions in the same contract, it only consumes more gas than normal local calls.',
+      location: item.src,
+      more: 'http://solidity.readthedocs.io/en/develop/control-structures.html#external-function-calls'
+    }
+  })
+}
+
+module.exports = {
+  name: name,
+  description: desc,
+  category: categories.MISC,
+  Module: assignAndCompare
+}

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,10 @@ var nodeTypes = {
   INLINEASSEMBLY: 'InlineAssembly',
   BLOCK: 'Block',
   NEWEXPRESSION: 'NewExpression',
-  RETURN: 'Return'
+  RETURN: 'Return',
+  IFSTATEMENT: 'IfStatement',
+  FORSTATEMENT: 'ForStatement',
+  WHILESTATEMENT: 'WhileStatement'
 }
 
 var basicTypes = {
@@ -145,7 +149,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
 }
 
@@ -423,6 +427,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
 
 /**
@@ -588,6 +610,30 @@ function isConstructor (node) {
   )
 }
 
+/**
+ * True if is block 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 isBlockWithTopLevelUnAssignedBinOp (node) {
+  return nodeType(node, exactMatch(nodeTypes.BLOCK)) && node.children && node.children.some(isBinaryOpInExpression) ||
+          isBlockLikeStatement(node) && node.children && node.children.some(isBinaryOpInExpression) // Second Case for if without braces
+}
+
+function isBlockLikeStatement (node) {
+  return (nodeType(node, exactMatch(nodeTypes.IFSTATEMENT)) || nodeType(node, exactMatch(nodeTypes.FORSTATEMENT)) || nodeType(node, exactMatch(nodeTypes.WHILESTATEMENT))) &&
+          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
@@ -897,6 +943,7 @@ module.exports = {
   isAbiNamespaceCall: isAbiNamespaceCall,
   isSpecialVariableAccess: isSpecialVariableAccess,
   isDynamicArrayAccess: isDynamicArrayAccess,
+  isBlockWithTopLevelUnAssignedBinOp: isBlockWithTopLevelUnAssignedBinOp,
   hasFunctionBody: hasFunctionBody,
   isInteraction: isInteraction,
   isEffect: isEffect,

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,8 @@ var testFiles = [
   'ctor.sol',
   'forgottenReturn.sol',
   'selfdestruct.sol',
-  'deleteDynamicArray.sol'
+  'deleteDynamicArray.sol',
+  'blockLevelCompare.sol'
 ]
 
 var testFileAsts = {}
@@ -62,7 +63,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -94,7 +96,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -126,7 +129,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -158,7 +162,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -190,7 +195,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -222,7 +228,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -254,7 +261,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -286,7 +294,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -318,7 +327,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -350,7 +360,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -382,7 +393,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -414,7 +426,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -446,7 +459,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -478,7 +492,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -510,7 +525,8 @@ 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
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -518,13 +534,50 @@ 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': 6
+  }
+
+  runModuleOnFiles(module, t, (file, report) => {
+    t.equal(report.length, lengthCheck[file], `${file} has right amount of assignAndCompare 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

@@ -0,0 +1,43 @@
+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;
+
+            c == 5;
+
+        }
+
+        breaker = false;
+    }
+    
+}