Dicembre 6, 2021
Introduzione
I conteggio ci ha chiesto di rivedere e verificare una serie di contratti con l’obiettivo finale di migliorare i contratti di governance comuni e dare loro maggiore flessibilità. Abbiamo esaminato il codice e ora pubblichiamo i nostri risultati.
Panoramica del sistema
Il sistema si basa su tre contratti principali:
- A
SafeGuard
modello di contratto. Questo contratto è iladmin
di unoTimelock
contratto e aggiunge uno strato di ruoli modulari sulTimelock
le azioni di. Questo contratto definisce diversi ruoli che hanno responsabilità e accesso separati sullo stato di una proposta (accodamento, cancellazione ed esecuzione). - A
SafeGuardFactory
il contratto distribuisce un nuovoSafeGuard
e un nuovo corrispondenteTimelock
contrarre. Quindi imposta l'indirizzo del blocco temporale all'interno del fileSafeGuard
contratto e registra il nuovoSafeGuard
nellaRegistry
. - A
Registry
che contiene un elenco di distribuitiSafeGuards
con il loro corrispondente numero di versione.
I SafeGuardFactory
fondamentalmente genererà a nuovi SafeGuard
ogni volta che viene chiamato. UN SafeGuard
è una conclusione Timelock
operazioni che aggiunge ruoli diversi e separati per ogni azione. I ruoli sono strutturati attraverso l'uso di OpenZeppelin AccessControlEnumerable
contratto offrendo maggiore flessibilità e compatibilità ai portafogli multisig e ai casi d'uso aziendali in cui più indirizzi possono avere lo stesso ruolo condiviso.
Aggiornare: Nel PR # 10, il team di Tally ha deciso di rimuovere il file Registry
contrarre. L'elenco delle protezioni implementate è ora archiviato nel file SafeGuardFactory
contratto, nel safeGuards
insieme enumerabile, insieme alla loro versione archiviata nel file safeGuardVersion
Mappatura.
Ruoli
I SafeGuard
il contratto definisce i seguenti ruoli:
Aggiornare: I CREATOR_ROLE
il ruolo è stato rimosso come correzione per il problema L02.
Obbiettivo
Abbiamo verificato il commit b2c63a9dfc4090be13320d999e7c6c1d842625d3
della safeguard
deposito. Nell'ambito sono i contratti intelligenti nel contracts
directory. comunque, il mocks
la directory è stata considerata fuori ambito.
Ipotesi
Il sistema non è pensato per essere aggiornabile. IL Registry
l'indirizzo è impostato nel costruttore della SafeGuardFactory
e ciascuno SafeGuard
può avere il Timelock
impostare una sola volta. Ciò significa che se un nuovo Registry
viene distribuito un nuovo SafeGuardFactory
deve essere schierato anch'esso. Se la Timelock
cambiamenti di implementazione, il vecchio SafeGuard
diventeranno obsoleti e sarà necessario implementarne di nuovi.
Inoltre, il sistema si basa fortemente sull’implementazione di a Timelock
contratto ritenuto estraneo all’ambito del presente audit. Il team non ha ancora finalizzato l'implementazione del Timelock
contrarre. Al momento della stesura di questo rapporto, l'implementazione del timelock da parte del Compound viene utilizzato nel progetto.
Il codice base è stato controllato da due revisori nel corso di una settimana e qui presentiamo i nostri risultati.
Gravità critica
Nessuno.
Elevata gravità
[H01] ETH può essere bloccato all'interno del Timelock
contratto
I Tally
il team originariamente basava le proprie implementazioni sul terreno GovernorBravoDelegate
Contratto composto.
Nel corso di tale audit, il Tally
squadra scoperta una limitazione nel governatore di Compound dove ETH veniva inviato direttamente al Timelock
non è disponibile per l'utilizzo da parte delle proposte di governance e, sebbene non sia permanentemente bloccato, richiede un'elaborata soluzione alternativa per essere recuperato.
Questo perché l'implementazione del governatore richiede che tutto il valore di una proposta venga allegato msg.value
dall'account che attiva l'esecuzione, non utilizzando in alcun modo il Timelock
Fondi dell'ETH.
I lo stesso problema è stato successivamente identificato nel SafeGuard
implementazione e il team è a conoscenza del problema ed è in procinto di risolverlo.
Mentre risolvi il problema, considera l'utilizzo dell'approccio adottato dalla libreria OpenZeppelin per lo stesso problema.
Aggiornare: Risolto il problema con il commit 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory può essere bloccato
I Registry
contratto è destinato a tenere traccia di tutti i SafeGuards
che l' SafeGuardFactory
produce. Ha l'esterno register
function che viene utilizzato per questo scopo.
Allo stesso tempo, il SafeGuardFactory
ha, nel suo costruttore, l'assegnazione del local registry
valore al valore di input. Non è possibile modificare il valore di registry
variabile e per questo motivo, se un nuovo Registry
viene implementato, deve essere implementata anche una nuova fabbrica.
I SafeGuardFactory
ha la createSafeGuard
function, responsabile del primo distribuendo un nuovo SafeGuard
, poi una nuova Timelock
con l'indirizzo del SafeGuard
as admin
, quindi impostando il timelock
variabile del SafeGuard
contratto e infine registrazione del SafeGuard
nel registro.
Il problema è che qualsiasi chiamata a createSafeGuard
può essere costretto a fallire da un utente malintenzionato che può registrare direttamente l'indirizzo deterministico del nuovo SafeGuard
prima della sua creazione. Ogni volta che un contratto crea a new
esempio, il suo nonce viene aumentato e l'indirizzo in cui verrà distribuita la nuova istanza del contratto può essere determinato dall'indirizzo del contratto originale e dal suo nonce. Pertanto, un utente malintenzionato può precalcolare molti degli indirizzi in cui si trova il file new SafeGuards
verrà distribuito e registrerà tali indirizzi nel file Registry
chiamando il register
funzione. Ciò comporterebbe le chiamate a createSafeGuard
a ritornare poiché l' Registry
contiene già l'indirizzo.
Per evitare che attori esterni chiamino pubblicamente il Register
contratto, valutare la possibilità di limitare l'accesso al file register
funzione per accettare chiamate esclusivamente da parte del SafeGuardFactory
.
Aggiornare: Risolto in PR # 10. Il team Tally ha rimosso il file Registry
contrarre.
Gravità media
Nessuno.
Bassa gravità
[L01] Codice commentato
I Registry
contratto include una riga di codice commentata. Per migliorare la leggibilità, valuta la possibilità di rimuoverlo dalla codebase.
Aggiornare: Risolto in PR # 10 e impegnarsi 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] L'amministratore di SafeGuard può assegnare il ruolo di creatore a qualsiasi indirizzo
I SafeGuard
il contratto definisce il ruolo di a CREATOR_ROLE
che, come suggerisce il nome, è assegnato all'ideatore della tutela.
Tuttavia, invocando , il grantRole
funzione del AccessControlEnumerable
contratto nella libreria dei contratti OpenZeppelin, un amministratore può concedere questo ruolo a qualsiasi indirizzo. Ciò potrebbe causare confusione perché il creatore del SafeGuard
non può che essere il SafeGuardFactory
.
In tutto il codice base, questo ruolo è stato utilizzato solo per limitare gli utenti dall'interagire con il file setTimelock
function della SafeGuard
contrarre. In base alla progettazione, il sistema lo garantisce setTimelock
function può essere chiamato una sola volta,da all'interno SafeGuardFactory
contratto.
Valuta la possibilità di rimuovere il CREATOR_ROLE
ruolo da SafeGuard
contratto e utilizzando il onlyOwner
modificatore nel setTimelock
funzione.
Aggiornare: Risolto in PR # 10.
[L03] Definizione e implementazione dell'interfaccia errate
I ISafeGuard
l'interfaccia non definisce il file queueTransactionWithDescription
function implementato nel SafeGuard
contratto e, allo stesso tempo, ne definisce il __abdicate, __queueSetTimelockPendingAdmin e __executeSetTimelockPendingAdmin funzioni ma non sono implementate.
Per migliorare la correttezza e la coerenza della base di codice, prendere in considerazione il refactoring del file ISafeGuard
interfaccia per corrispondere esattamente al file SafeGuard
attuazione.
Aggiornare: Risolto il problema con il commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Docstring mancanti
Alcuni contratti e funzioni nel codice base mancano di documentazione. Per esempio, alcune funzioni nel SafeGuard
contrarre.
Inoltre, alcune docstring utilizzano un linguaggio informale, come quello sopra la setTimelock
funzione nel SafeGuard
contratto.
Ciò ostacola la comprensione da parte dei revisori delle intenzioni del codice, che è fondamentale per valutare correttamente non solo la sicurezza ma anche la correttezza. Inoltre, le docstring migliorano la leggibilità e facilitano la manutenzione. Dovrebbero spiegare esplicitamente lo scopo o l'intenzione delle funzioni, gli scenari in cui possono fallire, i ruoli autorizzati a chiamarle, i valori restituiti e gli eventi emessi.
Prendi in considerazione la possibilità di documentare accuratamente tutte le funzioni (e i relativi parametri) che fanno parte dell'API pubblica dei contratti. Anche le funzioni che implementano funzionalità sensibili, anche se non pubbliche, dovrebbero essere chiaramente documentate. Quando scrivi docstring, considera di seguire il file Formato delle specifiche naturali di Ethereum (Specifiche Nazionali).
Aggiornare: Parzialmente fissato in PR # 10. Le docstring appropriate sono state aggiunte a varie funzioni in tutto il codice base. Tuttavia, oltre alle modifiche attuali, valuta la possibilità di apportare le seguenti modifiche:
- Aggiungi
description
la@param
nella stringa di documento sopraqueueTransactionWithDescription
function - Aggiungi
@param
nel docstring sopra lacreateSafeGuard
funzione inSafeGuardFactory
contratto - Aggiungi
@return
in docstring sopra le funzioni inSafeGuardFactory
contrarre.
[L05] Codice inutile o ripetuto
Ci sono punti nella base di codice in cui il codice viene ripetuto o non è necessario. Alcuni esempi sono:
- Linee 29-32 della
Registry
contratto sono inutili, perché il_add
funzione delEnumerableSet
contratto esegue già questi controlli rispetto ai valori già fissati. - Linee 62, 67, 73 ed 78 della
SafeGuard
contratto stanno ripetendo tutti la stessa identica operazione. Valuta la possibilità di incapsularlo in una funzione interna per evitare la duplicazione del codice. - Linee 62-63 ed 67-68 of
SafeGuard
vengono ripetuti. Considera l'idea di incapsularli in un'unica funzione interna. - L'uso di
gasleft
specificare quanto gas deve essere inoltrato nella chiamata della funzioneexecuteTransaction
non è necessario. Questo perché, a quel punto dell'esecuzione, tutto il gas rimasto verrà utilizzato per proseguire l'esecuzione. Se questo non è per esplicitezza, valuta la possibilità di rimuovere il filegas
parametro dalla chiamata.
Prendi in considerazione l'applicazione della soluzione suggerita per produrre un codice più pulito e migliorare la coerenza e la modularità della base di codice.
Aggiornare: Risolto in PR # 10 e impegnarsi 7fd27df16fc879d990d36a167a0b6e719e578558
.
Note e informazioni aggiuntive
[N01] Stile incoerente
Ci sono alcuni punti nel codice base in cui le differenze di stile influiscono sulla leggibilità, rendendo più difficile la comprensione del codice. Alcuni esempi sono:
- I
Registry
Il contratto utilizza stili diversi per le docstring nell'intero contratto. - I
SafeGuard
il contratto emette un evento quandoqueueTransactionWithDescription
viene chiamato ma non viene emesso alcun evento in altre funzioni che si occupano delle transazioni. - Nel
SafeGuard
contratto, a volte APPREZZIAMO viene utilizzato come parametro denominato e talvolta _valore viene utilizzato.
Prendendo in considerazione il valore che uno stile di codifica coerente aggiunge alla leggibilità del progetto, valuta la possibilità di applicare uno stile di codifica standard con l'aiuto di strumenti linter, come Solhint.
Aggiornare: Risolto in PR # 10 e impegnarsi 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Licenza mancante
Ai seguenti contratti all'interno della codebase manca un identificatore di licenza SPDX.
Per silenziare gli avvisi del compilatore e aumentare la coerenza nella codebase, prendi in considerazione l'aggiunta di un identificatore di licenza. Mentre lo fai, considera di fare riferimento a spdx.dev linee guida.
Aggiornare: Risolto in PR # 10 e impegnarsi 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] La dipendenza di OpenZeppelin Contract non è bloccata
Per evitare comportamenti imprevisti nel caso in cui vengano rilasciate modifiche di rilievo nei futuri aggiornamenti di Libreria di OpenZeppelin Contracts, valuta la possibilità di bloccare la versione di questa dipendenza in pacchetto.json file.
Aggiornare: Risolto in PR # 10.
[N04] La versione del compilatore Solidity non è bloccata
In tutta la base di codice, valutare la possibilità di aggiungere la versione del file Compilatore di solidità alla sua ultima versione stabile. Ciò dovrebbe aiutare a prevenire l'introduzione di bug imprevisti dovuti a versioni future incompatibili. Per scegliere una versione specifica, gli sviluppatori dovrebbero considerare sia le funzionalità del compilatore necessarie al progetto sia l'elenco dei bug noti associato a ciascuna versione del compilatore Solidity.
Aggiornare: Risolto in PR # 10.
[N05] Errore di battitura
In vari casi in tutto il codice base, la parola role
è scritto male come rol
. Uno di questi esempi è in la docstring all'interno del file constructor
della SafeGuard
contratto.
Valuta la possibilità di correggere questi errori di battitura per migliorare la leggibilità del codice.
Aggiornare: Parzialmente fissato in PR # 10. Mentre l'ortografia di role
è stato corretto, il commento "imposta il ruolo di amministratore su un indirizzo di amministratore definito" dovrebbe essere "imposta il ruolo di amministratore su un indirizzo di amministratore definito". Inoltre, "execute" è scritto in modo errato nel file SafeGuard
contratto in corso Linea 69, Linea 82, Linea 96 ed Linea 110 e "disponibile" è scritto in modo errato Linea 70, Linea 83, Linea 97, Linea 111. Inoltre, valuta la possibilità di sostituire parole informali come "andando" in SafeGuard
contratto con alternative formali come “andare a”.
[N06] Dichiara uint come uint256
Esistono diverse occorrenze nel codebase in cui vengono dichiarate le variabili uint
tipo di dati invece di uint256
. Ad esempio, il eta
variabile in QueueTransactionWithDescription
evento della SafeGuard
contrarre.
Per favorire l'esplicitezza, tutte le istanze di uint
dovrebbe essere dichiarato come uint256
.
Aggiornare: Risolto in PR # 10 e impegnarsi 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Importazione non utilizzata
I SafeGuard
il contratto importa il console
contratto ma non lo usa mai.
Per migliorare la leggibilità del codice, valuta la possibilità di rimuovere eventuali importazioni inutilizzate.
Aggiornare: Risolto in PR # 10.
Conclusioni
Sono state rilevate una vulnerabilità elevata e diverse altre minori e sono stati suggeriti consigli e soluzioni.
- &
- accesso
- Il mio account
- operanti in
- Action
- azioni
- aggiuntivo
- indirizzo
- Admin
- Tutti
- già
- Sebbene il
- api
- approccio
- in giro
- revisione
- fondamentalmente
- essendo
- bug
- affari
- chiamata
- casi
- Causare
- il cambiamento
- carica
- codice
- codifica
- Uncommon
- Compound
- confusione
- considerazione
- contiene
- continua
- contratto
- contratti
- potuto
- Creatore
- Corrente
- dati
- trattare
- Design
- sviluppatori
- diverso
- scoperto
- Elaborare
- ETH
- Evento
- eventi
- esempio
- fabbrica
- Caratteristiche
- Infine
- Nome
- Fissare
- Flessibilità
- essere trovato
- function
- fondi
- futuro
- GAS
- Dare
- scopo
- la governance
- Governatore
- linee guida
- avendo
- Aiuto
- qui
- Alta
- Come
- HTTPS
- implementato
- Aumento
- è aumentato
- Interfaccia
- IT
- conosciuto
- Lingua
- con i più recenti
- Biblioteca
- Licenza
- linea
- Lista
- locale
- bloccato
- guardò
- Fare
- partita
- componibile
- Multisig
- Altro
- presenti
- processi
- progetto
- proposta
- la percezione
- pubblicare
- registro
- Uscite
- rapporto
- deposito
- Risultati
- recensioni
- problemi di
- set
- regolazione
- condiviso
- smart
- Smart Contract
- solidità
- Regione / Stato
- style
- sistema
- Attraverso
- per tutto
- tempo
- strumenti
- pista
- Le transazioni
- Aggiornamenti
- us
- utenti
- APPREZZIAMO
- vulnerabilità
- Portafogli
- settimana
- OMS
- entro
- parole
- scrittura