1-дюймовий аудит з фіксованою ставкою

Перевидано Платоном

читають: 0

15 Лютого, 2022

Вступ

Команда 1inch команда попросила нас переглянути та перевірити їхні смарт-контракти з фіксованою ставкою Swap. Ми переглянули код і опублікували результати.

Сфера

Ми перевірили фіксацію b1600f61b77b6051388e6fb2cb0be776c5bcf2d1 в 1inch/fixed-rate-swap сховище. В обсязі був наступний договір:

- FixedRateSwap.sol

Усі інші файли та каталоги проектів, включаючи тести, зовнішні залежності та проекти, теорію ігор і дизайн стимулів, були виключені зі сфери цього аудиту. Передбачалося, що зовнішній код і залежності контракту працюють, як це задокументовано.

Загальний стан здоров'я

Ми виявили, що кодова база складна, часто не має достатньої документації, щоб пояснити нетривіальну математику та логіку, які використовуються в усьому. Для кодової бази такого розміру ми вважаємо, що сама кількість проблем, включених у цей звіт, свідчить про непотрібну складність. Разом із браком документації кодова база не на тому рівні вдосконалення, який ми очікували б від готової до виробництва системи. Усунення, хоч і розумної, безперервної функції комісії та заміна її трьома окремими лінійними функціями може значно полегшити міркування про більшу частину кодової бази. Простіший дизайн міг би усунути багато найскладніших блоків коду, у яких були виявлені проблеми. Рефакторинг і спрощення протоколу дуже доцільні, у цьому випадку ми рекомендуємо пройти додаткові аудити.

Загалом, команда 1inch завжди була готова відповісти на будь-які запитання під час аудиту, і з ними було дуже легко працювати. Вони сприйнятливі до відгуків і пропозицій щодо постійного вдосконалення.

Споживання газу

Існують деякі особливо газоємні частини кодової бази, такі як _powerHelper функції та двійковий пошук, який ми спостерігали з використанням понад 400 тисяч газів у деяких крайових випадках. Враховуючи характер протоколу та стабільні монети, з якими він має намір працювати, високе споживання газу, до якого призводить складність проекту, може підірвати мету протоколу та, безумовно, вплинути на загальну зручність його використання.

Огляд системи

Протокол свопу з фіксованою ставкою — це автоматичний маркет-мейкер (AMM), який використовує криву ціни постійної суми для полегшення свопів між активами, ціни яких є стабільними один для одного.
Він також реалізує механізм змінної комісії, який в цілому стягує 0.01% як комісії. Коли баланс токенів AMM досягає екстремальних умов, комісії або знижуються до 0%, або підвищуються до 0.2% залежно від того, чи є ця діяльність бажаною чи ні, відповідно, для складу активів пулу. Такі комісії зберігаються в пулі, де постачальники ліквідності отримають від них вигоду, утримуючи акції (власні AMM ERC20 токен LP).

Привілейовані ролі

У перевіреній версії протоколу немає привілейованих ролей.

Зовнішні залежності та довірчі припущення

Протокол не має наміру підтримувати ERC20 токени, які стягують комісію за переказ.

Крім того, варто зазначити, що можуть існувати активи, які, залежно від їх природи, можуть викликати неочікувану поведінку, подібну до описаної в цьому звіті, наприклад стабільні монети з невідповідною кількістю десяткових знаків між собою або активи, які можуть дозволяти необмежені швидкі позики .

Результати

Тут ми представляємо наші висновки.

Оновлення: Команда 1inch застосувала кілька виправлень на основі наших рекомендацій і надала нам набір комітів, спрямованих на відповідні виявлені проблеми. Ці коміти вказані для кожної відповідної проблеми, і ми розглядаємо виправлення, внесені в кожну окрему коміцію. Однак ми перевірили лише конкретні виправлення проблем, про які повідомили. Кодова база зазнала деяких інших змін, які ми не перевіряли, і ми рекомендуємо детально переглянути ці зміни під час майбутнього аудиту.

Критична тяжкість

Ні.

Високий ступінь тяжкості

Ні.

Середня тяжкість

[M01] Відсутність заходів безпеки може призвести до втрати коштів

