Аудит проекта DCHF

Оглавление

Ссылка на исследуемые контракты

Смарт контракты были предоставлены в 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?.
В исследуемых контрактах есть несколько участков кода содержащих такую проблему.

  • В контракте "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 функции при работе с активами возвращают булево значение. Необходимо обрабатывать возвращаемое значение.

  • В контракте "CommunityIssuance" на строке 225 не обрабатывается ответ при вызове функции "transfer()".
  • В контракте "MONStaking" на строке 118, 161, 182, не обрабатывается ответ при вызове функции "transfer()", на строке 138 при вызове функции "transferFrom()".
  • В контракте "TokenScript" на строке 19 не обрабатывается ответ при вызове функции "transfer()" и на строке 35 при вызове функции "transferFrom()".
Recommendation

Необходимо добавить обработку получаемого значения или использовать библиотеку SafeERC20.

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.
В контракте "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

Здесь представлена неполная схема взаимодействия всех смарт контрактов

Select a repo