---
# System prepended metadata

title: Security Audit of `GameContract` contracts.

---

# Security Audit of `GameContract` contracts. 

## Conclusion

This audit was made by 
Auditor: Vladimir Smelov <vladimirfol@gmail.com>.
LinkedIn: https://www.linkedin.com/in/vladimir-smelov-25021669/

Pre Date: 2023-06-12
Final Date: 2023-08-11

After thorough analysis and examination of the GameContract based on the stated methodology, I can confidently assert the following:

The contract does not contain any apparent backdoors or malicious code.
There are no significant vulnerabilities or issues pertaining to the transfer of funds.
Potential areas of concern, including math operations, event validations, and argument checks, have been carefully reviewed and deemed secure.
No risks associated with fund leakage or unintended locking of the contract have been identified.

## Scope

Repo: 
Pre Audit: commit 8067399b5fe1e37f21da97bc35ef190ba0460164
Final Audit: commit cf4f25ef62398b5b1dc99c250cd5be4b8b034e0e

## Methodology

1. Blind audit. Understand the structure of the code without reading any docs.
2. Ask questions to developers.
3. Run static analyzers.
4. Find problems with:
- backdoors;
- bugs;
- math;
- potential leaking of funds;
- potential locking of the contract;
- validate arguments and events;
- others.

## Result

#### CRITICAL-1.

At
- Contract.sol:257

```solidity=257
        bytes32 bet = encryptedBet ^ key;
```

key could be changed to get any value


nonce := getSignature -> hash(secret, gameId)
any device can calc nonce

begin:
hash(bet, nonce) = proof -> SC

finalize:
hash(0, nonce) ?= proof
hash(1, nonce) ?= proof

finalize(0, nonce)
hash(0, nonce) == proof

##### Status.
FIXED


______

#### MAJOR-1.

At
- Contract.sol:128

```solidity=128
        feeReceiver.transfer(value);
```

owner can break contract anytime by setting `feeReceiver` to the contract which does not accept incoming ETH transfers (with no `receive() external payable`).

Or use call and check success += if no success

##### Status.
FIXED


______

#### MAJOR-2.

At
- Contract.sol:358

```solidity=358
    function _withdrawEarnings(address receiver, uint256 value, bytes32 gameId) internal {
```

do pull-pattern, something like:

https://medium.com/heartbankstudio/smart-contract-design-patterns-8b7ca8b80dfb

https://docs.openzeppelin.com/contracts/4.x/api/security#PullPayment

##### Status.
FIXED

______

#### WARNING-1.

At
- ContractTron.sol:10

```solidity=10
contract GameContractTron is Ownable, ReentrancyGuard {
```

avoid code-duplication, you can use Contract with no proxy if you want non-upgradeable contract

##### Status.
FIXED

______

#### WARNING-2.

At
- Contract.sol:14

```solidity=14
    enum CoinSide { None, Closed, Heads, Tails, NotOpenedButPaymentRequested }
```

this is contr-intuitive. CoinSide could be Heads or Tails, consider the usage of another variable for clarity.

##### Status.
FIXED


______

#### WARNING-3.

At
- Contract.sol:100

```solidity=100
            _playerReferers[_msgSender()] = referer;
```

consider adding setReferrer onlyOwner function, to resolve tricky situations. For example, referrer as a human may stop to use specific wallet and move to another one.

For security you can approve re-setting the referrer by both, user and owner.

##### Status.
ACKNOWLEDGED


______

#### WARNING-4.

At
- Contract.sol:123

```solidity=123
        payable(_msgSender()).transfer(earnings);
```

- Contract.sol:128

```solidity=128
        feeReceiver.transfer(value);
```

never use transfer - https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/


##### Status.
FIXED


______

#### WARNING-5.

At
- Contract.sol:205

```solidity=205
            _games[id].totalGainings *= 2;
```

consider adding a comment how is totalGainings set to avoid misunderstanding.

##### Status.
FIXED


______

#### WARNING-6.

At
- Contract.sol

erc20/erc721 accidentally sent to the contract will be lost.

Consider add recoverERC20, recoverERC721 methods.

Take a look - https://github.com/vittominacori/eth-token-recover

##### Status.
ACKNOWLEDGED


______

#### WARNING-7.

At
- Contract.sol:231,244,311

```solidity=231
            return;
```
```solidity=244
            return;
```
```solidity=311
            return;
```
Better to raise an error to prevent misusing.

##### Status.
ACKNOWLEDGED

______

#### WARNING-8.

At
- Contract.sol:335

```solidity=335
            serviceEarnings = totalGainings * _serviceFee / (2 * DENOMINATOR);
```

explain why divided by 2

write detailed comment

##### Status.
FIXED


______

#### LOW-1.

At
- Contract.sol:2

```solidity=2
pragma solidity 0.8.6;
```

rename the file to GameContract.sol

##### Status.
FIXED


______

#### LOW-2.

At
- Contract.sol:3

```solidity=3
pragma experimental ABIEncoderV2;
```

activated by default https://docs.soliditylang.org/en/v0.8.0/080-breaking-changes.html

##### Status.
FIXED


______

#### LOW-3.

At
- Contract.sol:16-28

```solidity=16
    struct Game {
        address playerOne;
        bytes32 playerOneEncryptedBet;
        CoinSide playerOneChosenSide;
        address playerTwo;
        bytes32 playerTwoEncryptedBet;
        CoinSide playerTwoChosenSide;
        uint256  totalGainings;
        GameStatus status;
        uint256 gameCreatedTimestamp;
        uint256 gameStartedTimestamp;
        address winner;
    }
```

re-organise for efficient packing - https://dev.to/javier123454321/solidity-gas-optimizations-pt-3-packing-structs-23f4

