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

The `TwentyOne::call` function calls the `TwentyOne::endGame` function which transfers ether directly if a player wins, this would cause a player to lose their win if `endGame` reverts.

uddercover

Summary

The `call` function calculates the dealer's hand, then compares it to the player's to determine the winner. After getting the winner, the function calls `endGame` which resets the state variables and transfers ether to the player if they win. If this transfer fails, the entire transaction would revert including the player's win, causing them to miss out on their payout.

Vulnerability Details

Proof of Concept:

  1. A player starts a game and gets their cards.

  2. They choose to call immediately. They are lucky and win.

  3. Due to unknown circumstances the transfer of their payout fails.

  4. The entire transaction reverts, including their win.

  5. The player has to call again and bear the risk of losing.

Proof of code

Place the below code into TwentyOneTest in TwentyOne.t.sol

function testPlayerLosesWinIfEndGameReverts() public {
uint256 playerBalanceBeforeGame = player1.balance;
//Start game as player1
vm.startPrank(player1);
uint256 playersStartingHand = twentyOne.startGame{value: 1 ether}();
​
//The player wins, endGame is called but fails
vm.expectRevert();
twentyOne.call();
​
//After the revert, the players hand remains the same, the win is reverted but the dealers hand will change because of how the randomness is computed
uint256 playersHandAfterRevert = twentyOne.playersHand(player1);
console.log("Player's starting hand: ", playersStartingHand);
console.log("Player's hand after revert", playersHandAfterRevert);
assertEq(playersStartingHand, playersHandAfterRevert);
​
//New block.timestamp
vm.warp(2);
​
//This time, the player loses
twentyOne.call();
vm.stopPrank();
​
uint256 playerBalanceAfterGame = player1.balance;
assertGt(playerBalanceBeforeGame, playerBalanceAfterGame);
​
}

Impact

Players would lose their win if endGame reverts, causing them to have to risk losing again by re-calling call.

Tools Used

Foundry

Recommendations

Consider using a mapping to track player wins separately and use a different function for players to claim their wins manually.

​
+ mapping(address => uint256) private playerWins;
+ event PlayerWinsUpdated(address player, uint256 amount);
.
.
.
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) {
+ playerWins[player] += 2 ether;
- payable(player).transfer(2 ether); // Transfer the prize to the player
+ emit PlayerWinsUpdated(player, 2 ether);
- emit FeeWithdrawn(player, 2 ether); // Emit the prize withdrawal event
}
}
​
+ function claimWins() public {
+ require(playerWins[msg.sender] > 0);
+ uint256 sendValue = playerWins[msg.sender];
+ playerWins[msg.sender] = 0; // Reset mapping
+ emit FeeWithdrawn(player, sendValue); // Emit the prize withdrawal event
+ payable(msg.sender).call{value: sendValue}(""); // Transfer the prize to the player
+ }
Updates

Lead Judging Commences

inallhonesty Lead Judge 11 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.