Where can I get feedback on how to improve my smart contract?

Question

I built a fairly simple contract lottery that gives 20% of all proceedings to Give Directly, my favorite charity organization. I'd like to have someone who know what (s)he's doing to have a look at my code and help me polish it. Is there a community/slack/forum for that?

Here's the lottery: https://github.com/CrazybutSolid/ethereum-lottery

Thanks

Answer 1

1. I think the most serious problem is releasing control out of your contract by using:

gamblers[random].transfer(0.8 ether);

You are updating the contract state:

my_length = 0;
gamblers[1] = 0;
gamblers[2] = 0;
gamblers[3] = 0;
gamblers[4] = 0;
gamblers[5] = 0;

only after the funds are transferred so the easiest way to attack for a malicious contract winner crashes on receiving the prize causing your lottery to stuck and prevent starting a new round. A similar problem is well described in King of the Ether example in docs and you should favor offering the gamblers a way of pulling the reward instead of initiating the transfer as in pull payment pattern.

2. There is a fundamental problem with randomness as miners may try to trick the system by manipulating the block number. In short, it's only safe if you keep the lottery prize below the miners' reward per block. You may look at thread with the discussion about safe randomness in smart contracts.

3. Usually it's a better practice to separate conditions from contract login as in Condition oriented programming so you may try to refactor you payment condition as a functional modifier:

   require(msg.value == 0.2 ether);

4. Try to avoid magic numbers hardcodded in your contract like:

   msg.value == 0.2 ether

   my_length == 5

   gamblers[random].transfer(0.8 ether);

   It may be better to refactor all of the parameters to variables that may be defined in constructor. You may allow them to be modified by contract owner later on if your lotter is successful and you want to increase the number of gambler or the prize value :)

Answer 2

I just had a look at your contract and one important feedback I can give right away is, that your smart contract is not upgradable. Which means, if this contract is deployed, it can never be killed. If you kill it, you will loose all your data.

With the current code, in order to add/update any functionality, you will have to re-deploy the contract and use the newly deployed contract, which will eventually not have your old contract's data.

So, if you care about the data then you shall code your contract in a way so that it becomes upgradable and you can preserve the data associated with the contract.

You can read few blogs to understand one approach on how to write upgradable contract -

https://blog.colony.io/writing-upgradeable-contracts-in-solidity-6743f0eecc88

https://medium.com/@nrchandan/interfaces-make-your-solidity-contracts-upgradeable-74cd1646a717

Hope this helps.