Merge pull request #901 from soad003/staticanalysis_warn_on_selfdestruct

Staticanalysis warn on selfdestruct

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

@@ -0,0 +1,32 @@
+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. Be especially careful if this contract is planed to be used by other contracts (i.e. library contracts, interactions). Selfdestruction of the callee contract can leave callers in an inoperable state.</span>`,
+      location: item.src,
+      more: 'https://paritytech.io/blog/security-alert.html'
+    }
+  })
+}
+
+module.exports = {
+  name: name,
+  description: desc,
+  category: categories.SECURITY,
+  Module: selfdestruct
+}

src/app/staticanalysis/modules/staticAnalysisCommon.js

@@ -406,6 +406,15 @@ function isBuiltinFunctionCall (node) {
   return isLocalCall(node) && builtinFunctions[getLocalCallName(node) + '(' + getFunctionCallTypeParameterType(node) + ')'] === true
 }
 
+/**
+ * True if node is a call to selfdestruct
+ * @node {ASTNode} some AstNode
+ * @return {bool}
+ */
+function isSelfdestructCall (node) {
+  return isBuiltinFunctionCall(node) && getLocalCallName(node) === 'selfdestruct'
+}
+
 /**
  * True if is storage variable declaration
  * @node {ASTNode} some AstNode
@@ -466,6 +475,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 +808,7 @@ module.exports = {
   isPlusPlusUnaryOperation: isPlusPlusUnaryOperation,
   isMinusMinusUnaryOperation: isMinusMinusUnaryOperation,
   isBuiltinFunctionCall: isBuiltinFunctionCall,
+  isSelfdestructCall: isSelfdestructCall,
 
   // #################### Trivial Node Identification
   isFunctionDefinition: isFunctionDefinition,
@@ -799,6 +820,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

@@ -0,0 +1,12 @@
+contract sd {
+
+  function() public payable {  }
+  
+  function c () public constant {
+      selfdestruct(address(0xdeadbeef));
+  }
+  
+  function b () public payable {
+      selfdestruct(address(0xdeadbeef));
+  }
+}