Merge pull request #883 from soad003/bytesStringLength

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

@@ -14,5 +14,6 @@ module.exports = [
   require('./guardConditions'),
   require('./deleteDynamicArrays'),
   require('./assignAndCompare'),
-  require('./erc20Decimals')
+  require('./erc20Decimals'),
+  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 = {
@@ -871,6 +872,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) {
@@ -1017,6 +1032,8 @@ module.exports = {
   isAssertCall: isAssertCall,
   isRequireCall: isRequireCall,
   isIntDivision: isIntDivision,
+  isStringToBytesConversion: isStringToBytesConversion,
+  isBytesLengthCheck: isBytesLengthCheck,
 
   // #################### Trivial Node Identification
   isDeleteUnaryOperation: isDeleteUnaryOperation,

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

+var name = 'String Length: '
+var desc = 'Bytes length != String length'
+var categories = require('./categories')
+var common = require('./staticAnalysisCommon')
+
+function stringBytesLength () {
+  this.stringToBytesConversions = []
+  this.bytesLengthChecks = []
+}
+
+stringBytesLength.prototype.visit = function (node) {
+  if (common.isStringToBytesConversion(node)) this.stringToBytesConversions.push(node)
+  else if (common.isBytesLengthCheck(node)) this.bytesLengthChecks.push(node)
+}
+
+stringBytesLength.prototype.report = function (compilationResults) {
+  if (this.stringToBytesConversions.length > 0 && this.bytesLengthChecks.length > 0) {
+    return [{
+      warning: 'Bytes and string length are not the same since strings are assumed to be UTF-8 encoded (according to the ABI defintion) therefore one character is not nessesarily encoded in one byte of data.',
+      location: this.bytesLengthChecks[0],
+      more: 'https://github.com/ethereum/wiki/wiki/Ethereum-Contract-ABI#argument-encoding'
+    }]
+  } else {
+    return []
+  }
+}
+
+module.exports = {
+  name: name,
+  description: desc,
+  category: categories.MISC,
+  Module: stringBytesLength
+}
+

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

@@ -31,7 +31,8 @@ var testFiles = [
   'deleteDynamicArray.sol',
   'blockLevelCompare.sol',
   'intDivisionTruncate.sol',
-  'ERC20.sol'
+  'ERC20.sol',
+  'stringBytesLength.sol'
 ]
 
 var testFileAsts = {}
@@ -68,7 +69,8 @@ test('Integration test thisLocal.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -103,7 +105,8 @@ test('Integration test checksEffectsInteraction.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -138,7 +141,8 @@ test('Integration test constantFunctions.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -173,7 +177,8 @@ test('Integration test inlineAssembly.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -208,7 +213,8 @@ test('Integration test txOrigin.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -243,7 +249,8 @@ test('Integration test gasCosts.js', function (t) {
     'deleteDynamicArray.sol': 2,
     'blockLevelCompare.sol': 1,
     'intDivisionTruncate.sol': 1,
-    'ERC20.sol': 2
+    'ERC20.sol': 2,
+    'stringBytesLength.sol': 1
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -278,7 +285,8 @@ test('Integration test similarVariableNames.js', function (t) {
     'deleteDynamicArray.sol': 1,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -313,7 +321,8 @@ test('Integration test inlineAssembly.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -348,7 +357,8 @@ test('Integration test blockTimestamp.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -383,7 +393,8 @@ test('Integration test lowLevelCalls.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -418,7 +429,8 @@ test('Integration test blockBlockhash.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -453,7 +465,8 @@ test('Integration test noReturn.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -487,8 +500,9 @@ test('Integration test selfdestruct.js', function (t) {
     'selfdestruct.sol': 3,
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
+    'ERC20.sol': 0,
     'intDivisionTruncate.sol': 5,
-    'ERC20.sol': 0
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -523,7 +537,8 @@ test('Integration test guardConditions.js', function (t) {
     'deleteDynamicArray.sol': 1,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 1,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -558,7 +573,8 @@ test('Integration test deleteDynamicArrays.js', function (t) {
     'deleteDynamicArray.sol': 2,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -593,7 +609,8 @@ test('Integration test assignAndCompare.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 8,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -628,7 +645,8 @@ test('Integration test intDivisionTruncate.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 2,
-    'ERC20.sol': 0
+    'ERC20.sol': 0,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -663,7 +681,8 @@ test('Integration test erc20Decimal.js', function (t) {
     'deleteDynamicArray.sol': 0,
     'blockLevelCompare.sol': 0,
     'intDivisionTruncate.sol': 0,
-    'ERC20.sol': 1
+    'ERC20.sol': 1,
+    'stringBytesLength.sol': 0
   }
 
   runModuleOnFiles(module, t, (file, report) => {
@@ -671,6 +690,42 @@ test('Integration test erc20Decimal.js', function (t) {
   })
 })
 
+test('Integration test stringBytesLength.js', function (t) {
+  t.plan(testFiles.length)
+
+  var module = require('../../src/solidity-analyzer/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,
+    'ERC20.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