# 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
Sign in with Wallet
Connect another wallet