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

Failure in Automated Prize Transfer Leads to Potential Loss of Player Funds

pollersan

Summary

To determine the winner, the user has to call the call() function, which reveals the dealer's cards. If the user wins, the contract attempts to transfer the prize using this logic:

if (playerWon) {
payable(player).transfer(2 ether); // Transfer the prize to the player @audit- better use call
emit FeeWithdrawn(player, 2 ether); // Emit the prize withdrawal event
}

However, if the transfer fails for any reason, the transaction is reverted. This means that the user would have to call call() again to try to claim their prize. The problem is, this would lead to a different outcome because the dealer's cards could change each time the function is called, which might affect the result.

Vulnerability Details

When the call() function is used to determine the outcome of the game, the endGame() function is called if the player wins, which handles transferring the funds to the user.

function call() public {
require(
playersDeck[msg.sender].playersCards.length > 0,
"Game not started"
);
uint256 playerHand = playersHand(msg.sender);
​
// Calculate the dealer's threshold for stopping (between 17 and 21)
uint256 standThreshold = (uint256(
keccak256(
abi.encodePacked(block.timestamp, msg.sender, block.prevrandao)
)
) % 5) + 17;
​
// Dealer draws cards until their hand reaches or exceeds the threshold
while (dealersHand(msg.sender) < standThreshold) {
uint256 newCard = drawCard(msg.sender);
addCardForDealer(msg.sender, newCard);
}
​
uint256 dealerHand = dealersHand(msg.sender);
​
// Determine the winner
if (dealerHand > 21) {
emit PlayerWonTheGame(
"Dealer went bust, players winning hand: ",
playerHand
);
endGame(msg.sender, true);
} else if (playerHand > dealerHand) {
emit PlayerWonTheGame(
"Dealer's hand is lower, players winning hand: ",
playerHand
);
endGame(msg.sender, true); // @written
} else {
emit PlayerLostTheGame(
"Dealer's hand is higher, dealers winning hand: ",
dealerHand
);
endGame(msg.sender, false);
}
}

However, if the fund transfer fails for any reason, the entire transaction is reverted. This means the player would need to call call() again to attempt claiming the prize. The issue is that calling call() again could change the dealer’s cards, potentially altering the outcome of the game, causing the player to lose when they would have otherwise 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 @audit- better use call
emit FeeWithdrawn(player, 2 ether); // Emit the prize withdrawal event
}
}

Impact

The impact is high, as a user who has already won may lose the prize and the funds they wagered if they need to call the call() function again. This can happen because the dealer's hand is recalculated, potentially altering the outcome and causing the user to lose after initially winning.

Tools Used

Manual Review.

Foundry.

Remix - Ethereum IDE.

Recommendations

To avoid the risk of altering the game outcome, the contract should ensure that the fund transfer is atomic and cannot be retried. One way to do this is by using a “claimable” status flag in the contract, which can be set to true only once the game is complete and the funds have been successfully transferred. This way, the player can only claim their prize once, and any attempt to call the call() function after the game has ended would be prevented.

Updates

Lead Judging Commences

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