Summary
The RockPaperScissors contract has a poorly documented recovery mechanism for games abandoned by one player. While timeoutJoin cannot be used when a second player has joined but becomes inactive, a functional but obscure recovery path exists through the timeoutReveal function.
Vulnerability Details
The timeoutJoin function is designed to handle timeouts when no player joins a game, but it explicitly prevents recovery when a second player has already joined:
function timeoutJoin(uint256 _gameId) external {
Game storage game = games[_gameId];
require(game.state == GameState.Created, "Game must be in created state");
require(block.timestamp > game.joinDeadline, "Join deadline not reached yet");
require(game.playerB == address(0), "Someone has already joined the game");
_cancelGame(_gameId);
}
However, a recovery path exists through the timeoutReveal function through this sequence:
Player A creates a game
Player B joins the game but becomes inactive
Player A commits a move, changing the game state to GameState.Committed
Since Player B never commits, game.revealDeadline remains at its default value of 0
Player A calls timeoutReveal() which:
Passes the check game.state == GameState.Committed
Passes the check block.timestamp > game.revealDeadline (since revealDeadline is 0)
Enters the condition (!playerARevealed && !playerBRevealed) since nobody revealed
Calls _cancelGame(_gameId) which refunds both players
This recovery path works but is unintuitive and undocumented, creating a usability issue for players.
Impact
Low - Funds are not permanently at risk as there is a recovery mechanism available. However, the mechanism is unintuitive and undocumented, which may lead to confusion and unnecessary stress for users who believe their funds are locked.
Likelihood
High - It's highly probable that players will occasionally abandon games due to lost keys, disinterest, or other reasons. Given the popularity of blockchain games, player abandonment is an expected regular occurrence.
According to the CodeHawks risk matrix, this vulnerability has a Medium severity rating (Low impact + High likelihood = Medium).
Proof of Concept
pragma solidity ^0.8.13;
import "forge-std/Test.sol";
import "../src/RockPaperScissors.sol";
import "../src/WinningToken.sol";
contract RockPaperScissorsVulnTest is Test {
RockPaperScissors public game;
WinningToken public token;
address public admin;
address public playerA;
address public playerB;
address public playerC;
uint256 constant BET_AMOUNT = 0.1 ether;
uint256 constant TIMEOUT = 10 minutes;
uint256 constant TOTAL_TURNS = 1;
function setUp() public {
admin = address(this);
playerA = makeAddr("playerA");
playerB = makeAddr("playerB");
playerC = makeAddr("playerC");
vm.deal(playerA, 10 ether);
vm.deal(playerB, 10 ether);
vm.deal(playerC, 10 ether);
game = new RockPaperScissors();
token = WinningToken(game.winningToken());
vm.prank(address(game));
token.mint(playerA, 10);
vm.prank(address(game));
token.mint(playerB, 10);
vm.prank(address(game));
token.mint(playerC, 10);
}
function testPlayerCommitsAndTimeoutRecovery() public {
uint256 playerAInitialBalance = playerA.balance;
uint256 playerBInitialBalance = playerB.balance;
vm.prank(playerA);
uint256 gameId = game.createGameWithEth{value: BET_AMOUNT}(TOTAL_TURNS, TIMEOUT);
vm.prank(playerB);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
bytes32 saltA = keccak256(abi.encodePacked("salt for player A"));
bytes32 commitA = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Rock), saltA));
vm.prank(playerA);
game.commitMove(gameId, commitA);
vm.prank(playerA);
game.timeoutReveal(gameId);
(,,,,,,,,,,,,,,,RockPaperScissors.GameState state) = game.games(gameId);
assertEq(uint256(state), uint256(RockPaperScissors.GameState.Cancelled));
assertEq(playerA.balance, playerAInitialBalance);
assertEq(playerB.balance, playerBInitialBalance);
}
}
Tools Used
Manual code review and custom unit tests.
Recommendations
Improve the contract's usability and predictability by:
Adding a dedicated, clearly-named function for abandoned games:
function timeoutAbandonedGame(uint256 _gameId) external {
Game storage game = games[_gameId];
require(game.playerB != address(0), "No player has joined yet");
if (game.state == GameState.Created && block.timestamp > game.joinDeadline + 1 hours) {
_cancelGame(_gameId);
return;
}
if (game.state == GameState.Committed) {
bool onlyOneCommit = (game.commitA != bytes32(0) && game.commitB == bytes32(0)) ||
(game.commitA == bytes32(0) && game.commitB != bytes32(0));
if (onlyOneCommit && block.timestamp > game.joinDeadline + 1 hours) {
_cancelGame(_gameId);
return;
}
}
revert("Game not eligible for inactive timeout");
}
-
Clearly document all recovery mechanisms in comments and user documentation, including the existing path through timeoutReveal.
-
Add helper functions that make the current state of a game more transparent to users:
function isGameAbandoned(uint256 _gameId) external view returns (bool) {
Game storage game = games[_gameId];
}
