search

crytic / medusa

Parallelized, coverage-guided, mutational Solidity smart contract fuzzing, powered by go-ethereum
https://www.trailofbits.com/
GNU Affero General Public License v3.0
273 stars 33 forks source link

feat: call constant methods occasionally and allow assertion testing them #363

Closed 0xalpharush closed 13 hours ago

0xalpharush commented 1 month ago

closes https://github.com/crytic/medusa/issues/168

0xalpharush commented 1 week ago

This will also close https://github.com/crytic/medusa/issues/55 but I'm not sure about it's last requirement

Ensure the corpus.UpdateCorpusAndCoverageMaps method checks if the last call was to a view method. If it was, do not record the call sequence in the corpus at that step, as it's not a coverage-increasing sequence we'd be interested in recording, as it was not state changing.

This can be left for later work, if desired, but I don't followed why we'd special case the last call. cc @Xenomega

0xalpharush commented 16 hours ago

Additionally, I'm thinking if we move the TestViewMethods config variable outside of the AssertionTesting structure and put it in Testing. I'm thinking that testing view methods as a concept is more general than assertion testing or property testing. So maybe, we just push it up one level in the hierarchy. what do you think?

I understand this config to mean that assertion failures in constant methods will be treated as failure if assertion mode is enabled, but I see your point that this will affect the sequences generated for property mode as well. I suppose we could have CallViewMethods up one level and it enables assertion testing constant methods (if true) but also allows completely filtering out all view/pure methods (if false) in either mode.

We could capture this in an issue and decide this after merging and implementing filtering ( I want to replace https://github.com/crytic/medusa/pull/275 to be compatible with these changes and fix several bugs in its implementation)

anishnaik commented 16 hours ago

Additionally, I'm thinking if we move the TestViewMethods config variable outside of the AssertionTesting structure and put it in Testing. I'm thinking that testing view methods as a concept is more general than assertion testing or property testing. So maybe, we just push it up one level in the hierarchy. what do you think?

I understand this config to mean that assertion failures in constant methods will be treated as failure if assertion mode is enabled, but I see your point that this will affect the sequences generated for property mode as well. I suppose we could have CallViewMethods up one level and it enables assertion testing constant methods (if true) but also allows completely filtering out all view/pure methods (if false) in either mode.

We could capture this in an issue and decide this after merging and implementing filtering ( I want to replace #275 to be compatible with these changes and fix several bugs in its implementation)

Yeah I think this is fine for now and we can figure this out later