16 december 2021
Introductie
De 1inch team heeft ons gevraagd om hun Limiet Order Protocol v2 slimme contracten. We hebben de code bekeken en publiceren nu onze resultaten.
strekking
We hebben commit gecontroleerd 4d94eea25e4dac6271bfd703096a5c4a4d899b4a
van de 1inch/limit-order-protocol
opslagplaats. In scope waren de volgende contracten:
- 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 andere projectbestanden en mappen (inclusief tests), samen met externe afhankelijkheden en projecten, speltheorie en incentive-ontwerp, werden ook uitgesloten van de reikwijdte van deze audit. Externe code en contractafhankelijkheden werden verondersteld te werken zoals gedocumenteerd en back-endservices die door 1inch werden geleverd, werden verondersteld in het belang van het protocol te handelen.
Algemene gezondheid
Over het algemeen vonden we de codebase van het project leesbaar en goed georganiseerd, hoewel het zou kunnen profiteren van uitgebreidere documentatie, vooral rond de blokken assemblagecode, randgevallen van het protocol, activa/predicaten/externe bronnen die zullen worden gebruikt, verantwoordelijkheden/beperkingen van de geleverde back-endservice en interacties tussen actoren. Het project doet er alles aan om acties gas-efficiënt te maken, soms zelfs met het risico dat de code moeilijker te redeneren wordt; hieronder stellen we problemen die daarmee verband houden. Tijdens de audit was het 1inch-team zeer beschikbaar, responsief en zeer gemakkelijk om mee samen te werken.
Systeem overzicht
Het Limit Order Protocol maakt orde mogelijk makers
om off-chain orders te ondertekenen voor token swaps. Het protocol vergemakkelijkt vervolgens het invullen van eerder ondertekende bestellingen op bestelling takers
. Orders zijn zeer uitbreidbaar en kunnen externe contracten op verschillende punten tijdens het orderverwerkingsproces statisch oproepen. Deze uitbreidbaarheid doordrenkt het protocol met bruikbaarheid, maar het voegt zowel complexiteit als een groter aanvalsoppervlak toe voor de orders zelf.
Het is belangrijk op te merken dat er geen on-chain opslag van ordergegevens is. De vulstatus of annuleringsstatus van bestellingen wordt alleen bijgehouden via orderhashes. Hiervoor is het noodzakelijk dat bestellingen peer-to-peer of via een centrale partij worden gedeeld. In dit geval is het 1inch-team van plan om als die gecentraliseerde partij op te treden, ondertekende orders samen te voegen en die orders te gebruiken als een bron van liquiditeit voor hun andere protocollen. Bestellingen worden gepubliceerd via hun eigen API, zodat gebruikers ermee kunnen communiceren.
Deze centralisatie geeft het 1inch-team extreme controle over welke orders worden gepubliceerd en uiteindelijk worden uitgevoerd. Dit geeft hen ook de mogelijkheid om bestellingen te censureren, wat handig kan zijn in het geval van kwaadwillende of bedrieglijke bestellingen, maar ook kan worden misbruikt en hen in staat stelt een andere gebruiker op de voorgrond te plaatsen in het geval van een gunstige bestelling door deze niet via de API te tonen.
Bevoorrechte rollen
Hoewel de contracten waarin de rol wordt gebruikt buiten het bereik vallen, is er één bevoorrechte rol geïdentificeerd. Een immutableOwner
is ingesteld op de maker van een proxy-contract op het moment van constructie, en wordt gebruikt om de toegang tot de proxy's te beperken external
functies.
Externe afhankelijkheden en vertrouwensaannames
Het ontwerp van dit protocol vereist off-chain en on-chain componenten, en dit hybride model kan worden gebruikt om sommige aanvalsvectoren die we in ons rapport identificeren te verminderen, maar de kosten van dat vermogen zijn een grotere afhankelijkheid van het 1-inch team en de infrastructuur.
Daarnaast biedt het Limit Order Protocol functies die bedoeld zijn om prijzen op te halen uit Chainlink-orakels. We gingen ervan uit dat die orakels eerlijk, toegankelijk en goed functionerend waren.
Bovendien zijn er door de flexibiliteit van een bestelling meerdere contactpunten met externe contracten die niet gevalideerd zijn. Dit betekent dat een kwaadwillende gebruiker dergelijke oproepen kan misbruiken en zich kan voordoen als predicaten, activa of orakels met kwaadaardige contracten om acties uit te voeren tijdens het vullen van bestellingen. Hoewel het project in sommige gebieden is beschermd tegen herintreding, kunnen dergelijke vectoren denial-of-service-aanvallen of onopgemerkte spam-opdrachten veroorzaken. Het 1inch-team is zich ervan bewust dat er bepaalde problemen kunnen optreden bij het gebruik van onbekende contracten voor het protocol en heeft aangegeven van plan te zijn dat alleen grote "blue chip"-middelen volledig door het project zullen worden ondersteund. Er moet echter worden opgemerkt dat zelfs bij de meest populaire activa er intrinsiek gedrag is van elk activum dat problemen kan veroorzaken met protocollen die ze niet goed aanpakken, zoals een vergoeding tijdens overboekingen met USDT of het retourneren van een foutcode in plaats van een succes boolean met cTokens.
Bevindingen
Hier presenteren we onze bevindingen.
Kritieke ernst
Geen.
Hoge ernst
[H01] Inconsistente gegevens doorgegeven aan _makeCall
In het OrderMixin
contract, het _makeCall
functie wordt gebruikt om activa over te dragen van de nemer naar de maker en van de maker tot de nemer. Bij de laatste overdracht, de _makeCall
functie is onjuist doorgegeven aan de bestelling makerAsset
als de laatste parameter, wanneer het de bestelling zou moeten zijn makerAssetData
.
Als gevolg hiervan is elke proxyfunctionaliteit die afhankelijk is van de makerAssetData
argument zal breken.
Om consistent te zijn met de eerdere oproep tot: _makeCall
en om de proxy-functionaliteit volledig te ondersteunen, kunt u overwegen de order.makerAsset
parameter order.makerAssetData
.
update: Vast in trekverzoek # 57.
[H02] Gedeeltelijk ingevulde privébestellingen kunnen door iedereen worden ingevuld
Het protocol maakt de creatie van private en publieke orders mogelijk. Op particuliere bestellingen, alleen de allowedSender
adres, opgegeven door de maker tijdens het maken van de bestelling, kan de bestelling vullen.
In de OrderMixin
contract, validatie voor de allowedSender
adres heeft een onjuist bereik, wat betekent dat het alleen wordt geëvalueerd binnen de logica die de eerste vulling van een bestelling afhandelt. Als een privé-bestelling gedeeltelijk is ingevuld, dan wordt de cheque voor de allowedSender
adres is niet langer bereikbaar en de bestelling kan door iedereen worden ingevuld.
Om de bedoeling te verduidelijken of een gebruiker al dan niet gedeeltelijk gevulde privé-orders moet kunnen invullen, kunt u overwegen de reden voor het huidige gedrag te documenteren of de allowedSender
adres buiten het bereik van de eerste vulling om ervoor te zorgen dat deze wordt gevalideerd telkens wanneer een vulling wordt geprobeerd.
update: Vast in trekverzoek # 58.
[H03] Kwaadwillende maker kan profiteren van gedeeltelijke vullingen om activa van de nemer te stelen
Bestellingen van de OrderMixin
contract hebben de mogelijkheid om gedeeltelijk te worden ingevuld. Om gedeeltelijke vullingen te ondersteunen, vereist het protocol een manier om beide zijden van swaps te berekenen. Beide getMakerAmount
en getTakerAmount
velden worden voor dit exacte doel gedefinieerd door de maker van de bestelling.
Bij het invullen van een bestelling moeten afnemers ofwel de: makingAmount
of de takingAmount
waarden evenals a thresholdAmount
waarde. Er zijn twee verschillende code-paden die kunnen worden genomen, gebaseerd op of de makingAmount
of de takingAmount
werd verstrekt.
De eerste is wanneer de makingAmount
parameter is gedefinieerd. Het zou kunnen afkappen de makingAmount
waarde en ook Bereken de takingAmount
waarde ervoor. In deze situatie is de thresholdAmount
zorgt ervoor dat de takingAmount
waarde genomen is niet onverwacht groot.
De tweede is wanneer de takingAmount
parameter is gedefinieerd. In dat geval zal het Bereken de makingAmount
waarde, met de mogelijkheid van het afkappen en herberekenen van de takingAmount
waarde als dat gebeurt. In deze situatie is de thresholdAmount
waarde zorgt ervoor dat de makingAmount
geretourneerde waarde is niet onverwacht klein.
Er bestaan twee exploitatiemethoden, elk uniek voor een van de eerder genoemde codepaden. Deze exploitatiemethoden vereisen kwaadaardige getMakerAmount
en getTakerAmount
functies. Een eenvoudige implementatie van deze functies zou hetzelfde gedrag vertonen als AmountCalculator
's getMakerAmount
en getTakerAmount
functies, maar met een hardgecodeerde schakelaar die hen dwingt om een door de aanvaller gecontroleerde waarde terug te geven wanneer dat nodig is.
Het eerste, minder ernstige exploitpatroon omvat het eerste codepad waar de makingAmount
waarde is opgegeven in een vulopdracht. Een kwaadwillende maker zou wachten op een vulopdracht die specificeert: makingAmount
om in de mempool te verschijnen om het voorop te lopen. Ze zouden alle waarde behalve 1 van de kant van de maker aftappen en dan forceren _callGetTakerAmount
om het bedrag terug te geven dat is opgegeven in de gebruikers thresholdAmount
waarde (of hun toelage als deze lager is). Wanneer de transactie van de gebruiker uiteindelijk doorgaat, wisselen ze hun volledige thresholdAmount
waarde van takerAsset
voor een enkele eenheid van makerAsset
. Deze exploit wordt beperkt door het bedrag dat wordt gegeven door de thresholdAmount
waarde of het bedrag van de takerAsset
de gebruiker toegestaan op de LimitOrderProtocol
contract.
Het tweede, meer ernstige exploitpatroon omvat het tweede codepad waar de takingAmount
waarde is opgegeven. De kwaadwillende maker zou op dezelfde manier wachten op een vulopdracht die een takingAmount
waarde om in de mempool te verschijnen. Ze zouden de transactie voorop lopen en de makingAmount
waarde geretourneerd door _callGetMakerAmount
functie hoger zijn dan zowel de remainingMakerAmount
en thresholdAmount
. Ze zouden ook de takingAmount
geretourneerde waarde door _callGetTakerAmount
het bedrag zijn van takerAsset
actief toegestaan op de LimitOrderProtocol
door de nemer. Wanneer de transactie van de nemer doorgaat, zal het de inkorten makingAmount
waarde en dan herbereken de takingAmount
waarde. Deze herberekening is echter niet gegarandeerd lager, en in dit geval zal de nemer alle kosten kwijtraken takerAsset
die ze in het contract hebben toegestaan. In dit codepad, de thresholdAmount
Waarde is ervoor zorgen dat de makingAmount
is niet te laag, dus het nemen van alle nemers takerAsset
actief is niet aangevinkt. Het verloren geld wordt beperkt door het bedrag van de takerAsset
asset die de gebruiker heeft toegestaan op de LimitOrderProtocol
contract.
Deze exploits zijn niet mogelijk zonder deelopdrachten en meer specifiek deelopdrachten met kwaadwillende getMakerAmount
en getTakerAmount
implementaties.
Het belangrijkste probleem van de thresholdAmount
waardecontrole is dat het slechts één kant van de swap dekt, maar de andere kant kan worden gemanipuleerd via frontrunning. Er zijn geen garanties dat de waarde die de nemer oorspronkelijk voorstelde, ongewijzigd blijft. Overweeg om te verwijderen makingAmount
afkappen van beide codepaden en terugdraaien als de bestelling een vulling zo groot als gevraagd niet kan ondersteunen. Door dit te doen, thresholdAmount
kan worden gebruikt om de andere kant van de swap voldoende te beperken en onverwacht gedrag te voorkomen, zelfs in kwaadaardige orders.
update: Vast in trekverzoek # 83.
Middelmatige ernst
[M01] Statische argumenten doorgegeven na dynamische argumenten
In het OrderMixin
contract, het getTakerAmount
en getMakerAmount
bytes velden worden gebruikt als argumenten voor de _callGetTakerAmount
en _callGetMakerAmount
functies. Deze oproepen bieden een manier om de ene kant van de swap te berekenen op basis van de andere kant, en ze stellen gebruikers in staat om bestellingen gedeeltelijk uit te voeren.
De getTakerAmount
/getMakerAmount
velden zijn dynamische variabelen en staan voor de takerAmount
en makerAmount
waarden in de _callGetTakerAmount
en _callGetMakerAmount
functies. Het is mogelijk dat een kwaadwillende maker meer gegevens verstrekt dan verwacht in de getTakerAmount
engetMakerAmount
velden om de . te duwen takerAmount
en makerAmount
bytes voorbij waar ze worden verondersteld te zijn wanneer ze worden gedecodeerd in de volgende functie. Hierdoor kan de maker het doorgegeven bedrag van de nemer of maker een volledige bytes naar rechts verschuiven en zelfs volledig vervangen als er 32 bytes extra gegevens worden verstrekt.
Gebruikers moeten de . al handmatig controleren getTakerAmount
en getMakerAmount
velden in de volgorde, maar deze techniek is nogal moeilijk te herkennen. Ook vermeldenswaard is dat deze aanval zelfs van toepassing is op de intern vertrouwde getMakerAmount
en getTakerAmount
functies. Voor de meeste aanvallen zal het verstrekken van een redelijk drempelbedrag verlies van geld voorkomen.
Om dit te voorkomen, kunt u overwegen de statische argumenten vóór de dynamische argumenten te coderen om te voorkomen dat de dynamische argumenten een methode krijgen om de statische argumenten te controleren.
update: Niet gemaakt. Het 1inch-team verklaarde:
We zullen extra voorzichtig zijn met de validatie van getters. We zullen proberen om sanity-validatie van getters in onze sdk te implementeren die zal helpen bij het filteren van potentieel kwaadaardige bestellingen.
[M02] ERC721-bestellingen kunnen worden gemanipuleerd
Het is mogelijk om meer dan alleen ERC20's uit te wisselen via de OrderMixin
door een contract in te zetten dat dezelfde functiekiezer deelt als die van IERC20 transferFrom
, en het verstrekken van dat contract als de makerAsset
of de takerAsset
in een bestelling.
De out-of-scope proxy's, namelijk, ERC721Proxy
, ERC721ProxySafe
en ERC1155Proxy
contracten volgen dit patroon om ondersteuning te bieden voor: ERC721
en ERC1155
Munten. Aangezien de proxy's moeten worden aangeroepen met hetzelfde patroon als een IERC20 transferFrom
oproep, de handtekening moet beginnen met address from
, address to
en uint256 amount
. Al het andere dat de proxy's nodig hebben, kan daarna worden doorgegeven en wordt gedefinieerd in de volgorde als: makerAssetData
en takerAssetData
.
ERC1155's kunnen natuurlijk meerdere van dezelfde id-tokens tegelijk overdragen, wat betekent dat de ERC1155Proxy
contract maakt gebruik van de amount
veld. Aan de andere kant, ERC721
s hebben geen duidelijk gebruik voor de amount
veld. Omdat ze niet-fungible tokens vertegenwoordigen, zal een specifieke tokenId er maar één hebben, waardoor de amount
veld nutteloos. Hierdoor is de implementatie voor beide ERC721Proxy
en ERC721ProxySafe
contracten gebruiken de vereiste amount
veld als de tokenId
gebruiken.
Deze overbelasting van de amount
parameter creëert de mogelijkheid om gedeeltelijk te vullen ERC721
bestellingen om afzonderlijk vermelde tokens tegen gereduceerde prijzen te kopen. Er kan bijvoorbeeld een geval zijn waarin een enkele gebruiker meerdere ERC721
s van hetzelfde contract mogen worden overgedragen door de ERC721Proxy
contract en vermeldt ze in afzonderlijke limietorders.
Als de limietorders ook de getMakerAmount
en getTakerAmount
velden, is het mogelijk om deze gedeeltelijk in te vullen ERC721
bestellingen. Sinds de bestelling is amount
veld komt eigenlijk overeen met de tokenId
, kan een kwaadwillende gebruiker een gedeeltelijke vulling plaatsen op de ERC721
met de hogere tokenId, wat resulteert in a makingAmount
/takingAmount
een ERC721
dat zou kunnen overeenkomen met een lagere tokenId
. Het resultaat is de ERC721
met de lagere tokenId
zou worden overgedragen tegen de prijs van (higher tokenId price) * (lower tokenId's id) / (higher tokenId's id)
.
Deze exploit heeft een aantal vereisten:
- meervoudig
ERC721
s van hetzelfde contract worden toegestaan op een van beideERC721
volmacht door een enkele eigenaar. - Open bestelling voor een van de
ERC721
s dat is niet de laagstetokenId
van de toegestane. - Gedeeltelijke vullingen toegestaan op de bestelling.
Om de mogelijkheid van gedeeltelijke ERC721
vullingen, overweeg dan om de te scheiden amount
en tokenId
argumenten. Of de argumenten nu gescheiden zijn of niet, overweeg om dit ook te documenteren om gebruikers te waarschuwen voor dit gedrag en om dit patroon in de toekomst te vermijden.
update: Vast in trekverzoek # 59.
[M03] Ongedocumenteerde decimale aannames
De LimitOrderProtocol
contract erft de ChainlinkCalculator
contract via de OrderMixin
contract. Dit contract stelt twee functies bloot om het gebruik van Chainlink-orakels mogelijk te maken tijdens de: predikaten controleren en het opzoeken van de maker bedrag/afnemer bedrag.
Het contract maakt echter ongedocumenteerde aannames over het aantal decimalen waarin de Chainlink-orakels moeten rapporteren, evenals het aantal decimalen dat de functieparameters moeten bevatten. In bepaalde scenario's kan dit leiden tot onverwacht gedrag, waaronder verkeerde prijzen van activa en onbedoeld verlies van geld.
Meer specifiek, gedurende het hele contract is de impliciete veronderstelling dat de Chainlink-orakels zullen rapporteren met een nauwkeurigheid van 18 decimalen. Echter, niet alle Chainlink orakels rapport met dit aantal decimalen. Als het orakel een tokenpaar meldt dat in termen van een valuta is (bijvoorbeeld USD), heeft het slechts 8 decimalen nauwkeurig. Aangezien er geen beperkingen zijn op welke orakels kunnen worden gebruikt, mogen er geen impliciete veronderstellingen worden gemaakt over het aantal decimalen waarmee ze zullen rapporteren.
In verband hiermee is er een impliciete veronderstelling dat de amount
parameter voor de ChainlinkCalculator
functies zullen 18 decimalen gebruiken, samen met de misleidende expliciete verklaring dat de singlePrice
functie Calculates price of token relative to ETH scaled by 1e18
. In werkelijkheid, zelfs met een orakel dat doet rapport met 18 decimalen, de geretourneerde waarde van de singlePrice
functie zou worden geschaald met het aantal decimalen van de amount
parameter, die niet noodzakelijk 18 decimalen hoeft te zijn.
Evenzo is de doublePrice
functie gaat ervan uit dat twee Chainlink-orakels met hetzelfde aantal decimalen zullen rapporteren, waardoor het resultaat van de functie afwijkt van de verwachtingen.
Overweeg expliciet om aannames te documenteren met betrekking tot het aantal decimalen dat parameters en retourwaarden moeten zijn in termen van. Overweeg bovendien om berekeningen te beperken die afhankelijk zijn van orakels die deze aannames doorbreken, of om bij de relevante berekeningen rekening te houden met het werkelijke aantal decimalen.
update: Vast in trekverzoek # 75.
Lage ernst
[L01] Constanten niet expliciet gedeclareerd
Er zijn enkele gevallen waarin letterlijke waarden worden gebruikt met onverklaarde betekenis in de codebase. Bijvoorbeeld:
- In het
OrderMixin
contract, het_remaining
mapping is semantisch overbelast (zoals uitgelegd in het probleem Semantische overbelasting van mapping) om het resterende bedrag van activa voor een gedeeltelijk gevulde bestelling te volgen net zoals als een bestelling volledig is uitgevoerd. specifiek,0
betekent dat er geen vullingen zijn gekoppeld aan een bestelling,1
betekent dat een bestelling niet meer kan worden uitgevoerd, en alles groter dan1
betekent dat er een resterend bedrag aan de bestelling is gekoppeld dat mogelijk kan worden uitgevoerd. - In het
ChainlinkCalculator
contract, de letterlijke waarde1e18
wordt gebruikt in desinglePrice
functie.
Om de leesbaarheid van de code te verbeteren en refactoring te vergemakkelijken, kunt u overwegen een constante te definiëren voor elk magisch getal, en het een duidelijke en voor zichzelf sprekende naam te geven. Overweeg voor complexe waarden een inline-opmerking toe te voegen waarin wordt uitgelegd hoe ze zijn berekend of waarom ze zijn gekozen.
update: Vast in trekverzoek # 75 en trekverzoek # 76.
[L02] Kwaadwillenden kunnen de uitvoering van toegestane orders verhinderen
De OrderMixin
contract stelt maker-gebruikers in staat om in te dienen toegestane bestellingen zodat deze in één transactie kunnen worden uitgevoerd, in plaats van een afzonderlijke transactie voor goedkeuringen te hebben. Ook kunnen bestellers hun eigen vergunning indienen tijdens het invullen van de bestelling voor hetzelfde doel.
Omdat de vergunning van de maker echter in de bestellen, zouden zowel de vergunning van de maker als de nemer toegankelijk zijn terwijl de order-fill-transactie in de mempool is. Dit zou het voor elke kwaadwillende gebruiker mogelijk maken om die vergunningen te nemen en ze uit te voeren op de respectieve activacontracten terwijl ze de vultransactie vooroplopen. Omdat deze vergunningen een nonce
om een dubbele bestedingsaanval te voorkomen, zou de ordervultransactie mislukken als gevolg van het proberen om dezelfde vergunning te gebruiken die net tijdens de frontrun werd gebruikt.
Hoewel er geen veiligheidsrisico is en de maker een nieuwe bestelling kan plaatsen en de transactie vooraf kan goedkeuren, kan deze aanval zeker invloed hebben op de bruikbaarheid van toegestane bestellingen. Een gemotiveerde aanvaller zou inderdaad kunnen blokkeren allen toegestane orders met deze aanval. Overweeg om te valideren of de vergunning al is ingediend, of dat de vergoeding voldoende is, tijdens het vullen van de bestelling. Overweeg ook om gebruikers op de hoogte te stellen van deze mogelijke aanval tijdens het samenstellen van de bestelling.
update: Niet gemaakt. Het 1inch-team stelt:
We hadden eerder goedkeuringscontroles, maar besloten de vergunningstroom te vereenvoudigen om gewoon terug te keren naar mislukte goedkeuringen. We gaan nadenken over manieren om makers op de hoogte te stellen van het probleem.
[L03] Dubbele code
Er zijn gevallen van gedupliceerde code binnen de codebase. Het dupliceren van code kan later in de ontwikkelingslevenscyclus tot problemen leiden en maakt het project vatbaarder voor de introductie van fouten. Dergelijke fouten kunnen onbedoeld worden geïntroduceerd wanneer functionaliteitswijzigingen niet worden gerepliceerd in alle exemplaren van code die identiek zou moeten zijn. Voorbeelden van gedupliceerde code zijn:
In plaats van code te dupliceren, kunt u overwegen om slechts één contract of bibliotheek te hebben met de gedupliceerde code en deze te gebruiken wanneer de gedupliceerde functionaliteit vereist is.
update: Gedeeltelijk gefixeerd trekverzoek # 60.
[L04] Foutieve of misleidende testsuite
Er zijn gevallen in de testsuite waar de tests afwijken van het verwachte gedrag. Bijvoorbeeld:
- De
ChainlinkCalculator
contract wordt geërfd door deOrderMixin
contract. Tijdens de tests echterAmountCalculator.arbitraryStaticCall
functie wordt gebruikt om de . aan te roepenChainlinkCalculator
contract als een extern, zelfstandig contract. Ook al is het resultaat het verwachte resultaat, de test moet het gedrag weerspiegelen met het huidige ontwerp van het systeem en de verwachte use case door te bellenChainlinkCalculator
functies direct zonder gebruik te maken van de willekeurige statische oproep. - Hoewel de proxy-contracten buiten het bereik vielen, merkten we dat bij het testen van het protocol met ERC721-assets de
ERC721Proxy
contract wordt niet gebruikt om de activa in zijn test pak.
Aangezien de testsuite zelf buiten het bereik van deze audit valt, kunt u overwegen om de testsuite grondig door te nemen om er zeker van te zijn dat alle tests met succes worden uitgevoerd volgens de specificaties van het protocol.
update: Vast in trekverzoek # 57, trekverzoek # 59 en trekverzoek # 61.
[L05] Fouten en weglatingen in evenementen
In de hele codebase worden over het algemeen gebeurtenissen uitgezonden wanneer gevoelige wijzigingen in de contracten worden aangebracht. Veel gebeurtenissen missen echter geïndexeerde parameters en/of missen belangrijke parameters. Bijvoorbeeld:
Er zijn ook gevoelige acties die gebeurtenissen missen, zoals:
Overweeg om bestaande gebeurtenissen vollediger te indexeren en nieuwe parameters toe te voegen waar ze ontbreken. Overweeg ook om alle gebeurtenissen op zo'n volledige manier uit te zenden dat ze kunnen worden gebruikt om de status van het contract te herstellen door off-chain services.
update: Niet gemaakt. Het 1inch-team voegde echter een orderRemaining
parameter aan de OrderCanceled
evenement in trekverzoek # 62.
Het 1inch-team stelt:
We ontdekten dat er slechts een beperkte subset van gegevens nodig is om aan de frontend-behoeften te voldoen. Bij uitgebreide analyse zijn alle voorgestelde velden via tracering beschikbaar. Voor
OrderRFQMixin
we verwachten dat market makers hun eigen geavanceerde manier ontwikkelen om bij te houden welke orders zijn geannuleerd.
[L06] Opslagwijzigingen tijdens gebeurtenisemissie
In het NonceManager
contract, wanneer de NonceIncreased
evenement wordt uitgezonden, de nonce van de afzender van het bericht wordt ook verhoogd.
Het gelijktijdig uitvoeren van meerdere bewerkingen kan de codebase moeilijker maken om over te redeneren, vatbaarder voor fouten en kan ertoe leiden dat bewerkingen over het hoofd worden gezien of verkeerd worden begrepen.
Om de algehele opzet, leesbaarheid en duidelijkheid van de code te verbeteren, kunt u overwegen de nonce-waarde te verhogen voordat u de gebeurtenis uitzendt.
update: Vast in trekverzoek # 63.
[L07] Inconsistente decoderingsmethodologieën kunnen verschillen in uitkomsten veroorzaken
Om al zijn uitbreidbaarheid en flexibiliteit te ondersteunen, heeft het Limit Order Protocol routinematig te maken met dynamische bytes-gegevens en willekeurige retourwaarden van externe contracten. Als gevolg hiervan bevat het protocol een ArgumentsDecoder
bibliotheek om dynamische bytes-waarden efficiënter om te zetten in basisgegevenstypen. Deze bibliotheek wordt echter niet exclusief gebruikt, en in sommige gevallen abi.decode
in plaats daarvan wordt gebruikt. Bovendien gebruiken sommige contracten abi coder v1
terwijl anderen gebruiken abi coder v2
. De eerste presteert meer vergelijkbaar met de ArgumentsDecoder
bibliotheek, terwijl de laatste extra controles uitvoert bij het decoderen.
Het inconsistente gebruik van deze verschillende decoderingsmethodologieën kan resulteren in subtiele discrepanties tussen de intentie en het daadwerkelijke gedrag van de codebase.
Bijvoorbeeld, de simulateCalls
functie gebruikt alleen de ArgumentsDecoder.decodeBool
functie. Als het simulateCalls
functie wordt gebruikt om aanroepen te controleren die zouden worden gedaan in het predikaatgedeelte van een bestelling, dan kunnen de resultaten ervan afwijken van wat er feitelijk gebeurt wanneer de predikaatvoorwaarden worden geëvalueerd, omdat verschillende decoderingsmethodologieën worden gebruikt.
Dus, bijvoorbeeld, als een predikaat een extern staticcall
naar een functie die a . retourneert uint256
waarde groter dan één in plaats van de verwachte bool
, dan zal die oproep terugkeren, omdat de geretourneerde waarde is gedecodeerd met abi coder v2
's abi.decode
die dergelijke waarden niet accepteert als bool
. Als echter exact dezelfde oproep wordt gedaan met simulateCalls
dan is het wordt gewoon gemarkeerd als true
omdat decodeBool
behandelt elke waarde groter dan nul als true
.
Om het te maken simulateCalls
functie weerspiegelt volledig het gedrag van werkelijke predikaataanroepen, overweeg deze aan te passen om te gebruiken abi.decode
.
update: Vast in trekverzoek # 82.
[L08] Gebrek aan invoervalidatie
De fillOrderToWithPermit
en fillOrderTo
functies van de OrderMixin
contract, evenals de fillOrderRFQToWithPermit
en fillOrderRFQTo
functies van de OrderRFQMixin
contract, valideer de target
adres parameter.
Dit maakt het voor een gebruiker mogelijk om per ongeluk het nuladres door te geven en als gevolg daarvan de activa die ze moeten ontvangen na het invullen van een bestelling op slot te doen.
Om ervoor te zorgen dat gebruikers hun geld niet per ongeluk blokkeren, kunt u overwegen te valideren dat de target
adres is niet gelijk aan het nuladres in de genoemde functies.
update: Vast in trekverzoek # 78.
[L09] Lage testdekking van de unit
De dekking van de unittest voor het hele project is ongeveer 75%, waarbij sommige contracten een bijzonder lage dekking hebben.
Gezien het belang van unit-tests om code te valideren en regressies te voorkomen bij het herstructureren en ontwikkelen van nieuwe functies, raden we aan om de dekking van unit-tests aanzienlijk te verhogen tot ten minste 95%, en om edge-cases op te nemen die zelfs onwaarschijnlijke situaties dekken.
update: Niet gemaakt.
[L10] Misleidende of onvolledige inline documentatie
In de hele codebase zijn enkele gevallen van misleidende en/of onvolledige inline-documentatie geïdentificeerd die moeten worden verholpen.
Hieronder volgen voorbeelden van misleidende inline-documentatie:
- In het
ChainlinkCalculator
contract, hetsinglePrice
functie NatSpec@notice
label zegt dat hetCalculates price of token relative to ETH scaled by 1e18
, maar in feite is het resultaat de waarde ofamount
tokens geschaald door1e18
, waar het orakel mogelijk niet rapporteert in termen van ETH (bijvoorbeeld voor een paar zonder ETH). - In het
OrderRFQMixin
contract, hetinvalidatorForOrderRFQ
functie NatSpec@return
label is misleidend, omdat het citaat mogelijk niet is ingevuld om het betreffende ongeldigheidsbit in te stellen. De bestelling kan ook geannuleerd zijn. - op lijnen 147, 165 en 188 of
OrderMixin.sol
, de NatSpec@param
tags zijn ongrammaticaal. - Online 20 of
ERC1155Proxy.sol
@notice
tag stelt dat de berekende hash het resultaat is van het hashen van defunc_733NCGU
functie, waar het de . zou moeten zijnfunc_301JL5R
in plaats daarvan functioneren.
Hier volgen voorbeelden van onvolledige inline-documentatie:
- Functies in de
AmountCalculator
contract beschrijft geen van de parameters. - In het
ChainlinkCalculator
contract, hetsinglePrice
endoublePrice
functies beschrijven niet alle parameters. - In het
ImmutableOwner
contract, hebben de publieke variabele en modifier geen NatSpec. - In het
InteractiveNotificationReceiver
contract, hetnotifyFillOrder
functie beschrijft geen van de parameters. - In het
LimitOrderProtocol
contract, hetDOMAIN_SEPARATOR
functie heeft geen NatSpec. - Evenementen en kaarten in de
NonceManager
geen NatSpec hebben. - In het
OrderRFQMixin
contract,cancelOrderRFQ*
functies beschrijven niet de geretourneerde waarden. - In het
OrderMixin
contract, missen verschillende functies volledige NatSpec. - Online 168 of
OrderMixin.sol
en online 71 ofOrderRFQMixin.sol
, het mist de@dev
label. - Functies in de
PredicateHelper
contract beschrijven niet alle parameters.
Duidelijke inline documentatie is van fundamenteel belang om de bedoelingen van de code te schetsen. Mismatches tussen de inline documentatie en de implementatie kunnen leiden tot ernstige misvattingen over hoe het systeem zich moet gedragen. Overweeg deze fouten op te lossen om verwarring voor zowel ontwikkelaars, gebruikers als auditors te voorkomen.
update: Gedeeltelijk vast. Misleidende documentatie behandeld in trekverzoek # 75 en trekverzoek # 77.
Het 1inch-team stelt:
We hebben misleidende documenten opgelost. De voltooiing van de documenten zal later worden gedaan.
[L11] DoS-bestellingen mogelijk bij gebruik van hooks
De OrderMixin
contract implementeert functionaliteit om generieke off-chain swap-orders te vullen die voorwaarden kunnen hebben voor hun succes. Tijdens het vullen van de bestelling kan de bestelling controleer de vooraf gedefinieerde "predikaat" voorwaarden alvorens verder te gaan met de uitvoering.
Omdat deze basisvoorwaarden echter de logica van elk willekeurig contract zouden kunnen aanvallen, zou een kwaadwillende maker de afnemers kunnen laten geloven dat een bestelling zich correct gedraagt en geldig is wanneer deze buiten de keten wordt gecontroleerd, maar vervolgens mislukken wanneer wordt geprobeerd dezelfde bestelling uit te voeren aan de ketting. Deze verandering in het gedrag van het predikaat kan ofwel worden gemaakt door een variabele toestand waarvan de predikaten afhankelijk zijn, vooraf te laten lopen, door het verzonden gas te onderzoeken of zelfs welke adressen bij de oproep betrokken zijn, of door een andere logica.
Bovendien, als de maker een interactie tijdens de swap interactionTarget
contract kan zichzelf terugdraaien of de vergoeding intrekken om een succesvolle ordervulling te voorkomen, wat in wezen leidt tot hetzelfde resultaat als kwaadaardige predikaten.
Hoewel activa geen risico lopen, zullen gebruikers of bots die een gunstige bestelling vinden, de zwaardere last hebben om te proberen dit soort spam-bestellingen te identificeren die op het eerste gezicht legitiem lijken. In het geval dat ze dit soort bestellingen niet identificeren, zullen ze verspilde gaskosten oplopen. Om het aantal spam-bestellingen te verminderen, kunt u overwegen de beschikbare doelen voor deze hooks te beperken. Overweeg ook om gebruikers te waarschuwen voor deze mogelijkheid voordat ze proberen bestellingen uit te voeren.
update: Niet gemaakt. Het 1inch-team stelt:
We behandelen dat in onze backend en we zullen nadenken over de manieren om mogelijke gebruikers op de hoogte te stellen van het probleem.
[L12] Afronding kan ongunstig zijn voor taker
In het OrderMixin
en OrderRFQMixin
contracten, wanneer een bestelling wordt uitgevoerd en de afnemer slechts een makingAmount
or takingAmount
bedrag, probeert het protocol het tegenwaardebedrag van de swap te berekenen.
Er zijn twee problemen met deze berekeningen, de eerste is dat er geen documentatie of logica is die het aantal decimalen beperkt dat de bedragparameters moeten gebruiken, wat we hebben besproken in de Ongedocumenteerde decimale aannames kwestie.
Het tweede punt is dat het protocol tijdens deze berekeningen in het voordeel van de maker afrondt. Het afrondingsprobleem kan enorm worden verergerd wanneer de impliciete decimale aannames worden verbroken, maar zelfs als alles in de verwachte termen is, zal afronding plaatsvinden met kleine, oneven bedragen.
Overweeg om de afnemer toe te staan een minimumbedrag op te geven van makerAsset
actief dat ze bereid zijn te ontvangen samen met een maximumbedrag van takerAsset
activa die ze bereid zijn te ruilen, zodat de acceptatie van afrondingen explicieter is.
update: Niet gemaakt. Het 1inch-team stelt:
Het drempelbedrag zou voldoende moeten zijn voor de bescherming van de afnemer.
[L13] Tegenstrijdige orderafhandeling bij ontbreken van parameters
In het OrderMixin
contract, het fillOrderTo
functie maakt interne oproepen naar de _callGetMakerAmount
en _callGetTakerAmount
functioneert telkens wanneer een vulling wordt geprobeerd en ofwel de makingAmount
of de takingAmount
parameters respectievelijk nul zijn, of als de makingAmount
waarde is groter dan de remainingMakerAmount
waarde.
De _callGetMakerAmount
en _callGetTakerAmount
oproepen leiden tot teruggaven als de bestelling niet is gemaakt met de getMakerAmount
or getTakerAmount
parameters, respectievelijk, en een gedeeltelijke vulling wordt uitgevoerd.
An inline commentaar hiernaast _callGetMakerAmount
en een inline commentaar hiernaast _callGetTakerAmount
beweren dat "alleen hele vullingen zijn toegestaan" als de bestelling niet is gemaakt met getMakerAmount
or getTakerAmount
parameters.
Er zijn echter codepaden waarvoor dit niet geldt, omdat die paden de length
van beide getMakerAmount
en getTakerAmount
parameters.
Specifiek wanneer een taker
specificeert a takerAmount
waarde voor een bestelling die alleen een . heeft getMakerAmount
, tenzij die oproep aan getMakerAmount
retourneert een bedrag groter dan remainingMakerAmount
, kan een gedeeltelijke vulling worden uitgevoerd in tegenstelling tot de inline-documentatie.
Dit laat de intentie van die codepaden onduidelijk. Als dit het verwachte gedrag is, overweeg dan om de inline-documentatie aan te passen zodat deze explicieter is. Als dit onbedoeld gedrag is, overweeg dan altijd om de lengte van beide getMakerAmount
en getTakerAmount
parameters tegelijkertijd zodat de implementatie het gedrag versterkt dat wordt beschreven door de inline-documentatie.
update: Vast in trekverzoek # 79.
[L14] Beëindigde Chainlink-aanroepen gebruiken
De ChainlinkCalculator
contract is bedoeld om te worden gebruikt om Chainlink-orakels te doorzoeken. Het doet dit door te bellen naar hun latestTimestamp
en latestAnswer
methoden, die beide zijn afgekeurd. In feite zijn de methoden niet langer aanwezig in de API van Chainlink-aggregators vanaf versie drie.
Om mogelijke toekomstige onverenigbaarheden met Chainlink-orakels te voorkomen, kunt u overwegen de latestRoundData
methode in plaats daarvan.
update: Vast in trekverzoek # 67.
Opmerkingen en aanvullende informatie
[N01] Geen interfaces importeren
De AggregatorInterface
interface lijkt een subset van code te zijn die is gekopieerd van ChainLink
's openbare coderepository. De volledige interface is inbegrepen in: ChainLink
's contract npm pakket.
Indien mogelijk, om de kans op interface-mismatches en resulterende problemen te verminderen, in plaats van de interfaces van een ander project opnieuw te definiëren en/of te herschrijven, kunt u overwegen om in plaats daarvan interfaces te gebruiken die via hun officiële npm-pakketten zijn geïnstalleerd.
update: Vast in trekverzoek # 66.
[N02] Verouderde projectafhankelijkheden
Tijdens de installatie van de afhankelijkheden van het project, NPM waarschuwt dat een van de geïnstalleerde pakketten, Highlight
, "zal in de toekomst niet langer worden ondersteund of beveiligingsupdates ontvangen".
Hoewel het onwaarschijnlijk is dat dit pakket een veiligheidsrisico kan veroorzaken, kunt u overwegen de afhankelijkheid die dit pakket gebruikt te upgraden naar een onderhouden versie.
update: Vast in trekverzoek # 64.
[N03] Externe oproepen om methoden te bekijken zijn geen statische oproepen
In het grootste deel van de codebase maakt het protocol expliciet externe oproepen via OpenZeppelin's functionStaticCall
methode om de mogelijkheid van toestandsveranderingen te beperken wanneer deze niet worden verwacht of niet wenselijk zijn. Echter, in de ChainlinkCalculator
contract, ondanks de bedoeling om alleen extern te bellen naar: view
methoden op Chainlink-orakels, de externe oproepen in de singlePrice
en doublePrice
functies worden niet gemaakt via expliciet staticcall
s.
Hoewel we hier geen onmiddellijke beveiligingsproblemen hebben vastgesteld, kunt u overwegen om het aanvalsoppervlak te verkleinen, de consistentie te verbeteren en de bedoeling te verduidelijken. staticcall
s, voor alle externe oproepen naar view
functies in de ChainlinkCalculator
contract.
update: Niet gemaakt. Het 1inch-team stelt:
We denken dat syntaxiscomplicatie verbeteringen in consistentie teniet doet.
[N04] Niet vroegtijdig falen met ongeldige bestellingen
In het OrderMixin
contract, het fillOrderTo
functie behandelt de speciale voorwaarde wanneer een bestelling niet eerder is ingediend (remainingMakerAmount == 0
), maar het behandelt niet expliciet de voorwaarde wanneer de bestelling niet langer geldig is (remainingMakerAmount == 1
).
In het laatste scenario zal de functie uiteindelijk terugkeren, maar alleen na het verbranden van niet-triviale hoeveelheden gas. Om de bedoeling te verduidelijken, de leesbaarheid te vergroten en het gasverbruik te verminderen, kunt u overwegen om het scenario met ongeldige volgorde expliciet aan het begin van de functie af te handelen.
update: Vast in trekverzoek # 68.
[N05] Helpercontracten niet gemarkeerd als abstract
In Solidity, het sleutelwoord abstract
wordt gebruikt voor contracten die op zichzelf geen functionele contracten zijn, of niet bedoeld zijn om als zodanig te worden gebruikt. In plaats van, abstract
contracten worden overgenomen door andere contracten in het systeem om stand-alone functionele contracten te creëren.
Door de hele codebase zijn er voorbeelden van helpercontracten die niet als abstract zijn gemarkeerd, ondanks het feit dat ze niet bedoeld zijn om op zichzelf te worden ingezet. Bijvoorbeeld de AmountCalculator
, ChainlinkCalculator
, ImmutableOwner
, NonceManager
en PredicateHelper
contracten bestaan allemaal uit een basisset van functies die bedoeld zijn om te worden gebruikt door contracten over te nemen.
Overweeg om hulpcontracten te markeren als: abstract
om duidelijk aan te geven dat ze uitsluitend zijn ontworpen om functionaliteit toe te voegen aan contracten die ze erven.
update: Niet gemaakt. Het 1inch-team stelt:
Die helpers kunnen afzonderlijk worden ingezet. Ze worden alleen geërfd voor gasbesparing.
[N06] Inconsistente functievolgorde
Door de hele codebase volgt de functievolgorde over het algemeen de aanbevolen volgorde in de Solidity Style GuideDie: constructor
, fallback
, external
, public
, internal
, private
.
Echter, in de OrderMixin
contract, het public
checkPredicate
functie wijkt af van de stijlgids en snijdt de external
functies.
Om de algehele leesbaarheid van het project te verbeteren, kunt u overwegen de functievolgorde in de hele codebase te standaardiseren, zoals aanbevolen door de Solidity Style Guide.
update: Vast in trekverzoek # 69.
[N07] Inconsistente ordervulstroom
De OrderMixin
en RFQOrderMixin
contracten behandelen beide het vullen van ondertekende bestellingen, maar de algemene orderstroom tussen de twee contracten is inconsistent.
OrderMixin
's fillOrderTo
functie volgt deze algemene stroom (pseudo-code):
if ((takingAmount == 0) == (makingAmount == 0))
else if (takingAmount == 0)
else (handle makingAmount == 0) THEN swapTokens
Terwijl RFQOrderMixin
is analoog fillOrderRFQTo
functie volgt deze stroom (pseudo-code):
if (takingAmount == 0 && makingAmount == 0)
else if (takingAmount == 0)
else if (makingAmount == 0)
else revert THEN swapTokens
Er zijn geen inzichten uit de documentatie waarom de eerste voorwaardelijke in elk van deze twee functies verschilt, of waarom takingAmount
en makingAmount
kunnen niet allebei nul zijn in de laatste functie. Ook het geval waarin zowel a makingAmount
en takingAmount
worden verstrekt, is veel gemakkelijker te redeneren in de fillOrderRFQTo
functie, aangezien het duidelijk wordt behandeld in de finale else
blok.
Om de bedoeling te verduidelijken en de algehele leesbaarheid van de code te vergroten, kunt u overwegen ofwel de algemene orderstroom over deze twee contracten te standaardiseren, ofwel expliciet te documenteren waarom de verschillen bestaan.
update: Niet gemaakt. Het 1inch-team stelt:
Dit komt door aangepaste prijsfuncties in limietorders. Sinds
getMakerAmount
kan mogelijk aanzienlijk verschillen vangetTakerAmount
, dachten we dat het beter is om geen standaardoptie voor de nemer te maken, omdat dit hen waarschijnlijk in verwarring zal brengen in gevallen waarin die getters anders zullen zijn.
[N08] Foutmeldingen hebben een inconsistente indeling of zijn misleidend
Door de hele codebase, de require
en revert
foutmeldingen, die bedoeld zijn om gebruikers op de hoogte te stellen van de specifieke omstandigheden waardoor een transactie mislukt, bleken inconsistent geformatteerd te zijn.
Bijvoorbeeld, elk van de foutmeldingen op regels 85 van OrderMixin.sol
, 16 van ERC721ProxySafe.sol
en 26 van Permitable.sol
een andere stijl hanteren.
Bovendien zijn sommige foutmeldingen misleidend:
Foutmeldingen zijn bedoeld om gebruikers op de hoogte te stellen van falende omstandigheden, dus ze moeten voldoende informatie bevatten zodat passende correcties kunnen worden aangebracht om met het systeem te communiceren. Niet-informatieve foutmeldingen beschadigen de algehele gebruikerservaring enorm, waardoor de kwaliteit van het systeem afneemt. Bovendien kunnen inconsistent opgemaakte foutmeldingen voor onnodige verwarring zorgen. Overweeg daarom om de volledige codebase te bekijken om er zeker van te zijn dat elke require
en revert
verklaring heeft een foutmelding die consistent is opgemaakt, nauwkeurig, informatief en gebruiksvriendelijk is.
update: Gedeeltelijk gefixeerd trekverzoek # 81.
[N09] Inconsistent gebruik van benoemde retourvariabelen
Er is een inconsistent gebruik van benoemde retourvariabelen in de OrderMixin
contract. Sommige functies benoemde variabelen retourneren, anderen expliciete waarden retourneren, en anderen declareer een benoemde retourvariabele maar overschrijf deze met een expliciete retourverklaring.
Overweeg een consistente benadering te hanteren voor het retourneren van waarden in de hele codebase door alle benoemde retourvariabelen te verwijderen, ze expliciet als lokale variabelen te declareren en waar nodig de benodigde retourinstructies toe te voegen. Dit zou zowel de explicietheid als de leesbaarheid van de code verbeteren, en het kan ook helpen bij het verminderen van regressies tijdens toekomstige code-refactoren.
update: Vast in trekverzoek # 73.
[N10] De hash-berekening van de bestelling staat niet open voor de API
De external
functies remaining
, remainingRaw
en remainingsRaw
verwachten allemaal een orderhash voor een succesvolle operatie.
Echter, de helperfunctie _hash
, die de hash van een bestelling retourneert, heeft private
zichtbaarheid. Dit betekent dat gebruikers delen van de bestellingen en domeinstrings handmatig moeten inpakken om de hash van een bestelling te verkrijgen.
Om de kans op fouten bij het berekenen van orderhashes te voorkomen en gebruikers een methode te bieden voor het genereren van de respectieve hash van een order, kunt u overwegen de zichtbaarheid van de _hash
functie naar public
en de naam refactoring to hash
consistent zijn met de rest van de code.
update: Vast in trekverzoek # 74.
[N11] Semantische overbelasting van mapping
De _remaining
in kaart brengen in de OrderMixin
contract semantisch overbelast is om de status van bestellingen en de resterende hoeveelheid activa die beschikbaar zijn voor die bestellingen te volgen.
De drie staten die het kan aannemen zijn:
0
: De orderhash is nog niet gezien.1
: De bestelling is geannuleerd of volledig uitgevoerd.- Elke
uint
groter dan1
: De overigemakerAmount
beschikbaar om te worden gevuld op de bestelling plus1
.
Deze semantische overbelasting vereist het in- en uitpakken van deze waarde tijdens lookup
, cancellation
, initialization
en storage
.
Semantische overbelasting en de noodzakelijke logica om het mogelijk te maken, kan foutgevoelig zijn en kan de codebase moeilijker te begrijpen en te redeneren maken, het kan ook de deur openen voor regressies in toekomstige updates van de code.
Om de leesbaarheid van de code te verbeteren, kunt u overwegen de voltooiingsstatus van bestellingen in een afzonderlijke toewijzing bij te houden.
update: Niet gemaakt. Het 1inch-team zei dat een oplossing de gaskosten zou verhogen voor de fillOrder
functie.
[N12] Orders met vergunning maken oproepen naar willekeurige contracten mogelijk
De OrderMixin
contract erft de Permitable
contract om het vullen van orders met één transactie mogelijk te maken met activa die dergelijke accepteren permit
oproepen om toeslagen te wijzigen.
De belt naar de Permitable
contract valideer niet of het doelwit een toelaatbaar activum is, noch of het zelfs een activum is, waardoor een kwaadwillende gebruiker het adres van een willekeurig contract zou kunnen doorgeven dat een andere oproep zou kunnen uitvoeren voordat de ordervulling is voltooid.
Hoewel het contract is beschermd tegen herintreding, het verminderen van het aanvalsoppervlak en het voorkomen van het afroepen van externe contracten tijdens de uitvoering wordt altijd aanbevolen. Overweeg om de activa die betrokken zijn bij de vergunning te beperken tot de activa die bij de bestelling zijn betrokken, of tot een witte lijst met activa voor het protocol.
update: Niet gemaakt. Het 1inch-team stelt:
OrderMixin
heeft eigenlijk geen informatie over daadwerkelijke tokens alsmakerAsset
entakerAsset
soms zijn proxy's of andere tussenliggende contracten en wordt informatie over werkelijke tokens opgeslagen in willekeurige bytes. Er is dus geen haalbare manier om te beperken welke activapermit
wordt aangeroepen.
[N13] solhint
wordt nooit opnieuw ingeschakeld
In de hele codebase zijn er een paar: solhint-disable
verklaringen, met name die online 23 en online 41 of RevertReasonParser.sol
, die niet eindigen met solhint-enable
.
Ten gunste van explicietheid en om zo restrictief mogelijk te zijn bij het uitschakelen solhint
, overweeg om te gebruiken solhint-disable-line
or solhint-disable-next-line
in plaats daarvan, vergelijkbaar met lijn 16 van hetzelfde bestand.
update: Vast in trekverzoek # 72.
[N14] Typefouten
De codebase bevat de volgende typefouten:
Daarnaast is het project README
(buiten scope voor deze audit) bevat de volgende typefouten:
Overweeg deze typefouten te corrigeren om de leesbaarheid van de code te verbeteren.
update: Vast in trekverzoek # 71 en trekverzoek # 77.
[N15] Gebruik van uint
in plaats van uint256
Om explicietheid te bevorderen, zijn alle gevallen van uint
moet worden gedeclareerd als uint256
. Vooral die in de for
lussen op lijnen 98 en 119 of OrderMixin.sol
en lijnen 16 en 30 of PredicateHelper.sol
.
update: Vast in trekverzoek # 70.
Conclusies
Er zijn 3 zeer ernstige problemen gevonden. Er zijn enkele wijzigingen voorgesteld om de beste praktijken te volgen en het potentiële aanvalsoppervlak te verkleinen.
- &
- 7
- Over
- toegang
- Volgens
- Account
- over
- Handelen
- acties
- Extra
- adres
- Voordeel
- Alles
- Het toestaan
- al
- Hoewel
- hoeveelheden
- analyse
- api
- nadering
- argumenten
- rond
- aanwinst
- Activa
- controleren
- Back-end
- Begin
- wezen
- BEST
- 'best practices'
- Beetje
- bots
- bouw
- Bellen
- verzorging
- gevallen
- Veroorzaken
- Kettingschakel
- verandering
- controleren
- Controles
- code
- complex
- voorwaarde
- verwarring
- bouw
- bevat
- contract
- contracten
- Correcties
- Kosten
- kon
- Koppel
- schepper
- Valuta
- Actueel
- gegevens
- transactie
- Denial of Service
- het inzetten
- Design
- ontwikkelaars
- Ontwikkeling
- DEED
- verschillen
- anders
- domein
- verdubbelen
- dynamisch
- Vroeg
- rand
- aanmoedigen
- vooral
- ETH
- Event
- EVENTS
- alles
- voorbeeld
- uitwisseling
- verwacht
- ervaring
- Exploiteren
- Voordelen
- Velden
- Tot slot
- Voornaam*
- Bepalen
- Flexibiliteit
- stroom
- volgen
- gevonden
- vol
- functie
- fondsen
- toekomst
- spel
- GAS
- Algemeen
- Vrijgevigheid
- groot
- gids
- Behandeling
- hachee
- hashing
- met
- hulp
- Hoge
- zeer
- Hoe
- HTTPS
- Hybride
- identificeren
- Impact
- uitvoeren
- belangrijk
- importeren
- inclusief
- Inclusief
- Laat uw omzet
- meer
- info
- informatie
- Infrastructuur
- inzichten
- aandachtig
- belang
- Interface
- betrokken zijn
- problemen
- IT
- Groot
- groter
- leiden
- leidend
- Bibliotheek
- Beperkt
- Lijn
- Liquiditeit
- opgesomd
- lijsten
- lokaal
- keek
- lookup
- groot
- maker
- maken
- Markt
- mempool
- spiegel
- model
- meest
- Meest populair
- namelijk
- Nieuwe mogelijkheden
- non-fungibel
- niet-fungible tokens
- officieel
- open
- Operations
- Keuze
- orakel
- bestellen
- orders
- Overige
- eigenaar
- Patronen
- Populair
- presenteren
- het voorkomen van
- prijs
- prijsstelling
- privaat
- project
- projecten
- bescherming
- protocol
- zorgen voor
- biedt
- volmacht
- publiek
- publiceren
- inkomsten
- kwaliteit
- verhogen
- Realiteit
- verminderen
- vertrouwen
- verslag
- Rapporten
- bewaarplaats
- Voorwaarden
- REST
- Resultaten
- Retourneren
- beoordelen
- Risico
- rondes
- lopen
- sdk
- veiligheid
- Diensten
- reeks
- gedeeld
- Aandelen
- verschuiving
- gelijk
- Eenvoudig
- Klein
- slim
- Slimme contracten
- So
- stevigheid
- spam
- specifiek
- Uitgaven
- Spot
- begin
- Land
- Statement
- Staten
- Status
- mediaopslag
- stijl
- ingediend
- succes
- geslaagd
- Met goed gevolg
- ondersteuning
- ondersteunde
- Oppervlak
- Stap over voor slechts
- system
- doelwit
- proef
- Testen
- testen
- Door
- overal
- niet de tijd of
- samen
- teken
- tokens
- spoor
- Tracking
- transactie
- Trust
- unieke
- updates
- us
- bruikbaarheid
- USD
- USDT
- gebruikers
- utility
- waarde
- Bekijk
- zichtbaarheid
- wachten
- Wat
- whitelist
- binnen
- zonder
- Mijn werk
- waard
- nul