December 6, 2021
Predstavitev
O Tally ekipa nas je prosila, da pregledamo in revidiramo nabor pogodb s končnim ciljem izboljšati skupne pogodbe o upravljanju in jim dati več prožnosti. Pregledali smo kodo in zdaj objavljamo rezultate.
Pregled sistema
Sistem temelji na treh glavnih pogodbah:
- A
SafeGuard
predlogo pogodbe. Ta pogodba jeadmin
oTimelock
pogodbe in doda plast modularnih vlog čezTimelock
dejanja. Ta pogodba določa več vlog, ki imajo ločeno odgovornost in dostop do stanja predloga (čakalna vrsta, preklic in izvedba). - A
SafeGuardFactory
pogodba razporedi novSafeGuard
in ustrezno novoTimelock
pogodba. Nato nastavi naslov časovne zapore znotrajSafeGuard
pogodbo in registrira novSafeGuard
vRegistry
. - A
Registry
ki vsebuje seznam razporejenihSafeGuards
z njihovimi ustreznimi Številka različice.
O SafeGuardFactory
bo v bistvu ustvaril a novo SafeGuard
kadar koli je poklican. A SafeGuard
je ovitek okoli Timelock
Operacije ki doda različne in ločene vloge za vsako dejanje. Vloge so strukturirane z uporabo OpenZeppelina AccessControlEnumerable
pogodba, ki daje večjo prilagodljivost in združljivost denarnicam z več podpisi in primerom poslovne uporabe, kjer ima lahko več naslovov isto skupno vlogo.
Posodobitev: v PR # 10, se je ekipa Tally odločila odstraniti Registry
pogodba. Seznam razporejenih zaščitnih ukrepov je zdaj shranjen v SafeGuardFactory
pogodbo, v safeGuards
števen niz, skupaj z njihovo različico, shranjeno v safeGuardVersion
kartiranje.
vloge
O SafeGuard
pogodba določa naslednje vloge:
Posodobitev: O CREATOR_ROLE
vloga je bila odstranjena kot popravek za težavo L02.
področje uporabe
Revidirali smo zavezo b2c63a9dfc4090be13320d999e7c6c1d842625d3
od safeguard
repozitorij. V obsegu so pametne pogodbe v contracts
imenik. Vendar pa je mocks
Imenik je bil ocenjen kot izven obsega.
Predpostavke
Sistem ni namenjen nadgradnji. The Registry
naslov je nastavljen v konstruktorju od SafeGuardFactory
in vsak SafeGuard
lahko ima Timelock
nastavite samo enkrat. To pomeni, da če nov Registry
je razporejen nov SafeGuardFactory
mora biti tudi razporejen. Če je Timelock
izvedbene spremembe, staro SafeGuard
bodo zastareli in treba bo uvesti nove.
Poleg tega je sistem močno odvisen od izvajanja a Timelock
Naročilo ki se je štelo izven obsega te revizije. Ekipa še ni dokončala izvedbe Timelock
pogodba. V času pisanja tega poročila, Compoundova implementacija časovne zapore se uporablja v projektu.
Bazo kode sta v enem tednu pregledala dva revizorja in tukaj predstavljamo naše ugotovitve.
Kritična resnost
Jih ni.
Visoka resnost
[H01] ETH je mogoče zakleniti znotraj Timelock
Naročilo
O Tally
ekipa je svoje izvedbe prvotno temeljila na podlagi GovernorBravoDelegate
Sestavljena pogodba.
Med to revizijo je Tally
ekipa odkrila omejitev v Compoundovem guvernerju, kjer je ETH poslan neposredno na Timelock
ni na voljo za uporabo s predlogi za upravljanje, in čeprav ni trajno obtičal, zahteva dovršeno rešitev za pridobitev.
To je zato, ker izvedba guvernerja zahteva, da je priložena vsa vrednost predloga msg.value
z računom, ki sproži izvršitev, pri čemer nikakor ne uporablja Timelock
sredstva ETH.
O ista težava je bila pozneje ugotovljena v SafeGuard
Izvajanje in ekipa se zaveda težave in jo odpravlja.
Med odpravljanjem težave razmislite o uporabi pristopa sprejela knjižnica OpenZeppelin za isto izdajo.
Posodobitev: Popravljeno v objavi 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory je mogoče zamrzniti
O Registry
pogodba je namenjena spremljanju vseh SafeGuards
da SafeGuardFactory
proizvaja. Ima zunanjo register
funkcija ki se uporablja v ta namen.
Hkrati je SafeGuardFactory
ima v svojem konstruktorju dodelitev lokal registry
vrednost na vhodno vrednost. Ni možnosti za spremembo vrednosti registry
spremenljivka in iz tega razloga, če nov Registry
uvedena, je treba uvesti tudi novo tovarno.
O SafeGuardFactory
ima createSafeGuard
funkcija, zadolžen za prvo uvajanje novega SafeGuard
, Potem nova Timelock
z naslovom SafeGuard
as admin
, nato nastavite timelock
spremenljivka od SafeGuard
pogodbo in končno registracija SafeGuard
v registru.
Težava je v tem, da vsak klic na createSafeGuard
lahko napadalec, ki lahko neposredno registrira deterministični naslov novega, povzroči neuspeh SafeGuard
pred njegovim nastankom. Kadarkoli pogodba ustvarja a new
primer, se njegov nonce poveča, naslov, kjer bi bil razporejen novi primerek pogodbe, pa je mogoče določiti z naslovom prvotne pogodbe in njegovim nonce. Zato lahko napadalec vnaprej izračuna številne naslove, kjer je nov SafeGuards
bodo razporejeni in te naslove registrirali v Registry
s klicem na register
funkcijo. To bi povzročilo klice na createSafeGuard
do povrniti od Registry
že vsebuje naslov.
Da bi se izognili temu, da bi zunanji akterji javno klicali Register
pogodbe, razmislite o omejitvi dostopa do register
funkcijo za sprejemanje klicev izključno s strani SafeGuardFactory
.
Posodobitev: Fiksno v PR # 10. Ekipa Tally je odstranila Registry
pogodba.
Srednja resnost
Jih ni.
Nizka resnost
[L01] Komentirana koda
O Registry
pogodba vključuje komentirano vrstico kode. Če želite izboljšati berljivost, razmislite o odstranitvi iz kodne baze.
Posodobitev: Fiksno v PR # 10 in se zavezati 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] Skrbnik SafeGuard lahko dodeli vlogo ustvarjalca kateremu koli naslovu
O SafeGuard
pogodba določa vlogo a CREATOR_ROLE
ki je, kot že ime pove, dodeljeno ustvarjalcu varovala.
Vendar s priklicem o grantRole
funkcija AccessControlEnumerable
Naročilo v pogodbeni knjižnici OpenZeppelin lahko skrbnik dodeli to vlogo kateremu koli naslovu. To bi lahko povzročilo zmedo, ker ustvarjalec SafeGuard
lahko samo SafeGuardFactory
.
V celotni zbirki kode je bila ta vloga uporabljena samo za omejevanje uporabnikov pri interakciji z setTimelock
funkcija od SafeGuard
pogodba. Sistem po zasnovi to zagotavlja setTimelock
funkcija lahko kliče samo enkrat, Od znotraj SafeGuardFactory
Naročilo.
Razmislite o odstranitvi CREATOR_ROLE
vlogo iz SafeGuard
pogodbo in uporabo onlyOwner
sprememba v setTimelock
Funkcija.
Posodobitev: Fiksno v PR # 10.
[L03] Nepravilna definicija in implementacija vmesnika
O ISafeGuard
vmesnik ne definira queueTransactionWithDescription
funkcija izvajajo v SafeGuard
pogodbo, hkrati pa opredeljuje __abdicate, __queueSetTimelockPendingAdmin in __executeSetTimelockPendingAdmin funkcije, vendar niso implementirane.
Če želite izboljšati pravilnost in doslednost v kodni bazi, razmislite o preoblikovanju ISafeGuard
vmesnik, ki se natančno ujema z SafeGuard
izvajanje.
Posodobitev: Popravljeno v objavi 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Manjkajoči nizi dokumentov
Nekatere pogodbe in funkcije v bazi kode nimajo dokumentacije. na primer nekatere funkcije v SafeGuard
pogodba.
Poleg tega nekateri nizi dokumentov uporabljajo neformalen jezik, kot je ta nad setTimelock
funkcijo v SafeGuard
Naročilo.
To ovira pregledovalce pri razumevanju namena kode, ki je temeljnega pomena za pravilno oceno ne le varnosti, ampak tudi pravilnosti. Poleg tega nizi dokumentov izboljšajo berljivost in olajšajo vzdrževanje. Izrecno morajo razložiti namen ali namero funkcij, scenarije, po katerih lahko ne uspejo, vloge, ki jih lahko kličejo, vrnjene vrednosti in oddane dogodke.
Razmislite o temeljitem dokumentiranju vseh funkcij (in njihovih parametrov), ki so del javnega API-ja pogodbe. Tudi funkcije, ki izvajajo občutljive funkcije, tudi če niso javne, morajo biti jasno dokumentirane. Ko pišete nize dokumentov, upoštevajte Ethereum Natural Specification Format (NatSpec).
Posodobitev: Delno pritrjeno PR # 10. Ustrezni nizi dokumentov so bili dodani različnim funkcijam v celotni bazi kode. Vendar pa poleg trenutnih sprememb razmislite o naslednjih spremembah:
- Dodaj
description
kot@param
v zgornjem nizu dokumentovqueueTransactionWithDescription
funkcija - Dodaj
@param
v docstring nadcreateSafeGuard
deluje vSafeGuardFactory
Naročilo - Dodaj
@return
v nizih dokumentov nad funkcijami vSafeGuardFactory
pogodba.
[L05] Neuporabna ali ponavljajoča se koda
Obstajajo mesta v zbirki kode, kjer se koda ponavlja ali pa ni potrebna. Nekaj primerov:
- Vrstice 29-32 od
Registry
pogodbe so neuporabne, saj_add
funkcijaEnumerableSet
Naročilo te preglede že izvaja proti že nastavljenim vrednostim. - Vrstice 62, 67, 73 in 78 od
SafeGuard
pogodbe vse ponavljajo enako natančno operacijo. Razmislite o enkapsulaciji v notranjo funkcijo, da se izognete podvajanju kode. - Vrstice 62-63 in 67-68 of
SafeGuard
se ponavljajo. Razmislite o njihovi enkapsulaciji v eno samo notranjo funkcijo. - Uporaba
gasleft
da določite, koliko plina naj se posreduje v klicu funkcijeexecuteTransaction
je nepotrebno. To je zato, ker bo na tej točki izvedbe ves preostali plin porabljen za nadaljevanje izvedbe. Če to ni zaradi eksplicitnosti, razmislite o odstranitvigas
parameter iz klica.
Razmislite o uporabi predlaganega popravka, da ustvarite čistejšo kodo in izboljšate doslednost in modularnost baze kode.
Posodobitev: Fiksno v PR # 10 in se zavezati 7fd27df16fc879d990d36a167a0b6e719e578558
.
Opombe in dodatne informacije
[N01] Nedosleden slog
Obstaja nekaj mest v osnovi kode, kjer razlike v slogu vplivajo na berljivost, zaradi česar je težje razumeti kodo. Nekaj primerov:
- O
Registry
pogodba uporablja različne sloge za nize dokumentov v celotni pogodbi. - O
SafeGuard
pogodba oddaja dogodek, koqueueTransactionWithDescription
se kliče, vendar v drugih funkcijah, ki obravnavajo transakcije, niso oddani nobeni dogodki. - v
SafeGuard
pogodba, včasih vrednost se uporablja kot imenovan parameter in včasih _value se uporablja.
Ob upoštevanju vrednosti, ki jo dosleden slog kodiranja doda berljivosti projekta, razmislite o uveljavitvi standardnega sloga kodiranja s pomočjo orodij za linter, kot je Solhint.
Posodobitev: Fiksno v PR # 10 in se zavezati 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Manjkajoča licenca
Naslednjim pogodbam znotraj kodne baze manjka identifikator licence SPDX.
Če želite utišati opozorila prevajalnika in povečati skladnost v zbirki kode, razmislite o dodajanju identifikatorja licence. Pri tem razmislite o sklicevanju na spdx.dev smernice.
Posodobitev: Fiksno v PR # 10 in se zavezati 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] Odvisnost pogodbe OpenZeppelin ni pripeta
Da bi preprečili nepričakovano vedenje v primeru, da se v prihodnjih posodobitvah izdajo pokvarjene spremembe Knjižnica pogodb OpenZeppelin, razmislite o pripenjanju različice te odvisnosti v paket.json Datoteka.
Posodobitev: Fiksno v PR # 10.
[N04] Različica prevajalnika Solidity ni pripeta
V celotni bazi kode razmislite o pripenjanju različice Prevajalnik Solidity na najnovejšo stabilno različico. To naj bi pomagalo preprečiti vnos nepričakovanih napak zaradi nezdružljivih prihodnjih izdaj. Pri izbiri določene različice morajo razvijalci upoštevati funkcije prevajalnika, ki jih potrebuje projekt, in seznam znanih napak povezana z vsako različico prevajalnika Solidity.
Posodobitev: Fiksno v PR # 10.
[N05] Tiskarska napaka
V različnih primerih po vsej kodni bazi je beseda role
je napačno črkovano kot rol
. En tak primer je v niz dokumentov znotraj constructor
od SafeGuard
Naročilo.
Razmislite o popravku teh tipkarskih napak, da izboljšate berljivost kode.
Posodobitev: Delno pritrjeno PR # 10. Medtem ko je črkovanje role
je bil popravljen, komentar »nastavi skrbniško vlogo na definiran skrbniški naslov« bi moral biti »nastavi skrbniško vlogo na definiran skrbniški naslov«. Poleg tega je »izvedba« napačno črkovana v SafeGuard
pogodbo o 69. vrstica, 82. vrstica, 96. vrstica in 110. vrstica in "na voljo" je napačno črkovano 70. vrstica, 83. vrstica, 97. vrstica, 111. vrstica. Razmislite tudi o zamenjavi neformalnih besed, kot je npr "bom" in SafeGuard
pogodbo s formalnimi alternativami, kot je "bojem".
[N06] Razglasi uint kot uint256
V kodni bazi je več pojavov, kjer so deklarirane spremenljivke uint
podatkovni tip namesto uint256
. Na primer eta
spremenljivka v QueueTransactionWithDescription
dogodek od SafeGuard
pogodba.
Da bi dali prednost eksplicitnosti, vsi primeri uint
je treba razglasiti kot uint256
.
Posodobitev: Fiksno v PR # 10 in se zavezati 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Neuporabljen uvoz
O SafeGuard
pogodba uvozi console
pogodbo, vendar je nikoli ne uporablja.
Če želite izboljšati berljivost kode, razmislite o odstranitvi vseh neuporabljenih uvozov.
Posodobitev: Fiksno v PR # 10.
Sklepi
Najdene so bile ena visoka in več drugih manjših ranljivosti ter predlagana priporočila in popravki.
- &
- dostop
- Račun
- čez
- Ukrep
- dejavnosti
- Dodatne
- Naslov
- admin
- vsi
- že
- Čeprav
- API
- pristop
- okoli
- Revizija
- V bistvu
- počutje
- hrošči
- poslovni
- klic
- primeri
- Vzrok
- spremenite
- naboj
- Koda
- Kodiranje
- Skupno
- Sestavljeni
- zmeda
- premislek
- Vsebuje
- naprej
- Naročilo
- pogodbe
- bi
- kreator
- Trenutna
- datum
- deliti
- Oblikovanje
- Razvijalci
- drugačen
- odkril
- Izdelati
- ETH
- Event
- dogodki
- Primer
- Tovarna
- Lastnosti
- končno
- prva
- fiksna
- prilagodljivost
- je pokazala,
- funkcija
- Skladi
- Prihodnost
- GAS
- Giving
- Cilj
- upravljanje
- Guverner
- Smernice
- ob
- pomoč
- tukaj
- visoka
- Kako
- HTTPS
- izvajali
- Povečajte
- povečal
- vmesnik
- IT
- znano
- jezik
- Zadnji
- Knjižnica
- Licenca
- vrstica
- Seznam
- lokalna
- zaklenjeno
- Pogledal
- Izdelava
- Stave
- Modularna
- večznakovni
- Ostalo
- predstaviti
- Postopek
- Projekt
- snubitev
- javnega
- objavijo
- Registracija
- Izpusti
- poročilo
- Skladišče
- Rezultati
- pregleda
- varnost
- nastavite
- nastavitev
- deli
- pametna
- Pametne pogodbe
- trdnost
- Država
- slog
- sistem
- skozi
- vsej
- čas
- orodja
- sledenje
- Transakcije
- posodobitve
- us
- Uporabniki
- vrednost
- Ranljivosti
- Denarnice
- teden
- WHO
- v
- besede
- pisanje