---
# 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>.
Date: 2023-06-12

TODO

## Scope

TODO

## 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

##### INFORMAL

приведу пример
CoinSide = Heads = 2 = 0b10
CoinSide = Tails = 3 = 0b11

encryptedBet = 0b00
если загадывающий хочет он может дать
key = 0b01 -> encryptedBet ^ key = 0b10
key = 0b00 -> encryptedBet ^ key = 0b11

смысл этого тоже непонятно

if (coinSide != CoinSide.Heads && coinSide != CoinSide.Tails) {
    return CoinSide.Closed;
}

как может расшифровка секрета о том какой сайд монеты загадан дать чтото кроме орла или решки?
но с encryptedBet ^ key может дать что угодно

Давайте вместо xor рассмотрим использование add
(На самом деле с точки зрения криптостойкости они одинаковые)

Secret + key = result

Secret = 1

Вы думаете что знаете моё решение 

Дальше я такой думаю окей я хочу чтобы мой result=1, then key I will give =0, otherwise if I want the result to become 0 I will give key=-1

Примерно то же самое

Xor это совсем не необратимая функция мягко говоря 

Поэтому я и предлагал что

Secret = ecdsaSignature(result)

ECDSA signatures are designed to be computationally infeasible to reverse. This is a fundamental property of secure cryptographic systems: they are "one-way" functions.

То же самое можно сделать используя secret=keccak256(result, nonce)


##### Status.
NEW


______

#### 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.
NEW


______

#### 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.
NEW


______

#### 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.
NEW


______

#### 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.
NEW


______

#### 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.
NEW


______

#### 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.
NEW


______

#### WARNING-5.

At
- Contract.sol:205

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

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

##### Status.
NEW


______

#### 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.
NEW


______

#### 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.
NEW

______

#### WARNING-8.

At
- Contract.sol:335

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

explain why divided by 2

write detailed comment

##### Status.
NEW


______

#### LOW-1.

At
- Contract.sol:2

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

rename the file to GameContract.sol

##### Status.
NEW


______

#### 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.
NEW


______

#### 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.
NEW


______

#### LOW-4.

At
- Contract.sol:30

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

explicitly declare as public/internal

##### Status.
NEW


______

#### LOW-5.

At
- Contract.sol:39

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

typo in Earnings

##### Status.
NEW


______

#### LOW-6.

At
- Contract.sol:40

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

typo in Referrers

##### Status.
NEW


______

#### 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.
NEW


______

#### LOW-8.

At
- Contract.sol:58

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

typo in Assessed

##### Status.
NEW


______

#### LOW-9.

At
- Contract.sol:59

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

indexed gameId

##### Status.
NEW


______

#### 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.
NEW


______

#### LOW-11.

At
- Contract.sol:86

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

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

##### Status.
NEW


______

#### LOW-12.

At
- Contract.sol:94

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

rename to placeBidWithReferer

##### Status.
NEW


______

#### LOW-13.

At
- Contract.sol:97

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

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

##### Status.
NEW


______

#### LOW-14.

At
- Contract.sol:99

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

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

##### Status.
NEW


______

#### LOW-15.

At
- Contract.sol:131

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

typo in word receiver

##### Status.
NEW


______

#### 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.
NEW

______

#### LOW-18.

At
- Contract.sol:137

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

limit max value

##### Status.
NEW


______

#### LOW-19.

At
- Contract.sol:142

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

limit max value

##### Status.
NEW


______

#### LOW-20.

At
- Contract.sol:147

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

restrict low/high values

##### Status.
NEW


______

#### LOW-21.

At
- Contract.sol:152

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

restrict low/high values

##### Status.
NEW


______

#### LOW-22.

At
- Contract.sol:157

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

restrict low/high values

##### Status.
NEW


______

#### LOW-23.

At
- Contract.sol:162

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

restrict low/high values

##### Status.
NEW


______

#### 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.
NEW


______

#### 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.
NEW


______

#### 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.
NEW


______

#### LOW-27.

At
- Contract.sol:300

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

maybe create another status

##### Status.
NEW


______

#### 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.
NEW


______

#### LOW-29.

At
- Contract.sol:285

```solidity=285
            }
```

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

##### Status.
NEW


