Description:
In the RockPaperScissors::_handleTie() function, the contract attempts to refund both players when a game ends in a draw. It sends ETH to both players via two separate call{value: ...} instructions. However, if either call fails (e.g., if the address is a contract without a receive() or fallback() function), the transaction reverts, and no one receives a refund.
This refund pattern is reused in _cancelGame(), which is called when games are cancelled due to timeout or inactivity. If one of the player refunds fails there, the same issue occurs — all ETH remains stuck in the contract.
(bool successA,) = game.playerA.call{value: refundPerPlayer}("");
(bool successB,) = game.playerB.call{value: refundPerPlayer}("");
require(successA && successB, "Transfer failed");
(bool successA,) = game.playerA.call{value: game.bet}("");
require(successA, "Transfer to player A failed");
if (game.playerB != address(0)) {
(bool successB,) = game.playerB.call{value: game.bet}("");
require(successB, "Transfer to player B failed");
}
Impact
ETH remains stuck in the contract if just one refund fails.
No fallback or rescue mechanism exists.
Players may permanently lose their ETH.
Especially problematic when a player is a contract without a payable fallback, or due to temporary gas issues.
Proof Of Cooncept:
(for _handleTie()):
PlayerA creates a game with ETH.
A contract playerBContract (without receive() or fallback()) joins the game.
Players commit and reveal normally.
The game ends in a tie.
Neither player receives a refund.
function test_error_Refund_ETH_Draw() public {
vm.prank(playerA);
uint256 gameId = game.createGameWithEth{value: 1e18}(3, 5 minutes);
vm.prank(address(playerBContract));
playerBContract.joinGame(gameId);
uint256 playerABalance_Before = address(playerA).balance;
uint256 playerBContratBalance_Before = address(playerBContract).balance;
fisrtTurn(gameId);
secondTurn(gameId);
thirdTurn(gameId);
(
,,,,,,,,
uint256 currentTurn,
,,,,
uint8 scoreA,
uint8 scoreB,
)
= game.games(gameId);
uint256 playerABalance_After = address(playerA).balance;
uint256 playerBContratBalance_After = address(playerBContract).balance;
console2.log("scoreA", scoreA);
console2.log("scoreB", scoreB);
console2.log("currentTurn", currentTurn);
console2.log("-------------------------------");
console2.log("PlayerA Balance Before...", playerABalance_Before);
console2.log("PlayerBContract Balance..", playerBContratBalance_Before);
console2.log("-------------------------------");
console2.log("PlayerA Balance After....", playerABalance_After);
console2.log("PlayerBContract After....", playerBContratBalance_After);
}
Logs:
scoreA 1
scoreB 1
currentTurn 3
-------------------------------
PlayerA Balance Before... 0
PlayerBContract Balance.. 0
-------------------------------
PlayerA Balance After.... 0
PlayerBContract After.... 0
Result:
│ └─ ← [Revert] revert: Transfer failed
This same refund logic (and potential failure) also applies to _cancelGame() when refunding both players.
Tools Used
Manual Review and Foundry
Recommendations
Replace the combined require(successA && successB) pattern with individual refund attempts and RefundFailed/RefundSuccessful events to ensure graceful degradation:
_handleTie():
function _handleTie(...) {
...
if (game.bet > 0) {
// Calculate protocol fee (10% of total pot)
uint256 totalPot = game.bet * 2;
uint256 fee = (totalPot * PROTOCOL_FEE_PERCENT) / 100;
uint256 refundPerPlayer = (totalPot - fee) / 2;
// Accumulate fees for admin
accumulatedFees += fee;
emit FeeCollected(_gameId, fee);
// Refund both players
- (bool successA,) = game.playerA.call{value: refundPerPlayer}("");
- (bool successB,) = game.playerB.call{value: refundPerPlayer}("");
- require(successA && successB, "Transfer failed");
+ event RefundFailed(address indexed player, uint256 refundAmount);
+ event RefundSuccessful(address indexed player, uint256 refundAmount);
// Try to refund playerA
+ (bool successA, ) = game.playerA.call{value: refundPerPlayer}("");
+ if (!successA) {
+ emit RefundFailed(game.playerA, refundPerPlayer);
+ } else {
+ emit RefundSuccessful(game.playerA, refundPerPlayer);
+ }
// Try to refund playerB
+ (bool successB, ) = game.playerB.call{value: refundPerPlayer}("");
+ if (!successB) {
+ emit RefundFailed(game.playerB, refundPerPlayer);
+ } else {
+ emit RefundSuccessful(game.playerB, refundPerPlayer);
+ }
}
...
}
function _cancelGame(...) {
...
if (game.bet > 0) {
- (bool successA,) = game.playerA.call{value: game.bet}("");
- require(successA, "Transfer to player A failed");
- if (game.playerB != address(0)) {
- (bool successB,) = game.playerB.call{value: game.bet}("");
- require(successB, "Transfer to player B failed");
- }
+ event RefundFailed(address indexed player, uint256 refundAmount);
+ event RefundSuccessful(address indexed player, uint256 refundAmount);
// Refund both players individually
+ (bool successA, ) = game.playerA.call{value: game.bet}("");
+ if (!successA) {
+ emit RefundFailed(game.playerA, game.bet);
+ } else {
+ emit RefundSuccessful(game.playerA, game.bet);
+ }
+ (bool successB, ) = game.playerB.call{value: game.bet}("");
+ if (!successB) {
+ emit RefundFailed(game.playerB, game.bet);
+ } else {
+ emit RefundSuccessful(game.playerB, game.bet);
+ }
}
...
}
Additionally, consider implementing a claimRefund() function that lets players manually withdraw their share if the automatic transfer fails.
