Protocol Review

In the initialize function the USDB yields are set to automatic, however there is no mechanism to claim the yield which will be reflected in an increasing USDB balance for the contract.

Consider configuring YieldMode.CLAIMABLE and implementing a function to claim the USDB yields that have accrued for the contract. A similar approach could be implemented for the WETH token, though it is not expected that the contract holds WETH.

There is currently no cap on how many players can be added to a table, and in the updateTableCredits function these players must be iterated over to remove players and create the approved_players and player_credits arrays.

These loops may cost an extreme amount of gas and even require more than the block gas limit to execute if there are many players in a single game. This could ultimately result in trapped funds for users, though they will be able to emergency withdraw.

Consider capping the total amount of players which can be added to a table.

A user can request a buy-in for the zero address. If accepted, this user cannot be kicked from the game. This is because _removePlayerFromTable will attempt to transfer ERCUSDB to the zero address, which reverts. Furthermore, if the user was added to the pending playersToRemove, the loop in updateTableCredits will revert causing DoS for the entire table. Ultimately, this player will never be able to get removed from the game.

Validate that the player is not address(0) in requestBuyInOnBehalf

In the _cancelRequest function there is no validation to ensure that the player has an active request to cancel for the table.

Thus a user can cause the PlayerCancelRequest event to be emitted even if they did not have a pending request. This may cause unexpected behavior for frontend systems or other consumers of the events from the Ginza contract.

Consider reverting with a descriptive custom error if the table.playerRequest.remove(player) operation returns false. Or performing a dedicated validation first thing in the function.

The emergencyExit function does not have an onlyApprovedPlayer modifier, therefore any arbitrary player can call the emergencyExit function on a stale table.

Currently there is no effect beyond emitting the PlayerLeft event which may affect off-chain systems.

Consider adding an onlyApprovedPlayer modifier to the emergencyExit function.

In the initialize function in the Ginza contract there is no invocation of the __UUPSUpgradeable_init function. There is no logic in the __UUPSUpgradeable_init function in the UUPSUpgradeable contract, however it is a best practice to call this function if this were to change in a future iteration.

Consider calling the __UUPSUpgradeable_init function in the initialize function.

In the Ginza contract tables can only be opened and not closed.

Consider if this is the expected behavior. If not, consider implementing a function which that allows the table owner to close the table only after all players have left.

In the _cancelRequest function the table.playerRequest.remove(player) action is performed after transferring tokens to the user. However this accounting update ought to occur before giving the user their tokens to follow best practices.

Consider performing the table.playerRequest.remove(player) logic before transferring tokens to the user.

In the withdrawRake function the collectedRake entry for the user is reset to zero after the safeTransfer call sends their collected amount to the user. This is not immediately a risk as USDB is not a callback token, however it is a best practice to follow CEI for transfers to a user.

Consider updating all accounting before making transfers to the user.

In the playerTopUp and _requestBuyIn functions the safeTransferFrom action occurs after all accounting updates have occurred.

However it is a best practice to transfer funds in before performing state updates. This is because it is a valid state for the protocol to have received funds and not made accounting updates, but not for the protocol to have made accounting updates and not received funds.

Perform safeTransferFrom after all initial validations in a function, but before any of the accounting updates are performed in the playerTopUp and _requestBuyIn functions.

In the PlayerCancelRequest a useful piece of information may be who is initiating the cancellation, as either the user or the table owner may do so.

Consider if the canceler is a useful piece of information to add to the PlayerCancelRequest event.

In the updateTableCredits function there is no validation that a user is not in both of the winners and losers lists.

Consider implementing validations such that no user may appear in both the winner and loser lists.

The getAllPlayerRequestsForTable function iterates over all of the pending player requests to return the total player requests and amounts. There is no limit on how many player requests there are for a table, and there shouldn’t be, otherwise users could block preferred players from joining a game.

However this means that the amount of player requests for a table may be extensive, even enough to overflow the block gas limit. For this reason the getAllPlayerRequestsForTable function should be used for off-chain reading purposes only and no Smart Contract should attempt to integrate it into its own logic.

Be aware of this risk and warn potential integrators about it.

A user can be kicked with function kickPlayer which will create a removal request and emit a PlayerLeave event. If all other players are removed, the user can be kicked again and now will be directly removed from the table with _removePlayerFromTable. This will also emit a PlayerLeave event.

This may cause issues with the UI relying on these events, since it appears as if the player left twice without joining in between.

Be aware of this behavior and consider removing a user from playersToRemove if he was already directly removed with _removePlayerFromTable.

It is possible for a user to frontrun a call to setProtocolShare which would increase the share, and withdrawRake right before to pay the smaller fee.

Be aware of this behavior and document it appropriately.

A user can call function playerTopUp to increase their credit even if they are in the playersToRemove list because they are still considered active until removal. This may be unexpected from a user’s perspective.

Consider if this is intended behavior and document as necessary.

The chainSync function returns table data in the order of approvedPlayers, credits, seats, pendingPlayers, and requestedAmounts. However the batchChainSync function stores the returned values as approvedPlayers, seats, credits, pendingPlayers, and requestedAmounts where the seats and credits values are switched.

This will mistaken the players seat number for their credit amount and cause significant loss for the players using the system relying on this reporting.

Correct the ordering of the values in the batchChainSync to agree with the ordering of the chainSync function.

In the GinzaV4 contract the constructor with the _disableInitializers invocation has been removed. This is likely for testing purposes but the function call should be re-instated before deployment to avoid a malicious upgrade and self destruction of the implementation UUPS contract.

Re-introduce the constructor with the _disableInitializers call in all Ginza contracts before production use.

In the approveRequest function the new player is added to the approvedPlayers list before the findNextAvailableSeat operation. The findNextAvailableSeat operation uses the approvedPlayers list to find each taken seat to deduce if a seat is available or not.

Since the new player is added to the approvedPlayers list before the findNextAvailableSeat operation the findNextAvailableSeat function must add an additional unnecessary iteration for this new player for every seat that is checked.

Consider adding the new player to the approvedPlayers list after the findNextAvailableSeat operation to save on gas expenditure when approving a player.

When creating a request to join a table the desired seat is recorded in the seatAssignments mapping, however in the _cancelRequest function the user is not removed from the seatAssignments mapping.

Remove all remnants of the player’s request from the seatAssignments mapping in the _cancelRequest function.

In the GinzaV4 contract the emergencyWithdraw threshold has been reduced to 15 minutes, however the documentation in the contract suggests that the threshold is still 24 hours.

Update the documentation to reflect the new threshold.

In the initialize function the USDB yields are configured to automatic by default, however the functionality which exists to claim these yields is using a claimable yield mode.

Instead of including the makeYieldClaimable function, consider making the yield claimable by default in the intialize function.

In the batchChainSync function the blockNumber_ stack variable is declared and never used.

Remove the declaration of the blockNumber_ stack variable and instead leave a blank comma for the final return value from the chainSync function.