TwentyOne

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

[H-2] TwentyOne::playersHand Calculation is Off by One for Cards from 2 Through 10 Inclusive

Summary

In case modulo division result of 0 represents an Ace - The TwentyOne::playersHand calculation incorrectly adds the cardValue for cards equivalent to the real-world values 2, 3, 4, 5, 6, 7, 8, 9, and 10. This issue arises because the cardValue calculation performs a modulo 13 operation on numbers 1 through 52, which are intended to represent a full deck of cards, and directly adds the result to the TwentyOne::playersHand::playerTotal. The problem is that real-world cards do not have a value of 1, yet the modulo operation results in the value 1. This leads to the returned TwentyOne::playersHand::playerTotal being off by one for each drawn card with a value between 2 and 10 inclusive.

Vulnerability Details

The TwentyOne::playersHand calculation incorrectly adds the following cardValue:

uint256 cardValue = playersDeck[player].playersCards[i] % 13;

to the TwentyOne::playersHand::playerTotal for cards with values between 2 and 10. This is caused by the else condition in the following statement:

if (cardValue == 0 || cardValue >= 10) {
playerTotal += 10;
} else {
playerTotal += cardValue;
}

Issue

  • Card values in the range [1, 9] (representing real-world cards 2 through 10) are incorrectly added to playerTotal as cardValue.

  • The correct implementation should add these card values as cardValue + 1.

  • Card values in the range [10, 12] should be added as 10, representing the equivalent of the real-world cards Jack (J), Queen (Q), and King (K).

  • Aces (value 0 in modulo 13) should be handled separately according to BlackJack rules. Refer to the H-3 finding for more details.

Correct Behavior

  • For card values 1 through 9: playerTotal += cardValue + 1;

  • For card values 10 through 12: playerTotal += 10;

  • Aces (value 0): Handle separately based on H-3.

Impact

This bug occurs 100% of the time and causes incorrect calculations for TwentyOne::playersHand. This can result in:

  1. Players losing when they should have won.

  2. Players winning when they should have lost.

This undermines the fairness and reliability of the game.

Tools Used

  1. Manual Review

  2. Foundry (Proof-of-Concept unit tests can be provided upon request)

  3. ChatGPT

Recommendations

  1. Rewrite the if-else Statements:
    Update the if-else logic to properly calculate the TwentyOne::playersHand::playerTotal in line with BlackJack rules. The correct implementation should follow these rules:

    • Card values in the range [1, 9] (representing real-world cards 2 through 10) should be added as cardValue + 1.

    • Card values in the range [10, 12] (representing J, Q, K) should be added as 10.

    • Aces (value 0) should be handled separately. Refer to my [H-3] finding

    Example correction:

if (cardValue == 0) {
// Handle Ace separately (refer to H-3)
} else if (cardValue >= 10) {
playerTotal += 10; // J, Q, K
} else {
playerTotal += cardValue + 1; // Cards 2 through 10
  1. Refer to my H-3 finding:
    Ensure Ace handling is addressed as specified in the linked finding.

  2. Test Thoroughly:
    Create unit tests using Foundry to verify:

    • Correct calculations for all card ranges.

    • Proper handling of Aces according to the my [H-3] finding.

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.