Summary

The RockPaperScissors contract creates and initially owns the WinningToken contract through its constructor. Although this ownership relationship is correctly established at deployment, the contract lacks safeguards against ownership transfers and has no error handling for scenarios where token ownership might be inadvertently transferred away. If such a transfer occurs during system upgrades, administrative operations, or security incidents, games would be unable to complete properly, resulting in locked funds and incomplete game states.

Vulnerability Details

When a game completes, the _finishGame function attempts to mint token rewards for the winner:

This token minting operation can only succeed if the game contract is the owner of the token contract. The WinningToken implements the OpenZeppelin Ownable pattern which restricts minting to the owner:

The vulnerability arises because:

1. There's no check to verify that the game contract remains the token owner before attempting to mint

2. No error handling exists if the mint operation fails

3. No mechanism exists to recover from a failed game completion

4. The contract implicitly assumes it always has token minting permissions

Impact

This vulnerability represents a 'low probability, high impact' risk with several serious consequences:

• Game Completion Failure: Games cannot finish properly if token ownership changes

• Permanent Stuck State: Affected games remain perpetually in an unfinished state

• Fund Lockup: Players' bets remain locked in the contract with no way to recover them

• Silent Failure: No clear error reporting indicates the root cause to users or admins

• Game Abandonment: Players have no recourse when games cannot complete

While the probability of token ownership being transferred away in production is relatively low, the severity is high as it could completely disable core functionality of the platform without a clear path to recovery.

Tools Used

Manual code review and Foundry tests

Proof of Concept

The following test demonstrates this vulnerability:

Test Execution Output

Running the test produces the following error, confirming the vulnerability:

The exact error message OwnableUnauthorizedAccount confirms that the game contract loses the ability to mint tokens when ownership is transferred away, leaving games unable to complete.

Recommendations

Implement at least one of the following solutions, with preference for preventative measures:

1. Add Ownership Protection (Preventative):

   • Modify the token contract to prevent ownership transfers away from the game contract

   • Or override the transferOwnership function to include additional safety checks

   // In an updated version of WinningToken.sol

   function transferOwnership(address newOwner) public virtual override onlyOwner {

   require(newOwner == address(gameContract), "Can only transfer ownership to game contract");

   super.transferOwnership(newOwner);

   }

   • Alternatively, implement a timelock mechanism for ownership transfers that allows for recovery

2. Add Error Handling with Recovery (Reactive):

   • Implement try-catch to handle minting failures

   • Add a fallback method to finish games even if token minting fails

   function _finishGame(uint256 _gameId, address _winner) internal {

   // ... existing code ...

   // Check token ownership first to provide better error messages

   if (winningToken.owner() != address(this)) {

   emit TokenMintingError(_gameId, "Game contract is not token owner");

   // Still complete the game, just without token rewards

   game.state = GameState.Finished;

   return;

   }

   try winningToken.mint(_winner, tokensToMint) {

   // Minting successful

   } catch {

   // Log error but still finish the game

   emit TokenMintFailed(_gameId, _winner, tokensToMint);

   }

   // Always update game state regardless of token minting success

   game.state = GameState.Finished;

   // ... remaining code ...

   }

3. Redesign Token Minting Mechanism (Architectural):

   • Consider using an approved minter pattern instead of ownership

   • Implement proper role-based access control for minting operations

   • Decouple game completion from token minting success

   • Add an emergency function to allow admins to finish stuck games:

   function emergencyFinishGame(uint256 _gameId, address _winner) external {

   require(msg.sender == adminAddress, "Only admin can perform emergency operations");

   Game storage game = games[_gameId];

   require(game.state != GameState.Finished && game.state != GameState.Cancelled, "Game already ended");

   // Complete the game without minting tokens

   game.state = GameState.Finished;

   emit GameFinished(_gameId, _winner, game.bet * 2);

   // Transfer ETH prizes

   (bool success,) = _winner.call{value: game.bet * 2}("");

   require(success, "Prize transfer failed");

   }