Summary
TheRockPaperScissors::joinGameWithToken function is vulnerable to a reentrancy attack due to improper ordering of external calls and state updates*(not adhereing to CEI pattern)*. Specifically, it calls winningToken.transferFrom(...) — an external call — before updating critical state variables such as game.playerB.
Vulnerability Details
If winningToken is a malicious contract, it could reenter joinGameWithToken or other sensitive functions during the external call.
function joinGameWithToken(uint256 _gameId) external {
Game storage game = games[_gameId];
require(game.state == GameState.Created, "Game not open to join");
require(game.playerA != msg.sender, "Cannot join your own game");
require(block.timestamp <= game.joinDeadline, "Join deadline passed");
require(game.bet == 0, "This game requires ETH bet");
require(winningToken.balanceOf(msg.sender) >= 1, "Must have winning token");
@> winningToken.transferFrom(msg.sender, address(this), 1);
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);
}
Impact
An attacker could:
Join the same or multiple games recursively.
Overwrite game.playerB of different games.
Potentially exploit other game mechanics if combined with additional logic inside the reentered functions.
Even if financial damage isn't immediate, this can compromise the integrity of the game logic, allow unfair advantages, or freeze games in unusable states.
Tools Used
Manual Review
Slither
Recommendations
Adhere to CEI(Checks Effects Interaction) pattern.
function joinGameWithToken(uint256 _gameId) external {
Game storage game = games[_gameId];
require(game.state == GameState.Created, "Game not open to join");
require(game.playerA != msg.sender, "Cannot join your own game");
require(block.timestamp <= game.joinDeadline, "Join deadline passed");
require(game.bet == 0, "This game requires ETH bet");
require(winningToken.balanceOf(msg.sender) >= 1, "Must have winning token");
// Transfer token to contract
- winningToken.transferFrom(msg.sender, address(this), 1);
+ game.playerB = msg.sender;
- game.playerB = msg.sender;
+ winningToken.transferFrom(msg.sender, address(this), 1);
emit PlayerJoined(_gameId, msg.sender);
}
2 The `ReentrancyGuard`'s modifier `nonReentrant` has a status check under the hood that is set to `2` when a function is entered/being executed and `1` when not. This lock mechanism prevents reentrancy. For more information, see the source code [here](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/21c8312b022f495ebe3621d5daeed20552b43ff9/contracts/utils/ReentrancyGuard.sol#L37)
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
import "./WinningToken.sol";
/**
* @title Rock Paper Scissors Game
* @notice A fair implementation of Rock Paper Scissors on Ethereum
* @dev Players commit hashed moves, then reveal them to determine the winner
*/
-contract RockPaperScissors {
+contract RockPaperScissors is ReentrancyGuard {
// more code
- function joinGameWithToken(uint256 _gameId) external {
+ function joinGameWithToken(uint256 _gameId) external nonReentrant {
Game storage game = games[_gameId];
require(game.state == GameState.Created, "Game not open to join");
require(game.playerA != msg.sender, "Cannot join your own game");
require(block.timestamp <= game.joinDeadline, "Join deadline passed");
require(game.bet == 0, "This game requires ETH bet");
require(winningToken.balanceOf(msg.sender) >= 1, "Must have winning token");
// Transfer token to contract
- winningToken.transferFrom(msg.sender, address(this), 1);
+ game.playerB = msg.sender;
- game.playerB = msg.sender;
+ winningToken.transferFrom(msg.sender, address(this), 1);
emit PlayerJoined(_gameId, msg.sender);
}
}
