Warn if contract has a selfdestruct call (#347)

* Warn if contract has a selfdestruct call

Fix for #320

We are scanning the AST as suggested by @nventuro

* Fixed test fails.

I confirmed tests are passing locally this time ;)

* Minor style change

for consistency.

* @spalladino Correct, the original approach did not work.  I had misunderstood the structure of the AST.  I made the changes you suggested and added a Test.  Please let me know if this looks okay.

I'm unsure how to address ancestor concern.  I wrote a test for this, and commented it out because it fails ATM.  If you have a suggestion on how to proceed, I'm happy to take a crack at it.

* Addressing feedback

I made changes addressing the code review feedback from @spalladino.

For the parent implementations, I'm not sure how to go from node ID to the AST.  I coded an alternative which is similar.  Per the unit test it seems to work.  Thoughts on this?

* Added a delegatecall warning.

Including a unit test for that warning.

RE feedback: Changed the for loop style to for-of as suggested.

I considered adding a warning for callcode as well, but there is already a message "Warning: "callcode" has been deprecated in favour of "delegatecall"."

* Addressing feedback.

Updating per the latest feedback.  Note that I expect the test will still fail.  I'm having trouble locally...

When running the test suite, it simply skips the failing tests without message.

"....
    parseInit
      √ should not init
      √ should init with default when init is set
      √ should init when args is set
      √ should init with specific function
      √ should init with specific function and args

  Contract: add-all script

E:\zos-cli2>"

And when I try to run the test explicitly it also just terminates without messaging why..

"E:\zos-cli2>C:\Users\lopop\AppData\Roaming\npm\truffle test .\test\scripts\add-all.test.js
Using network 'test'.

  Contract: add-all script

E:\zos-cli2>"

I tried one-off local tests (for all 4 test cases) and it seems to work fine.

"E:\zostest>node ..\zos-cli2\lib\bin\zos-cli add SelfDestruct
Compiling contracts
Compiling .\contracts\Child.sol...
Compiling .\contracts\Nested2.sol...
Compiling .\contracts\SelfDestruct.sol...
Writing artifacts to .\build\contracts

Adding SelfDestruct
Contract SelfDestruct (or its parent class) has a selfdestruct call. This is potentially a security risk. Please review and consider removing this call.
Successfully written zos.json"

I can't repro the fail.  I'm unsure how to proceed at the moment.  I'm going to set this down and try again later, let me know if you have any tips.

thanks.

* Fixed test fails.

And addressed the feedback from @spalladino

contracts/mocks/Invalid.sol

@@ -17,3 +17,41 @@ contract WithFailingConstructor {
     assert(false);
   }
 }
+
+contract WithSelfDestruct {
+  uint256 public value;
+
+  constructor() public {
+    if (true)
+      selfdestruct(msg.sender);
+  }
+
+  function say() public pure returns (string) {
+    return "WithSelfDestruct";
+  }
+}
+
+contract WithParentWithSelfDestruct is WithSelfDestruct {
+  function say() public pure returns (string) {
+    return "WithParentWithSelfDestruct";
+  }
+}
+
+contract WithDelegateCall {
+  constructor(address _e) public {
+    require(_e.delegatecall(bytes4(keccak256("kill()"))));
+  }
+
+  function say() public pure returns (string) {
+    return "WithDelegateCall";
+  }
+}
+
+contract WithParentWithDelegateCall is WithDelegateCall {
+  constructor(address _e) public WithDelegateCall(_e) {
+  }
+
+  function say() public pure returns (string) {
+    return "WithParentWithDelegateCall";
+  }
+}

src/models/local/LocalBaseController.js

@@ -45,6 +45,16 @@ export default class LocalBaseController {
     if (this.hasConstructor(Contracts.getLocalPath(contractName))) {
       log.error(`Contract ${contractName} has an explicit constructor. Move it to an initializer function to use it with ZeppelinOS.`)
     }
+    // Log a warning anytime `selfdestruct` is found.  This is a potential security risk, 
+    // but not an error/throw as it may be a desired feature
+    if (this.hasSelfDestruct(Contracts.getLocalPath(contractName))) {
+      log.warn(`Contract ${contractName} (or its parent class) has a selfdestruct call. This is potentially a security risk. Please review and consider removing this call.`)
+    }
+    // Log a warning anytime `delegatecall` is found.  This is a potential security risk, 
+    // but not an error/throw as it may be a desired feature
+    if (this.hasDelegateCall(Contracts.getLocalPath(contractName))) {
+      log.warn(`Contract ${contractName} (or its parent class) has a delegatecall call. This is potentially a security risk, as the logic contract could be destructed by issuing a delegatecall to another contract with a selfdestruct instruction. Please review and consider removing this call.`)
+    }
     this.packageFile.addContract(contractAlias, contractName)
   }
 
