Warn if contract has a selfdestruct call #347

HardlyDifficult · 2018-08-01T23:04:07Z

Fix for #320

We are scanning the AST as suggested by @nventuro

@nventuro

Fix for #320 We are scanning the AST as suggested by @nventuro

HardlyDifficult · 2018-08-02T01:59:42Z

I'm working on the test fails now.

I confirmed tests are passing locally this time ;)

for consistency.

spalladino

Thanks for the contribution @HardlyDifficult! Could you add some tests to check that the feature works properly?

Also, I don't think the current method for scanning the AST manages to catch all cases, since it only scans the topmost set of statements in a function body. So, if there is a selfdestruct statement within a nested block, it will miss it. For instance:

contract Destructible {
  function hasDestruct() {
    if (true) {
      if (true) {
        if (true) {
          selfdestruct(address(0));
        }
      }
    }
  }
}

I think it'd be more robust to use a visitor pattern to walk the AST, and check for selfdestruct nodes anywhere in the tree.

Also, keep in mind that the selfdestruct call may be present in an ancestor, so you'd need to repeat this process for all contracts in the hierarchy. But I'd suggest to start from just the specific contract, and then work our way up!

…erstood 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.

HardlyDifficult · 2018-08-02T22:34:28Z

@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.

spalladino

Looks much better @HardlyDifficult, thanks!! I think the hasKeyValue approach can be improved using an actual AST visitor, but I couldn't find any existing implementation for the format returned by solc.

As for the parent implementations, note that the AST returns a contractDependencies node, with node IDs that refer to parent contracts you'd need to scan as well.

spalladino · 2018-08-03T13:20:31Z

test/scripts/add.test.js

@@ -92,10 +92,26 @@ 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 cull', async function() {


typo: cull => call

src/models/local/LocalBaseController.js

