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

Reentrancy Risk Due to External Token Transfer Before State Change in createGameWithToken and joinGameWithToken

ddeenny

Summary

The RockPaperScissors::createGameWithToken and RockPaperScissors::joinGameWithToken functions call the external WinningToken.transferFrom() before updating internal state variables such as gameCounter, game.playerA, or game.playerB. This breaks the checks-effects-interactions pattern and opens up a reentrancy vector, especially since WinningToken is a custom contract controlled by the same developers. While this does not currently lead to direct loss of funds or tokens, it introduces unnecessary risk, such as inconsistent game state or malicious recursive calls, especially if the token's behavior changes in the future.

Vulnerability Details

// createGameWithToken
​
// @audit-issue External call before internal state mutation — potential reentrancy if token is malicious
@> winningToken.transferFrom(msg.sender, address(this), 1);
​
// State mutation happens after external interaction
@> uint256 gameId = gameCounter++;
@> game.playerA = msg.sender;
@> game.state = GameState.Created;
​
​
// joinGameWithToken
​
// @audit-issue External call before internal state mutation — reentrancy risk via token fallback or hook
@> winningToken.transferFrom(msg.sender, address(this), 1);
​
// State mutation happens after external interaction
@> game.playerB = msg.sender;

Issue Explanation

  1. External Call Before State Mutation
    Calling transferFrom before updating internal state allows a malicious token contract to call back into the RockPaperScissors contract before it's fully initialized.

  2. Missing Reentrancy Guard
    Neither function has a nonReentrant modifier or any other reentrancy lock. If the token calls createGameWithToken again during transferFrom, it can cause unintended game creation.

  3. Custom Token Surface
    The WinningToken contract is deployed and owned by this system. If future upgrades add hooks or callbacks (like ERC777-style behavior), the reentrancy path could become exploitable.

  4. Inconsistent State Risk
    Reentrant calls during incomplete execution may create multiple partially-initialized games or pollute the games mapping with corrupted data.

Impact

  • Inconsistent State: Reentrant transferFrom calls could lead to duplicated or invalid game entries.

  • Game Counter Pollution: gameCounter++ may be called multiple times unintentionally.

  • Unexpected Behavior: Hard-to-detect bugs in downstream logic, game listings, or refunds may emerge.

  • Future-Exploit Vector: If WinningToken ever adds hooks or changes behavior, this becomes a critical security risk.

Tools Used

  • Aderyn

Recommendations

Follow the checks-effects-interactions pattern strictly:

Fix for createGameWithToken

// Safely update internal state first
uint256 gameId = gameCounter++;
Game storage game = games[gameId];
game.playerA = msg.sender;
game.bet = 0;
game.timeoutInterval = _timeoutInterval;
game.creationTime = block.timestamp;
game.joinDeadline = block.timestamp + joinTimeout;
game.totalTurns = _totalTurns;
game.currentTurn = 1;
game.state = GameState.Created;
​
// External call moved after internal state mutation
require(winningToken.transferFrom(msg.sender, address(this), 1), "Token transfer failed");
​
emit GameCreated(gameId, msg.sender, 0, _totalTurns);

Fix for joinGameWithToken

// External call moved after state change
require(winningToken.transferFrom(msg.sender, address(this), 1), "Token transfer failed");
​
// Safe to update state after external interaction
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);

Optional:

  • Add nonReentrant modifier to both functions for future-proofing.

  • Document that WinningToken must not contain callbacks into the game contract.

Even though this issue does not currently result in an exploit, proactively restructuring these functions improves safety, maintainability, and audit readiness.

Updates

Appeal created

m3dython Lead Judge 2 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
m3dython Lead Judge 2 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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