diff --git a/src/app/staticanalysis/modules/abstractAstView.js b/src/app/staticanalysis/modules/abstractAstView.js index 20de50aa5a..0a5fc80831 100644 --- a/src/app/staticanalysis/modules/abstractAstView.js +++ b/src/app/staticanalysis/modules/abstractAstView.js @@ -2,11 +2,20 @@ var common = require('./staticAnalysisCommon') var AstWalker = require('ethereum-remix').util.AstWalker function abstractAstView () { - this.contracts = null + this.contracts = [] this.currentContractIndex = null this.currentFunctionIndex = null this.currentModifierIndex = null this.isFunctionNotModifier = false + /* + file1: contract c{} + file2: import "file1" as x; contract c{} + therefore we have two contracts with the same name c. At the moment this is not handled because alias name "x" is not + available in the current AST implementation thus can not be resolved. + Additionally the fullQuallified function names e.g. [contractName].[functionName](param1Type, param2Type, ... ) must be prefixed to + fully support this and when inheritance is resolved it must include alias resolving e.g x.c = file1.c + */ + this.multipleContractsWithSameName = false } /** @@ -33,8 +42,7 @@ function abstractAstView () { * @contractsOut {list} return list for high level AST view * @return {ASTNode -> void} returns a function that can be used as visit function for static analysis modules, to build up a higher level AST view for further analysis. */ -abstractAstView.prototype.builder = function (relevantNodeFilter, contractsOut) { - this.contracts = contractsOut +abstractAstView.prototype.build_visit = function (relevantNodeFilter) { var that = this return function (node) { if (common.isContractDefinition(node)) { @@ -49,9 +57,6 @@ abstractAstView.prototype.builder = function (relevantNodeFilter, contractsOut) var currentContract = getCurrentContract(that) var inheritsFromName = common.getInheritsFromName(node) currentContract.inheritsFrom.push(inheritsFromName) - // add variables from inherited contracts - var inheritsFrom = that.contracts.find((contract) => common.getContractName(contract.node) === inheritsFromName) - currentContract.stateVariables = currentContract.stateVariables.concat(inheritsFrom.stateVariables) } else if (common.isFunctionDefinition(node)) { setCurrentFunction(that, { node: node, @@ -76,7 +81,36 @@ abstractAstView.prototype.builder = function (relevantNodeFilter, contractsOut) } } +abstractAstView.prototype.build_report = function (wrap) { + var that = this + return function (compilationResult) { + resolveStateVariablesInHierarchy(that.contracts) + return wrap(that.contracts, that.multipleContractsWithSameName) + } +} + +function resolveStateVariablesInHierarchy (contracts) { + contracts.map((c) => { + resolveStateVariablesInHierarchyForContract(c, contracts) + }) +} +function resolveStateVariablesInHierarchyForContract (currentContract, contracts) { + currentContract.inheritsFrom.map((inheritsFromName) => { + // add variables from inherited contracts + var inheritsFrom = contracts.find((contract) => common.getContractName(contract.node) === inheritsFromName) + if (inheritsFrom) { + currentContract.stateVariables = currentContract.stateVariables.concat(inheritsFrom.stateVariables) + } else { + console.log('abstractAstView.js: could not find contract defintion inherited from ' + inheritsFromName) + } + }) +} function setCurrentContract (that, contract) { + var name = common.getContractName(contract.node) + if (that.contracts.map((c) => common.getContractName(c.node)).filter((n) => n === name).length > 0) { + console.log('abstractAstView.js: two or more contracts with the same name dectected, import aliases not supported at the moment') + that.multipleContractsWithSameName = true + } that.currentContractIndex = (that.contracts.push(contract) - 1) } diff --git a/src/app/staticanalysis/modules/checksEffectsInteraction.js b/src/app/staticanalysis/modules/checksEffectsInteraction.js index 45c6803f71..b759d242eb 100644 --- a/src/app/staticanalysis/modules/checksEffectsInteraction.js +++ b/src/app/staticanalysis/modules/checksEffectsInteraction.js @@ -6,22 +6,25 @@ var fcallGraph = require('./functionCallGraph') var AbstractAst = require('./abstractAstView') function checksEffectsInteraction () { - this.contracts = [] - var that = this - - checksEffectsInteraction.prototype.visit = new AbstractAst().builder( - (node) => common.isInteraction(node) || common.isEffect(node) || common.isLocalCallGraphRelevantNode(node), - that.contracts + this.abstractAst = new AbstractAst() + this.visit = this.abstractAst.build_visit( + (node) => common.isInteraction(node) || common.isEffect(node) || common.isLocalCallGraphRelevantNode(node) ) + + this.report = this.abstractAst.build_report(report) } -checksEffectsInteraction.prototype.report = function (compilationResults) { +checksEffectsInteraction.prototype.visit = function () { throw new Error('checksEffectsInteraction.js no visit function set upon construction') } + +checksEffectsInteraction.prototype.report = function () { throw new Error('checksEffectsInteraction.js no report function set upon construction') } + +function report (contracts, multipleContractsWithSameName) { var warnings = [] - var hasModifiers = this.contracts.some((item) => item.modifiers.length > 0) + var hasModifiers = contracts.some((item) => item.modifiers.length > 0) - var callGraph = fcallGraph.buildGlobalFuncCallGraph(this.contracts) + var callGraph = fcallGraph.buildGlobalFuncCallGraph(contracts) - this.contracts.forEach((contract) => { + contracts.forEach((contract) => { contract.functions.forEach((func) => { func.changesState = checkIfChangesState(common.getFullQuallyfiedFuncDefinitionIdent(contract.node, func.node, func.parameters), getContext(callGraph, contract, func)) @@ -30,9 +33,10 @@ checksEffectsInteraction.prototype.report = function (compilationResults) { 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 the 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/src/app/staticanalysis/modules/constantFunctions.js b/src/app/staticanalysis/modules/constantFunctions.js index ac76a90cd9..59988606e0 100644 --- a/src/app/staticanalysis/modules/constantFunctions.js +++ b/src/app/staticanalysis/modules/constantFunctions.js @@ -6,22 +6,26 @@ var fcallGraph = require('./functionCallGraph') var AbstractAst = require('./abstractAstView') function constantFunctions () { - this.contracts = [] - var that = this + this.abstractAst = new AbstractAst() - constantFunctions.prototype.visit = new AbstractAst().builder( - (node) => common.isLowLevelCall(node) || common.isTransfer(node) || common.isExternalDirectCall(node) || common.isEffect(node) || common.isLocalCallGraphRelevantNode(node) || common.isInlineAssembly(node) || common.isNewExpression(node), - that.contracts + this.visit = this.abstractAst.build_visit( + (node) => common.isLowLevelCall(node) || common.isTransfer(node) || common.isExternalDirectCall(node) || common.isEffect(node) || common.isLocalCallGraphRelevantNode(node) || common.isInlineAssembly(node) || common.isNewExpression(node) ) + + this.report = this.abstractAst.build_report(report) } -constantFunctions.prototype.report = function (compilationResults) { +constantFunctions.prototype.visit = function () { throw new Error('constantFunctions.js no visit function set upon construction') } + +constantFunctions.prototype.report = function () { throw new Error('constantFunctions.js no report function set upon construction') } + +function report (contracts, multipleContractsWithSameName) { var warnings = [] - var hasModifiers = this.contracts.some((item) => item.modifiers.length > 0) + var hasModifiers = contracts.some((item) => item.modifiers.length > 0) - var callGraph = fcallGraph.buildGlobalFuncCallGraph(this.contracts) + var callGraph = fcallGraph.buildGlobalFuncCallGraph(contracts) - this.contracts.forEach((contract) => { + contracts.forEach((contract) => { contract.functions.forEach((func) => { func.potentiallyshouldBeConst = checkIfShouldBeConstant(common.getFullQuallyfiedFuncDefinitionIdent(contract.node, func.node, func.parameters), getContext(callGraph, contract, func)) @@ -30,16 +34,17 @@ constantFunctions.prototype.report = function (compilationResults) { 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 the 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}`, + warning: `${funcName}: Potentially should be constant but is not. ${comments}`, location: func.src, more: 'http://solidity.readthedocs.io/en/develop/contracts.html#constant-functions' }) } else { warnings.push({ - warning: `${funcName}: Is constant but potentially should not be.${comments}`, + warning: `${funcName}: Is constant but potentially should not be. ${comments}`, location: func.src, more: 'http://solidity.readthedocs.io/en/develop/contracts.html#constant-functions' }) diff --git a/src/app/staticanalysis/modules/list.js b/src/app/staticanalysis/modules/list.js index 320189b69b..cd17ab43cb 100644 --- a/src/app/staticanalysis/modules/list.js +++ b/src/app/staticanalysis/modules/list.js @@ -4,6 +4,7 @@ module.exports = [ require('./thisLocal'), require('./checksEffectsInteraction'), require('./constantFunctions') + // require('./similarVariableNames.js'), // require('./inlineAssembly'), // require('./blockTimestamp'), // require('./lowLevelCalls'), diff --git a/src/app/staticanalysis/modules/staticAnalysisCommon.js b/src/app/staticanalysis/modules/staticAnalysisCommon.js index 36c3014599..2876644005 100644 --- a/src/app/staticanalysis/modules/staticAnalysisCommon.js +++ b/src/app/staticanalysis/modules/staticAnalysisCommon.js @@ -55,7 +55,8 @@ var builtinFunctions = { 'mulmod(uint256,uint256,uint256)': true, 'selfdestruct(address)': true, 'revert()': true, - 'assert()': true + 'assert(bool)': true, + 'require(bool)': true } var lowLevelCallTypes = {