eth-sri / securify

[DEPRECATED] Security Scanner for Ethereum Smart Contracts
Apache License 2.0
215 stars 50 forks source link

[WIP] mark the end of block also with STOP #52

Closed hiqua closed 5 years ago

hiqua commented 5 years ago

I think this change would make sense given the surrounding code and comments, but not completely sure. This fixes the bug with the empty contract:

pragma solidity ^0.4.24;

contract C{
}

Blocked by #57.

ghost commented 5 years ago

DeepCode analyzed this pull request. There are no new issues.

ritzdorf commented 5 years ago
  1. Definitely makes sense. Good catch.
  2. Don't you think we should also add Return, Revert and Selfdestruct here? Probably we should define this in a more central place like doesOpcodeEndBlock() and doesOpcodeEndExecution() or so, so that these checks are consistent everywhere.
  3. I guess we should also add it here: https://github.com/eth-sri/securify/blob/f5e7e4ada1dd53a2fdfb80f6b773ce64c49dea0e/src/main/java/ch/securify/decompiler/Decompiler.java#L183 This sparks a more general question, whether we want to keep both of them. (Background: Decompiler is more precise if it works, but function detection rarely does. I don't know why it doesn't work though.) I realized that a couple of recent PRs were only against the fallback variant. (mea culpa) @ptsankov was in favour of keeping the Decompiler in, maybe he can elaborate on its advantages.
hiqua commented 5 years ago

I agree with everything you mention, and I have the same questions regarding Decompiler vs DecompilerFallback.

I think I'll first focus on having more tests before we do these changes, so that we can keep track of whether we introduce bugs or not.

hiqua commented 5 years ago

For STOP it is clear that it fixes some things using the examples I have added on another branch (basically Securify does not fail on SafeMath anymore), I'm looking for examples for the other opcodes now so that we can compare.

ritzdorf commented 5 years ago

Isn't this an example for REVERT?

pragma solidity ^0.4.24;
contract C{
  function() public payable{
    revert();
  }
}
hiqua commented 5 years ago

Yes, and it works also for the other opcodes, so perfect.

hiqua commented 5 years ago

I've added the other opcodes and tests, if the changes are good for you I'll reorganize the commits and merge.

ritzdorf commented 5 years ago

Can you briefly explain the changes that are going on in the 238 changed files? How does the end-to-end testing work? From where is it triggered?

hiqua commented 5 years ago

Can you briefly explain the changes that are going on in the 238 changed files? How does the end-to-end testing work? From where is it triggered?

Sorry that should be clearer now, basically I want to wait until the end_to_end_testing branch is merged before we merge this. It just compares the current output with saved output so that we manually acknowledge the changes. See #57 for more details.

hiqua commented 5 years ago

See #68.