16 Грудня, 2021
Вступ
Команда 1inch команда попросила нас переглянути та перевірити їх Протокол лімітного замовлення смарт-контракти v2. Ми переглянули код і опублікували результати.
Сфера
Ми перевірили фіксацію 4d94eea25e4dac6271bfd703096a5c4a4d899b4a
в 1inch/limit-order-protocol
сховище. У сферу дії входили такі договори:
- OrderMixin.sol
- OrderRFQMixin.sol
- PredicateHelper.sol
- RevertReasonParser.sol
- Permitable.sol
- ChainlinkCalculator.sol
- ArgumentsDecoder.sol
- AmountCalculator.sol
- NonceManager.sol
- LimitOrderProtocol.sol
- ImmutableOwner.sol
- InteractiveNotificationReceiver.sol
- AggregatorInterface.sol
- IDaiLikePermit.sol
Усі інші файли та каталоги проектів (включаючи тести), разом із зовнішніми залежностями та проектами, теорією ігор і дизайном стимулів також були виключені зі сфери цього аудиту. Передбачалося, що зовнішній код і залежності контракту працюють як задокументовано, а серверні послуги, які надає 1inch, діятимуть в найкращих інтересах протоколу.
Загальний стан здоров'я
Загалом ми виявили, що кодова база проекту є читабельною та добре організованою, хоча вона може отримати користь від більшої документації, особливо навколо блоків коду складання, граничних випадків протоколу, активів/предикатів/зовнішніх ресурсів, які будуть використовуватися, обов’язки/обмеження наданої серверної служби та взаємодії між учасниками. Проект докладає чимало зусиль, щоб зробити дії ефективними з використання газу, іноді навіть ризикуючи зробити код більш складним для міркування; ми піднімаємо пов’язані з цим питання нижче. Під час аудиту команда 1inch була дуже доступною, чуйною та з нею було дуже легко працювати.
Огляд системи
Протокол лімітного замовлення вмикає порядок makers
для підписання ордерів поза мережею для обміну токенів. Потім протокол полегшує заповнення попередньо підписаних замовлень за замовленням takers
. Замовлення є розширюваними та можуть статично викликати зовнішні контракти в кількох точках протягом усього процесу виконання замовлення. Ця розширюваність наповнює протокол корисністю, але додає як складності, так і більшої поверхні для атаки для самих замовлень.
Важливо зауважити, що деталі замовлення не зберігаються в мережі. Статус заповнення чи скасування замовлень відстежується лише через хеші замовлень. Це вимагає, щоб замовлення передавалися однорангово або через централізовану сторону. У цьому випадку команда 1inch має намір діяти як ця централізована сторона, агрегуючи підписані замовлення та використовуючи ці замовлення як джерело ліквідності для своїх інших протоколів. Замовлення публікуватимуться через власний API, щоб користувачі могли з ними взаємодіяти.
Ця централізація дає команді 1inch надзвичайний контроль над тим, які замовлення публікуються та остаточно виконуються. Це також дає їм можливість цензурувати замовлення, що може бути корисним у разі зловмисних або оманливих наказів, але також може бути зловживано та дозволити їм випередити будь-якого іншого користувача у випадку сприятливого замовлення, не показуючи його через API.
Привілейовані ролі
Незважаючи на те, що контракти, у яких використовується роль, виходили за рамки, було визначено одну привілейовану роль. Ан immutableOwner
встановлено для творця проксі-контракту під час створення та використовується для обмеження доступу до проксі-сервера external
функції.
Зовнішні залежності та довірчі припущення
Конструкція цього протоколу потребує компонентів поза ланцюгом і в ланцюзі, і цю гібридну модель можна використовувати для пом’якшення деяких векторів атак, які ми визначаємо в нашому звіті, але вартість цієї можливості полягає в підвищеній залежності від команди 1inch та інфраструктури.
Крім того, протокол обмежених ордерів надає функції, які призначені для отримання цін від оракулів Chainlink. Ми припускали, що ці оракули чесні, доступні та належним чином функціонують.
Крім того, через гнучкість замовлення є кілька точок контакту із зовнішніми контрактами, які не підтверджуються. Це означає, що зловмисний користувач може зловживати такими викликами та видавати себе за предикати, активи або оракули зі зловмисними контрактами, щоб виконувати дії під час виконання замовлення. Хоча в деяких областях проект захищено від повторного входу, такі вектори можуть спричинити атаки на відмову в обслуговуванні або непомічені замовлення спаму. Команда 1inch усвідомлює, що під час використання незнайомих контрактів для протоколу можуть виникнути певні проблеми, і заявила про свій намір, щоб проект повністю підтримував лише основні активи «блакитних фішок». Однак слід зазначити, що навіть для найпопулярніших активів існує внутрішня поведінка кожного активу, яка може спричинити проблеми з протоколами, які не вирішують їх належним чином, як-от наявність комісії під час переказів за допомогою USDT або повернення коду помилки замість логічне значення успіху з cTokens.
Результати
Тут ми представляємо наші висновки.
Критична тяжкість
Ні.
Високий ступінь тяжкості
[H01] Невідповідні дані передано _makeCall
У OrderMixin
контракт, _makeCall
функція використовується для передачі активів від того, хто бере до виробника , А потім від виробника до того, хто бере. В останній передачі в _makeCall
функція неправильно передала порядок makerAsset
як останній параметр, коли це має бути замовлення makerAssetData
.
У результаті будь-яка функція проксі-сервера, яка покладається на makerAssetData
суперечка зірветься.
Щоб бути узгодженим із попереднім викликом до _makeCall
і щоб повністю підтримувати функціональність проксі, розгляньте можливість оновлення order.makerAsset
параметр до order.makerAssetData
.
Оновлення: Зафіксовано в Запит на витяг #57.
[H02] Частково виконані приватні замовлення можуть бути виконані будь-ким
Протокол дозволяє створювати приватні та публічні замовлення. На приватні замовлення, тільки allowedSender
адреса, зазначена виробником при створенні замовлення, дає можливість виконати замовлення.
Однак у OrderMixin
договір, перевірка для allowedSender
адреса має неправильну область, що означає, що він оцінюється лише всередині логіки, яка обробляє перше заповнення замовлення. Якщо приватне замовлення виконано частково, то чек на allowedSender
адреса більше недоступна, і замовлення стає доступним для виконання будь-ким.
Щоб уточнити намір щодо того, чи повинен будь-який користувач мати можливість виконувати частково виконані приватні замовлення чи ні, подумайте про документування причини поточної поведінки або перевірку allowedSender
адреса поза межами першого заповнення, щоб переконатися, що він перевірятиметься під час кожної спроби заповнення.
Оновлення: Зафіксовано в Запит на витяг #58.
[H03] Зловмисник може скористатися перевагами часткового заповнення, щоб викрасти активи користувача
Замовлення від в OrderMixin
договір мають можливість бути частково заповненими. Для підтримки часткових заповнень протокол вимагає способу обчислення обох сторін обміну. Обидва getMakerAmount
та getTakerAmount
поля визначаються виробником замовлення саме для цієї мети.
Під час заповнення замовлення одержувачі повинні надати або makingAmount
або takingAmount
значення, а також a thresholdAmount
значення. Існує два різних шляхи коду, якими можна скористатися залежно від того, чи є makingAmount
або takingAmount
було надано.
Перший - це коли makingAmount
параметр визначено. Це може усікати makingAmount
значення, а також обчислити takingAmount
значення для нього. У цій ситуації thresholdAmount
забезпечує, що takingAmount
прийняте значення є не несподівано великий.
Другий - коли takingAmount
параметр визначено. У такому випадку буде обчислити makingAmount
значення, з можливістю о скорочуючи його та перерахунок takingAmount
значення, якщо це станеться. У цій ситуації thresholdAmount
значення гарантує, що makingAmount
повертається значення не несподівано малий.
Існують два методи використання, кожен з яких унікальний для одного з попередньо згаданих кодових шляхів. Ці методи використання вимагають зловмисників getMakerAmount
та getTakerAmount
функції. Проста реалізація цих функцій матиме ідентичну поведінку AmountCalculator
с getMakerAmount
та getTakerAmount
функції, але з жорстко закодованим перемикачем, який змушуватиме їх повертати контрольоване зловмисником значення, коли це необхідно.
Перший, менш серйозний шаблон експлойту включає перший кодовий шлях, де makingAmount
вказано значення у порядку заповнення. Шкідливий розробник чекатиме на наказ про заповнення, який визначає makingAmount
щоб з’явитися в mempool, щоб стати лідером. Вони злили б усе значення, крім 1, зі сторони виробника, а потім примусово _callGetTakerAmount
повернути суму, вказану користувачем thresholdAmount
значення (або їх надбавка, якщо вона менша). Коли транзакція користувача нарешті пройде, вони обміняються своїми повними місцями thresholdAmount
варто takerAsset
за одну одиницю makerAsset
. Цей експлойт обмежений сумою, яку надає thresholdAmount
значення або кількість takerAsset
користувач дозволив на LimitOrderProtocol
договір.
Другий, більш серйозний шаблон експлойту включає другий кодовий шлях, де takingAmount
вказано значення. Творець зловмисника так само чекатиме замовлення на заповнення, у якому зазначено a takingAmount
значення для відображення в mempool. Вони б ініціювали транзакцію та змусили makingAmount
значення, яке повертає _callGetMakerAmount
функція бути вищою, ніж обидва remainingMakerAmount
і thresholdAmount
. Вони б також встановили takingAmount
повернуте значення _callGetTakerAmount
бути сумою takerAsset
актив, дозволений на LimitOrderProtocol
беручим. Коли транзакція тейкера пройде, це буде обрізати makingAmount
значення, а потім перерахувати takingAmount
значення. Однак цей перерахунок не гарантовано буде нижчим, і в цьому випадку користувач витягне всі takerAsset
що вони дозволили на договір. У цьому кодовому шляху, thresholdAmount
значення є забезпечення того, що makingAmount
не є надто низьким, тому беручи всіх тих, хто бере takerAsset
актив не перевірено. Втрачені кошти обмежені сумою takerAsset
актив, який користувач дозволив на LimitOrderProtocol
договір.
Ці експлойти неможливі без часткових замовлень і, точніше, часткових замовлень зі зловмисниками getMakerAmount
та getTakerAmount
реалізації.
Основне питання в thresholdAmount
Перевірка значення полягає в тому, що вона охоплює лише одну сторону свопу, але іншою стороною можна маніпулювати через frontrunning. Немає ніяких гарантій, що вартість, запропонована покупцем, залишиться незмінною. Розгляньте можливість видалення makingAmount
усікання з обох кодових шляхів і повернення, якщо порядок не підтримує заповнення такого розміру, як запитувано. Роблячи це, thresholdAmount
можна використовувати для достатнього обмеження іншої сторони обміну та уникнення неочікуваної поведінки, навіть у зловмисних замовленнях.
Оновлення: Зафіксовано в Запит на витяг #83.
Середня тяжкість
[M01] Статичні аргументи, передані після динамічних аргументів
У OrderMixin
контракт, getTakerAmount
та getMakerAmount
поля bytes використовуються як аргументи для _callGetTakerAmount
та _callGetMakerAmount
функції. Ці виклики забезпечують спосіб обчислення однієї сторони свопу на основі іншої сторони, і вони дозволяють користувачам частково виконувати замовлення.
Команда getTakerAmount
/getMakerAmount
поля є динамічними змінними і упаковуються перед takerAmount
та makerAmount
значення в _callGetTakerAmount
та _callGetMakerAmount
функції. Зловмисник може надати більше даних, ніж очікувалося, у getTakerAmount
та getMakerAmount
поля для штовхання takerAmount
та makerAmount
байтів минуло, де вони передбачаються під час декодування в наступній функції. Це дозволяє розробнику зсунути передану кількість тейкера або мейкера на цілий байт праворуч і навіть повністю замінити їх, якщо надано додаткові 32 байти даних.
Користувачі вже повинні вручну переглянути getTakerAmount
та getMakerAmount
поля в порядку, але цю техніку досить важко помітити. Також варто зазначити, що ця атака стосується навіть внутрішньо довірених осіб getMakerAmount
та getTakerAmount
функції. Для більшості атак надання розумної порогової суми запобіжить втраті коштів.
Щоб запобігти цьому, подумайте про кодування статичних аргументів перед динамічними, щоб уникнути надання динамічним аргументам методу керування статичними аргументами.
Оновлення: Не виправлено. Команда 1inch заявила:
Ми будемо особливо обережні з перевіркою гетерів. Ми спробуємо реалізувати перевірку працездатності геттерів у нашому SDK, що допоможе фільтрувати потенційно зловмисні замовлення.
[M02] Наказами ERC721 можна маніпулювати
Можна обміняти не тільки ERC20 через OrderMixin
шляхом розгортання контракту, який використовує той самий селектор функцій, що й IERC20 transferFrom
, і надання цього контракту як makerAsset
або takerAsset
в порядку.
Проксі-сервери поза областю, а саме: ERC721Proxy
, ERC721ProxySafe
та ERC1155Proxy
контракти дотримуються цієї моделі, щоб забезпечити підтримку ERC721
та ERC1155
жетони. Оскільки проксі-сервери мають викликатися за тим самим шаблоном, що й IERC20 transferFrom
виклику, підпис повинен починатися з address from
, address to
та uint256 amount
. Все інше, що вимагається проксі-серверами, може бути передано після, і визначається в порядку як makerAssetData
та takerAssetData
.
ERC1155, природно, може передавати кілька однакових ідентифікаторів одночасно, що означає ERC1155Proxy
договір використовує amount
поле. З іншого боку, ERC721
s не мають очевидного використання для amount
поле. Оскільки вони представляють невзаємозамінні токени, конкретний tokenId матиме лише один існувати, відображаючи amount
поле марно. Через це реалізація для обох ERC721Proxy
та ERC721ProxySafe
контракти використовують необхід amount
поле як tokenId
замість цього.
Це перевантаження amount
параметр створює можливість часткового заповнення ERC721
замовлення, щоб придбати окремо вказані токени за зниженими цінами. Наприклад, може бути випадок, коли один користувач має кілька ERC721
s того самого договору, дозволеного для передачі ERC721Proxy
контракт і перераховує їх в окремих лімітних замовленнях.
Якщо лімітні замовлення також забезпечують getMakerAmount
та getTakerAmount
поля, їх можна буде частково заповнити ERC721
замовлення. З часу наказу amount
поле фактично відповідає tokenId
, зловмисник може частково заповнити ERC721
з вищим tokenId, що призводить до a makingAmount
/takingAmount
з ERC721
що може відповідати нижчому tokenId
. Результатом є ERC721
з нижньою tokenId
буде передано за ціною (higher tokenId price) * (lower tokenId's id) / (higher tokenId's id)
.
Цей експлойт має кілька вимог:
- множинний
ERC721
s з того самого договору, що дозволяється на будь-якомуERC721
доручення від одного власника. - Відкрити замовлення для одного з
ERC721
s це не найнижчийtokenId
із дозволених. - Допускається часткове заповнення замовлення.
Повністю прибрати можливість часткового ERC721
заповнює, подумайте про відокремлення amount
та tokenId
аргументи. Незалежно від того, розділені аргументи чи ні, подумайте про документування цього, щоб попередити користувачів про таку поведінку та уникнути подібної моделі в майбутньому.
Оновлення: Зафіксовано в Запит на витяг #59.
[M03] Недокументовані десяткові припущення
Команда LimitOrderProtocol
договір успадковує ChainlinkCalculator
договір через в OrderMixin
договір. Цей контракт надає дві функції, щоб дозволити використання оракулів Chainlink під час перевірка предикатів і пошук сума виробника/бере суму.
Однак контракт містить незадокументовані припущення щодо кількості десяткових знаків, які мають повідомляти оракули Chainlink, а також кількості десяткових знаків, які мають містити параметри функції. За певних сценаріїв це може призвести до неочікуваної поведінки, зокрема до неправильної оцінки активів і ненавмисної втрати коштів.
Точніше, в усьому контракті неявним припущенням є те, що оракули Chainlink повідомлятимуть із точністю до 18 знаків після коми. Однак ні усі оракули Chainlink звіт із цією кількістю десяткових знаків. Насправді, якщо оракул повідомляє про пару токенів у валюті (наприклад, долар США), він матиме лише 8 знаків після коми. Так як обмежень немає який можна використовувати оракули, не слід робити неявних припущень щодо кількості десяткових дробів, які вони повідомлятимуть.
Відповідно, існує неявне припущення, що amount
параметр для ChainlinkCalculator
функції використовуватимуть 18 десяткових знаків разом із оманливим явним оголошенням того, що singlePrice
функція Calculates price of token relative to ETH scaled by 1e18
. Насправді, навіть з оракулом, що робить звіт із 18 десятковими дробами, значення, що повертається singlePrice
функція буде масштабована за кількістю десяткових знаків amount
параметр, який не обов’язково може мати 18 знаків після коми.
Аналогічним чином, doublePrice
функція припускає, що два оракули Chainlink повідомлятимуть однакову кількість десяткових знаків, через що результат функції відхилятиметься від очікувань.
Подумайте про те, щоб чітко задокументувати припущення щодо кількості десяткових знаків, якими мають бути параметри та значення, що повертаються. Крім того, подумайте про обмеження обчислень, які залежать від оракулів, які порушують ці припущення, або про те, щоб відповідні обчислення враховували фактичну кількість десяткових дробів.
Оновлення: Зафіксовано в Запит на витяг #75.
Низька тяжкість
[L01] Константи, не оголошені явно
Є кілька випадків використання літеральних значень із нез’ясованим значенням у кодовій базі. Наприклад:
- У
OrderMixin
контракт,_remaining
відображення семантично перевантажене (як пояснюється в випуску Семантичне перевантаження відображення), щоб відстежувати кількість активів, що залишилися для частково виконаного замовлення а також якщо замовлення виконано повністю. Зокрема,0
означає, що жодних заповнень, пов’язаних із замовленням, не було зроблено,1
означає, що замовлення більше не може бути виконано, і все, що перевищує1
означає, що із замовленням є залишок суми, який потенційно можна виконати. - У
ChainlinkCalculator
договір, буквальне значення1e18
використовується вsinglePrice
функції.
Щоб покращити читабельність коду та полегшити рефакторинг, розгляньте можливість визначення константи для кожного магічного числа, давши йому чітку та зрозумілу назву. Для складних значень варто додати вбудований коментар, що пояснює, як вони були обчислені або чому їх вибрано.
Оновлення: Зафіксовано в Запит на витяг #75 та Запит на витяг #76.
[L02] Зловмисники можуть перешкоджати виконанню дозволених наказів
Команда OrderMixin
Контракт дозволяє користувачам виробника надсилати допустимі накази тому їх можна виконати в одній транзакції, замість того, щоб мати окрему транзакцію для схвалення. Крім того, приймачі замовлень можуть подати власний дозвіл під час виконання замовлення з тією ж метою.
Однак, оскільки дозвіл виробника міститься всередині порядок, дозволи як виробника, так і отримувача будуть доступні, поки транзакція виконання замовлення знаходиться в mempool. Це дасть змогу будь-якому зловмисному користувачеві взяти ці дозволи та виконати їх щодо відповідних контрактів активів, одночасно керуючи транзакцією заповнення. Оскільки ці дозволи мають a nonce
щоб запобігти атаці подвійного витрачання, транзакція заповнення замовлення завершиться невдало в результаті спроби використати той самий дозвіл, який щойно використовувався під час початкового випуску.
Хоча загрози безпеці немає, і виробник може створити нове замовлення та попередньо схвалити транзакцію, ця атака, безумовно, може вплинути на зручність використання дозволених замовлень. Дійсно, мотивований нападник міг заблокувати всі дозволені накази з цією атакою. Під час заповнення замовлення подумайте про підтвердження, якщо дозвіл уже надіслано або якщо надбавки достатньо. Також варто повідомити користувачів про цю можливу атаку під час створення замовлення.
Оновлення: Не виправлено. Команда 1inch заявляє:
Раніше ми проводили перевірки схвалення, але вирішили спростити потік дозволів, щоб просто повертатися після невдалих схвалень. Будемо думати, як повідомити виробників про проблему.
[L03] Дубльований код
У кодовій базі є екземпляри дубльованого коду. Дублювання коду може призвести до проблем у подальшому життєвому циклі розробки та робить проект більш схильним до появи помилок. Такі помилки можуть ненавмисно виникнути, коли зміни функціональності не відтворюються в усіх екземплярах коду, який має бути ідентичним. Приклади дубльованого коду:
Замість того, щоб дублювати код, подумайте про наявність лише одного контракту чи бібліотеки, що містить дубльований код, і використовуйте його щоразу, коли дубльована функція потрібна.
Оновлення: Частково закріплено в Запит на витяг #60.
[L04] Помилковий або оманливий набір тестів
У наборі тестів є випадки, коли тести відхиляються від очікуваної поведінки. Наприклад:
- Команда
ChainlinkCalculator
договір успадковуєтьсяOrderMixin
договір. Однак під час випробувань вAmountCalculator.arbitraryStaticCall
функція використовується для викликуChainlinkCalculator
договір як зовнішній, самостійний договір. Незважаючи на те, що результат є очікуваним, тест повинен відображати поведінку з поточним дизайном системи та очікуваним випадком використання шляхом викликуChainlinkCalculator
функціонує безпосередньо без використання довільного статичного виклику. - Хоча проксі-контракти виходили за рамки, ми помітили, що під час тестування протоколу з активами ERC721
ERC721Proxy
договір не використовується для обміну активами в його тестовий набір.
Оскільки сам набір тестів виходить за межі цього аудиту, будь ласка, подумайте про ретельну перевірку набору тестів, щоб переконатися, що всі тести виконуються успішно відповідно до специфікацій протоколу.
Оновлення: Зафіксовано в Запит на витяг #57, Запит на витяг #59 та Запит на витяг #61.
[L05] Помилки та упущення в подіях
У всій кодовій базі події, як правило, створюються, коли в контракти вносяться важливі зміни. Однак для багатьох подій відсутні проіндексовані параметри та/або відсутні важливі параметри. Наприклад:
Існують також конфіденційні дії, у яких відсутні події, наприклад:
Подумайте про повніше індексування існуючих подій і додавання нових параметрів там, де їх бракує. Крім того, подумайте про випромінювання всіх подій таким повним чином, щоб їх можна було використовувати для відновлення стану контракту службами поза мережею.
Оновлення: Не виправлено. Однак команда 1inch додала orderRemaining
параметр до OrderCanceled
подія в Запит на витяг #62.
Команда 1inch заявляє:
Ми виявили, що для задоволення потреб інтерфейсу потрібна лише обмежена частина даних. У разі детального аналізу всі запропоновані поля доступні через трасування. для
OrderRFQMixin
ми очікуємо, що маркет-мейкери створять власний складний спосіб відстеження того, які замовлення були скасовані.
[L06] Зміни зберігання під час видачі події
У NonceManager
договір, коли в NonceIncreased
випромінюється подія, nonce відправника повідомлення також збільшено.
Виконання кількох операцій одночасно може зробити базу коду складнішою для міркувань, більш схильною до помилок і може призвести до того, що операції не будуть помічені або неправильно зрозумілі.
Щоб покращити загальну навмисність, читабельність і ясність коду, подумайте про збільшення значення nonce перед тим, як видавати подію.
Оновлення: Зафіксовано в Запит на витяг #63.
[L07] Неузгоджені методології декодування можуть спричинити розбіжності в результатах
Щоб підтримувати всю свою розширюваність і гнучкість, протокол лімітного замовлення регулярно має працювати з даними динамічних байтів і довільними значеннями, що повертаються із зовнішніх контрактів. У результаті протокол містить ан ArgumentsDecoder
бібліотека для більш ефективного перетворення динамічних значень байтів у базові типи даних. Однак ця бібліотека використовується не виключно, а в деяких випадках abi.decode
замість нього використовується. Крім того, використовуються деякі контракти abi coder v1
поки інші використовують abi coder v2
. Перший діє більш подібно до ArgumentsDecoder
бібліотеки, тоді як остання виконує додаткові перевірки при декодуванні.
Непослідовне використання цих різних методологій декодування може призвести до тонких розбіжностей між наміром і фактичною поведінкою кодової бази.
Наприклад, simulateCalls
функція використовує лише ArgumentsDecoder.decodeBool
функція. Якщо simulateCalls
функція використовується для перевірки викликів, які будуть зроблені в предикатній частині замовлення, тоді її результати можуть відхилятися від того, що насправді відбувається під час оцінки предикатних умов, оскільки використовуються різні методології декодування.
Так, наприклад, якщо предикат створює зовнішнє staticcall
до деякої функції, яка повертає a uint256
значення більше за одиницю, а не очікуване bool
, тоді цей виклик буде скасовано, оскільки повертається значення розшифровано с abi coder v2
с abi.decode
яка не сприйматиме таких значень, як bool
. Однак, якщо такий самий дзвінок зроблено з simulateCalls
, то це буде просто позначено як true
, Так як decodeBool
обробляє будь-яке значення більше нуля як true
.
Зробити simulateCalls
функція повністю відображає поведінку фактичних викликів предикатів, розгляньте можливість її модифікації для використання abi.decode
.
Оновлення: Зафіксовано в Запит на витяг #82.
[L08] Відсутність перевірки введених даних
Команда fillOrderToWithPermit
та fillOrderTo
функції OrderMixin
договір, а також ст fillOrderRFQToWithPermit
та fillOrderRFQTo
функції OrderRFQMixin
договір, не підтверджуйте target
параметр адреси.
Це дає змогу користувачеві ненавмисно передати нульову адресу та, як наслідок, заблокувати активи, які він має отримати після виконання замовлення.
Щоб гарантувати, що користувачі випадково не заблокують свої кошти, перевірте, що target
адреса не дорівнює нульовій адресі у цитованих функціях.
Оновлення: Зафіксовано в Запит на витяг #78.
[L09] Низьке покриття модульного тесту
Охоплення одиничним тестуванням для всього проекту становить близько 75%, причому деякі з контрактів мають особливо низьке покриття.
Враховуючи важливість модульних тестів для перевірки коду та запобігання регресіям під час рефакторингу та розробки нових функцій, ми заохочуємо значно збільшити охоплення модульними тестами принаймні до 95%, включаючи крайові випадки, які охоплюють навіть малоймовірні ситуації.
Оновлення: Не виправлено.
[L10] Оманлива або неповна вбудована документація
У всій кодовій базі було виявлено кілька випадків оманливої та/або неповної вбудованої документації, і їх слід виправити.
Нижче наведено приклади оманливої внутрішньої документації:
- У
ChainlinkCalculator
контракт,singlePrice
функції NatSpec@notice
тег каже, що цеCalculates price of token relative to ETH scaled by 1e18
, але насправді її результатом є значення ofamount
токени, масштабовані за1e18
, де оракул може не повідомляти в термінах ETH (наприклад, для пари, яка не включає ETH). - У
OrderRFQMixin
контракт,invalidatorForOrderRFQ
функції NatSpec@return
тег вводить в оману, оскільки лапка може бути не заповнена для встановлення відповідного біта недійсності. Замовлення також можна було скасувати. - На лініях 147, 165 та 188 of
OrderMixin.sol
, NatSpec@param
теги неграматичні. - On line 20 of
ERC1155Proxy.sol
,@notice
тег стверджує, що обчислений хеш є результатом хешуванняfunc_733NCGU
функцію, де вона має бутиfunc_301JL5R
замість цього.
Нижче наведено приклади неповної вбудованої документації:
- Функції в
AmountCalculator
контракт не описує жодного з параметрів. - У
ChainlinkCalculator
контракт,singlePrice
таdoublePrice
функції не описують усі параметри. - У
ImmutableOwner
договір, публічна змінна та модифікатор не мають NatSpec. - У
InteractiveNotificationReceiver
контракт,notifyFillOrder
функція не описує жодного з параметрів. - У
LimitOrderProtocol
контракт,DOMAIN_SEPARATOR
функція не має NatSpec. - Події та відображення в
NonceManager
не мають NatSpec. - У
OrderRFQMixin
договір,cancelOrderRFQ*
функції не описують значення, що повертаються. - У
OrderMixin
у деяких функціях відсутній повний NatSpec. - On line 168 of
OrderMixin.sol
і в режимі онлайн 71 ofOrderRFQMixin.sol
, не вистачає@dev
бирка. - Функції в
PredicateHelper
контракт не описує всі параметри.
Чітка вбудована документація є фундаментальною для окреслення намірів коду. Невідповідності між вбудованою документацією та реалізацією можуть призвести до серйозних помилок щодо очікуваної поведінки системи. Подумайте про виправлення цих помилок, щоб уникнути плутанини для розробників, користувачів і аудиторів.
Оновлення: Частково виправлено. Документація, що вводить в оману, адресована Запит на витяг #75 та Запит на витяг #77.
Команда 1inch заявляє:
Ми виправили документи, що вводять в оману. Доопрацювання документів буде здійснено пізніше.
[L11] Накази DoS можливі при використанні хуків
Команда OrderMixin
Контракт реалізує функціональні можливості для виконання загальних замовлень своп поза мережею, які можуть мати умови для їх успіху. Під час заповнення замовлення замовлення може перевірте попередньо визначені умови «предикату». перед продовженням виконання.
Однак, оскільки ці предикатні умови можуть бути спрямовані на логіку будь-якого довільного контракту, зловмисник може змусити одержувачів повірити, що замовлення поводиться правильно та що воно дійсне під час перевірки його поза ланцюжком, але потім зазнає невдачі при спробі виконати те саме замовлення по ланцюгу. Ця зміна в поведінці предикату може бути зроблена або за допомогою початкового запуску деякого стану змінної, від якого залежать предикати, шляхом перевірки надісланого газу чи навіть того, які адреси беруть участь у виклику, або за допомогою іншої логіки.
Крім того, якщо виробник визначив a взаємодія під час обміну, interactionTarget
договір може скасувати сам себе або скасувати дозвіл, щоб запобігти успішному виконанню замовлення, що, по суті, призведе до того самого результату, що й зловмисні предикати.
Незважаючи на те, що активи не будуть під загрозою, користувачі або боти, які знайдуть вигідне замовлення, матимуть додатковий тягар, намагаючись ідентифікувати такі спам-замовлення, які на перший погляд можуть здатися законними. У випадку, якщо їм не вдасться визначити ці замовлення, вони понесуть витрати газу. Щоб зменшити кількість спам-замовлень, подумайте про обмеження доступних цілей для цих хуків. Також варто попередити користувачів про таку можливість, перш ніж вони спробують виконати замовлення.
Оновлення: Не виправлено. Команда 1inch заявляє:
Ми вирішуємо це на нашому сервері та подумаємо про способи сповіщення можливих користувачів про проблему.
[L12] Округлення може бути несприятливим для taker
У OrderMixin
та OrderRFQMixin
контракти, коли замовлення виконується, а одержувач надає лише a makingAmount
or takingAmount
сума, протокол намагається обчислити відповідну суму свопу.
Є дві проблеми з цими обчисленнями, перша полягає в тому, що немає документації чи логіки, що обмежує кількість десяткових знаків, які мають використовувати параметри суми, про що ми говорили в Недокументовані десяткові припущення проблема.
Друга проблема полягає в тому, що під час цих розрахунків протокол змінюється на користь виробника. Проблема округлення може бути значно загострена, якщо неявні десяткові припущення порушені, але навіть якщо все відповідає очікуваним умовам, округлення відбуватиметься з невеликими непарними сумами.
Розгляньте можливість дозволити одержувачу вказати мінімальну суму makerAsset
актив, який вони готові отримати разом із максимальною сумою takerAsset
актив, який вони готові обміняти, щоб прийняти будь-яке округлення було більш чітким.
Оновлення: Не виправлено. Команда 1inch заявляє:
Порогової суми має бути достатньо для захисту тейкера.
[L13] Суперечлива обробка замовлень за відсутності параметрів
У OrderMixin
контракт, fillOrderTo
функція здійснює внутрішні виклики до _callGetMakerAmount
та _callGetTakerAmount
функціонує щоразу, коли виконується спроба заповнення, або makingAmount
або takingAmount
параметри дорівнюють нулю, відповідно, або якщо makingAmount
значення більше, ніж remainingMakerAmount
value.
Команда _callGetMakerAmount
та _callGetTakerAmount
виклики призведуть до повернення, якщо замовлення не було створено за допомогою getMakerAmount
or getTakerAmount
параметрів, відповідно, і виконується часткове заповнення.
An вбудований коментар поруч _callGetMakerAmount
і вбудований коментар поруч _callGetTakerAmount
стверджувати, що «дозволені лише цілі заповнення», якщо замовлення не було створено за допомогою getMakerAmount
or getTakerAmount
параметри
Однак є шляхи коду, до яких це не стосується, оскільки ці шляхи не перевіряють length
s обох getMakerAmount
та getTakerAmount
параметри
Зокрема, коли a taker
вказує a takerAmount
значення для замовлення, яке має лише a getMakerAmount
, хіба що дзвонити до getMakerAmount
повертає суму, більшу ніж remainingMakerAmount
, часткове заповнення може бути виконано всупереч вбудованій документації.
Це залишає неясним цілеспрямованість цих кодових шляхів. Якщо це очікувана поведінка, розгляньте можливість змінити вбудовану документацію, щоб вона була більш чіткою. Якщо це ненавмисна поведінка, завжди перевіряйте довжину обох getMakerAmount
і getTakerAmount
параметри одночасно, щоб реалізація посилювала поведінку, описану вбудованою документацією.
Оновлення: Зафіксовано в Запит на витяг #79.
[L14] Використання застарілих викликів Chainlink
Команда ChainlinkCalculator
контракт призначений для використання для запиту оракулів Chainlink. Це робиться через здійснення дзвінків на їх latestTimestamp
та latestAnswer
методи, обидва з них застаріли. Фактично, методи більше не присутні в API агрегаторів Chainlink починаючи з третьої версії.
Щоб уникнути можливої майбутньої несумісності з оракулами Chainlink, розгляньте можливість використання latestRoundData
натомість метод.
Оновлення: Зафіксовано в Запит на витяг #67.
Примітки та додаткова інформація
[N01] Інтерфейси не імпортуються
Команда AggregatorInterface
інтерфейс виглядає як підмножина коду, скопійованого з ChainLink
загальнодоступне сховище коду. Повний інтерфейс включено ChainLink
Пакет контракту npm.
Якщо це можливо, щоб зменшити ймовірність невідповідності інтерфейсів і пов’язаних з цим проблем, замість того, щоб повторно визначати та/або переписувати інтерфейси іншого проекту, подумайте про використання інтерфейсів, встановлених через їхні офіційні пакети npm.
Оновлення: Зафіксовано в Запит на витяг #66.
[N02] Застарілі залежності проекту
Під час монтажу залежності проекту, NPM попереджає, що один із встановлених пакетів, Highlight
, «більше не підтримуватиметься та не отримуватиме оновлень безпеки в майбутньому».
Навіть якщо малоймовірно, що цей пакет може спричинити загрозу безпеці, подумайте про оновлення залежності, яка використовує цей пакет, до підтримуваної версії.
Оновлення: Зафіксовано в Запит на витяг #64.
[N03] Зовнішні виклики методів перегляду не є статичними викликами
У більшій частині кодової бази протокол явно здійснює зовнішні виклики через OpenZeppelin functionStaticCall
метод обмеження можливості змін стану, коли вони або не очікуються, або небажані. Проте в ChainlinkCalculator
контракту, незважаючи на намір здійснювати зовнішні дзвінки лише до view
методи на оракулах Chainlink, зовнішні виклики в singlePrice
та doublePrice
функції не створюються через явні staticcall
s.
Хоча ми не виявили жодних безпосередніх проблем безпеки, що випливають із цього, щоб зменшити поверхню атаки, покращити узгодженість і прояснити наміри, розгляньте можливість використання явного staticcall
s, для всіх зовнішніх дзвінків до view
функцій у ChainlinkCalculator
договір.
Оновлення: Не виправлено. Команда 1inch заявляє:
Ми вважаємо, що ускладнення синтаксису зводить нанівець покращення узгодженості.
[N04] Недострокова помилка через недійсні замовлення
У OrderMixin
контракт, fillOrderTo
функція обробляє спеціальну умову, коли замовлення раніше не було подано (remainingMakerAmount == 0
), але він явно не обробляє умову, коли замовлення більше не дійсне (remainingMakerAmount == 1
).
В останньому сценарії функція зрештою повернеться, але лише після спалювання нетривіальної кількості газу. Щоб уточнити наміри, підвищити читабельність і зменшити споживання газу, розгляньте можливість явної обробки сценарію недійсного порядку на початку функції.
Оновлення: Зафіксовано в Запит на витяг #68.
[N05] Допоміжні контракти, не позначені як абстрактні
У Solidity ключове слово abstract
використовується для контрактів, які або не є функціональними контрактами самі по собі, або не призначені для використання як такі. Натомість abstract
контракти успадковуються іншими контрактами в системі для створення автономних функціональних контрактів.
У всій кодовій базі є приклади допоміжних контрактів, які не позначені як абстрактні, незважаючи на те, що вони не призначені для самостійного розгортання. Наприклад, AmountCalculator
, ChainlinkCalculator
, ImmutableOwner
, NonceManager
та PredicateHelper
усі контракти складаються з базового набору функцій, які призначені для використання у спадкових контрактах.
Розгляньте позначення допоміжних контрактів як abstract
щоб чітко вказати, що вони призначені виключно для додавання функціональності до контрактів, які їх успадковують.
Оновлення: Не виправлено. Команда 1inch заявляє:
Ці помічники можна розгортати окремо. Вони передаються у спадок лише за економію газу.
[N06] Непослідовне впорядкування функцій
У всій кодовій базі порядок функцій зазвичай відповідає рекомендований порядок у посібнику зі стилю Solidity, який: constructor
, fallback
, external
, public
, internal
, private
.
Проте в OrderMixin
контракт, public
checkPredicate
функція відхиляється від керівництва по стилю, розділяючи навпіл external
функції.
Щоб покращити загальну розбірливість проекту, подумайте про стандартизацію порядку функцій у всій кодовій базі, як рекомендовано в посібнику зі стилю Solidity.
Оновлення: Зафіксовано в Запит на витяг #69.
[N07] Непослідовний потік заповнення замовлення
Команда OrderMixin
та RFQOrderMixin
обидва контракти обслуговують виконання підписаних замовлень, але загальний потік замовлень між двома контрактами непослідовний.
OrderMixin
с fillOrderTo
функція дотримується цього загального потоку (псевдокод):
if ((takingAmount == 0) == (makingAmount == 0))
else if (takingAmount == 0)
else (handle makingAmount == 0) THEN swapTokens
в той час як RFQOrderMixin
аналогічний fillOrderRFQTo
функція слідує цьому потоку (псевдокод):
if (takingAmount == 0 && makingAmount == 0)
else if (takingAmount == 0)
else if (makingAmount == 0)
else revert THEN swapTokens
З документації немає жодних уявлень про те, чому перша умова в кожній із цих двох функцій відрізняється, або чому takingAmount
та makingAmount
обидва не можуть дорівнювати нулю в останній функції. Крім того, випадок, коли обидва a makingAmount
і takingAmount
надаються набагато легше міркувати про в fillOrderRFQTo
функція, оскільки вона чітко обробляється у фіналі else
блок
Щоб уточнити наміри та підвищити загальну читабельність коду, подумайте про стандартизацію загального потоку замовлення в цих двох контрактах або чітке документування, чому існують відмінності.
Оновлення: Не виправлено. Команда 1inch заявляє:
Це пов’язано зі спеціальними функціями ціноутворення в лімітованих замовленнях. Оскільки
getMakerAmount
потенційно може суттєво відрізнятися відgetTakerAmount
, ми подумали, що краще не робити опцію за замовчуванням для приймача, оскільки це, ймовірно, заплутає їх у випадках, коли ці отримувачі будуть іншими.
[N08] Повідомлення про помилки невідповідно відформатовані або вводять в оману
У всій кодовій базі, require
та revert
Повідомлення про помилки, призначені для сповіщення користувачів про конкретні умови, що призводять до збою транзакції, виявилися невідповідним форматом.
Наприклад, кожне з повідомлень про помилки в рядках 85 з OrderMixin.sol
, 16 з ERC721ProxySafe.sol
та 26 з Permitable.sol
використовувати інший стиль.
Крім того, деякі повідомлення про помилки вводять в оману:
Повідомлення про помилки призначені для сповіщення користувачів про помилкові умови, тому вони повинні надавати достатньо інформації, щоб можна було внести відповідні виправлення для взаємодії з системою. Неінформативні повідомлення про помилки значно шкодять загальному досвіду користувача, тим самим знижуючи якість системи. Крім того, невідповідно відформатовані повідомлення про помилки можуть внести непотрібну плутанину. Тому подумайте про перегляд усієї кодової бази, щоб переконатися, що кожен require
та revert
оператор містить повідомлення про помилку, яке є незмінно відформатованим, точним, інформативним і зручним для користувача.
Оновлення: Частково закріплено в Запит на витяг #81.
[N09] Непослідовне використання іменованих повертаних змінних
Існує непослідовне використання іменованих повертаних змінних у OrderMixin
договір. Деякі функції повертає іменовані змінні, інші повертати явні значення, і інші оголосити іменовану змінну повернення, але перевизначити її з явним оператором return.
Розгляньте можливість узгодженого підходу до повернення значень у всій кодовій базі, видаливши всі іменовані змінні повернення, явно оголосивши їх як локальні змінні та додавши необхідні оператори повернення, де це необхідно. Це покращить чіткість і читабельність коду, а також може допомогти зменшити регресії під час майбутніх рефакторів коду.
Оновлення: Зафіксовано в Запит на витяг #73.
[N10] Розрахунок хешу замовлення не відкритий для API
Команда external
Функції remaining
, remainingRaw
та remainingsRaw
усі очікують хеш замовлення для успішної операції.
Однак допоміжна функція _hash
, який повертає хеш замовлення, має private
видимість. Це означає, що користувачам доведеться вручну запакувати частини замовлень і доменних рядків, щоб отримати хеш замовлення.
Щоб уникнути потенційних помилок під час обчислення хешів замовлення та надати користувачам метод генерації відповідного хешу замовлення, подумайте про розширення видимості _hash
функція до public
і рефакторинг назви hash
відповідати решті коду.
Оновлення: Зафіксовано в Запит на витяг #74.
[N11] Семантичне перевантаження відображення
Команда _remaining
відображення в OrderMixin
Контракт семантично перевантажений, щоб відстежувати статус замовлень і залишкову кількість активів, доступних для цих замовлень.
Три стани, які він може прийняти:
0
: Хеш замовлення ще не бачили.1
: Замовлення скасовано або повністю виконано.- будь-який
uint
більший за1
: НагадуванняmakerAmount
доступні для заповнення на замовлення плюс1
.
Це семантичне перевантаження вимагає обертання та розгортання цього значення під час lookup
, cancellation
, initialization
та storage
.
Семантичне перевантаження та необхідна логіка для його ввімкнення можуть бути схильні до помилок і можуть ускладнити розуміння кодової бази та міркувати про неї, це також може відкрити двері для регресії в майбутніх оновленнях коду.
Щоб покращити читабельність коду, розгляньте можливість відстеження стану виконання замовлень в окремому відображенні.
Оновлення: Не виправлено. Команда 1inch повідомила, що виправлення збільшить витрати на газ для fillOrder
функції.
[N12] Замовлення з дозволом дозволяють звернення до довільних контрактів
Команда OrderMixin
договір успадковує Permitable
договір, що дозволяє заповнювати замовлення на одну транзакцію активами, які приймають такі permit
заклики змінити надбавки.
Однак дзвінки до Permitable
контракт не перевіряйте, чи є ціль дозволеним активом, а також чи є він навіть активом, який може дозволити зловмисному користувачеві передати адресу довільного контракту, який може виконати інший виклик до завершення виконання замовлення.
Хоча договір є захищений від повторного входу, завжди рекомендується зменшувати поверхню атаки та запобігати викликам зовнішніх контрактів під час виконання. Розгляньте можливість обмеження активів, залучених до дозволу, активами, залученими до замовлення, або білим списком активів для протоколу.
Оновлення: Не виправлено. Команда 1inch заявляє:
OrderMixin
насправді не має інформації про фактичні токениmakerAsset
таtakerAsset
іноді це проксі або інші проміжні контракти, а інформація про фактичні токени зберігається в деяких довільних байтах. Отже, немає життєздатного способу обмежити який активpermit
викликається.
[N13] solhint
ніколи не вмикається повторно
У всій кодовій базі є кілька solhint-disable
заяви, зокрема ті, що знаходяться в мережі 23 і в режимі онлайн 41 of RevertReasonParser.sol
, які не закінчуються solhint-enable
.
На користь явності та якомога більшого обмеження під час відключення solhint
, розгляньте можливість використання solhint-disable-line
or solhint-disable-next-line
натомість подібно до лінії 16 того самого файлу.
Оновлення: Зафіксовано в Запит на витяг #72.
[N14] Помилки
Кодова база містить такі помилки:
Додатково проект README
(поза межами цього аудиту) містить такі помилки:
Виправте ці помилки, щоб покращити читабельність коду.
Оновлення: Зафіксовано в Запит на витяг #71 та Запит на витяг #77.
[N15] Використання uint
замість uint256
Для явності всі випадки uint
має бути оголошено як uint256
. Зокрема, ті, що в for
петлі на лініях 98 та 119 of OrderMixin.sol
і лінії 16 та 30 of PredicateHelper.sol
.
Оновлення: Зафіксовано в Запит на витяг #70.
Висновки
Виявлено 3 проблеми високого рівня серйозності. Деякі зміни були запропоновані, щоб слідувати найкращим практикам і зменшити потенційну поверхню атаки.
- &
- 7
- МЕНЮ
- доступ
- За
- рахунки
- через
- Діяти
- дії
- Додатковий
- адреса
- Перевага
- ВСІ
- Дозволити
- вже
- хоча
- суми
- аналіз
- API
- підхід
- аргументація
- навколо
- активи
- Активи
- аудит
- Back-end
- початок
- буття
- КРАЩЕ
- передового досвіду
- Біт
- боти
- будувати
- call
- який
- випадків
- Викликати
- Ланка ланцюга
- зміна
- контроль
- Перевірки
- код
- комплекс
- стан
- замішання
- будівництво
- містить
- контракт
- контрактів
- Виправлення
- витрати
- може
- Пара
- творець
- Валюта
- Поточний
- дані
- угода
- Відмова в обслуговуванні
- розгортання
- дизайн
- розробників
- розробка
- DID
- відрізняються
- різний
- домен
- подвійний
- динамічний
- Рано
- край
- заохочувати
- особливо
- ETH
- Event
- Події
- все
- приклад
- обмін
- очікуваний
- досвід
- Експлуатувати
- риси
- Поля
- в кінці кінців
- Перший
- виправляти
- Гнучкість
- потік
- стежити
- знайдений
- Повний
- функція
- засоби
- майбутнє
- гра
- ГАЗ
- Загальне
- дає
- великий
- керівництво
- Обробка
- мішанина
- хешування
- має
- допомога
- Високий
- дуже
- Як
- HTTPS
- гібрид
- ідентифікувати
- Impact
- здійснювати
- важливо
- імпорт
- включені
- У тому числі
- Augmenter
- збільшений
- інформація
- інформація
- Інфраструктура
- розуміння
- намір
- інтерес
- інтерфейс
- залучений
- питання
- IT
- великий
- більше
- вести
- провідний
- бібліотека
- обмеженою
- Лінія
- ліквідності
- Перераховані
- списки
- місцевий
- подивився
- пошук
- основний
- виробник
- Робить
- ринок
- Мемпул
- дзеркало
- модель
- найбільш
- Найбільш популярний
- а саме
- Нові можливості
- не замінний
- негібкі лексеми
- офіційний
- відкрити
- операції
- варіант
- оракул
- порядок
- замовлень
- Інше
- власник
- Викрійки
- популярний
- представити
- попередження
- price
- ціни без прихованих комісій
- приватний
- процес
- проект
- проектів
- захист
- протокол
- забезпечувати
- забезпечує
- повноваження
- громадськість
- публікувати
- покупка
- якість
- підвищення
- Реальність
- зменшити
- опора
- звітом
- Звіти
- Сховище
- Вимога
- REST
- результати
- Умови повернення
- огляд
- Risk
- турів
- прогін
- Sdk
- безпеку
- Послуги
- комплект
- загальні
- акції
- зсув
- аналогічний
- простий
- невеликий
- розумний
- Спритні контракти
- So
- солідність
- спам
- конкретно
- Витрати
- Spot
- старт
- стан
- Заява
- Штати
- Статус
- зберігання
- стиль
- представлений
- успіх
- успішний
- Успішно
- підтримка
- Підтриманий
- поверхню
- перемикач
- система
- Мета
- тест
- Тестування
- Тести
- через
- по всьому
- час
- разом
- знак
- Жетони
- трек
- Відстеження
- угода
- Довіряйте
- створеного
- Updates
- us
- юзабіліті
- USD
- USDT
- користувачі
- утиліта
- значення
- вид
- видимість
- чекати
- Що
- Білий список
- в
- без
- Work
- вартість
- нуль