December 6, 2021
Bevezetés
A Tally 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.
Rendszer áttekintő
The system relies on three main contracts:
- A
SafeGuard
contract template. This contract is theadmin
EgyTimelock
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
be aRegistry
. - A
Registry
which holds a list of deployedSafeGuards
with their corresponding verziószám.
A SafeGuardFactory
will basically spawn a új SafeGuard
whenever it is called. A SafeGuard
is a wrap around Timelock
művelet 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.
Frissítés: A 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
feltérképezése.
szerepek
A SafeGuard
contract defines the following roles:
Frissítés: A CREATOR_ROLE
role has been removed as a fix for the issue L02.
Kör
Ellenőriztük a kötelezettségvállalást b2c63a9dfc4090be13320d999e7c6c1d842625d3
az safeguard
repository. In scope are the smart contracts in the contracts
directory. However, the mocks
directory was deemed as out of scope.
Feltételezések
The system is not meant to be upgradeable. The Registry
address is set in the constructor az SafeGuardFactory
és mindegyik SafeGuard
lehet a 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
szerződés 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.
Kritikus súlyosság
Egyik sem.
Magas súlyosság
[H01] ETH can be locked inside the Timelock
szerződés
A Tally
team originally based their implementations on the ground of the GovernorBravoDelegate
Compound contract.
During the course of this audit, the Tally
team discovered egy korlátozás 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.
A same issue was later identified in the SafeGuard
végrehajtás 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.
Frissítés: Fixed in commit 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory can be freezed
A Registry
contract is intended to keep track of all the SafeGuards
hogy a SafeGuardFactory
termel. It has the external register
funkció which is used for this purpose.
Ugyanakkor a 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.
A SafeGuardFactory
rendelkezik a createSafeGuard
funkció, in charge of first deploying a new SafeGuard
, Akkor Egy új Timelock
with the address of the SafeGuard
as admin
, then setting the timelock
variable of the SafeGuard
contract and finally regisztrálva a SafeGuard
a nyilvántartóban.
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 létrehoz egy new
példa, 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
hívásával a register
function. This would result in the calls to createSafeGuard
nak nek visszaszáll mivel a 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
.
Frissítés: Rögzítve PR #10. The Tally team has removed the Registry
szerződés.
Közepes súlyosságú
Egyik sem.
Alacsony súlyosság
[L01] Commented out code
A Registry
szerződés tartalmazza a commented out line of code. To improve readability, consider removing it from the codebase.
Frissítés: Rögzítve PR #10 and commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] SafeGuard’s admin can assign the role of creator to any address
A 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 a grantRole
funkció AccessControlEnumerable
szerződés in the OpenZeppelin contract library, an admin can grant this role to any address. This could cause confusion because the alkotója a SafeGuard
can only be the SafeGuardFactory
.
Throughout the codebase, this role has been used only to restrict users from interacting with the setTimelock
funkció az SafeGuard
contract. By design, the system ensures that setTimelock
funkció can be called only once, Az belül SafeGuardFactory
szerződés.
Consider removing the CREATOR_ROLE
role from the SafeGuard
contract and using the onlyOwner
változás a setTimelock
funkciót.
Frissítés: Rögzítve PR #10.
[L03] Incorrect interface definition and implementation
A ISafeGuard
interface does not define the queueTransactionWithDescription
funkció ben valósul meg 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
végrehajtását.
Frissítés: Fixed in commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Missing docstrings
Some of the contracts and functions in the code base lack documentation. For example, egyes funkciók a SafeGuard
szerződés.
Additionally, some docstrings use informal language, such as the one feletti setTimelock
funkciója a SafeGuard
szerződés.
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).
Frissítés: Részben rögzítve 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:
- hozzáad
description
mint a@param
in the docstring abovequeueTransactionWithDescription
funkció - hozzáad
@param
a docstring feletticreateSafeGuard
funkciójaSafeGuardFactory
szerződés - hozzáad
@return
in docstrings above the functions inSafeGuardFactory
szerződés.
[L05] Useless or repeated code
There are places in the codebase where code is either repeated or not needed. Some examples are:
- 29-32. Sor az
Registry
contract are useless, because the_add
funkcióEnumerableSet
szerződés already performs these checks against the values already being set. - 62. vonalak, 67, 73 és a 78 az
SafeGuard
contract are all repeating the same exact operation. Consider encapsulating it into an internal function to avoid duplicating code. - 62-63. Sor és a 67-68 of
SafeGuard
are repeated. Consider encapsulating them into a single internal function. - A
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.
Frissítés: Rögzítve PR #10 and commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
Megjegyzések és további információk
[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:
- A
Registry
contract uses different styles for docstrings in the entire contract. - A
SafeGuard
contract is emitting an event whenqueueTransactionWithDescription
is called but no events are emitted in other functions dealing with transactions. - A
SafeGuard
contract, sometimes érték is used as named parameter and sometimes _érték használt.
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.
Frissítés: Rögzítve 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 irányelveket.
Frissítés: Rögzítve 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 csomag.json fájlt.
Frissítés: Rögzítve 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.
Frissítés: Rögzítve 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
az SafeGuard
szerződés.
Fontolja meg ezen elírási hibák kijavítását a kód olvashatóságának javítása érdekében.
Frissítés: Részben rögzítve 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
szerződést 69 sor, 82 sor, 96 sor és a 110 sor and “available” is misspelled on 70 sor, 83 sor, 97 sor, 111 sor. 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
. Például a eta
változó a QueueTransactionWithDescription
esemény az SafeGuard
szerződés.
Az explicititás előnyben részesítése érdekében a uint
ként kell bejelenteni uint256
.
Frissítés: Rögzítve PR #10 and commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Unused import
A SafeGuard
contract imports the console
contract but never uses it.
To improve readability of the code, consider removing any unused imports.
Frissítés: Rögzítve PR #10.
Következtetések
One high and several other minor vulnerabilities have been found and recommendations and fixes have been suggested.
- &
- hozzáférés
- Fiók
- át
- Akció
- cselekvések
- További
- cím
- admin
- Minden termék
- már
- Bár
- api
- megközelítés
- körül
- könyvvizsgálat
- Alapvetően
- hogy
- bogarak
- üzleti
- hívás
- esetek
- Okoz
- változik
- díj
- kód
- Kódolás
- Közös
- Összetett
- zavar
- megfontolás
- tartalmaz
- folytatódik
- szerződés
- szerződések
- tudott
- Teremtő
- Jelenlegi
- dátum
- foglalkozó
- Design
- fejlesztők
- különböző
- felfedezett
- Bonyolult
- ETH
- esemény
- események
- példa
- gyár
- Jellemzők
- Végül
- vezetéknév
- Rögzít
- Rugalmasság
- talált
- funkció
- alapok
- jövő
- GAS
- Giving
- cél
- kormányzás
- Kormányzó
- irányelvek
- tekintettel
- segít
- itt
- Magas
- Hogyan
- HTTPS
- végre
- Növelje
- <p></p>
- Felület
- IT
- ismert
- nyelv
- legutolsó
- könyvtár
- Engedély
- vonal
- Lista
- helyi
- zárt
- nézett
- Gyártás
- Mérkőzés
- moduláris
- multisig
- Más
- be
- folyamat
- program
- javaslat
- nyilvános
- közzétesz
- Regisztráció
- Releases
- jelentést
- raktár
- Eredmények
- Kritika
- biztonság
- készlet
- beállítás
- megosztott
- okos
- Intelligens szerződések
- szilárdság
- Állami
- stílus
- rendszer
- Keresztül
- egész
- idő
- szerszámok
- vágány
- Tranzakciók
- Frissítés
- us
- Felhasználók
- érték
- sérülékenységek
- Pénztárcák
- hét
- WHO
- belül
- szavak
- írás