Desember 6, 2021
Introduksjon
De Opptelling teamet ba oss om å gjennomgå og revidere et sett med kontrakter med det endelige målet å forbedre felles styringskontrakter og gi dem mer fleksibilitet. Vi så på koden og publiserer nå resultatene våre.
Systemoversikt
Systemet er avhengig av tre hovedkontrakter:
- A
SafeGuard
kontraktmal. Denne kontrakten eradmin
av enTimelock
kontrakt, og legger til et lag med modulære roller overTimelock
sine handlinger. Denne kontrakten definerer flere roller som har separat ansvar og tilgang over tilstanden til et forslag (kø, kansellering og utførelse). - A
SafeGuardFactory
kontrakt distribuerer en nySafeGuard
og en tilsvarende nyTimelock
kontrakt. Deretter setter den tidslåsadressen inne iSafeGuard
kontrakt og registrerer den nyeSafeGuard
innRegistry
. - A
Registry
som har en liste over utplasserteSafeGuards
med deres tilsvarende versjonsnummer.
De SafeGuardFactory
vil i utgangspunktet gyte en nytt SafeGuard
når det kalles. EN SafeGuard
er en sjal rundt Timelock
operasjoner som legger til forskjellige og atskilte roller for hver handling. Roller er strukturert gjennom bruk av OpenZeppelin's AccessControlEnumerable
kontrakt som gir mer fleksibilitet og kompatibilitet til multisig-lommebøkene og business use cases hvor flere adresser kan ha samme delte rolle.
Oppdatering: på PR # 10, har Tally-teamet bestemt seg for å fjerne Registry
kontrakt. Listen over utplasserte sikkerhetstiltak er nå lagret i SafeGuardFactory
kontrakt, i safeGuards
tallrike sett, sammen med deres versjon lagret i safeGuardVersion
kartlegging.
Roller
De SafeGuard
kontrakten definerer følgende roller:
Oppdatering: De CREATOR_ROLE
rollen er fjernet som en løsning for problemet L02.
Omfang
Vi reviderte forpliktelse b2c63a9dfc4090be13320d999e7c6c1d842625d3
av safeguard
oppbevaringssted. I omfang er de smarte kontraktene i contracts
katalog. Imidlertid mocks
katalogen ble ansett som utenfor omfanget.
Antagelser
Systemet er ikke ment å kunne oppgraderes. De Registry
adressen er satt i konstruktøren av SafeGuardFactory
og hver SafeGuard
kan ha Timelock
satt kun én gang. Dette betyr at hvis en ny Registry
er utplassert en ny SafeGuardFactory
må utplasseres også. Hvis Timelock
implementeringsendringer, det gamle SafeGuard
s vil bli foreldet, og nye vil måtte distribueres.
Dessuten er systemet sterkt avhengig av implementeringen av en Timelock
kontrakt som ble ansett utenfor rammen for denne revisjonen. Teamet har ennå ikke ferdigstilt implementeringen av Timelock
kontrakt. På tidspunktet for skriving av denne rapporten, forbindelsens implementering av timelock brukes i prosjektet.
Kodebasen har blitt revidert av to revisorer i løpet av en uke og her presenterer vi våre funn.
Kritisk alvorlighetsgrad
Ingen.
Høy alvorlighetsgrad
[H01] ETH kan låses inne i Timelock
kontrakt
De Tally
teamet opprinnelig baserte sine implementeringer på bakken av GovernorBravoDelegate
Sammensatt kontrakt.
I løpet av denne revisjonen har Tally
team oppdaget en begrensning i Compounds guvernør hvor ETH sendte direkte til Timelock
er ikke tilgjengelig for bruk av styringsforslag, og selv om det ikke sitter fast permanent, krever det en forseggjort løsning for å bli hentet.
Dette er fordi sysselmannens gjennomføring krever at all verdien av et forslag vedlegges som msg.value
av kontoen som utløser kjøringen, uten å bruke på noen måte Timelock
ETH-midler.
De samme problem ble senere identifisert i SafeGuard
gjennomføring og teamet er klar over problemet og er i ferd med å fikse det.
Mens du løser problemet, bør du vurdere å bruke tilnærmingen vedtatt av OpenZeppelin-biblioteket for samme utgave.
Oppdatering: Rettet i commit 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory kan fryses
De Registry
Kontrakten er ment å holde styr på alle SafeGuards
at SafeGuardFactory
produserer. Den har det ytre register
funksjon som brukes til dette formålet.
Samtidig SafeGuardFactory
har i sin konstruktør oppdraget til det lokale registry
verdi til inngangsverdien. Det er ingen mulighet til å endre verdien på registry
variabel og av denne grunn, hvis en ny Registry
blir utplassert, må en ny fabrikk også utplasseres.
De SafeGuardFactory
har createSafeGuard
funksjon, ansvarlig for først utplassere en ny SafeGuard
, deretter En ny Timelock
med adressen til SafeGuard
as admin
, deretter stille inn timelock
variabel av SafeGuard
kontrakt og til slutt registrerer SafeGuard
i registeret.
Problemet er at enhver samtale til createSafeGuard
kan bli tvunget til å mislykkes av en angriper som direkte kan registrere den deterministiske adressen til den nye SafeGuard
før den ble opprettet. Når en kontrakt skaper en new
f.eks, dens nonce økes, og adressen til hvor den nye forekomsten av kontrakten vil bli distribuert, kan bestemmes av den opprinnelige kontraktsadressen og dens manglende. Derfor kan en angriper forhåndsberegne mange av adressene hvor den nye SafeGuards
vil bli distribuert og registrere disse adressene i Registry
ved å ringe til register
funksjon. Dette ville resultere i samtalene til createSafeGuard
til tilbake siden Registry
inneholder allerede adressen.
For å unngå at eksterne aktører ringer offentlig til Register
kontrakt, vurder å begrense tilgangen til register
funksjon for å akseptere anrop utelukkende av SafeGuardFactory
.
Oppdatering: Fast i PR # 10. Tally-teamet har fjernet Registry
kontrakt.
Middels alvorlighetsgrad
Ingen.
Lav alvorlighetsgrad
[L01] Kommenterte ut kode
De Registry
kontrakt inkluderer en kommentert kodelinje. For å forbedre lesbarheten bør du vurdere å fjerne den fra kodebasen.
Oppdatering: Fast i PR # 10 og forplikte seg 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] SafeGuards administrator kan tildele rollen som skaper til en hvilken som helst adresse
De SafeGuard
kontrakt definerer rollen til en CREATOR_ROLE
som, som navnet antyder, er tildelt skaperen av beskyttelsen.
Imidlertid ved å påkalle de grantRole
funksjon av AccessControlEnumerable
kontrakt i OpenZeppelin-kontraktsbiblioteket kan en administrator gi denne rollen til en hvilken som helst adresse. Dette kan forårsake forvirring fordi skaperen av SafeGuard
kan bare være SafeGuardFactory
.
Gjennom hele kodebasen har denne rollen kun blitt brukt for å begrense brukere fra å samhandle med setTimelock
funksjon av SafeGuard
kontrakt. Ved design sørger systemet for det setTimelock
funksjon kan bare ringes én gang, Fra innenfor SafeGuardFactory
kontrakt.
Vurder å fjerne CREATOR_ROLE
rolle fra SafeGuard
kontrakt og bruk av onlyOwner
modifier i setTimelock
funksjon.
Oppdatering: Fast i PR # 10.
[L03] Feil grensesnittdefinisjon og implementering
De ISafeGuard
grensesnittet definerer ikke queueTransactionWithDescription
funksjon implementert i SafeGuard
kontrakt, og samtidig definerer den __abdicate, __queueSetTimelockPendingAdmin og __executeSetTimelockPendingAdmin funksjoner, men de er ikke implementert.
For å forbedre korrektheten og konsistensen i kodebasen, vurder å refaktorisere ISafeGuard
grensesnitt for å matche nøyaktig SafeGuard
gjennomføring.
Oppdatering: Rettet i commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Manglende dokumentstrenger
Noen av kontraktene og funksjonene i kodebasen mangler dokumentasjon. For eksempel, noen funksjoner i SafeGuard
kontrakt.
I tillegg bruker noen docstrings uformelt språk, for eksempel det ene ovenfor setTimelock
fungere i SafeGuard
kontrakt.
Dette hindrer anmeldernes forståelse av kodens intensjon, som er grunnleggende for å kunne vurdere ikke bare sikkerhet, men også korrekthet. I tillegg forbedrer docstrings lesbarheten og forenkler vedlikeholdet. De bør eksplisitt forklare formålet eller intensjonen med funksjonene, scenariene de kan mislykkes under, rollene som er tillatt å kalle dem, verdiene som returneres og hendelsene som sendes ut.
Vurder å grundig dokumentere alle funksjoner (og deres parametere) som er en del av kontraktenes offentlige API. Funksjoner som implementerer sensitiv funksjonalitet, selv om de ikke er offentlige, bør også være tydelig dokumentert. Når du skriver docstrings, bør du vurdere å følge Ethereum naturlig spesifikasjonsformat (NatSpec).
Oppdatering: Delvis fast i PR # 10. Riktige docstrings er lagt til ulike funksjoner i hele kodebasen. Men i tillegg til de gjeldende endringene, bør du vurdere å gjøre følgende endringer:
- Legg til
description
som den@param
i docstringen ovenforqueueTransactionWithDescription
funksjon - Legg til
@param
i docstring ovenforcreateSafeGuard
fungere iSafeGuardFactory
kontrakt - Legg til
@return
i docstrings over funksjonene iSafeGuardFactory
kontrakt.
[L05] Ubrukelig eller gjentatt kode
Det er steder i kodebasen hvor kode enten gjentas eller ikke er nødvendig. Noen eksempler er:
- Linjer 29-32 av
Registry
kontrakt er ubrukelig, fordi_add
funksjon avEnumerableSet
kontrakt utfører allerede disse kontrollene mot verdiene som allerede er satt. - Linjer 62, 67, 73 og 78 av
SafeGuard
kontrakten gjentar alle nøyaktig samme operasjon. Vurder å kapsle den inn i en intern funksjon for å unngå duplisering av kode. - Linjer 62-63 og 67-68 of
SafeGuard
gjentas. Vurder å kapsle dem inn i en enkelt intern funksjon. - Bruken av
gasleft
for å spesifisere hvor mye gass som skal videresendes i kallet til funksjonenexecuteTransaction
er unødvendig. Dette er fordi, på det tidspunktet for henrettelse, vil hele gassen som er igjen bli brukt til å fortsette henrettelsen. Hvis dette ikke er for eksplisitt, vurder å fjernegas
parameter fra samtalen.
Vurder å bruke den foreslåtte fikseringen for å produsere en renere kode og forbedre konsistensen og modulariteten over kodebasen.
Oppdatering: Fast i PR # 10 og forplikte seg 7fd27df16fc879d990d36a167a0b6e719e578558
.
Merknader og tilleggsinformasjon
[N01] Inkonsekvent stil
Det er noen steder i kodebasen hvor forskjeller i stil påvirker lesbarheten, noe som gjør det vanskeligere å forstå koden. Noen eksempler er:
- De
Registry
kontrakt bruker forskjellige stiler for docstrings i hele kontrakten. - De
SafeGuard
kontrakten avgir en hendelse nårqueueTransactionWithDescription
kalles, men ingen hendelser sendes ut i andre funksjoner som omhandler transaksjoner. - på
SafeGuard
kontrakt noen ganger verdi brukes som navngitt parameter og noen ganger _verdi benyttes.
Ta i betraktning verdien en konsistent kodestil legger til prosjektets lesbarhet, vurder å håndheve en standard kodestil ved hjelp av linterverktøy, for eksempel Solhint.
Oppdatering: Fast i PR # 10 og forplikte seg 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Manglende lisens
Følgende kontrakter i kodebasen mangler en SPDX-lisensidentifikator.
For å dempe kompilatoradvarsler og øke konsistensen på tvers av kodebasen, bør du vurdere å legge til en lisensidentifikator. Mens du gjør det vurdere å referere til spdx.dev retningslinjer.
Oppdatering: Fast i PR # 10 og forplikte seg 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] OpenZeppelin-kontraktens avhengighet er ikke festet
For å forhindre uventet oppførsel i tilfelle brytende endringer blir utgitt i fremtidige oppdateringer av OpenZeppelin Contracts sitt bibliotek, vurder å feste versjonen av denne avhengigheten i pakke.json filen.
Oppdatering: Fast i PR # 10.
[N04] Solidity-kompilatorversjonen er ikke festet
Gjennom hele kodebasen bør du vurdere å feste versjonen av Solidity kompilator til den siste stabile versjonen. Dette bør bidra til å forhindre introduksjon av uventede feil på grunn av inkompatible fremtidige utgivelser. For å velge en spesifikk versjon, bør utviklere vurdere både kompilatorens funksjoner som trengs av prosjektet og listen over kjente feil knyttet til hver Solidity-kompilatorversjon.
Oppdatering: Fast i PR # 10.
[N05] Skrivefeil
Ved ulike tilfeller i hele kodebasen, ordet role
er feilstavet som rol
. Et slikt eksempel er i docstringen i constructor
av SafeGuard
kontrakt.
Vurder å rette disse skrivefeilene for å forbedre kodelesbarheten.
Oppdatering: Delvis fast i PR # 10. Mens skrivemåten av role
har blitt korrigert, bør kommentaren "sett admin rolle til en definert admin adresse" være "sett admin rolle til en definert admin adresse". I tillegg er "execute" feilstavet i SafeGuard
kontrakt på linje 69, linje 82, linje 96 og linje 110 og "tilgjengelig" er feilstavet linje 70, linje 83, linje 97, linje 111. Vurder også å erstatte uformelle ord som f.eks "skal" in SafeGuard
kontrakt med formelle alternativer som "å gå til".
[N06] Erklær uint som uint256
Det er flere forekomster i kodebasen hvor variabler er deklarert uint
datatype i stedet for uint256
. For eksempel eta
variabel i QueueTransactionWithDescription
hendelse av SafeGuard
kontrakt.
For å favorisere eksplisitt, alle forekomster av uint
skal erklæres som uint256
.
Oppdatering: Fast i PR # 10 og forplikte seg 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Ubrukt import
De SafeGuard
kontrakt importerer console
kontrakt, men bruker den aldri.
For å forbedre lesbarheten til koden bør du vurdere å fjerne ubrukte importer.
Oppdatering: Fast i PR # 10.
Konklusjoner
En høy og flere andre mindre sårbarheter er funnet, og anbefalinger og rettelser er foreslått.
- &
- adgang
- Logg inn
- tvers
- Handling
- handlinger
- Ytterligere
- adresse
- admin
- Alle
- allerede
- Selv
- api
- tilnærming
- rundt
- revisjon
- I utgangspunktet
- være
- bugs
- virksomhet
- ring
- saker
- Årsak
- endring
- kostnad
- kode
- Koding
- Felles
- Compound
- forvirring
- hensyn
- inneholder
- fortsette
- kontrakt
- kontrakter
- kunne
- skaperen
- Gjeldende
- dato
- håndtering
- utforming
- utviklere
- forskjellig
- oppdaget
- Utdype
- ETH
- Event
- hendelser
- eksempel
- fabrikk
- Egenskaper
- Endelig
- Først
- Fix
- fleksibilitet
- funnet
- funksjon
- midler
- framtid
- GAS
- Giving
- mål
- styresett
- Guvernør
- retningslinjer
- å ha
- hjelpe
- her.
- Høy
- Hvordan
- HTTPS
- implementert
- Øke
- økt
- Interface
- IT
- kjent
- Språk
- siste
- Bibliotek
- Tillatelse
- linje
- Liste
- lokal
- låst
- så
- Making
- Match
- modulære
- multitegn
- Annen
- presentere
- prosess
- prosjekt
- forslag
- offentlig
- publisere
- registrere
- Utgivelser
- rapporterer
- Repository
- Resultater
- anmeldelse
- sikkerhet
- sett
- innstilling
- delt
- Smart
- Smarte kontrakter
- soliditet
- Tilstand
- stil
- system
- Gjennom
- hele
- tid
- verktøy
- spor
- Transaksjoner
- oppdateringer
- us
- Brukere
- verdi
- Sikkerhetsproblemer
- Lommebøker
- uke
- HVEM
- innenfor
- ord
- skriving