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
ценности, а также thresholdAmount
ценить. Существует два разных пути кода, которые можно использовать в зависимости от того, makingAmount
или takingAmount
был предоставлен.
Первый - когда makingAmount
параметр определен. Это могло бы усекать домен makingAmount
ценность, а также рассчитать takingAmount
ценность для этого. В этой ситуации thresholdAmount
обеспечивает, чтобы takingAmount
принятое значение не неожиданно большой.
Второй – когда takingAmount
параметр определен. В таком случае это будет рассчитать makingAmount
стоимость, с возможностью усекая это и перерасчет takingAmount
ценность, если это произойдет. В этой ситуации thresholdAmount
значение гарантирует, что makingAmount
возвращаемое значение не неожиданно маленький.
Существует два метода эксплуатации, каждый из которых уникален для одного из ранее упомянутых путей кода. Эти методы эксплуатации требуют вредоносного getMakerAmount
и getTakerAmount
функции. Простая реализация этих функций будет иметь такое же поведение, как и AmountCalculator
«s getMakerAmount
и getTakerAmount
функции, но с жестко запрограммированным переключателем, который заставит их при необходимости возвращать значение, контролируемое злоумышленником.
Первый, менее серьезный шаблон эксплойта включает в себя первый путь кода, где makingAmount
значение указано в порядке заполнения. Злоумышленник будет ждать заказа на исполнение, в котором указано makingAmount
появиться в мемпуле, чтобы опередить его. Они выкачают всю ценность, кроме 1, со стороны производителя, а затем заставят _callGetTakerAmount
вернуть сумму, указанную в пользовательском аккаунте thresholdAmount
стоимости (или их надбавки, если она меньше). Когда транзакция пользователя, наконец, пройдет, он полностью поменяет свои thresholdAmount
Стоимость takerAsset
за одну единицу makerAsset
. Этот эксплойт ограничен суммой, заданной thresholdAmount
стоимость или сумма takerAsset
пользователь разрешил на LimitOrderProtocol
контракт.
Второй, более серьезный шаблон эксплойта включает в себя второй путь кода, где takingAmount
значение указано. Злоумышленник аналогичным образом будет ждать заказа на исполнение, в котором указан takingAmount
значение, которое будет отображаться в мемпуле. Они бы инициировали транзакцию и заставили бы makingAmount
значение, возвращаемое _callGetMakerAmount
функция должна быть выше, чем обе remainingMakerAmount
и thresholdAmount
. Они также установили бы takingAmount
возвращаемое значение по _callGetTakerAmount
быть количеством takerAsset
актив, разрешенный к использованию LimitOrderProtocol
со стороны берущего. Когда транзакция тейкера пройдет, она усечь makingAmount
значение, а затем пересчитать takingAmount
ценить. Однако этот перерасчет не гарантированно будет ниже, и в этом случае он лишит получателя всех takerAsset
что они позволили по контракту. В этом пути кода thresholdAmount
Значение обеспечение того, чтобы makingAmount
не слишком низкий, поэтому берём всех берущих takerAsset
актив не проверен. Потерянные средства ограничены суммой takerAsset
актив, который пользователь разрешил на LimitOrderProtocol
контракт.
Эти эксплойты невозможны без частичных заказов и, точнее, частичных заказов с вредоносными getMakerAmount
и getTakerAmount
Реализации.
Главный вопрос в thresholdAmount
Проверка стоимости заключается в том, что она охватывает только одну сторону свопа, но другой стороной можно манипулировать посредством опережающего запуска. Нет никаких гарантий, что первоначально предложенная получателем стоимость останется неизменной. Рассмотрите возможность удаления makingAmount
усечение обоих путей кода и возврат, если заказ не может поддерживать запрошенное заполнение. Сделав это, thresholdAmount
может использоваться для достаточного ограничения другой стороны обмена и предотвращения неожиданного поведения, даже в злонамеренных заказах.
Обновление: Исправлено в запрос на вытягивание # 83.
Средней степени тяжести
[M01] Статические аргументы передаются после динамических аргументов
В OrderMixin
контракт, getTakerAmount
и getMakerAmount
Поля байтов используются в качестве аргументов для _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
не имеют очевидного применения для amount
поле. Поскольку они представляют собой невзаимозаменяемые токены, для конкретного идентификатора токена будет существовать только один, что делает amount
поле бесполезно. По этой причине реализация для обоих ERC721Proxy
и ERC721ProxySafe
контракты используют необходимые amount
поле как tokenId
.
Эта перегрузка amount
параметр создает возможность частичного заполнения ERC721
заказы на покупку отдельно перечисленных токенов по сниженным ценам. Например, может быть случай, когда у одного пользователя есть несколько ERC721
положений того же договора, разрешенных к передаче ERC721Proxy
контракт и размещает их в отдельных лимитных ордерах.
Если лимитные ордера также обеспечивают getMakerAmount
и getTakerAmount
поля, то можно будет частично заполнить эти ERC721
заказы. Поскольку заказ amount
поле фактически соответствует tokenId
, злоумышленник может частично заполнить ERC721
с более высоким tokenId, что приводит к makingAmount
/takingAmount
из ERC721
что может соответствовать более низкому tokenId
. Результатом является ERC721
с нижней tokenId
будет передано по цене (higher tokenId price) * (lower tokenId's id) / (higher tokenId's id)
.
Этот эксплойт имеет несколько требований:
- множественный
ERC721
из одного и того же контракта допускается либоERC721
прокси от одного владельца. - Открытый ордер на один из
ERC721
это не самое низкое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
контракт позволяет пользователям-производителям отправлять допустимые заказы поэтому их можно выполнить за одну транзакцию, вместо того, чтобы иметь отдельную транзакцию для утверждения. Кроме того, получатели заказов могут подать собственное разрешение при оформлении заказа с той же целью.
Однако, поскольку разрешение производителя содержится внутри заказразрешения как производителя, так и получателя будут доступны, пока транзакция исполнения заказа находится в мемпуле. Это позволит любому злоумышленнику получить эти разрешения и выполнить их по соответствующим контрактам на активы, одновременно выполняя транзакцию заполнения. Поскольку эти разрешения имеют 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
какой-то функции, которая возвращает uint256
значение больше единицы, а не ожидаемое bool
, то этот вызов вернется, потому что возвращаемое значение декодированный с помощью abi coder v2
«s 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
функция-х НатСпец@notice
день говорит, что этоCalculates price of token relative to ETH scaled by 1e18
, но на самом деле его результатом является ценностное ofamount
токены, масштабированные по1e18
, где оракул может не сообщать данные в единицах ETH (например, для пары, не включающей ETH). - В
OrderRFQMixin
контракт,invalidatorForOrderRFQ
функция-х НатСпец@return
день вводит в заблуждение, поскольку кавычка могла не быть заполнена для установки соответствующего бита инвалидатора. Также заказ можно было отменить. - На линиях 147, 165и 188 of
OrderMixin.sol
, НацСпец@param
теги неграмматические. - В сети 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. - В сети 168 of
OrderMixin.sol
и онлайн 71 ofOrderRFQMixin.sol
, здесь отсутствует@dev
тег. - Функции в
PredicateHelper
контракт не описывает все параметры.
Четкая встроенная документация имеет основополагающее значение для описания целей кода. Несоответствия встроенной документации и реализации могут привести к серьезным заблуждениям о том, как система должна себя вести. Рассмотрите возможность исправления этих ошибок, чтобы избежать путаницы среди разработчиков, пользователей и аудиторов.
Обновление: Частично исправлено. Вводящая в заблуждение документация, рассматриваемая в запрос на вытягивание # 75 и запрос на вытягивание # 77.
Команда 1inch заявляет:
Мы исправили вводящую в заблуждение документацию. Оформление документации будет завершено позже.
[L11] Возможны DoS-приказы при использовании хуков
Ассоциация OrderMixin
Контракт реализует функциональность для выполнения общих своп-ордеров вне сети, которые могут иметь условия для их успеха. Во время выполнения заказа заказ может проверьте предопределенные условия «предиката» прежде чем продолжить выполнение.
Однако, поскольку эти предикатные условия могут быть нацелены на логику любого произвольного контракта, злоумышленник может обмануть получателей, заставив их поверить в то, что заказ ведет себя правильно и что он действителен при проверке его вне цепочки, но затем потерпит неудачу при попытке выполнить тот же заказ. по цепочке. Это изменение в поведении предикатов может быть осуществлено либо путем запуска некоторого переменного состояния, от которого зависят предикаты, путем проверки отправленного газа или даже того, какие адреса участвуют в вызове, либо с помощью какой-либо другой логики.
Кроме того, если производитель определил взаимодействие во время обмена, interactionTarget
контракт может отменить свое действие или отозвать разрешение, чтобы предотвратить успешное выполнение заказа, что по сути приведет к тому же результату, что и злонамеренные предикаты.
Хотя активы не будут подвергаться риску, пользователи или боты, обнаружившие благоприятный заказ, будут нести дополнительную нагрузку, пытаясь идентифицировать такие виды спам-заказов, которые на первый взгляд могут показаться законными. В случае, если они не смогут идентифицировать такого рода заказы, они понесут напрасные расходы на газ. Чтобы уменьшить количество спам-заказов, рассмотрите возможность ограничения доступных целей для этих перехватчиков. Также рассмотрите возможность предупреждения пользователей об этой возможности, прежде чем они попытаются выполнить заказы.
Обновление: Не зафиксировано. Команда 1inch заявляет:
Мы решаем эту проблему на нашем бэкэнде и подумаем о том, как уведомить возможных пользователей о проблеме.
[L12] Округление может быть неблагоприятным для taker
В OrderMixin
и OrderRFQMixin
контракты, когда ордер исполняется, а тейкер предоставляет только makingAmount
or takingAmount
сумма, протокол пытается вычислить соответствующую сумму свопа.
В этих вычислениях есть две проблемы. Первая из них заключается в том, что нет документации или логики, ограничивающей количество десятичных знаков, которые должны использоваться в параметрах суммы, о чем мы говорили в разделе Недокументированные десятичные предположения вопрос.
Вторая проблема заключается в том, что в ходе этих расчетов протокол округляется в пользу производителя. Проблема округления может значительно усугубиться, если неявные предположения о десятичных дробях нарушаются, но даже если все соответствует ожидаемым значениям, округление будет происходить с небольшими, нечетными суммами.
Рассмотрите возможность предоставления получателю возможности указывать минимальную сумму makerAsset
актив, который они готовы получить вместе с максимальной суммой takerAsset
актив, который они готовы обменять, чтобы принятие любого округления было более явным.
Обновление: Не зафиксировано. Команда 1inch заявляет:
Пороговая сумма должна быть достаточной для защиты тейкера.
[L13] Противоречивая обработка заказов при отсутствии параметров
В OrderMixin
контракт, fillOrderTo
функция выполняет внутренние вызовы _callGetMakerAmount
и _callGetTakerAmount
функционирует всякий раз, когда предпринимается попытка заполнения, и либо makingAmount
или takingAmount
параметры равны нулю, соответственно, или если makingAmount
значение больше, чем remainingMakerAmount
значения.
Ассоциация _callGetMakerAmount
и _callGetTakerAmount
вызовы приведут к возврату, если заказ был создан не с помощью getMakerAmount
or getTakerAmount
параметры соответственно и выполняется частичное заполнение.
An встроенный комментарий рядом _callGetMakerAmount
и встроенный комментарий рядом _callGetTakerAmount
утверждать, что «допускаются только целые исполнения», если заказ был создан не с помощью getMakerAmount
or getTakerAmount
параметры.
Однако существуют пути кода, к которым это неприменимо, поскольку эти пути не проверяют length
с обоих getMakerAmount
и getTakerAmount
параметры.
В частности, когда taker
указывает takerAmount
стоимость заказа, который имеет только 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
«s 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
не могут оба быть нулевыми в последней функции. Кроме того, случай, когда оба makingAmount
и еще один takingAmount
гораздо легче рассуждать в fillOrderRFQTo
функция, поскольку она явно обрабатывается в финале else
блок.
Чтобы прояснить намерения и повысить общую читаемость кода, рассмотрите возможность стандартизации общего потока заказов по этим двум контрактам или явного документирования причин существования различий.
Обновление: Не зафиксировано. Команда 1inch заявляет:
Это связано с пользовательскими функциями ценообразования в лимитных ордерах. С
getMakerAmount
потенциально может существенно отличаться отgetTakerAmount
, мы подумали, что лучше не устанавливать опцию по умолчанию для получателя, поскольку это, вероятно, запутает его в тех случаях, когда эти получатели будут разными.
[N08] Сообщения об ошибках имеют непоследовательный формат или вводят в заблуждение.
По всей кодовой базе require
и revert
Сообщения об ошибках, предназначенные для уведомления пользователей о конкретных условиях, приводящих к сбою транзакции, оказались несогласованно отформатированы.
Например, каждое из сообщений об ошибках в строках 85 из OrderMixin.sol
, 16 из ERC721ProxySafe.sol
и 26 из Permitable.sol
использовать другой стиль.
Кроме того, некоторые сообщения об ошибках вводят в заблуждение:
Сообщения об ошибках предназначены для уведомления пользователей о сбоях, поэтому они должны предоставлять достаточно информации, чтобы можно было внести соответствующие исправления для взаимодействия с системой. Неинформативные сообщения об ошибках сильно ухудшают общее впечатление пользователя, тем самым снижая качество системы. Более того, непоследовательно отформатированные сообщения об ошибках могут привести к ненужной путанице. Поэтому рассмотрите возможность проверки всей базы кода, чтобы убедиться, что все require
и revert
Оператор содержит сообщение об ошибке, которое имеет последовательный формат, точное, информативное и удобное для пользователя.
Обновление: Частично исправлено в запрос на вытягивание # 81.
[N09] Непоследовательное использование именованных возвращаемых переменных
Существует непоследовательное использование именованных возвращаемых переменных в OrderMixin
договор. Некоторые функции возвращать именованные переменныедругие возвращать явные значения, и другие объявить именованную возвращаемую переменную, но переопределить ее с явным оператором возврата.
Рассмотрите возможность применения единообразного подхода к возвращаемым значениям во всей базе кода, удалив все именованные возвращаемые переменные, явно объявив их как локальные переменные и добавив необходимые операторы возврата, где это необходимо. Это улучшит как четкость, так и читаемость кода, а также может помочь уменьшить регрессии во время будущих рефакторингов кода.
Обновление: Исправлено в запрос на вытягивание # 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
вызывается.
[Н13] 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
- О нас
- доступ
- По
- Учетная запись
- через
- Действие (Act):
- действия
- дополнительный
- адрес
- плюс
- Все
- Позволяющий
- уже
- Несмотря на то, что
- суммы
- анализ
- API
- подхода
- Аргументы
- около
- активы
- Активы
- аудит
- Back-конец
- начало
- не являетесь
- ЛУЧШЕЕ
- лучшие практики
- Немного
- боты
- строить
- призывают
- заботится
- случаев
- Вызывать
- Звено цепи
- изменение
- контроль
- Проверки
- код
- комплекс
- состояние
- замешательство
- строительство
- содержит
- контракт
- контрактов
- исправления
- Расходы
- может
- Пара
- создатель
- Валюта
- Текущий
- данным
- сделка
- Отказ в обслуживании
- развертывание
- Проект
- застройщиков
- Развитие
- DID
- отличаться
- различный
- домен
- двойной
- динамический
- Рано
- Edge
- поощрять
- особенно
- ETH
- События
- События
- многое
- пример
- обмена
- ожидаемый
- опыт
- Эксплуатировать
- Особенности
- Поля
- в заключение
- First
- фиксированный
- Трансформируемость
- поток
- следовать
- найденный
- полный
- функция
- средства
- будущее
- игра
- ГАЗ
- Общие
- Отдаете
- большой
- инструкция
- Управляемость
- хэш
- Хеширования
- имеющий
- помощь
- High
- очень
- Как
- HTTPS
- Гибридный
- определения
- Влияние
- осуществлять
- важную
- импортирующий
- включены
- В том числе
- Увеличение
- расширились
- info
- информация
- Инфраструктура
- размышления
- намерение
- интерес
- Интерфейс
- вовлеченный
- вопросы
- IT
- большой
- больше
- вести
- ведущий
- Библиотека
- Ограниченный
- линия
- Ликвидность
- Включенный в список
- Списки
- локальным
- смотрел
- поиск
- основной
- производитель
- Создание
- рынок
- Mempool
- зеркало
- модель
- самых
- Самые популярные
- а именно
- Новые функции
- без взаимозаменяемыми
- невзаимозаменяемых токенов
- Официальный представитель в Грузии
- открытый
- Операционный отдел
- Опция
- оракул
- заказ
- заказы
- Другое
- владелец
- шаблон
- Популярное
- представить
- предупреждение
- цена
- цены
- частная
- процесс
- Проект
- проектов
- защиту
- протокол
- обеспечивать
- приводит
- полномочие
- что такое варган?
- публиковать
- покупки
- повышение
- Реальность
- уменьшить
- опора
- отчету
- Отчеты
- хранилище
- Требования
- ОТДЫХ
- Итоги
- Возвращает
- обзоре
- Снижение
- туры
- Run
- SDK
- безопасность
- Услуги
- набор
- общие
- Акции
- сдвиг
- аналогичный
- просто
- небольшой
- умный
- Смарт-контракты
- So
- основательность
- спам
- конкретно
- Расходы
- Спотовая торговля
- Начало
- Область
- заявление
- Области
- Статус:
- диск
- стиль
- представленный
- успех
- успешный
- Успешно
- поддержка
- Поддержанный
- Поверхность
- Коммутатор
- система
- цель
- тестXNUMX
- Тестирование
- тестов
- Через
- по всему
- время
- вместе
- знак
- Лексемы
- трек
- Отслеживание
- сделка
- Доверие
- созданного
- Updates
- us
- юзабилити
- USD
- USDT
- пользователей
- утилита
- ценностное
- Вид
- видимость
- ждать
- Что
- Белый список
- в
- без
- Работа
- стоимость
- нуль