Two months ago, I wrote a smart contract with Remix.ethereum.org. Line 1 of my code has:
pragma solidity ^0.4.11;
It compiled fine with no errors and nor warnings. I tested every function in Ropsten and it worked fine.
Now, I'm getting closer to deploying it on the Main net. I see that Remix has changed. Now, it says:
Static Analysis raised 50 warning(s) that requires your attention.
Under the Analysis tab, there are many warnings, that I cannot figure out how to resolve. One example is,
Potential Violation of Checks-Effects-Interaction pattern in
TestContract.testFunction(address): Could potentially lead to
re-entrancy vulnerability. Note: Modifiers are currently not
considered by this static analysis.
Another example is,
Gas requirement of function browser/TestContract
8.sol:MintableToken.transferFrom(address,address,uint256) unknown or not constant. If the gas requirement of a function is higher than the
block gas limit, it cannot be executed. Please avoid loops in your
functions or actions that modify large areas of storage (this includes
clearing or copying arrays in storage)
Another example is,
Fallback function of contract browser/TestContract 8.sol:Test requires
too much gas (null). If the fallback function requires more than 2300
gas, the contract cannot receive Ether.
Another example is,
Test.transferFrom(address,address,uint256): Potentially
should be constant but is not. Note: Modifiers are currently not
considered by this static analysis.
There are multiple occurrences of each of the above examples. Can I ignore all of these warnings? If not, how do I resolve them?
Under Settings, it was using Solidity version 0.4.11. If I change this to 0.4.17, I get dozens of warnings under the Compile tab as well. One example is,
browser/TestContract 8.sol:15:3 Warning: No visibility specified. Defaulting to "public":
function register(string key) {
^
Spanning multiple lines.
I can add "public" after the close bracket for the parameters in each of the functions to resolve the above warnings, but I'm not sure if that is the right thing to do.
Another warning example is,
browser/TestContract 8.sol:100:3 Warning: Function state mutability
can be restricted to purefunction mil(uint256 a, uint256 b) internal constant returns (uint256) {
^
Spanning multiple lines.
I have no idea how to resolve the above warning.
Am I correct to assume that I can ignore 0.4.17 and go back to 0.4.11?
0.4.11 up to 0.4.16 will not show these warnings under the Compiler tab. Should I use 0.4.16?
My code can be seen here
Best Answer
In general
You can let the code run, ignoring the compiler warnings, but it's always good not to do so. Most of the time, compiler warnings are warning you about something that may cause a bug in your code. Compared to compiler errors, the difference is your code is syntactically correct, hence can be compiled, but can lead to problems, especially when someone try to do something you don't expect others to do.
A good read regarding this can be found here, and you may refer to this question as well. And most importantly, read the security considerations from Solidity docs. That may save your life!
Anyhow, the compiler is not as intelligent as you and it claims what it's lacking from in statements with
Note:
as follows:Sometimes, these notes may tell the occasions that you can safely ignore these warnings.
Example
One error warning you have mentioned is:
Let's say you have
function A(uint val) {}
. You want it to be used only within the contract due to some prior validations you need to do (This is just an example ;)). And if the function is called without validation, that may cause some unwanted executions. And the only use offunction A
is to use it inside some other functions within the contract.Now if you keep the contract like above, since
function A()
is also by default set to public, anyone who use your contract can usefunction A
with anyuint
value and that may cause trouble. But if you had the access modifier offunction A
declared asprivate
, this trouble won't appear sincefunction A
can't be accessed publicly. And as your requirement is also to usefunction A
only inside the other functions within the contract, your goal is also achieved.As when no access modifier is declared, the compiler assumes a function to be public and it warns you that it's setting the function to be public and ask you to check whether it should really be public. In the above case, you will get warnings regarding both functions and setting
function A
to be private andfunction B
to be public will remove the warnings because now the compiler knows you are intentionally makingfunction B
public andfunction A
private.The warning regarding
function B
being set to public is a type of warning that you can safely ignore as your intention is also the same, but the warning regardingfunction A
is something you should really consider.What I suggest,
not to ignore the warning, but go through them and check whether they do really matter, with how you expect the code to behave and try to resolve them.
try to use the latest compiler version (atm 0.4.17), as newer stable versions tend to have less bugs and more features (hence it generates new warnings).
Some insight regarding some other warnings you get
You may refer to this answer and this question regarding the introduction of
view
andpure
type functions.This is for not following the pattern of Checks-Effects-Interactions as described here in the docs. Refer to the function that gets this warning and follow the pattern defined in the docs to avoid the warning.
And as per the code found in the link you posted, I think the
closeSale()
warning should probably be due to the compiler assuming that there can be a chance of re-entrancy, as you are calling the member eventLogSaleClosed(uint256 issuedSupply, uint256 restrictedTokens)
after interactions with token contract.What I can suggest is having that event inside the contract
token
. You can see here for re-entrancy vulnerability. It's just saying if the bad code there let another contract to withdraw money several times because they change the state variable shares after sending transaction which might take some time leaving the other contract to call the same function several times because it keepsmsg.sender
value non-zero until the transaction get executed. If you see the good code there, they first setshares[msg.sender]
to zero first. Since you are the one who developed this, you know the flow of expected events and can be sure that re-entrancy won't occur, it's safe to ignore it.What constant means is that it would not be changing the state. That's no state variable is changed. So normally, those functions are called constant functions and are defined with the keyword of constant. You may read this.
So if your functions are not changing any state variable, you can declare them constant. Solidity is reminding you about this.