+
+  hasKeyValue(data, key, value) {
+    if (!data) return false
+    if (data[key] === value) return true


spalladino · 2018-08-03T13:27:50Z

src/models/local/LocalBaseController.js

+    if (typeof(data) === 'object') {
+      for (const child_key in data) {
+        const child_value = data[child_key];
+        if (this.hasKeyValue(child_value, key, value)) {


I think the type check should be moved onto here: only call this.hasKeyValue if child_value is an object.

spalladino · 2018-08-03T13:28:57Z

src/models/local/LocalBaseController.js

+    if (!data) return false
+    if (data[key] === value) return true
+    if (typeof(data) === 'object') {
+      for (const child_key in data) {


Please make sure to use camelCase for all variable names, and not snake_case, for consistency

spalladino · 2018-08-03T13:29:17Z

test/scripts/add.test.js

+    });
+
+    // TODO: This warning is not yet implemented
+    // it('should warn when adding a contract who\'s parent has a selfdestruct call', async function() {


Use it.skip instead of commenting out the code if it's still unimplemented

@spalladino

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?

spalladino

@HardlyDifficult the new approach for querying ancestors looks good! I added a minor comment regarding style, to keep consistency with the rest of the codebase.

Could you look into whether it's possible to detect DELEGATECALLs with the same hasKeyValue method, as @nventuro suggested, so we output another warning for the same reason? (ie the contract may delegatecall into an arbitrary one that does selfdestruct)

spalladino · 2018-08-03T19:30:51Z

src/models/local/LocalBaseController.js

+    const ast = fs.parseJson(contractDataPath).ast
+    if (this.hasKeyValue(ast, "typeIdentifier", "t_function_selfdestruct_nonpayable$_t_address_$returns$__$")) return true
+    for (let i = 0; i < ast.nodes.length; i++) {
+      for (let j = 0; j < (ast.nodes.baseContracts || []).length; j++) {


Could you change these for-loops to use either the for-of construct for forEach method, for better readability?

HardlyDifficult · 2018-08-03T19:57:45Z

RE delegate call: happy too. I looked at this a bit but couldn't figure out how to use delegate call (never tried this in a contract before). Could you provide am example I can test with? I tried the following but it does not work (no error, but it does not actually selfdestruct the contract):

this.delegatecall(bytes4(keccak256("selfdestruct(uint256)")), msg.sender);

thanks,
nick.

spalladino · 2018-08-03T20:03:03Z

A delegatecall is written as a regular low-level call, the example you provided should work, but you'll probably need to be using a target address different than this (see here for an example). The goal would be to see how a delegatecall looks like in the AST, and capture it, as you did with the selfdestruct. Thanks!!

HardlyDifficult · 2018-08-03T23:49:11Z

Delegate call does not seem to create a clear entry in the AST we could scan for. A couple options I can think of:

Look for: 'typeIdentifier' == "t_stringliteral_ecd652fcce345f51df38ced4d0016cc21c8a20495863986a365f73a38368985d"
Or look for "t_contract$Delegate$42" and provide a more generic warning against making low level calls.

Either of these seem okay?

spalladino · 2018-08-06T14:02:07Z

@HardlyDifficult I made a small example, and the AST seems to record the delegate call with the type t_function_baredelegatecall_nonpayable$__$returns$_t_bool_$. If this is reliable, we could use this to check for delegatecalls, as we did with selfdestructs.

@nventuro or @ajsantander, given that you are working with the Solidity compiler internals, could you confirm if this is always the case for a delegatecall (at least when not using assembly)?

I think we are getting real close to wrapping this up!!

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"."

spalladino

Looks good @HardlyDifficult, thanks for the update! There is a test missing (the contract was added, but not the corresponding test case), and the CI is reporting a max call stack error (maybe hasTypeIdentifier is looping over the same baseContract or node over and over again?).

spalladino · 2018-08-06T21:49:20Z

test/scripts/add.test.js

+      this.logs.warns[0].should.match(/selfdestruct/i);
+    });
+
+    it('should warn when adding a contract with a delegatecall call', async function() {


Please add a test for WithParentWithDelegateCall

spalladino · 2018-08-06T21:52:15Z

src/models/local/LocalBaseController.js

+    // 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. Avoid using low-level function 'delegatecall'. This is potentially a security risk. Please review and consider removing this call.`)


Let's change:
Avoid using low-level function 'delegatecall'. This is potentially a security risk.
To the following:
This is potentially a security risk, as the logic contract could be destructed by issuing a delegatecall to another contract with a selfdestruct instruction.

WDYT?

spalladino · 2018-08-06T21:52:34Z

test/scripts/add.test.js

+      this.logs.warns[0].should.match(/selfdestruct/i);
+    });
+
+    it('should warn when adding a contract who\'s parent has a selfdestruct call', async function() {


who' s => whose (sorry, grammar nazi here)

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.

HardlyDifficult · 2018-08-08T03:18:41Z

Quick update: I have the stack overflow fixed locally, but currently investigating another issue.

(the AST is referencing every class in the .sol file, vs just ones related to the contract being tested... leading to false positives)

Will continue tomorrow.

@spalladino

And addressed the feedback from @spalladino

HardlyDifficult · 2018-08-08T22:40:47Z

I believe all the outstanding feedback has been addressed. Let me know if there's anything I missed.

thanks,
nick.

spalladino · 2018-08-09T15:17:25Z

Awesome @HardlyDifficult, thanks for the hard work!! I'll port the commit to the new monorepo at zeppelinos/zos.

Signed-off-by: Santiago Palladino <[email protected]>

Warn if contract has a selfdestruct call

c72a5d9

Fix for #320 We are scanning the AST as suggested by @nventuro

HardlyDifficult added 2 commits August 1, 2018 19:20

Fixed test fails.

5faf4e6

I confirmed tests are passing locally this time ;)

Minor style change

5dcedf9

for consistency.

gitcoinbot mentioned this pull request Aug 2, 2018

Logic contracts can be "accidentally killed" #320

Closed

spalladino suggested changes Aug 2, 2018

View reviewed changes

spalladino reviewed Aug 3, 2018

View reviewed changes

Addressing feedback

3b98da0

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?

spalladino reviewed Aug 3, 2018

View reviewed changes

HardlyDifficult added 2 commits August 6, 2018 12:16

Merge branch 'master' of https://github.com/zeppelinos/zos-cli

ea47e8e

spalladino reviewed Aug 6, 2018

View reviewed changes

Fixed test fails.

6354853

And addressed the feedback from @spalladino

spalladino approved these changes Aug 9, 2018

View reviewed changes

spalladino merged commit cbed3cc into zeppelinos:master Aug 9, 2018

spalladino pushed a commit to OpenZeppelin/openzeppelin-sdk that referenced this pull request Aug 14, 2018

Warn if contract has a selfdestruct call (zeppelinos/zos-cli#347)

014c6f7

Signed-off-by: Santiago Palladino <[email protected]>

spalladino mentioned this pull request Aug 17, 2018

Add link to documentation on selfdestruct warning OpenZeppelin/openzeppelin-sdk#26

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn if contract has a selfdestruct call #347

Warn if contract has a selfdestruct call #347

HardlyDifficult commented Aug 1, 2018

HardlyDifficult commented Aug 2, 2018

spalladino left a comment

HardlyDifficult commented Aug 2, 2018

spalladino left a comment

spalladino Aug 3, 2018

This comment was marked as resolved.

spalladino Aug 3, 2018

spalladino Aug 3, 2018

spalladino Aug 3, 2018

spalladino left a comment

spalladino Aug 3, 2018

HardlyDifficult commented Aug 3, 2018

spalladino commented Aug 3, 2018

HardlyDifficult commented Aug 3, 2018

spalladino commented Aug 6, 2018 •

edited

Loading

spalladino left a comment

spalladino Aug 6, 2018

spalladino Aug 6, 2018

spalladino Aug 6, 2018

HardlyDifficult commented Aug 8, 2018

HardlyDifficult commented Aug 8, 2018

spalladino commented Aug 9, 2018

Warn if contract has a selfdestruct call #347

Warn if contract has a selfdestruct call #347

Conversation

HardlyDifficult commented Aug 1, 2018

HardlyDifficult commented Aug 2, 2018

spalladino left a comment

Choose a reason for hiding this comment

HardlyDifficult commented Aug 2, 2018

spalladino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HardlyDifficult commented Aug 3, 2018

spalladino commented Aug 3, 2018

HardlyDifficult commented Aug 3, 2018

spalladino commented Aug 6, 2018 • edited Loading

spalladino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HardlyDifficult commented Aug 8, 2018

HardlyDifficult commented Aug 8, 2018

spalladino commented Aug 9, 2018

spalladino commented Aug 6, 2018 •

edited

Loading