Rules Wishlist

[federicobond]

This is my wishlist for additional rules. Let me know if you are against any of them getting implemented in the core. I will work on some of them but anything else is up for grabs.

• Suggest send instead of dangerous call.value() (WIP @federicobond)
• Suggest avoiding block.timestamp as it can be manipulated by miners (only in functions) (WIP @federicobond)
• Ensure any function that uses msg.value has payable modifier (WIP @federicobond)
• Ensure fallback function has payable modifier (WIP @federicobond)
• Ensure send return value is checked.
• Extract magic number to constant
• Suggest using selfdestruct instead of deprecated suicide (WIP @federicobond)
• Suggest better storage packing (see Warning here)
• Find duplicate code (this one is hard, perhaps we can write a generic implementation and then integrate it into solium)

[maraoz]

Ensure fallback function has payable modifier

Not always desirable. See for example: https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/Rejector.sol

[federicobond]

Updated. Thanks @maraoz!

[duaraghav8]

Agreed with @maroz and no issues with the rest.

My plan was to separate the linter into 2 layers - the base would only ensure compliance with the Solidity Style Guide.

The 2nd layer contains all the additional rules (like you've stated above) coming from the community. But this layer is optional and therefore can be enabled / disabled by the user at any time.

Until now, I've only been working with the Base. I think we should follow the 2-layer approach.
What do you think

[federicobond]

Eslint has some notion of rulesets. See here. Perhaps we can do something similar with Solium rules, where you can choose to enable the base or core ruleset or something more complete.

I must confess I am not a big fan of the current --sync implementation. I believe that most users will forget to do that and lose out on newly developed rules. It's better to allow users to define a base ruleset and add/remove rules as overrides to the base ruleset(s).

[duaraghav8]

You're right, we should deprecate --sync and have a configuration file system instead.
Something similar to ESLint's.

I'll blog the final plan before I start implementing it but let me know if you have ideas of your own.

[duaraghav8]

Also Rule proposal:

• Use the new constructor() instead of function with same name as contract (Support new constructors (Solidity 0.4.22+) #197)
• Standardise the newline character (either unix \n or windows \r\n) over codebase, dis-allow a mix of both line ending styles
• Ensure all contract files in the codebase use the same solidity version (examine the pragma statement for this). eg, if 2 files use different compiler versions (^0.4.6 and 0.4.7), then raise a warning. Allow user to pass an option to this rule via config to specify the version to settle on, so this rule can also fix the pragma statements to use same version across all contracts. ps- I think this would be a good practice, but would still like to verify. May not be the best thing always. See Solium Gitter for this discussion
• Warn user against the use of jump statements inside assembly blocks (see http://solidity.readthedocs.io/en/develop/assembly.html)
• Throw Warning if a solidity file does not include a PRAGMA directive on top
• Warn user against use of Inline Assembly.
• Warn user if they emit events without using the emit keyword (Proposal: add emit keyword; make it the only way to emit events ethereum/solidity#2877)
• enforce view over constant in function declaration (request: a rule that checks if a function is constant #183)
• Warn user if they're using _ without semicolon inside modifiers (see parser crashes when using underscore without semicolon in modifiers #112), fix for this rule is very easy too
• Function Ordering (http://solidity.readthedocs.io/en/develop/style-guide.html#order-of-functions)
• explicit-storage-type: rule that enforces explicit storage type for function params (this rule should allow to pass in type as an option and also have autofix capability)
• Aggregate all length-related checks into 1 rule max-len (like in eslint - https://eslint.org/docs/rules/max-len)
• no-use-before-define (see "no-unsed-vars" rule does not detect variable usage before declaration #79)
• Rule to ensure implicit type conversions follow recommended practices (http://solidity.readthedocs.io/en/develop/types.html#conversions-between-elementary-types)
• The remix browser solidity guys are doing an amazing job with their static analyser. We could collaborate with them or if that doesn't work out, implement their rules into solium.
• Solcheck is doing some great work. Possibly collab or bring in their rules as plugins/core rules.
• Error if experimental features are enabled for production code (pragma experimental ...)
• Return value of a function call is unused (simply check if the function call's immediate parent is a BlockStatement - means its return val not being used, warn the user)
• Also might be able to take up things from https://github.com/miguelmota/solidity-idiosyncrasies

[federicobond]

Rule:

• Insecure instantiation of array or mapping

See: https://ethereum.karalabe.com/talks/2016-hackethon.html#10

P.S. Is there a secure case for this construct? Or are all its uses insecure?

[federicobond]

• Contract uses send or call.value but has no payable functions or constructor

[federicobond]

• Warn when using tx.origin instead of msg.sender

[federicobond]

• Warn about using a =+ b or a =- b. See here.

As a general rule, a linter for smart contracts should be very strict with spacing around all keywords, operators, etc. to avoid these problems.

[federicobond]

• Find similar identifiers that could be confused

[Sjors]

• require explicit public, private, external or internal

See ConsenSys best practices. Or is there any situation where this is not possible?

[Sjors]

• a function with returns should actually return for each code path, and should return the correct type
• a function without returns should not have any return statement

[duaraghav8]

All security-related rules finalized in this thread are being implemented at https://github.com/duaraghav8/solium-plugin-security

[mushketyk]

Ensure send return value is checked

I was thinking about implementing this rule, and it seems that to avoid false positives we should keep track of all variables of type address and then check if method send is called on any of them. But if we are processing a contract that inherits from other contracts we would first have to get all variables of type address in all parent contracts. To do this, we need to ensure that a rule is processing parent contracts first, collect a set of addresses in parent contracts, and then check if send is called on any of addresses.

Do you think Solium should support something like this?
Am I overthinking this?

[duaraghav8]

@mushketyk you're not overthinking. There are several rules which are on pause because of this problem of Solium only being file-aware instead of project-aware, ie, at any given time, a rule only has context of 1 file whereas it should have context of the entire project so any external elements can also be considered. This problem is well documented in #83

Let's keep this rule on hold for now, because making solium project-aware is a major design change so it will take time. I'll work on it in near future.