December 6, 2021
Introducere
răboj Echipa ne-a cerut să revizuim și să audităm un set de contracte cu scopul final de a îmbunătăți contractele comune de guvernanță și de a le oferi mai multă flexibilitate. Ne-am uitat la cod și acum publicăm rezultatele noastre.
Vizualizarea sistemului
Sistemul se bazează pe trei contracte principale:
- A
SafeGuard
model de contract. Acest contract esteadmin
a unuiTimelock
contract și adaugă un strat de roluri modulare pesteTimelock
acțiunile lui. Acest contract definește mai multe roluri care au responsabilitate și acces separat asupra stării unei propuneri (în așteptare, anulare și execuție). - A
SafeGuardFactory
contractul implementează un nouSafeGuard
și un nou corespunzătorTimelock
contracta. Apoi setează adresa timelock în interiorulSafeGuard
contractează și înregistrează noulSafeGuard
înRegistry
. - A
Registry
care deține o listă de dislocațiSafeGuards
cu corespondentul lor Numărul versiunii.
SafeGuardFactory
practic va genera a nou SafeGuard
ori de câte ori este chemat. A SafeGuard
este o înfășurare Timelock
operațiuni care adaugă roluri diferite și separate pentru fiecare acțiune. Rolurile sunt structurate prin utilizarea OpenZeppelin's AccessControlEnumerable
contract care oferă mai multă flexibilitate și compatibilitate portofelelor multisig și cazurilor de utilizare în afaceri în care mai multe adrese pot avea același rol comun.
Actualizați: În PR #10, echipa Tally a decis să elimine Registry
contracta. Lista măsurilor de protecție implementate este acum stocată în SafeGuardFactory
contract, în safeGuards
setul enumerabil, împreună cu versiunea lor stocată în safeGuardVersion
cartografiere.
Roluri
SafeGuard
contractul definește următoarele roluri:
Actualizați: CREATOR_ROLE
rolul a fost eliminat ca remediere pentru problema L02.
domeniu
Am auditat commit b2c63a9dfc4090be13320d999e7c6c1d842625d3
a safeguard
repertoriu. În domeniul de aplicare sunt contractele inteligente din contracts
director. Însă mocks
directorul a fost considerat în afara domeniului de aplicare.
Presupuneri
Sistemul nu este menit să fie actualizat. The Registry
adresa este setată în constructor a SafeGuardFactory
și fiecare SafeGuard
poate avea Timelock
setat o singura data. Aceasta înseamnă că dacă un nou Registry
este implementat un nou SafeGuardFactory
trebuie de asemenea implementat. Dacă Timelock
modificări de implementare, cele vechi SafeGuard
Vor deveni învechite, iar altele noi vor trebui implementate.
În plus, sistemul se bazează în mare măsură pe implementarea unui Timelock
contract care a fost considerat în afara domeniului de aplicare al acestui audit. Echipa nu a finalizat încă implementarea programului Timelock
contracta. La momentul redactării acestui raport, implementarea de către Compus a timelock-ului este utilizat în proiect.
Baza de cod a fost auditată de doi auditori pe parcursul unei săptămâni și aici vă prezentăm constatările noastre.
Severitate critică
Nici unul.
Severitate ridicată
[H01] ETH poate fi blocat în interiorul Timelock
contract
Tally
echipa și-a bazat inițial implementările pe terenul GovernorBravoDelegate
Contract compus.
Pe parcursul acestui audit, Tally
echipa a descoperit o limitare în guvernatorul Compoundului unde ETH a trimis direct la Timelock
nu este disponibil pentru utilizare de către propunerile de guvernare și, deși nu este blocat permanent, necesită o soluție elaborată pentru a fi preluată.
Acest lucru se datorează faptului că implementarea guvernatorului necesită ca toată valoarea unei propuneri să fie atașată ca msg.value
de contul care declanșează execuția, nefolosind în nici un fel the Timelock
fonduri ETH.
aceeași problemă a fost identificată ulterior în SafeGuard
implementarea iar echipa este conștientă de problemă și este în proces de remediere.
În timp ce remediați problema, luați în considerare utilizarea abordării adoptat de biblioteca OpenZeppelin pentru aceeași problemă.
Actualizați: Fixat în comitere 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory poate fi înghețat
Registry
contractul este destinat să țină evidența tuturor SafeGuards
faptul că SafeGuardFactory
produce. Are exteriorul register
funcţie care este folosit în acest scop.
În același timp, SafeGuardFactory
are, în constructorul său, atribuirea localului registry
valoare la valoarea de intrare. Nu există posibilitatea de a schimba valoarea registry
variabilă și din acest motiv, dacă este nouă Registry
este desfășurată, trebuie să fie instalată și o nouă fabrică.
SafeGuardFactory
are createSafeGuard
funcţie, responsabil de primul desfășurarea unui nou SafeGuard
, Apoi un nou Timelock
cu adresa lui SafeGuard
as admin
, apoi setarea timelock
variabila a SafeGuard
contract și în sfârșit înregistrarea SafeGuard
în registru.
Problema este că orice apel la createSafeGuard
poate fi forțat să eșueze de către un atacator care poate înregistra direct adresa deterministă a noului SafeGuard
înainte de crearea sa. Ori de câte ori un contract creează o new
instanță, nonce-ul acestuia este mărit, iar adresa unde s-ar desfășura noua instanță a contractului poate fi determinată de adresa contractului inițial și de nonce. Prin urmare, un atacator poate precalcula multe dintre adresele unde este nou SafeGuards
vor fi implementate și vor înregistra acele adrese în Registry
apelând register
funcţie. Acest lucru ar duce la apeluri către createSafeGuard
la reveni din moment ce Registry
conține deja adresa.
Pentru a evita ca actori externi să apeleze public la Register
contract, luați în considerare restricționarea accesului la register
funcția de a accepta apeluri exclusiv de către SafeGuardFactory
.
Actualizați: Fixat în PR #10. Echipa Tally a eliminat Registry
contracta.
Severitate medie
Nici unul.
Severitate scăzută
[L01] Cod comentat
Registry
contractul include o linie de cod comentată. Pentru a îmbunătăți lizibilitatea, luați în considerare eliminarea acestuia din baza de cod.
Actualizați: Fixat în PR #10 și să se angajeze 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] Administratorul SafeGuard poate atribui rolul de creator oricărei adrese
SafeGuard
contractul definește rolul unui CREATOR_ROLE
care, după cum sugerează și numele, este atribuit creatorului garanției.
Cu toate acestea, prin invocare il grantRole
funcția AccessControlEnumerable
contract în biblioteca de contracte OpenZeppelin, un administrator poate acorda acest rol oricărei adrese. Acest lucru ar putea cauza confuzie deoarece creator al SafeGuard
poate fi doar SafeGuardFactory
.
În întreaga bază de cod, acest rol a fost folosit doar pentru a restricționa utilizatorii să interacționeze cu setTimelock
funcţie a SafeGuard
contracta. Prin proiectare, sistemul asigură acest lucru setTimelock
funcţie poate fi apelat o singură dată, din în cadrul SafeGuardFactory
contract.
Luați în considerare eliminarea CREATOR_ROLE
rol din SafeGuard
contract și folosind onlyOwner
schimbare în setTimelock
Funcția.
Actualizați: Fixat în PR #10.
[L03] Definirea și implementarea interfeței incorecte
ISafeGuard
interfața nu definește queueTransactionWithDescription
funcţie implementate în SafeGuard
contractul și, în același timp, definește __abdica, __queueSetTimelockPendingAdmin și __executeSetTimelockPendingAdmin funcții dar nu sunt implementate.
Pentru a îmbunătăți corectitudinea și coerența în baza de cod, luați în considerare refactorizarea ISafeGuard
interfață pentru a se potrivi exact cu SafeGuard
punerea în aplicare.
Actualizați: Fixat în comitere 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Docstrings lipsesc
Unele dintre contractele și funcțiile din baza de cod nu dispun de documentație. De exemplu, unele funcții în SafeGuard
contracta.
În plus, unele documente folosesc un limbaj informal, cum ar fi cel deasupra setTimelock
funcția în SafeGuard
contract.
Acest lucru împiedică examinatorii înțelegerea intenției codului, care este fundamentală pentru a evalua corect nu numai securitatea, ci și corectitudinea. În plus, documentele îmbunătățesc lizibilitatea și ușurează întreținerea. Aceștia ar trebui să explice în mod explicit scopul sau intenția funcțiilor, scenariile în care pot eșua, rolurile cărora le este permis să le apeleze, valorile returnate și evenimentele emise.
Luați în considerare documentarea completă a tuturor funcțiilor (și parametrilor acestora) care fac parte din API-ul public al contractelor. Funcțiile care implementează funcționalități sensibile, chiar dacă nu sunt publice, ar trebui să fie, de asemenea, documentate în mod clar. Când scrieți documente, luați în considerare următorul text Formatul de specificații naturale Ethereum (NatSpec).
Actualizați: Parțial fixat în PR #10. Docstring-uri adecvate au fost adăugate la diferite funcții de-a lungul bazei de cod. Cu toate acestea, pe lângă modificările actuale, luați în considerare efectuarea următoarelor modificări:
- Adăuga
description
ca@param
în documentul de mai susqueueTransactionWithDescription
funcţie - Adăuga
@param
în docstring deasupracreateSafeGuard
funcția înSafeGuardFactory
contract - Adăuga
@return
în docstrings deasupra funcțiilor înSafeGuardFactory
contracta.
[L05] Cod inutil sau repetat
Există locuri în baza de cod în care codul fie este repetat, fie nu este necesar. Câteva exemple sunt:
- Liniile 29-32 a
Registry
contractul sunt inutile, deoarece_add
funcțiaEnumerableSet
contract efectuează deja aceste verificări față de valorile deja setate. - Linii 62, 67, 73 și 78 a
SafeGuard
contractul repetă exact aceeași operațiune. Luați în considerare încapsularea acestuia într-o funcție internă pentru a evita duplicarea codului. - Liniile 62-63 și 67-68 of
SafeGuard
sunt repetate. Luați în considerare încapsularea lor într-o singură funcție internă. - Utilizarea
gasleft
pentru a specifica cât gaz trebuie redirecționat în apelul funcțieiexecuteTransaction
este inutil. Acest lucru se datorează faptului că, în acel punct de execuție, întregul gaz rămas va fi folosit pentru a continua execuția. Dacă acest lucru nu este pentru claritate, luați în considerare eliminareagas
parametrul din apel.
Luați în considerare aplicarea remedierii sugerate pentru a produce un cod mai curat și pentru a îmbunătăți consistența și modularitatea bazei de cod.
Actualizați: Fixat în PR #10 și să se angajeze 7fd27df16fc879d990d36a167a0b6e719e578558
.
Note și informații suplimentare
[N01] Stil inconsecvent
Există unele locuri în baza codului, unde diferențele de stil afectează lizibilitatea, făcând mai dificilă înțelegerea codului. Câteva exemple sunt:
-
Registry
contractul folosește stiluri diferite pentru documente în întregul contract. -
SafeGuard
contractul emite un eveniment cândqueueTransactionWithDescription
este apelat dar nu sunt emise evenimente în alte funcții care se ocupă de tranzacții. - În
SafeGuard
contract, uneori valoare este folosit ca parametru numit și uneori _valoare este folosit.
Luând în considerare valoarea pe care un stil de codare consecvent o adaugă lizibilității proiectului, luați în considerare aplicarea unui stil de codare standard cu ajutorul instrumentelor linter, cum ar fi Solhint.
Actualizați: Fixat în PR #10 și să se angajeze 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Licență lipsă
Următoarele contracte din baza de coduri lipsesc un identificator de licență SPDX.
Pentru a reduce la tăcere avertismentele compilatorului și pentru a crește consistența în baza de cod, luați în considerare adăugarea unui identificator de licență. În timp ce o faceți, luați în considerare referirea la spdx.dev orientări.
Actualizați: Fixat în PR #10 și să se angajeze 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] Dependența OpenZeppelin Contract nu este fixată
Pentru a preveni comportamentele neașteptate în cazul în care sunt lansate modificări nerespective în actualizările viitoare ale Biblioteca OpenZeppelin Contracts, luați în considerare fixarea versiunii acestei dependențe în fișierul pachet.json fișier.
Actualizați: Fixat în PR #10.
[N04] Versiunea compilatorului Solidity nu este fixată
Pe parcursul bazei de cod, luați în considerare fixarea versiunii Compilatorul de soliditate la cea mai recentă versiune stabilă. Acest lucru ar trebui să prevină introducerea de erori neașteptate din cauza versiunilor viitoare incompatibile. Pentru a alege o anumită versiune, dezvoltatorii ar trebui să ia în considerare atât caracteristicile compilatorului necesare proiectului, cât și lista erorilor cunoscute asociat cu fiecare versiune de compilator Solidity.
Actualizați: Fixat în PR #10.
[N05] Greșeală
În diferite cazuri de-a lungul bazei de cod, cuvântul role
este scris greșit ca rol
. Un astfel de exemplu este în docstring din cadrul constructor
a SafeGuard
contract.
Luați în considerare corectarea acestor greșeli de scriere pentru a îmbunătăți lizibilitatea codului.
Actualizați: Parțial fixat în PR #10. În timp ce ortografia lui role
a fost corectat, comentariul „setează rolul de administrator la o adresă de administrator definită” ar trebui să fie „setează rolul de administrator la o adresă de administrator definită”. În plus, „execute” este scris greșit în SafeGuard
contract pe line 69, line 82, line 96 și line 110 iar „disponibil” este scris greșit line 70, line 83, line 97, line 111. De asemenea, luați în considerare înlocuirea cuvintelor informale precum „voi” in SafeGuard
contract cu alternative formale precum „going to”.
[N06] Declarați uint ca uint256
Există mai multe apariții în baza de cod în care sunt declarate variabile uint
tip de date în loc de uint256
. De exemplu, eta
variabilă în QueueTransactionWithDescription
eveniment a SafeGuard
contracta.
Pentru a favoriza claritatea, toate cazurile de uint
ar trebui declarat ca uint256
.
Actualizați: Fixat în PR #10 și să se angajeze 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Import nefolosit
SafeGuard
contract importurile console
contract, dar nu-l folosește niciodată.
Pentru a îmbunătăți lizibilitatea codului, luați în considerare eliminarea oricăror importuri neutilizate.
Actualizați: Fixat în PR #10.
Concluzii
Au fost găsite o vulnerabilitate ridicată și alte câteva vulnerabilități minore și au fost sugerate recomandări și remedieri.
- &
- acces
- Cont
- peste
- Acțiune
- acțiuni
- Suplimentar
- adresa
- admin
- TOATE
- deja
- Cu toate ca
- api
- abordare
- în jurul
- de audit
- Pe scurt
- fiind
- gandaci
- afaceri
- apel
- cazuri
- Provoca
- Schimbare
- taxă
- cod
- Codificare
- Comun
- Compus
- confuzie
- considerare
- conține
- continua
- contract
- contracte
- ar putea
- creator
- Curent
- de date
- abuzive
- Amenajări
- Dezvoltatorii
- diferit
- a descoperit
- Elaborat
- ETH
- eveniment
- evenimente
- exemplu
- fabrică
- DESCRIERE
- În cele din urmă
- First
- Repara
- Flexibilitate
- găsit
- funcţie
- Fondurile
- viitor
- GAS
- Oferirea
- scop
- guvernare
- Guvernator
- orientări
- având în
- ajutor
- aici
- Înalt
- Cum
- HTTPS
- implementat
- Crește
- a crescut
- interfaţă
- IT
- cunoscut
- limbă
- Ultimele
- Bibliotecă
- Licență
- Linie
- Listă
- local
- blocat
- uitat
- Efectuarea
- Meci
- modular
- multisemn
- Altele
- prezenta
- proces
- proiect
- propunere
- public
- publica
- Inregistreaza-te
- Lansări
- raportează
- depozit
- REZULTATE
- revizuiască
- securitate
- set
- instalare
- comun
- inteligent
- Contracte inteligente
- soliditate
- Stat
- stil
- sistem
- Prin
- de-a lungul
- timp
- Unelte
- urmări
- Tranzacții
- actualizări
- us
- utilizatorii
- valoare
- Vulnerabilitățile
- Portofele
- săptămână
- OMS
- în
- cuvinte
- scris