Merge pull request #978 from ethereum/staticAnalysisModul

remix-analyzer/src/solidity-analyzer/modules/deleteFromDynamicArray.js

+var name = 'Delete from dynamic Array: '
+var desc = 'Using delete on an array leaves a gap'
+var categories = require('./categories')
+var common = require('./staticAnalysisCommon')
+
+function deleteFromDynamicArray () {
+  this.relevantNodes = []
+}
+
+deleteFromDynamicArray.prototype.visit = function (node) {
+  if (common.isDeleteFromDynamicArray(node)) this.relevantNodes.push(node)
+}
+
+deleteFromDynamicArray.prototype.report = function (compilationResults) {
+  return this.relevantNodes.map((node) => {
+    return {
+      warning: 'Using delete on an array leaves a gap. The length of the array remains the same. If you want to remove the empty position you need to shift items manually and update the length property.\n',
+      location: node.src,
+      more: 'https://github.com/miguelmota/solidity-idiosyncrasies'
+    }
+  })
+}
+
+module.exports = {
+  name: name,
+  description: desc,
+  category: categories.MISC,
+  Module: deleteFromDynamicArray
+}

remix-analyzer/src/solidity-analyzer/modules/forLoopIteratesOverDynamicArray.js

+var name = 'For loop iterates over dynamic array: '
+var desc = 'The number of \'for\' loop iterations depends on dynamic array\'s size'
+var categories = require('./categories')
+var common = require('./staticAnalysisCommon')
+
+function forLoopIteratesOverDynamicArray () {
+  this.relevantNodes = []
+}
+
+forLoopIteratesOverDynamicArray.prototype.visit = function (node) {
+  if (common.isForLoop(node) &&
+    node.children[1].children[1].attributes.member_name === 'length' &&
+    node.children[1].children[1].children[0].attributes.type.indexOf('[]') !== -1) {
+    this.relevantNodes.push(node)
+  }
+}
+
+forLoopIteratesOverDynamicArray.prototype.report = function (compilationResults) {
+  return this.relevantNodes.map((node) => {
+    return {
+      warning: 'Loops that do not have a fixed number of iterations, for example, loops that depend on storage values, have to be used carefully: Due to the block gas limit, transactions can only consume a certain amount of gas. The number of iterations in a loop can grow beyond the block gas limit which can cause the complete contract to be stalled at a certain point. Additionally, using unbounded loops incurs in a lot of avoidable gas costs. Carefully test how many items at maximum you can pass to such functions to make it successful.',
+      location: node.src,
+      more: 'http://solidity.readthedocs.io/en/v0.4.24/security-considerations.html#gas-limit-and-loops'
+    }
+  })
+}
+
+module.exports = {
+  name: name,
+  description: desc,
+  category: categories.GAS,
+  Module: forLoopIteratesOverDynamicArray
+}

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

@@ -15,5 +15,7 @@ module.exports = [
   require('./deleteDynamicArrays'),
   require('./assignAndCompare'),
   require('./erc20Decimals'),
-  require('./stringBytesLength')
+  require('./stringBytesLength'),
+  require('./deleteFromDynamicArray'),
+  require('./forLoopIteratesOverDynamicArray')
 ]

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

@@ -71,7 +71,8 @@ var builtinFunctions = {
   'require(bool)': true,
   'require(bool,string memory)': true,
   'gasleft()': true,
-  'blockhash(uint)': true
+  'blockhash(uint)': true,
+  'address(address)': true
 }
 
 var lowLevelCallTypes = {
@@ -501,6 +502,24 @@ function isDynamicArrayAccess (node) {
 }
 
 /**
+ * True if node is a delete instruction for an element from a dynamic array
+ * @node {ASTNode} node to check for
+ * @return {bool}
+ */
+function isDeleteFromDynamicArray (node) {
+  return isDeleteUnaryOperation(node) && isIndexAccess(node.children[0])
+}
+
+/**
+ * True if node is the access of an index
+ * @node {ASTNode} node to check for
+ * @return {bool}
+ */
+function isIndexAccess (node) {
+  return node && node.name === 'IndexAccess'
+}
+
+/**
  * True if call to code within the current contracts context including (delegate) library call
  * @node {ASTNode} some AstNode
  * @return {bool}
@@ -886,6 +905,15 @@ function isBytesLengthCheck (node) {
   return isMemberAccess(node, exactMatch(util.escapeRegExp(basicTypes.UINT)), undefined, util.escapeRegExp('bytes *'), 'length')
 }
 
+/**
+ * True if it is a 'for' loop
+ * @node {ASTNode} some AstNode
+ * @return {bool}
+ */
+function isForLoop (node) {
+  return nodeType(node, exactMatch(nodeTypes.FORSTATEMENT))
+}
+
 // #################### Complex Node Identification - Private
 
 function isMemberAccess (node, retType, accessor, accessorType, memberName) {
@@ -998,9 +1026,11 @@ module.exports = {
 
   // #################### Complex Node Identification
   isDeleteOfDynamicArray: isDeleteOfDynamicArray,
+  isDeleteFromDynamicArray: isDeleteFromDynamicArray,
   isAbiNamespaceCall: isAbiNamespaceCall,
   isSpecialVariableAccess: isSpecialVariableAccess,
   isDynamicArrayAccess: isDynamicArrayAccess,
+  isIndexAccess: isIndexAccess,
   isSubScopeWithTopLevelUnAssignedBinOp: isSubScopeWithTopLevelUnAssignedBinOp,
   hasFunctionBody: hasFunctionBody,
   isInteraction: isInteraction,
@@ -1034,6 +1064,7 @@ module.exports = {
   isIntDivision: isIntDivision,
   isStringToBytesConversion: isStringToBytesConversion,
   isBytesLengthCheck: isBytesLengthCheck,
+  isForLoop: isForLoop,
 
   // #################### Trivial Node Identification
   isDeleteUnaryOperation: isDeleteUnaryOperation,

remix-analyzer/test/analysis/test-contracts/deleteFromDynamicArray.sol

+pragma solidity ^0.4.22;
+contract arr {
+    uint[] array = [1,2,3];
+    function removeAtIndex() public returns (uint[]) {
+        delete array[1];
+        return array;
+    }
+
+    // TODO: deleteFromDynamicArray should not generate warnings if array item is shifted and removed
+    /* function safeRemoveAtIndex(uint index) returns (uint[]) {
+        if (index >= array.length) return;
+
+        for (uint i = index; i < array.length-1; i++) {
+            array[i] = array[i+1];
+        }
+
+        delete array[array.length-1];
+        array.length--;
+
+        return array;
+    } */
+}

remix-analyzer/test/analysis/test-contracts/forLoopIteratesOverDynamicArray.sol

+pragma solidity ^0.4.22;
+contract forLoopArr {
+    uint[] array;
+    constructor(uint[] _array) {
+        array = _array;
+    }
+
+    function shiftArrItem(uint index) returns(uint[]) {
+        // TODO: for (uint i = index; i < array.length-1; i++) should also generate warning
+        for (uint i = index; i < array.length; i++) {
+            array[i] = array[i+1];
+        }
+        return array;
+    }
+}