##### Status.
ACKNOWLEDGED


______

#### LOW-4.

At
- Contract.sol:30

```solidity=30
    uint256 constant DENOMINATOR = 10_000;
```

explicitly declare as public/internal

##### Status.
FIXED


______

#### LOW-5.

At
- Contract.sol:39

```solidity=39
    mapping (address => uint256) private _refererEarings;
```

typo in Earnings

##### Status.
FIXED


______

#### LOW-6.

At
- Contract.sol:40

```solidity=40
    mapping (address => address) private _playerReferers;
```

typo in Referrers

##### Status.
FIXED


______

#### LOW-7.

At
- Contract.sol:51-56

```solidity=51
    event GameCreated(bytes32 id, address indexed playerOne, uint256 indexed bidValue);
    event GameStarted(bytes32 id, uint256 startedTimestamp, address indexed playerOne, address indexed playerTwo, uint256 indexed totalGainings);
    event SecretOpened(bytes32 id, address indexed player, CoinSide coinSide);
    event GameFinished(bytes32 id, address indexed winner, address indexed looser, uint256 indexed gainings);
    event GameCancelled(bytes32 id);
    event GamePayout(bytes32 id, address indexed receiver, uint256 value);
```

indexed id?

##### Status.
FIXED


______

#### LOW-8.

At
- Contract.sol:58

```solidity=58
    event RefererAssesed(address indexed referer, uint256 value);
```

typo in Assessed

##### Status.
FIXED


______

#### LOW-9.

At
- Contract.sol:59

```solidity=59
    event BidOpeningFailed(address indexed sender, bytes32 gameId, string message);
```

indexed gameId

##### Status.
FIXED


______

#### LOW-10.

At
- Contract.sol:71

```solidity=71
        require(_feeReceiver != address(0), Errors.ZERO_ADDRESS);
```

put inside setFeeRecevier, otherwise if _feeReceiver is not valid, the contract will be locked.

##### Status.
FIXED


______

#### LOW-11.

At
- Contract.sol:86

```solidity=86
    ) external payable nonReentrant {
```

unclear function semantics, consider documenting, what is id what is key

##### Status.
FIXED


______

#### LOW-12.

At
- Contract.sol:94

```solidity=94
    function placeBid(
```

rename to placeBidWithReferer

##### Status.
FIXED


______

#### LOW-13.

At
- Contract.sol:97

```solidity=97
    ) external payable nonReentrant {
```

unclear function semantics, consider documenting, what is id what is key

##### Status.
FIXED


______

#### LOW-14.

At
- Contract.sol:99

```solidity=99
        if (_playerReferers[_msgSender()] == address(0)) {
```

there is no need to use _msgSender() use msg.sender

##### Status.
FIXED


______

#### LOW-15.

At
- Contract.sol:131

```solidity=131
    function setFeeRecevier(address payable newReceiver) public onlyOwner {
```

typo in word receiver

##### Status.
FIXED


______

#### LOW-16.

At
- Contract.sol:132

```solidity=132
        feeReceiver = newReceiver;
```

what if newReceiver is not a valid account receiving fees?
Consider the usage of push-pull pattern for fees to avoid contract locking.

##### Status.
FIXED

______

#### LOW-18.

At
- Contract.sol:137

```solidity=137
        _serviceFee = newValue;
```

limit max value

##### Status.
ACKNOWLEDGED


______

#### LOW-19.

At
- Contract.sol:142

```solidity=142
        _refererFee = newValue;
```

limit max value

##### Status.
ACKNOWLEDGED


______

#### LOW-20.

At
- Contract.sol:147

```solidity=147
        _lowestBid = newValue;
```

restrict low/high values

##### Status.
ACKNOWLEDGED


______

#### LOW-21.

At
- Contract.sol:152

```solidity=152
        _highestBid = newValue;
```

restrict low/high values

##### Status.
ACKNOWLEDGED


______

#### LOW-22.

At
- Contract.sol:157

```solidity=157
        _waitingForOpenBidSeconds = newValue;
```

restrict low/high values

##### Status.
ACKNOWLEDGED


______

#### LOW-23.

At
- Contract.sol:162

```solidity=162
        _minRefererPayoutValue = newValue;
```

restrict low/high values

##### Status.
ACKNOWLEDGED


______

#### LOW-24.

At
- Contract.sol:193

```solidity=193
            _games[id].totalGainings = msg.value;
```

it's not clear why the bid of the user goes to the totalGainings

##### Status.
FIXED


______

#### LOW-25.

At
- Contract.sol:213,220,223

```solidity=213
        for (uint i = 0; i < ids.length; i++) {
```
```solidity=220
        for (uint i = 0; i < ids.length; i++) {
```
```solidity=223
        for (uint256 i = 0; i < expiredIDs.length; i++) {
```

use unchecked declaration to save gas

##### Status.
FIXED


______

#### LOW-26.

At
- Contract.sol:218

```solidity=218
    function openBid(bytes32[] memory ids, bytes32[] memory keys, bytes32[] memory expiredIDs) public nonReentrant{
```

use other name

##### Status.
FIXED


______

#### LOW-27.

At
- Contract.sol:300

```solidity=300
        _games[id].status = GameStatus.Paid;
```

maybe create another status

##### Status.
FIXED


______

#### LOW-28.

At
- Contract.sol:251

```solidity=251
        if (_games[id].playerOneChosenSide > CoinSide.Closed && _games[id].playerTwoChosenSide > CoinSide.Closed) {
            _finishGame(id);
        }
```

This is unclear place. Consider documenting.

##### Status.
FIXED


______

#### LOW-29.

At
- Contract.sol:285

```solidity=285
            }
```

What will happen in case of `else` condition???

##### Status.
FIXED


