# SECURITY AUDIT OF *** SMART-CONTRACTS (GROX.SOLUTIONS - EXAMPLE)

# Assessment Principles

**Classification of Issues**

* 🟥 - CRITICAL ISSUES (critical, high severity):

Bugs and vulnerabilities that enable theft of funds, lock access to funds without possibility to restore it, or lead to any other loss of funds to be transferred to any party; high priority unacceptable bugs for deployment at mainnet; critical warnings for owners, customers or investors.

* 🟧 - PROBLEMS AND BUGS (medium, low severity):

Bugs that can trigger a contract failure, with further recovery only possible through manual modification of the contract state or contract replacement altogether; lack of necessary security precautions; other warnings.

* 🟨 - OPTIMISATION POSSIBILITIES (very low severity):

Possibilities to decrease cost of transactions and data storage of Smart-Contracts.

* ⬜ - COMMENTS AND RECOMENDATIONS (very low severity):

Tips and tricks, all other issues and recommendations, as well as errors that do not affect the functionality of the Smart-Contract.

* ❓ - QUESTIONS (any severity):

Questions to the logic of current implementation of smart-contracts or to the technical task; warnings about difference between smart contract and technical task.

#
**Other marks**

NOTE:

[100] / [200-300] - lines of code.

MARKS:

* ⛳ - Customer attention required
* ✅ - Solved or fixed as suggested
* ☑️ - Customer decided not to fix, and it’s OK
* ❌ - Customer decided not to fix, and it’s NOT OK

##
# DETECTED ISSUES

🟥 **CRITICAL ISSUES (7):**

| (🟥) | U0 | U1 | U2 | U3 | U4 |
|:----:|:---:|:---:|:---:|:---:|:---:|
| 1 | ⛳ | ❌
| 2 | ⛳ | ☑️
| 3 | ⛳ | ✅
| 4 | ⛳ | ⛳
| 5 | ⛳ | ⛳
| 6 | ⛳ | ⛳
| 7 | ⛳ | ⛳

1. ❌ В коде не используется библиотека безопасных вычислений SafeMath, в связи с чем присутствуют множественные опасности ошибок Overflow/Underflow: 120, 147, 200, 256, 257, 310

2. ☑️ [196-211] function startDraw, комментарий гласит: «может быть запущена кем угодно». Это не так, на функции модификатор onlyAdminOrOwner. Что означает только админ или владелец может розыграть призы и продолжить игру.

3. ✅ [233-278] function endDraw, так как адреса пользователей и адрес комиссии не проверяются на смарт контракт** и нет модификатора NonReentrant*** в коде присутствует возможность реализации Reentrance attack и перезахода в строках 265, 268 в функцию endDraw перед установкой drawInProcess = false [273].

** - типовая проверка на смарт-контракт.

function isContract(address addr) internal view returns (bool) {
    uint size;
    assembly { size := extcodesize(addr) }
    return size > 0;
}

*** - NonReentrant