Команда FixedRateSwap контракт полегшує обмін однієї стабільної монети для іншої, а також дозволяє користувачам депозит та вилучати активи для «часток» пулу ліквідності (токени LP).

Однак ані свопи, ані зняття коштів, ані депозити не надають користувачам механізму встановлення мінімально прийнятних значень прибутку від взаємодії з пулом.

Пул оцінює змінні комісії на основі того, як взаємодії впливають на склад пулу. Тим не менш, за такої конструкції, оскільки оцінені комісії надходитимуть безпосередньо власникам акцій пулу, для мажоритарного акціонера є економічний стимул проводити великі свопи токенів, маніпулюючи складом пулу, щоб змусити вивищувати комісії.

Подібним чином, переднє виконання також може використовуватися, щоб скористатися перевагами округлення протоколу в обчисленнях виведення маркерів. Хоча це малоймовірно, враховуючи поточний стан екосистеми щодо популярних стабільних монет, якщо fromBalance токена в пулі можна маніпулювати таким чином, щоб бути в порядку 1e36 * inputAmount для будь-якого свопу, тоді цей рядок в _getReturn функція буде скорочена до zero, що призводить до нульового значення outputAmount.

Однак тому свопи нульового значення захищені від, усічення в межах outputAmount обчислення відбуватиметься до вхідних даних у найгіршому випадку. Зокрема, а fromBalance будь-де в діапазоні [1e26 до 1e35] * the input amount може призвести до скорочення та зменшення outputAmount що може бути прибутковим для власників акцій пулу за рахунок користувачів.

Хоча такий сценарій наразі малоймовірний, майбутні зміни екосистеми та необмежені термінові позики можуть зробити це більш життєздатним вектором атаки.

Щоб уникнути ненавмисної втрати коштів і пом’якшити можливі атаки на передньому плані, подумайте про те, щоб дозволити користувачам вказувати мінімально прийнятне значення повернення для будь-якої взаємодії з протоколом, яка має грошові наслідки.

Оновлення: Зафіксовано в commit ea75a86. Однак не було додано жодного спеціального тесту для підтвердження того, що впровадження виправлення запобігає цій атаці.

Низька тяжкість

[L01] Депозит може бути повернений у разі переповнення

Якщо пул різко незбалансований, депозити повертаються під час розрахунку shift змінна в _getVirtualAmountsForDepositImpl функція якщо загальна сума депозиту не перевищує 1/1000 наявного балансу в пулі.

Наприклад, спроба внести співвідношення токенів, що становить менше 10, у пул із поточним балансом 10,000 XNUMX token0 і .0001 token1, повернеться. Внесення більшої суми вдасться.

Це можна використати для створення бар’єру, щоб інші користувачі не могли приєднатися до пулу. За такого сценарію все ще можна вийти з пулу та обміняти токени. Також можна внести депозит з точно таким же співвідношенням пулу. Якщо пул зміщується, щоб бути менш одностороннім, тоді знову стають можливими менші депозити.

Щоб уникнути потенційних атак типу «відмова в обслуговуванні», подумайте про перегляд кодової бази для кращого врахування цих крайніх випадків. Принаймні, подумайте про явну перевірку цих умов і повернення корисних повідомлень про помилки під час повернення.

Оновлення: Зафіксовано в commit 4aa5210.

[L02] Викиди неповної події

Команда Swap подія створюється кожного разу, коли протокол сприяє обміну маркерами.

Однак існує кілька загальнодоступних методів, доступних для виконання заміни токенів. The swap0To1 та swap1To0 функції надсилають результат обміну безпосередньо до msg.sender, але swap0To1For та swap1To0For функції надсилають результати обміну на адресу, явно вказану в to параметр.

Оскільки в обох випадках вона випустила ту саму подію, сторони поза ланцюгом не мають можливості відрізнити, який тип обміну було виконано або кому було доставлено результат обміну.

Аналогічним чином, Deposit та Withdrawal події приймають лише одну user параметр адреси, навіть якщо вони також видаються у функціях, де msg.sender може відрізнятися від сторони, яка отримує жетони.

