Sitelet https://web.archive.org/web/20201113050251/https://github.com/ethereum/solidity/pull/8950
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SMTChecker] Support to external calls to unknown code #8950

Merged
merged 3 commits into from Jul 2, 2020

Conversation

@leonardoalt
Copy link
Member

@leonardoalt leonardoalt commented May 14, 2020 •

Fixes #8972
Depends on #9159

tl;dr
Support reentrancy from unknown code without losing all knowledge.

This PR extends the CHC model to support external calls to unknown code. Since the code is unknown, it is sound to assume that it can nondeterministically call the analyzed contract, any number of times.
Because of that, the model is extended with 2 new types of rules that encode this nondeterminism, using a new predicate nondet_interface(state, state') for each analyzed contract, that allow the state to change nondeterministically from state to state'.

The first rule is a fact added for each contract:

  1. nondet_interface(state, state)
    This fact is the base case of the inductive rule, and allows the nondeterministic step to be applied without state changes.

The second rule is the inductive rule over each public function f of the contract:
2. nondet_interface(state, state') && summary_f(state', state'') => nondet_interface(state, state'')

When an external call to unknown code is seen, CHC simply adds the constraint
nondet_interface(currentState, postState) to the current block/rule. This means that the current contract can be called nondeterministically any number of times (including zero) before moving on to the next block.
Note that because it is nondeterministic, if a certain path allows for state changes in the contract, it will necessarily be reached and taken into account, so this model is sound.

If the contract has public functions that allow arbitrary state changes, knowledge will be lost at this point, but there's not much else one can do.
However, if the contract has a certain behavior where the current state might limit state changes in reentrant calls, this will be taken into account when generating invariants.
See the mutex.sol example added in this PR.

Copy link
Member

@ekpyron ekpyron left a comment

For the record: this needs a rebase ;-), but this looks quite good to me! I'm actually almost approving it right away, but maybe I should take some more time trying to come up with something that might break it...

One remark though: we could actually check if the function called is view or pure, i.e. if it'll be called with STATICCALL - in those cases this is actually too strong, i.e. we could in fact just keep the state for such calls (but we can also just relax those cases later, but it shouldn't actually be too complicated to do it here).

@leonardoalt
Copy link
Member Author

@leonardoalt leonardoalt commented May 15, 2020

Right, that makes sense! (re staticcall).

Yea, I also have a few test cases in mind that I still want to add.

@elenadimitrova elenadimitrova added this to In progress in Solidity via automation May 18, 2020
@leonardoalt leonardoalt force-pushed the smt_external_calls branch 5 times, most recently from ff789e6 to 2f49300 Jun 2, 2020
@leonardoalt leonardoalt force-pushed the smt_external_calls branch from 2f49300 to ab05c1a Jun 10, 2020
@leonardoalt
Copy link
Member Author

@leonardoalt leonardoalt commented Jun 10, 2020

Rebased on top of #9176 to see if the nondet problem is gone

@leonardoalt leonardoalt force-pushed the smt_external_calls branch 2 times, most recently from a629ec4 to e89fcd5 Jun 10, 2020
@leonardoalt
Copy link
Member Author

@leonardoalt leonardoalt commented Jun 11, 2020

Apparently the nondeterminism in the tests in my local arch build vs CI was that the arch z3 uses gmp whereas the others don't.

@leonardoalt leonardoalt force-pushed the smt_external_calls branch from e89fcd5 to eaf7c01 Jun 11, 2020
@leonardoalt
Copy link
Member Author

@leonardoalt leonardoalt commented Jun 11, 2020

Rebased, ready for review.

@leonardoalt leonardoalt force-pushed the smt_external_calls branch 7 times, most recently from 2027e87 to 35d8f2a Jun 11, 2020
@leonardoalt
Copy link
Member Author

@leonardoalt leonardoalt commented Jun 12, 2020

#9159 helps with the nondeterminism

@leonardoalt leonardoalt force-pushed the smt_external_calls branch 2 times, most recently from b998790 to bdfead5 Jun 12, 2020
}
}
// ----
// Warning: (306-320): Assertion violation happens here

This comment has been minimized.

@ekpyron

ekpyron Jun 12, 2020
Member

Which one is this?

This comment has been minimized.

@leonardoalt

leonardoalt Jun 12, 2020
Author Member

z == y

