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

--- 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 Здесь представлена неполная схема взаимодействия всех смарт контрактов ::: ![](https://i.imgur.com/Np6Lty2.jpg)