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

In contract `RockPaperScissors`, There is no function to call to withdraw `winningTokens` from the contract, resulting tokens stuck forever in the contract

geekybot

Summary

In functions _handleTie, _finishgame, _cancelGame of RockPaperScissors contract, tokens are never transferred back to the players, instead new tokens are minted to the players.

Vulnerability Details

This issue leads to the tokens deposited in function createGameWithToken and joinGameWithToken are forever stuck in the contract, as there are no function to withdraw these tokens.

According to the natspec of function _cancelGame the tokens should be refunded but new tokens were minted to players, leaving the actual bet tokens stuck in the contract.

// Return tokens for token games
if (game.bet == 0) {
if (game.playerA != address(0)) {
winningToken.mint(game.playerA, 1);
}
if (game.playerB != address(0)) {
winningToken.mint(game.playerB, 1);
}
}

Proof of Code
Add this block of code in RockPaperScissors.t.sol and test

// Test token refund bug when game is cancelled
function testTokenRefundBug() public {
vm.startPrank(playerA);
​
// Approve token transfer
token.approve(address(game), 1);
​
// Create game with token
gameId = game.createGameWithToken(TOTAL_TURNS, TIMEOUT);
​
// Verify token balances after game creation
assertEq(token.balanceOf(playerA), 9); // PlayerA balance decreased
assertEq(token.balanceOf(address(game)), 1); // Contract received token
uint256 initialSupply = token.totalSupply();
​
// Cancel game
game.cancelGame(gameId);
vm.stopPrank();
​
// Verify game is cancelled
(, , , , , , , , , , , , , , , RockPaperScissors.GameState state) = game
.games(gameId);
assertEq(
uint256(state),
uint256(RockPaperScissors.GameState.Cancelled)
);
​
// Bug: Instead of refunding the token, contract mints new token
assertEq(token.balanceOf(playerA), 10); // PlayerA got new minted token
assertEq(token.balanceOf(address(game)), 1); // Original token stuck in contract
assertEq(token.totalSupply(), initialSupply + 1); // Supply increased due to mint
​
// No function exists to withdraw stuck token from contract
// Admin cannot recover the token
}

Impact

This will inflate the total supply of winningToken along with tokens being stuck at RockPaperScissors contract.

Tools Used

Manual Review, Foundry test suites

Recommendations

Instead of minting new tokens it's recommended to transfer the tokens back to the players

// Return tokens for token games
if (game.bet == 0) {
if (game.playerA != address(0)) {
- winningToken.mint(game.playerA, 1);
+ winningToken.transfer(game.playerA, 1);
}
if (game.playerB != address(0)) {
- winningToken.mint(game.playerB, 1);
+ winningToken.transfer(game.playerB, 1);
}
}
Updates

Appeal created

m3dython Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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