Add CEI pattern static analysis

[anton-trunov]

CEI stands for "Checks, Effects, Interactions". We check no storage reads/writes (effects) occur after calling external contracts (interaction). See this blog post and this blogpost for more detail.

TODOs:

• analyze libraries, scripts, and predicates (only contracts are analyzed at the moment)
• process assembly blocks
• what is the effects of the __revert intrinsic
• Treat nested code blocks
• more tests
  • Tests using storage.var = ...
  • Tests using asm blocks directly
  • Tests using complex control flow
  • Tests having multiple contract calls or multiple storage writes in various scenarios
  • Tests with nested code blocks (there is a very simple test, this needs more testing in a separate PR)
• check for storage reads after interaction
  • Storage read intrinsics
  • StorageAccess expressions
  • asm blocks using srw or srwq
  • asm blocks using bal because bal actually reads from storage
• CLI option or attribute to control the warnings (an internal discussion showed that we cannot do this at the moment, I guess this should be handled as a separate issue)
• open an issue about unification of the currently existing storage annotation checks and effects inference in this PR

close #10

[anton-trunov]

An example of a warning issued by the CEI analyzer:

warning
  --> /Users/anton/fuel.network/sway/reentrancy/src/main.sw:29:5
   |
27 |
28 |       // interaction
29 |       other_contract.deposit(gas, amount, color, unused);
   |  _____-
30 | |     // effect -- therefore violation of CEI where effect should go before interaction
31 | |     let storage_key = 0x3dba0a4455b598b7655a7fb430883d96c9527ef275b49739e7b0ad12f8280eae;
32 | |     store(storage_key, ());
   | |__________________________- Storage modification after external contract interaction. Consider making all storage writes before calling another contract
33 |     }
34 |   }
   |
____

[mohammadfawaz]

Some initial thoughts! But this looks excellent! Thank you so much 🚀

[mohammadfawaz]

I think a revert (or a return) between a contract call and a storage write should not result in a reentrancy warning. That being said, we should probably be warning on dead code in that case anyways?

[mohammadfawaz]

I'm very happy with the overall change, thanks!

My only nitpick is that the auction test is too large and complex to belong in the E2E test repo. Basically, the test is hard to understand and it is unclear where the violation is. I suggest we go with a much smaller example that showcases the same violation.

[anton-trunov]

Just realized I need to treat nested code blocks. The fix is on the way

[anton-trunov]

@mohammadfawaz Pushed an updated, deeper, version of the analysis, please have a look. It still needs a bit more testing, but it already reproduces all the existing tests and I added a very simple test with a nested code block which was not processed previously.

[mohammadfawaz]

Generally this looks very reasonable. I'd certainly like to see more testing. At the very least, it would be nice to have full code coverage for cei_pattern_analysis.rs, but that can be done in a separate PR.

[mohammadfawaz]

Jump are not and will porbably never be allowed in asm blocks, so you're good!

Suggested change

	// TODO: jumps are not processed at the moment, so the CEI analysis will work only
	// for simple linear assembly blocks, which arguably is the most important use case so far
	No need to worry about jumps because they are not allowed in `asm` blocks.

[anton-trunov]

This is really good to know! I applied the change in a separate commit (added //)

[otrho]

Would it be possible to add a test which uses effects (ref mut args?) to confirm the left-to-right evaluation of function args and struct initialisers?

[sezna]

Looks great! The only thing I wonder is if someday down the line we can merge this and other AST-traversing checks we do into one traversal. But the traversals themselves are slightly different so it may not be generalizable (thinking of purity checking mainly).

[sezna]

Would it be possible to add a test which uses effects (ref mut args?) to confirm the left-to-right evaluation of function args and struct initialisers?

I'd be okay with that as a follow-up PR since this one has the approvals needed. Your call @anton-trunov

[mohammadfawaz]

Looks great! The only thing I wonder is if someday down the line we can merge this and other AST-traversing checks we do into one traversal. But the traversals themselves are slightly different so it may not be generalizable (thinking of purity checking mainly).

Actually @anton-trunov mentioned this to me recently when we were chatting. He thinks he has ideas on how to merge purity checking into this as he's already collecting storage access here anyways.

[mohammadfawaz]

@anton-trunov I've opened #3342 and assigned it to you so we can follow up with additional testing.

[anton-trunov]

Awesome! Thanks everyone!

[anton-trunov]

@otrho Great suggestion to test left-to-right evaluation on which the CEI analysis relies! In PR #3356, I added a compiler test for that: 332a240