TwentyOne
First Flight #29
Beginner FriendlyGameFiFoundrySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Mishandling of ETH transfer in `TwentyOne::endGame` can disrupt functioning of protocol when player wins and ETH transfer fails

trashpirate

Summary

The function TwentOne::endGame does not handle ETH transfers properly which means in case of a failed ETH transfer the function reverts and the player that wins does not receive a payout as expected.

// Ends the game, resets the state, and pays out if the player won
function endGame(address player, bool playerWon) internal {
delete playersDeck[player].playersCards; // Clear the player's cards
delete dealersDeck[player].dealersCards; // Clear the dealer's cards
delete availableCards[player]; // Reset the deck
if (playerWon) {
@> payable(player).transfer(2 ether); // Transfer the prize to the player
emit FeeWithdrawn(player, 2 ether); // Emit the prize withdrawal event
}
}

Vulnerability Details

The function TwentOne::endGame uses transfer to transfer ETH from the contract to the player. While this choice can prevent reentrancy attacks due to gas limits, this causes a problem when the function reverts. Since the game state is not stored, an ETH transfer that reverts prevents the winning player from collecting their payout. Calling the TwentOne::call again, will make the dealer to redraw the cards which leads to a new game result while the previous game state is lost.

Proof of Concept

  1. Player starts game with 1 ETH

  2. Player calls TwentyOne::call

  3. Player wins the game

  4. Function TwentyOne::call calls TwentyOne::endGame to pay out the player

  5. Function TwentyOne::endGame fails and reverts because of failed ETH transfer

  6. No payout for winner, new game can't be started until TwentyOne::endGame is successfully called

  7. Player calls TwentyOne::call but now looses

  8. Player never received payout for win

Code:

Place following code into TwentyOne.t.sol:

```solidity
function test_CallReverts_FailedEthTransfer() public {
// fund contract
vm.deal(address(twentyOne), 10 ether);
​
// start game
vm.prank(player1);
twentyOne.startGame{value: 1 ether}();
​
// playersHand == 14
console.log("Player's hand: ", twentyOne.playersHand(player1));
// player's starting balance
uint256 playerBalance = player1.balance;
​
// set randomization paramters on chain to make player win (dealer bust)
vm.prevrandao(1);
vm.roll(1);
​
// mocking revert for failed eth transfer
vm.mockCallRevert(player1, 2 ether, "", "");
​
// expecting reverts without data because [OutOfFunds]
vm.expectRevert(bytes(""));
​
// Player calls to compare hands
vm.prank(player1);
twentyOne.call();
​
// expecting revert when restarting the game
vm.expectRevert(bytes("Player's deck is already initialized"));
​
// player tries to restart the game
vm.prank(player1);
twentyOne.startGame{value: 1 ether}();
​
// time passes, this time dealer wins and player looses
vm.prevrandao(8);
vm.roll(1);
​
// player calls call again - looses the game
vm.prank(player1);
twentyOne.call();
​
// player did not win any ETH
assertEq(playerBalance, player1.balance);
}
```

Impact

This is medium impact as failed ETH transfers likely stem from smart contracts (eg. that are missing a fallback function or have a fallback function with high gas consumption) playing the game which one can assume for this type of protocol is fairly rare. However, when it does occur it does break critical functionality for the impacted player. Because when a player wins and the ETH transfer of the payout to the player fails, the player may cannot receive their payout unless they are lucky and calling the TwentOne::call function again results again in a win and the ETH transfer succeeds.

Tools Used

Foundry, manual review, custom test

Recommendations

To ensure smooth functioning of the protocol in any scenario, the best way to handle it is storing the owed payout for each winner which they can withdraw later to a specified address instead of paying out players directly in the TwentOne::endGame function. This way, if issues occur sending the ETH to a specific address, the funds can be withdrawn by the player to an alternative address at a later point. This also solves the issue that the player can't restart a new game until the previous game is successfully ended (i.e. funds received).

For example the following mapping and function could be added:

mapping (address => uint256) public owedPayouts;
​
function withdrawPayouts(address recipient) public {
uint256 payout = owedPayouts[msg.sender];
require(payout > 0, "No payout available");
owedPayouts[msg.sender] = 0;
payable(recipient).transfer(payout);
emit FeeWithdrawn(player, payout);
}

This would require the following change in the TwentOne::endGame function:

function endGame(address player, bool playerWon) internal {
delete playersDeck[player].playersCards; // Clear the player's cards
delete dealersDeck[player].dealersCards; // Clear the dealer's cards
delete availableCards[player]; // Reset the deck
if (playerWon) {
+ owedPayouts[player] += 2 ether; // Store the payout for the player
+ emit PlayerWins(player); // Emit player wins event
- payable(player).transfer(2 ether); // Transfer the prize to the player
- emit FeeWithdrawn(player, 2 ether); // Emit the prize withdrawal event
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Revert a bad outcome

Support

FAQs

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