6 Грудня, 2021
Вступ
Команда Таллі команда попросила нас переглянути та перевірити набір контрактів з кінцевою метою покращити загальні контракти про управління та надати їм більшої гнучкості. Ми переглянули код і опублікували результати.
Огляд системи
Система базується на трьох основних контрактах:
- A
SafeGuard
шаблон договору. Цей договір єadmin
зTimelock
контракт і додає рівень модульних ролей поверхTimelock
дії. Цей контракт визначає кілька ролей, які мають окрему відповідальність і доступ до стану пропозиції (черги, скасування та виконання). - A
SafeGuardFactory
договір розгортає новSafeGuard
і відповідний новийTimelock
договір. Потім він встановлює адресу блокування часу всерединіSafeGuard
договору та реєструє новSafeGuard
вRegistry
. - A
Registry
який містить список розгорнутихSafeGuards
з їх відповідними номер версії.
Команда SafeGuardFactory
в основному породить a new SafeGuard
щоразу, коли його викликають. А SafeGuard
це обертання навколо Timelock
операції який додає різні та окремі ролі для кожної дії. Ролі структуровані за допомогою OpenZeppelin AccessControlEnumerable
Контракт, що надає більше гнучкості та сумісності гаманцям із кількома підписами та бізнес-випадкам використання, де кілька адрес можуть мати однакову спільну роль.
Оновлення: У PR # 10, команда Tally вирішила видалити Registry
договір. Список розгорнутих засобів захисту тепер зберігається в SafeGuardFactory
договору, в ст safeGuards
перелічуваний набір разом із їх версією, що зберігається в safeGuardVersion
картографування.
Ролі
Команда SafeGuard
договір визначає такі ролі:
Оновлення: Команда CREATOR_ROLE
роль було видалено як виправлення проблеми L02.
Сфера
Ми перевірили фіксацію b2c63a9dfc4090be13320d999e7c6c1d842625d3
в safeguard
сховище. У сферу застосування входять смарт-контракти в contracts
каталог. Однак, mocks
каталог було визнано виключеним.
Припущення
Система не призначена для оновлення. The Registry
адреса встановлена в конструкторі в SafeGuardFactory
і кожен SafeGuard
може мати Timelock
встановити лише один раз. Це означає, що якщо новий Registry
розгортається новий SafeGuardFactory
також має бути розгорнуто. Якщо Timelock
зміни реалізації, стар SafeGuard
s застаріють, і доведеться розгортати нові.
Крім того, система значною мірою залежить від реалізації a Timelock
контракт що було визнано поза межами цього аудиту. Команда ще не завершила реалізацію Timelock
договір. На момент написання цього звіту Реалізація блокування часу Compound використовується в проекті.
Кодову базу перевіряли два аудитори протягом одного тижня, і тут ми представляємо наші висновки.
Критична тяжкість
Ні.
Високий ступінь тяжкості
[H01] ETH можна заблокувати всередині Timelock
контракт
Команда Tally
команда спочатку базувала свої реалізації на основі GovernorBravoDelegate
Складний договір.
Під час цього аудиту, Tally
команда виявила обмеження у губернаторі Compound, де ETH надсилається безпосередньо до Timelock
недоступний для використання пропозиціями щодо управління, і, хоча він не застряг назавжди, вимагає детального обхідного шляху для отримання.
Це пов’язано з тим, що для реалізації регулятора необхідно додати всю цінність пропозиції msg.value
обліковим записом, який запускає виконання, жодним чином не використовуючи Timelock
Кошти ETH.
Команда та сама проблема була пізніше виявлена в SafeGuard
реалізація і команда знає про проблему, і вона в процесі її вирішення.
Під час вирішення проблеми подумайте про використання підходу прийнятий бібліотекою OpenZeppelin для того ж питання.
Оновлення: Виправлено у коміті 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory можна заморозити
Команда Registry
договір призначений для відстеження всіх SafeGuards
який SafeGuardFactory
випускає. Воно має зовнішнє register
функція який використовується для цієї мети.
У той же час, SafeGuardFactory
має у своєму конструкторі призначення локального registry
значення до вхідного значення. Немає можливості змінити значення registry
змінна і з цієї причини, якщо нов Registry
буде розгорнуто нову фабрику також має бути розгорнуто.
Команда SafeGuardFactory
має createSafeGuard
функція, відповідальний за перш розгортання нового SafeGuard
, То нове Timelock
з адресою в SafeGuard
as admin
, потім налаштування timelock
змінна SafeGuard
договір і нарешті реєстрація SafeGuard
в реєстрі.
Проблема в тому, що будь-який дзвінок до createSafeGuard
може бути примусово виведена з ладу зловмисником, який може безпосередньо зареєструвати детерміновану адресу нового SafeGuard
до його створення. Щоразу, коли договір створює a new
екземпляр, його nonce збільшується, і адресу, де буде розгорнуто новий екземпляр контракту, можна визначити за вихідною адресою контракту та його nonce. Таким чином, зловмисник може заздалегідь розрахувати багато адрес, де нові SafeGuards
буде розгорнуто та зареєструє ці адреси в Registry
зателефонувавши до register
функція. Це призведе до дзвінків до createSafeGuard
до повернути так як Registry
вже містить адресу.
Щоб уникнути того, щоб зовнішні актори публічно називали Register
договору, розгляньте можливість обмеження доступу до register
функція для прийому дзвінків виключно від SafeGuardFactory
.
Оновлення: Зафіксовано в PR # 10. Команда Tally видалила Registry
договір.
Середня тяжкість
Ні.
Низька тяжкість
[L01] Закоментований код
Команда Registry
договір включає в себе закоментований рядок коду. Щоб покращити читабельність, подумайте про видалення його з кодової бази.
Оновлення: Зафіксовано в PR # 10 і здійснити 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] Адміністратор SafeGuard може призначити роль творця будь-якій адресі
Команда SafeGuard
договір визначає роль a CREATOR_ROLE
який, як випливає з назви, призначений творцю захисту.
Однак, викликаючи grantRole
функції AccessControlEnumerable
контракт у контрактній бібліотеці OpenZeppelin адміністратор може надати цю роль будь-якій адресі. Це може викликати плутанину, оскільки творець SafeGuard
може бути тільки SafeGuardFactory
.
У всій кодовій базі ця роль використовувалася лише для обмеження користувачів у взаємодії з setTimelock
функція в SafeGuard
договір. За задумом система це забезпечує setTimelock
функція можна викликати лише один раз, з в SafeGuardFactory
контракт.
Розгляньте можливість видалення CREATOR_ROLE
роль від в SafeGuard
договір і використовуючи в onlyOwner
зміна в setTimelock
функції.
Оновлення: Зафіксовано в PR # 10.
[L03] Неправильне визначення та реалізація інтерфейсу
Команда ISafeGuard
інтерфейс не визначає queueTransactionWithDescription
функція реалізовано в SafeGuard
договору, і в той же час він визначає __abdicate, __queueSetTimelockPendingAdmin і __executeSetTimelockPendingAdmin функції, але вони не реалізовані.
Щоб підвищити правильність і узгодженість кодової бази, розгляньте рефакторинг ISafeGuard
інтерфейс, щоб точно відповідати SafeGuard
реалізація.
Оновлення: Виправлено у коміті 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Відсутні рядки документації
Для деяких контрактів і функцій у кодовій базі бракує документації. Наприклад, деякі функції в SafeGuard
договір.
Крім того, деякі рядки документів використовують неформальну мову, як-от один вище setTimelock
функція в SafeGuard
контракт.
Це заважає рецензентам зрозуміти призначення коду, що має фундаментальне значення для правильної оцінки не лише безпеки, але й правильності. Крім того, рядки документів покращують читабельність і спрощують обслуговування. Вони повинні чітко пояснити мету або призначення функцій, сценарії, за яких вони можуть вийти з ладу, ролі, яким дозволено їх викликати, значення, що повертаються, і події, що виникають.
Розгляньте можливість ретельного документування всіх функцій (та їхніх параметрів), які є частиною публічного API контрактів. Функції, що реалізують конфіденційні функції, навіть якщо вони не є публічними, також мають бути чітко задокументовані. Під час написання рядків документів слід дотримуватися Природний формат специфікації Ethereum (NatSpec).
Оновлення: Частково закріплено в PR # 10. Належні рядки документації були додані до різних функцій у всій базі коду. Однак, крім поточних змін, розгляньте можливість внесення таких змін:
- додавати
description
в якості@param
у рядку документів вищеqueueTransactionWithDescription
функція - додавати
@param
в документ вищеcreateSafeGuard
функція вSafeGuardFactory
контракт - додавати
@return
у рядках документів над функціями вSafeGuardFactory
договір.
[L05] Марний або повторюваний код
У кодовій базі є місця, де код або повторюється, або непотрібний. Деякі приклади:
- Рядки 29-32 в
Registry
договору марні, тому що_add
функціїEnumerableSet
контракт вже виконує ці перевірки проти вже встановлених значень. - Рядки 62, 67, 73 та 78 в
SafeGuard
усі контракти повторюють ту саму точну операцію. Подумайте про інкапсуляцію його у внутрішню функцію, щоб уникнути дублювання коду. - Рядки 62-63 та 67-68 of
SafeGuard
повторюються. Розглянемо інкапсуляцію їх в одну внутрішню функцію. - Використання
gasleft
щоб вказати, скільки газу має бути направлено під час виклику функціїexecuteTransaction
є непотрібним. Це пояснюється тим, що в цей момент виконання весь газ, що залишився, буде використано для продовження виконання. Якщо це не для чіткості, подумайте про видаленняgas
параметр із виклику.
Подумайте про застосування запропонованого виправлення, щоб створити чистіший код і покращити послідовність і модульність кодової бази.
Оновлення: Зафіксовано в PR # 10 і здійснити 7fd27df16fc879d990d36a167a0b6e719e578558
.
Примітки та додаткова інформація
[N01] Непослідовний стиль
Є деякі місця в базі коду, де відмінності в стилях впливають на читабельність, ускладнюючи розуміння коду. Деякі приклади:
- Команда
Registry
контракт використовує різні стилі для рядків документації в усьому контракті. - Команда
SafeGuard
контракт створює подію, колиqueueTransactionWithDescription
викликається, але в інших функціях, що мають справу з транзакціями, події не генеруються. - У
SafeGuard
контракт, іноді значення використовується як іменований параметр, а іноді _value використовується.
Беручи до уваги цінність узгодженого стилю кодування, яке додає читабельності проекту, подумайте про те, щоб застосувати стандартний стиль кодування за допомогою інструментів linter, таких як Solhint.
Оновлення: Зафіксовано в PR # 10 і здійснити 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Відсутня ліцензія
У наступних контрактах у кодовій базі відсутній ідентифікатор ліцензії SPDX.
Щоб заглушити попередження компілятора та підвищити узгодженість у кодовій базі, додайте ідентифікатор ліцензії. Роблячи це, посилайтеся на spdx.dev керівні принципи.
Оновлення: Зафіксовано в PR # 10 і здійснити 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] Залежність контракту OpenZeppelin не закріплена
Щоб запобігти несподіваній поведінці в разі порушення, зміни будуть опубліковані в майбутніх оновленнях Бібліотека контрактів OpenZeppelin, розглянемо закріплення версії цієї залежності в файлі package.json файлу.
Оновлення: Зафіксовано в PR # 10.
[N04] Версія компілятора Solidity не закріплена
У всій базі коду розгляньте можливість закріплення версії Компілятор Solidity до останньої стабільної версії. Це має допомогти запобігти виникненню неочікуваних помилок через несумісні майбутні випуски. Щоб вибрати конкретну версію, розробники повинні враховувати як функції компілятора, необхідні для проекту, так і список відомих помилок пов’язані з кожною версією компілятора Solidity.
Оновлення: Зафіксовано в PR # 10.
[N05] Опечатка
У різних випадках по всій кодовій базі слово role
пишеться з помилкою як rol
. Одним із таких прикладів є в рядок документації в межах constructor
в SafeGuard
контракт.
Виправте ці помилки, щоб покращити читабельність коду.
Оновлення: Частково закріплено в PR # 10. Тоді як правопис о role
було виправлено, коментар «встановити роль адміністратора на визначену адресу адміністратора» має бути «встановити роль адміністратора на визначену адресу адміністратора». Крім того, «виконати» неправильно написано в SafeGuard
договір на лінія 69, лінія 82, лінія 96 та лінія 110 і слово «доступний» написано з помилкою лінія 70, лінія 83, лінія 97, лінія 111. Також подумайте про заміну неофіційних слів, таких як «буду» in SafeGuard
договір з формальними альтернативами, такими як «збирається».
[N06] Оголосити uint як uint256
У кодовій базі є кілька випадків, де оголошено змінні uint
тип даних замість uint256
. Наприклад, eta
змінна в QueueTransactionWithDescription
подія в SafeGuard
договір.
Для явності всі випадки uint
має бути оголошено як uint256
.
Оновлення: Зафіксовано в PR # 10 і здійснити 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Невикористаний імпорт
Команда SafeGuard
договір імпортує console
контракт, але ніколи ним не користується.
Щоб покращити читабельність коду, подумайте про видалення будь-якого невикористаного імпорту.
Оновлення: Зафіксовано в PR # 10.
Висновки
Було знайдено одну високу та кілька інших незначних уразливостей, а також запропоновано рекомендації та виправлення.
- &
- доступ
- рахунки
- через
- дію
- дії
- Додатковий
- адреса
- адмін
- ВСІ
- вже
- хоча
- API
- підхід
- навколо
- аудит
- В основному
- буття
- помилки
- бізнес
- call
- випадків
- Викликати
- зміна
- заряд
- код
- Кодування
- загальний
- З'єднання
- замішання
- розгляду
- містить
- продовжувати
- контракт
- контрактів
- може
- творець
- Поточний
- дані
- справу
- дизайн
- розробників
- різний
- відкритий
- Розробити
- ETH
- Event
- Події
- приклад
- завод
- риси
- в кінці кінців
- Перший
- виправляти
- Гнучкість
- знайдений
- функція
- засоби
- майбутнє
- ГАЗ
- дає
- мета
- управління
- Губернатор
- керівні вказівки
- має
- допомога
- тут
- Високий
- Як
- HTTPS
- реалізовані
- Augmenter
- збільшений
- інтерфейс
- IT
- відомий
- мова
- останній
- бібліотека
- ліцензія
- Лінія
- список
- місцевий
- замкнений
- подивився
- Робить
- матч
- модульний
- Мультисиг
- Інше
- представити
- процес
- проект
- пропозиція
- громадськість
- публікувати
- реєструвати
- Релізи
- звітом
- Сховище
- результати
- огляд
- безпеку
- комплект
- установка
- загальні
- розумний
- Спритні контракти
- солідність
- стан
- стиль
- система
- через
- по всьому
- час
- інструменти
- трек
- Transactions
- Updates
- us
- користувачі
- значення
- Уразливості
- Гаманці
- week
- ВООЗ
- в
- слова
- лист