6 grudnia 2021 r.
Wprowadzenie
Połączenia Zestawienie zespół poprosił nas o przegląd i audyt zestawu umów, a ostatecznym celem było ulepszenie umów o wspólnym zarządzaniu i zapewnienie im większej elastyczności. Przyjrzeliśmy się kodowi i teraz publikujemy nasze wyniki.
Przegląd systemu
System opiera się na trzech głównych kontraktach:
- A
SafeGuard
wzór umowy. Niniejsza umowa jestadmin
zTimelock
kontraktu i dodaje warstwę ról modułowych doTimelock
działania. W umowie tej zdefiniowano kilka ról, które mają odrębną odpowiedzialność i dostęp do stanu propozycji (kolejkowanie, anulowanie i wykonanie). - A
SafeGuardFactory
umowa wdraża nowąSafeGuard
i odpowiedni nowyTimelock
kontrakt. Następnie ustawia adres blokady czasowej w plikuSafeGuard
umowę i rejestruje nowąSafeGuard
doRegistry
. - A
Registry
który zawiera listę wdrożonychSafeGuards
z odpowiadającym im Numer wersji.
Połączenia SafeGuardFactory
zasadniczo spawnuje a nowa SafeGuard
kiedykolwiek zostanie wywołany. A SafeGuard
to owinięcie Timelock
operacje który dodaje różne i oddzielne role dla każdej akcji. Role są zorganizowane przy użyciu OpenZeppelin AccessControlEnumerable
umowa zapewniająca większą elastyczność i kompatybilność portfeli multisig i przypadków zastosowań biznesowych, w których kilka adresów może pełnić tę samą wspólną rolę.
aktualizacja: W Nr PR 10, zespół Tally zdecydował się usunąć plik Registry
kontrakt. Lista wdrożonych zabezpieczeń jest teraz przechowywana w pliku SafeGuardFactory
umowa, w safeGuards
przeliczalny zestaw wraz z ich wersją zapisaną w pliku safeGuardVersion
mapowanie.
role
Połączenia SafeGuard
umowa określa następujące role:
aktualizacja: Połączenia CREATOR_ROLE
rola została usunięta w ramach rozwiązania problemu L02.
Zakres
Skontrolowaliśmy zobowiązanie b2c63a9dfc4090be13320d999e7c6c1d842625d3
ukończenia safeguard
magazyn. Zakres obejmuje inteligentne kontrakty w contracts
informator. Jednakże mocks
katalog uznano za wykraczający poza zakres.
Założenia
System nie jest przeznaczony do aktualizacji. The Registry
adres jest ustawiony w konstruktorze ukończenia SafeGuardFactory
i każdy SafeGuard
może mieć Timelock
ustawić tylko raz. Oznacza to, że jeśli nowy Registry
zostaje wdrożony nowy SafeGuardFactory
też trzeba rozłożyć. Jeśli Timelock
zmiany wdrożeniowe, stare SafeGuard
staną się przestarzałe i trzeba będzie wdrożyć nowe.
Co więcej, system w dużym stopniu opiera się na implementacji m.in Timelock
umowa uznano, że nie wchodzi to w zakres niniejszego audytu. Zespół nie zakończył jeszcze wdrażania Timelock
kontrakt. W momencie pisania tego raportu, Implementacja timelocka w Związku jest używany w projekcie.
Baza kodu została skontrolowana przez dwóch audytorów w ciągu tygodnia i poniżej przedstawiamy nasze ustalenia.
Krytyczna dotkliwość
Brak.
Wysoka dotkliwość
[H01] ETH można zamknąć wewnątrz Timelock
umowa
Połączenia Tally
zespół pierwotnie opierał swoje implementacje na podstawie GovernorBravoDelegate
Umowa złożona.
W trakcie tego audytu, Tally
zespół odkryty ograniczenie w gubernatorze Compound, gdzie ETH jest wysyłane bezpośrednio do Timelock
nie jest dostępny do wykorzystania w propozycjach zarządzania i chociaż nie jest zablokowany na stałe, jego odzyskanie wymaga skomplikowanego obejścia.
Dzieje się tak, ponieważ wdrożenie gubernatora wymaga załączenia całej wartości wniosku jako msg.value
przez konto uruchamiające realizację, nie wykorzystując w żaden sposób Timelock
fundusze ETH.
Połączenia ten sam problem został później zidentyfikowany w SafeGuard
realizacja Zespół jest świadomy problemu i jest w trakcie jego rozwiązywania.
Rozwiązując problem, rozważ zastosowanie tego podejścia przyjęte przez bibliotekę OpenZeppelin w tym samym numerze.
aktualizacja: Naprawiono w zatwierdzeniu 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory można zamrozić
Połączenia Registry
Umowa ma na celu śledzenie wszystkich SafeGuards
że SafeGuardFactory
produkuje. Ma to, co zewnętrzne register
funkcjonować który jest używany w tym celu.
W tym samym czasie SafeGuardFactory
ma w swoim konstruktorze przypisanie elementu lokalnego registry
wartość do wartości wejściowej. Nie ma możliwości zmiany wartości registry
zmienna i z tego powodu, jeśli jest nowa Registry
zostanie wdrożony, należy również wdrożyć nową fabrykę.
Połączenia SafeGuardFactory
ma createSafeGuard
funkcjonować, odpowiedzialny za pierwszy wdrażanie nowego SafeGuard
, następnie nowa Timelock
z adresem SafeGuard
as admin
, następnie ustawienie timelock
zmienna SafeGuard
kontrakt i wreszcie rejestracja SafeGuard
w rejestrze.
Problem w tym, że każde wywołanie createSafeGuard
może zostać zmuszony do niepowodzenia przez osobę atakującą, która może bezpośrednio zarejestrować deterministyczny adres nowego SafeGuard
przed jego utworzeniem. Kiedykolwiek umowa tworzy a new
przykład, jego wartość jednorazowa jest zwiększana, a adres miejsca, w którym zostanie wdrożone nowe wystąpienie kontraktu, można określić na podstawie pierwotnego adresu kontraktu i jego wartości jednorazowej. Dlatego osoba atakująca może wstępnie obliczyć wiele adresów, pod którymi znajdują się nowe SafeGuards
zostaną wdrożone i zarejestrują te adresy w pliku Registry
dzwoniąc na register
funkcjonować. Spowodowałoby to wywołania do createSafeGuard
do przywrócić ponieważ Registry
zawiera już adres.
Aby uniknąć sytuacji, w której podmioty zewnętrzne wzywają publicznie do Register
umową, rozważ ograniczenie dostępu do register
funkcja umożliwiająca odbieranie połączeń wyłącznie przez SafeGuardFactory
.
aktualizacja: Naprawiono w Nr PR 10. Zespół Tally usunął plik Registry
kontrakt.
Średni stopień nasilenia
Brak.
Niska dotkliwość
[L01] Zakomentowany kod
Połączenia Registry
umowa obejmuje skomentowana linia kodu. Aby poprawić czytelność, rozważ usunięcie go z bazy kodu.
aktualizacja: Naprawiono w Nr PR 10 i popełnij 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] Administrator SafeGuarda może przypisać rolę twórcy do dowolnego adresu
Połączenia SafeGuard
umowa określa rolę a CREATOR_ROLE
który jak sama nazwa wskazuje przypisany jest twórcy zabezpieczenia.
Jednak wywołując dotychczasowy grantRole
funkcja AccessControlEnumerable
umowa w bibliotece kontraktów OpenZeppelin administrator może przypisać tę rolę dowolnemu adresowi. Może to powodować zamieszanie, ponieważ twórca SafeGuard
może być tylko SafeGuardFactory
.
W całym kodzie ta rola była używana wyłącznie w celu ograniczenia użytkownikom możliwości interakcji z setTimelock
funkcjonować ukończenia SafeGuard
kontrakt. Z założenia system to zapewnia setTimelock
funkcjonować można wywołać tylko raz, od w ciągu SafeGuardFactory
umowa.
Rozważ usunięcie CREATOR_ROLE
rola od SafeGuard
umowy i korzystania z onlyOwner
modyfikator setTimelock
funkcja.
aktualizacja: Naprawiono w Nr PR 10.
[L03] Nieprawidłowa definicja i implementacja interfejsu
Połączenia ISafeGuard
interfejs nie definiuje queueTransactionWithDescription
funkcjonować realizowane w SafeGuard
umowy, a jednocześnie określa __abdicate, __queueSetTimelockPendingAdmin i __executeSetTimelockPendingAdmin funkcje, ale nie są one zaimplementowane.
Aby poprawić poprawność i spójność bazy kodu, rozważ refaktoryzację ISafeGuard
interfejs dokładnie dopasowany do SafeGuard
realizacja.
aktualizacja: Naprawiono w zatwierdzeniu 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Brakujące dokumenty
Niektórym kontraktom i funkcjom w bazie kodu brakuje dokumentacji. Na przykład, niektóre funkcje SafeGuard
kontrakt.
Ponadto w niektórych dokumentach używany jest nieformalny język, taki jak ten wyżej setTimelock
funkcja w SafeGuard
umowa.
Utrudnia to recenzentom zrozumienie intencji kodu, co jest podstawą prawidłowej oceny nie tylko bezpieczeństwa, ale także poprawności. Dodatkowo dokumenty poprawiają czytelność i ułatwiają konserwację. Powinny wyraźnie wyjaśniać cel lub intencję funkcji, scenariusze, w których mogą one zawieść, role, które mogą je wywoływać, zwracane wartości i emitowane zdarzenia.
Rozważ dokładne udokumentowanie wszystkich funkcji (i ich parametrów), które są częścią publicznego interfejsu API kontraktów. Funkcje implementujące wrażliwe funkcje, nawet jeśli nie są publiczne, również powinny być jasno udokumentowane. Pisząc dokumenty, rozważ przestrzeganie Format specyfikacji Ethereum Natural (NatSpec).
aktualizacja: Częściowo naprawiono w Nr PR 10. Do różnych funkcji w całej bazie kodu dodano odpowiednie dokumenty. Jednakże oprócz bieżących zmian rozważ wprowadzenie następujących zmian:
- Dodaj
description
jak@param
w dokumentacji powyżejqueueTransactionWithDescription
funkcjonować - Dodaj
@param
dokumentacja wyżejcreateSafeGuard
funkcja wSafeGuardFactory
umowa - Dodaj
@return
w dokumentach powyżej funkcji wSafeGuardFactory
kontrakt.
[L05] Kod bezużyteczny lub powtarzający się
Są miejsca w bazie kodu, gdzie kod albo się powtarza, albo nie jest potrzebny. Oto kilka przykładów:
- Linie 29-32 ukończenia
Registry
umowy są bezużyteczne, ponieważ_add
funkcjaEnumerableSet
umowa przeprowadził już te kontrole względem wartości już ustawionych. - Linie 62, 67, 73 i 78 ukończenia
SafeGuard
umowy powtarzają dokładnie tę samą operację. Rozważ hermetyzację go w funkcję wewnętrzną, aby uniknąć powielania kodu. - Linie 62-63 i 67-68 of
SafeGuard
są powtarzane. Rozważ hermetyzację ich w jedną funkcję wewnętrzną. - Wykorzystanie
gasleft
określić, ile gazu ma zostać przesłane w wywołaniu funkcjiexecuteTransaction
jest niepotrzebne. Dzieje się tak dlatego, że w tym momencie wykonania cały pozostały gaz zostanie wykorzystany do kontynuowania wykonania. Jeśli nie ma to na celu jednoznaczności, rozważ usunięciegas
parametr z wywołania.
Rozważ zastosowanie sugerowanej poprawki, aby uzyskać czystszy kod i poprawić spójność i modułowość w stosunku do bazy kodu.
aktualizacja: Naprawiono w Nr PR 10 i popełnij 7fd27df16fc879d990d36a167a0b6e719e578558
.
Uwagi i dodatkowe informacje
[N01] Styl niespójny
Są pewne miejsca w bazie kodu, gdzie różnice w stylu wpływają na czytelność, utrudniając zrozumienie kodu. Oto kilka przykładów:
- Połączenia
Registry
kontrakt używa różnych stylów dokumentów w całej umowie. - Połączenia
SafeGuard
kontrakt emituje zdarzenie kiedyqueueTransactionWithDescription
jest wywoływana, ale w innych funkcjach obsługujących transakcje nie są emitowane żadne zdarzenia. - W
SafeGuard
czasami umowa wartość jest używany jako nazwany parametr, a czasami _wartość Jest używane.
Biorąc pod uwagę wartość, jaką spójny styl kodowania dodaje do czytelności projektu, rozważ wymuszenie standardowego stylu kodowania za pomocą narzędzi linterowych, takich jak Solhint.
aktualizacja: Naprawiono w Nr PR 10 i popełnij 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Brak licencji
W następujących kontraktach w bazie kodu brakuje identyfikatora licencji SPDX.
Aby wyciszyć ostrzeżenia kompilatora i zwiększyć spójność w bazie kodu, rozważ dodanie identyfikatora licencji. Robiąc to, rozważ odniesienie się do spdx.dev wytyczne.
aktualizacja: Naprawiono w Nr PR 10 i popełnij 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] Zależność OpenZeppelin Contract nie jest przypięta
Aby zapobiec nieoczekiwanym zachowaniom w przypadku, gdy istotne zmiany zostaną wydane w przyszłych aktualizacjach Biblioteka kontraktów OpenZeppelin, rozważ przypięcie wersji tej zależności w pakiet.json plik.
aktualizacja: Naprawiono w Nr PR 10.
[N04] Wersja kompilatora Solidity nie jest przypięta
W całej bazie kodu rozważ przypięcie wersji pliku Kompilator solidności do najnowszej stabilnej wersji. Powinno to pomóc zapobiec wprowadzaniu nieoczekiwanych błędów z powodu niekompatybilnych przyszłych wydań. Aby wybrać konkretną wersję, programiści powinni wziąć pod uwagę zarówno funkcje kompilatora potrzebne projektowi, jak i lista znanych błędów powiązane z każdą wersją kompilatora Solidity.
aktualizacja: Naprawiono w Nr PR 10.
[N05] Literówka
W różnych miejscach w kodzie słowo role
jest błędnie napisany jako rol
. Jeden z takich przykładów znajduje się w dokumentacja w pliku constructor
ukończenia SafeGuard
umowa.
Rozważ poprawienie tych literówek, aby poprawić czytelność kodu.
aktualizacja: Częściowo naprawiono w Nr PR 10. Podczas gdy pisownia role
zostało poprawione, komentarz „ustaw rolę administratora na zdefiniowany adres administratora” powinien brzmieć „ustaw rolę administratora na zdefiniowany adres administratora”. Ponadto słowo „wykonaj” jest błędnie zapisane w pliku SafeGuard
umowa na Linia 69, Linia 82, Linia 96 i Linia 110 i „dostępne” zostało napisane błędnie Linia 70, Linia 83, Linia 97, Linia 111. Rozważ także zastąpienie nieformalnych słów, takich jak "zamierzać" in SafeGuard
umowę z formalnymi alternatywami, takimi jak „zamierzam”.
[N06] Zadeklaruj uint jako uint256
W bazie kodu istnieje kilka wystąpień, w których deklarowane są zmienne uint
typ danych zamiast uint256
. Na przykład eta
zmienna w QueueTransactionWithDescription
wydarzenie ukończenia SafeGuard
kontrakt.
Aby sprzyjać jednoznaczności, wszystkie przypadki uint
należy zadeklarować jako uint256
.
aktualizacja: Naprawiono w Nr PR 10 i popełnij 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Niewykorzystany import
Połączenia SafeGuard
kontrakt importuje console
umowę, ale nigdy z niej nie korzysta.
Aby poprawić czytelność kodu, rozważ usunięcie wszelkich nieużywanych importów.
aktualizacja: Naprawiono w Nr PR 10.
wnioski
Znaleziono jedną wysoką i kilka innych mniejszych luk, a także zasugerowano zalecenia i poprawki.
- &
- dostęp
- Konto
- w poprzek
- Działania
- działania
- Dodatkowy
- adres
- Admin
- Wszystkie kategorie
- już
- Chociaż
- api
- podejście
- na około
- Audyt
- Gruntownie
- jest
- błędy
- biznes
- wezwanie
- Etui
- Spowodować
- zmiana
- opłata
- kod
- Kodowanie
- wspólny
- Mieszanka
- zamieszanie
- wynagrodzenie
- zawiera
- kontynuować
- umowa
- umowy
- mógłby
- twórca
- Aktualny
- dane
- czynienia
- Wnętrze
- deweloperzy
- różne
- odkryty
- Opracować
- ETH.
- wydarzenie
- wydarzenia
- przykład
- fabryka
- Korzyści
- W końcu
- i terminów, a
- Fix
- Elastyczność
- znaleziono
- funkcjonować
- fundusze
- przyszłość
- GAS
- Dający
- cel
- zarządzanie
- Gubernator
- wytyczne
- mający
- pomoc
- tutaj
- Wysoki
- W jaki sposób
- HTTPS
- realizowane
- Zwiększać
- wzrosła
- Interfejs
- IT
- znany
- język
- firmy
- Biblioteka
- Licencja
- Linia
- Lista
- miejscowy
- zamknięty
- wyglądał
- Dokonywanie
- Mecz
- Modułowa
- Multisig
- Inne
- teraźniejszość
- wygląda tak
- projekt
- wniosek
- publiczny
- publikować
- zarejestrować
- prasowe
- raport
- składnica
- Efekt
- przeglądu
- bezpieczeństwo
- zestaw
- ustawienie
- shared
- mądry
- Inteligentne kontrakty
- solidność
- Stan
- styl
- system
- Przez
- poprzez
- czas
- narzędzia
- śledzić
- transakcje
- Nowości
- us
- Użytkownicy
- wartość
- Luki w zabezpieczeniach
- Portfele
- tydzień
- KIM
- w ciągu
- słowa
- pisanie