Static Analysis: warn when bytes(str).length is used to eval string length

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

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

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

@@ -25,7 +25,8 @@ var nodeTypes = {
   IFSTATEMENT: 'IfStatement',
   FORSTATEMENT: 'ForStatement',
   WHILESTATEMENT: 'WhileStatement',
-  DOWHILESTATEMENT: 'DoWhileStatement'
+  DOWHILESTATEMENT: 'DoWhileStatement',
+  ELEMENTARYTYPENAMEEXPRESSION: 'ElementaryTypeNameExpression'
 }
 
 var basicTypes = {
@@ -859,6 +860,20 @@ function isTransfer (node) {
           undefined, exactMatch(basicTypes.ADDRESS), exactMatch(lowLevelCallTypes.TRANSFER.ident))
 }
 
+function isStringToBytesConversion (node) {
+  return isExplicitCast(node, util.escapeRegExp('string *'), util.escapeRegExp('bytes'))
+}
+
+function isExplicitCast (node, castFromType, castToType) {
+  return nodeType(node, exactMatch(nodeTypes.FUNCTIONCALL)) && nrOfChildren(node, 2) &&
+          nodeType(node.children[0], exactMatch(nodeTypes.ELEMENTARYTYPENAMEEXPRESSION)) && name(node.children[0], castToType) &&
+          nodeType(node.children[1], exactMatch(nodeTypes.IDENTIFIER)) && expressionType(node.children[1], castFromType)
+}
+
+function isBytesLengthCheck (node) {
+  return isMemberAccess(node, exactMatch(util.escapeRegExp(basicTypes.UINT)), undefined, util.escapeRegExp('bytes *'), 'length')
+}
+
 // #################### Complex Node Identification - Private
 
 function isMemberAccess (node, retType, accessor, accessorType, memberName) {
@@ -1000,6 +1015,8 @@ module.exports = {
   isAssertCall: isAssertCall,
   isRequireCall: isRequireCall,
   isIntDivision: isIntDivision,
+  isStringToBytesConversion: isStringToBytesConversion,
+  isBytesLengthCheck: isBytesLengthCheck,
 
   // #################### Trivial Node Identification
   isDeleteUnaryOperation: isDeleteUnaryOperation,

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

@@ -30,7 +30,8 @@ var testFiles = [
   'selfdestruct.sol',
   'deleteDynamicArray.sol',
   'blockLevelCompare.sol',
-  'intDivisionTruncate.sol'
+  'intDivisionTruncate.sol',
+  'stringBytesLength.sol'
 ]
 
 var testFileAsts = {}
@@ -66,7 +67,8 @@ test('Integration test thisLocal.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -100,7 +102,8 @@ test('Integration test checksEffectsInteraction.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -134,7 +137,8 @@ test('Integration test constantFunctions.js', function (t) {
     'selfdestruct.sol': 1,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -168,7 +172,8 @@ test('Integration test inlineAssembly.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -202,7 +207,8 @@ test('Integration test txOrigin.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -236,7 +242,8 @@ test('Integration test gasCosts.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 2,
     'blockLevelCompare.sol': 1,
-    'intDivisionTruncate.sol': 1
+    'intDivisionTruncate.sol': 1,
+    'stringBytesLength.sol': 1
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -270,7 +277,8 @@ test('Integration test similarVariableNames.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 1,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -304,7 +312,8 @@ test('Integration test inlineAssembly.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -338,7 +347,8 @@ test('Integration test blockTimestamp.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -372,7 +382,8 @@ test('Integration test lowLevelCalls.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -406,7 +417,8 @@ test('Integration test blockBlockhash.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -440,7 +452,8 @@ test('Integration test noReturn.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -474,7 +487,8 @@ test('Integration test selfdestruct.js', function (t) {
     'selfdestruct.sol': 3,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 5
+    'intDivisionTruncate.sol': 1, // 5
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -508,7 +522,8 @@ test('Integration test guardConditions.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 1,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 1
+    'intDivisionTruncate.sol': 1,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -542,7 +557,8 @@ test('Integration test deleteDynamicArrays.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 2,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -576,7 +592,8 @@ test('Integration test assignAndCompare.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 8,
-    'intDivisionTruncate.sol': 0
+    'intDivisionTruncate.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -610,7 +627,8 @@ test('Integration test intDivisionTruncate.js', function (t) {
     'selfdestruct.sol': 0,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
-    'intDivisionTruncate.sol': 2
+    'intDivisionTruncate.sol': 2,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -618,6 +636,41 @@ test('Integration test intDivisionTruncate.js', function (t) {
   })
 })
 
+test('Integration test stringBytesLength.js', function (t) {
+  t.plan(testFiles.length)
+
+  var module = require('../../src/analysis/modules/stringBytesLength')
+
+  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': 0,
+    'stringBytesLength.sol': 1
+  }
+
+  runModuleOnFiles(module, t, (file, report) => {
+    t.equal(report.length, lengthCheck[file], `${file} has right amount of stringBytesLength warnings`)
+  })
+})
+
 // #################### Helpers
 function runModuleOnFiles (module, t, cb) {
   var statRunner = new StatRunner()

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

+pragma solidity ^0.4.17;
+contract bytesString {
+    
+    function length(string a) public pure returns(uint) {
+        bytes memory x = bytes(a);
+
+        return x.length;
+    }
+
+}
\ No newline at end of file