December 6, 2021
Beskrivning
Smakämnen Tally teamet bad oss att granska och granska en uppsättning kontrakt med det slutliga målet att förbättra gemensamma förvaltningskontrakt och ge dem mer flexibilitet. Vi tittade på koden och publicerar nu våra resultat.
System översikt
Systemet bygger på tre huvudkontrakt:
- A
SafeGuard
kontrakt mall. Detta kontrakt äradmin
av enTimelock
kontrakt och lägger till ett lager av modulära roller överTimelock
s handlingar. Detta kontrakt definierar flera roller som har separat ansvar och åtkomst över tillståndet för ett förslag (kö, annullering och utförande). - A
SafeGuardFactory
kontrakt distribuerar en nySafeGuard
och en motsvarande nyTimelock
avtal. Sedan ställer den in tidslåsadressen inutiSafeGuard
kontrakt och registrerar det nyaSafeGuard
iRegistry
. - A
Registry
som innehåller en lista över utplaceradeSafeGuards
med deras motsvarande versionsnummer.
Smakämnen SafeGuardFactory
kommer i princip att skapa en ny SafeGuard
närhelst det kallas. A SafeGuard
är en sjal runt Timelock
verksamhet som lägger till olika och separerade roller för varje åtgärd. Roller struktureras genom användning av OpenZeppelin's AccessControlEnumerable
kontrakt som ger mer flexibilitet och kompatibilitet till multisig-plånböckerna och affärsanvändningsfall där flera adresser kan ha samma delade roll.
Uppdatering: I PR #10, har Tally-teamet beslutat att ta bort Registry
avtal. Listan över utplacerade skyddsåtgärder är nu lagrad i SafeGuardFactory
kontrakt, i safeGuards
enumerable set, tillsammans med deras version lagrad i safeGuardVersion
kartläggning.
roller
Smakämnen SafeGuard
kontraktet definierar följande roller:
Uppdatering: Smakämnen CREATOR_ROLE
roll har tagits bort som en lösning för problemet L02.
Omfattning
Vi granskade åtagandet b2c63a9dfc4090be13320d999e7c6c1d842625d3
av safeguard
förvaret. I omfattning är de smarta kontrakten i contracts
katalog. Men den mocks
katalogen ansågs vara utanför räckvidden.
antaganden
Systemet är inte tänkt att kunna uppgraderas. De Registry
adressen är inställd i konstruktören av SafeGuardFactory
och var och en SafeGuard
kan ha Timelock
ställs endast in en gång. Detta innebär att om en ny Registry
är utplacerad en ny SafeGuardFactory
måste sättas in också. Om Timelock
genomförandeförändringar, det gamla SafeGuard
s kommer att bli föråldrade och nya kommer att behöva distribueras.
Dessutom är systemet starkt beroende av implementeringen av en Timelock
kontrakt som ansågs utanför räckvidden för denna revision. Teamet har ännu inte slutfört genomförandet av Timelock
avtal. När denna rapport skrevs, föreningens implementering av timelock används i projektet.
Kodbasen har granskats av två revisorer under loppet av en vecka och här presenterar vi våra resultat.
Kritisk svårighetsgrad
Inga.
Hög svårighetsgrad
[H01] ETH kan låsas inuti Timelock
kontrakt
Smakämnen Tally
teamet baserade ursprungligen sina implementeringar på grund av GovernorBravoDelegate
Sammansatt kontrakt.
Under denna granskning har Tally
team upptäckt en begränsning i Compounds guvernör där ETH skickade direkt till Timelock
är inte tillgänglig för användning av förvaltningsförslag, och även om den inte har fastnat permanent, kräver det en utarbetad lösning för att hämtas.
Detta beror på att guvernörens genomförande kräver att hela värdet av ett förslag bifogas som msg.value
av kontot som utlöser exekveringen, utan att på något sätt använda Timelock
ETH-fonder.
Smakämnen samma problem identifierades senare i SafeGuard
genomförande och teamet är medvetet om problemet och håller på att åtgärda det.
Överväg att använda metoden när du löser problemet antogs av OpenZeppelin-biblioteket för samma nummer.
Uppdatering: Fixat i commit 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory kan frysas
Smakämnen Registry
kontrakt är avsett att hålla reda på alla SafeGuards
att SafeGuardFactory
producerar. Den har det yttre register
fungera som används för detta ändamål.
Samtidigt SafeGuardFactory
har i sin konstruktör tilldelning av den lokala registry
värde till ingångsvärdet. Det finns ingen möjlighet att ändra värdet på registry
variabel och av denna anledning, om en ny Registry
utplaceras måste en ny fabrik också sättas i drift.
Smakämnen SafeGuardFactory
har de createSafeGuard
fungera, ansvarig för först distribuera en ny SafeGuard
och sedan en ny Timelock
med adressen till SafeGuard
as admin
, ställ sedan in timelock
variabel av SafeGuard
kontrakt och slutligen registrerar SafeGuard
i registret.
Problemet är att alla samtal till createSafeGuard
kan tvingas misslyckas av en angripare som direkt kan registrera den deterministiska adressen för den nya SafeGuard
innan den skapades. Närhelst ett kontrakt skapar en new
exempel, dess nonce ökas, och adressen till var den nya instansen av kontraktet skulle distribueras kan bestämmas av den ursprungliga kontraktsadressen och dess nonce. Därför kan en angripare förberäkna många av adresserna där den nya SafeGuards
kommer att distribueras och registrera dessa adresser i Registry
genom att ringa register
fungera. Detta skulle resultera i samtal till createSafeGuard
till återgå eftersom Registry
innehåller redan adressen.
För att undvika att externa aktörer ringer offentligt Register
kontrakt, överväg att begränsa tillgången till register
funktion för att acceptera samtal uteslutande av SafeGuardFactory
.
Uppdatering: Fast i PR #10. Tally-teamet har tagit bort Registry
avtal.
Medel svårighetsgrad
Inga.
Låg svårighetsgrad
[L01] Kommenterade ut kod
Smakämnen Registry
kontrakt omfattar en kommenterad kodrad. För att förbättra läsbarheten, överväg att ta bort den från kodbasen.
Uppdatering: Fast i PR #10 och begå 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] SafeGuards administratör kan tilldela rollen som skapare till vilken adress som helst
Smakämnen SafeGuard
kontrakt definierar rollen för en CREATOR_ROLE
som, som namnet antyder, är tilldelad skaparen av skyddet.
Dock genom att åberopa d grantRole
funktion av AccessControlEnumerable
kontrakt i OpenZeppelins kontraktsbibliotek kan en administratör ge denna roll till vilken adress som helst. Detta kan orsaka förvirring eftersom skaparen av SafeGuard
kan bara vara SafeGuardFactory
.
Genom hela kodbasen har denna roll endast använts för att hindra användare från att interagera med setTimelock
fungera av SafeGuard
avtal. Genom design säkerställer systemet det setTimelock
fungera kan endast ringas en gång, från inom SafeGuardFactory
kontrakt.
Överväg att ta bort CREATOR_ROLE
roll från SafeGuard
kontrakt och använda onlyOwner
modifierare i setTimelock
funktion.
Uppdatering: Fast i PR #10.
[L03] Felaktig definition och implementering av gränssnittet
Smakämnen ISafeGuard
gränssnittet definierar inte queueTransactionWithDescription
fungera implementeras i SafeGuard
kontrakt, och samtidigt definierar det __abdicate, __queueSetTimelockPendingAdmin och __executeSetTimelockPendingAdmin funktioner men de är inte implementerade.
För att förbättra korrektheten och konsistensen i kodbasen, överväg att omfaktorera ISafeGuard
gränssnitt för att matcha exakt SafeGuard
genomförande.
Uppdatering: Fixat i commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Saknade docstrings
Vissa av kontrakten och funktionerna i kodbasen saknar dokumentation. Till exempel, några funktioner i SafeGuard
avtal.
Dessutom använder vissa docstrings informellt språk, till exempel det ena ovanför setTimelock
funktion i SafeGuard
kontrakt.
Detta hindrar granskarnas förståelse av kodens avsikt, vilket är grundläggande för att korrekt bedöma inte bara säkerhet utan också korrekthet. Dessutom förbättrar docstrings läsbarheten och underlättar underhållet. De bör uttryckligen förklara syftet eller avsikten med funktionerna, de scenarier under vilka de kan misslyckas, de roller som tillåts anropa dem, de värden som returneras och de händelser som sänds ut.
Överväg att noggrant dokumentera alla funktioner (och deras parametrar) som ingår i kontraktens publika API. Funktioner som implementerar känslig funktionalitet, även om de inte är offentliga, bör också tydligt dokumenteras. När du skriver docstrings, överväg att följa Ethereum Natural Specification Format (NatSpec).
Uppdatering: Delvis fast i PR #10. Korrekt docstrings har lagts till i olika funktioner i hela kodbasen. Men utöver de nuvarande ändringarna kan du överväga att göra följande ändringar:
- Lägg till
description
som@param
i docstringen ovanqueueTransactionWithDescription
fungera - Lägg till
@param
i docstring ovanförcreateSafeGuard
funktion iSafeGuardFactory
kontrakt - Lägg till
@return
i docstrings ovanför funktionerna iSafeGuardFactory
avtal.
[L05] Värdelös eller upprepad kod
Det finns ställen i kodbasen där kod antingen upprepas eller inte behövs. Några exempel är:
- Linjer 29-32 av
Registry
kontrakt är värdelösa, eftersom_add
funktion avEnumerableSet
kontrakt utför redan dessa kontroller mot de värden som redan är satta. - Linjer 62, 67, 73 och 78 av
SafeGuard
kontraktet upprepar alla exakt samma operation. Överväg att kapsla in den i en intern funktion för att undvika duplicering av kod. - Linjer 62-63 och 67-68 of
SafeGuard
upprepas. Överväg att kapsla in dem i en enda intern funktion. - Användningen av
gasleft
för att ange hur mycket gas som ska vidarebefordras i anropet av funktionenexecuteTransaction
är onödigt. Detta beror på att vid den tidpunkten för avrättningen kommer hela gasen som är kvar att användas för att fortsätta avrättningen. Om detta inte är för tydlighetens skull, överväg att ta bortgas
parameter från anropet.
Överväg att tillämpa den föreslagna fixen för att producera en renare kod och förbättra konsistensen och modulariteten över kodbasen.
Uppdatering: Fast i PR #10 och begå 7fd27df16fc879d990d36a167a0b6e719e578558
.
Anteckningar och ytterligare information
[N01] Inkonsekvent stil
Det finns några ställen i kodbasen där stilskillnader påverkar läsbarheten, vilket gör det svårare att förstå koden. Några exempel är:
- Smakämnen
Registry
kontrakt använder olika stilar för docstrings i hela kontraktet. - Smakämnen
SafeGuard
kontraktet avger en händelse närqueueTransactionWithDescription
anropas men inga händelser sänds ut i andra funktioner som hanterar transaktioner. - I
SafeGuard
kontrakt, ibland värde används som namngiven parameter och ibland _värde är använd.
Med tanke på värdet av en konsekvent kodningsstil tillför projektets läsbarhet, överväg att tillämpa en standardkodningsstil med hjälp av linterverktyg, som Solhint.
Uppdatering: Fast i PR #10 och begå 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Saknas licens
Följande kontrakt i kodbasen saknar en SPDX-licensidentifierare.
För att tysta kompilatorvarningar och öka konsistensen över kodbasen överväg att lägga till en licensidentifierare. När du gör det överväga att hänvisa till spdx.dev riktlinjer.
Uppdatering: Fast i PR #10 och begå 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] OpenZeppelin Contracts beroende är inte fäst
För att förhindra oväntade beteenden i fall att brytande ändringar släpps i framtida uppdateringar av OpenZeppelin Contracts bibliotek, överväg att fästa versionen av detta beroende i package.json fil.
Uppdatering: Fast i PR #10.
[N04] Solidity-kompilatorversionen är inte fäst
Överväg att fästa versionen av kodbasen i hela kodbasen Solidity kompilator till sin senaste stabila version. Detta bör hjälpa till att förhindra att oväntade buggar introduceras på grund av inkompatibla framtida utgåvor. För att välja en specifik version bör utvecklare överväga både kompilatorns funktioner som behövs av projektet och listan över kända buggar associerad med varje Solidity-kompilatorversion.
Uppdatering: Fast i PR #10.
[N05] Skrivfel
Vid olika tillfällen i kodbasen, ordet role
är felstavat som rol
. Ett sådant exempel finns i docstringen inom constructor
av SafeGuard
kontrakt.
Överväg att korrigera dessa stavfel för att förbättra kodens läsbarhet.
Uppdatering: Delvis fast i PR #10. Medan stavningen av role
har korrigerats, bör kommentaren "ställ in administratörsroll till en definierad adminadress" vara "ställ in administratörsroll till en definierad adminadress". Dessutom är "execute" felstavat i SafeGuard
kontrakt på linje 69, linje 82, linje 96 och linje 110 och "tillgänglig" är felstavad på linje 70, linje 83, linje 97, linje 111. Överväg också att ersätta informella ord som t.ex "kommer" in SafeGuard
kontrakt med formella alternativ som att "gå till".
[N06] Deklarera uint som uint256
Det finns flera förekomster i kodbasen där variabler deklareras uint
datatyp istället för uint256
. Till exempel, eta
variabel i QueueTransactionWithDescription
händelse av SafeGuard
avtal.
För att gynna uttrycklighet, alla instanser av uint
bör förklaras som uint256
.
Uppdatering: Fast i PR #10 och begå 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Oanvänd import
Smakämnen SafeGuard
kontrakt importerar console
kontrakt men använder det aldrig.
För att förbättra läsbarheten för koden, överväg att ta bort eventuella oanvända importer.
Uppdatering: Fast i PR #10.
Slutsatser
En hög och flera andra mindre sårbarheter har hittats och rekommendationer och korrigeringar har föreslagits.
- &
- tillgång
- Konto
- tvärs
- Handling
- åtgärder
- Annat
- adress
- administration
- Alla
- redan
- Även
- api
- tillvägagångssätt
- runt
- revision
- I grund och botten
- Där vi får lov att vara utan att konstant prestera,
- fel
- företag
- Ring
- fall
- Orsak
- byta
- laddning
- koda
- Kodning
- Gemensam
- Luktämne
- förvirring
- övervägande
- innehåller
- fortsätta
- kontrakt
- kontrakt
- kunde
- skaparen
- Aktuella
- datum
- som handlar om
- Designa
- utvecklare
- olika
- upptäckt
- Utveckla
- ETH
- händelse
- händelser
- exempel
- fabrik
- Funktioner
- Slutligen
- Förnamn
- Fast
- Flexibilitet
- hittade
- fungera
- fonder
- framtida
- GAS
- Ge
- Målet
- styrning
- Guvernör
- riktlinjer
- har
- hjälpa
- här.
- Hög
- Hur ser din drömresa ut
- HTTPS
- genomföras
- Öka
- ökat
- Gränssnitt
- IT
- känd
- språk
- senaste
- Bibliotek
- Licens
- linje
- Lista
- lokal
- låst
- såg
- Framställning
- Match
- modulära
- multisig
- Övriga
- presentera
- process
- projektet
- förslag
- allmän
- publicera
- registrera
- meddelanden
- rapport
- Repository
- Resultat
- översyn
- säkerhet
- in
- inställning
- delas
- smarta
- Smarta kontrakt
- fasthet
- Ange
- stil
- system
- Genom
- hela
- tid
- verktyg
- spår
- Transaktioner
- Uppdateringar
- us
- användare
- värde
- sårbarheter
- Plånböcker
- vecka
- VEM
- inom
- ord
- skrivning