From 3fe624a69e65fdb0fe7db418e45701ead1bdf68c Mon Sep 17 00:00:00 2001 From: Kaden Zipfel Date: Sun, 12 Jan 2020 13:50:26 -0800 Subject: [PATCH 1/6] Fix compiler erros --- contracts/RockPaperScissors.sol | 47 ++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/contracts/RockPaperScissors.sol b/contracts/RockPaperScissors.sol index 79820df..0b90759 100644 --- a/contracts/RockPaperScissors.sol +++ b/contracts/RockPaperScissors.sol @@ -21,7 +21,7 @@ contract RockPaperScissors { bytes32 commitment; Choice choice; } - + // events event Payout(address player, uint amount); @@ -42,20 +42,20 @@ contract RockPaperScissors { } // TODO: go through and write explicit 'stored' and 'memory' everywhere - function commit(bytes32 commitment) payable public { + function commit(bytes32 commitment) public payable { // only allow commit stages uint playerIndex; if(stage == Stage.FirstCommit) playerIndex = 0; else if(stage == Stage.SecondCommit) playerIndex = 1; - else revert(); + else revert("both players have already played"); //TODO: possible overflow uint commitAmount = bet + deposit; - require(msg.value >= commitAmount); - + require(msg.value >= commitAmount, "value must be greater than commit amount"); + // return any excess if(msg.value > commitAmount) msg.sender.transfer(msg.value - commitAmount); - + // store the commitment players[playerIndex] = CommitChoice(msg.sender, commitment, Choice.None); @@ -64,33 +64,33 @@ contract RockPaperScissors { // otherwise we must already be on the second, move to first reveal else stage = Stage.FirstReveal; } - + function reveal(Choice choice, bytes32 blindingFactor) public { - require(stage == Stage.FirstReveal || stage == Stage.SecondReveal); + require(stage == Stage.FirstReveal || stage == Stage.SecondReveal, "not at reveal stage"); // only valid choices - require(choice == Choice.Rock || choice == Choice.Paper || choice == Choice.Scissors); - + require(choice == Choice.Rock || choice == Choice.Paper || choice == Choice.Scissors, "invalid choice"); + // find the player index uint playerIndex; if(players[0].playerAddress == msg.sender) playerIndex = 0; else if (players[1].playerAddress == msg.sender) playerIndex = 1; // unknown player - else revert(); + else revert("unknown player"); // find the player data - CommitChoice storage commitChoice = players[playerIndex]; + CommitChoice storage commitChoice = players[playerIndex]; // check the hash, we have a hash of sender, choice, blind so that players cannot learn anything from a committment // if it were just choice, blind the other player could view this and submit it themselves to reliably achieve a draw - require(keccak256(abi.encodePacked(msg.sender, choice, blindingFactor)) == commitChoice.commitment); - + require(keccak256(abi.encodePacked(msg.sender, choice, blindingFactor)) == commitChoice.commitment, "invalid hash"); + // update if correct commitChoice.choice = choice; - if(stage == Stage.FirstReveal) { + if(stage == Stage.FirstReveal) { // TODO: possible overflow - // if this is the first reveal we set the deadline for the second one - revealDeadline = block.number + revealSpan; + // if this is the first reveal we set the deadline for the second one + revealDeadline = block.number + revealSpan; // if we're on first reveal, move to the second stage = Stage.SecondReveal; } @@ -101,7 +101,7 @@ contract RockPaperScissors { function distribute() public { // to distribute we need: // a) to be in the distribute stage OR b) still in the second reveal stage but past the deadline - require(stage == Stage.Distribute || (stage == Stage.SecondReveal && revealDeadline <= block.number)); + require(stage == Stage.Distribute || (stage == Stage.SecondReveal && revealDeadline <= block.number), "cannot yet"); // calulate value of payouts for players //TODO: possible overflow @@ -134,7 +134,8 @@ contract RockPaperScissors { // rock beats scissors player0Payout = winningAmount; player1Payout = deposit; - } + } + // TODO: Replace with assert statement else revert(); } @@ -149,6 +150,7 @@ contract RockPaperScissors { player0Payout = deposit; player1Payout = winningAmount; } + // TODO: Replace with assert statement else revert(); } else if(players[0].choice == Choice.Scissors) { @@ -162,16 +164,19 @@ contract RockPaperScissors { player0Payout = winningAmount; player1Payout = deposit; } + // TODO: Replace with assert statement else revert(); } + // TODO: Replace with assert statement else revert(); // send the payouts + // TODO: Use low level call instead of send if(player0Payout != 0 && players[0].playerAddress.send(player0Payout)){ - emit Payout(players[0].playerAddress, player0Payout); + emit Payout(players[0].playerAddress, player0Payout); } if(player1Payout != 0 && players[1].playerAddress.send(player1Payout)){ - emit Payout(players[1].playerAddress, player1Payout); + emit Payout(players[1].playerAddress, player1Payout); } //reset the state to play again From 2e96f8c9734f05fd034e2b8e2f6a624aaaaa1ced Mon Sep 17 00:00:00 2001 From: Kaden Zipfel Date: Sun, 12 Jan 2020 14:08:12 -0800 Subject: [PATCH 2/6] Overflow protection --- contracts/RockPaperScissors.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/RockPaperScissors.sol b/contracts/RockPaperScissors.sol index 0b90759..3223cf4 100644 --- a/contracts/RockPaperScissors.sol +++ b/contracts/RockPaperScissors.sol @@ -49,8 +49,8 @@ contract RockPaperScissors { else if(stage == Stage.SecondCommit) playerIndex = 1; else revert("both players have already played"); - //TODO: possible overflow uint commitAmount = bet + deposit; + require(commitAmount >= bet, "overflow error"); require(msg.value >= commitAmount, "value must be greater than commit amount"); // return any excess @@ -88,9 +88,9 @@ contract RockPaperScissors { commitChoice.choice = choice; if(stage == Stage.FirstReveal) { - // TODO: possible overflow // if this is the first reveal we set the deadline for the second one revealDeadline = block.number + revealSpan; + require(revealDeadline >= block.number, "overflow error"); // if we're on first reveal, move to the second stage = Stage.SecondReveal; } @@ -104,10 +104,10 @@ contract RockPaperScissors { require(stage == Stage.Distribute || (stage == Stage.SecondReveal && revealDeadline <= block.number), "cannot yet"); // calulate value of payouts for players - //TODO: possible overflow uint player0Payout; uint player1Payout; uint winningAmount = deposit + 2 * bet; + require(winningAmount / deposit == 2 * bet, "overflow error"); // we always draw with the same choices, and we dont lose our deposit even if neither revealed if(players[0].choice == players[1].choice) { From ab111c5f763a28133ea8c5d48c8f8c51f1e25776 Mon Sep 17 00:00:00 2001 From: Kaden Zipfel Date: Sun, 12 Jan 2020 14:22:52 -0800 Subject: [PATCH 3/6] Replace transfer and send with low-level calls --- contracts/RockPaperScissors.sol | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/contracts/RockPaperScissors.sol b/contracts/RockPaperScissors.sol index 3223cf4..ddd89df 100644 --- a/contracts/RockPaperScissors.sol +++ b/contracts/RockPaperScissors.sol @@ -54,7 +54,10 @@ contract RockPaperScissors { require(msg.value >= commitAmount, "value must be greater than commit amount"); // return any excess - if(msg.value > commitAmount) msg.sender.transfer(msg.value - commitAmount); + if(msg.value > commitAmount) { + (bool success, ) = msg.sender.call.value(msg.value - commitAmount)(""); + require(success, "call failed"); + } // store the commitment players[playerIndex] = CommitChoice(msg.sender, commitment, Choice.None); @@ -171,11 +174,13 @@ contract RockPaperScissors { else revert(); // send the payouts - // TODO: Use low level call instead of send - if(player0Payout != 0 && players[0].playerAddress.send(player0Payout)){ + if(player0Payout > 0) { + (bool success, ) = players[0].playerAddress.call.value(player0Payout)(""); + require(success, 'call failed'); emit Payout(players[0].playerAddress, player0Payout); - } - if(player1Payout != 0 && players[1].playerAddress.send(player1Payout)){ + } else if (player1Payout > 0) { + (bool success, ) = players[1].playerAddress.call.value(player1Payout)(""); + require(success, 'call failed'); emit Payout(players[1].playerAddress, player1Payout); } From d8fbba4fcf91584bb2d9c7aaf9f23cfa50062ede Mon Sep 17 00:00:00 2001 From: Kaden Zipfel Date: Sun, 12 Jan 2020 14:36:01 -0800 Subject: [PATCH 4/6] Add assert statements --- contracts/RockPaperScissors.sol | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/contracts/RockPaperScissors.sol b/contracts/RockPaperScissors.sol index ddd89df..e9fb460 100644 --- a/contracts/RockPaperScissors.sol +++ b/contracts/RockPaperScissors.sol @@ -128,6 +128,7 @@ contract RockPaperScissors { } // both players have made a choice, and they did not draw else if(players[0].choice == Choice.Rock) { + assert(players[1].choice == Choice.Paper || Choice.Scissors); if(players[1].choice == Choice.Paper) { // rock loses to paper player0Payout = deposit; @@ -138,11 +139,10 @@ contract RockPaperScissors { player0Payout = winningAmount; player1Payout = deposit; } - // TODO: Replace with assert statement - else revert(); } else if(players[0].choice == Choice.Paper) { + assert(players[1].choice == Choice.Rock || Choice.Scissors); if(players[1].choice == Choice.Rock) { // paper beats rock player0Payout = winningAmount; @@ -153,10 +153,9 @@ contract RockPaperScissors { player0Payout = deposit; player1Payout = winningAmount; } - // TODO: Replace with assert statement - else revert(); } else if(players[0].choice == Choice.Scissors) { + assert(players[1].choice == Choice.Paper || Choice.Rock); if(players[1].choice == Choice.Rock) { // scissors lose to paper player0Payout = deposit; @@ -167,11 +166,8 @@ contract RockPaperScissors { player0Payout = winningAmount; player1Payout = deposit; } - // TODO: Replace with assert statement - else revert(); } - // TODO: Replace with assert statement - else revert(); + else revert("invalid choice"); // send the payouts if(player0Payout > 0) { From 549daffef1a12c9e7bff48b57c00669932211266 Mon Sep 17 00:00:00 2001 From: Kaden Zipfel Date: Sun, 12 Jan 2020 14:37:36 -0800 Subject: [PATCH 5/6] Change version and fix errors --- contracts/RockPaperScissors.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/RockPaperScissors.sol b/contracts/RockPaperScissors.sol index e9fb460..46b5966 100644 --- a/contracts/RockPaperScissors.sol +++ b/contracts/RockPaperScissors.sol @@ -1,4 +1,4 @@ -pragma solidity ^0.4.24; +pragma solidity ^0.5.0; contract RockPaperScissors { enum Choice { @@ -128,7 +128,7 @@ contract RockPaperScissors { } // both players have made a choice, and they did not draw else if(players[0].choice == Choice.Rock) { - assert(players[1].choice == Choice.Paper || Choice.Scissors); + assert(players[1].choice == Choice.Paper || players[1].choice == Choice.Scissors); if(players[1].choice == Choice.Paper) { // rock loses to paper player0Payout = deposit; @@ -142,7 +142,7 @@ contract RockPaperScissors { } else if(players[0].choice == Choice.Paper) { - assert(players[1].choice == Choice.Rock || Choice.Scissors); + assert(players[1].choice == Choice.Rock || players[1].choice == Choice.Scissors); if(players[1].choice == Choice.Rock) { // paper beats rock player0Payout = winningAmount; @@ -155,7 +155,7 @@ contract RockPaperScissors { } } else if(players[0].choice == Choice.Scissors) { - assert(players[1].choice == Choice.Paper || Choice.Rock); + assert(players[1].choice == Choice.Paper || players[1].choice == Choice.Rock); if(players[1].choice == Choice.Rock) { // scissors lose to paper player0Payout = deposit; From 956fe9f015476b0a26cfd7b964ae3e55e24dad23 Mon Sep 17 00:00:00 2001 From: Kaden Zipfel Date: Sun, 12 Jan 2020 14:53:50 -0800 Subject: [PATCH 6/6] Modify comments --- contracts/RockPaperScissors.sol | 92 ++++++++++++--------------------- 1 file changed, 32 insertions(+), 60 deletions(-) diff --git a/contracts/RockPaperScissors.sol b/contracts/RockPaperScissors.sol index 46b5966..5cc8f8a 100644 --- a/contracts/RockPaperScissors.sol +++ b/contracts/RockPaperScissors.sol @@ -22,15 +22,14 @@ contract RockPaperScissors { Choice choice; } - // events event Payout(address player, uint amount); - //initialisation args + // Initialisation args uint public bet; uint public deposit; uint public revealSpan; - // state vars + // State vars CommitChoice[2] public players; uint public revealDeadline; Stage public stage = Stage.FirstCommit; @@ -41,9 +40,8 @@ contract RockPaperScissors { revealSpan = _revealSpan; } - // TODO: go through and write explicit 'stored' and 'memory' everywhere function commit(bytes32 commitment) public payable { - // only allow commit stages + // Only run during commit stages uint playerIndex; if(stage == Stage.FirstCommit) playerIndex = 0; else if(stage == Stage.SecondCommit) playerIndex = 1; @@ -53,89 +51,87 @@ contract RockPaperScissors { require(commitAmount >= bet, "overflow error"); require(msg.value >= commitAmount, "value must be greater than commit amount"); - // return any excess + // Return additional funds transferred if(msg.value > commitAmount) { (bool success, ) = msg.sender.call.value(msg.value - commitAmount)(""); require(success, "call failed"); } - // store the commitment + // Store the commitment players[playerIndex] = CommitChoice(msg.sender, commitment, Choice.None); - // if we're on the first commit, then move to the second + // If we're on the first commit, then move to the second if(stage == Stage.FirstCommit) stage = Stage.SecondCommit; - // otherwise we must already be on the second, move to first reveal + // Otherwise we must already be on the second, move to first reveal else stage = Stage.FirstReveal; } function reveal(Choice choice, bytes32 blindingFactor) public { + // Only run during reveal stages require(stage == Stage.FirstReveal || stage == Stage.SecondReveal, "not at reveal stage"); - // only valid choices + // Only accept valid choices require(choice == Choice.Rock || choice == Choice.Paper || choice == Choice.Scissors, "invalid choice"); - // find the player index + // Find the player index uint playerIndex; if(players[0].playerAddress == msg.sender) playerIndex = 0; else if (players[1].playerAddress == msg.sender) playerIndex = 1; - // unknown player + // Revert if unknown player else revert("unknown player"); - // find the player data + // Find player data CommitChoice storage commitChoice = players[playerIndex]; - // check the hash, we have a hash of sender, choice, blind so that players cannot learn anything from a committment - // if it were just choice, blind the other player could view this and submit it themselves to reliably achieve a draw + // Check the hash to ensure the commitment is correct require(keccak256(abi.encodePacked(msg.sender, choice, blindingFactor)) == commitChoice.commitment, "invalid hash"); - // update if correct + // Update choice if correct commitChoice.choice = choice; if(stage == Stage.FirstReveal) { - // if this is the first reveal we set the deadline for the second one + // If this is the first reveal, set the deadline for the second one revealDeadline = block.number + revealSpan; require(revealDeadline >= block.number, "overflow error"); - // if we're on first reveal, move to the second + // Move to second reveal stage = Stage.SecondReveal; } - // if we're on second reveal, move to distribute + // If we're on second reveal, move to distribute stage else stage = Stage.Distribute; } function distribute() public { - // to distribute we need: - // a) to be in the distribute stage OR b) still in the second reveal stage but past the deadline - require(stage == Stage.Distribute || (stage == Stage.SecondReveal && revealDeadline <= block.number), "cannot yet"); + // To distribute we need: + // a) To be in the distribute stage OR + // b) Still in the second reveal stage but past the deadline + require(stage == Stage.Distribute || (stage == Stage.SecondReveal && revealDeadline <= block.number), "cannot yet distribute"); - // calulate value of payouts for players + // Calculate value of payouts for players uint player0Payout; uint player1Payout; uint winningAmount = deposit + 2 * bet; require(winningAmount / deposit == 2 * bet, "overflow error"); - // we always draw with the same choices, and we dont lose our deposit even if neither revealed + // If both players picked the same choice, return their deposits and bets if(players[0].choice == players[1].choice) { player0Payout = deposit + bet; player1Payout = deposit + bet; } - // at least one person has made a choice, otherwise we wouldn't be here - // in the situation that only one person chose that person wins, and the person - // who did not will lose their deposit + // If only one player made a choice, they win else if(players[0].choice == Choice.None) { player1Payout = winningAmount; } else if(players[1].choice == Choice.None) { player0Payout = winningAmount; } - // both players have made a choice, and they did not draw else if(players[0].choice == Choice.Rock) { assert(players[1].choice == Choice.Paper || players[1].choice == Choice.Scissors); if(players[1].choice == Choice.Paper) { - // rock loses to paper + // Rock loses to paper player0Payout = deposit; player1Payout = winningAmount; } else if(players[1].choice == Choice.Scissors) { - // rock beats scissors + // Rock beats scissors player0Payout = winningAmount; player1Payout = deposit; } @@ -144,12 +140,12 @@ contract RockPaperScissors { else if(players[0].choice == Choice.Paper) { assert(players[1].choice == Choice.Rock || players[1].choice == Choice.Scissors); if(players[1].choice == Choice.Rock) { - // paper beats rock + // Paper beats rock player0Payout = winningAmount; player1Payout = deposit; } else if(players[1].choice == Choice.Scissors) { - // paper loses to scissors + // Paper loses to scissors player0Payout = deposit; player1Payout = winningAmount; } @@ -157,19 +153,19 @@ contract RockPaperScissors { else if(players[0].choice == Choice.Scissors) { assert(players[1].choice == Choice.Paper || players[1].choice == Choice.Rock); if(players[1].choice == Choice.Rock) { - // scissors lose to paper + // Scissors lose to rock player0Payout = deposit; player1Payout = winningAmount; } else if(players[1].choice == Choice.Paper) { - // scissors beats paper + // Scissors beats paper player0Payout = winningAmount; player1Payout = deposit; } } else revert("invalid choice"); - // send the payouts + // Send the payouts if(player0Payout > 0) { (bool success, ) = players[0].playerAddress.call.value(player0Payout)(""); require(success, 'call failed'); @@ -180,33 +176,9 @@ contract RockPaperScissors { emit Payout(players[1].playerAddress, player1Payout); } - //reset the state to play again + // Reset the state to play again delete players; revealDeadline = 0; stage = Stage.FirstCommit; } } - -// ISSUES - -// 1. We should pass in the address of the other player to 'commit', -// then adding a second commit should start the timer - -// concerns - assymetry in setting timeout -// concerns - timeout possibly not greatly reduced by addition of deposit - -// TODO: look at all the access modifiers for all members and functions -// TODO: what are the consequences? - -// TODO: checkout all the integer operations for possible overflows -// TODO: consider event logging - - -// IMPROVMENTS: - -// 1. Allow second player to reveal without committing. -// 2. Allow re-use of the contract? Or allow a self destruct to occur? -// 3. Choose where to send lost deposits. -// 4. Allow a player to forfeit for a cheaper gas cost? - -