Static Analysis: Warn on selfdestruct, functions with selfdestruct cannot be const

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) {

src/app/staticanalysis/modules/list.js

@@ -9,5 +9,6 @@ module.exports = [
   require('./blockTimestamp'),
   require('./lowLevelCalls'),
   require('./blockBlockhash'),
-  require('./noReturn')
+  require('./noReturn'),
+  require('./selfdestruct')
 ]

src/app/staticanalysis/modules/selfdestruct.js

+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`<span>Use of selfdestruct: can block calling contracts unexpectedly<br />
+                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.</span>`,
+      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
+}

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
@@ -467,6 +471,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
  * @return {bool}
@@ -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,

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()

test/staticanalysis/test-contracts/selfdestruct.sol

+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