ethereum / remix-project

Remix is a browser-based compiler and IDE that enables users to build Ethereum contracts with Solidity language and to debug transactions.
https://remix-ide.readthedocs.io
MIT License
2.46k stars 952 forks source link

[Static analysis] Random number generation #287

Open pauliax opened 6 years ago

pauliax commented 6 years ago

This is a feature request for new static analysis module.

Issue: Every experienced Solidity developer probably knows about this issue, but is there a possibility to add a static analysis module that informs the developer if he is using an unsafe known pattern for random number generation? I think this issue is very serious, as the wrong implementation could lead to a big loss of Ether (especially in lotteries and other gambling DApps). I do not mean cases when "Oraclize" is used, the main concern is about a more decentralized way to get such number. Some terrible examples:

bool won = (block.number % 2) == 0; var random = uint(sha3(block.timestamp)) % 2;

Recognition: Usually to generate a random number inside the contract developer uses some parameters of the block (timestamp, blockhash, number, difficulty, etc). Finding the usage of one of these parameters should not be hard. The main concern is finding when such parameter is used for a random number generation. Maybe the variable or function name contains words like "random", "rnd", etc?

More: From the documentation of Solidity: "Do not rely on block.timestamp, now and blockhash as a source of randomness, unless you know what you are doing. Both the timestamp and the block hash can be influenced by miners to some degree. Bad actors in the mining community can for example run a casino payout function on a chosen hash and just retry a different hash if they did not receive any money." https://media.readthedocs.org/pdf/solidity/develop/solidity.pdf

yann300 commented 6 years ago

@soad003 @chriseth would that be doable?

axic commented 6 years ago

I think someone needs to clearly write down the rules first and then it can be estimated how much work it is to implement.

One thing potentially worth warning for is using hashing functions with one or more block-constant variables and other constant literals, e.g. sha3(block.timestamp, block.number, 42) or sha3(block.timestamp)

However note that with recent versions of Solidity this will change, where sha3 was renamed to keccak256 and the new class of abi.encode* methods must be used since hashing functions require a bytes input only. The above examples will turn intokeccak256(abi.encodePacked(block.timestamp, block.number, 42)) or keccak256(abi.encodePacked(block.timestamp))

soad003 commented 6 years ago

We already have warnings for block.blockhash usage a well as block.timestamp. Maybe we could just adapt the warning that it explicitly stats that using those values as seed to generate randomness is not a good idea. The warning already stats that those values can be influenced by miners to a certain degree but it does not explicitly mention randomness.