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

[H-02] Dealer's Face Cards Calculated Incorrectly in TwentyOne Contract

iepathos

Summary

The TwentyOne contract incorrectly calculates the value of King cards (13) in the dealer's hand. Due to a logic discrepancy between playersHand() and dealersHand() functions, the King card (value 13) is calculated as 0 instead of 10 when modulo operation is applied, leading to incorrect hand totals for the dealer.

Vulnerability Details

Location: src/TwentyOne.sol

https://github.com/Cyfrin/2024-11-TwentyOne/blob/main/src/TwentyOne.sol#L43-L52

The vulnerability exists in the dealersHand() function where face card values are calculated differently than in the playersHand() function. While playersHand() correctly handles both King (13) and face cards, dealersHand() fails to account for cards with value 0 after modulo operation.

// In playersHand() - correct implementation
if (cardValue == 0 || cardValue >= 10) {
playerTotal += 10;
}
​
// In dealersHand() - incorrect implementation
if (cardValue >= 10) {
dealerTotal += 10;
} // Missing check for cardValue == 0

Proof of Concept:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
​
import {Test, console2} from "forge-std/Test.sol";
import {TwentyOne} from "../../src/TwentyOne.sol";
​
contract DealersHandFaceCardsTest is Test {
TwentyOne public game;
address player = address(0x1);
​
function setUp() public {
game = new TwentyOne();
vm.deal(address(game), 100 ether);
}
​
function testDealerFaceCardsCalculatedIncorrectly() public {
// Start game as player
vm.startPrank(player);
vm.deal(player, 2 ether);
game.startGame{value: 1 ether}();
​
// Force dealer's cards to be King (13) and Queen (12)
vm.store(
address(game),
keccak256(abi.encode(player, uint256(1))), // slot for dealersDeck[player]
bytes32(uint256(2)) // length of array = 2
);
bytes32 slot = keccak256(abi.encode(
keccak256(abi.encode(player, uint256(1))) // slot for dealersDeck[player].dealersCards array
));
// Store King and Queen
vm.store(address(game), bytes32(uint256(slot)), bytes32(uint256(13))); // King
vm.store(address(game), bytes32(uint256(slot) + 1), bytes32(uint256(12))); // Queen
​
uint256 total = game.dealersHand(player);
// King (13 % 13 = 0) is counted as 0 instead of 10
// Queen (12 % 13 = 12) is counted as 10
assertEq(total, 10, "Dealer's King + Queen should be calculated as 20 (10 + 10) but is calculated as 10 (0 + 10)");
vm.stopPrank();
}
}

Impact

The incorrect calculation of dealer's face cards has several significant impacts:

  1. Game Logic Breaking

    • Dealer's hand totals are incorrectly calculated when holding King cards

    • Game outcomes are unfairly skewed in favor of players when dealer holds Kings

    • Breaks standard Blackjack rules and expected game behavior

  2. Economic Impact

    • Contract may incorrectly determine game winners

    • Players might win when they should lose, or vice versa

    • Potential loss of contract funds due to incorrect game outcomes

  3. Trust and Fairness

    • Inconsistent card value calculations between player and dealer

    • Undermines the fairness of the game

    • Could be exploited by players aware of this discrepancy

Tools Used

  • Foundry Testing Framework

  • Manual Code Review

  • Custom test cases for dealer face card scenarios

Recommendations

  1. Fix Card Value Calculation:
    The core issue is that using modulo 13 makes Kings (13) evaluate to 0. Instead, we should handle face cards (11, 12, 13) consistently by treating them all as 10:

function dealersHand(address player) public view returns (uint256) {
uint256 dealerTotal = 0;
for (uint256 i = 0; i < dealersDeck[player].dealersCards.length; i++) {
uint256 rawCardValue = dealersDeck[player].dealersCards[i];
// Face cards (11, 12, 13) are worth 10
if (rawCardValue % 13 >= 10 || rawCardValue % 13 == 0) {
dealerTotal += 10;
} else {
dealerTotal += rawCardValue % 13;
}
}
return dealerTotal;
}

This fix ensures:

  • All face cards (Jack=11, Queen=12, King=13) are worth 10

  • Number cards (1-10) maintain their correct values

  • Consistent with standard Blackjack rules

  1. Standardize Card Value Calculation:

    • Apply the same fix to both playersHand() and dealersHand() functions

    • Consider extracting the card value calculation into a separate pure function:

function getCardValue(uint256 rawCardValue) internal pure returns (uint256) {
uint256 cardValue = rawCardValue % 13;
return (cardValue >= 10 || cardValue == 0) ? 10 : cardValue;
}

  1. Add Comprehensive Testing:

    • Test all face card combinations (10, Jack, Queen, King)

    • Verify all face cards are valued at 10

    • Include edge cases with multiple face cards

Updates

Lead Judging Commences

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

Asymmetric calculation of hands is rigged in the player`s favor.

Support

FAQs

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