6 décembre 2021
Introduction
La Pointage L'équipe nous a demandé d'examiner et d'auditer un ensemble de contrats dans le but final d'améliorer les contrats de gouvernance communs et de leur donner plus de flexibilité. Nous avons examiné le code et publions maintenant nos résultats.
Présentation du système
Le système repose sur trois contrats principaux :
- A
SafeGuard
modèle de contrat. Ce contrat est leadmin
d'unTimelock
contrat, et ajoute une couche de rôles modulaires sur leTimelock
les actions de. Ce contrat définit plusieurs rôles qui ont une responsabilité et un accès distincts sur l'état d'une proposition (mise en file d'attente, annulation et exécution). - A
SafeGuardFactory
contrat déploie un nouveauSafeGuard
et un nouveau correspondantTimelock
Contrat. Ensuite, il définit l'adresse de timelock à l'intérieur duSafeGuard
contrat et enregistre le nouveauSafeGuard
into theRegistry
. - A
Registry
qui contient une liste des déployésSafeGuards
avec leur correspondant numéro de version.
La SafeGuardFactory
va essentiellement engendrer un neufs SafeGuard
chaque fois qu'il est appelé. UN SafeGuard
est un enveloppement Timelock
qui ajoute des rôles différents et séparés pour chaque action. Les rôles sont structurés grâce à l'utilisation d'OpenZeppelin AccessControlEnumerable
contrat donnant plus de flexibilité et de compatibilité aux portefeuilles multisig et aux cas d'utilisation commerciale où plusieurs adresses peuvent avoir le même rôle partagé.
Mettre à jour: Dans le RP #10, l'équipe Tally a décidé de supprimer le Registry
Contrat. La liste des sauvegardes déployées est maintenant stockée dans le SafeGuardFactory
contrat, dans le safeGuards
ensemble énumérable, ainsi que leur version stockée dans le safeGuardVersion
cartographie.
Rôles
La SafeGuard
contrat définit les rôles suivants :
Mettre à jour: La CREATOR_ROLE
Le rôle a été supprimé en tant que correctif pour le problème L02.
Domaine
Nous avons audité l'engagement b2c63a9dfc4090be13320d999e7c6c1d842625d3
des safeguard
dépôt. Dans le champ d'application sont les contrats intelligents dans le contracts
annuaire. Cependant, le mocks
répertoire a été jugé hors de portée.
Hypothèses
Le système n'est pas censé être évolutif. Le Registry
l'adresse est définie dans le constructeur des SafeGuardFactory
et chacun SafeGuard
peut avoir le Timelock
réglé une seule fois. Cela signifie que si un nouveau Registry
est déployé un nouveau SafeGuardFactory
doit également être déployé. Si la Timelock
changements de mise en œuvre, l'ancien SafeGuard
s deviendront obsolètes, et de nouveaux devront être déployés.
De plus, le système repose fortement sur la mise en œuvre d'un Timelock
contrat qui a été jugé hors de la portée de cet audit. L'équipe n'a pas encore finalisé la mise en œuvre du Timelock
Contrat. Au moment de la rédaction de ce rapport, la mise en œuvre du timelock par le Compound est utilisé dans le projet.
La base de code a été auditée par deux auditeurs au cours d'une semaine et nous présentons ici nos conclusions.
Gravité critique
Aucun.
Haute gravité
[H01] ETH peut être verrouillé à l'intérieur du Timelock
contrat
La Tally
L'équipe a initialement basé ses implémentations sur le terrain du GovernorBravoDelegate
Contrat composé.
Au cours de cet audit, le Tally
l'équipe a découvert une limite dans le gouverneur de Compound où l'ETH a envoyé directement au Timelock
n'est pas disponible pour être utilisé par les propositions de gouvernance et, bien qu'il ne soit pas bloqué en permanence, nécessite une solution de contournement élaborée pour être récupéré.
En effet, la mise en œuvre du gouverneur nécessite que toute la valeur d'une proposition soit jointe en tant que msg.value
par le compte qui déclenche l'exécution, n'utilisant en aucune façon le Timelock
Fonds EPF.
La le même problème a été identifié plus tard dans le SafeGuard
la mise en oeuvre et l'équipe est consciente du problème et est en train de le résoudre.
Lors de la résolution du problème, envisagez d'utiliser l'approche adopté par la bibliothèque OpenZeppelin pour le même problème.
Mettre à jour: Corrigé dans le commit 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory peut être gelé
La Registry
contrat est destiné à garder une trace de tous les SafeGuards
que l' SafeGuardFactory
produit. Il a l'extérieur register
fonction qui est utilisé à cette fin.
Dans le même temps, le SafeGuardFactory
a, dans son constructeur, l'assignation du local registry
valeur à la valeur d'entrée. Il n'y a aucune possibilité de changer la valeur de registry
variable et pour cette raison, si un nouveau Registry
se déploie, une nouvelle usine doit également être déployée.
La SafeGuardFactory
a l' createSafeGuard
fonction, en charge du premier déployer un nouveau SafeGuard
, puis une nouvelle Timelock
avec l'adresse du SafeGuard
as admin
, puis régler le timelock
variable de la SafeGuard
contrat et enfin enregistrer le SafeGuard
dans le registre.
Le problème est que tout appel à createSafeGuard
peut être forcé à échouer par un attaquant qui peut enregistrer directement l'adresse déterministe du nouveau SafeGuard
avant sa création. Chaque fois qu'un contrat crée un new
instance, son nonce est augmenté et l'adresse où la nouvelle instance du contrat serait déployée peut être déterminée par l'adresse du contrat d'origine et son nonce. Par conséquent, un attaquant peut précalculer de nombreuses adresses où le nouveau SafeGuards
seront déployés et enregistrer ces adresses dans le Registry
en appelant le register
une fonction. Cela se traduirait par des appels à createSafeGuard
à revenir depuis l' Registry
contient déjà l'adresse.
Pour éviter que des acteurs externes appellent publiquement Register
contrat, envisagez de restreindre l'accès au register
fonction d'accepter les appels exclusivement par le SafeGuardFactory
.
Mettre à jour: Fixé dans RP #10. L'équipe Tally a supprimé le Registry
contrat.
Gravité moyenne
Aucun.
Faible gravité
[L01] Code commenté
La Registry
le contrat comprend une ligne de code commentée. Pour améliorer la lisibilité, envisagez de le supprimer de la base de code.
Mettre à jour: Fixé dans RP #10 et s'engager 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] L'administrateur de SafeGuard peut attribuer le rôle de créateur à n'importe quelle adresse
La SafeGuard
contrat définit le rôle d'un CREATOR_ROLE
qui, comme son nom l'indique, est attribué au créateur de la sauvegarde.
Cependant, en invoquant le grantRole
fonction de la AccessControlEnumerable
contrat dans la bibliothèque de contrats OpenZeppelin, un administrateur peut accorder ce rôle à n'importe quelle adresse. Cela pourrait prêter à confusion parce que le créateur du SafeGuard
ne peut être que le SafeGuardFactory
.
Dans toute la base de code, ce rôle a été utilisé uniquement pour empêcher les utilisateurs d'interagir avec le setTimelock
fonction des SafeGuard
Contrat. De par sa conception, le système garantit que setTimelock
fonction ne peut être appelé qu'une seule fois, de au sein du SafeGuardFactory
contrat.
Pensez à supprimer le CREATOR_ROLE
rôle de la SafeGuard
contrat et en utilisant le onlyOwner
modificateur dans l' setTimelock
la fonction.
Mettre à jour: Fixé dans RP #10.
[L03] Définition et implémentation d'interface incorrectes
La ISafeGuard
l'interface ne définit pas queueTransactionWithDescription
fonction mis en œuvre dans le SafeGuard
contrat, et en même temps, il définit les __abdicate, __queueSetTimelockPendingAdmin et __executeSetTimelockPendingAdmin fonctions mais elles ne sont pas implémentées.
Pour améliorer l'exactitude et la cohérence de la base de code, envisagez de refactoriser le ISafeGuard
interface pour correspondre exactement à la SafeGuard
la mise en œuvre.
Mettre à jour: Corrigé dans le commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Docstrings manquants
Certains contrats et fonctions de la base de code manquent de documentation. Par example, certaines fonctions dans l' SafeGuard
contrat.
De plus, certaines docstrings utilisent un langage informel, comme celui au-dessus de l' setTimelock
fonction de l' SafeGuard
contrat.
Cela empêche les examinateurs de comprendre l'intention du code, qui est fondamentale pour évaluer correctement non seulement la sécurité mais aussi l'exactitude. De plus, les docstrings améliorent la lisibilité et facilitent la maintenance. Ils doivent expliquer explicitement le but ou l'intention des fonctions, les scénarios dans lesquels elles peuvent échouer, les rôles autorisés à les appeler, les valeurs renvoyées et les événements émis.
Envisagez de bien documenter toutes les fonctions (et leurs paramètres) qui font partie de l'API publique des contrats. Les fonctions mettant en œuvre des fonctionnalités sensibles, même si elles ne sont pas publiques, doivent également être clairement documentées. Lorsque vous écrivez des docstrings, envisagez de suivre les Format de spécification naturelle Ethereum (spéc.Nat).
Mettre à jour: Partiellement fixé dans RP #10. Des docstrings appropriés ont été ajoutés à diverses fonctions dans la base de code. Cependant, en plus des modifications actuelles, envisagez d'apporter les modifications suivantes :
- Ajouter
description
car@param
dans la docstring ci-dessusqueueTransactionWithDescription
fonction - Ajouter
@param
dans l' chaîne de documentation au-dessus de l'createSafeGuard
fonctionner dansSafeGuardFactory
contrat - Ajouter
@return
dans les docstrings au-dessus des fonctions dansSafeGuardFactory
contrat.
[L05] Code inutile ou répété
Il y a des endroits dans la base de code où le code est répété ou inutile. Certains exemples sont:
- Lignes 29 à 32 des
Registry
contrat sont inutiles, car le_add
fonction de laEnumerableSet
contrat effectue déjà ces vérifications par rapport aux valeurs déjà définies. - Lignes 62, 67, 73 ainsi que 78 des
SafeGuard
contrat répètent tous exactement la même opération. Envisagez de l'encapsuler dans une fonction interne pour éviter la duplication de code. - Lignes 62 à 63 ainsi que 67-68 of
SafeGuard
sont répétés. Envisagez de les encapsuler dans une seule fonction interne. - L'utilisation de
gasleft
pour spécifier la quantité de gaz à transmettre lors de l'appel de la fonctionexecuteTransaction
est inutile. En effet, à ce point d'exécution, tout le gaz restant sera utilisé pour continuer l'exécution. Si ce n'est pas par souci d'explicitation, envisagez de supprimer legas
paramètre de l'appel.
Envisagez d'appliquer le correctif suggéré pour produire un code plus propre et améliorer la cohérence et la modularité sur la base de code.
Mettre à jour: Fixé dans RP #10 et s'engager 7fd27df16fc879d990d36a167a0b6e719e578558
.
Remarques et informations supplémentaires
[N01] Style incohérent
Il existe certains endroits dans la base de code, où les différences de style affectent la lisibilité, ce qui rend plus difficile la compréhension du code. Certains exemples sont:
- La
Registry
contract utilise différents styles pour les docstrings dans l'ensemble du contrat. - La
SafeGuard
contrat émet un événement lorsquequeueTransactionWithDescription
est appelée mais aucun événement n'est émis dans les autres fonctions traitant des transactions. - Dans le
SafeGuard
contrat, parfois Plus-value est utilisé comme paramètre nommé et parfois _valeur est utilisé.
Compte tenu de la valeur qu'un style de codage cohérent ajoute à la lisibilité du projet, envisagez d'appliquer un style de codage standard à l'aide d'outils de linter, tels que Solhint.
Mettre à jour: Fixé dans RP #10 et s'engager 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Permis manquant
Les contrats suivants dans la base de code manquent d'un identifiant de licence SPDX.
Pour faire taire les avertissements du compilateur et augmenter la cohérence dans la base de code, envisagez d'ajouter un identifiant de licence. En le faisant, pensez à vous référer à spdx.dev lignes directrices.
Mettre à jour: Fixé dans RP #10 et s'engager 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] La dépendance du contrat OpenZeppelin n'est pas épinglée
Pour éviter les comportements inattendus en cas de rupture de cas, des modifications seront publiées dans les futures mises à jour du Bibliothèque d'OpenZeppelin Contracts, pensez à épingler la version de cette dépendance dans le package.json fichier.
Mettre à jour: Fixé dans RP #10.
[N04] La version du compilateur Solidity n'est pas épinglée
Tout au long de la base de code, envisagez d'épingler la version du Compilateur de solidité à sa dernière version stable. Cela devrait aider à éviter l'introduction de bogues inattendus en raison de futures versions incompatibles. Pour choisir une version spécifique, les développeurs doivent tenir compte à la fois des fonctionnalités du compilateur nécessaires au projet et la liste des bugs connus associé à chaque version du compilateur Solidity.
Mettre à jour: Fixé dans RP #10.
[N05] Faute de frappe
À divers endroits dans la base de code, le mot role
est mal orthographié comme rol
. Un tel exemple est dans la docstring au sein de la constructor
des SafeGuard
contrat.
Pensez à corriger ces fautes de frappe pour améliorer la lisibilité du code.
Mettre à jour: Partiellement fixé dans RP #10. Alors que l'orthographe de role
a été corrigé, le commentaire "définir le rôle d'administrateur sur une adresse d'administrateur définie" devrait être "définir le rôle d'administrateur sur une adresse d'administrateur définie". De plus, "exécuter" est mal orthographié dans le SafeGuard
contrat sur ligne 69, ligne 82, ligne 96 ainsi que ligne 110 et "disponible" est mal orthographié sur ligne 70, ligne 83, ligne 97, ligne 111. Envisagez également de remplacer les mots informels tels que "va" in SafeGuard
contrat avec des alternatives formelles telles que "aller à".
[N06] Déclarer uint comme uint256
Il existe plusieurs occurrences dans la base de code où les variables sont déclarées de uint
type de données au lieu de uint256
. Par exemple, le eta
variable dans le QueueTransactionWithDescription
un événement des SafeGuard
contrat.
Pour favoriser l'explicitation, toutes les instances de uint
doit être déclaré comme uint256
.
Mettre à jour: Fixé dans RP #10 et s'engager 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Importation inutilisée
La SafeGuard
contrat importe le console
contrat mais ne l'utilise jamais.
Pour améliorer la lisibilité du code, envisagez de supprimer toutes les importations inutilisées.
Mettre à jour: Fixé dans RP #10.
Conclusions
Une vulnérabilité élevée et plusieurs autres vulnérabilités mineures ont été trouvées et des recommandations et des correctifs ont été suggérés.
- &
- accès
- Compte
- à travers
- Action
- actes
- Supplémentaire
- propos
- admin
- Tous
- déjà
- Bien que
- api
- une approche
- autour
- audit
- En gros
- va
- bogues
- la performance des entreprises
- Appelez-nous
- cas
- Causes
- Change
- charge
- code
- Codage
- Commun
- Composé
- confusion
- considération
- contient
- continuer
- contrat
- contrats
- pourriez
- créateur
- Courant
- données
- traitement
- Conception
- mobiles
- différent
- découvert
- Élaborer
- ETH
- événement
- événements
- exemple
- PERSONNEL
- Fonctionnalités:
- finalement
- Prénom
- Fixer
- Flexibilité
- trouvé
- fonction
- fonds
- avenir
- GAS
- Don
- objectif
- gouvernance
- Gouverneur
- lignes directrices
- ayant
- aider
- ici
- Haute
- Comment
- HTTPS
- mis en œuvre
- Améliore
- increased
- Interfaces
- IT
- connu
- langue
- Nouveautés
- Bibliothèque
- Licence
- Gamme
- Liste
- locales
- fermé
- regardé
- Fabrication
- Match
- application
- Multisig
- Autre
- représentent
- processus
- Projet
- proposition
- public
- publier
- vous inscrire
- de Presse
- rapport
- dépôt
- Résultats
- Avis
- sécurité
- set
- mise
- commun
- smart
- Contrats intelligents
- solidité
- Région
- Catégorie
- combustion propre
- Avec
- tout au long de
- fiable
- les outils
- suivre
- Transactions
- Actualités
- us
- utilisateurs
- Plus-value
- vulnérabilités
- Portefeuilles
- semaine
- WHO
- dans les
- des mots
- écriture