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

Excess Ether Sent by Player Causes Unfair Loss in Winnings

koinerdegen

Summary

The contract allows players to send more than 1 ether to start the game, which violates the game’s intended betting logic. Players are supposed to bet exactly 1 ether and win 2 ether if they succeed. However, if a player sends more than 1 ether, they only win the hardcoded 2 ether, losing the excess ether sent. This inconsistency is due to the lack of a strict check on msg.value in the startGame() function.

Vulnerability Details

The startGame() function does not enforce a condition that ensures the player sends exactly 1 ether. The function logic accepts any amount greater than or equal to 1 ether. This behavior is evident in the following code snippet:

function startGame() public payable returns (uint256) {
address player = msg.sender;
require(msg.value >= 1 ether, "not enough ether sent");
initializeDeck(player);
uint256 card1 = drawCard(player);
uint256 card2 = drawCard(player);
addCardForPlayer(player, card1);
addCardForPlayer(player, card2);
return playersHand(player);
}
​

As a result, players can start the game with more than 1 ether, but the endGame() function is hardcoded to transfer only 2 ether as winnings. This leads to a situation where the player loses the excess amount they sent.

Test Case Output

The issue was replicated with the following test case:

function testUserCanSendMoreThenOneEtherToPlayButWillWinOnly2Ether()
public
{
uint256 userStartedBalance = player1.balance;
uint256 contractStartedBalance = address(twentyOne).balance;
​
vm.startPrank(player1);
twentyOne.startGame{value: 3 ether}();
uint256 userMidBalance = player1.balance;
uint256 contractMidBalance = address(twentyOne).balance;
twentyOne.call();
vm.stopPrank();
​
uint256 userEndedBalance = player1.balance;
uint256 contractEndedBalance = address(twentyOne).balance;
console.log("userStartedBalance: ", userStartedBalance);
console.log("userMidBalance: ", userMidBalance);
console.log("userEndedBalance: ", userEndedBalance);
console.log("contractStartedBalance: ", contractStartedBalance);
console.log("contractMidBalance: ", contractMidBalance);
console.log("contractEndedBalance: ", contractEndedBalance);
assertNotEq(userStartedBalance - userEndedBalance, 2e18);
assertNotEq(contractEndedBalance - contractStartedBalance, 0e18);
}

Output:

Ran 1 test for test/TwentyOne.t.sol:TwentyOneTest
[PASS] testUserCanSendMoreThenOneEtherToPlayButWillWinOnly2Ether() (gas: 1160391)
Logs:
userStartedBalance: 10000000000000000000
userMidBalance: 7000000000000000000
userEndedBalance: 9000000000000000000
contractStartedBalance: 10000000000000000000
contractMidBalance: 13000000000000000000
contractEndedBalance: 11000000000000000000

From the logs, we see:

  • The user started with 10 ether and ended with 9 ether after winning, losing the excess 1 ether they sent to start the game.

  • The contract balance shows the excess 1 ether added to it.

The issue stems from the startGame() function accepting msg.value >= 1 ether instead of enforcing msg.value == 1 ether.

Code Behavior

Relevant function snippets:

  1. startGame()

    • Accepts any value greater than or equal to 1 ether.

    • Does not refund or enforce exact ether for starting the game.

  2. endGame()

    • Transfers a hardcoded 2 ether as winnings regardless of the initial msg.value.

function endGame(address player, bool playerWon) internal {
delete playersDeck[player].playersCards;
delete dealersDeck[player].dealersCards;
delete availableCards[player];
if (playerWon) {
payable(player).transfer(2 ether); // Transfer the prize to the player
emit FeeWithdrawn(player, 2 ether); // Emit the prize withdrawal event
}
}
​

Impact

  1. Financial Loss for Player:

    • Players can lose any excess ether sent beyond 1 ether when starting the game.

    • In the test case, the player sent 3 ether and lost 1 ether even after winning the game.

  2. Inconsistent Game Logic:

    • The betting logic contradicts the intended behavior of betting 1 ether and winning 2 ether.

  3. Unfair Advantage to Contract:

    • Excess ether sent by players is retained in the contract, leading to unintended financial gains for the contract.

Tools Used

  • Forge (Foundry): Used to run and validate test cases.

  • EVM Error Logs: Inspected transaction traces and logs to confirm unexpected ether retention.

  • Manual Code Review: Identified the lack of msg.value validation in startGame().

Recommendations

  1. Enforce Exact Betting Amount in startGame(): Update the startGame() function to ensure players can only bet exactly 1 ether. This can be done as follows:

function startGame() public payable returns (uint256) {
require(
address(this).balance >= 2 ether,
"Not enough ether on contract to start game"
);
require(msg.value == 1 ether, "start game only with 1 ether");
​
address player = msg.sender;
initializeDeck(player);
uint256 card1 = drawCard(player);
uint256 card2 = drawCard(player);
addCardForPlayer(player, card1);
addCardForPlayer(player, card2);
return playersHand(player);
}

  1. Revert for Incorrect Ether Amounts: Any transaction sending more or less than 1 ether should revert with an appropriate error message.

  2. Test New Logic: Write additional test cases to confirm the contract reverts if msg.value is not exactly 1 ether.

  3. Documentation Update: Clearly document the betting rules for players, emphasizing the requirement to send exactly 1 ether.

By implementing these changes, the game will maintain fairness and align with its intended logic, avoiding unintended ether loss for players.

Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

[INVALID] User mistake, too much ETH sent

Support

FAQs

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