Rock Paper Scissors
First Flight #38
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

we can cancel the game even before the revealDeadline

lucky2892000

[H-1] summary

we can cancel the game even before the revealDeadline

vulnerability details

playerA creates the game and sent the gameId to playerB to participate. As there is flaw in this RockPaperScissors::timeoutReveal playerB can be able to cancel the game even before the deadline time and playerA who created the game itself cant participate in the game

-> if anyone know that the new games are created , they can able to participate and can be able to cancel the game instantly and thus by not playing the player who has created the game itself

Issue is in the below condition in RockPaperScissors::timeoutReveal

require(block.timestamp > game.revealDeadline, "Reveal phase not timed out yet");

if we can see that this game.revealDeadline will once be updated once both the player has been commited their moves , by default of this value is zero which make block.timestamp > game.revealDeadline condition to true

POC

function testTimeoutRevealAttack() public {
uint256 totalTurns = 3;
uint256 timeoutInterval = 10 minutes;
bytes32 _salt = bytes32(keccak256("salt"));
RockPaperScissors.Move move = RockPaperScissors.Move.Paper;
​
vm.prank(playerA);
uint256 gameNum = game.createGameWithEth{value: 1 ether}(totalTurns, timeoutInterval);
​
vm.startPrank(playerB);
game.joinGameWithEth{value: 1 ether}(gameNum);
bytes32 commit = keccak256(abi.encodePacked(move, _salt));
game.commitMove(gameNum, commit);
game.timeoutReveal(gameNum);
vm.stopPrank();
// here playerA can't commit his move
vm.prank(playerA);
game.commitMove(gameNum, commit);
}

impact - High

likelyhood - High

Recommendations

Update the game.revealDeadline, when atleast one of the player has been comitted his move

we need to update the condition in RockPaperScissors::commitMove

- if (game.commitA != bytes32(0) && game.commitB != bytes32(0)) {
+ if (game.commitA != bytes32(0) || game.commitB != bytes32(0)) {
game.revealDeadline = block.timestamp + game.timeoutInterval;
}

NOTE: by changing to the above condition , there will be an issue with RockPaperScissors::revealMove

As we all know that once can only reveal only when both the players has been commmited

function revealMove(uint256 _gameId, uint8 _move, bytes32 _salt) external {
Game storage game = games[_gameId];
​
require(msg.sender == game.playerA || msg.sender == game.playerB, "Not a player in this game");
require(game.state == GameState.Committed, "Game not in reveal phase");
require(block.timestamp <= game.revealDeadline, "Reveal phase timed out");
require(_move >= 1 && _move <= 3, "Invalid move");
+ require(game.commitA != bytes32(0) && game.commitB != bytes32(0), "one of the player has not committed");
​
.......
}
Updates

Appeal created

m3dython Lead Judge 2 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Missing check for game existence in timeoutJoin

timeoutJoin function allows cancellation and emits a GameCancelled event for any game ID

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.