@@ -92,6 +102,35 @@ export default class LocalBaseController {
     return !!abi.find(fn => fn.type === "constructor");
   }
 
+  hasSelfDestruct(contractDataPath) {
+    return this.hasTypeIdentifier(contractDataPath, "t_function_selfdestruct_nonpayable$_t_address_$returns$__$")
+  }
+
+  hasDelegateCall(contractDataPath) {
+    return this.hasTypeIdentifier(contractDataPath, "t_function_baredelegatecall_nonpayable$__$returns$_t_bool_$")
+  }
+
+  hasTypeIdentifier(contractDataPath, typeIdentifier) {
+    if (!fs.exists(contractDataPath)) return false
+    const contractJson = fs.parseJson(contractDataPath)
+    for (const node of contractJson.ast.nodes.filter((n) => n.name === contractJson.contractName)) {
+      if (this.hasKeyValue(node, "typeIdentifier", typeIdentifier)) return true
+      for (const baseContract of node.baseContracts || []) {
+        if (this.hasTypeIdentifier(Contracts.getLocalPath(baseContract.baseName.name), typeIdentifier)) return true
+      }
+    }
+    return false
+  }
+
+  hasKeyValue(data, key, value) {
+    if (!data) return false
+    if (data[key] === value) return true
+    for (const childKey in data) {
+      if (typeof(data[childKey]) === 'object' && this.hasKeyValue(data[childKey], key, value)) return true
+    }
+    return false
+  }
+
   getContractClass(contractAlias) {
     const contractName = this.packageFile.contract(contractAlias);
     if (contractName) {

test/mocks/packages/package-with-invalid-contracts.zos.json

@@ -3,6 +3,10 @@
   "version": "1.1.0",
   "contracts": {
     "Impl": "ImplV1",
-    "WithFailingConstructor": "WithFailingConstructor"
+    "WithFailingConstructor": "WithFailingConstructor",
+    "WithSelfDestruct": "WithSelfDestruct",
+    "WithParentWithSelfDestruct": "WithParentWithSelfDestruct",
+    "WithDelegateCall": "WithDelegateCall",
+    "WithParentWithDelegateCall": "WithParentWithDelegateCall"
   }
 }

test/scripts/add.test.js

@@ -92,10 +92,39 @@ contract('add script', function() {
       this.logs.errors[0].should.match(/constructor/i);
     });
 
-    it('should not warn when adding a contract without a constructor', async function() {
+    it('should not warn when adding a contract without a constructor or a selfdestruct call', async function() {
       add({ contractsData, packageFile: this.packageFile });
 
       this.logs.errors.should.have.lengthOf(0);
+      this.logs.warns.should.have.lengthOf(0);
+    });
+
+    it('should warn when adding a contract with a selfdestruct call', async function() {
+      add({ contractsData: [{ name: 'WithSelfDestruct', alias: 'WithSelfDestruct' }], packageFile: this.packageFile });
+
+      this.logs.warns.should.have.lengthOf(1);
+      this.logs.warns[0].should.match(/selfdestruct/i);
+    });
+
+    it('should warn when adding a contract whose parent has a selfdestruct call', async function() {
+      add({ contractsData: [{ name: 'WithParentWithSelfDestruct', alias: 'WithParentWithSelfDestruct' }], packageFile: this.packageFile });
+
+      this.logs.warns.should.have.lengthOf(1);
+      this.logs.warns[0].should.match(/selfdestruct/i);
+    });
+
+    it('should warn when adding a contract with a delegatecall call', async function() {
+      add({ contractsData: [{ name: 'WithDelegateCall', alias: 'WithDelegateCall' }], packageFile: this.packageFile });
+
+      this.logs.warns.should.have.lengthOf(1);
+      this.logs.warns[0].should.match(/delegatecall/i);
+    });
+
+    it('should warn when adding a contract whose parent has a delegatecall call', async function() {
+      add({ contractsData: [{ name: 'WithParentWithDelegateCall', alias: 'WithParentWithDelegateCall' }], packageFile: this.packageFile });
+
+      this.logs.warns.should.have.lengthOf(1);
+      this.logs.warns[0].should.match(/delegatecall/i);
     });
   });
 });