Skip to content
This repository has been archived by the owner on Jul 11, 2019. It is now read-only.

Logic contracts can be "accidentally killed" #320

Closed
spalladino opened this issue Jul 16, 2018 · 14 comments
Closed

Logic contracts can be "accidentally killed" #320

spalladino opened this issue Jul 16, 2018 · 14 comments
Assignees
@spalladino
Labels
gitcoin:assigned Under development via gitcoin kind:discussion To discuss on a task before implementing it topic:security Security reviews and considerations
Milestone
v2.0.0

Comments

@spalladino
Copy link
Contributor

spalladino commented Jul 16, 2018 •
edited
Loading

Logic contracts registered in an app are currently open to the same attack as the 2nd Parity Wallet hack. Given that we one of our goals is to make deployments more secure, I'd like to address this at least before the anniversary of the hack (November 7th).

All jokes aside, we are not initializing logic contracts as they are deployed via zos push. This will also be true as we move onto the new constructor contracts pattern.

These are the options that come to mind to prevent the issue:

1- Push forward zeppelinos/zos-lib#33, which prevents any calls whatsoever to a logic contract. This is the safest solution by far. It requires a new feature altogether in Solidity though, or meddling with the assembly code, which would break verifications, and potentially introduce unexpected bugs. However, even with the new feature, it requires the user to change their contracts to be used with zOS, something we're trying to avoid (unless we modify it automatically from the CLI before deployment).

2- Check the contract code for any SELFDESTRUCT calls. We could either check the binary (which would provide little info the user as where the call is issued, and may also fire some false positives if there is raw data with the same value as the opcode), or the source code of the contract and all its ancestors. The best we can do here is print a warning to the user, indicating that contract could be insecure.

3- Force initialization of logic contracts when registered. This adds a lot of burden to the end user, who needs to provide "fake" params (which will never be used) for every contract to be registered. It also doesn't protect against this problem, since a SELFDESTRUCT in the contract logic could be callable regardless of the initialization parameteres.

I'd go with (2) for the time being. Thoughts?
@spalladino spalladino added topic:security Security reviews and considerations kind:discussion To discuss on a task before implementing it labels Jul 16, 2018
@spalladino spalladino added this to the v2.0.0 milestone Jul 16, 2018
@spalladino spalladino added the gitcoin:bounty worthy Gitcoin bounty worthy label Jul 16, 2018
@frangio frangio mentioned this issue Jul 16, 2018
Closed
@frangio
Copy link
Contributor

frangio commented Jul 16, 2018

Agree on going with (2) for now.

Eventually we could use trustless off-chain computation ✨ (e.g. TrueBit) to perform all of these safety checks on contracts. I'm not sure if that would be in App, Directory, or elsewhere though...

@nventuro
Copy link
Contributor

Instead of looking at the raw bytecode (which will always contain raw binary data because of the included metadata), and parsing the source code (which may be a bit fragile), we could traverse the AST (recursively), where a selfdestruct call will show up as an expression with the following fields:

"typeDescriptions": {
  "typeIdentifier": "t_function_selfdestruct_nonpayable$_t_address_$returns$__$",
  "typeString": "function (address)"
}

Keep in mind though that a contract may still selfdestruct even if the opcode isn't present by doing a DELEGATECALL.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 330.0 DAI (330.0 USD @ $1.0/DAI) attached to it.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 4 weeks, 1 day from now. Please review their questions below:

  1. mikb456 has started work.
  • Q: I will see with what is going on. With what I can do or not do.

Learn more on the Gitcoin Issue Details page.

@mikb456
Copy link

mikb456 commented Jul 25, 2018

I am recognizing some problems within the whole protocol in the git hub tool.

@spalladino
Copy link
Contributor Author

@mikb456 care to ellaborate?

@gitcoinbot
Copy link

gitcoinbot commented Aug 1, 2018 •
edited
Loading

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 7 months, 1 week ago.
Please review their action plans below:

1) hardlydifficult has started work.

I have an implementation and am working on a pull request, soon.

Learn more on the Gitcoin Issue Details page.

@HardlyDifficult HardlyDifficult mentioned this issue Aug 1, 2018
Merged
@gitcoinbot
Copy link

gitcoinbot commented Aug 2, 2018 •
edited
Loading

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 330.0 DAI (330.0 USD @ $1.0/DAI) has been submitted by:

  1. @hardlydifficult
  2. @hardlydifficult
  3. @hardlydifficult
  4. @auroq

@vs77bb please take a look at the submitted work:

@spalladino spalladino self-assigned this Aug 6, 2018
@spalladino spalladino added gitcoin:assigned Under development via gitcoin and removed gitcoin:bounty worthy Gitcoin bounty worthy labels Aug 6, 2018
spalladino pushed a commit that referenced this issue Aug 9, 2018
@spalladino
Warn if contract has a selfdestruct call (#347)
cbed3cc 
* 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
@HardlyDifficult
Copy link

Anything more I could do to help resolve this?

@spalladino
Copy link
Contributor Author

Just remind me to close the issue associated to the PR :-P
Thanks for the help @HardlyDifficult!!

Fixed by @HardlyDifficult via #347
cc @gitcoinbot @vs77bb

@HardlyDifficult
Copy link

@vs77bb could you close out the bounty as well? Let me know if there's anything you need from me.

@HardlyDifficult
Copy link

@spalladino could you please re-open this and assign to @vs77bb for visibility?

@spalladino
Copy link
Contributor Author

Sorry @HardlyDifficult, but github does not allow me to assign the issue to someone who did not participate in the thread. Have you tried reaching out on gitcoin's Slack?

@vs77bb
Copy link

vs77bb commented Aug 24, 2018 •
edited
Loading

@spalladino @HardlyDifficult Apologies this took so long to pay out, great work @HardlyDifficult! (This has been paid out 💸).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@spalladino spalladino

Labels
gitcoin:assigned Under development via gitcoin kind:discussion To discuss on a task before implementing it topic:security Security reviews and considerations
Projects
None yet
Milestone
v2.0.0
Development

No branches or pull requests
7 participants
@spalladino @frangio @nventuro @HardlyDifficult @vs77bb @gitcoinbot @mikb456