Розгляньте можливість додавання індексованого recipient адресний параметр для подій, щоб вони могли більш повно передати, які дії виконує протокол.

Оновлення: Визнав і не виправлятиму.

[L03] Відсутність перевірки введених даних

Команда public getReturn функція не має певної перевірки вхідних даних. зокрема:

Це не підтверджує, що tokenFrom та tokenTo адреси — це токени, які складають пул.
Це не підтверджує, що inputAmount параметр відмінний від нуля.
Це не підтверджує, що fromBalance + toBalance значення відмінне від нуля.

Крім того, ні withdrawFor ні withdrawForWithRatio функції підраховують, чи достатньо у користувача акцій, щоб зняти запитану суму. У будь-якому випадку вилучення буде скасовано, якщо умова не виконується, але повідомлення про помилки нечіткі та видаються лише після споживання непотрібного газу.

Нарешті, withdrawForWithRatio функція нехтує тим, щоб значення, які використовуються для розрахунків віртуального балансу, були меншими або дорівнюють фактичним балансам контракту. Такий сценарій призведе до загадкового повернення арифметики при розрахунку balanceX та balanceY для _getRealAmountsForWithdrawImpl функція.

Відсутність перевірки контрольованих користувачем параметрів може призвести до помилкових або невдалих транзакцій, які важко налагодити. Щоб уникнути цього, подумайте про покращення чіткості повідомлень про помилки та додавання перевірки введених даних, щоб вирішити вищезазначені проблеми.

Оновлення: Частково закріплено в commit 63b6a95 та commit 7c0ade7. Це було підтверджено, лише якщо inputAmount значення дорівнює нулю, і було переміщено порядок операцій на ранню помилку під час записування токенів LP, щоб зменшити вартість газу. Заява команди 1inch щодо цієї проблеми:

Перевірка tokenFrom і tokenTo не є обов’язковою, оскільки в методі swap вони замінюються з констант

[L04] Константи не оголошені або не названі чітко

У FixedRateSwap договір, буквальні значення 998, 1000 та 1002, які використовуються для проходження зборів у _getRealAmountsForWithdrawImpl та _getVirtualAmountsForDepositImpl функції, не мають супровідних пояснень або вбудованої документації.

Крім того, відсутність внутрішньої документації також впливає на неоднозначно названі константи які використовуються в обчисленнях у всій кодовій базі.

Щоб покращити читабельність коду та полегшити рефакторинг, подумайте про визначення константи для кожного «магічного значення» та переконайтеся, що всі константи мають чіткі та зрозумілі назви. Для складних значень розгляньте можливість додавання вбудованої документації з поясненням того, як вони були обчислені або чому їх було обрано.

Оновлення: Зафіксовано в commit 7091076.

[L05] Оманлива або неповна вбудована документація

У всій кодовій базі було виявлено кілька випадків оманливої та/або неповної вбудованої документації, і їх слід виправити.

Нижче наведено приклади неповної вбудованої документації:

Нижче наведено приклади оманливої внутрішньої документації:

Команда розрахунки приватної функції _powerHelper не вказуйте прямо, що результат множення ділиться на 1e18 щоб запобігти подвійному масштабуванню значення. Крім того, експонента коефіцієнта масштабування також відповідає потужності, наданій функцією, що може викликати додаткову плутанину, якщо його не враховувати.

Чітка вбудована документація є фундаментальною для окреслення намірів коду. Невідповідності між вбудованою документацією та реалізацією можуть призвести до серйозних помилок щодо очікуваної поведінки системи. Подумайте про виправлення будь-яких помилок і додавання додаткової документації, якщо вони виявлені, щоб уникнути плутанини для розробників, користувачів і аудиторів.

Оновлення: Визнав і не виправлятиму.

[L06] Немає доступного звіту про покриття

Незважаючи на те, README файл вказує на звіт про покриття, він недоступний.

Крім того, немає інструкцій щодо запуску сценаріїв тестового покриття.

Спробуйте зробити звіт про покриття доступним і чітко задокументувати, як запускати сценарії тестового покриття.

