Audit findings for CardRoomLeaderboard.sol

[bh2smith]

CardRoomLeaderboard.sol Audit Summary

Scope

Contract reviewed:

• contracts/src/CardRoomLeaderboard.sol

The contract stores player stats for six card games and exposes functions for players to submit solo or vs-AI results. It tracks wins, losses, total cards remaining, games played, last played timestamp, and per-game player lists.

Summary

The contract is simple and has no direct fund custody, external calls, delegatecalls, ownership controls, or upgradeability surface. The main risks are game-integrity and scalability related rather than classic Solidity exploit vectors.

Existing Foundry tests pass:

• 27 passed
• 0 failed

Findings

High: Game Results Are Fully Self-Reported

Affected functions:

• recordSoloResult(uint8 gameId, bool won, uint8 cardsRemaining)
• recordVsAiResult(uint8 gameId, bool won)

Any address can submit arbitrary results for itself. For example:

• A solo player can call recordSoloResult(GOLF, true, 0) without proving they won.
• A vs-AI player can call recordVsAiResult(HEARTS, true) without proving gameplay occurred.
• The per-player rate limit only slows repeat submissions. It does not verify correctness.
• Sybil addresses can bypass per-address rate limits.

Impact:

If leaderboard integrity matters, the leaderboard can be manipulated.

Recommendation:

Use one of the following:

• Authorized result submitter
• Backend/server-signed result attestations
• Oracle or verifier contract
• Cryptographic proof of gameplay, if practical
• Accept the current model only if the leaderboard is intentionally casual/trust-based

Medium: getLeaderboard Does Not Scale

Affected function:

• getLeaderboard(uint8 gameId)

The function returns every player for a game and loops over the entire _players[gameId] array. Since the array grows forever and anyone can add themselves by submitting a result, the function can eventually become too large for practical RPC responses or gas-limited view execution.

Impact:

The leaderboard read path may become unusable as player count grows or if spammed.

Recommendation:

Do not return the full leaderboard on-chain. Prefer:

• Store full cumulative per-player stats.
• Maintain a bounded top 100 leaderboard in contract state.
• Return only the top 100 from the contract.
• Use events/off-chain indexing for full historical rankings.

A reasonable design is:

• Keep stats[gameId][player] for all players.
• Keep _topPlayers[gameId] capped at 100.
• Update the top list when results are recorded.
• Expose paginated reads only if full on-chain browsing is truly needed.

Low: Read Functions Accept Invalid Game IDs

Affected functions:

• getPlayerStats(uint8 gameId, address player)
• getLeaderboard(uint8 gameId)
• getPlayerCount(uint8 gameId)

Write functions validate game IDs, but read functions silently return empty/default data for invalid IDs.

Impact:

Not directly exploitable, but it can hide frontend or integration bugs.

Recommendation:

Add validation:

require(gameId < GAME_COUNT, "Invalid game");

Informational: Leaderboard Ordering Is Off-Chain

Affected function:

• getLeaderboard(uint8 gameId)

The contract returns players in insertion order, not ranked order. Ranking is currently expected to happen off-chain/frontend-side.

Impact:

This is fine if intentional, but the function name may imply canonical sorted leaderboard semantics.

Recommendation:

Either:

• Rename/document the function as returning raw stats, or
• Maintain a sorted bounded top 100 leaderboard on-chain.

Suggested Direction

The most practical contract design is:

1. Keep cumulative stats for all players.
2. Maintain a top 100 leaderboard per game on-chain.
3. Define explicit scoring rules per game type.
4. Use GameResult events for off-chain full leaderboard reconstruction.
5. Add validation to all read functions.

Suggested ranking examples:

Solo games:

• Wins descending
• Average cards remaining ascending
• Games played descending as tie-breaker

Vs-AI games:

• wins - losses descending
• Wins descending
• Games played descending as tie-breaker

Consider adding a minimum games threshold if one-game lucky scores should not dominate.

Overall Assessment

The contract is structurally simple and low-risk from a traditional Solidity security perspective, but it is not secure against leaderboard manipulation because all results are user-submitted. The main production-readiness improvements are result verification and bounded leaderboard reads.