Desember 16, 2021
Pengantar
Grafik 1inch tim meminta kami untuk meninjau dan mengaudit mereka Protokol Batas Pesanan kontrak cerdas v2. Kami melihat kode dan sekarang mempublikasikan hasil kami.
Cakupan
Kami mengaudit komit 4d94eea25e4dac6271bfd703096a5c4a4d899b4a
dari 1inch/limit-order-protocol
gudang. Dalam ruang lingkup adalah kontrak berikut:
- OrderMixin.sol
- OrderRFQMixin.sol
- PredicateHelper.sol
- RevertReasonParser.sol
- Permitable.sol
- ChainlinkCalculator.sol
- ArgumentsDecoder.sol
- AmountCalculator.sol
- NonceManager.sol
- LimitOrderProtocol.sol
- ImmutableOwner.sol
- InteractiveNotificationReceiver.sol
- AggregatorInterface.sol
- IDaiLikePermit.sol
Semua file dan direktori proyek lainnya (termasuk pengujian), bersama dengan dependensi dan proyek eksternal, teori permainan, dan desain insentif, juga dikeluarkan dari ruang lingkup audit ini. Kode eksternal dan dependensi kontrak diasumsikan berfungsi sebagaimana yang didokumentasikan dan layanan back-end yang disediakan oleh 1inch diasumsikan bertindak untuk kepentingan terbaik protokol.
Kesehatan secara keseluruhan
Secara umum, kami menemukan basis kode proyek dapat dibaca dan diatur dengan baik, meskipun dapat mengambil manfaat dari dokumentasi yang lebih luas, terutama di sekitar blok kode perakitan, kasus tepi protokol, aset/predikat/sumber daya eksternal yang akan digunakan, tanggung jawab/keterbatasan layanan back-end yang disediakan, dan interaksi antar aktor. Proyek ini berusaha keras untuk membuat tindakan hemat gas, kadang-kadang bahkan dengan risiko membuat kode lebih sulit untuk dipikirkan; kami mengangkat masalah yang terkait dengan itu di bawah ini. Selama audit, tim 1 inci sangat siap sedia, responsif, dan sangat mudah untuk diajak bekerja sama.
Ikhtisar Sistem
Protokol Pesanan Batas memungkinkan pesanan makers
untuk menandatangani perintah off-chain untuk token swap. Protokol kemudian memfasilitasi pengisian pesanan yang ditandatangani sebelumnya berdasarkan pesanan takers
. Pesanan sangat dapat diperluas, dan dapat memanggil kontrak eksternal secara statis di beberapa titik selama proses pengisian pesanan. Ekstensibilitas ini mengilhami protokol dengan utilitas, tetapi menambahkan kompleksitas dan permukaan serangan yang lebih besar untuk perintah itu sendiri.
Penting untuk dicatat bahwa tidak ada penyimpanan on-chain untuk detail pesanan. Status pengisian atau status pembatalan pesanan hanya dilacak melalui hash pesanan. Ini mengharuskan pesanan dibagikan peer-to-peer atau melalui pihak terpusat. Dalam hal ini, tim 1inch bermaksud untuk bertindak sebagai pihak terpusat, menggabungkan pesanan yang ditandatangani dan menggunakan pesanan tersebut sebagai sumber likuiditas untuk protokol mereka yang lain. Pesanan akan dipublikasikan melalui API mereka sendiri sehingga pengguna dapat berinteraksi dengan mereka.
Pemusatan ini memberi tim 1 inci kontrol ekstrem atas pesanan mana yang diterbitkan dan akhirnya dieksekusi. Ini juga memberi mereka kemampuan untuk menyensor pesanan, yang dapat berguna dalam kasus perintah jahat atau menipu, tetapi juga dapat disalahgunakan dan memungkinkan mereka untuk menjalankan pengguna lain jika ada pesanan yang menguntungkan dengan tidak menunjukkannya melalui API.
Peran Istimewa
Meskipun kontrak peran yang digunakan berada di luar cakupan, satu peran istimewa telah diidentifikasi. Sebuah immutableOwner
diatur ke pembuat kontrak proxy pada saat konstruksi, dan digunakan untuk membatasi akses ke proxy external
fungsi.
Ketergantungan eksternal dan asumsi kepercayaan
Desain protokol ini memerlukan komponen off-chain dan on-chain, dan model hibrid ini dapat digunakan untuk mengurangi beberapa vektor serangan yang kami identifikasi dalam laporan kami, tetapi biaya kemampuan itu meningkatkan ketergantungan pada tim dan infrastruktur 1 inci.
Selain itu, Protokol Pesanan Batas menyediakan fungsi yang dimaksudkan untuk mengambil harga dari oracle Chainlink. Kami menganggap oracle itu jujur, dapat diakses, dan berfungsi dengan baik.
Selain itu, karena fleksibilitas pesanan, ada beberapa titik kontak dengan kontrak eksternal yang tidak divalidasi. Ini berarti bahwa pengguna jahat dapat menyalahgunakan panggilan tersebut dan menyamar sebagai predikat, aset, atau nubuat dengan kontrak jahat untuk menjalankan tindakan selama pengisian pesanan. Meskipun proyek dilindungi di beberapa area dari reentrancy, vektor tersebut dapat menyebabkan serangan penolakan layanan atau perintah spam yang tidak terdeteksi. Tim 1 inci menyadari bahwa masalah tertentu mungkin muncul saat menggunakan kontrak yang tidak dikenal untuk protokol dan telah menunjukkan niat mereka bahwa hanya aset "blue chip" utama yang akan didukung penuh oleh proyek. Namun, perlu dicatat bahwa bahkan dengan aset yang paling populer, ada perilaku intrinsik dari setiap aset yang dapat menyebabkan masalah pada protokol yang tidak menanganinya dengan benar, seperti memiliki biaya selama transfer dengan USDT atau mengembalikan kode kesalahan alih-alih sukses boolean dengan cTokens.
Temuan
Di sini kami menyajikan temuan kami.
Tingkat keparahan kritis
Tidak ada.
Keparahan tinggi
[H01] Data yang tidak konsisten diteruskan ke _makeCall
Dalam majalah OrderMixin
kontrak, itu _makeCall
fungsi yang digunakan untuk mentransfer aset dari pengambil ke pembuat lalu dari pembuat ke pengambil. Dalam transfer terakhir, _makeCall
fungsi salah melewati pesanan makerAsset
sebagai parameter terakhir, ketika itu harus menjadi urutan makerAssetData
.
Akibatnya, fungsi proxy apa pun yang bergantung pada makerAssetData
argumen akan pecah.
Agar konsisten dengan panggilan sebelumnya ke _makeCall
dan untuk sepenuhnya mendukung fungsionalitas proxy, pertimbangkan untuk memperbarui order.makerAsset
parameter untuk order.makerAssetData
.
Update: Diperbaiki tarik permintaan # 57.
[H02] Pesanan pribadi yang terisi sebagian dapat diisi oleh siapa saja
Protokol memungkinkan pembuatan pesanan pribadi dan publik. Atas pesanan pribadi, hanya allowedSender
alamat, yang ditentukan oleh pembuat selama pembuatan pesanan, dapat memenuhi pesanan.
Namun, dalam OrderMixin
kontrak, validasi untuk allowedSender
alamat cakupannya salah, artinya hanya dievaluasi di dalam logika yang menangani pengisian pertama pesanan. Jika pesanan pribadi terisi sebagian, maka cek untuk allowedSender
alamat tidak lagi dapat dijangkau dan pesanan dapat diisi oleh siapa saja.
Untuk memperjelas maksud seputar apakah pengguna harus dapat mengisi pesanan pribadi yang terisi sebagian atau tidak, pertimbangkan untuk mendokumentasikan alasan perilaku saat ini atau memvalidasi allowedSender
alamat di luar cakupan pengisian pertama untuk memastikan bahwa itu akan divalidasi setiap kali pengisian dicoba.
Update: Diperbaiki tarik permintaan # 58.
[H03] Pembuat jahat dapat mengambil keuntungan dari pengisian sebagian untuk mencuri aset pengambil
Pesanan dari OrderMixin
kontrak memiliki kemampuan untuk dipenuhi sebagian. Untuk mendukung pengisian parsial, protokol memerlukan cara untuk menghitung kedua sisi swap. Keduanya getMakerAmount
dan getTakerAmount
bidang ditentukan oleh pembuat pesanan untuk tujuan yang tepat ini.
Saat mengisi pesanan, pengambil harus memberikan: makingAmount
atau itu takingAmount
nilai serta thresholdAmount
nilai. Ada dua jalur kode berbeda yang dapat diambil, berdasarkan jika makingAmount
atau itu takingAmount
disediakan.
Yang pertama adalah ketika makingAmount
parameter ditentukan. Itu bisa memotong itu makingAmount
nilai dan juga hitung takingAmount
nilai untuk itu. Dalam situasi ini, thresholdAmount
memastikan bahwa takingAmount
nilai yang diambil adalah tidak terduga besar.
Yang kedua adalah ketika takingAmount
parameter ditentukan. Dalam kasus seperti itu, itu akan hitung makingAmount
nilai, dengan kemungkinan memotongnya dan menghitung ulang takingAmount
nilai jika itu terjadi. Dalam situasi ini, thresholdAmount
nilai memastikan bahwa makingAmount
nilai yang dikembalikan adalah tidak disangka kecil.
Ada dua metode eksploitasi, masing-masing unik untuk salah satu jalur kode yang disebutkan sebelumnya. Metode eksploitasi ini membutuhkan malware getMakerAmount
dan getTakerAmount
fungsi. Implementasi sederhana dari fungsi-fungsi ini akan memiliki perilaku yang identik dengan AmountCalculator
's getMakerAmount
dan getTakerAmount
fungsi, tetapi dengan sakelar berkode keras yang akan memaksa mereka untuk mengembalikan nilai yang dikendalikan penyerang saat dibutuhkan.
Pola eksploitasi pertama yang tidak terlalu parah melibatkan jalur kode pertama di mana makingAmount
nilai ditentukan dalam urutan pengisian. Pembuat jahat akan menunggu pesanan pengisian yang menentukan makingAmount
untuk muncul di mempool untuk menjalankannya. Mereka akan menguras semua nilai kecuali 1 dari sisi pembuat dan kemudian memaksa _callGetTakerAmount
untuk mengembalikan jumlah yang ditentukan di akun pengguna thresholdAmount
nilai (atau tunjangan mereka jika kurang). Ketika transaksi pengguna akhirnya berhasil, mereka akan menukar penuh mereka thresholdAmount
senilai takerAsset
untuk satu unit makerAsset
. Eksploitasi ini dibatasi oleh jumlah yang diberikan oleh thresholdAmount
nilai atau jumlah takerAsset
pengguna diizinkan di LimitOrderProtocol
kontrak.
Pola eksploitasi kedua yang lebih parah melibatkan jalur kode kedua di mana takingAmount
nilai ditentukan. Pembuat jahat juga akan menunggu perintah pengisian yang ditentukan a takingAmount
nilai untuk muncul di mempool. Mereka akan menjalankan transaksi dan memaksa makingAmount
nilai yang dikembalikan oleh _callGetMakerAmount
berfungsi lebih tinggi dari keduanya remainingMakerAmount
dan thresholdAmount
. Mereka juga akan mengatur takingAmount
nilai yang dikembalikan oleh _callGetTakerAmount
menjadi jumlah takerAsset
aset diperbolehkan pada LimitOrderProtocol
oleh pengambil. Ketika transaksi pengambil berjalan, itu akan potong makingAmount
nilai dan kemudian menghitung ulang takingAmount
nilai. Perhitungan ulang ini tidak dijamin lebih rendah, dan dalam hal ini akan menguras pengambil semua takerAsset
yang mereka izinkan dalam kontrak. Dalam jalur kode ini, thresholdAmount
Nilai adalah memastikan bahwa makingAmount
tidak terlalu rendah, jadi ambil semua pengambil takerAsset
aset tidak dicentang. Dana yang hilang dibatasi oleh jumlah takerAsset
aset yang diizinkan pengguna di LimitOrderProtocol
kontrak.
Eksploitasi ini tidak mungkin dilakukan tanpa perintah parsial dan, lebih khusus lagi, perintah parsial dengan perintah jahat getMakerAmount
dan getTakerAmount
implementasi.
Masalah utama dari thresholdAmount
pemeriksaan nilai adalah bahwa itu hanya mencakup satu sisi swap, tetapi sisi lain dapat dimanipulasi melalui frontrunning. Tidak ada jaminan bahwa nilai yang semula diajukan oleh pengambil tetap tidak berubah. Pertimbangkan untuk menghapus makingAmount
pemotongan dari jalur kode dan pengembalian jika pesanan tidak dapat mendukung pengisian sebesar yang diminta. Dengan melakukan ini, thresholdAmount
dapat digunakan untuk membatasi sisi lain swap dan menghindari perilaku tak terduga, bahkan dalam perintah jahat.
Update: Diperbaiki tarik permintaan # 83.
Keparahan sedang
[M01] Argumen statis diteruskan setelah argumen dinamis
Dalam majalah OrderMixin
kontrak, itu getTakerAmount
dan getMakerAmount
bidang byte digunakan sebagai argumen untuk _callGetTakerAmount
dan _callGetMakerAmount
fungsi. Panggilan ini menyediakan cara untuk menghitung satu sisi swap berdasarkan sisi lain, dan memungkinkan pengguna untuk mengisi sebagian pesanan.
Grafik getTakerAmount
/getMakerAmount
bidang adalah variabel dinamis dan dikemas di depan takerAmount
dan makerAmount
nilai-nilai di _callGetTakerAmount
dan _callGetMakerAmount
fungsi. Ada kemungkinan bagi pembuat jahat untuk memberikan lebih banyak data daripada yang diharapkan di getTakerAmount
dangetMakerAmount
bidang untuk mendorong takerAmount
dan makerAmount
byte melewati di mana mereka diasumsikan ketika sedang diterjemahkan dalam fungsi berikutnya. Ini memungkinkan pembuat untuk menggeser jumlah pengambil atau pembuat yang diteruskan sebesar satu byte penuh ke kanan dan bahkan menggantinya sepenuhnya jika 32 byte data tambahan disediakan.
Pengguna sudah harus meninjau secara manual getTakerAmount
dan getMakerAmount
bidang dalam urutan, tetapi teknik ini agak sulit dikenali. Juga perlu diperhatikan, serangan ini bahkan berlaku untuk yang dipercaya secara internal getMakerAmount
dan getTakerAmount
fungsi. Untuk sebagian besar serangan, memberikan jumlah ambang batas yang wajar akan mencegah hilangnya dana.
Untuk mencegah hal ini, pertimbangkan pengkodean argumen statis sebelum argumen dinamis untuk menghindari memberikan argumen dinamis metode untuk mengontrol argumen statis.
Update: Tidak tetap. Tim 1 inci menyatakan:
Kami akan lebih berhati-hati dengan validasi getter. Kami akan mencoba menerapkan validasi kewarasan getter di SDK kami yang akan membantu memfilter pesanan yang berpotensi berbahaya.
[M02] Perintah ERC721 dapat dimanipulasi
Dimungkinkan untuk bertukar lebih dari sekadar ERC20 melalui OrderMixin
dengan menerapkan kontrak yang memiliki pemilih fungsi yang sama dengan IERC20 transferFrom
, dan memberikan kontrak itu sebagai makerAsset
atau itu takerAsset
dalam sebuah perintah.
Proksi di luar cakupan, yaitu, ERC721Proxy
, ERC721ProxySafe
, dan ERC1155Proxy
kontrak mengikuti pola ini untuk memberikan dukungan bagi ERC721
dan ERC1155
token. Karena proxy harus dipanggil dengan pola yang sama dengan IERC20 transferFrom
panggilan, tanda tangan harus dimulai dengan address from
, address to
dan uint256 amount
. Apa pun yang dibutuhkan proxy dapat diteruskan setelahnya, dan didefinisikan dalam urutan sebagai makerAssetData
dan takerAssetData
.
ERC1155 secara alami dapat mentransfer beberapa token id yang sama sekaligus, yang berarti ERC1155Proxy
kontrak memanfaatkan amount
bidang. Di samping itu, ERC721
s tidak memiliki kegunaan yang jelas untuk amount
bidang. Karena mereka mewakili token yang tidak dapat dipertukarkan, tokenId tertentu hanya akan memiliki satu, membuat amount
lapangan tidak berguna. Karena itu, implementasi untuk keduanya ERC721Proxy
dan ERC721ProxySafe
kontrak menggunakan yang diperlukan amount
lapangan sebagai tokenId
sebagai gantinya.
Kelebihan beban ini amount
parameter menciptakan kemungkinan pengisian sebagian ERC721
pesanan untuk membeli token yang terdaftar secara terpisah dengan harga diskon. Misalnya, mungkin ada kasus di mana satu pengguna memiliki banyak ERC721
s dari kontrak yang sama diizinkan untuk ditransfer oleh ERC721Proxy
kontrak dan daftar mereka dalam limit order terpisah.
Jika limit order juga menyediakan getMakerAmount
dan getTakerAmount
bidang, dimungkinkan untuk mengisi sebagian ini ERC721
perintah. Sejak pesanan amount
bidang sebenarnya sesuai dengan tokenId
, pengguna jahat dapat mengisi sebagian pada ERC721
dengan tokenId yang lebih tinggi, menghasilkan a makingAmount
/takingAmount
dari ERC721
yang bisa sesuai dengan yang lebih rendah tokenId
. Hasilnya adalah ERC721
dengan yang lebih rendah tokenId
akan ditransfer dengan harga (higher tokenId price) * (lower tokenId's id) / (higher tokenId's id)
.
Eksploitasi ini memiliki beberapa persyaratan:
- kelipatan
ERC721
s dari kontrak yang sama untuk diizinkan pada keduanyaERC721
proxy oleh satu pemilik. - Buka pesanan untuk salah satu dari
ERC721
s itu bukan yang terendahtokenId
dari yang diperbolehkan. - Pengisian sebagian diperbolehkan pada pesanan.
Untuk sepenuhnya menghilangkan kemungkinan sebagian ERC721
mengisi, pertimbangkan untuk memisahkan amount
dan tokenId
argumen. Apakah argumennya terpisah atau tidak, pertimbangkan juga untuk mendokumentasikannya untuk memperingatkan pengguna tentang perilaku ini dan untuk menghindari pola ini di masa mendatang.
Update: Diperbaiki tarik permintaan # 59.
[M03] Asumsi desimal tidak berdokumen
Grafik LimitOrderProtocol
kontrak mewarisi ChainlinkCalculator
kontrak melalui OrderMixin
kontrak. Kontrak ini memaparkan dua fungsi untuk memungkinkan penggunaan oracle Chainlink selama pemeriksaan predikat dan pencarian jumlah pembuat/jumlah pengambil.
Namun, kontrak membuat asumsi yang tidak terdokumentasi tentang jumlah desimal yang harus dilaporkan oleh oracle Chainlink, serta jumlah desimal yang harus dikandung oleh parameter fungsi. Dalam skenario tertentu, ini dapat menyebabkan perilaku yang tidak terduga, termasuk salah menentukan harga aset dan kehilangan dana yang tidak disengaja.
Lebih khusus, di seluruh kontrak asumsi implisit adalah bahwa oracle Chainlink akan melaporkan dengan 18 desimal presisi. Namun, tidak semua oracle Chainlink laporkan dengan jumlah desimal ini. Faktanya, jika oracle melaporkan pasangan token dalam mata uang (USD, misalnya), itu hanya akan memiliki presisi 8 desimal. Karena tidak ada batasan pada yang nubuat dapat digunakan, asumsi implisit tidak boleh dibuat tentang jumlah desimal yang akan mereka laporkan.
Terkait, ada asumsi implisit bahwa amount
parameter untuk ChainlinkCalculator
fungsi akan menggunakan 18 desimal, bersama dengan deklarasi eksplisit yang menyesatkan bahwa singlePrice
fungsi Calculates price of token relative to ETH scaled by 1e18
. Pada kenyataannya, bahkan dengan oracle yang tidak laporkan dengan 18 desimal, nilai balik dari singlePrice
fungsi akan diskalakan dengan jumlah desimal dari amount
parameter, yang mungkin tidak harus 18 desimal.
Demikian pula, doublePrice
function mengasumsikan bahwa dua oracle Chainlink akan melaporkan dengan jumlah desimal yang sama, menyebabkan hasil fungsi menyimpang dari harapan.
Pertimbangkan secara eksplisit mendokumentasikan asumsi mengenai jumlah desimal yang parameter dan nilai kembalian harus dalam hal. Selanjutnya, pertimbangkan untuk membatasi perhitungan yang bergantung pada ramalan yang mematahkan asumsi tersebut, atau meminta perhitungan yang relevan memperhitungkan jumlah desimal yang sebenarnya.
Update: Diperbaiki tarik permintaan # 75.
Tingkat keparahan rendah
[L01] Konstanta tidak dideklarasikan secara eksplisit
Ada beberapa kemunculan nilai literal yang digunakan dengan makna yang tidak dapat dijelaskan dalam basis kode. Sebagai contoh:
- Dalam majalah
OrderMixin
kontrak, itu_remaining
pemetaan secara semantik kelebihan beban (seperti yang dijelaskan dalam masalah Pemetaan yang berlebihan secara semantik) untuk melacak jumlah aset yang tersisa untuk pesanan yang terisi sebagian dan juga jika pesanan telah terisi penuh. Secara khusus,0
berarti tidak ada pengisian yang terkait dengan pesanan yang telah dibuat,1
berarti pesanan tidak bisa lagi diisi, dan apa pun yang lebih besar dari1
berarti ada jumlah sisa yang terkait dengan pesanan yang berpotensi dapat diisi. - Dalam majalah
ChainlinkCalculator
kontrak, nilai literal1e18
digunakan dalamsinglePrice
fungsi.
Untuk meningkatkan keterbacaan kode dan memfasilitasi pemfaktoran ulang, pertimbangkan untuk mendefinisikan konstanta untuk setiap angka ajaib, memberinya nama yang jelas dan cukup jelas. Untuk nilai kompleks, pertimbangkan untuk menambahkan komentar sebaris yang menjelaskan cara penghitungannya atau alasan pemilihannya.
Update: Diperbaiki tarik permintaan # 75 dan tarik permintaan # 76.
[L02] Pihak jahat dapat mencegah pelaksanaan perintah yang diizinkan
Grafik OrderMixin
kontrak memungkinkan pengguna pembuat untuk mengirimkan perintah yang diizinkan sehingga dapat dilakukan dalam satu transaksi, daripada harus memiliki transaksi terpisah untuk persetujuan. Juga, pengambil pesanan bisa menyerahkan izin mereka sendiri selama pengisian pesanan untuk tujuan yang sama.
Namun, karena izin pembuatnya terkandung di dalam urutan, izin pembuat dan pengambil akan dapat diakses saat transaksi pengisian pesanan ada di mempool. Ini akan memungkinkan setiap pengguna jahat untuk mengambil izin tersebut dan mengeksekusinya pada kontrak aset masing-masing sambil menjalankan transaksi pengisian. Karena izin ini memiliki nonce
untuk mencegah serangan pembelanjaan ganda, transaksi pengisian pesanan akan gagal karena mencoba menggunakan izin yang sama yang baru saja digunakan selama frontrun.
Meskipun tidak ada risiko keamanan, dan pembuatnya dapat membuat pesanan baru dan menyetujui transaksi sebelumnya, serangan ini tentu saja dapat memengaruhi kegunaan pesanan yang diizinkan. Memang, penyerang yang termotivasi dapat memblokir semua perintah yang diizinkan dengan serangan ini. Pertimbangkan untuk memvalidasi jika izin sudah diserahkan, atau jika tunjangan cukup, selama pesanan terisi. Juga pertimbangkan untuk memberi tahu pengguna tentang kemungkinan serangan ini selama komposisi pesanan.
Update: Tidak tetap. Tim 1 inci menyatakan:
Kami memiliki pemeriksaan persetujuan sebelumnya tetapi memutuskan untuk menyederhanakan aliran izin untuk hanya mengembalikan persetujuan yang gagal. Kami akan memikirkan cara untuk memberi tahu pembuat tentang masalah ini.
[L03] Kode duplikat
Ada contoh kode yang digandakan dalam basis kode. Menduplikasi kode dapat menyebabkan masalah di kemudian hari dalam siklus hidup pengembangan dan membuat proyek lebih rentan terhadap pengenalan kesalahan. Kesalahan tersebut dapat secara tidak sengaja diperkenalkan ketika perubahan fungsionalitas tidak direplikasi di semua contoh kode yang seharusnya identik. Contoh kode yang digandakan meliputi:
Daripada menggandakan kode, pertimbangkan untuk hanya memiliki satu kontrak atau pustaka yang berisi kode duplikat dan menggunakannya kapan pun fungsi duplikat diperlukan.
Update: Diperbaiki sebagian tarik permintaan # 60.
[L04] Rangkaian tes yang salah atau menyesatkan
Ada beberapa contoh dalam rangkaian pengujian di mana pengujian menyimpang dari perilaku yang diharapkan. Misalnya:
- Grafik
ChainlinkCalculator
kontrak diwarisi olehOrderMixin
kontrak. Namun, selama tesAmountCalculator.arbitraryStaticCall
fungsi yang digunakan untuk memanggilChainlinkCalculator
kontrak sebagai eksternal, kontrak independen. Meskipun hasilnya adalah yang diharapkan, pengujian harus mencerminkan perilaku dengan desain sistem saat ini dan kasus penggunaan yang diantisipasi dengan memanggilChainlinkCalculator
berfungsi secara langsung tanpa menggunakan panggilan statis arbitrer. - Meskipun kontrak proxy berada di luar cakupan, kami memperhatikan bahwa saat menguji protokol dengan aset ERC721,
ERC721Proxy
kontrak tidak digunakan untuk menukar aset dalam rangkaian pengujian.
Karena rangkaian pengujian itu sendiri berada di luar cakupan audit ini, harap pertimbangkan untuk meninjau rangkaian pengujian secara menyeluruh untuk memastikan semua pengujian berjalan dengan sukses sesuai dengan spesifikasi protokol.
Update: Diperbaiki tarik permintaan # 57, tarik permintaan # 59, dan tarik permintaan # 61.
[L05] Kesalahan dan kelalaian dalam acara
Sepanjang basis kode, peristiwa umumnya dipancarkan ketika perubahan sensitif dibuat pada kontrak. Namun, banyak peristiwa tidak memiliki parameter yang diindeks dan/atau tidak memiliki parameter penting. Sebagai contoh:
Ada juga tindakan sensitif yang kurang peristiwa, seperti:
Pertimbangkan untuk mengindeks lebih lengkap peristiwa yang ada dan menambahkan parameter baru jika kurang. Juga, pertimbangkan untuk memancarkan semua peristiwa dengan cara yang lengkap sehingga dapat digunakan untuk membangun kembali status kontrak dengan layanan off-chain.
Update: Tidak tetap. Namun, tim 1 inci memang menambahkan orderRemaining
parameter ke OrderCanceled
acara di tarik permintaan # 62.
Tim 1 inci menyatakan:
Kami menemukan bahwa hanya sebagian kecil data yang diperlukan untuk memenuhi kebutuhan frontend. Dalam kasus analisis ekstensif, semua bidang yang disarankan tersedia melalui penelusuran. Untuk
OrderRFQMixin
kami mengharapkan pembuat pasar untuk membangun cara canggih mereka sendiri untuk melacak pesanan apa yang telah dibatalkan.
[L06] Penyimpanan berubah selama emisi acara
Dalam majalah NonceManager
kontrak, ketika NonceIncreased
peristiwa dipancarkan, nonce pengirim pesan juga meningkat.
Menjalankan beberapa operasi secara bersamaan dapat membuat basis kode lebih sulit untuk dipikirkan, lebih rentan terhadap kesalahan, dan dapat menyebabkan operasi diabaikan atau disalahpahami.
Untuk meningkatkan keseluruhan intensionalitas, keterbacaan, dan kejelasan kode, pertimbangkan untuk meningkatkan nilai nonce sebelum memancarkan acara.
Update: Diperbaiki tarik permintaan # 63.
[L07] Metodologi decoding yang tidak konsisten dapat menyebabkan perbedaan hasil
Untuk mendukung semua ekstensibilitas dan fleksibilitasnya, Protokol Pesanan Batas secara rutin harus menangani data byte dinamis dan nilai pengembalian sewenang-wenang dari kontrak eksternal. Akibatnya, protokol mencakup ArgumentsDecoder
perpustakaan untuk lebih efisien mengubah nilai byte dinamis menjadi tipe data dasar. Namun, perpustakaan ini tidak digunakan secara eksklusif, dan dalam beberapa kasus abi.decode
digunakan sebagai gantinya. Selain itu, beberapa kontrak menggunakan abi coder v1
sementara yang lain menggunakan abi coder v2
. Yang pertama berkinerja lebih mirip dengan ArgumentsDecoder
perpustakaan, sedangkan yang terakhir melakukan pemeriksaan tambahan saat decoding.
Penggunaan yang tidak konsisten dari metodologi decoding yang berbeda ini dapat mengakibatkan perbedaan yang tidak kentara antara niat dan perilaku aktual dari basis kode.
Misalnya, simulateCalls
fungsi hanya menggunakan ArgumentsDecoder.decodeBool
fungsi. Jika simulateCalls
fungsi digunakan untuk memeriksa panggilan yang akan dilakukan di bagian predikat dari suatu perintah, kemudian hasilnya bisa menyimpang dari apa yang sebenarnya terjadi ketika kondisi predikat dievaluasi, karena metodologi decoding yang berbeda digunakan.
Jadi, misalnya, jika predikat membuat eksternal staticcall
ke beberapa fungsi yang mengembalikan a uint256
nilai lebih besar dari satu daripada yang diharapkan bool
, maka panggilan itu akan kembali, karena nilai pengembaliannya adalah diterjemahkan dengan abi coder v2
's abi.decode
yang tidak akan menerima nilai-nilai seperti bool
. Namun, jika panggilan yang sama persis dilakukan dengan simulateCalls
, lalu itu hanya akan ditandai sebagai true
, Karena decodeBool
memperlakukan nilai apa pun yang lebih besar dari nol sebagai true
.
Untuk membuat simulateCalls
berfungsi sepenuhnya mencerminkan perilaku panggilan predikat yang sebenarnya, pertimbangkan untuk memodifikasinya untuk digunakan abi.decode
.
Update: Diperbaiki tarik permintaan # 82.
[L08] Kurangnya validasi input
Grafik fillOrderToWithPermit
dan fillOrderTo
fungsi dari OrderMixin
kontrak, serta fillOrderRFQToWithPermit
dan fillOrderRFQTo
fungsi dari OrderRFQMixin
kontrak, tidak memvalidasi target
parameter alamat.
Ini memungkinkan pengguna untuk secara tidak sengaja memasukkan alamat nol dan, sebagai akibatnya, mengunci aset yang seharusnya mereka terima setelah mengisi pesanan.
Untuk memastikan bahwa pengguna tidak secara tidak sengaja mengunci dana mereka, pertimbangkan untuk memvalidasi bahwa target
alamat tidak sama dengan alamat nol dalam fungsi yang dikutip.
Update: Diperbaiki tarik permintaan # 78.
[L09] Cakupan pengujian unit rendah
Cakupan uji unit untuk keseluruhan proyek adalah sekitar 75%, dengan beberapa kontrak memiliki cakupan yang sangat rendah.
Mempertimbangkan pentingnya pengujian unit untuk memvalidasi kode dan mencegah regresi saat memfaktorkan ulang dan mengembangkan fitur baru, kami mendorong peningkatan cakupan pengujian unit secara signifikan hingga setidaknya 95%, dan termasuk kasus tepi yang mencakup situasi yang bahkan tidak mungkin.
Update: Tidak tetap.
[L10] Dokumentasi sebaris yang menyesatkan atau tidak lengkap
Sepanjang basis kode, beberapa contoh dokumentasi sebaris yang menyesatkan dan/atau tidak lengkap telah diidentifikasi dan harus diperbaiki.
Berikut ini adalah contoh dokumentasi sebaris yang menyesatkan:
- Dalam majalah
ChainlinkCalculator
kontrak, itusinglePrice
fungsi Spesifikasi Nat@notice
label mengatakan bahwa ituCalculates price of token relative to ETH scaled by 1e18
, tetapi pada kenyataannya, hasilnya adalah nilai ofamount
token diskalakan oleh1e18
, di mana oracle mungkin tidak melaporkan dalam istilah ETH (untuk pasangan yang tidak termasuk ETH, misalnya). - Dalam majalah
OrderRFQMixin
kontrak, ituinvalidatorForOrderRFQ
fungsi Spesifikasi Nat@return
label menyesatkan, karena kutipan mungkin belum diisi untuk bit invalidator masing-masing yang telah ditetapkan. Pesanan juga bisa dibatalkan. - di jalur 147, 165, dan 188 of
OrderMixin.sol
, Spesifikasi Nat@param
tag tidak gramatikal. - On line 20 of
ERC1155Proxy.sol
, yang@notice
tag menyatakan bahwa hash yang dihitung adalah hasil hashingfunc_733NCGU
fungsi, di mana seharusnyafunc_301JL5R
fungsi sebagai gantinya.
Berikut ini adalah contoh dokumentasi inline yang tidak lengkap:
- Fungsi dalam
AmountCalculator
kontrak tidak menjelaskan parameter apa pun. - Dalam majalah
ChainlinkCalculator
kontrak, itusinglePrice
dandoublePrice
fungsi tidak menggambarkan semua parameter. - Dalam majalah
ImmutableOwner
kontrak, variabel publik dan pengubah tidak memiliki NatSpec. - Dalam majalah
InteractiveNotificationReceiver
kontrak, itunotifyFillOrder
fungsi tidak menjelaskan parameter apa pun. - Dalam majalah
LimitOrderProtocol
kontrak, ituDOMAIN_SEPARATOR
fungsi tidak memiliki NatSpec. - Acara dan pemetaan di
NonceManager
tidak memiliki NatSpec. - Dalam majalah
OrderRFQMixin
kontrak,cancelOrderRFQ*
fungsi tidak menggambarkan nilai kembalian. - Dalam majalah
OrderMixin
kontrak, beberapa fungsi kurang lengkap NatSpec. - On line 168 of
OrderMixin.sol
dan online 71 ofOrderRFQMixin.sol
, itu hilang@dev
menandai. - Fungsi dalam
PredicateHelper
kontrak tidak menggambarkan semua parameter.
Dokumentasi sebaris yang jelas sangat penting untuk menguraikan maksud kode. Ketidakcocokan antara dokumentasi inline dan implementasi dapat menyebabkan kesalahpahaman yang serius tentang bagaimana sistem diharapkan berperilaku. Pertimbangkan untuk memperbaiki kesalahan ini untuk menghindari kebingungan bagi pengembang, pengguna, dan auditor.
Update: Sebagian diperbaiki. Dokumentasi menyesatkan dibahas di tarik permintaan # 75 dan tarik permintaan # 77.
Tim 1 inci menyatakan:
Kami telah memperbaiki dokumen yang menyesatkan. Penyempurnaan dokumen akan dilakukan kemudian.
[L11] Perintah DoS dimungkinkan saat menggunakan kait
Grafik OrderMixin
contract mengimplementasikan fungsionalitas untuk mengisi pesanan swap off-chain generik yang dapat memiliki kondisi untuk keberhasilannya. Selama pesanan terisi, pesanan dapat periksa kondisi "predikat" yang telah ditentukan sebelumnya sebelum melanjutkan eksekusi.
Namun, karena kondisi predikat ini dapat menargetkan logika kontrak arbitrer apa pun, pembuat jahat dapat mengelabui penerima agar percaya bahwa pesanan berperilaku benar dan valid saat memeriksanya di luar rantai, tetapi kemudian gagal saat mencoba mengisi pesanan yang sama. di-rantai. Perubahan dalam perilaku predikat ini dapat dibuat dengan menjalankan beberapa status variabel di mana predikat bergantung, dengan memeriksa gas yang dikirim atau bahkan alamat mana yang terlibat dalam panggilan, atau dengan logika lain.
Selanjutnya, jika pembuat mendefinisikan interaksi selama pertukaran, yang interactionTarget
kontrak dapat mengembalikan dirinya sendiri atau mencabut tunjangan untuk mencegah pemenuhan pesanan yang berhasil, yang pada dasarnya mengarah pada hasil yang sama dengan predikat jahat.
Meskipun aset tidak akan berisiko, pengguna atau bot yang menemukan pesanan yang menguntungkan akan memiliki beban yang meningkat untuk mencoba mengidentifikasi pesanan spam semacam ini yang mungkin tampak sah di permukaan. Jika mereka gagal mengidentifikasi pesanan semacam ini, mereka akan dikenakan biaya gas yang terbuang percuma. Untuk mengurangi jumlah pesanan spam, pertimbangkan untuk membatasi target yang tersedia untuk kait ini. Pertimbangkan juga untuk memperingatkan pengguna tentang kemungkinan ini sebelum mereka mencoba memenuhi pesanan.
Update: Tidak tetap. Tim 1 inci menyatakan:
Kami menanganinya di backend kami dan kami akan memikirkan cara untuk memberi tahu calon pengambil tentang masalah ini.
[L12] Pembulatan bisa jadi tidak menguntungkan untuk taker
Dalam majalah OrderMixin
dan OrderRFQMixin
kontrak, ketika pesanan sedang diisi dan pengambil hanya memberikan makingAmount
or takingAmount
jumlah, protokol mencoba untuk menghitung jumlah lawan dari swap.
Ada dua masalah dengan perhitungan ini, yang pertama adalah bahwa tidak ada dokumentasi atau logika yang membatasi jumlah desimal yang harus digunakan oleh parameter jumlah, yang kami bahas di bagian Asumsi desimal tidak berdokumen isu.
Masalah kedua adalah bahwa, dalam proses perhitungan ini, protokol berputar untuk mendukung pembuatnya. Masalah pembulatan bisa sangat diperburuk ketika asumsi desimal implisit rusak, tetapi bahkan ketika semuanya sesuai dengan yang diharapkan, pembulatan akan terjadi dengan jumlah kecil dan ganjil.
Pertimbangkan untuk mengizinkan pengambil untuk menentukan jumlah minimum makerAsset
aset yang bersedia mereka terima bersama-sama dengan jumlah maksimum takerAsset
aset yang ingin mereka tukar, sehingga penerimaan pembulatan apa pun lebih eksplisit.
Update: Tidak tetap. Tim 1 inci menyatakan:
Jumlah ambang batas harus cukup untuk perlindungan pengambil.
[L13] Penanganan pesanan yang kontradiktif saat kekurangan parameter
Dalam majalah OrderMixin
kontrak, itu fillOrderTo
fungsi membuat panggilan internal ke _callGetMakerAmount
dan _callGetTakerAmount
berfungsi setiap kali pengisian dicoba dan baik makingAmount
atau itu takingAmount
parameter adalah nol, masing-masing, atau jika makingAmount
nilainya lebih besar dari remainingMakerAmount
nilai.
Grafik _callGetMakerAmount
dan _callGetTakerAmount
panggilan akan menyebabkan pengembalian jika pesanan tidak dibuat dengan getMakerAmount
or getTakerAmount
parameter, masing-masing, dan pengisian parsial sedang dieksekusi.
An komentar sebaris di samping _callGetMakerAmount
dan komentar sebaris di samping _callGetTakerAmount
mengklaim bahwa "hanya seluruh isi yang diizinkan" jika pesanan tidak dibuat dengan getMakerAmount
or getTakerAmount
parameter.
Namun, ada jalur kode yang tidak berlaku, karena jalur tersebut tidak memeriksa length
s dari keduanya getMakerAmount
dan getTakerAmount
parameter.
Secara khusus, Ketika sebuah taker
menentukan takerAmount
nilai untuk pesanan yang hanya memiliki getMakerAmount
, kecuali panggilan itu ke getMakerAmount
mengembalikan jumlah yang lebih besar dari remainingMakerAmount
, pengisian sebagian dapat dieksekusi bertentangan dengan dokumentasi sebaris.
Ini membuat intensionalitas jalur kode tersebut tidak jelas. Jika ini adalah perilaku yang diharapkan, pertimbangkan untuk memodifikasi dokumentasi sebaris sehingga lebih eksplisit. Jika ini adalah perilaku yang tidak disengaja, pertimbangkan untuk selalu memeriksa panjang keduanya getMakerAmount
dan getTakerAmount
parameter secara bersamaan sehingga implementasi memperkuat perilaku yang dijelaskan oleh dokumentasi inline.
Update: Diperbaiki tarik permintaan # 79.
[L14] Menggunakan panggilan Chainlink yang tidak digunakan lagi
Grafik ChainlinkCalculator
contract dimaksudkan untuk digunakan untuk menanyakan oracle Chainlink. Ia melakukannya dengan melakukan panggilan ke latestTimestamp
dan latestAnswer
metode, keduanya telah ditinggalkan. Faktanya, metode tidak lagi ada di API agregator Chainlink pada versi tiga.
Untuk menghindari potensi ketidakcocokan di masa depan dengan oracle Chainlink, pertimbangkan untuk menggunakan latestRoundData
metode sebagai gantinya.
Update: Diperbaiki tarik permintaan # 67.
Catatan & Informasi Tambahan
[N01] Tidak mengimpor antarmuka
Grafik AggregatorInterface
antarmuka tampaknya merupakan bagian dari kode yang disalin dari ChainLink
repositori kode publik. Antarmuka lengkap disertakan dalam ChainLink
paket npm kontrak.
Jika memungkinkan, untuk mengurangi potensi ketidakcocokan antarmuka dan masalah yang diakibatkannya, daripada mendefinisikan ulang dan/atau menulis ulang antarmuka proyek lain, pertimbangkan untuk menggunakan antarmuka yang diinstal melalui paket npm resmi mereka.
Update: Diperbaiki tarik permintaan # 66.
[N02] Ketergantungan proyek yang tidak digunakan lagi
Selama pemasangan dependensi proyek, NPM memperingatkan bahwa salah satu paket yang diinstal, Highlight
, "tidak akan lagi didukung atau menerima pembaruan keamanan di masa mendatang".
Meskipun sepertinya paket ini tidak dapat menyebabkan risiko keamanan, pertimbangkan untuk memutakhirkan ketergantungan yang menggunakan paket ini ke versi yang dipertahankan.
Update: Diperbaiki tarik permintaan # 64.
[N03] Panggilan eksternal untuk melihat metode bukan panggilan statis
Di sebagian besar basis kode, protokol secara eksplisit membuat panggilan eksternal melalui OpenZeppelin functionStaticCall
metode untuk membatasi kemungkinan perubahan keadaan ketika mereka tidak diharapkan atau tidak diinginkan. Namun, di ChainlinkCalculator
kontrak, meskipun niat membuat panggilan eksternal hanya untuk view
metode pada oracle Chainlink, panggilan eksternal di singlePrice
dan doublePrice
fungsi tidak dibuat melalui eksplisit staticcall
s.
Meskipun kami tidak mengidentifikasi masalah keamanan langsung apa pun yang berasal dari ini, untuk mengurangi permukaan serangan, meningkatkan konsistensi, dan memperjelas maksud, pertimbangkan untuk menggunakan eksplisit staticcall
s, untuk semua panggilan eksternal ke view
fungsi di ChainlinkCalculator
kontrak.
Update: Tidak tetap. Tim 1 inci menyatakan:
Kami berpikir bahwa komplikasi sintaks membatalkan peningkatan konsistensi.
[N04] Tidak gagal lebih awal dengan pesanan yang tidak valid
Dalam majalah OrderMixin
kontrak, itu fillOrderTo
fungsi menangani kondisi khusus ketika pesanan belum pernah diajukan sebelumnya (remainingMakerAmount == 0
), tetapi tidak secara eksplisit menangani kondisi saat pesanan tidak lagi valid (remainingMakerAmount == 1
).
Dalam skenario terakhir, fungsi tersebut pada akhirnya akan kembali, tetapi hanya setelah membakar sejumlah gas yang tidak sepele. Untuk memperjelas maksud, meningkatkan keterbacaan, dan mengurangi penggunaan gas, pertimbangkan untuk menangani skenario urutan tidak valid secara eksplisit menjelang awal fungsi.
Update: Diperbaiki tarik permintaan # 68.
[N05] Kontrak pembantu tidak ditandai sebagai abstrak
Dalam Solidity, kata kuncinya abstract
digunakan untuk kontrak yang bukan merupakan kontrak fungsional dengan haknya sendiri, atau tidak dimaksudkan untuk digunakan seperti itu. Sebagai gantinya, abstract
kontrak diwarisi oleh kontrak lain dalam sistem untuk membuat kontrak fungsional yang berdiri sendiri.
Di seluruh basis kode, ada contoh kontrak pembantu yang tidak ditandai sebagai abstrak, meskipun faktanya kontrak tersebut tidak dimaksudkan untuk digunakan sendiri. Misalnya, AmountCalculator
, ChainlinkCalculator
, ImmutableOwner
, NonceManager
, dan PredicateHelper
kontrak semuanya terdiri dari seperangkat fungsi dasar yang dimaksudkan untuk digunakan oleh kontrak pewarisan.
Pertimbangkan untuk menandai kontrak pembantu sebagai abstract
untuk secara jelas menandakan bahwa mereka dirancang semata-mata untuk menambahkan fungsionalitas ke kontrak yang mewarisinya.
Update: Tidak tetap. Tim 1 inci menyatakan:
Pembantu tersebut dapat digunakan secara terpisah. Mereka diwarisi hanya untuk penghematan gas.
[N06] Pengurutan fungsi tidak konsisten
Sepanjang basis kode, urutan fungsi umumnya mengikuti urutan yang direkomendasikan dalam Panduan Gaya Soliditas, yang mana: constructor
, fallback
, external
, public
, internal
, private
.
Namun, di OrderMixin
kontrak, itu public
checkPredicate
fungsi menyimpang dari panduan gaya, membagi dua external
fungsi.
Untuk meningkatkan keterbacaan proyek secara keseluruhan, pertimbangkan untuk menstandardisasi urutan fungsi di seluruh basis kode, seperti yang direkomendasikan oleh Solidity Style Guide.
Update: Diperbaiki tarik permintaan # 69.
[N07] Aliran pengisian pesanan tidak konsisten
Grafik OrderMixin
dan RFQOrderMixin
kontrak keduanya menangani pengisian pesanan yang ditandatangani, tetapi aliran pesanan umum antara kedua kontrak tidak konsisten.
OrderMixin
's fillOrderTo
fungsi mengikuti alur umum ini (kode semu):
if ((takingAmount == 0) == (makingAmount == 0))
else if (takingAmount == 0)
else (handle makingAmount == 0) THEN swapTokens
Sedangkan RFQOrderMixin
analogi fillOrderRFQTo
fungsi mengikuti aliran ini (kode semu):
if (takingAmount == 0 && makingAmount == 0)
else if (takingAmount == 0)
else if (makingAmount == 0)
else revert THEN swapTokens
Tidak ada wawasan dari dokumentasi mengapa kondisi pertama di masing-masing dari dua fungsi ini berbeda, atau mengapa takingAmount
dan makingAmount
tidak bisa keduanya nol dalam fungsi yang terakhir. Juga, kasus di mana keduanya makingAmount
dan takingAmount
disediakan jauh lebih mudah untuk dipikirkan dalam fillOrderRFQTo
fungsi, karena ditangani dengan jelas di final else
blok.
Untuk memperjelas maksud dan meningkatkan keterbacaan kode secara keseluruhan, pertimbangkan untuk menstandardisasi alur pesanan umum di kedua kontrak ini, atau secara eksplisit mendokumentasikan mengapa ada perbedaan.
Update: Tidak tetap. Tim 1 inci menyatakan:
Ini karena fungsi penetapan harga khusus dalam pesanan terbatas. Sejak
getMakerAmount
secara substansial dapat berbeda darigetTakerAmount
, kami pikir lebih baik tidak membuat opsi default untuk pengambil karena mungkin akan membingungkan mereka jika pengambil tersebut berbeda.
[N08] Pesan kesalahan diformat secara tidak konsisten atau menyesatkan
Di seluruh basis kode, require
dan revert
pesan kesalahan, yang dimaksudkan untuk memberi tahu pengguna tentang kondisi tertentu yang menyebabkan transaksi gagal, ternyata formatnya tidak konsisten.
Misalnya, masing-masing pesan kesalahan di saluran 85 dari OrderMixin.sol
, 16 dari ERC721ProxySafe.sol
, dan 26 dari Permitable.sol
menerapkan gaya yang berbeda.
Selain itu, beberapa pesan kesalahan menyesatkan:
Pesan kesalahan dimaksudkan untuk memberi tahu pengguna tentang kondisi yang gagal, sehingga mereka harus memberikan informasi yang cukup sehingga koreksi yang tepat dapat dilakukan untuk berinteraksi dengan sistem. Pesan kesalahan yang tidak informatif sangat merusak pengalaman pengguna secara keseluruhan, sehingga menurunkan kualitas sistem. Selain itu, pesan kesalahan yang diformat secara tidak konsisten dapat menimbulkan kebingungan yang tidak perlu. Oleh karena itu, pertimbangkan untuk meninjau seluruh basis kode untuk memastikan setiap require
dan revert
pernyataan memiliki pesan kesalahan yang diformat secara konsisten, akurat, informatif, dan ramah pengguna.
Update: Diperbaiki sebagian tarik permintaan # 81.
[N09] Penggunaan variabel pengembalian bernama yang tidak konsisten
Ada penggunaan variabel pengembalian bernama yang tidak konsisten di OrderMixin
kontrak. Beberapa fungsi kembalikan variabel bernama, yang lainnya mengembalikan nilai eksplisit, dan lain-lain mendeklarasikan variabel pengembalian bernama tetapi menimpanya dengan pernyataan pengembalian eksplisit.
Pertimbangkan untuk mengadopsi pendekatan yang konsisten untuk mengembalikan nilai di seluruh basis kode dengan menghapus semua variabel pengembalian bernama, secara eksplisit mendeklarasikannya sebagai variabel lokal, dan menambahkan pernyataan pengembalian yang diperlukan jika sesuai. Ini akan meningkatkan baik ketegasan dan keterbacaan kode, dan juga dapat membantu mengurangi regresi selama refactor kode di masa mendatang.
Update: Diperbaiki tarik permintaan # 73.
[N10] Perhitungan hash pesanan tidak terbuka untuk API
Grafik external
fungsi remaining
, remainingRaw
dan remainingsRaw
semua mengharapkan hash pesanan untuk operasi yang sukses.
Namun, fungsi pembantu _hash
, yang mengembalikan hash dari suatu pesanan, has private
visibilitas. Ini berarti bahwa pengguna harus mengemas bagian dari pesanan dan string domain secara manual untuk mendapatkan hash pesanan.
Untuk menghindari potensi kesalahan saat menghitung hash pesanan dan untuk menyediakan metode bagi pengguna untuk menghasilkan hash masing-masing pesanan, pertimbangkan untuk memperluas visibilitas _hash
berfungsi untuk public
dan refactoring nama menjadi hash
agar konsisten dengan sisa kode.
Update: Diperbaiki tarik permintaan # 74.
[N11] Pemetaan yang berlebihan semantik
Grafik _remaining
pemetaan di OrderMixin
kontrak secara semantik kelebihan beban untuk melacak status pesanan dan sisa jumlah aset yang tersedia untuk pesanan tersebut.
Tiga negara yang dapat diambil adalah:
0
: Urutan hash belum terlihat.1
: Pesanan telah dibatalkan atau terisi penuh.- Apa saja
uint
lebih besar dari1
: Yang tersisamakerAmount
tersedia untuk diisi pada pesanan plus1
.
Overloading semantik ini membutuhkan pembungkusan dan pelepasan nilai ini selama lookup
, cancellation
, initialization
, dan storage
.
Kelebihan semantik dan logika yang diperlukan untuk mengaktifkannya dapat rentan terhadap kesalahan dan dapat membuat basis kode lebih sulit untuk dipahami dan dipikirkan, juga dapat membuka pintu untuk regresi di pembaruan kode di masa mendatang.
Untuk meningkatkan keterbacaan kode, pertimbangkan untuk melacak status penyelesaian pesanan dalam pemetaan terpisah.
Update: Tidak tetap. Tim 1 inci menyebutkan bahwa perbaikan akan meningkatkan biaya bahan bakar untuk fillOrder
fungsi.
[N12] Pesanan dengan izin memungkinkan panggilan ke kontrak sewenang-wenang
Grafik OrderMixin
kontrak mewarisi Permitable
kontrak untuk memungkinkan pengisian pesanan transaksi tunggal dengan aset yang menerimanya permit
panggilan untuk mengubah tunjangan.
Namun, panggilan ke Permitable
kontrak tidak memvalidasi apakah target adalah aset yang diizinkan atau bahkan aset, yang dapat memungkinkan pengguna jahat melewati alamat kontrak arbitrer yang dapat mengeksekusi panggilan lain sebelum pengisian pesanan selesai.
Meskipun kontraknya adalah dilindungi dari masuk kembali, mengurangi permukaan serangan dan mencegah pemanggilan kontrak eksternal selama eksekusi selalu disarankan. Pertimbangkan untuk membatasi aset yang terlibat dalam izin ke aset yang terlibat dalam pesanan atau ke daftar putih aset untuk protokol.
Update: Tidak tetap. Tim 1 inci menyatakan:
OrderMixin
sebenarnya tidak memiliki info tentang token yang sebenarnya sebagaimakerAsset
dantakerAsset
terkadang proxy atau kontrak perantara lainnya dan info tentang token aktual disimpan dalam beberapa byte yang berubah-ubah. Jadi tidak ada cara yang layak untuk membatasi aset manapermit
dipanggil.
[N13] solhint
tidak pernah diaktifkan kembali
Di seluruh basis kode, ada beberapa solhint-disable
pernyataan, khususnya yang on line 23 dan online 41 of RevertReasonParser.sol
, yang tidak diakhiri dengan solhint-enable
.
Mendukung ketegasan dan seketat mungkin saat menonaktifkan solhint
, pertimbangkan untuk menggunakan solhint-disable-line
or solhint-disable-next-line
sebagai gantinya, mirip dengan garis 16 dari file yang sama.
Update: Diperbaiki tarik permintaan # 72.
[N14] Salah ketik
Basis kode berisi kesalahan ketik berikut:
Selain itu proyek README
(di luar cakupan audit ini) berisi kesalahan ketik berikut:
Pertimbangkan untuk memperbaiki kesalahan ketik ini untuk meningkatkan keterbacaan kode.
Update: Diperbaiki tarik permintaan # 71 dan tarik permintaan # 77.
[N15] Penggunaan uint
alih-alih uint256
Untuk mendukung ketegasan, semua contoh dari uint
harus dinyatakan sebagai uint256
. Secara khusus, mereka yang berada di for
loop pada garis 98 dan 119 of OrderMixin.sol
dan garis 16 dan 30 of PredicateHelper.sol
.
Update: Diperbaiki tarik permintaan # 70.
Kesimpulan
3 masalah tingkat keparahan tinggi ditemukan. Beberapa perubahan diusulkan untuk mengikuti praktik terbaik dan mengurangi potensi serangan ke permukaan.
- &
- 7
- Tentang Kami
- mengakses
- Menurut
- Akun
- di seluruh
- Bertindak
- tindakan
- Tambahan
- alamat
- Keuntungan
- Semua
- Membiarkan
- sudah
- Meskipun
- jumlah
- analisis
- api
- pendekatan
- argumen
- sekitar
- aset
- Aktiva
- Audit
- Back-end
- Awal
- makhluk
- TERBAIK
- Praktik Terbaik
- Bit
- bot
- membangun
- panggilan
- yang
- kasus
- Menyebabkan
- Rantai
- perubahan
- memeriksa
- Cek
- kode
- kompleks
- kondisi
- kebingungan
- konstruksi
- mengandung
- kontrak
- kontrak
- Koreksi
- Biaya
- bisa
- sepasang
- pencipta
- Currency
- terbaru
- data
- transaksi
- Denial of Service
- penggelaran
- Mendesain
- pengembang
- Pengembangan
- MELAKUKAN
- berbeda
- berbeda
- domain
- dua kali lipat
- dinamis
- Awal
- Tepi
- mendorong
- terutama
- ETH
- Acara
- peristiwa
- segala sesuatu
- contoh
- Pasar Valas
- diharapkan
- pengalaman
- Mengeksploitasi
- Fitur
- Fields
- Akhirnya
- Pertama
- Memperbaiki
- keluwesan
- aliran
- mengikuti
- ditemukan
- penuh
- fungsi
- dana-dana
- masa depan
- permainan
- GAS
- Umum
- Pemberian
- besar
- membimbing
- Penanganan
- hash
- hashing
- memiliki
- membantu
- High
- sangat
- Seterpercayaapakah Olymp Trade? Kesimpulan
- HTTPS
- Hibrida
- mengenali
- Dampak
- melaksanakan
- penting
- pengimporan
- termasuk
- Termasuk
- Meningkatkan
- Pada meningkat
- Info
- informasi
- Infrastruktur
- wawasan
- maksud
- bunga
- Antarmuka
- terlibat
- masalah
- IT
- besar
- lebih besar
- memimpin
- terkemuka
- Perpustakaan
- Terbatas
- baris
- Likuiditas
- Daftar
- daftar
- lokal
- tampak
- lookup
- utama
- pembuat
- Membuat
- Pasar
- Mempool
- cermin
- model
- paling
- Paling Populer
- yaitu
- Fitur Baru
- non-sepadan
- token non-sepadan
- resmi
- Buka
- Operasi
- pilihan
- peramal
- urutan
- perintah
- Lainnya
- pemilik
- pola
- Populer
- menyajikan
- mencegah
- harga pompa cor beton mini
- di harga
- swasta
- proses
- proyek
- memprojeksikan
- perlindungan
- protokol
- memberikan
- menyediakan
- wakil
- publik
- menerbitkan
- membeli
- kualitas
- menaikkan
- Kenyataan
- menurunkan
- kepercayaan
- melaporkan
- laporan
- gudang
- Persyaratan
- ISTIRAHAT
- Hasil
- Pengembalian
- ulasan
- Risiko
- putaran
- Run
- SDK
- keamanan
- Layanan
- set
- berbagi
- saham
- bergeser
- mirip
- Sederhana
- kecil
- pintar
- Kontrak Cerdas
- So
- soliditas
- Spam
- Secara khusus
- Pengeluaran
- Spot
- awal
- Negara
- Pernyataan
- Negara
- Status
- penyimpanan
- gaya
- disampaikan
- sukses
- sukses
- berhasil
- mendukung
- Didukung
- Permukaan
- Beralih
- sistem
- target
- uji
- pengujian
- tes
- Melalui
- di seluruh
- waktu
- bersama
- token
- Token
- jalur
- Pelacakan
- .
- Kepercayaan
- unik
- Pembaruan
- us
- kegunaan
- USD
- USDT
- Pengguna
- kegunaan
- nilai
- View
- jarak penglihatan
- menunggu
- Apa
- whitelist
- dalam
- tanpa
- Kerja
- bernilai
- nol