From b736eb3e55e190bc97fb04d052152dce9c424499 Mon Sep 17 00:00:00 2001 From: soad003 Date: Tue, 21 Nov 2017 14:58:19 +0100 Subject: [PATCH] Static Analysis: Warn on selfdestruct, functions with selfdestruct cannot be const --- .../modules/constantFunctions.js | 18 ++++- src/app/staticanalysis/modules/list.js | 3 +- .../staticanalysis/modules/selfdestruct.js | 33 +++++++++ .../modules/staticAnalysisCommon.js | 17 +++++ .../staticAnalysisIntegration-test.js | 70 +++++++++++++++---- .../test-contracts/selfdestruct.sol | 13 ++++ 6 files changed, 137 insertions(+), 17 deletions(-) create mode 100644 src/app/staticanalysis/modules/selfdestruct.js create mode 100644 test/staticanalysis/test-contracts/selfdestruct.sol diff --git a/src/app/staticanalysis/modules/constantFunctions.js b/src/app/staticanalysis/modules/constantFunctions.js index 23e7714ba9..24c237c433 100644 --- a/src/app/staticanalysis/modules/constantFunctions.js +++ b/src/app/staticanalysis/modules/constantFunctions.js @@ -10,7 +10,14 @@ function constantFunctions () { this.abstractAst = new AbstractAst() 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) + (node) => common.isLowLevelCall(node) || + common.isTransfer(node) || + common.isExternalDirectCall(node) || + common.isEffect(node) || + common.isLocalCallGraphRelevantNode(node) || + common.isInlineAssembly(node) || + common.isNewExpression(node) || + common.isSelfdestructCall(node) ) this.report = this.abstractAst.build_report(report) @@ -28,8 +35,12 @@ function report (contracts, multipleContractsWithSameName) { contracts.forEach((contract) => { contract.functions.forEach((func) => { - func.potentiallyshouldBeConst = checkIfShouldBeConstant(common.getFullQuallyfiedFuncDefinitionIdent(contract.node, func.node, func.parameters), + if (common.isPayableFunction(func.node)) { + func.potentiallyshouldBeConst = false + } else { + func.potentiallyshouldBeConst = checkIfShouldBeConstant(common.getFullQuallyfiedFuncDefinitionIdent(contract.node, func.node, func.parameters), getContext(callGraph, contract, func)) + } }) contract.functions.filter((func) => common.hasFunctionBody(func.node)).forEach((func) => { @@ -76,7 +87,8 @@ function isConstBreaker (node, context) { isCallOnNonConstExternalInterfaceFunction(node, context) || common.isCallToNonConstLocalFunction(node) || common.isInlineAssembly(node) || - common.isNewExpression(node) + common.isNewExpression(node) || + common.isSelfdestructCall(node) } function isCallOnNonConstExternalInterfaceFunction (node, context) { diff --git a/src/app/staticanalysis/modules/list.js b/src/app/staticanalysis/modules/list.js index 417d9b363c..e510f323f6 100644 --- a/src/app/staticanalysis/modules/list.js +++ b/src/app/staticanalysis/modules/list.js @@ -9,5 +9,6 @@ module.exports = [ require('./blockTimestamp'), require('./lowLevelCalls'), require('./blockBlockhash'), - require('./noReturn') + require('./noReturn'), + require('./selfdestruct') ] diff --git a/src/app/staticanalysis/modules/selfdestruct.js b/src/app/staticanalysis/modules/selfdestruct.js new file mode 100644 index 0000000000..e8336629a5 --- /dev/null +++ b/src/app/staticanalysis/modules/selfdestruct.js @@ -0,0 +1,33 @@ +var name = 'Selfdestruct: ' +var desc = 'Be aware of caller contracts.' +var categories = require('./categories') +var common = require('./staticAnalysisCommon') +var yo = require('yo-yo') + +function selfdestruct () { + this.relevantNodes = [] +} + +selfdestruct.prototype.visit = function (node) { + if (common.isSelfdestructCall(node)) { + this.relevantNodes.push(node) + } +} + +selfdestruct.prototype.report = function () { + return this.relevantNodes.map(function (item, i) { + return { + warning: yo`Use of selfdestruct: can block calling contracts unexpectedly
+ Please, be especially carefull if this contract is referenced by other contracts (i.e. library contracts, interactions). Selfdestruction of called contracts can render callers inoperable.`, + location: item.src, + more: 'https://www.coindesk.com/ethereum-client-bug-freezes-user-funds-fallout-remains-uncertain/' + } + }) +} + +module.exports = { + name: name, + description: desc, + category: categories.SECURITY, + Module: selfdestruct +} diff --git a/src/app/staticanalysis/modules/staticAnalysisCommon.js b/src/app/staticanalysis/modules/staticAnalysisCommon.js index 41238f64bb..e54c86b776 100644 --- a/src/app/staticanalysis/modules/staticAnalysisCommon.js +++ b/src/app/staticanalysis/modules/staticAnalysisCommon.js @@ -406,6 +406,10 @@ function isBuiltinFunctionCall (node) { return isLocalCall(node) && builtinFunctions[getLocalCallName(node) + '(' + getFunctionCallTypeParameterType(node) + ')'] === true } +function isSelfdestructCall (node) { + return isBuiltinFunctionCall(node) && getLocalCallName(node) === 'selfdestruct' +} + /** * True if is storage variable declaration * @node {ASTNode} some AstNode @@ -466,6 +470,17 @@ function isConstantFunction (node) { ) } +/** + * True if is function defintion has payable modifier + * @node {ASTNode} some AstNode + * @return {bool} + */ +function isPayableFunction (node) { + return isFunctionDefinition(node) && ( + node.attributes.stateMutability === 'payable' + ) +} + /** * True if unary increment operation * @node {ASTNode} some AstNode @@ -788,6 +803,7 @@ module.exports = { isPlusPlusUnaryOperation: isPlusPlusUnaryOperation, isMinusMinusUnaryOperation: isMinusMinusUnaryOperation, isBuiltinFunctionCall: isBuiltinFunctionCall, + isSelfdestructCall: isSelfdestructCall, // #################### Trivial Node Identification isFunctionDefinition: isFunctionDefinition, @@ -799,6 +815,7 @@ module.exports = { isAssignment: isAssignment, isContractDefinition: isContractDefinition, isConstantFunction: isConstantFunction, + isPayableFunction: isPayableFunction, isInlineAssembly: isInlineAssembly, isNewExpression: isNewExpression, isReturn: isReturn, diff --git a/test/staticanalysis/staticAnalysisIntegration-test.js b/test/staticanalysis/staticAnalysisIntegration-test.js index c1021b6b9d..b4df7b82e7 100644 --- a/test/staticanalysis/staticAnalysisIntegration-test.js +++ b/test/staticanalysis/staticAnalysisIntegration-test.js @@ -28,7 +28,8 @@ var testFiles = [ 'library.sol', 'transfer.sol', 'ctor.sol', - 'forgottenReturn.sol' + 'forgottenReturn.sol', + 'selfdestruct.sol' ] var testFileAsts = {} @@ -60,7 +61,8 @@ test('Integration test thisLocal.js', function (t) { 'library.sol': 0, 'transfer.sol': 0, 'ctor.sol': 0, - 'forgottenReturn.sol': 0 + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -90,7 +92,8 @@ test('Integration test checksEffectsInteraction.js', function (t) { 'library.sol': 1, 'transfer.sol': 1, 'ctor.sol': 0, - 'forgottenReturn.sol': 0 + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -120,7 +123,8 @@ test('Integration test constantFunctions.js', function (t) { 'library.sol': 1, 'transfer.sol': 0, 'ctor.sol': 0, - 'forgottenReturn.sol': 0 + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 1 } runModuleOnFiles(module, t, (file, report) => { @@ -150,7 +154,8 @@ test('Integration test inlineAssembly.js', function (t) { 'library.sol': 0, 'transfer.sol': 0, 'ctor.sol': 0, - 'forgottenReturn.sol': 0 + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -180,7 +185,8 @@ test('Integration test txOrigin.js', function (t) { 'library.sol': 0, 'transfer.sol': 0, 'ctor.sol': 0, - 'forgottenReturn.sol': 0 + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -210,7 +216,8 @@ test('Integration test gasCosts.js', function (t) { 'library.sol': 1, 'transfer.sol': 1, 'ctor.sol': 0, - 'forgottenReturn.sol': 3 + 'forgottenReturn.sol': 3, + 'selfdestruct.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -240,7 +247,8 @@ test('Integration test similarVariableNames.js', function (t) { 'library.sol': 0, 'transfer.sol': 0, 'ctor.sol': 1, - 'forgottenReturn.sol': 0 + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -270,7 +278,8 @@ test('Integration test inlineAssembly.js', function (t) { 'library.sol': 0, 'transfer.sol': 0, 'ctor.sol': 0, - 'forgottenReturn.sol': 0 + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -300,7 +309,8 @@ test('Integration test blockTimestamp.js', function (t) { 'library.sol': 0, 'transfer.sol': 0, 'ctor.sol': 0, - 'forgottenReturn.sol': 0 + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -330,7 +340,8 @@ test('Integration test lowLevelCalls.js', function (t) { 'library.sol': 1, 'transfer.sol': 0, 'ctor.sol': 0, - 'forgottenReturn.sol': 0 + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -360,7 +371,8 @@ test('Integration test blockBlockhash.js', function (t) { 'library.sol': 0, 'transfer.sol': 0, 'ctor.sol': 0, - 'forgottenReturn.sol': 0 + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -390,7 +402,8 @@ test('Integration test noReturn.js', function (t) { 'library.sol': 0, 'transfer.sol': 0, 'ctor.sol': 0, - 'forgottenReturn.sol': 1 + 'forgottenReturn.sol': 1, + 'selfdestruct.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -398,6 +411,37 @@ test('Integration test noReturn.js', function (t) { }) }) +test('Integration test selfdestruct.js', function (t) { + t.plan(testFiles.length) + + var module = require('../../src/app/staticanalysis/modules/selfdestruct') + + 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': 1, + 'library.sol': 0, + 'transfer.sol': 0, + 'ctor.sol': 0, + 'forgottenReturn.sol': 0, + 'selfdestruct.sol': 2 + } + + runModuleOnFiles(module, t, (file, report) => { + t.equal(report.length, lengthCheck[file], `${file} has right amount of selfdestruct warnings`) + }) +}) + // #################### Helpers function runModuleOnFiles (module, t, cb) { var statRunner = new StatRunner() diff --git a/test/staticanalysis/test-contracts/selfdestruct.sol b/test/staticanalysis/test-contracts/selfdestruct.sol new file mode 100644 index 0000000000..4be7a53f2c --- /dev/null +++ b/test/staticanalysis/test-contracts/selfdestruct.sol @@ -0,0 +1,13 @@ +contract sd { + uint120 x; + function() public payable { } + + function c () public constant { + //x++; + selfdestruct(address(0xdeadbeef)); + } + + function b () public payable { + selfdestruct(address(0xdeadbeef)); + } +} \ No newline at end of file