New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Outcome can be predicted #1

Closed
Raz0r opened this Issue Dec 25, 2017 · 25 comments

Comments

Projects
None yet
5 participants
@Raz0r

Raz0r commented Dec 25, 2017

To choose the winner(s) Slotthereum uses block.blockhash(block.number - N) where N is a number pointing to some past block. An attacker just needs to create a contract with the copied getNumber() function and call placeBet() with the computed value.

Consider using commit-reveal approach to determine lottery winners.

@edse

This comment has been minimized.

Contributor

edse commented Dec 25, 2017

@Raz0r thanks for the feedback but you are missing a few things:

By the time the attacker want to place his bet, someone else can take it.

Ethereum has new blocks every 15 seconds in average, so again, by the time the attacker broadcast his transaction a new block can come and change the winner number.

If you are so confident about your claim can you win a few rounds on mainnet? There is a jackpot of $200.

https://slotthereum.github.io
https://lotthereum.github.io

@Raz0r

This comment has been minimized.

@edse

This comment has been minimized.

Contributor

edse commented Dec 26, 2017

Really nice @Raz0r! Thanks again for the heads up. I'm working on a fix for that. PR coming soon.

@edse edse closed this in f783cde Dec 27, 2017

@Raz0r

This comment has been minimized.

Raz0r commented Dec 27, 2017

The changes you have made do not fix the problem, since the attacker can still look up seed and nonce in contract storage (storage indices 5 and 6 respectively). Any implementation that determines the winner in the same transaction when betting occurs will be flawed.

What you need to do is a commit-reveal approach. You can read more here: https://blog.winsome.io/random-number-generation-on-winsome-io-future-blockhashes-fe44b1c61d35

@edse

This comment has been minimized.

Contributor

edse commented Dec 27, 2017

Nope, because the seed is regenerated during the bet, so what you will see is the seed from the previous bet.

@Raz0r

This comment has been minimized.

Raz0r commented Dec 27, 2017

The newly generated seed is a hash of the previous seed and current nonce which are known to the attacker. It is trivial to precompute this new seed.

@edse

This comment has been minimized.

Contributor

edse commented Dec 27, 2017

... plus a number between 0-255. Good luck!

@Raz0r

This comment has been minimized.

@jnardiello

This comment has been minimized.

jnardiello commented Dec 27, 2017

LoL

@persas

This comment has been minimized.

persas commented Dec 27, 2017

JAJAJAJAJAJAJAJA, Good luck!

@f3d0x0

This comment has been minimized.

f3d0x0 commented Dec 27, 2017

Never say "good luck" anymore.
Never say "good luck" anymore.
Never say "good luck" anymore.
.
.
Lession learned.

@edse edse reopened this Dec 27, 2017

@edse edse closed this in aa4088e Dec 27, 2017

@edse

This comment has been minimized.

Contributor

edse commented Dec 27, 2017

@Raz0r

This comment has been minimized.

Raz0r commented Dec 27, 2017

You haven't updated the source code of the contract. Is it security by obscurity now? How can users trust the lottery? There is no point to use blockchain then.

@edse

This comment has been minimized.

Contributor

edse commented Dec 27, 2017

Come on @Raz0r! You're better than this. The code is on blockchain. You can decompile it! 😃

@edse

This comment has been minimized.

Contributor

edse commented Dec 27, 2017

@Raz0r

This comment has been minimized.

Raz0r commented Dec 27, 2017

The latest commit has introduced two new bugs:

  • there is no check that block number supplied to block.blockhash() is greater than block.number - 256. If games[gameId].blockNumber points to a block that is 256 blocks older than the current block, then block.blockhash() will always return zero as is stated by Solidity documentation:
    image The attacker just needs to wait for 256 blocks since his bet and call placeBet() again which will use zero as a hash for getNumber(). He can use the bug below to not let other users to call placeBet() while he is waiting for 256 blocks to be mined.
  • an attacker can block the lottery completely by causing an exception when it tries to send ether in
    games[lastGameId].player.transfer(games[lastGameId].prize);
    To do that he needs to make a bet from his contract that does not have a fallback function. It will block the lottery for other users. Alternatively, he may implement a conditional fallback function that will unblock the lottery at his will. Consider using "pull" strategy instead of the "push" one for external calls. More info here: https://consensys.github.io/smart-contract-best-practices/recommendations/#favor-pull-over-push-for-external-calls
@edse

This comment has been minimized.

Contributor

edse commented Dec 28, 2017

Working on it! Issue reopened. Thanks again @Raz0r.

@edse edse reopened this Dec 28, 2017

@edse

This comment has been minimized.

Contributor

edse commented Dec 30, 2017

  • Block number issue solved by storing the block hash instead of the block number.

  • To solve the transfer issue, users now have request a withdraw in a new transaction completely isolated from the bet.

@edse edse closed this Dec 30, 2017

@Raz0r

This comment has been minimized.

Raz0r commented Dec 30, 2017

The latest fix is a step back. By storing block.blockhash(block.number - 1) the algorithm is nothing better than the original implemenation. The only difference is that you choose the winner in the next placeBet() call but one can still predict the outcome because the blockhash of a last block is known to the attacker. Storing block.number that you did previously is a right approach. You just need to check that the result of block.blockhash() is not zero. You can refund the player if there were no bets within 256 blocks time interval. If this is not suitable for you, you can check out Signidice algo. However, in this case make sure not to use ECDSA (see https://github.com/pertsev/web3_utilz/tree/master/ECDSA%20signature%20generating%20(cheating)).

@edse

This comment has been minimized.

Contributor

edse commented Dec 30, 2017

  1. "one can still predict the outcome because the blockhash of a last block is known to the attacker" - But the attacker only knows the hash after his bet is placed, so he cant predict it before placing the bet.
@Raz0r

This comment has been minimized.

Raz0r commented Dec 30, 2017

The attacker creates a contract that calls placeBet() via internal message, block.blockhash() will yield the same result within the same transaction.

See exploit code: https://etherscan.io/address/0xb8fbef1c1108505fa76df25fded7f74f0f01d9b0#code

@edse

This comment has been minimized.

Contributor

edse commented Dec 30, 2017

I can check if tx.origin == msg.sender to make sure this method is not being called from another contract.

@Raz0r

This comment has been minimized.

Raz0r commented Dec 30, 2017

This is not a good solution, because legitimate multi-sig contracts (e.g. Mist wallet) will not be able to call your contract. tx.origin is generally considered as a bad security practice (ethereum/solidity#683).

Anyway, an attacker does not need a contract at all to perform the attack, because he can look up the last block's hash and submit his tx. It wouldn't be possible if you used some state variables from the current block, like block.difficulty, block.gaslimit, etc. But again, I do not advise to use these state vars as a source of entropy.

@edse

This comment has been minimized.

Contributor

edse commented Dec 30, 2017

@Raz0r

This comment has been minimized.

Raz0r commented Dec 31, 2017

So, in a2b4059 you deliberately have dropped support of all multi-sigs. And out of all block variables you have chosen the most predictable one - now which is alias to block.timestamp. By the way, there is no such function now(), the contract won't compile. The current code is STILL predictable, instead of a contract an attacker may use an off-chain script that monitors contract storage and looks up blockhash of the last block, nonce and seed. Although now adds some entropy it is feasible to predict it. I am not even saying here about possibility of miners' manipulation.

At this point I am giving up to make further suggestions. This thread contains plenty of information to build a safer PRNG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment