Desember 6, 2021
Pengantar
Grafik Menghitung tim meminta kami untuk meninjau dan mengaudit serangkaian kontrak dengan tujuan akhir untuk meningkatkan kontrak tata kelola bersama dan memberi mereka lebih banyak fleksibilitas. Kami melihat kode dan sekarang mempublikasikan hasil kami.
Ikhtisar sistem
Sistem ini bergantung pada tiga kontrak utama:
- A
SafeGuard
templat kontrak. Kontrak ini adalahadmin
dariTimelock
kontrak, dan menambahkan lapisan peran modular di atasTimelock
tindakan. Kontrak ini mendefinisikan beberapa peran yang memiliki tanggung jawab dan akses terpisah atas status proposal (antrean, pembatalan, dan eksekusi). - A
SafeGuardFactory
kontrak menyebarkan yang baruSafeGuard
dan yang baru yang sesuaiTimelock
kontrak. Kemudian ia menetapkan alamat kunci waktu di dalamSafeGuard
kontrak dan daftarkan yang baruSafeGuard
ke dalamRegistry
. - A
Registry
yang menyimpan daftar yang dikerahkanSafeGuards
dengan mereka yang sesuai nomor versi.
Grafik SafeGuardFactory
pada dasarnya akan menelurkan yang baru SafeGuard
kapanpun dipanggil. A SafeGuard
adalah membungkus Timelock
operasi yang menambahkan peran yang berbeda dan terpisah untuk setiap tindakan. Peran disusun melalui penggunaan OpenZeppelin's AccessControlEnumerable
contract memberikan lebih banyak fleksibilitas dan kompatibilitas ke dompet multisig dan kasus penggunaan bisnis di mana beberapa alamat dapat memiliki peran bersama yang sama.
Update: Dalam majalah PR # 10, tim Tally telah memutuskan untuk menghapus Registry
kontrak. Daftar pengamanan yang diterapkan sekarang disimpan di dalam SafeGuardFactory
kontrak, dalam safeGuards
set enumerable, bersama dengan versi mereka disimpan di safeGuardVersion
pemetaan.
Peran
Grafik SafeGuard
kontrak mendefinisikan peran berikut:
Update: Grafik CREATOR_ROLE
peran telah dihapus sebagai perbaikan untuk masalah L02.
Cakupan
Kami mengaudit komit b2c63a9dfc4090be13320d999e7c6c1d842625d3
dari safeguard
gudang. Dalam ruang lingkup adalah kontrak pintar di contracts
direktori. Namun, mocks
direktori dianggap di luar ruang lingkup.
Asumsi
Sistem ini tidak dimaksudkan untuk dapat diupgrade. Itu Registry
alamat sudah ditentukan dalam konstruktor dari SafeGuardFactory
dan masing-masing SafeGuard
dapat memiliki Timelock
ditetapkan hanya sekali. Artinya jika baru Registry
dikerahkan yang baru SafeGuardFactory
harus dikerahkan juga. jika Timelock
perubahan implementasi, yang lama SafeGuard
s akan menjadi usang, dan yang baru harus digunakan.
Selain itu, sistem ini sangat bergantung pada implementasi a Timelock
kontrak yang dianggap di luar ruang lingkup audit ini. Tim belum menyelesaikan implementasi Timelock
kontrak. Pada saat penulisan laporan ini, implementasi Timelock dari Compound sedang digunakan dalam proyek.
Basis kode telah diaudit oleh dua auditor selama satu minggu dan di sini kami menyajikan temuan kami.
Tingkat keparahan kritis
Tidak ada.
Keparahan tinggi
[H01] ETH dapat dikunci di dalam Timelock
kontrak
Grafik Tally
tim awalnya mendasarkan implementasi mereka di atas dasar GovernorBravoDelegate
Kontrak majemuk.
Selama audit ini, Tally
tim ditemukan batasan di gubernur Compound tempat ETH dikirim langsung ke Timelock
tidak tersedia untuk digunakan oleh proposal tata kelola, dan meskipun tidak macet secara permanen, memerlukan solusi yang rumit untuk diambil.
Hal ini karena pelaksanaan gubernur mengharuskan semua nilai proposal dilampirkan sebagai msg.value
oleh akun yang memicu eksekusi, tidak menggunakan cara apa pun Timelock
dana ETH.
Grafik masalah yang sama kemudian diidentifikasi dalam SafeGuard
implementasi dan tim mengetahui masalah tersebut dan sedang dalam proses memperbaikinya.
Saat memperbaiki masalah, pertimbangkan untuk menggunakan pendekatan diadopsi oleh perpustakaan OpenZeppelin untuk masalah yang sama.
Update: Tetap di komit 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory dapat dibekukan
Grafik Registry
kontrak dimaksudkan untuk melacak semua SafeGuards
bahwa SafeGuardFactory
menghasilkan. Ini memiliki eksternal register
fungsi yang digunakan untuk tujuan ini.
Pada saat yang sama, SafeGuardFactory
memiliki, dalam konstruktornya, penetapan lokal registry
nilai ke nilai input. Tidak ada kemungkinan untuk mengubah nilai registry
variabel dan untuk alasan ini, jika new Registry
akan dikerahkan, pabrik baru harus dikerahkan juga.
Grafik SafeGuardFactory
memiliki createSafeGuard
fungsi, bertanggung jawab pertama menyebarkan yang baru SafeGuard
, kemudian baru Timelock
dengan alamat SafeGuard
as admin
, lalu atur timelock
variabel dari SafeGuard
kontrak dan akhirnya mendaftarkan SafeGuard
dalam registri.
Masalahnya adalah bahwa setiap panggilan ke createSafeGuard
dapat dipaksa untuk gagal oleh penyerang yang dapat langsung mendaftarkan alamat deterministik yang baru SafeGuard
sebelum penciptaannya. Kapanpun kontrak menciptakan a new
contoh, nonce-nya ditingkatkan, dan alamat tempat instance baru kontrak akan disebarkan dapat ditentukan oleh alamat kontrak asli dan nonce-nya. Oleh karena itu, penyerang dapat menghitung lebih dulu banyak alamat di mana SafeGuards
akan dikerahkan dan mendaftarkan alamat-alamat itu di Registry
dengan memanggil register
fungsi. Ini akan menghasilkan panggilan ke createSafeGuard
untuk kembali sejak Registry
sudah berisi alamatnya.
Untuk menghindari aktor eksternal memanggil secara terbuka Register
kontrak, pertimbangkan untuk membatasi akses ke register
berfungsi untuk menerima panggilan secara eksklusif oleh SafeGuardFactory
.
Update: Diperbaiki PR # 10. Tim Tally telah menghapus Registry
kontrak.
Keparahan sedang
Tidak ada.
Tingkat keparahan rendah
[L01] Mengomentari kode
Grafik Registry
kontrak termasuk baris kode yang dikomentari. Untuk meningkatkan keterbacaan, pertimbangkan untuk menghapusnya dari basis kode.
Update: Diperbaiki PR # 10 dan berkomitmen 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] Admin SafeGuard dapat menetapkan peran pembuat ke alamat mana pun
Grafik SafeGuard
kontrak mendefinisikan peran a CREATOR_ROLE
yang, seperti namanya, ditugaskan kepada pencipta perlindungan.
Namun, dengan memanggil itu grantRole
fungsi dari AccessControlEnumerable
kontrak di perpustakaan kontrak OpenZeppelin, admin dapat memberikan peran ini ke alamat mana pun. Hal ini dapat menyebabkan kebingungan karena pencipta SafeGuard
hanya bisa menjadi SafeGuardFactory
.
Di seluruh basis kode, peran ini hanya digunakan untuk membatasi pengguna berinteraksi dengan setTimelock
fungsi dari SafeGuard
kontrak. Dengan desain, sistem memastikan bahwa setTimelock
fungsi hanya bisa dipanggil sekali, Dari dalam SafeGuardFactory
kontrak.
Pertimbangkan untuk menghapus CREATOR_ROLE
peran dari SafeGuard
kontrak dan menggunakan onlyOwner
pengubah dalam setTimelock
fungsi.
Update: Diperbaiki PR # 10.
[L03] Definisi dan implementasi antarmuka yang salah
Grafik ISafeGuard
antarmuka tidak mendefinisikan queueTransactionWithDescription
fungsi dilaksanakan di SafeGuard
kontrak, dan pada saat yang sama, itu mendefinisikan __abdicate, __queueSetTimelockPendingAdmin dan __executeSetTimelockPendingAdmin fungsi tetapi tidak diimplementasikan.
Untuk meningkatkan kebenaran dan konsistensi dalam basis kode, pertimbangkan untuk memfaktorkan ulang ISafeGuard
antarmuka untuk mencocokkan persis dengan SafeGuard
implementasi.
Update: Tetap di komit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Dokumen hilang
Beberapa kontrak dan fungsi dalam basis kode tidak memiliki dokumentasi. Sebagai contoh, beberapa fungsi dalam SafeGuard
kontrak.
Selain itu, beberapa docstrings menggunakan bahasa informal, seperti yang atas setTimelock
fungsi di SafeGuard
kontrak.
Ini menghalangi pemahaman pengulas tentang maksud kode, yang merupakan dasar untuk menilai dengan benar tidak hanya keamanan tetapi juga kebenarannya. Selain itu, docstrings meningkatkan keterbacaan dan memudahkan perawatan. Mereka harus secara eksplisit menjelaskan tujuan atau maksud dari fungsi, skenario di mana mereka dapat gagal, peran yang diizinkan untuk memanggil mereka, nilai yang dikembalikan dan peristiwa yang dipancarkan.
Pertimbangkan untuk mendokumentasikan semua fungsi (dan parameternya) secara menyeluruh yang merupakan bagian dari API publik kontrak. Fungsi yang mengimplementasikan fungsionalitas sensitif, meskipun tidak bersifat publik, juga harus didokumentasikan dengan jelas. Saat menulis docstrings, pertimbangkan untuk mengikuti Format Spesifikasi Ethereum Natural (Spesifikasi Nat).
Update: Diperbaiki sebagian PR # 10. Docstrings yang tepat telah ditambahkan ke berbagai fungsi di seluruh basis kode. Namun, selain perubahan saat ini, pertimbangkan untuk membuat perubahan berikut:
- Add
description
sebagai@param
dalam docstring di atasqueueTransactionWithDescription
fungsi - Add
@param
dalam doktrin atascreateSafeGuard
fungsi diSafeGuardFactory
kontrak - Add
@return
dalam docstrings di atas fungsi diSafeGuardFactory
kontrak.
[L05] Kode yang tidak berguna atau berulang
Ada tempat di basis kode di mana kode diulang atau tidak diperlukan. Beberapa contohnya adalah:
- Baris 29-32 dari
Registry
kontrak tidak berguna, karena_add
fungsi dariEnumerableSet
kontrak sudah melakukan pemeriksaan ini terhadap nilai-nilai yang sudah ditetapkan. - Garis 62, 67, 73 dan 78 dari
SafeGuard
kontrak semua mengulangi operasi yang sama persis. Pertimbangkan untuk mengenkapsulasinya ke dalam fungsi internal untuk menghindari duplikasi kode. - Baris 62-63 dan 67-68 of
SafeGuard
diulang. Pertimbangkan untuk mengenkapsulasinya menjadi satu fungsi internal. - Penggunaan
gasleft
untuk menentukan berapa banyak gas yang harus diteruskan dalam panggilan fungsiexecuteTransaction
tidak perlu. Sebab, pada titik eksekusi tersebut, seluruh sisa gas akan digunakan untuk melanjutkan eksekusi. Jika ini bukan untuk eksplisit, pertimbangkan untuk menghapusgas
parameter dari panggilan.
Pertimbangkan untuk menerapkan perbaikan yang disarankan untuk menghasilkan kode yang lebih bersih dan meningkatkan konsistensi dan modularitas pada basis kode.
Update: Diperbaiki PR # 10 dan berkomitmen 7fd27df16fc879d990d36a167a0b6e719e578558
.
Catatan & Informasi Tambahan
[N01] Gaya tidak konsisten
Ada beberapa tempat di basis kode, di mana perbedaan gaya mempengaruhi keterbacaan, sehingga lebih sulit untuk memahami kode. Beberapa contohnya adalah:
- Grafik
Registry
kontrak menggunakan gaya yang berbeda untuk docstrings di seluruh kontrak. - Grafik
SafeGuard
kontrak memancarkan suatu peristiwa ketikaqueueTransactionWithDescription
dipanggil tetapi tidak ada peristiwa yang dipancarkan dalam fungsi lain yang berhubungan dengan transaksi. - Dalam majalah
SafeGuard
kontrak, kadang-kadang nilai digunakan sebagai parameter bernama dan terkadang _nilai digunakan.
Mempertimbangkan nilai gaya pengkodean yang konsisten menambah keterbacaan proyek, pertimbangkan untuk menerapkan gaya pengkodean standar dengan bantuan alat linter, seperti Solhint.
Update: Diperbaiki PR # 10 dan berkomitmen 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Lisensi tidak ada
Kontrak berikut dalam basis kode tidak memiliki pengidentifikasi lisensi SPDX.
Untuk membungkam peringatan kompiler dan meningkatkan konsistensi di seluruh basis kode, pertimbangkan untuk menambahkan pengenal lisensi. Saat melakukannya pertimbangkan untuk merujuk ke spdx.dev pedoman.
Update: Diperbaiki PR # 10 dan berkomitmen 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] Ketergantungan Kontrak OpenZeppelin tidak disematkan
Untuk mencegah perilaku tak terduga jika perubahan yang melanggar dirilis di pembaruan mendatang dari Perpustakaan Kontrak OpenZeppelin, pertimbangkan untuk memasang pin pada versi ketergantungan ini di package.json file.
Update: Diperbaiki PR # 10.
[N04] Versi kompiler soliditas tidak disematkan
Di seluruh basis kode, pertimbangkan untuk menyematkan versi Kompiler soliditas ke versi stabil terbarunya. Ini akan membantu mencegah munculnya bug yang tidak terduga karena rilis mendatang yang tidak kompatibel. Untuk memilih versi tertentu, pengembang harus mempertimbangkan fitur kompiler yang dibutuhkan oleh proyek dan daftar bug yang diketahui terkait dengan setiap versi kompiler Solidity.
Update: Diperbaiki PR # 10.
[N05] Salah ketik
Pada berbagai contoh di seluruh basis kode, kata role
salah eja sebagai rol
. Salah satu contohnya adalah di docstring di dalam constructor
dari SafeGuard
kontrak.
Pertimbangkan untuk memperbaiki kesalahan ketik ini untuk meningkatkan keterbacaan kode.
Update: Diperbaiki sebagian PR # 10. Sedangkan ejaan role
telah diperbaiki, komentar "setel peran admin ke alamat admin yang ditentukan" seharusnya menjadi "setel peran admin ke alamat admin yang ditentukan". Selain itu, "eksekusi" salah eja di SafeGuard
kontrak pada baris 69, baris 82, baris 96 dan baris 110 dan "tersedia" salah eja pada baris 70, baris 83, baris 97, baris 111. Juga, pertimbangkan untuk mengganti kata-kata informal seperti "akan" in SafeGuard
kontrak dengan alternatif formal seperti "pergi ke".
[N06] Nyatakan uint sebagai uint256
Ada beberapa kejadian dalam basis kode di mana variabel dideklarasikan uint
tipe data bukan uint256
. Misalnya, eta
variabel dalam QueueTransactionWithDescription
peristiwa dari SafeGuard
kontrak.
Untuk mendukung ketegasan, semua contoh dari uint
harus dinyatakan sebagai uint256
.
Update: Diperbaiki PR # 10 dan berkomitmen 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Impor yang tidak digunakan
Grafik SafeGuard
kontrak impor console
kontrak tetapi tidak pernah menggunakannya.
Untuk meningkatkan keterbacaan kode, pertimbangkan untuk menghapus impor yang tidak digunakan.
Update: Diperbaiki PR # 10.
Kesimpulan
Satu kerentanan tinggi dan beberapa kerentanan kecil lainnya telah ditemukan dan rekomendasi serta perbaikan telah disarankan.
- &
- mengakses
- Akun
- di seluruh
- Tindakan
- tindakan
- Tambahan
- alamat
- admin
- Semua
- sudah
- Meskipun
- api
- pendekatan
- sekitar
- Audit
- Pada dasarnya
- makhluk
- bug
- bisnis
- panggilan
- kasus
- Menyebabkan
- perubahan
- biaya
- kode
- Pengkodean
- Umum
- Senyawa
- kebingungan
- pertimbangan
- mengandung
- terus
- kontrak
- kontrak
- bisa
- pencipta
- terbaru
- data
- berurusan
- Mendesain
- pengembang
- berbeda
- ditemukan
- Rumit
- ETH
- Acara
- peristiwa
- contoh
- pabrik
- Fitur
- Akhirnya
- Pertama
- Memperbaiki
- keluwesan
- ditemukan
- fungsi
- dana-dana
- masa depan
- GAS
- Pemberian
- tujuan
- pemerintahan
- Gubernur
- pedoman
- memiliki
- membantu
- di sini
- High
- Seterpercayaapakah Olymp Trade? Kesimpulan
- HTTPS
- diimplementasikan
- Meningkatkan
- Pada meningkat
- Antarmuka
- IT
- dikenal
- bahasa
- Terbaru
- Perpustakaan
- Lisensi
- baris
- Daftar
- lokal
- terkunci
- tampak
- Membuat
- Cocok
- modular
- banyak tanda
- Lainnya
- menyajikan
- proses
- proyek
- usul
- publik
- menerbitkan
- daftar
- Pers
- melaporkan
- gudang
- Hasil
- ulasan
- keamanan
- set
- pengaturan
- berbagi
- pintar
- Kontrak Cerdas
- soliditas
- Negara
- gaya
- sistem
- Melalui
- di seluruh
- waktu
- alat
- jalur
- Transaksi
- Pembaruan
- us
- Pengguna
- nilai
- Kerentanan
- Wallet
- minggu
- SIAPA
- dalam
- kata
- penulisan