owned this note
owned this note
Published
Linked with GitHub
---
title: 'Аудит проекта DCHF'
---
Аудит проекта DCHF
===
## Оглавление
[TOC]
## Ссылка на исследуемые контракты
Смарт контракты были предоставлены в zip-архиве: dchf-audit.zip. SHA256 для этого файла равно 422634256D6B50B41A1F97E56B7174F27E44DD9B0D3D207CB1BFB16D82D004B0.
## Ре-аудит исследуемых контрактов
В промежутке времени с 26 ноября 2022 по 03 декабря 2022 был проведен повторный аудит исследуемых контрактов.
Был предоставлен доступ к приватному репозиторию по адресу:
https://bitbucket.org/grizzlyfi/dchf-contracts/src/master/. Коммит для реаудита имеет номер:
901c1b05372fbc17bc3474152e9a3916a119d96a
## Классификация найденных уязвимостей
• CRITICAL: Ошибки, приводящие к краже активов, блокировке доступа к контрактам, на балансе которых есть активы. А также недополученная прибыль или любая другая потеря активов.
• MIDDLE: Ошибки, которые могут привести к сбою работы контракта. Дальнейшее восстановление возможно только путем ручного изменения состояния контракта или его замены.
• WARNINGS: Ошибки, которые могут нарушить предполагаемую логику контракта или подвергнуть его DoS-атакам и т.д.
• COMMENTS: Другие проблемы и рекомендации, сообщенные для команды.
---
## Найденные проблемы
### CRITICAL
#### 1. Возможно проведение Reentrancy-атак
##### Description
Проведение Reentrancy-атаки может привести к потере ETH, который был на балансе контракта. Об этом написано, например, здесь: [What is a Reentrancy Attack?](https://www.certik.com/resources/blog/3K7ZUAKpOr1GW75J2i0VHh-what-is-a-reentracy-attack).
В исследуемых контрактах есть несколько участков кода содержащих такую проблему.
- В контракте "StabilityPool" на строках 182 и 224 функции "provideToSP()" и "withdrawFromSP()" может вызвать кто угодно. Внутри этих функций вызывается "_sendAssetGainToDepositor()". При вызове функции "_sendAssetGainToDepositor()" на строке 703 происходит отправка эфира. Если вызывающая сторона будет смарт контрактом с fallback() функцией обратно вызывающей "provideToSP()" и "withdrawFromSP()", то возможно проведение Reentrancy-атаки и выкачка эфира с баланса контракта.
- Такая же уязвимость наблюдается для контракта "ETHTransferScript" на строке 6.
##### Recommendation
Необходимо подключить библиотеку "ReentrancyGuard" также, как это, например, сделано в контракте "ActivePool" на строке 104.
##### Fixing the issue
Данная пролема была исправлена в коммите 9d886e1f180b7ac51ab0be77c924458afa174798
#### 2. Неверное значение эфира
##### Description
В контракте "BorrowerOperationsScript" на строке 23 для вычисления "msg.value" вызывается функция "getValueOrArg()". Эта функция возвращает количество эфира для payable функции "openTrove()" в контракте "BorrowerOperations". Однако, если значение переменной "_asset" окажется адресом токена все равно в "msg.value" будет записано значение "_assetAmountSent". Значение "_assetAmountSent" также используется для определения количества токенов, если передаются токены. Таким образом при передаче токенов будет передано "_assetAmountSent" токенов и такое же количество эфира. Но фактически будут учтены только токены, а эфир просто будет передан на баланс контракта "BorrowerOperations". Аналогичная ситуация для функции "addColl()" на строке 39 и функции "adjustTrove()" на строке 89.
##### Recommendation
Необходимо исправить логику проверки в функции "getValueOrArg()". Например, сделать по аналогии с проверкой в контракте "BorrowerWrappersScript" на строке 178.
##### Fixing the issue
Этот контракт и функция были удалены
### MIDDLE
#### 1. Нарушение стандарта ERC-20
##### Description
В соответствии со стандартом [ERC-20](https://eips.ethereum.org/EIPS/eip-20) функции при работе с активами возвращают булево значение. Необходимо обрабатывать возвращаемое значение.
- В контракте "CommunityIssuance" на строке 225 не обрабатывается ответ при вызове функции "transfer()".
- В контракте "MONStaking" на строке 118, 161, 182, не обрабатывается ответ при вызове функции "transfer()", на строке 138 при вызове функции "transferFrom()".
- В контракте "TokenScript" на строке 19 не обрабатывается ответ при вызове функции "transfer()" и на строке 35 при вызове функции "transferFrom()".
##### Recommendation
Необходимо добавить обработку получаемого значения или использовать библиотеку [SafeERC20](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.7/contracts/token/ERC20/utils/SafeERC20.sol).
##### Fixing the issue
Данная пролема была исправлена в коммите f6af9db8addb65a8dc181b6241970242d8cc21f5
#### 2. Нехватка газа для выполнения всех итераций цикла (DoS)
##### Description
Для каждой итерации цикла требуется газ. Может наступить момент, когда газа потребуется больше, чем выделено для записи одного блока. В этом случае все итерации цикла завершатся неудачно.
- В контракте "TroveManager" на строке 1134 переменной "_maxIterations" присваивается максимальное значение "type(uint256).max". Далее идет выполение цикла "while". При определенных условиях вызов функции "redeemCollateral()" будет завершаться ревертом из-за нехватки газа.
- В контракте "MONStaking" выполняется цикл "for" с количеством итераций цикла "ASSET_TYPE.length" на строках 153-169. Количество элементов массива "ASSET_TYPE" на строке 215 можно увеливать на неограниченное число. Может возникнуть ситуация, что перестанет работать функция "unstake()" и токены останутся заблокированными на балансе контракта.
##### Recommendation
Необходимо добавить ограничение на максимальное количество итераций цикла.
##### Fixing the issue
Данная пролема была принята к сведению.
#### 3. Возможна блокировка токенов на балансе контракта
##### Description
В контракте "MONStaking" при каждой процедуре "stake()" на строке 118 и "unstake()" на строке 161 на адрес "msg.sender" высылается часть токенов "dchfToken". Для корректной работы этих процедур необходимо, чтобы на балансе контракта было достаточное количество токенов "dchfToken". Эти токены переводит администрация проекта. Но в случае, если по каким-либо причинам потребуется забрать часть токенов с баланса контракта, то для этого нет разработанного функционала. Таким образом токены останутся заблокированными на балансе контракта.
##### Recommendation
Необходимо разработать функционал для экстренного перевода токенов "dchfToken" с баланса контракта "MONStaking".
##### Fixing the issue
Данная пролема была принята к сведению.
#### 4. Неверный прайс токена
##### Description
В контракте "PriceFeed" на строках 245, 249, 293, 306 делается преобразование переменной типа "int256" в переменную типа "uint256". Если значение переменной будет отрицательное, то после преобразования произойдет арифметическое переполнение. Необходимо перед преобразованием числа делать проверку его значения и брать значение по модулю.
Аналогично для контракта "PriceFeedV2" на строках 239, 243, 287, 300.
##### Recommendation
Необходимо исправить эти ошибки.
##### Fixing the issue
Данная пролема была принята к сведению.
### WARNINGS
#### 1. Неверное использование функционала
##### Description
В контракте "AdminContract" на строках 42-49 делаются бессмысленные действия, которые не дают никакого результата.
##### Recommendation
Необходимо сделать контракт "CheckContract" предком и на строках 42-49 делать вызов функции "checkContract()". Код можно переделать также, как, например, сделано в контракте "CommunityIssuance".
##### Fixing the issue
Данная пролема была принята к сведению.
#### 2. Возможна запись неверного значения в event
##### Description
В контракте "StabilityPool" на строкe 778 вызывается функция "sendMON" с параметром "depositorMONGain". Далее в событии "MONPaidToDepositor" записывается значение параметра "depositorMONGain". Но фактическое значение этого параметра может оказаться меньше и даже равно 0. Это видно в контракте "CommunityIssuance" на строке 219.
##### Recommendation
Рекомендуется сделать, чтобы функция "sendMON()" в контракте "CommunityIssuance" возвращала фактическое значение. Далее в контракте "StabilityPool" это значение будет обрабатываться.
##### Fixing the issue
Данная пролема была принята к сведению.
#### 3. Пустой контракт
##### Description
Контракт "GasPool" пустой. Однако в контракте "BorrowerOperations" на строках 133, 260, 580 идет работа с адресом такого контракта. И в контракте "TroveManager" на строках 72, 879, 1003, 1093 идет работа с адресом такого контракта.
##### Recommendation
Рекомендуется добавить логику или удалить этот контракт.
##### Fixing the issue
Данная пролема была принята к сведению.
#### 4. Нет проверок
##### Description
Перед тем, как использовать ecrecover() необходимо сделать дополнительные проверки параметров digest, v, r, s. Все это делаеттся в библиотеке ECDSA от [OpenZeppelin](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.7/contracts/utils/cryptography/ECDSA.sol).
В контракте "ERC20Permit" на строке 99 используется "ecrecover()" без проверок.
##### Recommendation
Рекомендуется подключить библиотеку для проведения проверок.
##### Fixing the issue
Данная пролема была принята к сведению.
### COMMENTS
#### 1. Ненужный импорт
##### Description
В контракте "BorrowerWrappersScript" на строке 14 есть неиспользуемый импорт.
##### Recommendation
Рекомендуется удалить ненужные импорты.
##### Fixing the issue
Данная пролема была принята к сведению.
#### 2. Неоптимизированные данные
##### Description
При сохранении структурированных данных можно делать оптимизацию. Например, если в структуре 4 поля с типом данных uint256, то при сохранении в storage будет занято 4 слота памяти. Если значения этих данных не очень большие, их можно переделать в тип uint64. В этом случае будет занят только 1 слот памяти. Это позволяет экономить газ.
- В контракте "LockedMON" на строках 16-22 описана структура "Rule" с данными типа uint256. Можно у части данных переопределить тип данных.
- В контракте "SortedTroves" на строках 69-75 описана структура "Data".
- В контракте "StabilityPool" на строках 53-59 описана структура "Snapshots".
##### Recommendation
Рекомендуется переопределить часть типов данных у структур.
##### Fixing the issue
Данная пролема была принята к сведению.
==================================================
## Новые найденные проблемы
#### 1. Возможна кража токенов у пользователей
##### Description
В контракте "CommunityIssuance" на строке 146 осуществляется вызов функции "safeTransferFrom()" для адреса кошелька, переданного в параметре "_spender". Функцию "_addFundToStabilityPoolFrom" можно вызвать на строке 128. Это может сделать владелец контракта или администратор контракта. Таким образом у любого пользователя, давшего approve() на адрес этого контракта, владелец контракта или администратор контракта могут без ведома пользователя забрать токены "monToken" с его кошелька. Затем владелец вызвав функцию "removeFundFromStabilityPool()", может перевести эти токены на свой кошелек.
##### Recommendation
Для пополнения баланса контракта использовать только служебные кошельки. Т.е. в качестве "_spender" использовать только служебные кошельки.
#### 2. Опасное тождество
##### Description
В контракте "CommunityIssuance" на строках 117 и 170 осуществляется вызов функции "disableStabilityPool()". Но это возможно только при выполненении строгого равенства значений переменных "totalMONIssued[_pool]" и "MONSupplyCaps[_pool]".
Если значение будет отличаться на ничтожно малую величину, такую, как 1/10**-18 степени равенство уже не выполнится. Например, такое отличие может произойти при делении целых чисел. Это приведет к тому, что вызов функции "disableStabilityPool()" не произойдет, хотя значения переменных будут почти тождественны
##### Recommendation
В таких случаях нельзя использовать строгое равенство значений переменных.
#### 3. Нет проверки адреса
##### Description
В контракте "DfrancParameters" на строке 114 осуществляется установка маппинг-переменной "hasCollateralConfigured" в истинное значение. Но проверки ключа маппинга, переменной "_asset" на то, что она не 0 не делается. Установка нулевого адреса в "true" может привести к непредсказуемым последствиям для работы контракта.
##### Recommendation
Необходимо добавить проверку для переменной "_asset".
#### 4. Нет необходимого функционала
##### Description
В контракте "DfrancParameters" на строке 114 осуществляется установка маппинг-переменной "hasCollateralConfigured" в истинное значение. Но коллатералы могут резко начать терять свою стоимость, например, как с токенами LUNA или FTT. В этом случае можно было бы заблокировать работы с таким токеном. Но сейчас такого функционала нет.
##### Recommendation
Необходимо добавить функционал для блокировки токенов.
#### 5. Возможна блокировка работы
##### Description
В контракте "SortedTroves" на строке 245 производится уменьшение значения переменной "data[_asset].size".
Но нет проверки, что это значение больше 0. Отсутствие такой проверки может привести к блокировке работы функции "remove()" и, как следствие, блокировке работы функционала для ликвидации "Trove" в контракте "TroveManager".
##### Recommendation
Исправить ошибку.
#### 6. Лишние переменные
##### Description
В контракте "DCHFToken" на строке 44 определена переменная "borrowerOps". В функциях "addBorrowerOps()" и "removeBorrowerOps" значение переменной меняется. Но в этом контракте и в других, связанных с эттим, больше нигде не используется эта переменная. Вместо этой переменной для проверок используется другая переменная "validBorrowerOps". Аналогично для переменной "troveManagers" на строке 43.
##### Recommendation
Можно уменьшить стоимость транзакций, если не использовать лишние переменные. Для фронта массив адресов для "borrowerOps" и "troveManagers" можно получить из событий.
## Appendix
:::info
Здесь представлена неполная схема взаимодействия всех смарт контрактов
:::
