Summary
The RockPaperScissors::createGameWithToken function is vulnerable to a reentrancy attack due to improper ordering of external calls and state updates*(not adhering to CEI pattern)*. Specifically, it calls winningToken.transferFrom(...) — an external call — before updating state variable gameId.
Vulnerability Details
If winningToken is a malicious contract, it could reenter RockPaperScissors::createGameWithToken or other sensitive functions during the external call.
function createGameWithToken(uint256 _totalTurns, uint256 _timeoutInterval) external returns (uint256) {
require(winningToken.balanceOf(msg.sender) >= 1, "Must have winning token");
require(_totalTurns > 0, "Must have at least one turn");
require(_totalTurns % 2 == 1, "Total turns must be odd");
require(_timeoutInterval >= 5 minutes, "Timeout must be at least 5 minutes");
@> winningToken.transferFrom(msg.sender, address(this), 1);
@> 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;
emit GameCreated(gameId, msg.sender, 0, _totalTurns);
return gameId;
}
Impact
An attacker could perform Reentrancy attacks, causing duplicated or corrupted game entries
Tools Used
Manual Review
Slither
Recommendations
Adhere to CEI(Checks Effects Interaction) pattern.
function createGameWithToken(uint256 _totalTurns, uint256 _timeoutInterval) external returns (uint256) {
require(winningToken.balanceOf(msg.sender) >= 1, "Must have winning token");
require(_totalTurns > 0, "Must have at least one turn");
require(_totalTurns % 2 == 1, "Total turns must be odd");
require(_timeoutInterval >= 5 minutes, "Timeout must be at least 5 minutes");
// Transfer token to contract
- winningToken.transferFrom(msg.sender, address(this), 1);
+ uint256 gameId = gameCounter++;
- uint256 gameId = gameCounter++;
+ winningToken.transferFrom(msg.sender, address(this), 1);
Game storage game = games[gameId];
game.playerA = msg.sender;
game.bet = 0; // Zero ether bet because using token
game.timeoutInterval = _timeoutInterval;
game.creationTime = block.timestamp;
game.joinDeadline = block.timestamp + joinTimeout;
game.totalTurns = _totalTurns;
game.currentTurn = 1;
game.state = GameState.Created;
emit GameCreated(gameId, msg.sender, 0, _totalTurns);
return gameId;
}
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 createGameWithToken(uint256 _totalTurns, uint256 _timeoutInterval) external returns (uint256) {
+ function createGameWithToken(uint256 _totalTurns, uint256 _timeoutInterval) external nonReentrant returns (uint256) {
require(winningToken.balanceOf(msg.sender) >= 1, "Must have winning token");
require(_totalTurns > 0, "Must have at least one turn");
require(_totalTurns % 2 == 1, "Total turns must be odd");
require(_timeoutInterval >= 5 minutes, "Timeout must be at least 5 minutes");
// Transfer token to contract
- winningToken.transferFrom(msg.sender, address(this), 1);
+ uint256 gameId = gameCounter++;
- uint256 gameId = gameCounter++;
+ winningToken.transferFrom(msg.sender, address(this), 1);
Game storage game = games[gameId];
game.playerA = msg.sender;
game.bet = 0; // Zero ether bet because using token
game.timeoutInterval = _timeoutInterval;
game.creationTime = block.timestamp;
game.joinDeadline = block.timestamp + joinTimeout;
game.totalTurns = _totalTurns;
game.currentTurn = 1;
game.state = GameState.Created;
emit GameCreated(gameId, msg.sender, 0, _totalTurns);
return gameId;
}
}