contract ReentrancyGuard {

    uint256 private constant _NOT_ENTERED = 1;
    uint256 private constant _ENTERED = 2;

    uint256 private _status;

    constructor () internal {
        _status = _NOT_ENTERED;
    }

    modifier nonReentrant() {
        require(_status != _ENTERED, "ReentrancyGuard: reentrant call");

        _status = _ENTERED;
        _;
        _status = _NOT_ENTERED;
    }

4. ⛳ [280-292] function createRound, в виду того что в массив depositsBuffer пользователи встают при условии rounds[currentRoundNumber].users.length >= 10 [147], если depositsBuffer будет содержать более 10 записей, цикл в строках 288-290 самозациклится, что сломает контракт сделав невозможным розыгрыш.

5. ⛳ [316-318] ownerEndDraw, параметр seed задается овнером, что позволяет гарантированно манипулировать исходом розыгрыша.

6. ⛳ [366] function setTicketPrice, нет проверки параметра на 0, в связи с чем есть потенциальная возможность сломать контракт, если в новом раунде будет зафиксирована неверная стоимость.

7. ⛳ [334] function setMinRoundDuration, параметр newValue не проверяется на 0, в связи с чем структура контракта позволяет произвести атаку на пользователей в текущем раунде, гарантированно мгновенно розыграв в свою пользу приз.

#
🟧 **PROBLEMS AND BUGS (6):**

| (🟧) | U0 | U1 | U2 | U3 | U4 |
|:---------------:|:---:|:---:|:---:|:---:|:---:|
| 1 | ⛳ | ⛳
| 2 | ⛳ | ⛳
| 3 | ⛳ | ✅
| 4 | ⛳ | ⛳
| 5 | ⛳ | ⛳
| 6 | ⛳ | ⛳

1. ⛳ [198] function startDraw, [213] function startDrawAdmin, имеет модификатор isWorking, что дает возможность не позволить розыграть приз собранных средств.

2. ⛳ [48] bool public development, является излишней и создает несколько спорных моментов в коде в строках 206 и 316.

3. ✅ [316-318] ownerEndDraw, параметр seed задается овнером, в виду чего вызов оракла не нужен (транзакция callback упадет, так как розыгрыш будет уже произведен).

4. ⛳ [345] function setCommissionWalet, [349] function setAdmin, нет проверок параметра на нулевой адрес.

5. ⛳ [353] function setCommissionPercent, нет ограничений на comissionPercent (пример: не является 0, не превышает 10%, не превышает 100%).

6. ⛳ [357] function setOraclizeGasLimit, [361] function setCustomGasPrice, нет проверок параметра на 0.
#
🟨 **OPTIMISATION POSSIBILITIES (8):**

| (🟨) | U0 | U1 | U2 | U3 | U4 |
|:---------------:|:---:|:---:|:---:|:---:|:---:|
| 1 | ⛳ | ✅
| 2 | ⛳ | ☑️
| 3 | ⛳ | ☑️
| 4 | ⛳ | ✅
| 5 | ⛳ | ⛳
| 6 | ⛳ | ⛳
| 7 | ⛳ | ⛳
| 8 | ⛳ | ⛳

1. ✅[51] uint256 public minDeposit = 1; [338] function setMinDeposit; [106-109] modifier isOpen; - не задействованы в коде.

2. ☑️[57-58] uint256 public maxTotalJp = 0; uint256 public sumTotalJp = 0; - чисто статистические данные, функционально в коде не используются, нагружают СК. Могут быть исключены в пользу использования ивентов.

3. ☑️[68-74] struct User, mapping userAccounts; - чисто статистические данные, функционально в коде не используются, нагружают СК. Могут быть исключены в пользу использования ивентов.

4. ✅[75] address[] users; - чисто статистические данные, функционально в коде не используются, нагружают СК. Могут быть исключены в пользу использования ивентов, либо заменены на uint256 users;

5. ⛳[83] uint256 winnerIndex; - функционально в коде используются лишь в функции endDraw, нагружает СК. Можно исключить переделав функционал в функции endDraw.

6. ⛳[126-133] fallback с msg.value == 0 не несет никакого функционала и не нужен пользователям. Вопрос – какая необходимость интегрировать данный цикл с gasBurnCounter?

7. ⛳[184-190] при покупке "последнего" билета можно добавить вызов функции startDraw для сокращения количества транзакций в цикле.

8. ⛳[201, 311] require(rounds[currentRoundNumber].users.length >= 2, "Need more players."); - излишняя проверка, на две строки выше require(rounds[currentRoundNumber].startDrawTime!=0, "Round don't start yet."); итак подразумевает что пользователей больше или равно 2.
...
#
⬜ **COMMENTS AND RECOMENDATIONS (16):**

| (⬜) | U0 | U1 | U2 | U3 | U4 |
|:---------------:|:---:|:---:|:---:|:---:|:---:|
| 1 | ⛳ |☑️
| 2 | ⛳ |✅
| 3 | ⛳ |☑️
| 4 | ⛳ |⛳
| 5 | ⛳ | ⛳
| 6 | ⛳ | ⛳
| 7 | ⛳ | ⛳
| 8 | ⛳ | ⛳
| 9 | ⛳ | ⛳
| 10 | ⛳ | ⛳
| 11 | ⛳ | ⛳
| 12 | ⛳ | ⛳
| 13 | ⛳ | ⛳
| 14 | ⛳ | ⛳
| 15 | ⛳ | ⛳
| 16 | ⛳ | ⛳

1. ☑️[3] Источником энтропии в СК является ораклайзер. Теоретически владельцы оракла могут вмешаться в передаваемые данные (с учетом репутации сервиса – крайне маловероятно).

2. ✅[1] Перед деплоем рекомендуется выставить установленную версию солидити (без ^).

3. ☑️[46-60] Все изменяемые / изменяющиеся в процессе использования контракта переменные лучше устанавливать не в коде, а в конструкторе.

4. ⛳По коду во множестве мест хромает стилистика кода: отсутствие необходимых пробелов, лишние пробелы и тд.

5. ⛳По коду множество комментариев на русском языке – рекомендуется убрать их / заменить на английские.

6. ⛳В коде можно реализовать включение / выключение оракла.

7. ⛳Все internal и private функции можно переписать в стилистику _name() вместо name()/nameInternal().

8. ⛳[128, 144, 288, 361] Стилистику написания типа данных uint рекомендуется привести к uint256 во всех случаях употребления.

9. ⛳[148] В коде не реализовано ивента для важного для пользователя события – добавление его покупки в очередь depositsBuffer.

10. ⛳[153] ticketToPurchaze, опечатка в слове purchase.

11. ⛳[164] if (ticketToPurchaze > 0)… - лучше переделать в require.

12. ⛳[184] if (rounds[currentRoundNumber].users.length == 2… - лучше переписать на >= 2.

13. ⛳[199] "Round don't start yet." – грамматически правильнее написать «The round hasn't started yet».

14. ⛳[231] event PaymentError(address User, uint256 value); - рекомендуется поднять данный ивент выше по коду к остальным ивентам, а также исправить User на user.

15. ⛳[235] "Draw not in process!" - – грамматически правильнее написать «Drawing is not in process / Drawing hasn't started yet»

16. ⛳[271] emit EndDraw; в третьем поле указывается totalPrize хотя для пользователя лучше указать сумму с учетом комиссии.

#
❓ **QUESTIONS (3):**

| (❓) | U0 | U1 | U2 | U3 | U4 |
|:---------------:|:---:|:---:|:---:|:---:|:---:|
| 1 (🟨) | ⛳ | ⛳
| 2 (🟥) | ⛳ | ☑️
| 3 (⬜) | ⛳ | ☑️

1. ⛳[126-133] fallback с msg.value == 0 не несет никакого функционала и не нужен пользователям. Вопрос – какая необходимость интегрировать данный цикл с gasBurnCounter? (🟨- low severity)

2. ☑️[326-329] function ownerWithdraw является «легальным» бэкдором владельца для вывода кассы проекта. (🟥 - critical severity)

3. ☑️[360] Комиссия при продаже снимается не с выручки пользователя а с баланса контракта, как доп трата. Как правило в проектах комиссия накладывается на сумму (то есть сумма вывода уменьшается на размер комиссии). (⬜ - very low severity)

##

# Conclusion

Смарт-контракт имеет значительные баги и уязвимости, множество возможностей для оптимизации и требует исправления перед деплоем на основную сеть.

##