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 7 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.