6 декабря 2021
Введение
Ассоциация число Команда попросила нас просмотреть и проверить ряд контрактов с конечной целью улучшить контракты общего управления и придать им большую гибкость. Мы просмотрели код и теперь публикуем наши результаты.
Системный Обзор
Система опирается на три основных контракта:
- A
SafeGuard
образец договора. Этот контракт являетсяadmin
в АTimelock
контракт и добавляет слой модульных ролей поверхTimelock
действия. Этот контракт определяет несколько ролей, которые несут отдельную ответственность и доступ к состоянию предложения (постановка в очередь, отмена и исполнение). - A
SafeGuardFactory
контракт развертывает новыйSafeGuard
и соответствующее новоеTimelock
договор. Затем он устанавливает адрес временной блокировки внутриSafeGuard
контракт и регистрирует новыйSafeGuard
вRegistry
. - A
Registry
который содержит список развернутыхSafeGuards
с соответствующими номер версии.
Ассоциация SafeGuardFactory
в основном порождает new SafeGuard
всякий раз, когда он вызывается. А SafeGuard
это обертка вокруг Timelock
операции это добавляет разные и отдельные роли для каждого действия. Роли структурированы с помощью OpenZeppelin. AccessControlEnumerable
контракт, обеспечивающий большую гибкость и совместимость с мультиподписными кошельками и вариантами использования в бизнесе, когда несколько адресов могут иметь одну и ту же общую роль.
Обновление: В PR № 10, команда Tally решила удалить Registry
договор. Список развернутых средств защиты теперь хранится в SafeGuardFactory
контракт, в safeGuards
перечислимый набор вместе с их версией, хранящейся в safeGuardVersion
отображение.
роли
Ассоциация SafeGuard
контракт определяет следующие роли:
Обновление: Ассоциация CREATOR_ROLE
роль была удалена в качестве исправления проблемы L02.
Объем
Мы проверили коммит b2c63a9dfc4090be13320d999e7c6c1d842625d3
safeguard
хранилище. Речь идет о смарт-контрактах в contracts
каталог. Однако mocks
каталог был сочтен выходящим за рамки.
Предположения
Система не предназначена для обновления. Registry
адрес установлен в конструкторе SafeGuardFactory
и каждый SafeGuard
может иметь Timelock
установить только один раз. Это означает, что если новый Registry
развернут новый SafeGuardFactory
тоже надо развернуть. Если Timelock
изменения реализации, старый SafeGuard
s устареют, и придется внедрять новые.
Кроме того, система во многом зависит от реализации 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
до его создания. Всякий раз, когда контракт создает new
пример, его одноразовый номер увеличивается, и адрес, по которому будет развернут новый экземпляр контракта, может определяться исходным адресом контракта и его одноразовым номером. Таким образом, злоумышленник может заранее вычислить многие адреса, по которым будет открыт новый SafeGuards
будут развернуты и зарегистрируют эти адреса в Registry
позвонив register
функция. Это приведет к вызовам createSafeGuard
в возвращаться С Registry
уже содержит адрес.
Во избежание того, чтобы внешние игроки публично называли Register
контракта, рассмотрите возможность ограничения доступа к register
функция приема вызовов исключительно SafeGuardFactory
.
Обновление: Исправлено в PR № 10. Команда Tally удалила Registry
контракт.
Средней степени тяжести
Нет.
Низкая степень серьезности
[L01] Закомментированный код
Ассоциация Registry
контракт включает в себя закомментированная строка кода. Чтобы улучшить читаемость, рассмотрите возможность удаления его из базы кода.
Обновление: Исправлено в PR № 10 и совершить 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] Администратор SafeGuard может назначить роль создателя любому адресу.
Ассоциация SafeGuard
договор определяет роль 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 (НатСпец).
Обновление: Частично исправлено в 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
контракт, иногда ценностное используется как именованный параметр, а иногда и _ценность используется.
Принимая во внимание ценность, которую единообразный стиль кодирования повышает читабельность проекта, рассмотрите возможность применения стандартного стиля кодирования с помощью инструментов линтера, таких как Solhint.
Обновление: Исправлено в PR № 10 и совершить 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Отсутствует лицензия
В следующих контрактах в базе кода отсутствует идентификатор лицензии SPDX.
Чтобы отключить предупреждения компилятора и повысить согласованность всей кодовой базы, рассмотрите возможность добавления идентификатора лицензии. При этом подумайте о том, чтобы обратиться к spdx.dev руководящие принципы.
Обновление: Исправлено в PR № 10 и совершить 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] Зависимость контракта OpenZeppelin не закреплена
Чтобы предотвратить непредвиденное поведение в случае выпуска критических изменений в будущих обновлениях Библиотека OpenZeppelin Contractsрассмотрите возможность закрепления версии этой зависимости в пакет.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
- подхода
- около
- аудит
- в основном
- не являетесь
- ошибки
- бизнес
- призывают
- случаев
- Вызывать
- изменение
- заряд
- код
- Кодирование
- Общий
- Соединение
- замешательство
- рассмотрение
- содержит
- продолжать
- контракт
- контрактов
- может
- создатель
- Текущий
- данным
- занимавшийся
- Проект
- застройщиков
- различный
- открытый
- Разрабатывать
- ETH
- События
- События
- пример
- завод
- Особенности
- в заключение
- First
- фиксированный
- Трансформируемость
- найденный
- функция
- средства
- будущее
- ГАЗ
- Отдаете
- цель
- управление
- Губернатор
- методические рекомендации
- имеющий
- помощь
- здесь
- High
- Как
- HTTPS
- в XNUMX году
- Увеличение
- расширились
- Интерфейс
- IT
- известный
- язык
- последний
- Библиотека
- Лицензия
- линия
- Список
- локальным
- запертый
- смотрел
- Создание
- Совпадение
- модульный
- Multisig
- Другие контрактные услуги
- представить
- процесс
- Проект
- рассматривается
- что такое варган?
- публиковать
- зарегистрироваться
- публикации
- отчету
- хранилище
- Итоги
- обзоре
- безопасность
- набор
- установка
- общие
- умный
- Смарт-контракты
- основательность
- Область
- стиль
- система
- Через
- по всему
- время
- инструменты
- трек
- Сделки
- Updates
- us
- пользователей
- ценностное
- Уязвимости
- Кошельки
- неделя
- КТО
- в
- слова
- письмо