Static Analysis: Bugfix constant function check, Similar var names allow number prefix, add delete dynamic array modul

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

@@ -33,10 +33,10 @@ function report (contracts, multipleContractsWithSameName) {
     contract.functions.forEach((func) => {
       if (isPotentialVulnerableFunction(func, getContext(callGraph, contract, func))) {
         var funcName = common.getFullQuallyfiedFuncDefinitionIdent(contract.node, func.node, func.parameters)
-        var comments = (hasModifiers) ? '<br/><i>Note:</i> Modifiers are currently not considered by this static analysis.' : ''
-        comments += (multipleContractsWithSameName) ? '<br/><i>Note:</i> Import aliases are currently not supported by this static analysis.' : ''
+        var comments = (hasModifiers) ? 'Note: Modifiers are currently not considered by this static analysis.' : ''
+        comments += (multipleContractsWithSameName) ? 'Note: Import aliases are currently not supported by this static analysis.' : ''
         warnings.push({
-          warning: `Potential Violation of Checks-Effects-Interaction pattern in <i>${funcName}</i>: Could potentially lead to re-entrancy vulnerability. ${comments}`,
+          warning: `Potential Violation of Checks-Effects-Interaction pattern in ${funcName}: Could potentially lead to re-entrancy vulnerability. ${comments}`,
           location: func.src,
           more: 'http://solidity.readthedocs.io/en/develop/security-considerations.html#re-entrancy'
         })

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

@@ -16,7 +16,8 @@ function constantFunctions () {
               common.isLocalCallGraphRelevantNode(node) ||
               common.isInlineAssembly(node) ||
               common.isNewExpression(node) ||
-              common.isSelfdestructCall(node)
+              common.isSelfdestructCall(node) ||
+              common.isDeleteUnaryOperation(node)
   )
 
   this.report = this.abstractAst.build_report(report)
@@ -45,8 +46,8 @@ function report (contracts, multipleContractsWithSameName) {
     contract.functions.filter((func) => common.hasFunctionBody(func.node)).forEach((func) => {
       if (common.isConstantFunction(func.node) !== func.potentiallyshouldBeConst) {
         var funcName = common.getFullQuallyfiedFuncDefinitionIdent(contract.node, func.node, func.parameters)
-        var comments = (hasModifiers) ? '<br/><i>Note:</i> Modifiers are currently not considered by this static analysis.' : ''
-        comments += (multipleContractsWithSameName) ? '<br/><i>Note:</i> Import aliases are currently not supported by this static analysis.' : ''
+        var comments = (hasModifiers) ? 'Note: Modifiers are currently not considered by this static analysis.' : ''
+        comments += (multipleContractsWithSameName) ? 'Note: Import aliases are currently not supported by this static analysis.' : ''
         if (func.potentiallyshouldBeConst) {
           warnings.push({
             warning: `${funcName} : Potentially should be constant but is not. ${comments}`,
@@ -87,7 +88,8 @@ function isConstBreaker (node, context) {
         common.isCallToNonConstLocalFunction(node) ||
         common.isInlineAssembly(node) ||
         common.isNewExpression(node) ||
-        common.isSelfdestructCall(node)
+        common.isSelfdestructCall(node) ||
+        common.isDeleteUnaryOperation(node)
 }
 
 function isCallOnNonConstExternalInterfaceFunction (node, context) {

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

@@ -0,0 +1,29 @@
+var name = 'Delete on dynamic Array: '
+var desc = 'Use require and appropriately'
+var categories = require('./categories')
+var common = require('./staticAnalysisCommon')
+
+function deleteDynamicArrays () {
+  this.rel = []
+}
+
+deleteDynamicArrays.prototype.visit = function (node) {
+  if (common.isDeleteOfDynamicArray(node)) this.rel.push(node)
+}
+
+deleteDynamicArrays.prototype.report = function (compilationResults) {
+  return this.rel.map((node) => {
+    return {
+      warning: 'The “delete” operation when applied to a dynamically sized array in Solidity generates code to delete each of the elements contained. If the array is large, this operation can surpass the block gas limit and raise an OOG exception. Also nested dynamically sized objects can produce the same results.',
+      location: node.src,
+      more: 'http://solidity.readthedocs.io/en/latest/types.html?highlight=array#delete'
+    }
+  })
+}
+
+module.exports = {
+  name: name,
+  description: desc,
+  category: categories.GAS,
+  Module: deleteDynamicArrays
+}

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

@@ -14,7 +14,7 @@ guardConditions.prototype.visit = function (node) {
 guardConditions.prototype.report = function (compilationResults) {
   if (this.guards.length > 0) {
     return [{
-      warning: 'Use <i>assert(x)</i> if you never ever want <i>x</i> to be false, not in any circumstance (apart from a bug in your code). Use <i>require(x)</i> if <i>x</i> can be false, due to e.g. invalid input or a failing external component.',
+      warning: 'Use assert(x) if you never ever want x to be false, not in any circumstance (apart from a bug in your code). Use require(x) if x can be false, due to e.g. invalid input or a failing external component.',
       more: 'http://solidity.readthedocs.io/en/develop/control-structures.html#error-handling-assert-require-revert-and-exceptions'
     }]
   }

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

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

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

@@ -4,6 +4,8 @@ var categories = require('./categories')
 var common = require('./staticAnalysisCommon')
 var AbstractAst = require('./abstractAstView')
 var levenshtein = require('fast-levenshtein')
+var remixLib = require('remix-lib')
+var util = remixLib.util
 
 function similarVariableNames () {
   this.abstractAst = new AbstractAst()
@@ -53,7 +55,7 @@ function findSimilarVarNames (vars) {
   var similar = []
   var comb = {}
   vars.map((varName1) => vars.map((varName2) => {
-    if (varName1.length > 1 && varName2.length > 1 && varName2 !== varName1 && !isCommonPrefixedVersion(varName1, varName2) && !(comb[varName1 + ';' + varName2] || comb[varName2 + ';' + varName1])) {
+    if (varName1.length > 1 && varName2.length > 1 && varName2 !== varName1 && !isCommonPrefixedVersion(varName1, varName2) && !isCommonNrSuffixVersion(varName1, varName2) && !(comb[varName1 + ';' + varName2] || comb[varName2 + ';' + varName1])) {
       comb[varName1 + ';' + varName2] = true
       var distance = levenshtein.get(varName1, varName2)
       if (distance <= 2) similar.push({ var1: varName1, var2: varName2, distance: distance })
@@ -66,6 +68,12 @@ function isCommonPrefixedVersion (varName1, varName2) {
   return (varName1.startsWith('_') && varName1.slice(1) === varName2) || (varName2.startsWith('_') && varName2.slice(1) === varName1)
 }
 
+function isCommonNrSuffixVersion (varName1, varName2) {
+  var ref = '^' + util.escapeRegExp(varName1.slice(0, -1)) + '[0-9]$'
+
+  return varName2.match(ref) != null
+}
+
 function getFunctionVariables (contract, func) {
   return contract.stateVariables.concat(func.localVariables)
 }

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

@@ -58,7 +58,11 @@ var builtinFunctions = {
   'selfdestruct(address)': true,
   'revert()': true,
   'assert(bool)': true,
-  'require(bool)': true
+  'require(bool)': true,
+  'require(bool,string memory)': true,
+  'revert(string memory)': true,
+  'gasleft()': true,
+  'blockhash(uint)': true
 }
 
 var lowLevelCallTypes = {
@@ -404,6 +408,24 @@ function hasFunctionBody (funcNode) {
   return findFirstSubNodeLTR(funcNode, exactMatch(nodeTypes.BLOCK)) != null
 }
 
+/**
+ * True if node is a delete instruction of a dynamic array
+ * @node {ASTNode} node to check for
+ * @return {bool}
+ */
+function isDeleteOfDynamicArray (node) {
+  return isDeleteUnaryOperation(node) && isDynamicArrayAccess(node.children[0])
+}
+
+/**
+ * True if node is node is a ref to a dynamic array
+ * @node {ASTNode} node to check for
+ * @return {bool}
+ */
+function isDynamicArrayAccess (node) {
+  return node && nodeType(node, exactMatch(nodeTypes.IDENTIFIER)) && (node.attributes.type.endsWith('[] storage ref') || node.attributes.type === 'bytes storage ref' || node.attributes.type === 'string storage ref')
+}
+
 /**
  * True if call to code within the current contracts context including (delegate) library call
  * @node {ASTNode} some AstNode
@@ -540,6 +562,15 @@ function isPlusPlusUnaryOperation (node) {
   return nodeType(node, exactMatch(nodeTypes.UNARYOPERATION)) && operator(node, exactMatch(util.escapeRegExp('++')))
 }
 
+/**
+ * True if unary delete operation
+ * @node {ASTNode} some AstNode
+ * @return {bool}
+ */
+function isDeleteUnaryOperation (node) {
+  return nodeType(node, exactMatch(nodeTypes.UNARYOPERATION)) && operator(node, exactMatch(util.escapeRegExp('delete')))
+}
+
 /**
  * True if unary decrement operation
  * @node {ASTNode} some AstNode
@@ -827,6 +858,8 @@ module.exports = {
   getFunctionOrModifierDefinitionReturnParameterPart: getFunctionOrModifierDefinitionReturnParameterPart,
 
   // #################### Complex Node Identification
+  isDeleteOfDynamicArray: isDeleteOfDynamicArray,
+  isDynamicArrayAccess: isDynamicArrayAccess,
   hasFunctionBody: hasFunctionBody,
   isInteraction: isInteraction,
   isEffect: isEffect,
@@ -858,6 +891,7 @@ module.exports = {
   isRequireCall: isRequireCall,
 
   // #################### Trivial Node Identification
+  isDeleteUnaryOperation: isDeleteUnaryOperation,
   isFunctionDefinition: isFunctionDefinition,
   isModifierDefinition: isModifierDefinition,
   isInheritanceSpecifier: isInheritanceSpecifier,

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

@@ -2094,3 +2094,19 @@ test('staticAnalysisCommon: function call with of function with function paramet
 
   t.equals(common.getFunctionCallTypeParameterType(node1), 'function (uint256,uint256) pure returns (uint256),uint256,uint256', 'Extracts param right type')
 })
+
+test('staticAnalysisCommon: require call', function (t) {
+  t.plan(3)
+  var node = {'attributes': {'argumentTypes': null, 'isConstant': false, 'isLValue': false, 'isPure': false, 'isStructConstructorCall': false, 'lValueRequested': false, 'names': [null], 'type': 'tuple()', 'type_conversion': false}, 'children': [{'attributes': {'argumentTypes': [{'typeIdentifier': 't_bool', 'typeString': 'bool'}, {'typeIdentifier': 't_stringliteral_80efd193f332877914d93edb0b3ef5c6a7eecd00c6251c3fd7f146b60b40e6cd', 'typeString': 'literal_string \'fuu\''}], 'overloadedDeclarations': [90, 91], 'referencedDeclaration': 91, 'type': 'function (bool,string memory) pure', 'value': 'require'}, 'id': 50, 'name': 'Identifier', 'src': '462:7:0'}, {'attributes': {'argumentTypes': null, 'commonType': {'typeIdentifier': 't_address', 'typeString': 'address'}, 'isConstant': false, 'isLValue': false, 'isPure': false, 'lValueRequested': false, 'operator': '==', 'type': 'bool'}, 'children': [{'attributes': {'argumentTypes': null, 'isConstant': false, 'isLValue': false, 'isPure': false, 'lValueRequested': false, 'member_name': 'sender', 'referencedDeclaration': null, 'type': 'address'}, 'children': [{'attributes': {'argumentTypes': null, 'overloadedDeclarations': [null], 'referencedDeclaration': 87, 'type': 'msg', 'value': 'msg'}, 'id': 51, 'name': 'Identifier', 'src': '470:3:0'}], 'id': 52, 'name': 'MemberAccess', 'src': '470:10:0'}, {'attributes': {'argumentTypes': null, 'overloadedDeclarations': [null], 'referencedDeclaration': 10, 'type': 'address', 'value': 'owner'}, 'id': 53, 'name': 'Identifier', 'src': '484:5:0'}], 'id': 54, 'name': 'BinaryOperation', 'src': '470:19:0'}, {'attributes': {'argumentTypes': null, 'hexvalue': '667575', 'isConstant': false, 'isLValue': false, 'isPure': true, 'lValueRequested': false, 'subdenomination': null, 'token': 'string', 'type': 'literal_string \'fuu\'', 'value': 'fuu'}, 'id': 55, 'name': 'Literal', 'src': '491:5:0'}], 'id': 56, 'name': 'FunctionCall', 'src': '462:35:0'}
+
+  t.equals(common.isRequireCall(node), true)
+  t.equals(common.getFunctionCallType(node), 'function (bool,string memory) pure', 'Extracts right type')
+  t.equals(common.getFunctionCallTypeParameterType(node), 'bool,string memory', 'Extracts param right type')
+})
+
+test('staticAnalysisCommon: isDeleteOfDynamicArray', function (t) {
+  t.plan(2)
+  var node = {'attributes': {'argumentTypes': null, 'isConstant': false, 'isLValue': false, 'isPure': false, 'lValueRequested': false, 'operator': 'delete', 'prefix': true, 'type': 'tuple()'}, 'children': [{'attributes': {'argumentTypes': null, 'overloadedDeclarations': [null], 'referencedDeclaration': 4, 'type': 'uint256[] storage ref', 'value': 'users'}, 'id': 58, 'name': 'Identifier', 'src': '514:5:0'}], 'id': 59, 'name': 'UnaryOperation', 'src': '507:12:0'}
+  t.equals(common.isDeleteOfDynamicArray(node), true)
+  t.equals(common.isDynamicArrayAccess(node.children[0]), true, 'Extracts right type')
+})

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

@@ -27,7 +27,8 @@ var testFiles = [
   'transfer.sol',
   'ctor.sol',
   'forgottenReturn.sol',
-  'selfdestruct.sol'
+  'selfdestruct.sol',
+  'deleteDynamicArray.sol'
 ]
 
 var testFileAsts = {}
@@ -60,7 +61,8 @@ test('Integration test thisLocal.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -91,7 +93,8 @@ test('Integration test checksEffectsInteraction.js', function (t) {
     'transfer.sol': 1,
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -122,7 +125,8 @@ test('Integration test constantFunctions.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 1
+    'selfdestruct.sol': 1,
+    'deleteDynamicArray.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -153,7 +157,8 @@ test('Integration test inlineAssembly.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -184,7 +189,8 @@ test('Integration test txOrigin.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -215,7 +221,8 @@ test('Integration test gasCosts.js', function (t) {
     'transfer.sol': 1,
     'ctor.sol': 0,
     'forgottenReturn.sol': 3,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 2
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -246,7 +253,8 @@ test('Integration test similarVariableNames.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 1,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 1
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -277,7 +285,8 @@ test('Integration test inlineAssembly.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -308,7 +317,8 @@ test('Integration test blockTimestamp.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -339,7 +349,8 @@ test('Integration test lowLevelCalls.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -370,7 +381,8 @@ test('Integration test blockBlockhash.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -401,7 +413,8 @@ test('Integration test noReturn.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 0,
     'forgottenReturn.sol': 1,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -432,7 +445,8 @@ test('Integration test selfdestruct.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 2
+    'selfdestruct.sol': 2,
+    'deleteDynamicArray.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -463,7 +477,8 @@ test('Integration test guardConditions.js', function (t) {
     'transfer.sol': 0,
     'ctor.sol': 0,
     'forgottenReturn.sol': 0,
-    'selfdestruct.sol': 0
+    'selfdestruct.sol': 0,
+    'deleteDynamicArray.sol': 1
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -471,6 +486,38 @@ test('Integration test guardConditions.js', function (t) {
   })
 })
 
+test('Integration test deleteDynamicArrays.js', function (t) {
+  t.plan(testFiles.length)
+
+  var module = require('../../src/analysis/modules/deleteDynamicArrays')
+
+  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': 2
+  }
+
+  runModuleOnFiles(module, t, (file, report) => {
+    t.equal(report.length, lengthCheck[file], `${file} has right amount of deleteDynamicArrays warnings`)
+  })
+})
+
 // #################### Helpers
 function runModuleOnFiles (module, t, cb) {
   var statRunner = new StatRunner()

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

@@ -0,0 +1,37 @@
+pragma solidity ^0.4.22;
+contract arr {
+    uint[] users;
+    
+    bytes access_rights_per_user;
+    
+    uint user_index;
+     
+    address owner;
+    
+    string grr = "message";
+    
+    uint[100] last_100_users;
+    
+    constructor(address owner1) public {
+       owner = owner1;
+       user_index = 0;
+    }
+    
+    function addUser(uint id, byte rights) public{
+        users[user_index] = id;
+        last_100_users[user_index % 100] = id;
+        access_rights_per_user[user_index] = rights;
+        user_index++;
+    }
+    
+    function resetState() public{
+        require(msg.sender == owner, grr);
+        delete users;
+        delete access_rights_per_user;
+        delete last_100_users;
+    }  
+    
+    function bla(string bal) public {
+        grr = bal;
+    }
+}