# 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