16 grudnia 2021 r.
Wprowadzenie
Połączenia 1inch zespół poprosił nas o dokonanie przeglądu i audytu ich Protokół zlecenia limitu inteligentne kontrakty v2. Przyjrzeliśmy się kodowi i teraz publikujemy nasze wyniki.
Zakres
Skontrolowaliśmy zobowiązanie 4d94eea25e4dac6271bfd703096a5c4a4d899b4a
ukończenia 1inch/limit-order-protocol
magazyn. Zakresem objęte były następujące umowy:
- 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
Wszystkie inne pliki i katalogi projektów (w tym testy), wraz z zewnętrznymi zależnościami i projektami, teorią gier i projektami motywacyjnymi, również zostały wyłączone z zakresu tego audytu. Założono, że kod zewnętrzny i zależności kontraktowe działają zgodnie z dokumentacją, a usługi zaplecza świadczone przez 1inch założono, że działają w najlepszym interesie protokołu.
Ogólny stan zdrowia
Ogólnie rzecz biorąc, uznaliśmy, że baza kodu projektu jest czytelna i dobrze zorganizowana, chociaż przydałaby się obszerniejsza dokumentacja, szczególnie dotycząca bloków kodu asemblera, przypadków brzegowych protokołu, zasobów/predykatów/zasobów zewnętrznych, które będą używane, obowiązki/ograniczenia świadczonej usługi zaplecza i interakcje między aktorami. Projekt dokłada wszelkich starań, aby działania były jak najbardziej wydajne, czasami nawet ryzykując, że kod będzie trudniejszy do zrozumienia; poruszamy kwestie z tym związane poniżej. Przez cały czas trwania audytu zespół 1inch był wysoce dostępny, responsywny i bardzo łatwy w obsłudze.
Przegląd systemu
Protokół zlecenia z limitem umożliwia złożenie zamówienia makers
do podpisywania zleceń poza łańcuchem dla swapów tokenowych. Protokół ułatwia wówczas realizację podpisanych wcześniej zamówień na zlecenie takers
. Zamówienia są wysoce rozszerzalne i mogą statycznie wywoływać kontrakty zewnętrzne w kilku punktach procesu realizacji zamówienia. Ta rozszerzalność dodaje protokołowi użyteczności, ale dodaje zarówno złożoności, jak i większej powierzchni ataku dla samych zamówień.
Należy zauważyć, że szczegóły zamówienia nie są przechowywane w łańcuchu. Stan wypełnienia lub status anulowania zamówień jest śledzony wyłącznie za pomocą skrótów zamówień. Wymaga to dzielenia się zamówieniami w trybie peer-to-peer lub za pośrednictwem strony scentralizowanej. W tym przypadku zespół 1inch zamierza działać jako scentralizowana strona, agregując podpisane zlecenia i wykorzystując je jako źródło płynności dla swoich innych protokołów. Zamówienia będą publikowane za pośrednictwem własnego API, dzięki czemu użytkownicy będą mogli z nimi wchodzić w interakcję.
Ta centralizacja daje zespołowi 1inch ekstremalną kontrolę nad tym, które zamówienia są publikowane i ostatecznie wykonywane. Daje im to również możliwość cenzurowania zamówień, co może być przydatne w przypadku zamówień złośliwych lub oszukańczych, ale może również zostać nadużyte i pozwolić im na wyprzedzenie dowolnego innego użytkownika w przypadku korzystnego zamówienia poprzez niepokazywanie go przez API.
Role uprzywilejowane
Chociaż umowy, w których ta rola jest wykorzystywana, wykraczały poza zakres, zidentyfikowano jedną uprzywilejowaną rolę. Jakiś immutableOwner
jest ustawiany na twórcę umowy proxy w momencie jej tworzenia i służy do ograniczania dostępu do proxy external
funkcje.
Zależności zewnętrzne i założenia zaufania
Konstrukcja tego protokołu wymaga komponentów poza łańcuchem i w łańcuchu, a ten model hybrydowy można wykorzystać do ograniczenia niektórych wektorów ataków, które zidentyfikowaliśmy w naszym raporcie, ale kosztem tej możliwości jest zwiększone uzależnienie od 1-calowego zespołu i infrastruktury.
Dodatkowo protokół zlecenia z limitem udostępnia funkcje, które mają na celu pobieranie cen z wyroczni Chainlink. Założyliśmy, że te wyrocznie są uczciwe, dostępne i prawidłowo funkcjonujące.
Ponadto, ze względu na elastyczność zamówienia, istnieje kilka punktów styku z umowami zewnętrznymi, które nie podlegają walidacji. Oznacza to, że złośliwy użytkownik może nadużyć takich wywołań i podszywać się pod predykaty, zasoby lub wyrocznie za pomocą złośliwych kontraktów, aby wykonywać działania podczas realizacji zamówień. Chociaż projekt jest w niektórych obszarach chroniony przed ponownym wejściem, wektory takie mogą powodować ataki typu „odmowa usługi” lub niewykryte zamówienia spamowe. Zespół 1inch jest świadomy, że w przypadku korzystania z nieznanych kontraktów dla protokołu mogą pojawić się pewne problemy, i wyraził swój zamiar, aby w ramach projektu w pełni wspierać tylko główne aktywa „największych firm”. Należy jednak zauważyć, że nawet w przypadku najpopularniejszych aktywów istnieją nieodłączne zachowania każdego zasobu, które mogą powodować problemy w protokołach, które nie obsługują ich prawidłowo, np. pobieranie opłaty podczas przelewów z USDT lub zwracanie kodu błędu zamiast Wartość logiczna sukcesu z cTokenami.
Ustalenia
Tutaj przedstawiamy nasze ustalenia.
Krytyczna dotkliwość
Brak.
Wysoka dotkliwość
[H01] Przekazano niespójne dane _makeCall
W OrderMixin
kontrakt, plik _makeCall
funkcja służy do przenoszenia aktywów od odbiorcy do twórcy , a następnie od twórcy do odbiorcy. W tym ostatnim transferze _makeCall
funkcja została błędnie przekazana do zamówienia makerAsset
jako ostatni parametr, gdy powinien być parametrem zamówienia makerAssetData
.
W rezultacie każda funkcja serwera proxy oparta na makerAssetData
argument zostanie złamany.
Aby zachować spójność z wcześniejszym wezwaniem do _makeCall
i aby w pełni obsługiwać funkcjonalność proxy, rozważ aktualizację order.makerAsset
parametr order.makerAssetData
.
aktualizacja: Naprawiono w żądanie ściągnięcia # 57.
[H02] Częściowo wypełnione zamówienia prywatne mogą być realizowane przez każdego
Protokół umożliwia tworzenie zamówień prywatnych i publicznych. Na zamówienia prywatne wyłącznie allowedSender
adres podany przez producenta podczas tworzenia zamówienia, ma możliwość realizacji zamówienia.
Jednak w OrderMixin
kontrakt, walidacja dla allowedSender
adres ma nieprawidłowy zakres, co oznacza, że jest oceniany tylko w ramach logiki obsługującej pierwsze wypełnienie zamówienia. Jeżeli zamówienie prywatne jest częściowo zrealizowane, wówczas czek na allowedSender
adres nie jest już osiągalny i zamówienie może zostać zrealizowane przez kogokolwiek.
Aby wyjaśnić zamiary dotyczące tego, czy dowolny użytkownik powinien mieć możliwość realizacji częściowo wypełnionych zamówień prywatnych, czy nie, rozważ udokumentowanie przyczyny bieżącego zachowania lub zatwierdzenie allowedSender
adres poza zakresem pierwszego wypełnienia, aby mieć pewność, że zostanie on zweryfikowany przy każdej próbie wypełnienia.
aktualizacja: Naprawiono w żądanie ściągnięcia # 58.
[H03] Złośliwy twórca może wykorzystać częściowe wypełnienia, aby ukraść aktywa odbiorcy
Zamówienia z OrderMixin
kontrakt ma możliwość częściowego wypełnienia. Aby obsługiwać częściowe wypełnienia, protokół wymaga sposobu obliczenia obu stron zamiany. Obydwa getMakerAmount
i getTakerAmount
pola są definiowane przez producenta zamówienia właśnie w tym celu.
Wypełniając zamówienie, odbiorcy muszą podać albo makingAmount
albo takingAmount
wartości, a także a thresholdAmount
wartość. Istnieją dwie różne ścieżki kodu, które można wybrać, w zależności od tego, czy makingAmount
albo takingAmount
był pod warunkiem.
Pierwszy z nich ma miejsce wtedy, gdy makingAmount
parametr jest zdefiniowany. To mogło ścięty dotychczasowy makingAmount
wartość i także Oblicz takingAmount
wartość dla tego. W tej sytuacji thresholdAmount
zapewnia, że takingAmount
przyjmowana wartość to nie nieoczekiwanie duży.
Drugi przypadek to sytuacja, w której takingAmount
parametr jest zdefiniowany. W takim przypadku tak będzie Oblicz makingAmount
wartości, z możliwością obcinając to i przeliczenie takingAmount
wartość, jeśli tak się stanie. W tej sytuacji thresholdAmount
wartość gwarantuje, że makingAmount
zwracana wartość to nie niespodziewanie mały.
Istnieją dwie metody wykorzystania, każda unikalna dla jednej z wyżej wymienionych ścieżek kodu. Te metody wykorzystania wymagają złośliwego oprogramowania getMakerAmount
i getTakerAmount
Funkcje. Prosta implementacja tych funkcji miałaby identyczne zachowanie AmountCalculator
„s getMakerAmount
i getTakerAmount
funkcje, ale z zakodowanym na stałe przełącznikiem, który w razie potrzeby zmusi je do zwrócenia wartości kontrolowanej przez atakującego.
Pierwszy, mniej poważny wzorzec exploita obejmuje pierwszą ścieżkę kodu, w której makingAmount
wartość jest określona w kolejności wypełniania. Złośliwy twórca czekałby na określoną kolejność wypełniania makingAmount
aby pojawić się w pamięci, aby go wyprzedzić. Usunęliby całą wartość z wyjątkiem 1 ze strony twórcy, a następnie wymusiliby _callGetTakerAmount
zwrócić kwotę określoną w koncie użytkownika thresholdAmount
wartość (lub ich dodatek, jeśli jest mniejszy). Kiedy transakcja użytkownika w końcu zostanie zrealizowana, zamieni on całą swoją kwotę thresholdAmount
warto takerAsset
za pojedynczą jednostkę makerAsset
. Ten exploit jest ograniczony kwotą podaną przez thresholdAmount
wartość lub ilość takerAsset
użytkownik dopuszczony na LimitOrderProtocol
kontrakt.
Drugi, poważniejszy wzorzec exploitów obejmuje drugą ścieżkę kodu, w której plik takingAmount
wartość jest określona. Złośliwy twórca w podobny sposób czekałby na polecenie wypełnienia zawierające a takingAmount
wartość, która ma być widoczna w pamięci. Wyprzedziliby transakcję i wymusiliby makingAmount
wartość zwrócona przez _callGetMakerAmount
funkcja jest wyższa niż obie remainingMakerAmount
oraz thresholdAmount
. Ustawiliby także takingAmount
zwrócona wartość przez _callGetTakerAmount
być ilością takerAsset
zasób dozwolony na LimitOrderProtocol
przez odbiorcę. Kiedy transakcja odbiorcy zostanie zrealizowana, tak się stanie obciąć makingAmount
wartość i wtedy przeliczyć takingAmount
wartość. Nie ma jednak gwarancji, że to ponowne obliczenie będzie niższe i w tym przypadku pozbawia przyjmującego wszystkich takerAsset
które dopuścili w umowie. W tej ścieżce kodu thresholdAmount
wartosc jest upewniając się, że makingAmount
nie jest zbyt niski, więc bierzemy wszystkich chętnych takerAsset
zasób nie jest zaznaczony. Utracone środki są ograniczone kwotą takerAsset
zasób, na który użytkownik pozwolił LimitOrderProtocol
kontrakt.
Exploity te nie są możliwe bez częściowych zamówień, a dokładniej częściowych zamówień ze złośliwym oprogramowaniem getMakerAmount
i getTakerAmount
wdrożenia.
Główną kwestią thresholdAmount
sprawdzenie wartości polega na tym, że obejmuje tylko jedną stronę zamiany, ale drugą stroną można manipulować poprzez frontrunning. Nie ma pewności, że wartość pierwotnie zaproponowana przez przyjmującego pozostanie niezmieniona. Rozważ usunięcie makingAmount
obcięcie obu ścieżek kodu i przywrócenie, jeśli zamówienie nie może obsłużyć wypełnienia tak dużego, jak żądano. Robiąc to, thresholdAmount
można wykorzystać do wystarczającego ograniczenia drugiej strony zamiany i uniknięcia nieoczekiwanego zachowania, nawet w przypadku złośliwych zamówień.
aktualizacja: Naprawiono w żądanie ściągnięcia # 83.
Średni stopień nasilenia
[M01] Argumenty statyczne przekazywane po argumentach dynamicznych
W OrderMixin
kontrakt, plik getTakerAmount
i getMakerAmount
Pola bytes są używane jako argumenty dla _callGetTakerAmount
i _callGetMakerAmount
Funkcje. Wywołania te umożliwiają obliczenie jednej strony swapu na podstawie drugiej strony i umożliwiają użytkownikom częściową realizację zamówień.
Połączenia getTakerAmount
/getMakerAmount
pola są zmiennymi dynamicznymi i są upakowane przed takerAmount
i makerAmount
wartości w _callGetTakerAmount
i _callGetMakerAmount
Funkcje. Złośliwy twórca może dostarczyć więcej danych niż oczekiwano w pliku getTakerAmount
igetMakerAmount
pola do pchania takerAmount
i makerAmount
bajtów poza miejscem, w którym zakłada się, że są dekodowane w następnej funkcji. Pozwala to twórcy przesunąć przekazaną kwotę takera lub twórcy o pełne bajty w prawo, a nawet całkowicie je zastąpić, jeśli dostarczone zostaną dodatkowe 32 bajty danych.
Użytkownicy muszą już ręcznie przeglądać plik getTakerAmount
i getMakerAmount
pola w kolejności, ale technika ta jest raczej trudna do wykrycia. Warto również zauważyć, że atak ten dotyczy nawet osób zaufanych wewnętrznie getMakerAmount
i getTakerAmount
Funkcje. W przypadku większości ataków zapewnienie rozsądnej kwoty progowej zapobiegnie utracie środków.
Aby temu zapobiec, rozważ zakodowanie argumentów statycznych przed argumentami dynamicznymi, aby uniknąć nadawania argumentom dynamicznym metody kontrolowania argumentów statycznych.
aktualizacja: Nie naprawiony. Zespół 1 cala stwierdził:
Zwrócimy szczególną uwagę na walidację modułów pobierających. Spróbujemy zaimplementować w naszym SDK weryfikację poprawności modułów pobierających, która pomoże w filtrowaniu potencjalnie złośliwych zamówień.
[M02] Można manipulować zamówieniami ERC721
Za pośrednictwem serwisu można wymieniać nie tylko ERC20 OrderMixin
poprzez wdrożenie kontraktu, który ma ten sam selektor funkcji co IERC20 transferFrom
i udostępnienie tej umowy jako makerAsset
albo takerAsset
w zamówieniu.
Serwery proxy spoza zakresu, tj. ERC721Proxy
, ERC721ProxySafe
, ERC1155Proxy
umowy są zgodne z tym wzorem, aby zapewnić wsparcie ERC721
i ERC1155
żetony. Ponieważ proxy muszą być wywoływane według tego samego wzorca co IERC20 transferFrom
wywołanie, podpis musi zaczynać się od address from
, address to
i uint256 amount
. Wszystko inne, czego wymagają proxy, może zostać przekazane później i jest zdefiniowane w kolejności jako makerAssetData
i takerAssetData
.
Urządzenia ERC1155 mogą naturalnie przesyłać wiele takich samych tokenów identyfikacyjnych jednocześnie, co oznacza ERC1155Proxy
umowa korzysta z amount
pole. Z drugiej strony, ERC721
s nie mają oczywistego zastosowania dla amount
pole. Ponieważ reprezentują tokeny niewymienne, konkretny tokenId będzie miał tylko jeden, co sprawi, że amount
pole bezużyteczne. Z tego powodu wdrożenie dla obu ERC721Proxy
i ERC721ProxySafe
umowy stosują wymagane amount
pole jako tokenId
zamiast.
To przeciążenie amount
parametr stwarza możliwość częściowego wypełnienia ERC721
zamówienia w celu zakupu oddzielnie wystawionych tokenów po obniżonych cenach. Na przykład może zaistnieć sytuacja, w której jeden użytkownik ma wielu ERC721
tej samej umowy, które mogą zostać przeniesione przez ERC721Proxy
kontraktu i wyszczególnia je w oddzielnych zleceniach z limitem.
Jeśli zlecenia z limitem zapewniają również getMakerAmount
i getTakerAmount
pola, możliwe będzie ich częściowe wypełnienie ERC721
Zamówienia. Od zamówienia amount
pole faktycznie odpowiada tokenId
, złośliwy użytkownik może częściowo wypełnić plik ERC721
z wyższym tokenId, co skutkuje a makingAmount
/takingAmount
z ERC721
co mogłoby odpowiadać niższemu tokenId
. Rezultatem jest ERC721
z niższym tokenId
zostaną przeniesione po cenie (higher tokenId price) * (lower tokenId's id) / (higher tokenId's id)
.
Ten exploit ma kilka wymagań:
- Wielokrotność
ERC721
s z tej samej umowy, które mają być dozwolone w obu przypadkachERC721
pełnomocnika jednego właściciela. - Otwórz zamówienie na jeden z
ERC721
to nie jest najniższa wartośćtokenId
z dozwolonych. - Częściowe wypełnienia dozwolone w zamówieniu.
Aby całkowicie usunąć możliwość częściowego ERC721
wypełnienia, rozważ oddzielenie amount
i tokenId
argumenty. Niezależnie od tego, czy argumenty są rozdzielone, czy nie, rozważ również udokumentowanie tego, aby ostrzec użytkowników o tym zachowaniu i uniknąć tego wzorca w przyszłości.
aktualizacja: Naprawiono w żądanie ściągnięcia # 59.
[M03] Nieudokumentowane założenia dziesiętne
Połączenia LimitOrderProtocol
umowa dziedziczy ChainlinkCalculator
umowę za pośrednictwem OrderMixin
kontrakt. Niniejsza umowa udostępnia dwie funkcje umożliwiające korzystanie z wyroczni Chainlink podczas sprawdzenie predykatów i wyszukiwanie ilość producenta/kwota biorcy.
Jednak umowa zawiera nieudokumentowane założenia dotyczące liczby miejsc po przecinku, w jakich wyrocznie Chainlink powinny raportować, a także liczby miejsc po przecinku, które powinny zawierać parametry funkcji. W niektórych scenariuszach może to prowadzić do nieoczekiwanych zachowań, w tym błędnej wyceny aktywów i niezamierzonej utraty środków.
Mówiąc dokładniej, w całej umowie zakłada się, że wyrocznie Chainlink będą raportować z dokładnością do 18 miejsc po przecinku. Jednak nie wszystkie wyrocznie Chainlink raport z tą liczbą miejsc po przecinku. W rzeczywistości, jeśli wyrocznia zgłosi parę tokenów wyrażoną w walucie (na przykład USD), będzie miała tylko 8 miejsc po przecinku. Ponieważ nie ma żadnych ograniczeń dot który można używać wyroczni, nie należy przyjmować ukrytych założeń co do liczby miejsc po przecinku, z jaką będą raportowane.
W związku z tym istnieje ukryte założenie, że amount
parametr dla ChainlinkCalculator
funkcje będą używać 18 miejsc po przecinku wraz z wprowadzającą w błąd wyraźną deklaracją, że singlePrice
funkcjonować Calculates price of token relative to ETH scaled by 1e18
. W rzeczywistości nawet z wyrocznią robi raport z 18 miejscami po przecinku, wartość zwracana przez singlePrice
funkcja będzie skalowana według liczby miejsc po przecinku amount
parametr, który niekoniecznie musi wynosić 18 miejsc po przecinku.
Podobnie, doublePrice
funkcja zakłada, że dwie wyrocznie Chainlink będą raportować z tą samą liczbą miejsc po przecinku, co spowoduje, że wynik funkcji będzie odbiegał od oczekiwań.
Rozważ wyraźne udokumentowanie założeń dotyczących liczby miejsc po przecinku, według których powinny być parametry i zwracane wartości. Ponadto rozważ ograniczenie obliczeń zależnych od wyroczni, które łamią te założenia, lub rozważenie, czy odpowiednie obliczenia uwzględniają rzeczywistą liczbę miejsc po przecinku.
aktualizacja: Naprawiono w żądanie ściągnięcia # 75.
Niska dotkliwość
[L01] Stałe nie zadeklarowane jawnie
W kodzie występuje kilka przypadków użycia wartości dosłownych o niewyjaśnionym znaczeniu. Na przykład:
- W
OrderMixin
kontrakt, plik_remaining
mapowanie jest przeciążone semantycznie (jak wyjaśniono w problemie Semantyczne przeciążenie mapowania), aby śledzić ilość aktywów pozostałą dla częściowo zrealizowanego zamówienia jak również jeżeli zamówienie zostało w całości zrealizowane. Konkretnie,0
oznacza, że nie wykonano żadnych wypełnień związanych z zamówieniem,1
oznacza, że nie można już zrealizować zamówienia i niczego większego niż1
oznacza, że z zamówieniem związana jest pozostała kwota, którą można potencjalnie zrealizować. - W
ChainlinkCalculator
umowy, wartość dosłowna1e18
jest używany wsinglePrice
funkcja.
Aby poprawić czytelność kodu i ułatwić refaktoryzację, rozważ zdefiniowanie stałej dla każdej magicznej liczby, nadając jej jasną i oczywistą nazwę. W przypadku wartości złożonych rozważ dodanie wbudowanego komentarza wyjaśniającego, w jaki sposób zostały obliczone lub dlaczego zostały wybrane.
aktualizacja: Naprawiono w żądanie ściągnięcia # 75 i żądanie ściągnięcia # 76.
[L02] Złośliwe strony mogą uniemożliwić wykonanie dozwolonych zleceń
Połączenia OrderMixin
umowa pozwala użytkownikom na przesyłanie plików dopuszczalne zlecenia dzięki czemu można je wykonać w jednej transakcji, zamiast konieczności posiadania osobnej transakcji do zatwierdzenia. Zleceniodawcy też mogą przedstawić własne zezwolenie podczas realizacji zamówienia w tym samym celu.
Ponieważ jednak zezwolenie producenta jest zawarte w pliku zamówienie, zarówno zezwolenia twórcy, jak i odbiorcy będą dostępne, gdy transakcja realizacji zamówienia będzie znajdować się w pamięci. Umożliwiłoby to każdemu złośliwemu użytkownikowi uzyskanie tych zezwoleń i wykonanie ich na odpowiednich kontraktach dotyczących aktywów, jednocześnie wyprzedzając transakcję wypełnienia. Ponieważ te pozwolenia mają nonce
aby zapobiec atakowi polegającemu na podwójnym wydatkowaniu, transakcja wypełnienia zamówienia nie powiedzie się w wyniku próby użycia tego samego zezwolenia, które zostało właśnie użyte podczas frontrunu.
Chociaż nie ma żadnego zagrożenia bezpieczeństwa, a twórca mógłby utworzyć nowe zamówienie i wstępnie zatwierdzić transakcję, atak ten z pewnością mógłby mieć wpływ na użyteczność dozwolonych zamówień. Rzeczywiście, zmotywowany napastnik może blokować cała kolekcja dopuszczalne rozkazy z tym atakiem. Podczas realizacji zamówienia rozważ sprawdzenie, czy zezwolenie zostało już złożone lub czy zasiłek jest wystarczający. Rozważ także powiadomienie użytkowników o możliwym ataku podczas tworzenia zamówienia.
aktualizacja: Nie naprawiony. Zespół 1 cala stwierdza:
Już wcześniej przeprowadzaliśmy kontrole zatwierdzeń, ale postanowiliśmy uprościć przepływ pozwoleń, aby po prostu przywrócić je w przypadku nieudanych zatwierdzeń. Zastanowimy się, jak powiadomić twórców o problemie.
[L03] Zduplikowany kod
W bazie kodu występują przypadki zduplikowanego kodu. Powielanie kodu może prowadzić do problemów na późniejszym etapie cyklu rozwojowego i sprawia, że projekt jest bardziej podatny na wprowadzanie błędów. Takie błędy mogą zostać przypadkowo wprowadzone, gdy zmiany funkcjonalności nie zostaną odtworzone we wszystkich wystąpieniach kodu, który powinien być identyczny. Przykłady zduplikowanego kodu obejmują:
Zamiast duplikować kod, rozważ posiadanie tylko jednej umowy lub biblioteki zawierającej zduplikowany kod i używanie go, gdy wymagana jest zduplikowana funkcjonalność.
aktualizacja: Częściowo naprawiono w żądanie ściągnięcia # 60.
[L04] Błędny lub wprowadzający w błąd zestaw testów
W zestawie testów występują przypadki, w których testy odbiegają od oczekiwanego zachowania. Na przykład:
- Połączenia
ChainlinkCalculator
umowa jest dziedziczona przezOrderMixin
kontrakt. Jednak podczas testówAmountCalculator.arbitraryStaticCall
Funkcja służy do wywoływaniaChainlinkCalculator
jako umowa zewnętrzna, niezależna. Nawet jeśli wynik jest taki, jakiego oczekiwano, test powinien odzwierciedlać zachowanie przy obecnym projekcie systemu i przewidywanym przypadku użycia, wywołującChainlinkCalculator
działa bezpośrednio, bez użycia dowolnego wywołania statycznego. - Chociaż umowy proxy wykraczały poza zakres, zauważyliśmy, że podczas testowania protokołu z zasobami ERC721 plik
ERC721Proxy
kontrakt nie służy do zamiany znajdujących się w nim aktywów zestaw testów.
Ponieważ sam zestaw testów nie jest objęty zakresem tego audytu, prosimy o dokładne przejrzenie zestawu testów, aby upewnić się, że wszystkie testy przebiegają pomyślnie, zgodnie ze specyfikacjami protokołu.
aktualizacja: Naprawiono w żądanie ściągnięcia # 57, żądanie ściągnięcia # 59, żądanie ściągnięcia # 61.
[L05] Błędy i pominięcia w wydarzeniach
W całym kodzie zdarzenia są zazwyczaj emitowane po wprowadzeniu wrażliwych zmian w kontraktach. Jednak w wielu zdarzeniach brakuje indeksowanych parametrów i/lub ważnych parametrów. Na przykład:
Istnieją również wrażliwe akcje, którym brakuje zdarzeń, takich jak:
Rozważ pełniejsze indeksowanie istniejących zdarzeń i dodanie nowych parametrów tam, gdzie ich brakuje. Rozważ także emisję wszystkich zdarzeń w sposób tak kompletny, aby można je było wykorzystać do odbudowania stanu kontraktu przez usługi poza łańcuchem.
aktualizacja: Nie naprawiony. Jednak zespół 1-calowy dodał orderRemaining
parametr do OrderCanceled
wydarzenie w żądanie ściągnięcia # 62.
Zespół 1 cala stwierdza:
Odkryliśmy, że do zaspokojenia potrzeb frontendu wymagany jest tylko ograniczony podzbiór danych. W przypadku rozbudowanej analizy wszystkie sugerowane pola są dostępne poprzez śledzenie. Dla
OrderRFQMixin
oczekujemy, że animatorzy rynku zbudują własny, wyrafinowany sposób śledzenia anulowanych zamówień.
[L06] Zmiany pamięci podczas emisji zdarzenia
W NonceManager
umowa, gdy NonceIncreased
emitowane jest zdarzenie, Zwiększa się także wartość nonce nadawcy wiadomości.
Wykonywanie wielu operacji jednocześnie może sprawić, że kod będzie trudniejszy do zrozumienia, będzie bardziej podatny na błędy i może prowadzić do przeoczenia lub niezrozumienia operacji.
Aby poprawić ogólną celowość, czytelność i przejrzystość kodu, rozważ zwiększenie wartości jednorazowej przed wyemitowaniem zdarzenia.
aktualizacja: Naprawiono w żądanie ściągnięcia # 63.
[L07] Niespójne metodologie dekodowania mogą powodować rozbieżności w wynikach
Aby zapewnić całą swoją rozszerzalność i elastyczność, protokół Limit Order rutynowo musi radzić sobie z dynamicznymi danymi bajtowymi i dowolnymi wartościami zwracanymi z kontraktów zewnętrznych. W rezultacie protokół zawiera ArgumentsDecoder
bibliotekę, aby efektywniej konwertować wartości bajtów dynamicznych na podstawowe typy danych. Jednak ta biblioteka nie jest używana wyłącznie i w niektórych przypadkach abi.decode
jest używany zamiast tego. Dodatkowo niektóre umowy korzystają abi coder v1
podczas gdy inni korzystają abi coder v2
. Ten pierwszy działa bardziej podobnie do ArgumentsDecoder
biblioteka, podczas gdy ta ostatnia przeprowadza dodatkowe kontrole podczas dekodowania.
Niespójne użycie tych różnych metod dekodowania może skutkować subtelnymi rozbieżnościami między intencją a rzeczywistym zachowaniem bazy kodu.
Na przykład, simulateCalls
funkcja używa tylko ArgumentsDecoder.decodeBool
funkcjonować. Jeśli simulateCalls
funkcja służy do sprawdzania wywołań, które zostałyby wykonane w części predykatu zamówienia, wówczas jej wyniki mogą odbiegać od tego, co faktycznie ma miejsce, gdy oceniane są warunki predykatu, ponieważ stosowane są różne metodologie dekodowania.
Na przykład, jeśli predykat tworzy predykat zewnętrzny staticcall
do jakiejś funkcji, która zwraca a uint256
wartość większą od jedności, a nie oczekiwaną bool
, wówczas to wywołanie zostanie przywrócone, ponieważ zwracaną wartością jest dekodowany za pomocą abi coder v2
„s abi.decode
które nie przyjmą takich wartości jak bool
. Jeśli jednak wykonane zostanie dokładnie to samo wywołanie za pomocą simulateCalls
, potem to zostanie po prostu oznaczony jako true
, Ponieważ decodeBool
traktuje każdą wartość większą od zera jako true
.
Aby simulateCalls
funkcja w pełni odzwierciedla zachowanie rzeczywistych wywołań predykatów, rozważ modyfikację jej w celu użycia abi.decode
.
aktualizacja: Naprawiono w żądanie ściągnięcia # 82.
[L08] Brak walidacji wejścia
Połączenia fillOrderToWithPermit
i fillOrderTo
funkcje OrderMixin
umowy, a także fillOrderRFQToWithPermit
i fillOrderRFQTo
funkcje OrderRFQMixin
umowy, nie potwierdzaj target
parametr adresu.
Dzięki temu użytkownik może przypadkowo podać adres zerowy i w efekcie zablokować aktywa, które ma otrzymać po zrealizowaniu zamówienia.
Aby mieć pewność, że użytkownicy nie zablokują przypadkowo swoich środków, rozważ sprawdzenie, czy plik target
adres nie jest równy adresowi zerowemu w cytowanych funkcjach.
aktualizacja: Naprawiono w żądanie ściągnięcia # 78.
[L09] Niski zasięg testów jednostkowych
Pokrycie testami jednostkowymi dla całego projektu wynosi około 75%, przy czym niektóre kontrakty charakteryzują się szczególnie niskim pokryciem.
Biorąc pod uwagę znaczenie testów jednostkowych do sprawdzania poprawności kodu i zapobiegania regresji podczas refaktoryzacji i opracowywania nowych funkcji, zachęcamy do znacznego zwiększenia zasięgu testów jednostkowych do co najmniej 95% i uwzględnienia przypadków brzegowych, które obejmują nawet mało prawdopodobne sytuacje.
aktualizacja: Nie naprawiony.
[L10] Wprowadzająca w błąd lub niekompletna dokumentacja inline
W całym kodzie zidentyfikowano kilka przypadków wprowadzającej w błąd i/lub niekompletnej dokumentacji wbudowanej i należy je naprawić.
Oto przykłady wprowadzającej w błąd dokumentacji wbudowanej:
- W
ChainlinkCalculator
kontrakt, pliksinglePrice
Funkcje NatSpec@notice
etykieta mówi, że toCalculates price of token relative to ETH scaled by 1e18
, ale w rzeczywistości jego wynikiem jest wartość ofamount
tokeny skalowane według1e18
, gdzie wyrocznia może nie raportować w kategoriach ETH (na przykład dla pary nie zawierającej ETH). - W
OrderRFQMixin
kontrakt, plikinvalidatorForOrderRFQ
Funkcje NatSpec@return
etykieta wprowadza w błąd, ponieważ cudzysłów mógł nie zostać wypełniony w celu ustawienia odpowiedniego bitu unieważniającego. Zamówienie może zostać również anulowane. - Na liniach 147, 165, 188 of
OrderMixin.sol
, NatSpec@param
tagi są niegramatyczne. - Online 20 of
ERC1155Proxy.sol
The@notice
tag stwierdza, że obliczony skrót jest wynikiem mieszaniafunc_733NCGU
funkcję, gdzie powinna byćfunc_301JL5R
zamiast funkcji.
Poniżej znajdują się przypadki niekompletnej dokumentacji wbudowanej:
- Funkcje w
AmountCalculator
umowa nie określa żadnego z parametrów. - W
ChainlinkCalculator
kontrakt, pliksinglePrice
idoublePrice
funkcje nie opisują wszystkich parametrów. - W
ImmutableOwner
kontrakt, zmienna publiczna i modyfikator nie mają NatSpec. - W
InteractiveNotificationReceiver
kontrakt, pliknotifyFillOrder
funkcja nie opisuje żadnego z parametrów. - W
LimitOrderProtocol
kontrakt, plikDOMAIN_SEPARATOR
funkcja nie ma NatSpec. - Wydarzenia i mapowania w
NonceManager
nie mam NatSpec. - W
OrderRFQMixin
kontrakt,cancelOrderRFQ*
funkcje nie opisują zwracanych wartości. - W
OrderMixin
kontraktu, w kilku funkcjach brakuje pełnego NatSpec. - Online 168 of
OrderMixin.sol
i online 71 ofOrderRFQMixin.sol
, brakuje@dev
etykietka. - Funkcje w
PredicateHelper
umowa nie opisuje wszystkich parametrów.
Przejrzysta dokumentacja inline ma fundamentalne znaczenie dla nakreślenia intencji kodu. Niedopasowania pomiędzy dokumentacją wbudowaną a implementacją mogą prowadzić do poważnych błędnych wyobrażeń na temat oczekiwanego zachowania systemu. Rozważ naprawienie tych błędów, aby uniknąć zamieszania zarówno dla programistów, użytkowników, jak i audytorów.
aktualizacja: Częściowo naprawione. Wprowadzająca w błąd dokumentacja, o której mowa w żądanie ściągnięcia # 75 i żądanie ściągnięcia # 77.
Zespół 1 cala stwierdza:
Naprawiliśmy wprowadzające w błąd dokumenty. Uzupełnienie dokumentów nastąpi później.
[L11] Możliwe rozkazy DoS przy użyciu hooków
Połączenia OrderMixin
kontrakt implementuje funkcjonalność umożliwiającą realizację ogólnych zleceń zamiany poza łańcuchem, które mogą mieć warunki powodzenia. Podczas realizacji zamówienia, zamówienie może sprawdź predefiniowane warunki „predykatu”. przed kontynuowaniem wykonywania.
Jednakże, ponieważ te warunki predykatów mogą być ukierunkowane na logikę dowolnego dowolnego kontraktu, złośliwy twórca może oszukać odbiorców, aby uwierzyli, że zamówienie zachowuje się poprawnie i że jest ważne podczas sprawdzania go poza łańcuchem, ale następnie kończy się niepowodzeniem przy próbie wypełnienia tego samego zamówienia na łańcuchu. Ta zmiana w zachowaniu predykatów może zostać dokonana albo poprzez wyprzedzenie jakiegoś zmiennej stanu, od którego zależą predykaty, poprzez sprawdzenie wysłanego gazu lub nawet adresów biorących udział w wywołaniu, albo poprzez inną logikę.
Ponadto, jeśli producent zdefiniował interakcja podczas wymianyThe interactionTarget
kontrakt mógłby się cofnąć lub cofnąć limit, uniemożliwiając pomyślną realizację zamówienia, co zasadniczo prowadziłoby do tego samego rezultatu, co złośliwe predykaty.
Chociaż zasoby nie będą zagrożone, użytkownicy lub boty znajdujące korzystne zamówienie będą obciążeni większym wysiłkiem w zakresie identyfikowania tego rodzaju zamówień zawierających spam, które na pozór mogą wydawać się uzasadnione. Jeżeli nie zidentyfikują tego rodzaju zamówień, poniosą koszty zmarnowanego gazu. Aby zmniejszyć liczbę zamówień zawierających spam, rozważ ograniczenie dostępnych celów dla tych haków. Rozważ także ostrzeżenie użytkowników o tej możliwości przed próbą realizacji zamówień.
aktualizacja: Nie naprawiony. Zespół 1 cala stwierdza:
Zajmiemy się tym na naszym backendzie i zastanowimy się, w jaki sposób powiadomić potencjalnych chętnych o problemie.
[L12] Zaokrąglanie może być niekorzystne dla taker
W OrderMixin
i OrderRFQMixin
umów, gdy zlecenie jest w trakcie realizacji, a przyjmujący przekazuje jedynie a makingAmount
or takingAmount
kwoty, protokół podejmuje próbę obliczenia kwoty odpowiadającej swapowi.
Z tymi obliczeniami wiążą się dwa problemy. Po pierwsze, nie ma dokumentacji ani logiki ograniczającej liczbę miejsc po przecinku, jakich powinny używać parametry kwoty, co omówiliśmy w artykule Nieudokumentowane założenia dziesiętne kwestia.
Druga kwestia jest taka, że w trakcie tych obliczeń protokół rozstrzyga na korzyść producenta. Problem z zaokrągleniami może się znacznie pogorszyć, jeśli złamane zostaną ukryte założenia dotyczące liczby dziesiętnej, ale nawet jeśli wszystko będzie zgodne z oczekiwaniami, zaokrąglenia będą dotyczyć małych, nieparzystych kwot.
Rozważ umożliwienie przyjmującemu określenia minimalnej kwoty makerAsset
aktywa, które chcą otrzymać, łącznie z maksymalną kwotą takerAsset
aktywów, które są skłonni zamienić, tak aby akceptacja wszelkich zaokrągleń była bardziej wyraźna.
aktualizacja: Nie naprawiony. Zespół 1 cala stwierdza:
Kwota progowa powinna wystarczyć do ochrony przyjmującego.
[L13] Sprzeczna obsługa zleceń przy braku parametrów
W OrderMixin
kontrakt, plik fillOrderTo
Funkcja wykonuje wewnętrzne wywołania metody _callGetMakerAmount
i _callGetTakerAmount
działa przy każdej próbie wypełnienia i albo makingAmount
albo takingAmount
parametry wynoszą odpowiednio zero lub jeśli makingAmount
wartość jest większa niż remainingMakerAmount
wartość.
Połączenia _callGetMakerAmount
i _callGetTakerAmount
wywołania spowodują cofnięcie, jeśli zamówienie nie zostało utworzone za pomocą getMakerAmount
or getTakerAmount
parametrów i wykonywane jest częściowe wypełnienie.
An komentarz wbudowany obok _callGetMakerAmount
oraz komentarz wbudowany obok _callGetTakerAmount
twierdzić, że „dozwolone są tylko całe wypełnienia”, jeśli zamówienie nie zostało utworzone za pomocą getMakerAmount
or getTakerAmount
parametry.
Istnieją jednak ścieżki kodu, których to nie dotyczy, ponieważ te ścieżki nie sprawdzają length
z obu getMakerAmount
i getTakerAmount
parametry.
Dokładniej, kiedy taker
określa a takerAmount
wartość dla zamówienia, które ma tylko a getMakerAmount
, chyba że to wezwanie do getMakerAmount
zwraca kwotę większą niż remainingMakerAmount
, częściowe wypełnienie może zostać wykonane wbrew dokumentacji wbudowanej.
To sprawia, że celowość tych ścieżek kodu jest niejasna. Jeśli jest to oczekiwane zachowanie, rozważ modyfikację wbudowanej dokumentacji, aby była bardziej przejrzysta. Jeśli jest to niezamierzone zachowanie, rozważ zawsze sprawdzenie długości obu getMakerAmount
oraz getTakerAmount
parametrów jednocześnie, dzięki czemu implementacja wzmacnia zachowanie opisane w dokumentacji wbudowanej.
aktualizacja: Naprawiono w żądanie ściągnięcia # 79.
[L14] Używanie przestarzałych wywołań Chainlink
Połączenia ChainlinkCalculator
kontrakt ma być używany do wysyłania zapytań do wyroczni Chainlink. Odbywa się to poprzez wykonywanie połączeń do ich latestTimestamp
i latestAnswer
metody, oba zostały przestarzałe. W rzeczywistości metody te nie są już obecne w interfejsie API agregatorów Chainlink od wersji trzeciej.
Aby uniknąć potencjalnych przyszłych niezgodności z wyroczniami Chainlink, rozważ użycie latestRoundData
zamiast tego.
aktualizacja: Naprawiono w żądanie ściągnięcia # 67.
Uwagi i dodatkowe informacje
[N01] Nie importuję interfejsów
Połączenia AggregatorInterface
interfejs wydaje się być podzbiorem kodu skopiowanego z ChainLink
publiczne repozytorium kodu. Pełny interfejs jest zawarty w ChainLink
pakiet kontraktu npm.
Jeśli to możliwe, aby zmniejszyć ryzyko niedopasowania interfejsów i wynikających z nich problemów, zamiast przedefiniowywać i/lub przepisywać interfejsy innego projektu, rozważ zamiast tego użycie interfejsów zainstalowanych poprzez oficjalne pakiety npm.
aktualizacja: Naprawiono w żądanie ściągnięcia # 66.
[N02] Przestarzałe zależności projektu
Podczas instalacji zależności projektu, NPM ostrzega, że jeden z zainstalowanych pakietów, Highlight
, „nie będą już obsługiwane ani otrzymywać aktualizacji zabezpieczeń w przyszłości”.
Nawet jeśli jest mało prawdopodobne, że ten pakiet może spowodować zagrożenie bezpieczeństwa, rozważ uaktualnienie zależności, która używa tego pakietu, do wersji obsługiwanej.
aktualizacja: Naprawiono w żądanie ściągnięcia # 64.
[N03] Zewnętrzne wywołania metod przeglądania nie są wywołaniami statycznymi
W większości kodu protokół jawnie wykonuje wywołania zewnętrzne za pośrednictwem OpenZeppelin functionStaticCall
metoda ograniczająca możliwość zmian stanu, gdy są one albo nieoczekiwane, albo niepożądane. Jednakże w ChainlinkCalculator
umowy, pomimo zamiaru wykonywania wyłącznie połączeń zewnętrznych view
metody na wyroczniach Chainlink, wywołania zewnętrzne w singlePrice
i doublePrice
funkcje nie są tworzone jawnie staticcall
s.
Chociaż nie znaleźliśmy żadnych wynikających z tego bezpośrednich problemów związanych z bezpieczeństwem, aby zmniejszyć powierzchnię ataku, poprawić spójność i wyjaśnić intencje, rozważ użycie jawnego staticcall
s, dla wszystkich połączeń zewnętrznych do view
funkcje w ChainlinkCalculator
kontrakt.
aktualizacja: Nie naprawiony. Zespół 1 cala stwierdza:
Uważamy, że komplikacje składniowe niweczą poprawę spójności.
[N04] Brak wcześniejszych niepowodzeń w przypadku nieprawidłowych zamówień
W OrderMixin
kontrakt, plik fillOrderTo
funkcja obsługuje warunek specjalny, gdy zamówienie nie zostało wcześniej złożone (remainingMakerAmount == 0
), ale nie obsługuje jawnie warunku, gdy zamówienie nie jest już ważne (remainingMakerAmount == 1
).
W tym drugim scenariuszu funkcja ostatecznie powróci, ale dopiero po spaleniu nietrywialnych ilości gazu. Aby wyjaśnić intencje, zwiększyć czytelność i zmniejszyć zużycie gazu, rozważ jawną obsługę scenariusza nieprawidłowego zamówienia na początku funkcji.
aktualizacja: Naprawiono w żądanie ściągnięcia # 68.
[N05] Kontrakty pomocnicze nie są oznaczone jako abstrakcyjne
W Solidity słowo kluczowe abstract
jest używany w przypadku umów, które albo same w sobie nie są umowami funkcjonalnymi, albo nie są przeznaczone do stosowania w tym charakterze. Zamiast, abstract
umowy są dziedziczone przez inne umowy w systemie, tworząc samodzielne umowy funkcjonalne.
W całym kodzie znajdują się przykłady kontraktów pomocniczych, które nie są oznaczone jako abstrakcyjne, mimo że nie są przeznaczone do samodzielnego wdrażania. Na przykład AmountCalculator
, ChainlinkCalculator
, ImmutableOwner
, NonceManager
, PredicateHelper
Wszystkie kontrakty składają się z podstawowego zestawu funkcji, które mają być używane podczas dziedziczenia kontraktów.
Rozważ oznaczenie kontraktów pomocniczych jako abstract
wyraźnie zaznaczyć, że mają one wyłącznie na celu dodanie funkcjonalności do umów, które je dziedziczą.
aktualizacja: Nie naprawiony. Zespół 1 cala stwierdza:
Pomocników tych można wdrożyć osobno. Dziedziczone są wyłącznie w celu oszczędzania gazu.
[N06] Niespójna kolejność funkcji
W całym kodzie kolejność funkcji jest ogólnie zgodna z zalecana kolejność w Przewodniku po stylach Solidity, który jest: constructor
, fallback
, external
, public
, internal
, private
.
Jednakże w OrderMixin
kontrakt, plik public
checkPredicate
funkcja odbiega od przewodnika po stylach, dzieląc na pół external
funkcje.
Aby poprawić ogólną czytelność projektu, rozważ standaryzację kolejności funkcji w całej bazie kodu, zgodnie z zaleceniami Przewodnika po stylu Solidity.
aktualizacja: Naprawiono w żądanie ściągnięcia # 69.
[N07] Niespójny przepływ realizacji zamówień
Połączenia OrderMixin
i RFQOrderMixin
Obie umowy zajmują się realizacją podpisanych zamówień, ale ogólny przepływ zamówień między obiema umowami jest niespójny.
OrderMixin
„s fillOrderTo
funkcja podąża za tym ogólnym przebiegiem (pseudokod):
if ((takingAmount == 0) == (makingAmount == 0))
else if (takingAmount == 0)
else (handle makingAmount == 0) THEN swapTokens
Natomiast RFQOrderMixin
jest analogiczne fillOrderRFQTo
funkcja podąża za tym przepływem (pseudokod):
if (takingAmount == 0 && makingAmount == 0)
else if (takingAmount == 0)
else if (makingAmount == 0)
else revert THEN swapTokens
Z dokumentacji nie wynika, dlaczego pierwszy warunek w każdej z tych dwóch funkcji różni się lub dlaczego takingAmount
i makingAmount
W tej drugiej funkcji oba nie mogą wynosić zero. Również przypadek, gdy zarówno a makingAmount
oraz takingAmount
są o wiele łatwiejsze do uzasadnienia w fillOrderRFQTo
funkcję, ponieważ jest ona obsługiwana wyraźnie w wersji końcowej else
blok.
Aby wyjaśnić intencje i zwiększyć ogólną czytelność kodu, rozważ standaryzację ogólnego przepływu zamówień w tych dwóch umowach lub wyraźne udokumentowanie, dlaczego istnieją różnice.
aktualizacja: Nie naprawiony. Zespół 1 cala stwierdza:
Wynika to z niestandardowych funkcji cenowych w zamówieniach z limitem. Od
getMakerAmount
mogą potencjalnie znacząco różnić się odgetTakerAmount
, pomyśleliśmy, że lepiej nie ustawiać domyślnej opcji dla przyjmującego, ponieważ prawdopodobnie wprowadzi to ich w błąd w przypadkach, gdy te pobierające będą inne.
[N08] Komunikaty o błędach są niespójnie sformatowane lub wprowadzają w błąd
W całej bazie kodu plik require
i revert
Stwierdzono, że komunikaty o błędach, które mają powiadamiać użytkowników o szczególnych warunkach powodujących niepowodzenie transakcji, mają niespójny format.
Na przykład każdy z komunikatów o błędach w liniach 85 z OrderMixin.sol
, 16 z ERC721ProxySafe.sol
, 26 z Permitable.sol
zastosuj inny styl.
Ponadto niektóre komunikaty o błędach wprowadzają w błąd:
Komunikaty o błędach mają na celu powiadamianie użytkowników o nieprawidłowych warunkach, dlatego powinny dostarczać wystarczających informacji, aby można było wprowadzić odpowiednie poprawki w celu interakcji z systemem. Nieinformacyjne komunikaty o błędach znacznie szkodzą ogólnemu doświadczeniu użytkownika, obniżając w ten sposób jakość systemu. Co więcej, niespójnie sformatowane komunikaty o błędach mogą wprowadzić niepotrzebne zamieszanie. Dlatego rozważ przejrzenie całej bazy kodu, aby upewnić się, że każdy require
i revert
instrukcja zawiera komunikat o błędzie, który jest spójnie sformatowany, dokładny, informacyjny i przyjazny dla użytkownika.
aktualizacja: Częściowo naprawiono w żądanie ściągnięcia # 81.
[N09] Niespójne użycie nazwanych zmiennych zwracanych
Występuje niespójne użycie nazwanych zmiennych zwracanych w OrderMixin
kontrakt. Niektóre funkcje zwróć nazwane zmienne, inni zwróć jawne wartości, i inni zadeklaruj nazwaną zmienną zwracaną, ale zastąp ją z wyraźną instrukcją return.
Rozważ przyjęcie spójnego podejścia do zwracania wartości w całej bazie kodu, usuwając wszystkie nazwane zmienne zwracane, jawnie deklarując je jako zmienne lokalne i dodając niezbędne instrukcje zwracania w razie potrzeby. Poprawiłoby to zarówno jednoznaczność, jak i czytelność kodu, a także może pomóc w ograniczeniu regresji podczas przyszłych refaktoryzacji kodu.
aktualizacja: Naprawiono w żądanie ściągnięcia # 73.
[N10] Obliczanie skrótu zamówienia nie jest otwarte dla API
Połączenia external
Funkcje remaining
, remainingRaw
i remainingsRaw
wszyscy oczekują skrótu zamówienia w celu pomyślnej operacji.
Jednak funkcja pomocnicza _hash
, który zwraca skrót zamówienia, ma private
widoczność. Oznacza to, że użytkownicy będą musieli ręcznie spakować części zamówień i ciągi domen, aby uzyskać skrót zamówienia.
Aby uniknąć potencjalnych błędów podczas obliczania skrótu zamówienia i zapewnić użytkownikom metodę generowania odpowiedniego skrótu zamówienia, rozważ rozszerzenie widoczności skrótu _hash
funkcja do public
i refaktoryzacja nazwy na hash
aby zachować spójność z resztą kodu.
aktualizacja: Naprawiono w żądanie ściągnięcia # 74.
[N11] Semantyczne przeciążenie mapowania
Połączenia _remaining
mapowanie w OrderMixin
kontrakt jest semantycznie przeciążony, aby śledzić status zamówień i pozostałą ilość aktywów dostępnych dla tych zamówień.
Trzy stany, które może przyjąć, to:
0
: Hash zamówienia nie został jeszcze wyświetlony.1
: Zamówienie zostało anulowane lub całkowicie zrealizowane.- Każdy
uint
większy niż1
: PozostałemakerAmount
dostępne do wypełnienia na zamówienie plus1
.
To semantyczne przeciążenie wymaga zawijania i rozpakowywania tej wartości podczas lookup
, cancellation
, initialization
, storage
.
Przeciążenie semantyczne i niezbędna logika, aby je włączyć, mogą być podatne na błędy i mogą sprawić, że baza kodu będzie trudniejsza do zrozumienia i uzasadnienia, może również otworzyć drzwi do regresji w przyszłych aktualizacjach kodu.
Aby poprawić czytelność kodu, rozważ śledzenie stanu realizacji zamówień w osobnym mapowaniu.
aktualizacja: Nie naprawiony. Zespół 1inch stwierdził, że poprawka zwiększy koszty gazu dla fillOrder
funkcja.
[N12] Zamówienia z zezwoleniem umożliwiają zawieranie umów dowolnych
Połączenia OrderMixin
umowa dziedziczy Permitable
kontrakt umożliwiający realizację pojedynczego zlecenia transakcyjnego aktywami, które je akceptują permit
wzywa do zmiany uprawnień.
Jednakże wzywa do Permitable
umowa nie sprawdzaj, czy cel jest dozwolonym zasobem ani czy w ogóle jest zasobem, który mógłby pozwolić złośliwemu użytkownikowi na przekazanie adresu dowolnego kontraktu, który mógłby wykonać kolejne wywołanie przed zakończeniem realizacji zamówienia.
Chociaż umowa jest zabezpieczone przed ponownym wejściem, zawsze zalecane jest zmniejszenie powierzchni ataku i zapobieganie wywoływaniu kontraktów zewnętrznych w trakcie realizacji. Rozważ ograniczenie zasobu objętego zezwoleniem do zasobów objętych zamówieniem lub do białej listy zasobów dla protokołu.
aktualizacja: Nie naprawiony. Zespół 1 cala stwierdza:
OrderMixin
w rzeczywistości nie zawiera informacji o rzeczywistych tokenach jakomakerAsset
itakerAsset
czasami są to proxy lub inne umowy pośrednie, a informacje o rzeczywistych tokenach są przechowywane w niektórych dowolnych bajtach. Nie ma zatem realnego sposobu na ograniczenie tego, który zasóbpermit
zostaje wezwany.
[N13] solhint
nie jest nigdy ponownie włączony
W całym kodzie istnieje kilka solhint-disable
wypowiedzi, szczególnie te dostępne online 23 i online 41 of RevertReasonParser.sol
, które nie są zakończone solhint-enable
.
Na rzecz jednoznaczności i możliwie najbardziej restrykcyjnego sposobu wyłączania solhint
, rozważ użycie solhint-disable-line
or solhint-disable-next-line
zamiast tego, podobnie jak line 16 tego samego pliku.
aktualizacja: Naprawiono w żądanie ściągnięcia # 72.
[N14] Literówki
Baza kodu zawiera następujące literówki:
Dodatkowo projekt README
(poza zakresem tej kontroli) zawiera następujące literówki:
Rozważ poprawienie tych literówek, aby poprawić czytelność kodu.
aktualizacja: Naprawiono w żądanie ściągnięcia # 71 i żądanie ściągnięcia # 77.
[N15] Użycie uint
zamiast uint256
Aby sprzyjać jednoznaczności, wszystkie przypadki uint
należy zadeklarować jako uint256
. W szczególności te znajdujące się w for
pętle na liniach 98 i 119 of OrderMixin.sol
i linie 16 i 30 of PredicateHelper.sol
.
aktualizacja: Naprawiono w żądanie ściągnięcia # 70.
wnioski
Znaleziono 3 problemy o dużej wadze. Zaproponowano pewne zmiany, aby zastosować się do najlepszych praktyk i zmniejszyć potencjalną powierzchnię ataku.
- &
- 7
- O nas
- dostęp
- Stosownie
- Konto
- w poprzek
- działać
- działania
- Dodatkowy
- adres
- Korzyść
- Wszystkie kategorie
- Pozwalać
- już
- Chociaż
- kwoty
- analiza
- api
- podejście
- argumenty
- na około
- kapitał
- Aktywa
- Audyt
- Back-end
- Początek
- jest
- BEST
- Najlepsze praktyki
- Bit
- boty
- budować
- wezwanie
- który
- Etui
- Spowodować
- Ogniwo łańcucha
- zmiana
- kontrola
- Wykrywanie urządzeń szpiegujących
- kod
- kompleks
- warunek
- zamieszanie
- Budowa
- zawiera
- umowa
- umowy
- Korekty
- Koszty:
- mógłby
- Para
- twórca
- Waluta
- Aktualny
- dane
- sprawa
- Denial of Service
- wdrażanie
- Wnętrze
- deweloperzy
- oprogramowania
- ZROBIŁ
- różnić się
- różne
- domena
- Podwójna
- dynamiczny
- Wcześnie
- krawędź
- zachęcać
- szczególnie
- ETH.
- wydarzenie
- wydarzenia
- wszystko
- przykład
- wymiana
- spodziewany
- doświadczenie
- Wykorzystać
- Korzyści
- Łąka
- W końcu
- i terminów, a
- Fix
- Elastyczność
- pływ
- obserwuj
- znaleziono
- pełny
- funkcjonować
- fundusze
- przyszłość
- gra
- GAS
- Ogólne
- Dający
- wspaniały
- poprowadzi
- Prowadzenie
- haszysz
- mieszanie
- mający
- pomoc
- Wysoki
- wysoko
- W jaki sposób
- HTTPS
- Hybrydowy
- zidentyfikować
- Rezultat
- wdrożenia
- ważny
- importowanie
- włączony
- Włącznie z
- Zwiększać
- wzrosła
- Informacje
- Informacja
- Infrastruktura
- spostrzeżenia
- zamiar
- odsetki
- Interfejs
- zaangażowany
- problemy
- IT
- duży
- większe
- prowadzić
- prowadzący
- Biblioteka
- Ograniczony
- Linia
- Płynność
- Katalogowany
- wykazy
- miejscowy
- wyglądał
- wyszukiwania
- poważny
- producent
- Dokonywanie
- rynek
- Mempool
- lustro
- model
- większość
- Najbardziej popularne posty
- mianowicie
- Nowe funkcje
- nie zamienialny
- niezniszczalne tokeny
- urzędnik
- koncepcja
- operacje
- Option
- wyrocznia
- zamówienie
- Zlecenia
- Inne
- właściciel
- Wzór
- Popularny
- teraźniejszość
- zapobieganie
- Cena
- wycena
- prywatny
- wygląda tak
- projekt
- projektowanie
- ochrona
- protokół
- zapewniać
- zapewnia
- pełnomocnik
- publiczny
- publikować
- zakup
- jakość
- podnieść
- Rzeczywistość
- zmniejszyć
- poleganie
- raport
- Raporty
- składnica
- wymagania
- REST
- Efekt
- powraca
- przeglądu
- Ryzyko
- rundy
- run
- Sdk
- bezpieczeństwo
- Usługi
- zestaw
- shared
- Akcje
- przesunięcie
- podobny
- Prosty
- mały
- mądry
- Inteligentne kontrakty
- So
- solidność
- spam
- swoiście
- Spędzanie
- Spot
- początek
- Stan
- Zestawienie sprzedaży
- Zjednoczone
- Rynek
- przechowywanie
- styl
- składane
- sukces
- udany
- Z powodzeniem
- wsparcie
- Utrzymany
- Powierzchnia
- Przełącznik
- system
- cel
- test
- Testowanie
- Testy
- Przez
- poprzez
- czas
- razem
- żeton
- Żetony
- śledzić
- Śledzenie
- transakcja
- Zaufaj
- wyjątkowy
- Nowości
- us
- użyteczność
- USD
- USDT
- Użytkownicy
- użyteczność
- wartość
- Zobacz i wysłuchaj
- widoczność
- czekać
- Co
- whitelist
- w ciągu
- bez
- Praca
- wartość
- zero