Merge pull request #766 from soad003/dynamicArrayDeleteWarning

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

+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

@@ -27,7 +27,10 @@ var basicTypes = {
   UINT: 'uint256',
   BOOL: 'bool',
   ADDRESS: 'address',
-  BYTES32: 'bytes32'
+  BYTES32: 'bytes32',
+  STRING_MEM: 'string memory',
+  BYTES_MEM: 'bytes memory',
+  BYTES4: 'bytes4'
 }
 
 var basicRegex = {
@@ -57,8 +60,12 @@ var builtinFunctions = {
   'mulmod(uint256,uint256,uint256)': true,
   'selfdestruct(address)': true,
   'revert()': true,
+  'revert(string memory)': true,
   'assert(bool)': true,
-  'require(bool)': true
+  'require(bool)': true,
+  'require(bool,string memory)': true,
+  'gasleft()': true,
+  'blockhash(uint)': true
 }
 
 var lowLevelCallTypes = {
@@ -78,6 +85,29 @@ var specialVariables = {
   }
 }
 
+var abiNamespace = {
+  ENCODE: {
+    obj: 'abi',
+    member: 'encode',
+    type: buildFunctionSignature([], [basicTypes.BYTES_MEM], false, 'pure')
+  },
+  ENCODEPACKED: {
+    obj: 'abi',
+    member: 'encodePacked',
+    type: buildFunctionSignature([], [basicTypes.BYTES_MEM], false, 'pure')
+  },
+  ENCODE_SELECT: {
+    obj: 'abi',
+    member: 'encodeWithSelector',
+    type: buildFunctionSignature([basicTypes.BYTES4], [basicTypes.BYTES_MEM], false, 'pure')
+  },
+  ENCODE_SIG: {
+    obj: 'abi',
+    member: 'encodeWithSignature',
+    type: buildFunctionSignature([basicTypes.STRING_MEM], [basicTypes.BYTES_MEM], false, 'pure')
+  }
+}
+
 // #################### Trivial Getters
 
 function getType (node) {
@@ -405,6 +435,24 @@ function hasFunctionBody (funcNode) {
 }
 
 /**
+ * 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
  * @return {bool}
@@ -419,7 +467,16 @@ function isLocalCallGraphRelevantNode (node) {
  * @return {bool}
  */
 function isBuiltinFunctionCall (node) {
-  return isLocalCall(node) && builtinFunctions[getLocalCallName(node) + '(' + getFunctionCallTypeParameterType(node) + ')'] === true
+  return (isLocalCall(node) && builtinFunctions[getLocalCallName(node) + '(' + getFunctionCallTypeParameterType(node) + ')'] === true) || isAbiNamespaceCall(node)
+}
+
+/**
+ * True if is builtin function like assert, sha3, erecover, ...
+ * @node {ASTNode} some AstNode
+ * @return {bool}
+ */
+function isAbiNamespaceCall (node) {
+  return Object.keys(abiNamespace).some((key) => abiNamespace.hasOwnProperty(key) && node.children && node.children[0] && isSpecialVariableAccess(node.children[0], abiNamespace[key]))
 }
 
 /**
@@ -541,6 +598,15 @@ function isPlusPlusUnaryOperation (node) {
 }
 
 /**
+ * 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
  * @return {bool}
@@ -778,8 +844,8 @@ function exactMatch (regexStr) {
  *  list of return type names
  * @return {Boolean} isPayable
  */
-function buildFunctionSignature (paramTypes, returnTypes, isPayable) {
-  return 'function (' + util.concatWithSeperator(paramTypes, ',') + ')' + ((isPayable) ? ' payable' : '') + ((returnTypes.length) ? ' returns (' + util.concatWithSeperator(returnTypes, ',') + ')' : '')
+function buildFunctionSignature (paramTypes, returnTypes, isPayable, additionalMods) {
+  return 'function (' + util.concatWithSeperator(paramTypes, ',') + ')' + ((isPayable) ? ' payable' : '') + ((additionalMods) ? ' ' + additionalMods : '') + ((returnTypes.length) ? ' returns (' + util.concatWithSeperator(returnTypes, ',') + ')' : '')
 }
 
 /**
@@ -827,6 +893,10 @@ module.exports = {
   getFunctionOrModifierDefinitionReturnParameterPart: getFunctionOrModifierDefinitionReturnParameterPart,
 
   // #################### Complex Node Identification
+  isDeleteOfDynamicArray: isDeleteOfDynamicArray,
+  isAbiNamespaceCall: isAbiNamespaceCall,
+  isSpecialVariableAccess: isSpecialVariableAccess,
+  isDynamicArrayAccess: isDynamicArrayAccess,
   hasFunctionBody: hasFunctionBody,
   isInteraction: isInteraction,
   isEffect: isEffect,
@@ -858,6 +928,7 @@ module.exports = {
   isRequireCall: isRequireCall,
 
   // #################### Trivial Node Identification
+  isDeleteUnaryOperation: isDeleteUnaryOperation,
   isFunctionDefinition: isFunctionDefinition,
   isModifierDefinition: isModifierDefinition,
   isInheritanceSpecifier: isInheritanceSpecifier,

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

+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;
+    }
+}
\ No newline at end of file