Оновлення: Частково виправлено. Звіт про покриття тепер доступний, однак README файл все ще не пояснює, як запускати сценарії.

[L07] Потенційно небезпечна неперевірена математика

Протягом усього FixedRateSwap договір, існує багато застосувань unchecked математика. Основна причина використання unchecked Математика полягає у видаленні перевірок переповнення/недоповнення у випадках, які або покладаються на таку поведінку, або, як відомо, не переповнюють/переповнюють. Це сприяє економії газу, але, якщо використовувати його неправильно, це може призвести до неочікуваних результатів і потенційної вразливості.

Екземпляри unchecked виявлено арифметику, яка потенційно може переповнюватися/переповнюватися. Наприклад:

Необхідні значення, необхідні для переповнення цих обчислень, у поєднанні з розумними перевірками в усій кодовій базі часто перешкоджають досягненню цих переповнень на практиці, але вони все ще є теоретично можливо. Не використовуйте unchecked математика, за винятком випадків, коли можливе переповнення повністю виключено, щоб запобігти неочікуваним результатам і зменшити загальну поверхню атаки протоколу.

Оновлення: Визнав і не виправлятиму.

[L08] Небезпечне явне приведення цілих чисел

Команда Swap подія займає address і два int256 параметрів і випромінюється в swap0To1, swap1To0, swap0To1For та swap1To0For функції.

Однак під час випромінювання цілочисельні значення, що передаються до події, явно формуються з uint256 до int256 значень.

Незважаючи на те, що сьогодні навряд чи це буде проблемою на практиці, розвиток екосистем, наприклад необмежена швидка позика активів стейблкойнів, може призвести до небажаної поведінки цього дизайну в майбутньому. На даний досить великий uint256 значення, явно переведене в an int256 type скоротить його значення. У результаті системи поза ланцюгом, залежні від точності випромінювання події, можуть бути введені в оману.

Розгляньте можливість перевизначення Swap подія, з якою потрібно мати справу безпосередньо uint256 значення, щоб функції, які створюють подію, могли відмовитися від явних приведення.

Оновлення: Зафіксовано в commit 8436c6c. Команда 1 дюйм імпортував OpenZeppelin SafeCast бібліотека щоб безпечно відлити згадані справи.

[L09] Процес вилучення може призвести до підлоги

Коли акціонер використовує або withdraw або withdrawFor функції, договір розраховує суму активів що вони мають право на надання кількість акцій. Потім ці активи передаються зазначеному одержувачу.

Однак, оскільки if використовуються заяви а не require твердження, щоб перевірити, чи потрібно надіслати будь-який актив одержувачу, якщо пул незбалансований і кількість часток невелика, контракт може перевищувати вартість, яку потрібно надіслати для одного або обох активів.

З огляду на те, що задіяні значення обов’язково будуть досить малими, і враховуючи той факт, що протокол не може повністю обмежити зняття коштів, коли вихід одного токена може фактично дорівнювати нулю, подумайте про документування цієї потенційної поведінки округлення, щоб користувачі знали про це під час зняття.

Оновлення: Визнав і не виправлятиму.

Примітки та додаткова інформація

[N01] Застарілі залежності проекту

Під час монтажу залежності проекту, NPM попереджає, що один із встановлених пакетів, Highlight, «більше не підтримуватиметься та не отримуватиме оновлень безпеки в майбутньому».

Навіть якщо малоймовірно, що цей пакет може спричинити загрозу безпеці, подумайте про оновлення залежності, яка використовує цей пакет, до підтримуваної версії.

Оновлення: Зафіксовано в commit 0a2b55d. Однак для інсталяції тепер потрібно використовувати програму -force прапор на поточних версіях LTS вузла, щоб досягти успіху.

[N02] Плата за пул може стимулювати незбалансовані депозити

При внесенні в FixedRateSwap контракт, _getVirtualAmountsForDeposit функція розраховує віртуальну вартість депозиту. Віртуальні суми — це початкові суми, масштабовані після стягнення комісії відповідно до співвідношення поточних активів пулу.

Якщо користувач вносить кошти на поточний коефіцієнт активів у пулі тоді з них не стягується плата. В іншому випадку з них стягується комісія на основі різниці між коефіцієнтом депозитів і коефіцієнтом поточних активів пулу.

