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

WinningToken Accumulation in the RockPaperScissors Due to Incorrect transferFrom Usage

0x_martonyx

Summary

The WinningToken contract inherits the ERC20Burnable contract from Openzeppelin contracts but it never implemented it. And according to the structure of the game contract, it mints new tokens to winners at the end of every Game and then the previous tokens (sent by users) are left in the contract, never burned, refunded, nor reused.

Vulnerability Details

The createGameWithToken and the joinGameWithToken functions of the RockPaperScissors contract transfers (instead of burning) the required winningToken from players to the game contract. This leads to:

  • Accumulation of tokens in the contract with no recovery mechanism.

here's the bug

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++;
​
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;
}

Proof of Concept

  1. Alice creates game (transfers 1 token to contract)

  2. Bob joins game (transfers 1 token to contract)

  3. Game concludes (mints 2 new tokens to winner)

  4. Result:

    • Net supply change: +2 tokens

    • Contract balance grows indefinitely

Impact

Tokens are permanently locked in the contract, reducing access to token supply and potentially disrupting game economics.

Tools Used

Manual Review

Recommendations

Change the transferFrom command to burnFrom in both the createGameWithToken and the joinGameWithToken functions of the RockPaperScissorscontract

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");
​
+ // Burn tokens from the sender
+ winningToken.burnFrom(msg.sender, 1);
- winningToken.transferFrom(msg.sender, address(this), 1);
​
uint256 gameId = gameCounter++;
​
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;
}

then remove this line from the test file

function testCreateGameWithToken() public {
vm.startPrank(playerA);
​
// Approve token transfer
token.approve(address(game), 1);
​
// Create a game with token
vm.expectEmit(true, true, false, true);
emit GameCreated(0, playerA, 0, TOTAL_TURNS);
​
gameId = game.createGameWithToken(TOTAL_TURNS, TIMEOUT);
vm.stopPrank();
​
// Verify token transfer
assertEq(token.balanceOf(playerA), 9);
@> // assertEq(token.balanceOf(address(game)), 1);
​
// Verify game details
(address storedPlayerA,,,,,,,,,,,,,,, RockPaperScissors.GameState state) = game.games(gameId);
​
assertEq(storedPlayerA, playerA);
assertEq(uint256(state), uint256(RockPaperScissors.GameState.Created));
}
Updates

Appeal created

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

Minting Instead of Transferring Staked Tokens

Mints new tokens upon game completion or cancellation for token-based games

Support

FAQs

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