6 Aralık 2021
Giriş
The çetele team asked us to review and audit a set of contracts with the final goal to improve common governance contracts and give them more flexibility. We looked at the code and now publish our results.
Sistem görünümü
The system relies on three main contracts:
- A
SafeGuard
contract template. This contract is theadmin
bir bölgesininTimelock
contract, and adds a layer of modular roles over theTimelock
‘s actions. This contract defines several roles that have separate responsibility and access over the state of a proposal (queueing, cancellation and execution). - A
SafeGuardFactory
contract deploys a newSafeGuard
and a corresponding newTimelock
contract. Then it sets the timelock address inside theSafeGuard
contract and registers the newSafeGuard
içineRegistry
. - A
Registry
which holds a list of deployedSafeGuards
karşılık gelenleriyle Sürüm numarası.
The SafeGuardFactory
will basically spawn a yeni SafeGuard
whenever it is called. A SafeGuard
is a wrap around Timelock
operasyonlar that adds different and separated roles for each action. Roles are structured through the use of OpenZeppelin’s AccessControlEnumerable
contract giving more flexibility and compatibility to the multisig wallets and business use cases where several addresses can have the same shared role.
Güncelleme: içinde PR # 10, the Tally team has decided to remove the Registry
contract. The list of deployed safeguards is now stored within the SafeGuardFactory
contract, in the safeGuards
enumerable set, along with their version stored in the safeGuardVersion
haritalama.
Roller
The SafeGuard
contract defines the following roles:
Güncelleme: The CREATOR_ROLE
role has been removed as a fix for the issue L02.
kapsam
Taahhüdü denetledik b2c63a9dfc4090be13320d999e7c6c1d842625d3
arasında safeguard
repository. In scope are the smart contracts in the contracts
directory. However, the mocks
directory was deemed as out of scope.
varsayımlar
The system is not meant to be upgradeable. The Registry
address is set in the constructor arasında SafeGuardFactory
ve her biri SafeGuard
alabilir Timelock
set only once. This means that if a new Registry
is deployed a new SafeGuardFactory
must be deployed too. If the Timelock
implementation changes, the old SafeGuard
s will become obsolete, and new ones will have to be deployed.
Moreover, the system heavily relies on the implementation of a Timelock
sözleşme that was deemed out of scope for this audit. The team has not yet finalized the implementation of the Timelock
contract. At the time of writing this report, the Compound’s implementation of timelock is being used in the project.
The codebase has been audited by two auditors during the course of one week and here we present our findings.
Kritik şiddet
Yok.
Yüksek şiddet
[H01] ETH can be locked inside the Timelock
sözleşme
The Tally
team originally based their implementations on the ground of the GovernorBravoDelegate
Compound contract.
During the course of this audit, the Tally
team discovered a limitation in Compound’s governor where ETH sent directly to the Timelock
is not available for use by governance proposals, and although it is not permanently stuck, requires an elaborate workaround to be retrieved.
This is because the governor implementation requires all the value of a proposal to be attached as msg.value
by the account that triggers the execution, not using in any way the Timelock
ETH funds.
The same issue was later identified in the SafeGuard
uygulama and the team is aware of the issue and it is in the process of fixing it.
While fixing the issue, consider using the approach adopted by the OpenZeppelin library for the same issue.
Güncelleme: Taahhütte düzeltildi 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory can be freezed
The Registry
contract is intended to keep track of all the SafeGuards
ki SafeGuardFactory
üretir. It has the external register
işlev which is used for this purpose.
Aynı zamanda, SafeGuardFactory
has, in its constructor, the assignation of the local registry
value to the input value. There’s no possibility to change the value of the registry
variable and for this reason, if a new Registry
gets deployed, a new factory must be deployed too.
The SafeGuardFactory
olan createSafeGuard
işlev, in charge of first deploying a new SafeGuard
, Daha sonra Yeni Timelock
with the address of the SafeGuard
as admin
, then setting the timelock
variable of the SafeGuard
contract and finally kayıt SafeGuard
kayıt defterinde.
The issue is that any call to createSafeGuard
can be forced to fail by an attacker who can directly register the deterministic address of the new SafeGuard
prior to its creation. Whenever a contract oluşturur new
örnek, its nonce is increased, and the address of where the new instance of the contract would be deployed can be determined by the original contract address and its nonce. Therefore, an attacker can precalculate many of the addresses where the new SafeGuards
will be deployed and register those addresses in the Registry
arayarak register
function. This would result in the calls to createSafeGuard
için dönmek yana Registry
already contains the address.
To avoid having external actors calling publicly the Register
contract, consider restricting the access to the register
function to accept calls exclusively by the SafeGuardFactory
.
Güncelleme: Sabit PR # 10. The Tally team has removed the Registry
sözleşme.
Orta önem
Yok.
Düşük önem derecesi
[L01] Commented out code
The Registry
sözleşme içerir a commented out line of code. To improve readability, consider removing it from the codebase.
Güncelleme: Sabit PR # 10 and commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] SafeGuard’s admin can assign the role of creator to any address
The SafeGuard
contract defines the role of a CREATOR_ROLE
which, as the name suggests, is assigned to the creator of the safeguard.
However, by invoking the grantRole
işlevi AccessControlEnumerable
sözleşme in the OpenZeppelin contract library, an admin can grant this role to any address. This could cause confusion because the yaratıcısı SafeGuard
can only be the SafeGuardFactory
.
Throughout the codebase, this role has been used only to restrict users from interacting with the setTimelock
işlev arasında SafeGuard
contract. By design, the system ensures that setTimelock
işlev can be called only once, Dan içinde SafeGuardFactory
sözleşme.
Consider removing the CREATOR_ROLE
role from the SafeGuard
contract and using the onlyOwner
niteleyici içinde setTimelock
fonksiyonu.
Güncelleme: Sabit PR # 10.
[L03] Incorrect interface definition and implementation
The ISafeGuard
interface does not define the queueTransactionWithDescription
işlev içinde uygulanan SafeGuard
contract, and at the same time, it defines the __abdicate, __queueSetTimelockPendingAdmin and __executeSetTimelockPendingAdmin functions but they are not implemented.
To improve correctness and consistency in the codebase, consider refactoring the ISafeGuard
interface to match exactly the SafeGuard
uygulanması.
Güncelleme: Taahhütte düzeltildi 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Eksik dokstringler
Some of the contracts and functions in the code base lack documentation. For example, bazı fonksiyonlar içinde SafeGuard
sözleşme.
Additionally, some docstrings use informal language, such as the one Yukarıdaki setTimelock
işlevinde SafeGuard
sözleşme.
This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned and the events emitted.
Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Güncelleme: Kısmen sabitlendi PR # 10. Proper docstrings have been added to various functions throughout the code base. However, in addition to the current changes, consider making the following changes:
- Ekle
description
gibi@param
in the docstring abovequeueTransactionWithDescription
işlev - Ekle
@param
içinde belge YukarıdakicreateSafeGuard
içinde işlemekSafeGuardFactory
sözleşme - Ekle
@return
in docstrings above the functions inSafeGuardFactory
sözleşme.
[L05] Useless or repeated code
There are places in the codebase where code is either repeated or not needed. Some examples are:
- 29-32 Satırları arasında
Registry
contract are useless, because the_add
işleviEnumerableSet
sözleşme already performs these checks against the values already being set. - Çizgiler 62, 67, 73 ve 78 arasında
SafeGuard
contract are all repeating the same exact operation. Consider encapsulating it into an internal function to avoid duplicating code. - 62-63 Satırları ve 67-68 of
SafeGuard
are repeated. Consider encapsulating them into a single internal function. - Kullanımı
gasleft
to specify how much gas should be forwarded in the call of the functionexecuteTransaction
is unnecessary. This is because, at that point of execution, the entire gas left will be used to continue the execution. If this is not for expliciteness, consider removing thegas
parameter from the call.
Consider applying the suggested fixed to produce a cleaner code and improve consistency and modularity over the codebase.
Güncelleme: Sabit PR # 10 and commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
Notlar ve Ek Bilgiler
[N01] Inconsistent style
There are some places in the code base, where differences in style affect the readability, making it more difficult to understand the code. Some examples are:
- The
Registry
contract uses different styles for docstrings in the entire contract. - The
SafeGuard
contract is emitting an event whenqueueTransactionWithDescription
is called but no events are emitted in other functions dealing with transactions. - içinde
SafeGuard
contract, sometimes değer is used as named parameter and sometimes _değer kullanıldı.
Taking into consideration the value a consistent coding style adds to the project’s readability, consider enforcing a standard coding style with help of linter tools, such as Solhint.
Güncelleme: Sabit PR # 10 and commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Missing license
The following contracts within the code base are missing an SPDX license identifier.
To silence compiler warnings and increase consistency across the codebase consider adding a license identifier. While doing it consider referring to spdx.dev kılavuzlar.
Güncelleme: Sabit PR # 10 and commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] OpenZeppelin Contract’s dependency is not pinned
To prevent unexpected behaviors in case breaking changes are released in future updates of the OpenZeppelin Contracts’ library, consider pinning the version of this dependency in the paket.json dosyası.
Güncelleme: Sabit PR # 10.
[N04] Solidity compiler version is not pinned
Throughout the code base, consider pinning the version of the Solidity compiler to its latest stable version. This should help prevent introducing unexpected bugs due to incompatible future releases. To choose a specific version, developers should consider both the compiler’s features needed by the project and the list of known bugs associated with each Solidity compiler version.
Güncelleme: Sabit PR # 10.
[N05] Typo
At various instances throughout the code base, the word role
is misspelled as rol
. One such example is in the docstring within the constructor
arasında SafeGuard
sözleşme.
Kodun okunabilirliğini artırmak için bu yazım hatalarını düzeltmeyi düşünün.
Güncelleme: Kısmen sabitlendi PR # 10. While the spelling of role
has been corrected, the comment “set admin role the an defined admin address” should be “set admin role to a defined admin address”. Additionally, “execute” is misspelled in the SafeGuard
sözleşme çizgi 69, çizgi 82, çizgi 96 ve çizgi 110 and “available” is misspelled on çizgi 70, çizgi 83, çizgi 97, çizgi 111. Also, consider replacing informal words such as “gonna” in SafeGuard
contract with formal alternatives such as “going to”.
[N06] Declare uint as uint256
There are several occurrences in the codebase where variables are declared of uint
data type instead of uint256
. Örneğin, eta
değişken QueueTransactionWithDescription
olay arasında SafeGuard
sözleşme.
To favor explicitness, all instances of uint
should be declared as uint256
.
Güncelleme: Sabit PR # 10 and commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Unused import
The SafeGuard
contract imports the console
contract but never uses it.
To improve readability of the code, consider removing any unused imports.
Güncelleme: Sabit PR # 10.
Sonuç
One high and several other minor vulnerabilities have been found and recommendations and fixes have been suggested.
- &
- erişim
- Hesap
- karşısında
- Action
- eylemler
- Ek
- adres
- Gizem
- Türkiye
- zaten
- Rağmen
- api
- yaklaşım
- etrafında
- denetim
- temel olarak
- olmak
- böcek
- iş
- çağrı
- durumlarda
- Sebeb olmak
- değişiklik
- ücret
- kod
- kodlama
- ortak
- Bileşik
- karışıklık
- dikkate
- içeren
- devam etmek
- sözleşme
- sözleşmeleri
- olabilir
- yaratıcı
- akım
- veri
- ilgili
- Dizayn
- geliştiriciler
- farklı
- keşfetti
- ayrıntılı
- ETH
- Etkinlikler
- olaylar
- örnek
- fabrika
- Özellikler
- Nihayet
- Ad
- sabit
- Esneklik
- bulundu
- işlev
- para
- gelecek
- GAZ
- Verilmesi
- gol
- yönetim
- vali
- kuralları yenileyerek
- sahip olan
- yardım et
- okuyun
- Yüksek
- Ne kadar
- HTTPS
- uygulanan
- Artırmak
- artmış
- arayüzey
- IT
- bilinen
- dil
- son
- Kütüphane
- Lisans
- çizgi
- Liste
- yerel
- kilitli
- baktı
- Yapımı
- Maç
- modüler
- çoklu imza
- Diğer
- mevcut
- süreç
- proje
- öneri
- halka açık
- yayınlamak
- kayıt olmak
- Bildirileri
- rapor
- Depo
- Sonuçlar
- yorum
- güvenlik
- set
- ayar
- Paylaşılan
- akıllı
- Akıllı Sözleşmeler
- katılık
- Eyalet
- stil
- sistem
- İçinden
- boyunca
- zaman
- araçlar
- iz
- işlemler
- Güncellemeler
- us
- kullanıcılar
- değer
- güvenlik açıkları
- Cüzdan
- hafta
- DSÖ
- içinde
- sözler
- yazı yazıyor