Ця конструкція передбачає, що коли співвідношення активів у пулі незбалансоване, користувачі будуть стимулювати робити депозити з таким самим незбалансованим співвідношенням, а не робити депозити з таким співвідношенням, яке зробить пул більш збалансованим.

Щоб стимулювати більш збалансований пул, подумайте про стимулювання депозитів, які збалансують пул, а не штрафують їх. Крім того, якщо це неможливо, подумайте про те, щоб пояснити причину в документації проекту.

Оновлення: Визнав і не виправлятиму.

[N03] Незадокументовані неявні вимоги до схвалення

Команда FixedRateSwap договір неявно припускає, що йому було надано відповідну надбавку перед виконанням свопи та депозити які обов'язково передають жетони.

На користь чіткості та покращення загальної ясності кодової бази розгляньте можливість документування всіх вимог до затвердження у вбудованій документації відповідних функцій.

Оновлення: Визнав і не виправлятиму.

[N04] Заплутана неявна перевірка `outputAmount`

Команда getReturn функція надає три параметри, а саме маркер для обміну з (tokenFrom), маркер для обміну на (tokenTo) і введену суму (inputAmount значення tokenFrom актив). Відповідний outputAmount значення (результатна сума для tokenTo актив) є тоді розраховано та повернуто.

Перед обчисленням функція Вимагається який inputAmount значення менше або дорівнює балансу токенів пулу tokenTo актив. Однак, outputAmount значення, можливо, більш інтуїтивно зрозуміле значення для перевірки tokenTo баланс активів, ніколи явно не перевіряється для того самого стану.

Насправді, математика, яка використовується для обчислення outputAmount значення гарантує, що воно буде точно менше або дорівнює inputAmount value.

Однак навмисність такої поведінки незрозуміла, тобто неочевидно, чи ця конструкція призначена для того, щоб швидше виходити з ладу під час виконання, щоб зменшити витрати на газ, чи ні. Спробуйте чітко задокументувати обґрунтування використаної точної перевірки. Крім того, розгляньте перевірку балансу to актив більше, ніж outputAmount значення на відміну від inputAmount value.

Оновлення: Зафіксовано в commit 10f4d9c.

[N05] Непослідовність імен

Команда FixedRateSwap договір встановлює явний пара жетонів для яких призначені операції обміну, зняття та депозиту. У всій кодовій базі ці маркери позначені індексом будь-якого 0 or 1, як в token0 та token1. Функції, які мають описові назви для передачі деталей про використання, також використовують ці індекси маркерів, наприклад swap0To1 функції.

Однак для withdrawForWithRatio функція, параметр, який визначає пропорцію для отримання token0 проти token1 називається firstTokenShare що може внести плутанину в те, що це посилання на token1 актив, а не token0 актив.

Щоб покращити загальну читабельність і зменшити потенційну плутанину, подумайте про те, щоб угоди про іменування були узгодженими в усій кодовій базі.

Оновлення: Зафіксовано в commit 57ad4cd.

[N06] Повідомлення про повернення мають неузгоджений формат

Команда require оператори в конструкторі в FixedRateSwap оформлено інакше, ніж усі інші require твердження в договорі.

Оскільки неузгоджено відформатовані повідомлення про повернення можуть викликати непотрібну плутанину, переконайтеся, що всі require оператори мають повідомлення про повернення, які послідовно відформатовані, точні, інформативні та зручні для користувача.

Оновлення: Зафіксовано в commit 0aa4e9d.

[N07] Непослідовне використання іменованих повертаних змінних

Існує непослідовне використання іменованих повертаних змінних у FixedRateSwap договір.

Зокрема, хоча більшість функцій повертає іменовані змінні, функція decimals, _getVirtualAmountsForDepositImpl, _getRealAmountsForWithdrawImpl та _checkVirtualAmountsFormula функції повертають явні значення.

Розгляньте можливість узгодженого підходу до повернення значень у всій кодовій базі, видаливши всі іменовані змінні повернення, явно оголосивши їх як локальні змінні та додавши необхідні оператори повернення, де це необхідно. Це покращить чіткість і читабельність коду, а також може допомогти зменшити регресії під час майбутніх рефакторів коду.

