Summary
Incomplete and incorrect game state management, absence of Reveal state
Vulnerability Details
The game state management in contract RockPaperScissors lacks of GameState.Revealed state usage, leading to inccorect handling of entire reveal phase and mixing it with commit phase. Hereby protocol behavior becomes unstable, opening the door for a critical vulnerabilities all over the game contract implementation. Affected are almost all ot the core functions in the smart contract:
joinGameWithEth
joinGameWithToken
commitMove
revealMove
timeoutReveal
canTimeoutReveal
Impact
mixing commit and reveal phase
incorrect or missing game state transitions (both commit and reveal state)
incorrect and incompleted function validations
incorrect or irrelevant error messages
Tools Used
Manual review
Foundry
Recommendations
Implement game state management using separate reveal phase and avoid mixing it with the commit phase. Review and update all state transitions, introducing the missed state GameState.Revealed, which will make the entire contract logic more robust, consistend and will make it less error prone, closing the door for most of the critical vulnerabilites available now.
function joinGameWithEth(uint256 _gameId) external payable {
// ...
game.playerB = msg.sender;
+ game.state = GameState.Committed;
emit PlayerJoined(_gameId, msg.sender);
}
function joinGameWithToken(uint256 _gameId) external {
// ...
game.playerB = msg.sender;
+ game.state = GameState.Committed;
emit PlayerJoined(_gameId, msg.sender);
}
function commitMove(uint256 _gameId, bytes32 _commitHash) 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.Created || game.state == GameState.Committed, "Game not in commit phase");
+ require(game.state == GameState.Committed, "Game not in commit phase");
+ // most of the following code could be removed, since state transition to state Committed is moved in correcponding joinGameWith*** functions
- if (game.currentTurn == 1 && game.commitA == bytes32(0) && game.commitB == bytes32(0)) {
- // First turn, first commits
- require(game.playerB != address(0), "Waiting for player B to join");
- game.state = GameState.Committed;
- } else {
- // Later turns or second player committing
- require(game.state == GameState.Committed, "Not in commit phase");
- require(game.moveA == Move.None && game.moveB == Move.None, "Moves already committed for this turn");
- }
// ...
emit MoveCommitted(_gameId, msg.sender, game.currentTurn);
// If both players have committed, set the reveal deadline
if (game.commitA != bytes32(0) && game.commitB != bytes32(0)) {
game.revealDeadline = block.timestamp + game.timeoutInterval;
+ game.state = GameState.Revealed;
}
}
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(game.state == GameState.Revealed, "Game not in reveal phase");
require(block.timestamp <= game.revealDeadline, "Reveal phase timed out");
require(_move >= 1 && _move <= 3, "Invalid move");
// ...
}
function timeoutReveal(uint256 _gameId) 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(game.state == GameState.Revealed, "Game not in reveal phase");
require(block.timestamp > game.revealDeadline, "Reveal phase not timed out yet");
// ...
}
function canTimeoutReveal(uint256 _gameId) external view returns (bool canTimeout, address winnerIfTimeout) {
Game storage game = games[_gameId];
- if (game.state != GameState.Committed || block.timestamp <= game.revealDeadline) {
+ if (game.state != GameState.Revealed || block.timestamp <= game.revealDeadline) {
return (false, address(0));
}
// ...
}
