6. December, 2021
Introduktion
Tally team bad os om at gennemgå og revidere et sæt kontrakter med det endelige mål at forbedre fælles styringskontrakter og give dem mere fleksibilitet. Vi kiggede på koden og offentliggør nu vores resultater.
Systemoversigt
Systemet bygger på tre hovedkontrakter:
- A
SafeGuard
kontrakt skabelon. Denne kontrakt eradmin
af enTimelock
kontrakt, og tilføjer et lag af modulære roller overTimelock
s handlinger. Denne kontrakt definerer flere roller, der har separat ansvar og adgang til et forslags tilstand (kø, annullering og eksekvering). - A
SafeGuardFactory
kontrakt indsætter en nySafeGuard
og en tilsvarende nyTimelock
kontrakt. Derefter indstiller den timelock-adressen inde iSafeGuard
kontrakt og registrerer den nyeSafeGuard
ind iRegistry
. - A
Registry
som har en liste over udsendteSafeGuards
med deres tilsvarende versionsnummer.
SafeGuardFactory
vil dybest set afføde en ny SafeGuard
når det kaldes. EN SafeGuard
er en indpakning Timelock
operationer der tilføjer forskellige og adskilte roller for hver handling. Roller er struktureret ved brug af OpenZeppelin's AccessControlEnumerable
kontrakt, der giver mere fleksibilitet og kompatibilitet til multisig-tegnebøgerne og business use cases, hvor flere adresser kan have den samme delte rolle.
Update: I PR #10, har Tally-teamet besluttet at fjerne Registry
kontrakt. Listen over indsatte sikkerhedsforanstaltninger er nu gemt i SafeGuardFactory
kontrakt, i safeGuards
talrige sæt, sammen med deres version gemt i safeGuardVersion
kortlægning.
roller
SafeGuard
kontrakten definerer følgende roller:
Update: CREATOR_ROLE
rolle er blevet fjernet som en løsning på problemet L02.
Anvendelsesområde
Vi reviderede forpligtelse b2c63a9dfc4090be13320d999e7c6c1d842625d3
af safeguard
depot. I omfang er de smarte kontrakter i contracts
vejviser. Imidlertid mocks
bibliotek blev anset for at være uden for rækkevidde.
Forudsætninger
Systemet er ikke beregnet til at kunne opgraderes. Det Registry
adresse er indstillet i konstruktøren af SafeGuardFactory
og hver SafeGuard
kan have Timelock
indstilles kun én gang. Det betyder, at hvis en ny Registry
er indsat en ny SafeGuardFactory
skal også sættes ind. Hvis Timelock
implementeringsændringer, det gamle SafeGuard
s vil blive forældede, og nye skal implementeres.
Desuden er systemet stærkt afhængig af implementeringen af en Timelock
kontrakt som blev vurderet uden for denne revisions omfang. Teamet har endnu ikke afsluttet implementeringen af Timelock
kontrakt. På tidspunktet for skrivningen af denne rapport, forbindelsens implementering af timelock bliver brugt i projektet.
Kodebasen er blevet revideret af to revisorer i løbet af en uge, og her præsenterer vi vores resultater.
Kritisk sværhedsgrad
Ingen.
Høj sværhedsgrad
[H01] ETH kan låses inde i Timelock
kontrakt
Tally
teamet oprindeligt baseret deres implementeringer på jorden af GovernorBravoDelegate
Sammensat kontrakt.
I løbet af denne revision har Tally
team opdaget en begrænsning i Compounds guvernør, hvor ETH sendte direkte til Timelock
er ikke tilgængelig til brug for styringsforslag, og selvom det ikke sidder fast permanent, kræver det en omfattende løsning, der skal hentes.
Dette skyldes, at guvernørens implementering kræver, at al værdien af et forslag vedlægges som msg.value
af den konto, der udløser eksekveringen, uden på nogen måde at bruge Timelock
ETH midler.
samme problem blev senere identificeret i SafeGuard
implementering og teamet er klar over problemet, og det er i gang med at løse det.
Overvej at bruge fremgangsmåden, mens du løser problemet vedtaget af OpenZeppelin-biblioteket til samme nummer.
Update: Rettet i commit 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory kan fryses
Registry
kontrakt er beregnet til at holde styr på alle SafeGuards
at SafeGuardFactory
producerer. Det har det ydre register
funktion som bruges til dette formål.
På samme tid, den SafeGuardFactory
har i sin konstruktør tildeling af det lokale registry
værdi til inputværdien. Der er ingen mulighed for at ændre værdien af registry
variabel og af denne grund, hvis en ny Registry
bliver indsat, skal en ny fabrik også indsættes.
SafeGuardFactory
har createSafeGuard
funktion, ansvarlig for først indsætte en ny SafeGuard
, derefter En ny Timelock
med adressen på SafeGuard
as admin
, derefter indstilles timelock
variabel af SafeGuard
kontrakt og til sidst registrering af SafeGuard
i registret.
Problemet er, at ethvert opkald til createSafeGuard
kan blive tvunget til at fejle af en angriber, der direkte kan registrere den nyes deterministiske adresse SafeGuard
før dens oprettelse. Når en kontrakt skaber en new
instans, dens nonce øges, og adressen på, hvor den nye forekomst af kontrakten vil blive implementeret, kan bestemmes af den oprindelige kontraktadresse og dens nonce. Derfor kan en angriber forudberegne mange af de adresser, hvor den nye SafeGuards
vil blive indsat og registrere disse adresser i Registry
ved at ringe til register
fungere. Dette ville resultere i opkald til createSafeGuard
til tilbage eftersom Registry
indeholder allerede adressen.
For at undgå at eksterne aktører ringer offentligt til Register
kontrakt, overveje at begrænse adgangen til register
funktion til at acceptere opkald udelukkende af SafeGuardFactory
.
Update: Rettet ind PR #10. Tally-teamet har fjernet Registry
kontrakt.
Medium sværhedsgrad
Ingen.
Lav sværhedsgrad
[L01] Kommenteret kode
Registry
kontrakten omfatter en kommenteret kodelinje. For at forbedre læsbarheden bør du overveje at fjerne den fra kodebasen.
Update: Rettet ind PR #10 og forpligte sig 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] SafeGuards administrator kan tildele rollen som skaber til enhver adresse
SafeGuard
kontrakt definerer rollen som en CREATOR_ROLE
som, som navnet antyder, er tildelt skaberen af sikringen.
Dog ved at påberåbe sig og grantRole
funktion af AccessControlEnumerable
kontrakt i OpenZeppelin kontraktbiblioteket kan en administrator tildele denne rolle til enhver adresse. Dette kan forårsage forvirring, fordi skaberen af SafeGuard
kan kun være SafeGuardFactory
.
Gennem hele kodebasen er denne rolle kun blevet brugt til at begrænse brugere i at interagere med setTimelock
funktion af SafeGuard
kontrakt. Det sikrer systemet ved design setTimelock
funktion kan kun kaldes én gangfra i SafeGuardFactory
kontrakt.
Overvej at fjerne CREATOR_ROLE
rolle fra SafeGuard
kontrakt og brug af onlyOwner
modifier i setTimelock
funktion.
Update: Rettet ind PR #10.
[L03] Forkert grænsefladedefinition og -implementering
ISafeGuard
grænsefladen definerer ikke queueTransactionWithDescription
funktion implementeret i SafeGuard
kontrakt, og samtidig definerer den __abdicate, __queueSetTimelockPendingAdmin og __executeSetTimelockPendingAdmin funktioner, men de er ikke implementeret.
For at forbedre korrektheden og konsistensen i kodebasen kan du overveje at omfaktorere ISafeGuard
interface til at matche nøjagtigt SafeGuard
implementering.
Update: Rettet i commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Manglende docstrings
Nogle af kontrakterne og funktionerne i kodebasen mangler dokumentation. For eksempel, nogle funktioner i SafeGuard
kontrakt.
Derudover bruger nogle docstrings uformelt sprog, såsom det ene over setTimelock
funktion i SafeGuard
kontrakt.
Dette hindrer anmeldernes forståelse af kodens hensigt, hvilket er grundlæggende for korrekt at vurdere ikke kun sikkerhed, men også rigtighed. Derudover forbedrer docstrings læsbarheden og letter vedligeholdelsen. De skal eksplicit forklare formålet med eller hensigten med funktionerne, de scenarier, hvorunder de kan fejle, de roller, der er tilladt at kalde dem, de returnerede værdier og de udsendte hændelser.
Overvej grundigt at dokumentere alle funktioner (og deres parametre), der er en del af kontrakternes offentlige API. Funktioner, der implementerer følsom funktionalitet, selvom de ikke er offentlige, bør også være klart dokumenteret. Når du skriver docstrings, skal du overveje at følge Ethereum Natural Specification Format (NatSpec).
Update: Delvist fastgjort PR #10. Korrekte docstrings er blevet tilføjet til forskellige funktioner i hele kodebasen. Ud over de nuværende ændringer kan du dog overveje at foretage følgende ændringer:
- Tilføj
description
som@param
i docstringen ovenforqueueTransactionWithDescription
funktion - Tilføj
@param
i dokstring overcreateSafeGuard
funktion iSafeGuardFactory
kontrakt - Tilføj
@return
i docstrings over funktionerne iSafeGuardFactory
kontrakt.
[L05] Ubrugelig eller gentaget kode
Der er steder i kodebasen, hvor kode enten gentages eller ikke er nødvendig. Nogle eksempler er:
- Linjer 29-32 af
Registry
kontrakt er ubrugelige, fordi_add
funktion afEnumerableSet
kontrakt udfører allerede disse kontroller mod de værdier, der allerede er fastsat. - Linje 62, 67, 73 , 78 af
SafeGuard
kontrakten gentager alle den samme nøjagtige operation. Overvej at indkapsle det i en intern funktion for at undgå duplikering af kode. - Linjer 62-63 , 67-68 of
SafeGuard
gentages. Overvej at indkapsle dem i en enkelt intern funktion. - Brugen af
gasleft
for at angive, hvor meget gas der skal videresendes i opkaldet af funktionenexecuteTransaction
er unødvendigt. Dette skyldes, at på det tidspunkt af henrettelsen vil hele den resterende gas blive brugt til at fortsætte henrettelsen. Hvis dette ikke er for eksplicit, overvej at fjernegas
parameter fra opkaldet.
Overvej at anvende den foreslåede fix for at producere en renere kode og forbedre konsistens og modularitet over kodebasen.
Update: Rettet ind PR #10 og forpligte sig 7fd27df16fc879d990d36a167a0b6e719e578558
.
Bemærkninger og yderligere oplysninger
[N01] Inkonsekvent stil
Der er nogle steder i kodebasen, hvor forskelle i stil påvirker læsbarheden, hvilket gør det sværere at forstå koden. Nogle eksempler er:
-
Registry
kontrakt bruger forskellige stilarter til docstrings i hele kontrakten. -
SafeGuard
kontrakten udsender en begivenhed, nårqueueTransactionWithDescription
kaldes, men der udsendes ingen hændelser i andre funktioner, der beskæftiger sig med transaktioner. - I
SafeGuard
kontrakt nogle gange værdi bruges som navngivet parameter og nogle gange _værdi anvendes.
Under hensyntagen til den værdi, en konsekvent kodningsstil tilføjer til projektets læsbarhed, kan du overveje at håndhæve en standard kodningsstil ved hjælp af linter-værktøjer, såsom Solhint.
Update: Rettet ind PR #10 og forpligte sig 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Manglende licens
Følgende kontrakter i kodebasen mangler et SPDX-licens-id.
For at dæmpe kompileringsadvarsler og øge konsistensen på tværs af kodebasen, bør du overveje at tilføje en licens-id. Mens du gør det overveje at henvise til spdx.dev retningslinjer.
Update: Rettet ind PR #10 og forpligte sig 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] OpenZeppelin Contracts afhængighed er ikke fastgjort
For at forhindre uventet adfærd i tilfælde af at brydende ændringer frigives i fremtidige opdateringer af OpenZeppelin Contracts' bibliotek, overvej at fastgøre versionen af denne afhængighed i pakke.json fil.
Update: Rettet ind PR #10.
[N04] Solidity-kompilerversionen er ikke fastgjort
Overvej i hele kodebasen at fastgøre versionen af Solidity compiler til den seneste stabile version. Dette skulle hjælpe med at forhindre introduktion af uventede fejl på grund af inkompatible fremtidige udgivelser. For at vælge en specifik version, bør udviklere overveje både compilerens funktioner, der er nødvendige for projektet og listen over kendte fejl tilknyttet hver Solidity-kompilerversion.
Update: Rettet ind PR #10.
[N05] Tastefejl
I forskellige tilfælde i hele kodebasen, ordet role
er stavet forkert som rol
. Et sådant eksempel er i docstringen inden for constructor
af SafeGuard
kontrakt.
Overvej at rette disse tastefejl for at forbedre kodelæsbarheden.
Update: Delvist fastgjort PR #10. Mens stavningen af role
er blevet rettet, skal kommentaren "indstil administratorrolle til en defineret administratoradresse" være "indstil administratorrolle til en defineret administratoradresse". Derudover er "execute" stavet forkert i SafeGuard
kontrakt på line 69, line 82, line 96 , line 110 og "tilgængelig" er stavet forkert line 70, line 83, line 97, line 111. Overvej også at erstatte uformelle ord som f.eks "vil" in SafeGuard
kontrakt med formelle alternativer såsom "at gå til".
[N06] Erklær uint som uint256
Der er flere forekomster i kodebasen, hvor variabler er deklareret uint
datatype i stedet for uint256
. F.eks eta
variabel i QueueTransactionWithDescription
begivenhed af SafeGuard
kontrakt.
For at favorisere eksplicithed, alle tilfælde af uint
skal erklæres som uint256
.
Update: Rettet ind PR #10 og forpligte sig 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Ubrugt import
SafeGuard
kontrakt importerer console
kontrakt, men bruger den aldrig.
For at forbedre kodens læsbarhed bør du overveje at fjerne enhver ubrugt import.
Update: Rettet ind PR #10.
konklusioner
En høj og flere andre mindre sårbarheder er blevet fundet, og anbefalinger og rettelser er blevet foreslået.
- &
- adgang
- Konto
- tværs
- Handling
- aktioner
- Yderligere
- adresse
- admin
- Alle
- allerede
- Skønt
- api
- tilgang
- omkring
- revision
- I bund og grund
- være
- bugs
- virksomhed
- ringe
- tilfælde
- Årsag
- lave om
- afgift
- kode
- Kodning
- Fælles
- Forbindelse
- forvirring
- overvejelse
- indeholder
- fortsæt
- kontrakt
- kontrakter
- kunne
- skaberen
- Nuværende
- data
- beskæftiger
- Design
- udviklere
- forskellige
- opdaget
- Uddybe
- ETH
- begivenhed
- begivenheder
- eksempel
- fabrik
- Funktionalitet
- Endelig
- Fornavn
- Fix
- Fleksibilitet
- fundet
- funktion
- fonde
- fremtiden
- GAS
- Give
- mål
- regeringsførelse
- Guvernør
- retningslinjer
- have
- hjælpe
- link.
- Høj
- Hvordan
- HTTPS
- implementeret
- Forøg
- øget
- grænseflade
- IT
- kendt
- Sprog
- seneste
- Bibliotek
- Licens
- Line (linje)
- Liste
- lokale
- låst
- kiggede
- Making
- Match
- modulær
- multitegn
- Andet
- præsentere
- behandle
- projekt
- forslag
- offentlige
- offentliggøre
- register
- Udgivelser
- indberette
- Repository
- Resultater
- gennemgå
- sikkerhed
- sæt
- indstilling
- delt
- Smart
- Smarte kontrakter
- soliditet
- Tilstand
- stil
- systemet
- Gennem
- hele
- tid
- værktøjer
- spor
- Transaktioner
- opdateringer
- us
- brugere
- værdi
- Sårbarheder
- Punge
- uge
- WHO
- inden for
- ord
- skrivning