Оновлення: Визнав і не виправлятиму.

[N08] Оптимізація газу

У FixedRateSwap контракту, є можливості для кількох простих скорочень споживання газу. Наприклад:

Команда _getVirtualAmountsForDeposit private функція завжди викликається лише з одного місця в кодовій базі, і це depositFor функція. В межах depositFor є виклики функції token0.balanceOf(address(this)) та token1.balanceOf(address(this)). Однак ті самі дзвінки здійснюються у верхній частині _getVirtualAmountsForDeposit функція. Останній функції можна було б просто передати необхідні значення, замість того, щоб зчитувати баланси двічі на depositFor дзвінок.
У _getRealAmountsForWithdrawImpl private функція, secondTokenShare змінна визначається як _ONE - firstTokenShare, на наступний рядок, точнісінько те саме віднімання виконується знову, коли воно може просто використовувати secondTokenShare змінна.

Щоб зменшити витрати на газ і ще більше спростити кодову базу, подумайте про вирішення вищезазначених випадків.

Оновлення: Зафіксовано в commit 34974ee.

[N09] Неправильна видимість функції

Команда withdrawWithRatio функція не викликається внутрішньо жодною з функцій у FixedRateSwap договір. Установіть видимість на external замість public.

Оновлення: Зафіксовано в commit cb852f5.

[N10] Залежність від відповідності `decimals` може бути проблематично

Протокол неявно вимагає, щоб обидва токени всередині пулу підтримували decimals метод щоб будівництво басейну було успішним. Насправді цей метод поширений майже всюди, але технічно він необов’язковий компонент специфікації ERC20, в якому прямо вказано, що контракти «НЕ ПОВИННІ очікувати, що ці значення будуть присутні». Наразі будь-які токени, які не підтримують опцію decimals метод не можна використовувати в межах протоколу.

Можливо, більш проблематично, як частина цих викликів маркера decimals, протокол додатково вимагає, щоб обидва маркери повертали ідентичні значення.

Однак простір токенів стабільної монети не є однорідним у цьому відношенні. Він складається з багатьох токенів, які повертають різні значення для decimals. Наприклад, поки USDT та USDC звіт 6 десяткових, DAI повідомляє 18 знаків після коми.

Якщо це навмисні обмеження протоколу, подумайте про те, щоб торкнутися їх явно та надати короткі обґрунтування через вбудовану документацію. Також розгляньте можливість надання кращих повідомлень про помилки під час створення для маркерів, які не підтримують decimals метод. Крім того, розгляньте можливість зробити протокол більш надійним, щоб він міг обробляти маркери, які не підтримують decimals метод або пари, що повідомляють про різні decimals в межах одного пулу.

Оновлення: Зафіксовано в commit b49d808. Додано вбудовану документацію.

[N11] Друкарська помилка

Ми виявили таку друкарську помилку в кодовій базі:

Повернення повідомлення на лінія 92 of FixedRateSwap.sol починається без великої літери, що робить його несумісним з рештою коду.

Щоб покращити загальну послідовність і читабельність кодової бази, подумайте про виправлення цієї та будь-яких інших друкарських помилок у всій кодовій базі.

Оновлення: Зафіксовано в commit a92d16a.

[N12] Непотрібно `virtual` функція

Команда FixedRateSwap договір успадковує від OpenZeppelin's ERC20 договір але це перекриває його ERC20.decimals функція. Це потрібно, тому що FixedRateSwapтокен пулу ліквідності, будучи залежним від decimals активів, які складають пул, обов’язково є динамічним.

Однак, незважаючи на те, що ця основна реалізація функції має бути остаточною, вона є такою визначається за допомогою virtual ключове слово, сигналізуючи про те, що це не обов’язково остаточна реалізація, і дозволяючи її знову перевизначати.

Щоб уникнути плутанини та прояснити наміри, подумайте про видалення virtual ключове слово або документування причин його збереження.

Оновлення: Зафіксовано в commit 8bce5ec.