6. Dezember 2021
Einleitung
Das Übereinstimmen Team hat uns gebeten, eine Reihe von Verträgen zu überprüfen und zu auditieren, mit dem Endziel, Common-Governance-Verträge zu verbessern und ihnen mehr Flexibilität zu verleihen. Wir haben uns den Code angeschaut und veröffentlichen nun unsere Ergebnisse.
Systemübersicht
Das System stützt sich auf drei Hauptverträge:
- A
SafeGuard
Vertragsmuster. Dieser Vertrag ist dieadmin
einerTimelock
Vertrag und fügt eine Schicht modularer Rollen über dem hinzuTimelock
's Aktionen. Dieser Vertrag definiert mehrere Rollen, die separate Verantwortung und Zugriff auf den Status eines Angebots haben (Warteschlange, Stornierung und Ausführung). - A
SafeGuardFactory
Vertrag stellt eine neue bereitSafeGuard
und ein entsprechendes neuesTimelock
Vertrag. Dann setzt es die Timelock-Adresse in derSafeGuard
Vertrag und registriert den neuenSafeGuard
in dieRegistry
. - A
Registry
die eine Liste der bereitgestellten enthältSafeGuards
mit ihren entsprechenden Versionsnummer.
Das SafeGuardFactory
wird im Grunde a spawnen neu SafeGuard
wann immer es gerufen wird. EIN SafeGuard
ist ein Rundumschlag Timelock
Geschäftstätigkeit das unterschiedliche und getrennte Rollen für jede Aktion hinzufügt. Rollen werden durch die Verwendung von OpenZeppelin's strukturiert AccessControlEnumerable
Vertrag, der Multisig-Wallets und Geschäftsanwendungsfällen mehr Flexibilität und Kompatibilität verleiht, bei denen mehrere Adressen die gleiche gemeinsame Rolle haben können.
Update: Im PR # 10, hat das Tally-Team beschlossen, die zu entfernen Registry
Vertrag. Die Liste der eingesetzten Schutzmaßnahmen wird jetzt in gespeichert SafeGuardFactory
Vertrag, im safeGuards
aufzählbaren Satz, zusammen mit ihrer Version, die in der gespeichert ist safeGuardVersion
Kartierung.
Rollen
Das SafeGuard
Der Vertrag definiert die folgenden Rollen:
Update: Das CREATOR_ROLE
Rolle wurde als Fix für das Problem L02 entfernt.
Geltungsbereich
Wir haben Commitment geprüft b2c63a9dfc4090be13320d999e7c6c1d842625d3
dauert ebenfalls 3 Jahre. Das erste Jahr ist das sog. safeguard
Repository. In den Anwendungsbereich fallen die Smart Contracts in der contracts
Verzeichnis. Allerdings ist die mocks
Verzeichnis wurde als außerhalb des Geltungsbereichs betrachtet.
Annahmen
Das System soll nicht erweiterbar sein. Das Registry
Adresse eingestellt im Konstrukteur dauert ebenfalls 3 Jahre. Das erste Jahr ist das sog. SafeGuardFactory
und jede SafeGuard
kann das haben Timelock
nur einmal einstellen. Das bedeutet, wenn ein neues Registry
wird neu eingesetzt SafeGuardFactory
müssen auch eingesetzt werden. Wenn die Timelock
Implementierungsänderungen, die alte SafeGuard
s werden obsolet, und neue müssen bereitgestellt werden.
Darüber hinaus stützt sich das System stark auf die Implementierung von a Timelock
Vertrag die für diese Prüfung als außerhalb des Geltungsbereichs erachtet wurde. Das Team hat die Implementierung noch nicht abgeschlossen Timelock
Vertrag. Zum Zeitpunkt der Erstellung dieses Berichts die Implementierung von Timelock in Compound wird im Projekt verwendet.
Die Codebasis wurde im Laufe einer Woche von zwei Auditoren geprüft und hier präsentieren wir unsere Ergebnisse.
Kritische Schwere
Keiner.
Hoher Schweregrad
[H01] ETH kann im Inneren gesperrt werden Timelock
Vertrag
Das Tally
Team basierte seine Implementierungen ursprünglich auf dem Boden der GovernorBravoDelegate
Zusammengesetzter Vertrag.
Im Rahmen dieser Prüfung wird die Tally
Team entdeckt eine Einschränkung im Gouverneur von Compound, wo die ETH direkt an die gesendet hat Timelock
steht nicht für die Verwendung durch Governance-Vorschläge zur Verfügung, und obwohl es nicht dauerhaft hängen bleibt, erfordert es eine aufwändige Problemumgehung, um abgerufen zu werden.
Dies liegt daran, dass die Governor-Implementierung erfordert, dass der gesamte Wert eines Vorschlags als beigefügt wird msg.value
durch das Konto, das die Ausführung auslöst, in keiner Weise die Timelock
ETH-Gelder.
Das Das gleiche Problem wurde später in der identifiziert SafeGuard
Implementierung und das Team ist sich des Problems bewusst und ist dabei, es zu beheben.
Ziehen Sie beim Beheben des Problems die Verwendung des Ansatzes in Betracht von der OpenZeppelin-Bibliothek für das gleiche Problem übernommen.
Update: Im Commit behoben 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory kann eingefroren werden
Das Registry
Vertrag soll den Überblick über alle behalten SafeGuards
, dass die SafeGuardFactory
produziert. Es hat das Äußere register
Funktion die für diesen Zweck verwendet wird.
Zur gleichen Zeit, das SafeGuardFactory
hat in seinem Konstruktor die Zuweisung des Lokalen registry
Wert auf den Eingabewert. Es gibt keine Möglichkeit, den Wert von zu ändern registry
variabel und aus diesem Grund, wenn eine neue Registry
eingesetzt wird, muss auch eine neue Fabrik errichtet werden.
Das SafeGuardFactory
hat der createSafeGuard
Funktion, zuerst zuständig Bereitstellung einer neuen SafeGuard
und dann ein neuer Timelock
mit der Adresse des SafeGuard
as admin
, dann die einstellen timelock
variabel der SafeGuard
Vertrag und schließlich Registrierung der SafeGuard
in der Registrierung.
Das Problem ist, dass jeder Anruf an createSafeGuard
kann von einem Angreifer zum Scheitern gezwungen werden, der die deterministische Adresse des Neuen direkt registrieren kann SafeGuard
vor seiner Entstehung. Immer wenn ein Vertrag schafft ein new
Instanz, seine Nonce wird erhöht, und die Adresse, an der die neue Vertragsinstanz bereitgestellt werden würde, kann anhand der ursprünglichen Vertragsadresse und ihrer Nonce bestimmt werden. Daher kann ein Angreifer viele der Adressen vorausberechnen, wo die neuen SafeGuards
bereitgestellt werden und diese Adressen in der registrieren Registry
indem Sie die anrufen register
Funktion. Dies würde zu den Aufrufen führen createSafeGuard
zu zurückkehren Da die Registry
enthält bereits die Adresse.
Um zu vermeiden, dass externe Akteure das öffentlich nennen Register
Vertrag, erwägen Sie, den Zugriff auf die einzuschränken register
Funktion Anrufe ausschließlich von der anzunehmen SafeGuardFactory
.
Update: Fest eingebaut PR # 10. Das Tally-Team hat die entfernt Registry
Vertrag.
Mittlere Schwere
Keiner.
Geringe Schwere
[L01] Code auskommentiert
Das Registry
Vertrag beinhaltet eine auskommentierte Codezeile. Um die Lesbarkeit zu verbessern, sollten Sie erwägen, es aus der Codebasis zu entfernen.
Update: Fest eingebaut PR # 10 und begehen 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] Der Administrator von SafeGuard kann jeder Adresse die Rolle des Erstellers zuweisen
Das SafeGuard
Vertrag definiert die Rolle von a CREATOR_ROLE
die, wie der Name schon sagt, dem Ersteller der Sicherung zugeordnet ist.
Allerdings durch Aufruf grantRole
Funktion des AccessControlEnumerable
Vertrag In der OpenZeppelin-Vertragsbibliothek kann ein Administrator diese Rolle jeder Adresse zuweisen. Dies könnte zu Verwirrung führen, da die Schöpfer der SafeGuard
kann nur die sein SafeGuardFactory
.
In der gesamten Codebasis wurde diese Rolle nur verwendet, um Benutzer daran zu hindern, mit dem zu interagieren setTimelock
Funktion dauert ebenfalls 3 Jahre. Das erste Jahr ist das sog. SafeGuard
Vertrag. Das System stellt dies konstruktionsbedingt sicher setTimelock
Funktion kann nur einmal aufgerufen werden, von innerhalb der SafeGuardFactory
Vertrag.
Ziehen Sie in Betracht, die CREATOR_ROLE
Rolle aus der SafeGuard
Vertrag und Nutzung der onlyOwner
Modifikator der setTimelock
Funktion.
Update: Fest eingebaut PR # 10.
[L03] Falsche Schnittstellendefinition und -implementierung
Das ISafeGuard
Schnittstelle definiert nicht die queueTransactionWithDescription
Funktion umgesetzt in der SafeGuard
Vertrag, und gleichzeitig definiert es die __abdicate, __queueSetTimelockPendingAdmin und __executeSetTimelockPendingAdmin Funktionen, aber sie sind nicht implementiert.
Um die Korrektheit und Konsistenz in der Codebasis zu verbessern, sollten Sie eine Umgestaltung der ISafeGuard
Schnittstelle, um genau die zu entsprechen SafeGuard
Umsetzung.
Update: Im Commit behoben 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Fehlende Dokumentzeichenfolgen
Einige der Verträge und Funktionen in der Codebasis sind nicht dokumentiert. Zum Beispiel, einige Funktionen der SafeGuard
Vertrag.
Darüber hinaus verwenden einige Docstrings eine informelle Sprache, wie z über dem setTimelock
Funktion in der SafeGuard
Vertrag.
Dies behindert das Verständnis der Prüfer für die Absicht des Codes, was von grundlegender Bedeutung ist, um nicht nur die Sicherheit, sondern auch die Korrektheit richtig einzuschätzen. Darüber hinaus verbessern Docstrings die Lesbarkeit und erleichtern die Wartung. Sie sollten den Zweck oder die Absicht der Funktionen, die Szenarien, in denen sie fehlschlagen können, die Rollen, die sie aufrufen dürfen, die zurückgegebenen Werte und die ausgegebenen Ereignisse, explizit erläutern.
Erwägen Sie, alle Funktionen (und ihre Parameter), die Teil der öffentlichen API der Verträge sind, gründlich zu dokumentieren. Funktionen, die vertrauliche Funktionen implementieren, sollten ebenfalls klar dokumentiert werden, auch wenn sie nicht öffentlich sind. Beachten Sie beim Schreiben von Dokumentzeichenfolgen die Natürliches Spezifikationsformat von Ethereum (NatSpec).
Update: Teilweise fixiert in PR # 10. Korrekte Docstrings wurden verschiedenen Funktionen in der gesamten Codebasis hinzugefügt. Zusätzlich zu den aktuellen Änderungen sollten Sie jedoch die folgenden Änderungen vornehmen:
- Speichern
description
wie die@param
im obigen DokumentstringqueueTransactionWithDescription
Funktion - Speichern
@param
der Dokumentzeichenfolge über demcreateSafeGuard
Funktion inSafeGuardFactory
Vertrag - Speichern
@return
in docstrings über den Funktionen inSafeGuardFactory
Vertrag.
[L05] Nutzloser oder wiederholter Code
Es gibt Stellen in der Codebasis, an denen Code entweder wiederholt wird oder nicht benötigt wird. Einige Beispiele sind:
- Zeilen 29-32 dauert ebenfalls 3 Jahre. Das erste Jahr ist das sog.
Registry
Vertrag sind nutzlos, weil die_add
Funktion desEnumerableSet
Vertrag führt diese Prüfungen bereits durch gegen die bereits eingestellten Werte. - Zeilen 62, 67, 73 und 78 dauert ebenfalls 3 Jahre. Das erste Jahr ist das sog.
SafeGuard
Vertrag wiederholen alle genau die gleiche Operation. Erwägen Sie, es in eine interne Funktion zu kapseln, um doppelten Code zu vermeiden. - Zeilen 62-63 und 67-68 of
SafeGuard
werden wiederholt. Erwägen Sie, sie in einer einzigen internen Funktion zu kapseln. - Die Verwendung von
gasleft
um festzulegen, wie viel Gas im Aufruf der Funktion weitergeleitet werden sollexecuteTransaction
ist unnötig. Dies liegt daran, dass an diesem Punkt der Hinrichtung das gesamte verbleibende Gas verwendet wird, um die Hinrichtung fortzusetzen. Wenn dies nicht der Deutlichkeit dient, ziehen Sie in Betracht, die zu entfernengas
Parameter aus dem Aufruf.
Erwägen Sie, die vorgeschlagene Korrektur anzuwenden, um einen saubereren Code zu erstellen und die Konsistenz und Modularität der Codebasis zu verbessern.
Update: Fest eingebaut PR # 10 und begehen 7fd27df16fc879d990d36a167a0b6e719e578558
.
Hinweise und zusätzliche Informationen
[N01] Uneinheitlicher Stil
Es gibt einige Stellen in der Codebasis, an denen Stilunterschiede die Lesbarkeit beeinträchtigen und das Verständnis des Codes erschweren. Einige Beispiele sind:
- Das
Registry
Vertrag verwendet im gesamten Vertrag unterschiedliche Stile für Dokumentzeichenfolgen. - Das
SafeGuard
Vertrag gibt ein Ereignis aus, wennqueueTransactionWithDescription
wird aufgerufen, aber in anderen Funktionen, die sich mit Transaktionen befassen, werden keine Ereignisse ausgegeben. - Im
SafeGuard
Vertrag, manchmal Wert wird als benannter Parameter und manchmal verwendet _Wert wird eingesetzt.
Unter Berücksichtigung des Werts, den ein konsistenter Codierungsstil zur Lesbarkeit des Projekts hinzufügt, sollten Sie erwägen, einen Standardcodierungsstil mit Hilfe von Linter-Tools wie Solhint durchzusetzen.
Update: Fest eingebaut PR # 10 und begehen 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Fehlende Lizenz
Den folgenden Verträgen innerhalb der Codebasis fehlt eine SPDX-Lizenzkennung.
Um Compiler-Warnungen zum Schweigen zu bringen und die Konsistenz in der Codebasis zu erhöhen, sollten Sie eine Lizenzkennung hinzufügen. Denken Sie dabei daran, sich darauf zu beziehen spdx.dev Richtlinien.
Update: Fest eingebaut PR # 10 und begehen 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] Die Abhängigkeit von OpenZeppelin Contract ist nicht festgelegt
Um unerwartete Verhaltensweisen zu vermeiden, falls in zukünftigen Updates der Version Änderungen vorgenommen werden Bibliothek von OpenZeppelin ContractsZiehen Sie in Betracht, die Version dieser Abhängigkeit in der paket.json Datei.
Update: Fest eingebaut PR # 10.
[N04] Solidity-Compiler-Version ist nicht gepinnt
Ziehen Sie in Betracht, in der gesamten Codebasis die Version der Solidity-Compiler auf die neueste stabile Version. Dies sollte dazu beitragen, unerwartete Fehler aufgrund inkompatibler zukünftiger Versionen zu vermeiden. Um eine bestimmte Version auszuwählen, sollten Entwickler sowohl die für das Projekt erforderlichen Funktionen des Compilers als auch berücksichtigen die Liste der bekannten Fehler jeder Solidity-Compiler-Version zugeordnet.
Update: Fest eingebaut PR # 10.
[N05] Tippfehler
An verschiedenen Stellen in der gesamten Codebasis wird das Wort role
ist falsch geschrieben als rol
. Ein solches Beispiel ist in der docstring innerhalb der constructor
dauert ebenfalls 3 Jahre. Das erste Jahr ist das sog. SafeGuard
Vertrag.
Erwägen Sie, diese Tippfehler zu korrigieren, um die Lesbarkeit des Codes zu verbessern.
Update: Teilweise fixiert in PR # 10. Während die Schreibweise von role
wurde korrigiert, der Kommentar „Setze Admin-Rolle auf eine definierte Admin-Adresse“ sollte „Setze Admin-Rolle auf eine definierte Admin-Adresse“ lauten. Außerdem ist „execute“ in der falsch geschrieben SafeGuard
Vertrag auf Linie 69, Linie 82, Linie 96 und Linie 110 und "verfügbar" ist falsch geschrieben Linie 70, Linie 83, Linie 97, Linie 111. Erwägen Sie auch, informelle Wörter wie z "Werde" in SafeGuard
Vertrag mit formalen Alternativen wie „going to“.
[N06] Deklarieren Sie uint als uint256
Es gibt mehrere Vorkommen in der Codebasis, wo Variablen deklariert werden uint
Datentyp statt uint256
. Zum Beispiel die eta
Variable in der QueueTransactionWithDescription
Event dauert ebenfalls 3 Jahre. Das erste Jahr ist das sog. SafeGuard
Vertrag.
Um die Explizitheit zu fördern, alle Instanzen von uint
sollte deklariert werden als uint256
.
Update: Fest eingebaut PR # 10 und begehen 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Unbenutzter Import
Das SafeGuard
Vertrag importiert die console
Vertrag, nutzt ihn aber nie.
Um die Lesbarkeit des Codes zu verbessern, sollten Sie alle nicht verwendeten Importe entfernen.
Update: Fest eingebaut PR # 10.
Schlussfolgerungen
Es wurden eine hohe und mehrere andere kleinere Schwachstellen gefunden und Empfehlungen und Korrekturen vorgeschlagen.
- &
- Zugang
- Konto
- über
- Action
- Aktionen
- Zusätzliche
- Adresse
- Administrator
- Alle
- bereits
- Obwohl
- Bienen
- Ansatz
- um
- Prüfung
- Grundsätzlich gilt
- Sein
- Bugs
- Geschäft
- rufen Sie uns an!
- Fälle
- Verursachen
- Übernehmen
- berechnen
- Code
- Programmierung
- gemeinsam
- Compounds
- Verwirrung
- Berücksichtigung
- enthält
- fortsetzen
- Vertrag
- Verträge
- könnte
- Schöpfer
- Strom
- technische Daten
- Behandlung
- Design
- Entwickler
- anders
- entdeckt
- Erarbeiten
- ETH
- Event
- Veranstaltungen
- Beispiel
- Fabrik
- Eigenschaften
- Endlich
- Vorname
- Fixieren
- Flexibilität
- gefunden
- Funktion
- Mittel
- Zukunft
- GAS
- Unterstützung
- Kundenziele
- Governance
- Gouverneur
- Richtlinien
- mit
- Hilfe
- hier
- High
- Ultraschall
- HTTPS
- umgesetzt
- Erhöhung
- hat
- Schnittstelle
- IT
- bekannt
- Sprache
- neueste
- Bibliothek
- Lizenz
- Line
- Liste
- aus einer regionalen
- verschlossen
- sah
- Making
- Spiel
- modulare
- Multisig
- Andere
- Gegenwart
- Prozessdefinierung
- Projekt
- Angebot
- Öffentlichkeit
- veröffentlichen
- Registrieren
- Mitteilungen
- berichten
- Quelle
- Die Ergebnisse
- Überprüfen
- Sicherheitdienst
- kompensieren
- Einstellung
- von Locals geführtes
- smart
- Smart Contracts
- solide
- Bundesstaat
- Stil
- System
- Durch
- während
- Zeit
- Werkzeuge
- verfolgen sind
- Transaktionen
- Updates
- us
- Nutzer
- Wert
- Sicherheitslücken
- Börsen
- Woche
- WHO
- .
- Worte
- Schreiben