דצמבר 6, 2021
מבוא
אל האני לִסְפּוֹר הצוות ביקש מאיתנו לבחון ולבקר מערך חוזים במטרה סופית לשפר חוזי ממשל משותפים ולתת להם יותר גמישות. הסתכלנו על הקוד ועכשיו מפרסמים את התוצאות שלנו.
סקירת המערכת
המערכת מסתמכת על שלושה חוזים עיקריים:
- A
SafeGuard
תבנית חוזה. חוזה זה הוא הadmin
שלTimelock
חוזה, ומוסיף שכבה של תפקידים מודולריים מעלTimelock
הפעולות של. חוזה זה מגדיר מספר תפקידים שיש להם אחריות וגישה נפרדת על מצב ההצעה (עמידה בתור, ביטול וביצוע). - A
SafeGuardFactory
חוזה פורס חדשSafeGuard
וחדש מקבילTimelock
חוֹזֶה. ואז הוא מגדיר את כתובת נעילת הזמן בתוךSafeGuard
חוזה ורושם את החדשSafeGuard
אלRegistry
. - A
Registry
שמחזיקה רשימה של פרוסיםSafeGuards
עם המקביל שלהם מספר גרסה.
אל האני SafeGuardFactory
בעצם יוליד א חדש SafeGuard
בכל פעם שהוא נקרא. א SafeGuard
הוא עוטף Timelock
פעולות שמוסיף תפקידים שונים ומופרדים לכל פעולה. תפקידים בנויים באמצעות שימוש ב-OpenZeppelin's AccessControlEnumerable
חוזה המעניק יותר גמישות ותאימות לארנקי multisig ולמקרי שימוש עסקיים שבהם מספר כתובות יכולות למלא את אותו תפקיד משותף.
עדכון: ב יחסי ציבור מס '10, צוות Tally החליט להסיר את Registry
חוֹזֶה. רשימת אמצעי ההגנה שנפרסו מאוחסנת כעת בתוך SafeGuardFactory
חוזה, ב safeGuards
ערכה ספורת, יחד עם הגרסה שלהם המאוחסנת ב- safeGuardVersion
מיפוי.
תפקידים
אל האני SafeGuard
החוזה מגדיר את התפקידים הבאים:
עדכון: אל האני CREATOR_ROLE
התפקיד הוסר כתיקון לבעיה L02.
היקף
ביקרנו התחייבות b2c63a9dfc4090be13320d999e7c6c1d842625d3
של safeguard
מאגר. בהיקפם נמצאים החוזים החכמים ב- contracts
מַדרִיך. אולם, ה mocks
ספרייה נחשבה מחוץ לתחום.
הנחות
המערכת לא אמורה להיות ניתנת לשדרוג. ה Registry
כתובת מוגדרת בקונסטרוקטור של SafeGuardFactory
וכל אחד SafeGuard
יכול לקבל את Timelock
מוגדר פעם אחת בלבד. זה אומר שאם חדש Registry
פרוס חדש SafeGuardFactory
חייבים לפרוס גם. אם ה Timelock
שינויים ביישום, הישן SafeGuard
s יתיישנו, ויהיה צורך לפרוס חדשים.
יתרה מכך, המערכת מסתמכת במידה רבה על יישום א Timelock
חוזה שנחשב מחוץ לתחום של ביקורת זו. הצוות עדיין לא סיים את יישום ה- Timelock
חוֹזֶה. בזמן כתיבת דו"ח זה, הטמעת Timelock של המתחם נמצא בשימוש בפרויקט.
בסיס הקוד נבדק על ידי שני אודיטורים במהלך שבוע אחד וכאן אנו מציגים את הממצאים שלנו.
חומרה קריטית
אין.
חומרה גבוהה
[H01] ניתן לנעול את ה-ETH בתוך Timelock
חוזה
אל האני Tally
הצוות ביסס במקור את היישום שלהם על הקרקע של GovernorBravoDelegate
חוזה מורכב.
במהלך ביקורת זו, ה Tally
צוות גילה מגבלה במושל של מתחם שבו ETH שלחה ישירות ל Timelock
אינו זמין לשימוש על ידי הצעות ממשל, ולמרות שהוא אינו תקוע לצמיתות, מצריך אחזור מעקף מורכב.
הסיבה לכך היא שהיישום של הנגיד מחייב את כל הערך של הצעה לצרף כ msg.value
על ידי החשבון שמפעיל את הביצוע, בלי להשתמש בשום אופן ב- Timelock
קרנות ETH.
אל האני אותה בעיה זוהתה מאוחר יותר ב- SafeGuard
הפעלה והצוות מודע לבעיה והוא נמצא בתהליך של תיקון.
בעת תיקון הבעיה, שקול להשתמש בגישה אומצה על ידי ספריית OpenZeppelin עבור אותו נושא.
עדכון: תוקן ב-commit 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] ניתן להקפיא את SafeGuardFactory
אל האני Registry
החוזה נועד לעקוב אחר כל SafeGuards
כי SafeGuardFactory
ייצור. יש לו את החיצוני register
פונקציה המשמש למטרה זו.
יחד עם זאת, SafeGuardFactory
יש בקונסטרוקטור שלו את ההקצאה של המקומי registry
ערך לערך הקלט. אין אפשרות לשנות את הערך של registry
משתנה ומסיבה זו, אם חדש Registry
נפרס, יש לפרוס גם מפעל חדש.
אל האני SafeGuardFactory
יש createSafeGuard
פונקציה, אחראי על הראשון פריסת חדש SafeGuard
, לאחר מכן חדש Timelock
עם הכתובת של ה SafeGuard
as admin
, ואז להגדיר את timelock
משתנה של SafeGuard
חוזה ולבסוף רישום ה SafeGuard
ברישום.
הבעיה היא שכל קריאה ל createSafeGuard
יכול להיאלץ להיכשל על ידי תוקף שיכול לרשום ישירות את הכתובת הדטרמיניסטית של החדש SafeGuard
לפני יצירתו. בכל פעם חוזה יוצר new
למשל, אי-התרחבות שלו גדלה, והכתובת שבה ייפרס המופע החדש של החוזה יכולה להיקבע על פי כתובת החוזה המקורית ואי-היא שלו. לכן, תוקף יכול לחשב מראש רבות מהכתובות שבהן החדש SafeGuards
ייפרסו וירשמו את הכתובות הללו ב- Registry
על ידי קורא ל- register
פוּנקצִיָה. זה יביא לקריאות אל createSafeGuard
ל לחזור מאז Registry
כבר מכיל את הכתובת.
כדי להימנע מכך ששחקנים חיצוניים יקראו בפומבי את Register
חוזה, שקול להגביל את הגישה ל- register
פונקציה לקבל שיחות באופן בלעדי על ידי SafeGuardFactory
.
עדכון: קבוע ב יחסי ציבור מס '10. צוות טלי הסיר את Registry
חוזה.
חומרה בינונית
אין.
חומרה נמוכה
[L01] הגיבו על קוד
אל האני Registry
החוזה כולל שורת קוד עם הערות. כדי לשפר את הקריאה, שקול להסיר אותו מבסיס הקוד.
עדכון: קבוע ב יחסי ציבור מס '10 ולהתחייב 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] המנהל של SafeGuard יכול להקצות את תפקיד היוצר לכל כתובת
אל האני SafeGuard
החוזה מגדיר את תפקידו של א CREATOR_ROLE
אשר, כפי שהשם מרמז, מוקצה ליוצר ההגנה.
עם זאת, באמצעות פניה מה היא grantRole
הפונקציה של AccessControlEnumerable
חוזה בספריית החוזים של OpenZeppelin, מנהל יכול להעניק תפקיד זה לכל כתובת. זה עלול לגרום לבלבול מכיוון ש היוצר של ה SafeGuard
יכול להיות רק ה SafeGuardFactory
.
לאורך כל בסיס הקוד, התפקיד הזה שימש רק כדי להגביל את האינטראקציה של משתמשים עם setTimelock
פונקציה של SafeGuard
חוֹזֶה. לפי התכנון, המערכת מבטיחה זאת setTimelock
פונקציה ניתן להתקשר פעם אחת בלבד, מ בתוך SafeGuardFactory
חוזה.
שקול להסיר את CREATOR_ROLE
תפקיד מה SafeGuard
חוזה ושימוש ב onlyOwner
מתקן ב setTimelock
פונקציה.
עדכון: קבוע ב יחסי ציבור מס '10.
[L03] הגדרה ויישום שגויים של ממשק
אל האני ISafeGuard
הממשק אינו מגדיר את queueTransactionWithDescription
פונקציה מיושם ב SafeGuard
חוזה, ובו בזמן, הוא מגדיר את __abdicate, __queueSetTimelockPendingAdmin ו__executeSetTimelockPendingAdmin פונקציות אך הן אינן מיושמות.
כדי לשפר את הנכונות והעקביות בבסיס הקוד, שקול לשנות את ה- ISafeGuard
ממשק שיתאים בדיוק ל SafeGuard
יישום.
עדכון: תוקן ב-commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] חסרים מחרוזות docstrings
חלק מהחוזים והפונקציות בבסיס הקוד חסרים תיעוד. לדוגמה, כמה פונקציות ב SafeGuard
חוזה.
בנוסף, חלק מהמחרוזות משתמשות בשפה לא רשמית, כמו זו מעל ה setTimelock
פונקציה ב SafeGuard
חוזה.
זה מפריע לבודקים להבין את כוונת הקוד, שהיא בסיסית להערכת נכון לא רק אבטחה אלא גם נכונות. בנוסף, מחרוזות docstrings משפרים את הקריאות ומקלים על התחזוקה. עליהם להסביר במפורש את המטרה או הכוונה של הפונקציות, התרחישים שבהם הם עלולים להיכשל, התפקידים המותרים לקרוא להם, הערכים שהוחזרו והאירועים הנפלטים.
שקול תיעוד יסודי של כל הפונקציות (והפרמטרים שלהן) שהן חלק מה-API הציבורי של החוזים. פונקציות המיישמות פונקציונליות רגישה, גם אם אינן ציבוריות, צריכות להיות מתועדות בבירור גם כן. בעת כתיבת מחרוזות docstrings, שקול לעקוב אחר ה- פורמט מפרט טבעי של Ethereum (NatSpec).
עדכון: מקובע חלקית פנימה יחסי ציבור מס '10. מחרוזות docstrings מתאימות נוספו לפונקציות שונות בכל בסיס הקוד. עם זאת, בנוסף לשינויים הנוכחיים, שקול לבצע את השינויים הבאים:
- להוסיף
description
כמו@param
במחרוזת הדוק שלמעלהqueueTransactionWithDescription
פונקציה - להוסיף
@param
ב דוקטרינג מעל הcreateSafeGuard
פונקציה בSafeGuardFactory
חוזה - להוסיף
@return
ב-docstrings מעל הפונקציות בSafeGuardFactory
חוזה.
[L05] קוד חסר תועלת או חוזר
ישנם מקומות בבסיס הקוד שבהם הקוד חוזר או אינו נחוץ. חלק מהדוגמאות הן:
- קווים 29-32 של
Registry
חוזה הם חסרי תועלת, כי_add
הפונקציה שלEnumerableSet
חוזה כבר מבצע את הבדיקות הללו כנגד הערכים שכבר נקבעו. - קווים 62, 67, 73 ו 78 של
SafeGuard
כל החוזה חוזרים על אותה פעולה מדויקת. שקול לכלול אותו בפונקציה פנימית כדי להימנע משכפול קוד. - קווים 62-63 ו 67-68 of
SafeGuard
חוזרים על עצמם. שקול לכלול אותם בפונקציה פנימית אחת. - השימוש
gasleft
כדי לציין כמה גז יש להעביר בקריאה של הפונקציהexecuteTransaction
הוא מיותר. הסיבה לכך היא שבאותה נקודת ביצוע, כל הגז שנותר ישמש להמשך הביצוע. אם זה לא נועד למפורש, שקול להסיר אתgas
פרמטר מהשיחה.
שקול להחיל את התיקון המוצע כדי לייצר קוד נקי יותר ולשפר את העקביות והמודולריות על פני בסיס הקוד.
עדכון: קבוע ב יחסי ציבור מס '10 ולהתחייב 7fd27df16fc879d990d36a167a0b6e719e578558
.
הערות ומידע נוסף
[N01] סגנון לא עקבי
ישנם כמה מקומות בבסיס הקוד, שבהם הבדלים בסגנון משפיעים על הקריאה, מה שמקשה על הבנת הקוד. חלק מהדוגמאות הן:
- אל האני
Registry
החוזה משתמש בסגנונות שונים עבור מחרוזות docstrings בחוזה כולו. - אל האני
SafeGuard
החוזה משדר אירוע כאשרqueueTransactionWithDescription
נקרא אך לא נפלטים אירועים בפונקציות אחרות העוסקות בעסקאות. - ב
SafeGuard
חוזה, לפעמים ערך משמש כפרמטר בשם ולפעמים _ערך משמש.
אם ניקח בחשבון את הערך שסגנון קידוד עקבי מוסיף לקריאות הפרויקט, שקול לאכוף סגנון קידוד סטנדרטי בעזרת כלים של לינטר, כגון Solhint.
עדכון: קבוע ב יחסי ציבור מס '10 ולהתחייב 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] חסר רישיון
בחוזים הבאים בתוך בסיס הקוד חסר מזהה רישיון SPDX.
כדי להשתיק אזהרות מהדר ולהגביר את העקביות בכל בסיס הקוד, שקול להוסיף מזהה רישיון. תוך כדי כך שקול להתייחס spdx.dev הנחיות.
עדכון: קבוע ב יחסי ציבור מס '10 ולהתחייב 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] התלות של חוזה OpenZeppelin אינה מוצמדת
כדי למנוע התנהגויות בלתי צפויות במקרה ששינויים שוברים ישוחררו בעדכונים עתידיים של ה- הספרייה של OpenZeppelin Contracts, שקול להצמיד את הגרסה של התלות הזו ב- package.json קובץ.
עדכון: קבוע ב יחסי ציבור מס '10.
[N04] גרסת המהדר Solidity אינה מוצמדת
בכל בסיס הקוד, שקול להצמיד את הגרסה של ה- מהדר Solidity לגרסה היציבה האחרונה שלו. זה אמור לעזור למנוע הצגת באגים בלתי צפויים עקב מהדורות עתידיות לא תואמות. כדי לבחור גרסה ספציפית, מפתחים צריכים לשקול גם את תכונות המהדר הדרושות לפרויקט וגם רשימת הבאגים הידועים המשויך לכל גרסת מהדר של Solidity.
עדכון: קבוע ב יחסי ציבור מס '10.
[N05] שגיאת הקלדה
במקרים שונים ברחבי בסיס הקוד, המילה role
כתוב שגוי כ rol
. דוגמה אחת כזו נמצאת ב המחרוזת ב- constructor
של SafeGuard
חוזה.
שקול לתקן שגיאות הקלדה אלה כדי לשפר את קריאות הקוד.
עדכון: מקובע חלקית פנימה יחסי ציבור מס '10. בעוד הכתיב של role
תוקנה, ההערה "הגדר תפקיד אדמין את כתובת אדמין מוגדרת" צריכה להיות "הגדר תפקיד אדמין לכתובת אדמין מוגדרת". בנוסף, "ביצוע" מאוית שגוי ב- SafeGuard
חוזה על קו 69, קו 82, קו 96 ו קו 110 ו"זמין" מאוית שגוי קו 70, קו 83, קו 97, קו 111. כמו כן, שקול להחליף מילים לא רשמיות כגון "עומד ל" in SafeGuard
חוזה עם חלופות פורמליות כגון "הולך".
[N06] הכריז על uint כ-uint256
ישנם מספר מופעים בבסיס הקוד שבהם מכריזים על משתנים uint
סוג נתונים במקום uint256
. לדוגמא, ה eta
משתנה ב- QueueTransactionWithDescription
אירוע של SafeGuard
חוזה.
כדי להעדיף מפורשות, כל המקרים של uint
יש להכריז כ uint256
.
עדכון: קבוע ב יחסי ציבור מס '10 ולהתחייב 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] יבוא לא בשימוש
אל האני SafeGuard
חוזה מייבא את console
חוזה אבל אף פעם לא משתמש בו.
כדי לשפר את קריאות הקוד, שקול להסיר כל יבוא שאינו בשימוש.
עדכון: קבוע ב יחסי ציבור מס '10.
מסקנות
נמצאה נקודת תורפה אחת גבוהה ועוד כמה נקודות תורפה, והוצעו המלצות ותיקונים.
- &
- גישה
- חֶשְׁבּוֹן
- לרוחב
- פעולה
- פעולות
- נוסף
- כתובת
- מנהל
- תעשיות
- כְּבָר
- למרות
- API
- גישה
- סביב
- בדיקה
- בעיקרון
- להיות
- באגים
- עסקים
- שיחה
- מקרים
- לגרום
- שינוי
- תשלום
- קוד
- סִמוּל
- Common
- תרכובת
- בלבול
- התחשבות
- מכיל
- להמשיך
- חוזה
- חוזים
- יכול
- יוצר
- נוֹכְחִי
- נתונים
- התמודדות
- עיצוב
- מפתחים
- אחר
- גילה
- משוכלל
- ETH
- אירוע
- אירועים
- דוגמה
- מפעל
- תכונות
- בסופו של דבר
- ראשון
- לסדר
- גמישות
- מצא
- פונקציה
- כספים
- עתיד
- גז
- נתינה
- מטרה
- ממשל
- נגיד
- הנחיות
- יש
- לעזור
- כאן
- גָבוֹהַ
- איך
- HTTPS
- יושם
- להגדיל
- גדל
- מִמְשָׁק
- IT
- ידוע
- שפה
- האחרון
- סִפְרִיָה
- רישיון
- קו
- רשימה
- מקומי
- נעול
- נראה
- עשייה
- להתאים
- מודולרי
- מולטי-סיג
- אחר
- להציג
- תהליך
- פּרוֹיֶקט
- הצעה
- ציבורי
- לפרסם
- הירשם
- עיתונות
- לדווח
- מאגר
- תוצאות
- סקירה
- אבטחה
- סט
- הצבה
- משותף
- חכם
- חוזים חכמים
- מוּצָקוּת
- מדינה
- סגנון
- מערכת
- דרך
- בכל
- זמן
- כלים
- לעקוב
- עסקות
- עדכונים
- us
- משתמשים
- ערך
- פגיעויות
- ארנקים
- שבוע
- מי
- בתוך
- מילים
- כתיבה