2

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

  • They way you calculate the winner is exploitable by the fifth bet. The fifth user can just wait until the current block would mean he is the winner and place his bet accordingly. Check blog.oraclize.it/… for how to add random data to your smart contract. – Jacob Jul 3 '17 at 9:45
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.

  1. 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.

  2. 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);

  3. 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 :)

  • I'm glad to hear that :). If you think that this answer helped you please mark the question as answered to help other users navigate through answers. – Jakub Wojciechowski Jul 9 '17 at 12:29
1

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.

  • this is very helpful. What I'm wondering is: if I can add/update, means I can edit, which means I could "cheat" the users. I thought one of the main benefits of smart contracts is that they're unmodifiable. Why should I add a possibility to edit it? – Anto Jul 2 '17 at 19:47
  • In some use-cases, you need to have the upgradability feature. Let's say, I'm developing a DApp on insurance, then after deploying the smart contract I realized that I should have one extra feature as well but I missed adding it. In that case, upgradability of contract becomes very important. The control(power of upgradability) of the smart contract shall always be with the owner, who has deployed it. Also, just to mention - smart contracts are mutable but once a transaction is added on the blockchain, that is always immutable. – Sanchit Jul 2 '17 at 19:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.