diff --git a/remix-solidity/src/analysis/modules/checksEffectsInteraction.js b/remix-solidity/src/analysis/modules/checksEffectsInteraction.js index 1e7e81224a..54a1dd051b 100644 --- a/remix-solidity/src/analysis/modules/checksEffectsInteraction.js +++ b/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) ? '
Note: Modifiers are currently not considered by this static analysis.' : '' - comments += (multipleContractsWithSameName) ? '
Note: 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 ${funcName}: 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' }) diff --git a/remix-solidity/src/analysis/modules/constantFunctions.js b/remix-solidity/src/analysis/modules/constantFunctions.js index 82750772ea..5da3562eb6 100644 --- a/remix-solidity/src/analysis/modules/constantFunctions.js +++ b/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) ? '
Note: Modifiers are currently not considered by this static analysis.' : '' - comments += (multipleContractsWithSameName) ? '
Note: 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) { diff --git a/remix-solidity/src/analysis/modules/deleteDynamicArrays.js b/remix-solidity/src/analysis/modules/deleteDynamicArrays.js new file mode 100644 index 0000000000..b1143d179e --- /dev/null +++ b/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 +} diff --git a/remix-solidity/src/analysis/modules/guardConditions.js b/remix-solidity/src/analysis/modules/guardConditions.js index 7a4dbf548d..a9d877c57c 100644 --- a/remix-solidity/src/analysis/modules/guardConditions.js +++ b/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 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.', + 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' }] } diff --git a/remix-solidity/src/analysis/modules/list.js b/remix-solidity/src/analysis/modules/list.js index a4e0ea6b0e..3cbad9d87c 100644 --- a/remix-solidity/src/analysis/modules/list.js +++ b/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') ] diff --git a/remix-solidity/src/analysis/modules/similarVariableNames.js b/remix-solidity/src/analysis/modules/similarVariableNames.js index 1cbfcec9aa..ef4f5451a5 100644 --- a/remix-solidity/src/analysis/modules/similarVariableNames.js +++ b/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) } diff --git a/remix-solidity/src/analysis/modules/staticAnalysisCommon.js b/remix-solidity/src/analysis/modules/staticAnalysisCommon.js index e9fc7fce4c..9a041bdde5 100644 --- a/remix-solidity/src/analysis/modules/staticAnalysisCommon.js +++ b/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, diff --git a/remix-solidity/test/analysis/staticAnalysisCommon-test.js b/remix-solidity/test/analysis/staticAnalysisCommon-test.js index b9e39c81f2..fa740828ce 100644 --- a/remix-solidity/test/analysis/staticAnalysisCommon-test.js +++ b/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') +}) diff --git a/remix-solidity/test/analysis/staticAnalysisIntegration-test.js b/remix-solidity/test/analysis/staticAnalysisIntegration-test.js index d708f6ea9b..6139c1e8a7 100644 --- a/remix-solidity/test/analysis/staticAnalysisIntegration-test.js +++ b/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() diff --git a/remix-solidity/test/analysis/test-contracts/deleteDynamicArray.sol b/remix-solidity/test/analysis/test-contracts/deleteDynamicArray.sol new file mode 100644 index 0000000000..bb179b21f6 --- /dev/null +++ b/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; + } +} \ No newline at end of file