16. Dezember 2021
Einleitung
Das 1inch Team hat uns gebeten, ihre zu überprüfen und zu auditieren Limit-Order-Protokoll v2 intelligente Verträge. Wir haben uns den Code angeschaut und veröffentlichen nun unsere Ergebnisse.
Geltungsbereich
Wir haben Commitment geprüft 4d94eea25e4dac6271bfd703096a5c4a4d899b4a
dauert ebenfalls 3 Jahre. Das erste Jahr ist das sog. 1inch/limit-order-protocol
Repository. Im Umfang waren die folgenden Verträge:
- 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
Alle anderen Projektdateien und -verzeichnisse (einschließlich Tests) sowie externe Abhängigkeiten und Projekte, Spieltheorie und Incentive-Design wurden ebenfalls von dieser Prüfung ausgenommen. Es wurde davon ausgegangen, dass externe Code- und Vertragsabhängigkeiten wie dokumentiert funktionieren und dass von 1inch bereitgestellte Back-End-Dienste im besten Interesse des Protokolls handeln.
Allgemeine Gesundheit
Im Allgemeinen fanden wir die Codebasis des Projekts lesbar und gut organisiert, obwohl sie von einer umfassenderen Dokumentation profitieren könnte, insbesondere in Bezug auf die Blöcke des Assemblercodes, Randfälle des Protokolls, Assets/Prädikate/externe Ressourcen, die verwendet werden, Verantwortlichkeiten/Einschränkungen des bereitgestellten Back-End-Dienstes und Interaktionen zwischen den Akteuren. Das Projekt unternimmt große Anstrengungen, um Aktionen gaseffizient zu gestalten, gelegentlich sogar auf die Gefahr hin, dass der Code schwieriger zu begründen ist; Wir sprechen diesbezügliche Fragen weiter unten an. Während des gesamten Audits war das 1-Zoll-Team hochverfügbar, reaktionsschnell und sehr einfach zu handhaben.
Systemübersicht
Das Limit-Order-Protokoll ermöglicht die Bestellung makers
Off-Chain-Orders für Token-Swaps zu unterzeichnen. Das Protokoll erleichtert dann das Ausfüllen von zuvor unterzeichneten Aufträgen nach Auftrag takers
. Aufträge sind in hohem Maße erweiterbar und können externe Verträge an mehreren Stellen während des Auftragserfüllungsprozesses statisch aufrufen. Diese Erweiterbarkeit verleiht dem Protokoll Nützlichkeit, fügt jedoch sowohl Komplexität als auch eine größere Angriffsfläche für die Befehle selbst hinzu.
Es ist wichtig anzumerken, dass es keine On-Chain-Speicherung von Bestelldetails gibt. Der Füllstand oder Stornierungsstatus von Bestellungen wird nur über Bestell-Hashes verfolgt. Dies erfordert, dass Bestellungen Peer-to-Peer oder über eine zentralisierte Partei geteilt werden. In diesem Fall beabsichtigt das 1-Zoll-Team, als diese zentralisierte Partei zu fungieren, unterzeichnete Aufträge zu aggregieren und diese Aufträge als Liquiditätsquelle für ihre anderen Protokolle zu verwenden. Bestellungen werden über ihre eigene API veröffentlicht, damit Benutzer mit ihnen interagieren können.
Diese Zentralisierung gibt dem 1-Zoll-Team extreme Kontrolle darüber, welche Aufträge veröffentlicht und letztendlich ausgeführt werden. Dies gibt ihnen auch die Möglichkeit, Bestellungen zu zensieren, was bei böswilligen oder irreführenden Bestellungen nützlich sein könnte, aber auch missbraucht werden könnte und es ihnen ermöglicht, anderen Benutzern im Falle einer günstigen Bestellung zuvorzukommen, indem sie diese nicht über die API anzeigt.
Privilegierte Rollen
Obwohl die Verträge, in denen die Rolle verwendet wird, außerhalb des Geltungsbereichs lagen, wurde eine privilegierte Rolle identifiziert. Ein immutableOwner
wird zum Zeitpunkt der Konstruktion auf den Ersteller eines Proxy-Vertrags festgelegt und wird verwendet, um den Zugriff auf den Proxy-Vertrag zu beschränken external
Funktionen.
Externe Abhängigkeiten und Vertrauensannahmen
Das Design dieses Protokolls erfordert Off-Chain- und On-Chain-Komponenten, und dieses Hybridmodell kann verwendet werden, um einige Angriffsvektoren abzuschwächen, die wir in unserem Bericht identifizieren, aber die Kosten dieser Fähigkeit sind die erhöhte Abhängigkeit vom 1-Zoll-Team und der Infrastruktur.
Darüber hinaus bietet das Limit Order Protocol Funktionen, die dazu gedacht sind, Preise von Chainlink-Orakeln abzurufen. Wir gingen davon aus, dass diese Orakel ehrlich, zugänglich und ordnungsgemäß funktionieren.
Darüber hinaus gibt es aufgrund der Flexibilität eines Auftrags mehrere Berührungspunkte mit externen Verträgen, die nicht validiert werden. Dies bedeutet, dass ein böswilliger Benutzer solche Aufrufe missbrauchen und Prädikate, Vermögenswerte oder Orakel mit böswilligen Verträgen imitieren könnte, um Aktionen während der Auftragserfüllung auszuführen. Obwohl das Projekt in einigen Bereichen gegen Wiedereintritt geschützt ist, könnten solche Vektoren Denial-of-Service-Angriffe oder unentdeckte Spam-Bestellungen verursachen. Das 1-Zoll-Team ist sich bewusst, dass bestimmte Probleme auftreten können, wenn unbekannte Verträge für das Protokoll verwendet werden, und hat seine Absicht bekundet, dass nur große „Blue-Chip“-Assets vollständig vom Projekt unterstützt werden. Es sollte jedoch beachtet werden, dass es selbst bei den beliebtesten Assets intrinsische Verhaltensweisen von jedem Asset gibt, die Probleme bei Protokollen verursachen können, die sie nicht richtig ansprechen, wie z. B. eine Gebühr bei Überweisungen mit USDT oder die Rückgabe eines Fehlercodes anstelle von a Erfolg boolean mit cTokens.
Befund
Hier stellen wir unsere Erkenntnisse vor.
Kritische Schwere
Keiner.
Hoher Schweregrad
[H01] Es wurden inkonsistente Daten übergeben _makeCall
Im OrderMixin
Vertrag, die _makeCall
Funktion wird verwendet, um Vermögenswerte zu übertragen vom Nehmer zum Macher und dann vom Macher zum Abnehmer. Bei der letzteren Übertragung wird die _makeCall
Funktion wird die Bestellung falsch übergeben makerAsset
als letzten Parameter, wann es der Auftrag sein soll makerAssetData
.
Daher ist jede Proxy-Funktionalität, die auf der makerAssetData
Argument wird brechen.
In Übereinstimmung mit dem früheren Aufruf an _makeCall
und um die Proxy-Funktionalität vollständig zu unterstützen, sollten Sie die Aktualisierung der order.makerAsset
Parameter order.makerAssetData
.
Update: Fest eingebaut Pull-Anfrage # 57.
[H02] Teilweise ausgeführte Privataufträge können von jedermann ausgeführt werden
Das Protokoll ermöglicht die Erstellung privater und öffentlicher Aufträge. Bei Privatbestellungen nur die allowedSender
Adresse, die vom Hersteller bei der Erstellung der Bestellung angegeben wurde, die Bestellung ausführen kann.
In der OrderMixin
Vertrag, Bestätigung für die allowedSender
Adresse hat einen falschen Geltungsbereich, was bedeutet, dass er nur innerhalb der Logik ausgewertet wird, die die erste Ausführung eines Auftrags handhabt. Wenn eine private Bestellung teilweise ausgeführt wird, dann wird die Prüfung für die allowedSender
Adresse nicht mehr erreichbar ist und der Auftrag von jedermann ausfüllbar wird.
Um die Absicht zu klären, ob ein Benutzer in der Lage sein sollte, teilweise ausgefüllte private Bestellungen auszuführen oder nicht, ziehen Sie in Betracht, entweder den Grund für das aktuelle Verhalten zu dokumentieren oder die zu validieren allowedSender
Adresse außerhalb des Bereichs der ersten Füllung, um sicherzustellen, dass sie bei jedem Füllversuch validiert wird.
Update: Fest eingebaut Pull-Anfrage # 58.
[H03] Böswilliger Ersteller könnte Teilfüllungen ausnutzen, um das Vermögen des Abnehmers zu stehlen
Bestellungen aus der OrderMixin
Vertrag kann teilweise ausgefüllt werden. Um Teilfüllungen zu unterstützen, erfordert das Protokoll eine Möglichkeit, beide Seiten von Swaps zu berechnen. Beide getMakerAmount
und getTakerAmount
Felder werden vom Besteller genau für diesen Zweck definiert.
Beim Ausfüllen einer Bestellung müssen die Abnehmer entweder die angeben makingAmount
oder im takingAmount
Werte sowie a thresholdAmount
Wert. Es gibt zwei verschiedene Codepfade, die verwendet werden können, je nachdem, ob die makingAmount
oder im takingAmount
wurde bereitgestellt.
Der erste ist, wenn die makingAmount
Parameter definiert ist. Es könnte abschneiden makingAmount
Wert und auch berechne das takingAmount
Wert dafür. In dieser Situation ist die thresholdAmount
sorgt dafür, dass die takingAmount
Wert genommen ist nicht unerwartet groß.
Der zweite ist, wenn die takingAmount
Parameter definiert ist. In einem solchen Fall wird es berechne das makingAmount
Wert, mit der Möglichkeit von es abschneiden und Neuberechnung der takingAmount
Wert, wenn das passiert. In dieser Situation ist die thresholdAmount
Wert sorgt dafür, dass die makingAmount
zurückgegebener Wert ist nicht unerwartet klein.
Es gibt zwei Ausnutzungsmethoden, von denen jede für einen der zuvor erwähnten Codepfade einzigartig ist. Diese Ausbeutungsmethoden erfordern böswillige getMakerAmount
und getTakerAmount
Funktionen. Eine einfache Implementierung dieser Funktionen würde ein identisches Verhalten aufweisen AmountCalculator
's getMakerAmount
und getTakerAmount
Funktionen, aber mit einem fest codierten Schalter, der sie zwingt, bei Bedarf einen vom Angreifer kontrollierten Wert zurückzugeben.
Das erste, weniger schwerwiegende Exploit-Muster betrifft den ersten Codepfad, in dem die makingAmount
Wert angegeben ist in einer Füllreihenfolge. Ein böswilliger Hersteller würde auf einen Ausführungsauftrag warten, der dies spezifiziert makingAmount
um im Mempool aufzutauchen, um es voranzutreiben. Sie würden den gesamten Wert außer 1 von der Herstellerseite abziehen und dann erzwingen _callGetTakerAmount
um den im Konto des Benutzers angegebenen Betrag zurückzugeben thresholdAmount
Wert (oder deren Abzug, falls er geringer ist). Wenn die Transaktion des Benutzers schließlich abgeschlossen ist, tauscht er seine volle Summe aus thresholdAmount
im Wert von takerAsset
für eine einzelne Einheit von makerAsset
. Dieser Exploit ist durch die angegebene Menge begrenzt thresholdAmount
Wert oder die Höhe der takerAsset
der Benutzer erlaubt auf der LimitOrderProtocol
Vertrag.
Das zweite, schwerwiegendere Exploit-Muster betrifft den zweiten Codepfad, in dem die takingAmount
Wert angegeben ist. Der böswillige Hersteller würde in ähnlicher Weise auf einen Ausführungsauftrag warten, der a spezifiziert takingAmount
Wert, der im Mempool angezeigt wird. Sie würden die Transaktion vorantreiben und die Transaktion erzwingen makingAmount
Wert zurückgegeben von _callGetMakerAmount
Funktion höher als beide sein remainingMakerAmount
und für thresholdAmount
. Sie würden auch die einstellen takingAmount
Rückgabewert von _callGetTakerAmount
die Menge sein takerAsset
Vermögenswert auf dem erlaubt LimitOrderProtocol
vom Abnehmer. Wenn die Transaktion des Abnehmers durchgeht, wird sie es tun kürze die makingAmount
Wert und dann neu berechnen takingAmount
Wert. Es ist jedoch nicht garantiert, dass diese Neuberechnung niedriger ist, und in diesem Fall wird dem Abnehmer alles entzogen takerAsset
die sie im Vertrag zugelassen haben. In diesem Codepfad wird die thresholdAmount
Wert ist sicherstellen, dass die makingAmount
ist nicht zu niedrig, also alle Nehmer nehmen takerAsset
Vermögenswert ist deaktiviert. Die verlorenen Mittel sind durch die Höhe des Betrags begrenzt takerAsset
Asset, auf dem der Benutzer zugelassen ist LimitOrderProtocol
Vertrag.
Diese Exploits sind nicht möglich ohne Teilbestellungen und insbesondere Teilbestellungen mit böswilligen getMakerAmount
und getTakerAmount
Implementierungen.
Das Hauptproblem der thresholdAmount
Value Check ist, dass nur eine Seite des Swaps abgedeckt wird, die andere Seite aber über Frontrunning manipuliert werden kann. Es gibt keine Zusicherung, dass der ursprünglich vorgeschlagene Wert des Käufers unverändert bleibt. Erwägen Sie das Entfernen makingAmount
Abschneiden von beiden Codepfaden und Zurücksetzen, wenn die Reihenfolge eine so große Füllung wie angefordert nicht unterstützen kann. Dadurch wird die thresholdAmount
kann verwendet werden, um die andere Seite des Swaps ausreichend einzuschränken und unerwartetes Verhalten zu vermeiden, selbst bei böswilligen Bestellungen.
Update: Fest eingebaut Pull-Anfrage # 83.
Mittlere Schwere
[M01] Statische Argumente werden nach dynamischen Argumenten übergeben
Im OrderMixin
Vertrag, die getTakerAmount
und getMakerAmount
Bytes-Felder werden als Argumente für die verwendet _callGetTakerAmount
und _callGetMakerAmount
Funktionen. Diese Aufrufe bieten eine Möglichkeit, eine Seite des Swaps auf der Grundlage der anderen Seite zu berechnen, und sie ermöglichen Benutzern, Aufträge teilweise auszuführen.
Das getTakerAmount
/getMakerAmount
Felder sind dynamische Variablen und werden vor die gepackt takerAmount
und makerAmount
Werte in der _callGetTakerAmount
und _callGetMakerAmount
Funktionen. Es ist möglich, dass ein böswilliger Hersteller mehr Daten als erwartet in der bereitstellt getTakerAmount
undgetMakerAmount
Felder zu schieben takerAmount
und makerAmount
Bytes, die dort liegen, wo sie angenommen werden, wenn sie in der nächsten Funktion dekodiert werden. Dies ermöglicht dem Hersteller, die übergebene Taker- oder Maker-Menge um ein volles Byte nach rechts zu verschieben und sogar vollständig zu ersetzen, wenn zusätzliche 32 Byte an Daten bereitgestellt werden.
Benutzer müssen die bereits manuell überprüfen getTakerAmount
und getMakerAmount
Felder in der Reihenfolge, aber diese Technik ist ziemlich schwer zu erkennen. Bemerkenswert ist auch, dass dieser Angriff sogar auf intern vertrauenswürdige Personen angewendet wird getMakerAmount
und getTakerAmount
Funktionen. Bei den meisten Angriffen verhindert die Bereitstellung eines angemessenen Schwellenwerts den Verlust von Geldern.
Um dies zu verhindern, sollten Sie die statischen Argumente vor den dynamischen Argumenten codieren, um zu vermeiden, dass den dynamischen Argumenten eine Methode zur Steuerung der statischen Argumente gegeben wird.
Update: Nicht behoben. Das 1-Zoll-Team erklärte:
Wir werden besonders auf die Getter-Validierung achten. Wir werden versuchen, eine Plausibilitätsvalidierung von Gettern in unserem SDK zu implementieren, die beim Filtern potenziell bösartiger Bestellungen helfen wird.
[M02] ERC721-Aufträge können manipuliert werden
Es ist möglich, mehr als nur ERC20 über die auszutauschen OrderMixin
durch Bereitstellung eines Vertrags, der denselben Funktionsselektor wie der von IERC20 verwendet transferFrom
, und Bereitstellung dieses Vertrags als makerAsset
oder im takerAsset
in einer Bestellung.
Die Out-of-Scope-Proxys, nämlich ERC721Proxy
, ERC721ProxySafe
und ERC1155Proxy
Verträge folgen diesem Muster, um Unterstützung zu bieten ERC721
und ERC1155
Token. Da die Proxys mit dem gleichen Muster wie ein IERC20 aufgerufen werden müssen transferFrom
Aufruf, die Signatur muss mit beginnen address from
, address to
und uint256 amount
. Alles andere, was die Proxys benötigen, kann nachgereicht werden und wird in der Bestellung als definiert makerAssetData
und takerAssetData
.
ERC1155s können natürlich mehrere gleiche ID-Token auf einmal übertragen, was bedeutet, dass die ERC1155Proxy
Vertrag nutzt die amount
Feld. Andererseits, ERC721
s haben keine offensichtliche Verwendung für die amount
Feld. Da sie nicht fungible Token darstellen, existiert für eine bestimmte TokenId nur eine, wodurch die amount
Feld unbrauchbar. Aus diesem Grund ist die Implementierung für beide ERC721Proxy
und ERC721ProxySafe
Verträge verwenden die erforderlichen amount
Feld als die tokenId
stattdessen.
Diese Überlastung der amount
Parameter schafft die Möglichkeit der teilweisen Füllung ERC721
Bestellungen, um separat aufgeführte Token zu ermäßigten Preisen zu erwerben. Beispielsweise könnte es einen Fall geben, in dem ein einzelner Benutzer mehrere hat ERC721
s desselben Vertrages dürfen von übertragen werden ERC721Proxy
Vertrag und listet sie in separaten Limit-Orders auf.
Wenn die Limit-Orders auch die getMakerAmount
und getTakerAmount
Felder können teilweise ausgefüllt werden ERC721
Aufträge. Da ist die Bestellung amount
Feld entspricht eigentlich dem tokenId
, kann ein böswilliger Benutzer eine teilweise Füllung auf der platzieren ERC721
mit der höheren tokenId, was zu a makingAmount
/takingAmount
eines ERC721
das könnte einem niedrigeren entsprechen tokenId
. Das Ergebnis ist das ERC721
mit dem unteren tokenId
würde zum Preis von übertragen werden (higher tokenId price) * (lower tokenId's id) / (higher tokenId's id)
.
Dieser Exploit hat einige Anforderungen:
- Mehrere
ERC721
s aus demselben Vertrag, die auf beiden zulässig sindERC721
Proxy durch einen einzigen Eigentümer. - Offene Bestellung für einen der
ERC721
s das ist nicht das niedrigstetokenId
von den erlaubten. - Teilbefüllungen in der Bestellung erlaubt.
Um die Möglichkeit der teilweisen vollständig zu entfernen ERC721
füllt, ziehen Sie in Betracht, die zu trennen amount
und tokenId
Argumente. Unabhängig davon, ob die Argumente getrennt sind oder nicht, sollten Sie dies auch dokumentieren, um Benutzer auf dieses Verhalten aufmerksam zu machen und dieses Muster in Zukunft zu vermeiden.
Update: Fest eingebaut Pull-Anfrage # 59.
[M03] Undokumentierte Dezimalannahmen
Das LimitOrderProtocol
Vertrag erbt die ChainlinkCalculator
Vertrag durch die OrderMixin
Vertrag. Dieser Vertrag stellt zwei Funktionen zur Verfügung, um die Verwendung von Chainlink-Orakeln während der Prädikate prüfen und die Suche nach der Hersteller Menge/Abnehmerbetrag.
Der Vertrag macht jedoch undokumentierte Annahmen über die Anzahl der Dezimalstellen, die die Chainlink-Orakel melden sollten, sowie über die Anzahl der Dezimalstellen, die die Funktionsparameter enthalten sollten. In bestimmten Szenarien könnte dies zu unerwartetem Verhalten führen, einschließlich der Fehlbewertung von Vermögenswerten und dem unbeabsichtigten Verlust von Geldern.
Genauer gesagt ist im gesamten Vertrag die implizite Annahme, dass die Chainlink-Orakel mit einer Genauigkeit von 18 Dezimalstellen berichten werden. Allerdings nicht alle Chainlink-Orakel Bericht mit dieser Anzahl von Dezimalstellen. Wenn das Orakel ein Token-Paar meldet, das sich auf eine Währung (z. B. USD) bezieht, hat es nur eine Genauigkeit von 8 Dezimalstellen. Da gibt es keine Beschränkungen bzgl welche Orakel verwendet werden können, sollten keine impliziten Annahmen über die Anzahl der Dezimalstellen getroffen werden, mit denen sie berichten.
Dementsprechend gibt es eine implizite Annahme, dass die amount
Parameter für die ChainlinkCalculator
Funktionen verwenden 18 Dezimalstellen, zusammen mit der irreführenden expliziten Deklaration, dass die singlePrice
Funktion Calculates price of token relative to ETH scaled by 1e18
. In Wirklichkeit sogar mit einem Orakel die Bericht mit 18 Dezimalstellen, der Rückgabewert der singlePrice
Funktion würde durch die Anzahl der Dezimalstellen der skaliert werden amount
Parameter, der nicht unbedingt 18 Dezimalstellen sein muss.
Ähnlich, die doublePrice
Die Funktion geht davon aus, dass zwei Chainlink-Orakel mit der gleichen Anzahl von Dezimalstellen berichten, was dazu führt, dass das Ergebnis der Funktion von den Erwartungen abweicht.
Erwägen Sie, Annahmen bezüglich der Anzahl von Dezimalstellen, die Parameter und Rückgabewerte haben sollten, explizit zu dokumentieren. Ziehen Sie außerdem in Betracht, entweder Berechnungen einzuschränken, die von Orakeln abhängen, die diese Annahmen brechen, oder die relevanten Berechnungen die tatsächliche Anzahl von Dezimalstellen berücksichtigen zu lassen.
Update: Fest eingebaut Pull-Anfrage # 75.
Geringe Schwere
[L01] Nicht explizit deklarierte Konstanten
Es gibt ein paar Vorkommen von Literalwerten, die mit ungeklärter Bedeutung in der Codebasis verwendet werden. Zum Beispiel:
- Im
OrderMixin
Vertrag, die_remaining
Mapping ist semantisch überladen (wie in der Ausgabe erklärt Semantische Überladung des Mappings), um die verbleibende Asset-Menge für eine teilweise ausgeführte Bestellung zu verfolgen und auch der wenn eine Bestellung vollständig ausgeführt wurde. Speziell,0
bedeutet, dass keine mit einer Bestellung verbundenen Füllungen vorgenommen wurden,1
bedeutet, dass eine Bestellung nicht mehr ausgeführt werden kann, und alles, was größer als ist1
bedeutet, dass mit der Bestellung ein Restbetrag verbunden ist, der möglicherweise ausgeführt werden kann. - Im
ChainlinkCalculator
Vertrag, der wörtliche Wert1e18
wird im verwendetsinglePrice
Funktion.
Um die Lesbarkeit des Codes zu verbessern und das Refactoring zu erleichtern, sollten Sie erwägen, für jede magische Zahl eine Konstante zu definieren und ihr einen eindeutigen und selbsterklärenden Namen zu geben. Ziehen Sie bei komplexen Werten in Betracht, einen Inline-Kommentar hinzuzufügen, der erklärt, wie sie berechnet wurden oder warum sie ausgewählt wurden.
Update: Fest eingebaut Pull-Anfrage # 75 und Pull-Anfrage # 76.
[L02] Böswillige Parteien könnten die Ausführung zulässiger Befehle verhindern
Das OrderMixin
Der Vertrag ermöglicht es Herstellerbenutzern, sich einzureichen zulässige Befehle Diese können also in einer Transaktion ausgeführt werden, anstatt eine separate Transaktion für Genehmigungen zu benötigen. Auch Auftragsnehmer können eine eigene Genehmigung vorlegen während der Auftragserfüllung zum gleichen Zweck.
Denn die Herstellererlaubnis ist darin enthalten Auftrag, wären sowohl die Hersteller- als auch die Abnehmergenehmigung zugänglich, während sich die Order-Fill-Transaktion im Mempool befindet. Dies würde es jedem böswilligen Benutzer ermöglichen, diese Genehmigungen zu nehmen und sie für die entsprechenden Asset-Verträge auszuführen, während er die Fülltransaktion vorantreibt. Denn diese Genehmigungen haben a nonce
Um einen doppelten Ausgabenangriff zu verhindern, würde die Ausführungstransaktion der Bestellung fehlschlagen, da versucht wird, dieselbe Genehmigung zu verwenden, die gerade während des Frontruns verwendet wurde.
Obwohl kein Sicherheitsrisiko besteht und der Hersteller eine neue Bestellung erstellen und die Transaktion vorab genehmigen könnte, könnte dieser Angriff sicherlich die Verwendbarkeit zulässiger Bestellungen beeinträchtigen. Tatsächlich könnte ein motivierter Angreifer blocken alle zulässige Befehle mit diesem Angriff. Überprüfen Sie, ob die Genehmigung bereits eingereicht wurde oder ob die Genehmigung ausreicht, während die Bestellung ausgeführt wird. Erwägen Sie auch, die Benutzer während der Auftragserstellung über diesen möglichen Angriff zu informieren.
Update: Nicht behoben. Das 1-Zoll-Team erklärt:
Wir hatten zuvor Genehmigungsprüfungen, entschieden uns aber, den Genehmigungsfluss zu vereinfachen, um nur auf erfolglose Genehmigungen zurückzugreifen. Wir werden darüber nachdenken, wie wir Hersteller über das Problem informieren können.
[L03] Doppelter Code
Es gibt Fälle von dupliziertem Code innerhalb der Codebasis. Das Duplizieren von Code kann später im Entwicklungslebenszyklus zu Problemen führen und macht das Projekt anfälliger für die Einführung von Fehlern. Solche Fehler können versehentlich eingeführt werden, wenn Funktionsänderungen nicht über alle Codeinstanzen repliziert werden, die identisch sein sollten. Beispiele für duplizierten Code sind:
Anstatt Code zu duplizieren, sollten Sie erwägen, nur einen Vertrag oder eine Bibliothek zu haben, die den duplizierten Code enthält, und ihn zu verwenden, wann immer die duplizierte Funktionalität erforderlich ist.
Update: Teilweise fixiert in Pull-Anfrage # 60.
[L04] Fehlerhafte oder irreführende Testsuite
Es gibt Fälle in der Testsuite, in denen die Tests von ihrem erwarteten Verhalten abweichen. Zum Beispiel:
- Das
ChainlinkCalculator
Vertrag wird von der geerbtOrderMixin
Vertrag. Doch während der Tests dieAmountCalculator.arbitraryStaticCall
Funktion wird verwendet, um die aufzurufenChainlinkCalculator
Vertrag als externer, unabhängiger Vertrag. Auch wenn das Ergebnis das erwartete ist, sollte der Test das Verhalten mit dem aktuellen Design des Systems und dem erwarteten Anwendungsfall durch den Aufruf widerspiegelnChainlinkCalculator
funktioniert direkt ohne den willkürlichen statischen Aufruf. - Obwohl die Proxy-Verträge außerhalb des Geltungsbereichs lagen, stellten wir fest, dass beim Testen des Protokolls mit ERC721-Assets die
ERC721Proxy
Vertrag wird nicht verwendet, um die Vermögenswerte in seinem zu tauschen Testsuite.
Da die Testsuite selbst nicht Gegenstand dieses Audits ist, sollten Sie die Testsuite gründlich überprüfen, um sicherzustellen, dass alle Tests gemäß den Spezifikationen des Protokolls erfolgreich ausgeführt werden.
Update: Fest eingebaut Pull-Anfrage # 57, Pull-Anfrage # 59 und Pull-Anfrage # 61.
[L05] Fehler und Auslassungen in Veranstaltungen
In der gesamten Codebasis werden im Allgemeinen Ereignisse ausgegeben, wenn vertrauliche Änderungen an den Verträgen vorgenommen werden. Vielen Ereignissen fehlen jedoch indizierte Parameter und/oder es fehlen wichtige Parameter. Zum Beispiel:
Es gibt auch sensible Aktionen, denen Ereignisse fehlen, wie zum Beispiel:
Erwägen Sie, vorhandene Ereignisse vollständiger zu indizieren und neue Parameter hinzuzufügen, wo sie fehlen. Erwägen Sie auch, alle Ereignisse so vollständig auszugeben, dass sie verwendet werden können, um den Vertragsstatus durch Off-Chain-Dienste wiederherzustellen.
Update: Nicht behoben. Das 1-Zoll-Team hat jedoch eine hinzugefügt orderRemaining
Parameter auf die OrderCanceled
Veranstaltung in Pull-Anfrage # 62.
Das 1-Zoll-Team erklärt:
Wir haben festgestellt, dass nur eine begrenzte Teilmenge von Daten erforderlich ist, um die Frontend-Anforderungen zu erfüllen. Bei umfangreichen Analysen stehen alle vorgeschlagenen Felder per Tracing zur Verfügung. Für
OrderRFQMixin
Wir erwarten von Market Makern, dass sie ihre eigene ausgefeilte Methode entwickeln, um zu verfolgen, welche Orders storniert wurden.
[L06] Speicheränderungen während der Ereignisausgabe
Im NonceManager
Vertrag, wenn die NonceIncreased
Ereignis wird ausgegeben, die Nonce des Nachrichtensenders wird ebenfalls erhöht.
Das gleichzeitige Ausführen mehrerer Operationen kann die Codebasis schwerer nachvollziehbar und fehleranfälliger machen und dazu führen, dass Operationen übersehen oder missverstanden werden.
Um die allgemeine Intentionalität, Lesbarkeit und Klarheit des Codes zu verbessern, sollten Sie den Nonce-Wert erhöhen, bevor Sie das Ereignis ausgeben.
Update: Fest eingebaut Pull-Anfrage # 63.
[L07] Inkonsistente Dekodierungsmethoden können zu Diskrepanzen bei den Ergebnissen führen
Um all seine Erweiterbarkeit und Flexibilität zu unterstützen, muss das Limit Order Protocol routinemäßig mit dynamischen Bytedaten und willkürlichen Rückgabewerten von externen Verträgen umgehen. Als Ergebnis enthält das Protokoll eine ArgumentsDecoder
Bibliothek, um dynamische Bytewerte effizienter in grundlegende Datentypen umzuwandeln. Diese Bibliothek wird jedoch nicht ausschließlich und in einigen Fällen verwendet abi.decode
wird stattdessen verwendet. Darüber hinaus werden einige Verträge verwendet abi coder v1
während andere es verwenden abi coder v2
. Ersteres verhält sich ähnlicher wie das ArgumentsDecoder
Bibliothek, während letztere beim Dekodieren zusätzliche Prüfungen durchführt.
Die inkonsistente Verwendung dieser unterschiedlichen Decodierungsmethoden kann zu subtilen Diskrepanzen zwischen der Absicht und dem tatsächlichen Verhalten der Codebasis führen.
Zum Beispiel kann die simulateCalls
Funktion verwendet nur die ArgumentsDecoder.decodeBool
Funktion. Wenn die simulateCalls
-Funktion verwendet wird, um Aufrufe zu überprüfen, die im Prädikatteil einer Bestellung erfolgen würden, dann könnten ihre Ergebnisse von dem abweichen, was tatsächlich auftritt, wenn die Prädikatbedingungen ausgewertet werden, da unterschiedliche Decodierungsmethoden verwendet werden.
Also zum Beispiel, wenn ein Prädikat ein Äußeres macht staticcall
zu einer Funktion, die a zurückgibt uint256
Wert größer als eins statt erwartet bool
, dann wird dieser Aufruf zurückgesetzt, da der Rückgabewert ist dekodiert mit abi coder v2
's abi.decode
die solche Werte wie nicht akzeptieren bool
. Allerdings, wenn genau der gleiche Anruf mit getätigt wird simulateCalls
, dann es wird einfach als markiert true
Da decodeBool
behandelt jeden Wert größer als Null als true
.
Um das zu machen simulateCalls
Funktion das Verhalten tatsächlicher Prädikataufrufe vollständig widerspiegelt, ziehen Sie in Betracht, sie für die Verwendung zu ändern abi.decode
.
Update: Fest eingebaut Pull-Anfrage # 82.
[L08] Fehlende Eingabevalidierung
Das fillOrderToWithPermit
und fillOrderTo
Funktionen der OrderMixin
Vertrag, sowie die fillOrderRFQToWithPermit
und fillOrderRFQTo
Funktionen der OrderRFQMixin
Vertrag, nicht validieren target
Adressparameter.
Dies ermöglicht es einem Benutzer, versehentlich die Nulladresse einzugeben und infolgedessen die Vermögenswerte zu sperren, die er nach dem Ausfüllen einer Bestellung erhalten soll.
Um sicherzustellen, dass Benutzer ihre Gelder nicht versehentlich sperren, sollten Sie prüfen, ob die target
Adresse ist in den genannten Funktionen nicht gleich der Nulladresse.
Update: Fest eingebaut Pull-Anfrage # 78.
[L09] Niedrige Einheitentestabdeckung
Die Unit-Test-Abdeckung für das gesamte Projekt liegt bei etwa 75 %, wobei einige der Verträge eine besonders geringe Abdeckung aufweisen.
In Anbetracht der Bedeutung von Unit-Tests zur Validierung von Code und zur Vermeidung von Regressionen beim Refactoring und der Entwicklung neuer Funktionen empfehlen wir, die Unit-Test-Abdeckung deutlich auf mindestens 95 % zu erhöhen und Grenzfälle einzubeziehen, die auch unwahrscheinliche Situationen abdecken.
Update: Nicht behoben.
[L10] Irreführende oder unvollständige Inline-Dokumentation
In der gesamten Codebasis wurden einige Fälle von irreführender und/oder unvollständiger Inline-Dokumentation identifiziert, die behoben werden sollten.
Im Folgenden finden Sie Beispiele für irreführende Inline-Dokumentation:
- Im
ChainlinkCalculator
Vertrag, diesinglePrice
Funktion NatSpez@notice
Etikett sagt, dass esCalculates price of token relative to ETH scaled by 1e18
, aber tatsächlich ist das Ergebnis die Wert ofamount
Token skaliert um1e18
, wobei das Orakel möglicherweise nicht in Bezug auf ETH meldet (z. B. für ein Paar, das ETH nicht enthält). - Im
OrderRFQMixin
Vertrag, dieinvalidatorForOrderRFQ
Funktion NatSpez@return
Etikett ist irreführend, da das Anführungszeichen möglicherweise nicht ausgefüllt wurde, damit das jeweilige Invalidator-Bit gesetzt wurde. Die Bestellung kann auch storniert worden sein. - Auf Linien 147, 165 und 188 of
OrderMixin.sol
, die NatSpec@param
Tags sind ungrammatisch. - Online 20 of
ERC1155Proxy.sol
, der@notice
-Tag gibt an, dass der berechnete Hash das Ergebnis des Hashings von istfunc_733NCGU
Funktion, wo es sein solltefunc_301JL5R
Funktion stattdessen.
Im Folgenden finden Sie Beispiele für unvollständige Inline-Dokumentation:
- Funktionen im
AmountCalculator
Vertrag beschreiben keinen der Parameter. - Im
ChainlinkCalculator
Vertrag, diesinglePrice
unddoublePrice
Funktionen beschreiben nicht alle Parameter. - Im
ImmutableOwner
Vertrag haben die öffentliche Variable und der Modifikator keine NatSpec. - Im
InteractiveNotificationReceiver
Vertrag, dienotifyFillOrder
Die Funktion beschreibt keinen der Parameter. - Im
LimitOrderProtocol
Vertrag, dieDOMAIN_SEPARATOR
Funktion hat keine NatSpec. - Ereignisse und Zuordnungen in der
NonceManager
haben keine NatSpec. - Im
OrderRFQMixin
Vertrag,cancelOrderRFQ*
Funktionen beschreiben nicht die Rückgabewerte. - Im
OrderMixin
Vertrag, einige Funktionen fehlen vollständig NatSpec. - Online 168 of
OrderMixin.sol
und online 71 ofOrderRFQMixin.sol
, es fehlt die@dev
-Tag. - Funktionen im
PredicateHelper
Vertrag nicht alle Parameter beschreiben.
Eine klare Inline-Dokumentation ist von grundlegender Bedeutung, um die Absichten des Codes zu skizzieren. Diskrepanzen zwischen der Inline-Dokumentation und der Implementierung können zu schwerwiegenden Missverständnissen darüber führen, wie sich das System verhalten soll. Erwägen Sie, diese Fehler zu beheben, um Verwirrung für Entwickler, Benutzer und Prüfer gleichermaßen zu vermeiden.
Update: Teilweise behoben. Irreführende Dokumentation adressiert in Pull-Anfrage # 75 und Pull-Anfrage # 77.
Das 1-Zoll-Team erklärt:
Wir haben irreführende Dokumente korrigiert. Die Vervollständigung der Unterlagen erfolgt später.
[L11] DoS-Befehle bei Verwendung von Hooks möglich
Das OrderMixin
Vertrag implementiert Funktionen zum Erfüllen generischer Off-Chain-Swap-Aufträge, die Bedingungen für ihren Erfolg haben könnten. Während die Bestellung ausgeführt wird, kann die Bestellung Überprüfen Sie die vordefinierten „Prädikat“-Bedingungen bevor Sie mit der Ausführung fortfahren.
Da diese Prädikatenbedingungen jedoch auf die Logik eines beliebigen Vertrags abzielen könnten, könnte ein böswilliger Hersteller die Abnehmer glauben machen, dass sich eine Bestellung korrekt verhält und gültig ist, wenn sie außerhalb der Kette geprüft wird, aber dann scheitert, wenn sie versucht, dieselbe Bestellung auszuführen in der Kette. Diese Änderung des Verhaltens der Prädikate könnte entweder durch Vorauslaufen eines variablen Zustands, von dem die Prädikate abhängen, durch Untersuchen des gesendeten Gases oder sogar der an dem Anruf beteiligten Adressen, oder durch eine andere Logik erfolgen.
Wenn der Hersteller außerdem a Interaktion während des Austauschs, der interactionTarget
Der Vertrag könnte sich selbst zurücksetzen oder die Genehmigung widerrufen, um eine erfolgreiche Auftragserfüllung zu verhindern, was im Wesentlichen zu demselben Ergebnis führt wie böswillige Prädikate.
Obwohl Vermögenswerte nicht gefährdet sind, haben Benutzer oder Bots, die eine günstige Bestellung finden, die erhöhte Belastung, diese Art von Spam-Bestellungen zu identifizieren, die oberflächlich legitim erscheinen können. Falls sie diese Art von Aufträgen nicht identifizieren, entstehen ihnen Kosten für verschwendetes Gas. Um die Menge an Spam-Bestellungen zu reduzieren, sollten Sie die verfügbaren Ziele für diese Hooks einschränken. Erwägen Sie auch, Benutzer vor dieser Möglichkeit zu warnen, bevor sie versuchen, Bestellungen auszuführen.
Update: Nicht behoben. Das 1-Zoll-Team erklärt:
Wir handhaben das in unserem Back-End und überlegen, wie wir mögliche Interessenten über das Problem informieren können.
[L12] Rundung kann ungünstig sein für taker
Im OrderMixin
und OrderRFQMixin
Verträge, wenn ein Auftrag ausgeführt wird und der Abnehmer nur a makingAmount
or takingAmount
Betrag, versucht das Protokoll, den Gegenbetrag des Swaps zu berechnen.
Bei diesen Berechnungen gibt es zwei Probleme. Das erste ist, dass es keine Dokumentation oder Logik gibt, die die Anzahl der Dezimalstellen begrenzt, die die Betragsparameter verwenden sollten, was wir in the angesprochen haben Undokumentierte Dezimalannahmen Problem.
Das zweite Problem ist, dass sich das Protokoll im Laufe dieser Berechnungen zugunsten des Herstellers dreht. Das Rundungsproblem kann stark verschärft werden, wenn die impliziten Dezimalannahmen nicht eingehalten werden, aber selbst wenn alles den Erwartungen entspricht, wird bei kleinen, ungeraden Beträgen gerundet.
Erwägen Sie, dem Abnehmer zu erlauben, einen Mindestbetrag anzugeben makerAsset
Vermögenswert, den sie zu erhalten bereit sind, zusammen mit einem Höchstbetrag von takerAsset
Vermögenswerte, die sie zu tauschen bereit sind, so dass die Akzeptanz einer Rundung eindeutiger ist.
Update: Nicht behoben. Das 1-Zoll-Team erklärt:
Der Schwellenwert sollte für den Schutz des Abnehmers ausreichen.
[L13] Widersprüchliche Auftragsabwicklung bei fehlenden Parametern
Im OrderMixin
Vertrag, die fillOrderTo
Funktion macht interne Aufrufe an die _callGetMakerAmount
und _callGetTakerAmount
funktioniert, wenn eine Füllung versucht wird und entweder die makingAmount
oder im takingAmount
Parameter jeweils null sind, oder wenn die makingAmount
Wert ist größer als die remainingMakerAmount
Wert.
Das _callGetMakerAmount
und _callGetTakerAmount
Anrufe führen zu Stornierungen, wenn die Bestellung nicht mit erstellt wurde getMakerAmount
or getTakerAmount
Parameter, und es wird eine Teilfüllung ausgeführt.
An Inline-Kommentar daneben _callGetMakerAmount
und ein Inline-Kommentar daneben _callGetTakerAmount
behaupten, dass „nur ganze Füllungen erlaubt sind“, wenn die Bestellung nicht mit erstellt wurde getMakerAmount
or getTakerAmount
Parameter.
Es gibt jedoch Codepfade, für die dies nicht gilt, da diese Pfade das nicht überprüfen length
s von beiden getMakerAmount
und getTakerAmount
Parameter.
Insbesondere Wenn ein taker
spezifiziert a takerAmount
Wert für eine Bestellung, die nur a hat getMakerAmount
, es sei denn, dieser Anruf getMakerAmount
gibt einen Betrag zurück, der größer als ist remainingMakerAmount
, kann entgegen der Inline-Dokumentation eine teilweise Füllung ausgeführt werden.
Dadurch bleibt die Absicht dieser Codepfade unklar. Wenn dies das erwartete Verhalten ist, ziehen Sie in Betracht, die Inline-Dokumentation so zu ändern, dass sie expliziter ist. Wenn dies ein unbeabsichtigtes Verhalten ist, sollten Sie immer die Längen beider überprüfen getMakerAmount
und für getTakerAmount
Parameter gleichzeitig, sodass die Implementierung das in der Inline-Dokumentation beschriebene Verhalten verstärkt.
Update: Fest eingebaut Pull-Anfrage # 79.
[L14] Verwendung veralteter Chainlink-Aufrufe
Das ChainlinkCalculator
Vertrag soll verwendet werden, um Chainlink-Orakel abzufragen. Dies geschieht durch Anrufe bei ihnen latestTimestamp
und latestAnswer
Methoden, beide sind veraltet. Tatsächlich sind die Methoden in der API von Chainlink-Aggregatoren nicht mehr vorhanden ab Version drei.
Um potenzielle zukünftige Inkompatibilitäten mit Chainlink-Orakeln zu vermeiden, sollten Sie die Verwendung von erwägen latestRoundData
Methode statt.
Update: Fest eingebaut Pull-Anfrage # 67.
Hinweise und zusätzliche Informationen
[N01] Schnittstellen werden nicht importiert
Das AggregatorInterface
Die Schnittstelle scheint eine Teilmenge des kopierten Codes zu sein ChainLink
das öffentliche Code-Repository von. Die vollständige Schnittstelle ist in enthalten ChainLink
's Vertrag npm Paket.
Um das Potenzial für Schnittstellenkonflikte und daraus resultierende Probleme zu verringern, sollten Sie nach Möglichkeit die Verwendung von Schnittstellen in Betracht ziehen, die über ihre offiziellen npm-Pakete installiert wurden, anstatt die Schnittstellen eines anderen Projekts neu zu definieren und/oder neu zu schreiben.
Update: Fest eingebaut Pull-Anfrage # 66.
[N02] Veraltete Projektabhängigkeiten
Während der Installation des Projektabhängigkeiten, NPM warnt, dass eines der installierten Pakete, Highlight
, „wird in Zukunft nicht mehr unterstützt oder erhält keine Sicherheitsupdates“.
Auch wenn es unwahrscheinlich ist, dass dieses Paket ein Sicherheitsrisiko darstellen könnte, ziehen Sie in Betracht, die Abhängigkeit, die dieses Paket verwendet, auf eine gewartete Version zu aktualisieren.
Update: Fest eingebaut Pull-Anfrage # 64.
[N03] Externe Aufrufe von Ansichtsmethoden sind keine statischen Aufrufe
Über den größten Teil der Codebasis führt das Protokoll explizit externe Aufrufe über OpenZeppelin durch functionStaticCall
Methode, um die Möglichkeit für Zustandsänderungen einzuschränken, wenn sie entweder nicht erwartet oder nicht erwünscht sind. Allerdings im ChainlinkCalculator
Vertrag trotz der Absicht, nur externe Gespräche zu führen view
Methoden auf Chainlink-Orakel, die externen Aufrufe in der singlePrice
und doublePrice
Funktionen werden nicht explizit ausgeführt staticcall
s.
Wir haben zwar keine unmittelbaren Sicherheitsbedenken festgestellt, die sich daraus ergeben, aber um die Angriffsfläche zu reduzieren, die Konsistenz zu verbessern und die Absicht zu verdeutlichen, sollten Sie die Verwendung von „explicit“ in Betracht ziehen staticcall
s, für alle externen Anrufe an view
Funktionen in der ChainlinkCalculator
Vertrag.
Update: Nicht behoben. Das 1-Zoll-Team erklärt:
Wir glauben, dass Syntaxkomplikationen Verbesserungen der Konsistenz zunichte machen.
[N04] Mit ungültigen Orders nicht vorzeitig scheitern
Im OrderMixin
Vertrag, die fillOrderTo
Funktion behandelt die besondere Bedingung, wenn eine Bestellung noch nicht übermittelt wurde (remainingMakerAmount == 0
), behandelt aber nicht explizit die Bedingung, wenn die Bestellung nicht mehr gültig ist (remainingMakerAmount == 1
).
Im letzteren Szenario kehrt die Funktion schließlich zurück, aber erst nach dem Verbrennen nicht trivialer Gasmengen. Um die Absicht zu verdeutlichen, die Lesbarkeit zu verbessern und den Gasverbrauch zu reduzieren, sollten Sie das Szenario der ungültigen Reihenfolge explizit zu Beginn der Funktion behandeln.
Update: Fest eingebaut Pull-Anfrage # 68.
[N05] Helferverträge nicht als abstrakt gekennzeichnet
In Solidität das Schlüsselwort abstract
wird für Verträge verwendet, die entweder selbst keine funktionalen Verträge sind oder nicht als solche verwendet werden sollen. Stattdessen, abstract
Verträge werden von anderen Verträgen im System vererbt, um eigenständige funktionale Verträge zu erstellen.
In der gesamten Codebasis gibt es Beispiele für Hilfsverträge, die nicht als abstrakt gekennzeichnet sind, obwohl sie nicht für den eigenständigen Einsatz gedacht sind. Zum Beispiel die AmountCalculator
, ChainlinkCalculator
, ImmutableOwner
, NonceManager
und PredicateHelper
Verträge bestehen alle aus einem Basissatz von Funktionen, die dazu bestimmt sind, von Erbverträgen verwendet zu werden.
Erwägen Sie, Helferverträge als zu markieren abstract
um deutlich zu machen, dass sie ausschließlich dazu bestimmt sind, Verträgen, die sie erben, Funktionen hinzuzufügen.
Update: Nicht behoben. Das 1-Zoll-Team erklärt:
Diese Helfer können separat eingesetzt werden. Sie werden nur für Gaseinsparungen vererbt.
[N06] Inkonsistente Funktionsreihenfolge
In der gesamten Codebasis folgt die Funktionsreihenfolge im Allgemeinen der empfohlene Reihenfolge im Solidity Style Guide, welches ist: constructor
, fallback
, external
, public
, internal
, private
.
Allerdings in der OrderMixin
Vertrag, die public
checkPredicate
Funktion weicht vom Styleguide ab und halbiert die external
Funktionen.
Um die allgemeine Lesbarkeit des Projekts zu verbessern, ziehen Sie in Erwägung, die Reihenfolge der Funktionen in der gesamten Codebasis zu standardisieren, wie im Solidity Style Guide empfohlen.
Update: Fest eingebaut Pull-Anfrage # 69.
[N07] Inkonsistenter Order Fill Flow
Das OrderMixin
und RFQOrderMixin
Verträge handhaben beide die Ausführung unterzeichneter Aufträge, aber der allgemeine Auftragsablauf zwischen den beiden Verträgen ist uneinheitlich.
OrderMixin
's fillOrderTo
Funktion folgt diesem allgemeinen Ablauf (Pseudo-Code):
if ((takingAmount == 0) == (makingAmount == 0))
else if (takingAmount == 0)
else (handle makingAmount == 0) THEN swapTokens
Während RFQOrderMixin
ist analog fillOrderRFQTo
Funktion folgt diesem Ablauf (Pseudo-Code):
if (takingAmount == 0 && makingAmount == 0)
else if (takingAmount == 0)
else if (makingAmount == 0)
else revert THEN swapTokens
Es gibt keine Erkenntnisse aus der Dokumentation darüber, warum sich die erste Bedingung in jeder dieser beiden Funktionen unterscheidet oder warum takingAmount
und makingAmount
in der letzteren Funktion können nicht beide Null sein. Auch der Fall, wo sowohl a makingAmount
und einem takingAmount
bereitgestellt werden, ist viel einfacher zu argumentieren in der fillOrderRFQTo
Funktion, da es im Finale klar gehandhabt wird else
blockieren.
Um die Absicht zu verdeutlichen und die allgemeine Lesbarkeit des Codes zu verbessern, sollten Sie entweder den allgemeinen Auftragsfluss über diese beiden Verträge hinweg standardisieren oder explizit dokumentieren, warum die Unterschiede bestehen.
Update: Nicht behoben. Das 1-Zoll-Team erklärt:
Dies liegt an benutzerdefinierten Preisfunktionen in Limit-Orders. Seit
getMakerAmount
möglicherweise erheblich davon abweichen könnengetTakerAmount
, dachten wir, dass es besser ist, keine Standardoption für den Taker festzulegen, da dies ihn wahrscheinlich verwirren wird, wenn diese Getter unterschiedlich sind.
[N08] Fehlermeldungen sind uneinheitlich formatiert oder irreführend
In der gesamten Codebasis ist die require
und revert
Es wurde festgestellt, dass Fehlermeldungen, die Benutzer über die besonderen Bedingungen informieren sollen, die zum Fehlschlagen einer Transaktion führen, inkonsistent formatiert sind.
Zum Beispiel jede der Fehlermeldungen auf Zeilen 85 von OrderMixin.sol
, 16 von ERC721ProxySafe.sol
und 26 von Permitable.sol
einen anderen Stil anwenden.
Außerdem sind einige Fehlermeldungen irreführend:
Fehlermeldungen sollen Benutzer über fehlerhafte Bedingungen informieren, daher sollten sie genügend Informationen bereitstellen, damit entsprechende Korrekturen vorgenommen werden können, um mit dem System zu interagieren. Uninformative Fehlermeldungen beeinträchtigen die allgemeine Benutzererfahrung erheblich und verringern somit die Qualität des Systems. Darüber hinaus können inkonsistent formatierte Fehlermeldungen unnötige Verwirrung stiften. Erwägen Sie daher, die gesamte Codebasis zu überprüfen, um sicherzustellen, dass alle require
und revert
-Anweisung hat eine Fehlermeldung, die konsistent formatiert, genau, informativ und benutzerfreundlich ist.
Update: Teilweise fixiert in Pull-Anfrage # 81.
[N09] Inkonsistente Verwendung benannter Rückgabevariablen
Es gibt eine inkonsistente Verwendung von benannten Rückgabevariablen in der OrderMixin
Vertrag. Einige Funktionen benannte Variablen zurückgeben, Andere explizite Werte zurückgeben, und andere Deklarieren Sie eine benannte Rückgabevariable, überschreiben Sie sie jedoch mit einer expliziten return-Anweisung.
Erwägen Sie einen konsistenten Ansatz zur Rückgabe von Werten in der gesamten Codebasis, indem Sie alle benannten Rückgabevariablen entfernen, sie explizit als lokale Variablen deklarieren und gegebenenfalls die erforderlichen Rückgabeanweisungen hinzufügen. Dies würde sowohl die Explizitheit als auch die Lesbarkeit des Codes verbessern und kann auch dazu beitragen, Regressionen bei zukünftigen Code-Refaktorisierungen zu reduzieren.
Update: Fest eingebaut Pull-Anfrage # 73.
[N10] Die Hash-Berechnung der Bestellung ist für die API nicht offen
Das external
Funktionen remaining
, remainingRaw
und remainingsRaw
alle erwarten einen Bestell-Hash für einen erfolgreichen Betrieb.
Allerdings die Helferfunktion _hash
, die den Hash einer Bestellung zurückgibt, hat private
Sichtweite. Dies bedeutet, dass Benutzer Teile der Bestellungen und Domain-Strings manuell packen müssen, um den Hash einer Bestellung zu erhalten.
Um mögliche Fehler bei der Berechnung von Bestell-Hashes zu vermeiden und Benutzern eine Methode zum Generieren des jeweiligen Hashs einer Bestellung bereitzustellen, sollten Sie die Sichtbarkeit von erweitern _hash
Funktion zu public
und Umgestaltung des Namens zu hash
um mit dem Rest des Codes konsistent zu sein.
Update: Fest eingebaut Pull-Anfrage # 74.
[N11] Semantische Überladung des Mappings
Das _remaining
Kartierung in der OrderMixin
Vertrag ist semantisch überladen, um den Status von Bestellungen und die verbleibende Menge an Vermögenswerten zu verfolgen, die für diese Bestellungen verfügbar sind.
Die drei Zustände, die es annehmen kann, sind:
0
: Der Bestell-Hash wurde noch nicht gesehen.1
: Die Bestellung wurde entweder storniert oder vollständig ausgeführt.- Jedes
uint
größer als1
: Der RestmakerAmount
verfügbar, um auf der Bestellung plus ausgefüllt zu werden1
.
Diese semantische Überladung erfordert das Ein- und Auspacken dieses Werts während lookup
, cancellation
, initialization
und storage
.
Semantisches Überladen und die notwendige Logik, um es zu ermöglichen, können fehleranfällig sein und die Codebasis schwerer verständlich und vernünftig machen, es kann auch die Tür für Regressionen in zukünftigen Aktualisierungen des Codes öffnen.
Um die Lesbarkeit des Codes zu verbessern, sollten Sie den Fertigstellungsstatus von Bestellungen in einem separaten Mapping verfolgen.
Update: Nicht behoben. Das 1-Zoll-Team gab an, dass eine Lösung die Gaskosten für den erhöhen würde fillOrder
Funktion.
[N12] Aufträge mit Erlaubnis erlauben Aufrufe zu beliebigen Verträgen
Das OrderMixin
Vertrag erbt die Permitable
Vertrag, um die Ausführung von Einzeltransaktionen mit Vermögenswerten zu ermöglichen, die dies akzeptieren permit
Aufrufe, Zulagen zu ändern.
Allerdings ist die ruft an die Permitable
Vertrag validieren Sie nicht, ob das Ziel ein zulässiger Vermögenswert ist oder ob es sich überhaupt um einen Vermögenswert handelt, der es einem böswilligen Benutzer ermöglichen könnte, die Adresse eines willkürlichen Vertrags zu übergeben, der einen weiteren Anruf ausführen könnte, bevor die Auftragserfüllung abgeschlossen ist.
Obwohl der Vertrag steht vor Wiedereinstieg geschützt, die Angriffsfläche zu reduzieren und das Aufrufen externer Kontrakte während der Ausführung zu verhindern, wird immer empfohlen. Erwägen Sie, entweder die von der Genehmigung betroffenen Vermögenswerte auf die Vermögenswerte zu beschränken, die in der Anordnung enthalten sind, oder auf eine Whitelist für Vermögenswerte für das Protokoll.
Update: Nicht behoben. Das 1-Zoll-Team erklärt:
OrderMixin
hat eigentlich keine Informationen über tatsächliche Token wiemakerAsset
undtakerAsset
manchmal sind Proxys oder andere Zwischenverträge und Informationen über tatsächliche Tokens werden in einigen willkürlichen Bytes gespeichert. Es gibt also keine praktikable Möglichkeit, welches Asset einzuschränkenpermit
aufgerufen wird.
[N13] solhint
wird nie wieder aktiviert
In der gesamten Codebasis gibt es ein paar solhint-disable
Aussagen, insbesondere die online 23 und online 41 of RevertReasonParser.sol
, die nicht mit abgeschlossen werden solhint-enable
.
Zu Gunsten von Eindeutigkeit und möglichst restriktivem Deaktivieren solhint
erwägen, zu verwenden solhint-disable-line
or solhint-disable-next-line
stattdessen ähnlich wie Linie 16 derselben Datei.
Update: Fest eingebaut Pull-Anfrage # 72.
[N14] Tippfehler
Die Codebasis enthält die folgenden Tippfehler:
Zusätzlich die des Projekts README
(außerhalb des Geltungsbereichs dieser Prüfung) enthält die folgenden Tippfehler:
Erwägen Sie, diese Tippfehler zu korrigieren, um die Lesbarkeit des Codes zu verbessern.
Update: Fest eingebaut Pull-Anfrage # 71 und Pull-Anfrage # 77.
[N15] Verwendung von uint
statt uint256
Um die Explizitheit zu fördern, alle Instanzen von uint
sollte deklariert werden als uint256
. Insbesondere diejenigen in der for
Schleifen auf Linien 98 und 119 of OrderMixin.sol
und Linien 16 und 30 of PredicateHelper.sol
.
Update: Fest eingebaut Pull-Anfrage # 70.
Schlussfolgerungen
Es wurden 3 Probleme mit hohem Schweregrad gefunden. Einige Änderungen wurden vorgeschlagen, um Best Practices zu folgen und die potenzielle Angriffsfläche zu reduzieren.
- &
- 7
- Über uns
- Zugang
- Nach
- Konto
- über
- Handlung
- Aktionen
- Zusätzliche
- Adresse
- Vorteil
- Alle
- Zulassen
- bereits
- Obwohl
- Beträge
- Analyse
- Bienen
- Ansatz
- Argumente
- um
- Vermögenswert
- Details
- Prüfung
- Back-End
- Anfang
- Sein
- BESTE
- Best Practices
- Bit
- Bots
- bauen
- rufen Sie uns an!
- österreichische Unternehmen
- Fälle
- Verursachen
- Kettenglied
- Übernehmen
- Überprüfung
- Schecks
- Code
- Komplex
- Zustand
- Verwirrung
- Baugewerbe
- enthält
- Vertrag
- Verträge
- Korrekturen
- Kosten
- könnte
- Paar
- Schöpfer
- Währung
- Strom
- technische Daten
- Deal
- Denial of Service
- Bereitstellen
- Design
- Entwickler
- Entwicklung
- DID
- abweichen
- anders
- Domain
- doppelt
- dynamisch
- Früh
- Edge
- ermutigen
- insbesondere
- ETH
- Event
- Veranstaltungen
- alles
- Beispiel
- Austausch-
- erwartet
- ERFAHRUNGEN
- Ausnutzen
- Eigenschaften
- Felder
- Endlich
- Vorname
- Fixieren
- Flexibilität
- Fluss
- folgen
- gefunden
- voller
- Funktion
- Mittel
- Zukunft
- Spiel
- GAS
- Allgemeines
- Unterstützung
- groß
- Guide
- Handling
- Hash-
- Hashing
- mit
- Hilfe
- High
- hoch
- Ultraschall
- HTTPS
- Hybrid
- identifizieren
- Impact der HXNUMXO Observatorien
- implementieren
- wichtig
- Einfuhr
- inklusive
- Einschließlich
- Erhöhung
- hat
- Info
- Information
- Infrastruktur
- Einblicke
- Absicht
- Interesse
- Schnittstelle
- beteiligt
- Probleme
- IT
- grosse
- größer
- führen
- führenden
- Bibliothek
- Limitiert
- Line
- Liquidity
- Gelistet
- Listen
- aus einer regionalen
- sah
- Nachschlagen
- Dur
- Hersteller
- Making
- Markt
- Mempool
- Spiegel
- Modell
- vor allem warme
- Am beliebtesten
- nämlich
- Neue Funktionen
- nicht fungibel
- Nicht fungible Token
- offiziell
- XNUMXh geöffnet
- Einkauf & Prozesse
- Option
- Orakel
- Auftrag
- Bestellungen
- Andere
- Eigentümer
- Schnittmuster
- Beliebt
- Gegenwart
- Verhütung
- Preis
- gebühr
- privat
- Prozessdefinierung
- Projekt
- Projekte
- Sicherheit
- Protokoll
- die
- bietet
- Stellvertreter
- Öffentlichkeit
- veröffentlichen
- Kauf
- Qualität
- erhöhen
- Realität
- Veteran
- Vertrauen
- berichten
- Meldungen
- Quelle
- Voraussetzungen:
- REST
- Die Ergebnisse
- Rückgabe
- Überprüfen
- Risiko
- Runde
- Führen Sie
- Sdk
- Sicherheitdienst
- Leistungen
- kompensieren
- von Locals geführtes
- Shares
- verschieben
- ähnlich
- Einfacher
- klein
- smart
- Smart Contracts
- So
- solide
- Spam
- speziell
- Ausgabe
- Spot
- Anfang
- Bundesstaat
- Erklärung
- Staaten
- Status
- Lagerung
- Stil
- eingereicht
- Erfolg
- erfolgreich
- Erfolgreich
- Support
- Unterstützte
- Oberfläche
- Schalter
- System
- Target
- Test
- Testen
- Tests
- Durch
- während
- Zeit
- gemeinsam
- Zeichen
- Tokens
- verfolgen sind
- Tracking
- Transaktion
- Vertrauen
- einzigartiges
- Updates
- us
- Nutzbarkeit
- USD
- USDT
- Nutzer
- Nutzen
- Wert
- Anzeigen
- Sichtbarkeit
- warten
- Was
- Whitelist
- .
- ohne
- Arbeiten
- wert
- Null