Summary
The RockPaperScissor::createGameWithEth function lacks an upper bound for the uint256 _timeoutInterval parameter, allowing arbitrarily large values (e.g., 10 years). This could lock games because the RockPaperScissor::Game::revealDeadline may become unreachable, preventing players from resolving the game.
Vulnerability Details
function createGameWithEth(uint256 _totalTurns, uint256 _timeoutInterval) external payable returns (uint256) {
require(msg.value >= minBet, "Bet amount too small");
require(_totalTurns > 0, "Must have at least one turn");
require(_totalTurns % 2 == 1, "Total turns must be odd");
require(_timeoutInterval >= 5 minutes, "Timeout must be at least 5 minutes");
function commitMove(uint256 _gameId, bytes32 _commitHash) external {
if (game.commitA != bytes32(0) && game.commitB != bytes32(0)) {
game.revealDeadline = block.timestamp + game.timeoutInterval;
}
}
The createGameWithEth function in the RockPaperScissor contract accepts a _timeoutInterval parameter to set the RockPaperScissor::Game::timeoutInterval variable, which determines the time allowed to play a game. This variable is later used to set the RockPaperScissor::Game::revealDeadline, which specifies the deadline for players to reveal their moves. The revealDeadline is set in the RockPaperScissor::commitMove function when both players have committed their moves.
The createGameWithEth function enforces a minimum _timeoutInterval of 5 minutes (_timeoutInterval >= 5 minutes) but does not impose an upper bound. This allows users to specify excessively large values for _timeoutInterval (e.g., 315,360,000 seconds or 10 years). When timeoutInterval is used to set revealDeadline in commitMove, an excessively large timeoutInterval results in a revealDeadline far in the future, making it practically unreachable.
Relevant GitHub Links
Impact
Locked Games: A revealDeadline set far in the future prevents players from revealing moves and resolving the game, locking any staked ETH (e.g., msg.value) until the deadline passes.
User Experience Issues: Accidental large inputs by users could render games unplayable, reducing trust in the contract.
uint256 constant TIMEOUT = 10 * 365 days;
function testCommitMoves() public {
gameId = createAndJoinGame();
bytes32 saltA = keccak256(abi.encodePacked("salt for player A"));
bytes32 commitA = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Rock), saltA));
vm.prank(playerA);
vm.expectEmit(true, true, false, true);
emit MoveCommitted(gameId, playerA, 1);
game.commitMove(gameId, commitA);
bytes32 saltB = keccak256(abi.encodePacked("salt for player B"));
bytes32 commitB = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Paper), saltB));
vm.prank(playerB);
vm.expectEmit(true, true, false, true);
emit MoveCommitted(gameId, playerB, 1);
game.commitMove(gameId, commitB);
(,,,uint256 timeoutInterval,uint256 revealDeadline,,,,, bytes32 storedCommitA, bytes32 storedCommitB,,,,, RockPaperScissors.GameState state) =
game.games(gameId);
console.log("Timeout interval: ", timeoutInterval);
console.log("Reveal deadline: ", revealDeadline);
assertEq(storedCommitA, commitA);
assertEq(storedCommitB, commitB);
assertEq(uint256(state), uint256(RockPaperScissors.GameState.Committed));
}
Before add an upper bound check
forge test --match-test testCommitMoves -vvv 1 ↵
[⠊] Compiling...
[⠑] Compiling 2 files with Solc 0.8.20
[⠘] Solc 0.8.20 finished in 49.72s
Compiler run successful with warnings:
Warning (2018): Function state mutability can be restricted to view
--> test/RockPaperScissorsTest.t.sol:866:5:
|
866 | function testOwnershipFunctions() public {
| ^ (Relevant source part starts here and spans across multiple lines).
Ran 1 test for test/RockPaperScissorsTest.t.sol:RockPaperScissorsTest
[PASS] testCommitMoves() (gas: 349436)
Logs:
Timeout interval: 315360000
Reveal deadline: 315360001
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.09ms (3.34ms CPU time)
After
forge test --match-test testCommitMoves -vvv
[⠊] Compiling...
[⠰] Compiling 2 files with Solc 0.8.20
[⠔] Solc 0.8.20 finished in 39.37s
Compiler run successful with warnings:
Warning (2018): Function state mutability can be restricted to view
--> test/RockPaperScissorsTest.t.sol:866:5:
|
866 | function testOwnershipFunctions() public {
| ^ (Relevant source part starts here and spans across multiple lines).
Ran 1 test for test/RockPaperScissorsTest.t.sol:RockPaperScissorsTest
[FAIL: revert: Timeout must be at least 5 minutes and at most 7 days] testCommitMoves() (gas: 18581)
Traces:
[18581] RockPaperScissorsTest::testCommitMoves()
├─ [0] VM::prank(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF])
│ └─ ← [Return]
├─ [954] RockPaperScissors::createGameWithEth{value: 100000000000000000}(3, 315360000 [3.153e8])
│ └─ ← [Revert] revert: Timeout must be at least 5 minutes and at most 7 days
└─ ← [Revert] revert: Timeout must be at least 5 minutes and at most 7 days
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 39.68ms (2.35ms CPU time)
Ran 1 test suite in 321.99ms (39.68ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/RockPaperScissorsTest.t.sol:RockPaperScissorsTest
[FAIL: revert: Timeout must be at least 5 minutes and at most 7 days] testCommitMoves() (gas: 18581)
Relevant Github links
Tools Used
Recommendations
To prevent arbitrarily large _timeoutInterval values, add an upper bound check to the createGameWithEth function. A reasonable maximum, such as 7 days (604,800 seconds), balances flexibility with practicality, ensuring games can be resolved within a manageable timeframe.
- require(_timeoutInterval >= 5 minutes, "Timeout must be at least 5 minutes");
+ require(_timeoutInterval >= 5 minutes && _timeoutInterval <= 7 days, "Timeout must be between 5 minutes and 7 days");