This comment has been minimized.

@leonardoalt

leonardoalt Jun 12, 2020
Author Member

it fails because State.c is not necessarily the caller

This comment has been minimized.

@leonardoalt

leonardoalt Jun 12, 2020
Author Member

and since the state is not carried over to the called contract, it doesn't know anything about c's state

@leonardoalt leonardoalt force-pushed the smt_external_calls branch from bdfead5 to c80fea8 Jun 12, 2020
@Marenz
Copy link
Contributor

@Marenz Marenz commented Jun 16, 2020

This needs a rebase to resolve the conflicts

@leonardoalt leonardoalt force-pushed the smt_external_calls branch 3 times, most recently from dfec5ef to 9a15de4 Jun 29, 2020
libsolidity/formal/CHC.cpp Outdated Show resolved Hide resolved
@leonardoalt leonardoalt force-pushed the smt_external_calls branch from 9a15de4 to 7627ea7 Jul 1, 2020
Solidity automation moved this from In progress to Review in progress Jul 1, 2020
@ekpyron
ekpyron approved these changes Jul 1, 2020
Copy link
Member

@ekpyron ekpyron left a comment

We decided to change the implemented pure function case.

@leonardoalt leonardoalt force-pushed the smt_external_calls branch from 7627ea7 to 5517e81 Jul 1, 2020
@leonardoalt
Copy link
Member Author

@leonardoalt leonardoalt commented Jul 1, 2020

@ekpyron updated

@ekpyron
ekpyron approved these changes Jul 1, 2020
Copy link
Member

@ekpyron ekpyron left a comment

I wonder what's the best way to have CHC track if we know that the address in a state variable like C c; is of a contract type C for which we know the code (i.e. CHC sees where it is deployed)...

Maybe there's just a boolean flag for C c; that means "contains valid C" and that is initialized to false and set to true on any c = new C()? And then calling c.f() will take the undefined path, if it's false and assume the code of C.f being called, if it's true?

But yeah - maybe we can really just assume that everything named C is actually a C, I'm not entirely sure... it would feel weird to me in any case.

@leonardoalt
Copy link
Member Author

@leonardoalt leonardoalt commented Jul 2, 2020

@ekpyron created #9287 to track that

@leonardoalt leonardoalt merged commit b19c194 into develop Jul 2, 2020
37 checks passed
37 checks passed
ci/circleci: b_archlinux Your tests passed on CircleCI!
Details
ci/circleci: b_docs Your tests passed on CircleCI!
Details
ci/circleci: b_ems Your tests passed on CircleCI!
Details
ci/circleci: b_osx Your tests passed on CircleCI!
Details
ci/circleci: b_ubu Your tests passed on CircleCI!
Details
ci/circleci: b_ubu18 Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_asan Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_asan_clang Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_clang Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_cxx20 Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_ossfuzz Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_release Your tests passed on CircleCI!
Details
ci/circleci: chk_antlr_grammar Your tests passed on CircleCI!
Details
ci/circleci: chk_buglist Your tests passed on CircleCI!
Details
ci/circleci: chk_coding_style Your tests passed on CircleCI!
Details
ci/circleci: chk_docs_pragma_min_version Your tests passed on CircleCI!
Details
ci/circleci: chk_errorcodes Your tests passed on CircleCI!
Details
ci/circleci: chk_proofs Your tests passed on CircleCI!
Details
ci/circleci: chk_pylint Your tests passed on CircleCI!
Details
ci/circleci: chk_spelling Your tests passed on CircleCI!
Details
ci/circleci: t_ems_compile_ext_colony Your tests passed on CircleCI!
Details
ci/circleci: t_ems_compile_ext_gnosis Your tests passed on CircleCI!
Details
ci/circleci: t_ems_compile_ext_zeppelin Your tests passed on CircleCI!
Details
ci/circleci: t_ems_solcjs Your tests passed on CircleCI!
Details
ci/circleci: t_osx_cli Your tests passed on CircleCI!
Details
ci/circleci: t_osx_soltest Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_asan_cli Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_asan_constantinople Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_asan_constantinople_clang Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_clang_soltest Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_cli Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_release_cli Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_release_soltest Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_soltest Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_soltest_enforce_yul Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Solidity automation moved this from Review in progress to Done Jul 2, 2020
@leonardoalt leonardoalt deleted the smt_external_calls branch Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Solidity
  
Done
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.