Warning when calling other contracts in constructor

[androlo]

There should be a warning when calls to other contracts is made inside a constructor.

Motivation

Calling another contract (B) in the constructor of a contract (A) will give B access to the partially initialized account of A (through msg.sender, or in asm, caller). Account creation is a special circumstance where the account that is being initialized actually has code in it, but any calls to that account during initialization (i.e calls from contracts that are themselves called in the constructor) will be done to a version of the account with no code in it. Generally speaking - code inside a constructor does not always behave as "regular" code, and can also cause other code not to behave as expected, and this is one of those cases.

Suggestion

Adding a warning to the analyzer when someone tries to call another account from inside a constructor.

This would of course not be water proof, as it could still call (internal) functions that in turn calls other contracts etc. Perhaps it should warn when calling any function from inside a constructor (or at least those that are not pure/view), though that would become very complicated. Feels like this is the type of issue that would likely become more complicated as work progresses.

Additionally, the use of codesize and extcodesize(address) should probably be flagged too, and potentially other things that expects a fully initialized object.

If accepted, I would not mind trying to work this in myself, and make a PR!

More info

If someone is interested in initialization issues in general, i wrote a short blog post about it here (https://github.com/androlo/solidity-workshop/blob/master/blogs/2017-07-26-constructors-classes-and-contracts.md). It brings up similar issues from other (mainly object oriented) languages, and talks a bit about the dangers of partially initialized objects. Not all of it applies to Solidity though.

[chriseth]

The problem is that there are valid use-cases for calling functions in constructor, especially creating other contracts in a constructor. Do you have a suggestion about how to silence such a warning? Note that every external function call in Solidity is prefixed with an extcodesize(address) != 0-check, so the worst thing that can happen is that the constructor call fails or that ether that is sent does not go through the fallback function.

[axic]

It would be a good candidate for the static analyzer in Remix

[androlo]

@chriseth I don't know what's best. I also does not like it when i get weird warnings for stuff that isn't actually wrong, so maybe it's a bad idea.

@axic yes that is what i was actually thinking about, just didn't know how to put it. so this is a remix issue then?

i think the extcodesize thing is great but it seems like it would cause problems in cases like this, and the caller wouldn't know why exactly it fails, so the warning would just be a heads up that they're doing something that potentially will not work like they expect. if this is remix related then just let me know and i will close the issue, or just close it yourselves.

[axic]

We have two static analyzers:

• one inside the compiler which checks for things which cannot be turned off (i.e. they are always an issue, such as an error or a warning we want to enforce)
• one inside Remix (in Javascript) which can check for anything from the guidelines, including false positives, because it can be turned off per-module basis

See https://github.com/ethereum/browser-solidity/tree/master/src/app/staticanalysis. It was mostly authored by @soad003, who is really helpful and probably would be happy receiving a helping hand :)

[androlo]

@axic ah, remix seems like the right one then. i will close and just make a small issue in remix and link to this old one. thanks. maybe if it is possible i could look into it and try implementing it and PR (got experience working with javascript).

[MicahZoltu]

This would be an acceptable solution if remix were a tool that could be used in projects other than small toy projects like ICOs and tokens. My understanding is that remix isn't built in a way that makes it unable outside the browser and that means it can't be used as part of CI and it can't effectively deal with large projects.

Either remix needs to be turned into a library rather than a browser app, or features should stop being added to remix directly as they aren't actually usable by any meaningful projects.

[chriseth]

@MicahZoltu you are right, we should extract the static analysis module into a self-contained npm module. @androlo do you want to do that, too? -> https://github.com/ethereum/browser-solidity/issues/888

[androlo]

@chriseth i'll take a look.

[yann300]

There are plans to split the remix and browser-solidity into several npm modules,
but we were not there yet. seems like a good occasion to start

[soad003]

@androlo @axic sorry for the late answer. I really like the idea of a standalone npm module for the static analyser and yes a helping hand is always appreciated. @androlo if you need help or if you have any questions about some implementation detail, let me know!

[androlo]

@soad003 great. i will let you know. this is fairly low prio to me though.

[androlo]

As an npm module this would have to include some form of validation of the provided AST objects themselves. The best thing would be to have that in a stand alone AST util for JS, which would have that functionality as well as some other things, allow walking, etc. I'm not sure that exists. A very clean way of doing it is by using TS, and include proper types for all the nodes. That way the static analyzer (as well as anyone else that uses it) could use it to make sure the objects it is working on is a proper solidity AST, and then use those types when working with them. If the calling code is not TS, then they can just use the js version as usual.

[soad003]

@androlo walking the ast is provided by the package ethereum-remix
(AstWalker, see https://github.com/ethereum/browser-solidity/blob/master/src/app/staticanalysis/staticAnalysisRunner.js).

All the util functionality is provided in
https://github.com/ethereum/browser-solidity/blob/master/src/app/staticanalysis/modules/staticAnalysisCommon.js
things like node identification and extraction of information of certain nodes.

In general i am pro type-system, it would make the usage easier and less error prone (I would go for elm instead for typescript as lang of choice but just my opinion). Although i am not sure if it is a wanted to to have another language in the development stack. I guess you have to talk to @chriseth in that regard.