דצמבר 16, 2021
מבוא
השמיים 1inch הצוות ביקש מאיתנו לבחון ולבקר אותם פרוטוקול הזמנה מוגבלת חוזים חכמים v2. הסתכלנו על הקוד ועכשיו מפרסמים את התוצאות שלנו.
היקף
ביקרנו התחייבות 4d94eea25e4dac6271bfd703096a5c4a4d899b4a
של 1inch/limit-order-protocol
מאגר. בהיקפם היו החוזים הבאים:
- 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
כל שאר הקבצים והספריות של הפרויקטים (כולל בדיקות), יחד עם תלות ופרויקטים חיצוניים, תורת המשחקים ועיצוב התמריצים, לא נכללו אף הם מהיקף ביקורת זו. ההנחה הייתה שתלויות קוד וחוזה חיצוניים פועלים כפי שמתועדים והנחו ששירותי קצה אחוריים שסופקו על ידי 1inch פועלים לטובת הפרוטוקול.
בריאות כללית
באופן כללי, מצאנו את בסיס הקוד של הפרויקט כקריא ומאורגן היטב, אם כי הוא יכול להפיק תועלת מתיעוד נרחב יותר, במיוחד סביב בלוקים של קוד ההרכבה, מקרי קצה של הפרוטוקול, נכסים/פרדיקטים/משאבים חיצוניים שישמשו, אחריות/מגבלות של שירות העורף הניתן, ואינטראקציות בין שחקנים. הפרויקט עושה מאמצים רבים כדי להפוך את הפעולות לחסכוניות בגז, מדי פעם אפילו תוך סיכון של קושי להנמק את הקוד; אנו מעלים סוגיות הקשורות לכך להלן. לאורך כל הביקורת, צוות 1 אינץ' היה זמין מאוד, מגיב וקל מאוד לעבוד איתו.
סקירת מערכת
פרוטוקול Limit Order מאפשר סדר makers
לחתום על הזמנות מחוץ לרשת להחלפת אסימונים. לאחר מכן, הפרוטוקול מקל על מילוי הזמנות שנחתמו בעבר לפי הזמנה takers
. הזמנות ניתנות להרחבה מאוד, ויכולות להתקשר לחוזים חיצוניים בכמה נקודות לאורך תהליך מילוי ההזמנות. יכולת הרחבה זו מטביעה את הפרוטוקול בתועלת, אך היא מוסיפה גם מורכבות וגם משטח התקפה גדול יותר עבור ההזמנות עצמן.
חשוב לציין שאין אחסון בשרשרת של פרטי הזמנה. מעקב אחר מצב המילוי או הביטול של הזמנות מתבצע רק באמצעות גיבוב הזמנות. זה מצריך שהזמנות יהיו משותפות עמית לעמית או באמצעות גורם מרוכז. במקרה זה, צוות 1inch מתכוון לפעול כגורם ריכוזי זה, לצבור הזמנות חתומות ולהשתמש בהזמנות אלו כמקור נזילות לפרוטוקולים האחרים שלהם. הזמנות יפורסמו באמצעות API משלהן, כך שהמשתמשים יוכלו ליצור איתם אינטראקציה.
ריכוזיות זו מעניקה לצוות 1 אינץ' שליטה קיצונית על אילו הזמנות מתפרסמות ובסופו של דבר מבוצעות. זה גם נותן להם את היכולת לצנזר פקודות, מה שיכול להיות שימושי במקרה של פקודות זדוניות או מטעות, אבל יכול גם להיות מנוצל לרעה ולאפשר להם להריץ כל משתמש אחר במקרה של הזמנה חיובית על ידי אי הצגתו דרך ה-API.
תפקידים מיוחסים
למרות שהחוזים שבהם נעשה שימוש בתפקיד היו מחוץ לתחום, זוהה תפקיד מיוחס אחד. א immutableOwner
מוגדר ליוצר חוזה proxy בזמן הבנייה, ומשמש להגבלת גישה ל-proxy's external
פונקציות.
תלות חיצונית והנחות אמון
העיצוב של פרוטוקול זה מחייב רכיבים מחוץ לשרשרת ורכיבים על השרשרת, וניתן להשתמש במודל ההיברידי הזה כדי למתן כמה וקטורי תקיפה שאנו מזהים בדוח שלנו, אך העלות של יכולת זו היא התלות מוגברת בצוות ובתשתית ה-1 אינץ'.
בנוסף, פרוטוקול Limit Order מספק פונקציות שנועדו לאחזר מחירים מאורקל של Chainlink. הנחנו שהאורקלים האלה כנים, נגישים ומתפקדים כראוי.
יתרה מכך, בשל הגמישות של הזמנה, ישנן מספר נקודות מגע עם חוזים חיצוניים שאינם מאושרים. משמעות הדבר היא שמשתמש זדוני עלול לעשות שימוש לרעה בשיחות כאלה ולהתחזות לפרדיקטים, נכסים או אורקלים עם חוזים זדוניים כדי לבצע פעולות במהלך מילוי הזמנות. למרות שהפרויקט מוגן באזורים מסוימים מפני כניסה חוזרת, וקטורים כאלה עלולים לגרום להתקפות מניעת שירות או הזמנות ספאם שלא אותרו. צוות ה-1 אינץ' מודע לכך שבעיות מסוימות עשויות להופיע בעת שימוש בחוזים לא מוכרים עבור הפרוטוקול והצביעו על כוונתם שרק נכסי "שבב כחול" גדולים יתמכו במלואם על ידי הפרויקט. עם זאת, יש לציין שגם עם הנכסים הפופולריים ביותר יש התנהגויות פנימיות מכל נכס שעלולות לגרום לבעיות בפרוטוקולים שאינם מטפלים בהם כראוי, כגון תשלום במהלך העברות עם USDT או החזרת קוד שגיאה במקום הצלחה בוליאנית עם cTokens.
ממצאים
כאן אנו מציגים את הממצאים שלנו.
חומרה קריטית
אין.
חומרה גבוהה
[H01] נתונים לא עקביים הועברו אל _makeCall
ב OrderMixin
החוזה, _makeCall
הפונקציה משמשת להעברת נכסים מהלוקח אל היוצר ולאחר מכן מהיוצר ללוקח. בהעברה האחרונה, ה _makeCall
הפונקציה הועברה בצורה שגויה של ההזמנה makerAsset
כפרמטר האחרון, כאשר הוא צריך להיות של ההזמנה makerAssetData
.
כתוצאה מכך, כל פונקציונליות פרוקסי המסתמכת על makerAssetData
ויכוח יישבר.
כדי להיות עקבי עם הקריאה הקודמת ל _makeCall
וכדי לתמוך באופן מלא בפונקציונליות פרוקסי, שקול לעדכן את order.makerAsset
פרמטר order.makerAssetData
.
עדכון: קבוע ב משיכת בקשה מס '57.
[H02] כל אחד יכול למלא הזמנות פרטיות במילוי חלקי
הפרוטוקול מאפשר יצירת פקודות פרטיות וציבוריות. בהזמנות פרטיות, רק את allowedSender
כתובת, שצוינה על ידי היוצר במהלך יצירת ההזמנה, מסוגלת למלא את ההזמנה.
עם זאת, ב OrderMixin
חוֹזֶה, אימות עבור allowedSender
כתובת הוא בהיקף שגוי, כלומר הוא מוערך רק בתוך ההיגיון שמטפל במילוי הראשון של הזמנה. אם הזמנה פרטית מולאה חלקית, אזי ההמחאה עבור ה allowedSender
הכתובת כבר לא נגישה וההזמנה ניתנת למילוי על ידי כל אחד.
כדי להבהיר את הכוונה לגבי האם כל משתמש אמור להיות מסוגל למלא הזמנות פרטיות שמלאו חלקית או לא, שקול לתעד את הסיבה להתנהגות הנוכחית או לאמת את allowedSender
כתובת מחוץ להיקף המילוי הראשון כדי להבטיח שהיא תאושר בכל פעם שניסית מילוי.
עדכון: קבוע ב משיכת בקשה מס '58.
[H03] יצרן זדוני עלול לנצל מילוי חלקי כדי לגנוב את הנכסים של הלוקח
הזמנות מה OrderMixin
לחוזה יש את היכולת להתמלא חלקית. כדי לתמוך במילוי חלקי, הפרוטוקול דורש דרך לחשב את שני הצדדים של החלפות. שניהם getMakerAmount
ו getTakerAmount
שדות מוגדרים על ידי יוצר ההזמנה למטרה זו בדיוק.
בעת מילוי הזמנה, על המקבלים לספק את ה- makingAmount
או takingAmount
ערכים וכן א thresholdAmount
ערך. ישנם שני נתיבי קוד שונים שניתן לקחת, בהתבסס על אם makingAmount
או takingAmount
סופק.
הראשון הוא כאשר makingAmount
פרמטר מוגדר. זה יכול לקטוע מה היא makingAmount
ערך וגם לחשב את takingAmount
ערך עבורו. במצב זה, ה thresholdAmount
מבטיח שה- takingAmount
הערך שנלקח הוא לא גדול באופן בלתי צפוי.
השני הוא כאשר takingAmount
פרמטר מוגדר. במקרה כזה, זה יקרה לחשב את makingAmount
ערך, עם אפשרות של לקצץ אותו ו חישוב מחדש של takingAmount
ערך אם זה יקרה. במצב זה, ה thresholdAmount
ערך מבטיח כי makingAmount
הערך המוחזר הוא לא קטן באופן בלתי צפוי.
קיימות שתי שיטות ניצול, כל אחת ייחודית לאחד מנתיבי הקוד שהוזכרו קודם לכן. שיטות ניצול אלו דורשות זדוניות getMakerAmount
ו getTakerAmount
פונקציות. ליישום פשוט של פונקציות אלו תהיה התנהגות זהה ל AmountCalculator
's getMakerAmount
ו getTakerAmount
פונקציות, אבל עם מתג מקודד קשה שיאלץ אותם להחזיר ערך נשלט על ידי תוקף בעת הצורך.
דפוס הניצול הראשון, הפחות חמור, כולל את נתיב הקוד הראשון שבו makingAmount
הערך מצוין בסדר מילוי. יצרן זדוני יחכה להזמנת מילוי שמפרטת makingAmount
להופיע ב-mempool כדי להפעיל אותו מראש. הם היו מרוקנים את כל הערך מלבד 1 מהצד של היוצר ואז מפעילים כוח _callGetTakerAmount
כדי להחזיר את הסכום שצוין בסכום של המשתמש thresholdAmount
ערך (או הקצבה שלהם אם היא פחותה). כאשר העסקה של המשתמש תעבור לבסוף, הוא יחליף את מלוא הסכום thresholdAmount
שווי של takerAsset
עבור יחידה אחת של makerAsset
. ניצול זה מוגבל על ידי הסכום שניתן על ידי thresholdAmount
ערך או הסכום של takerAsset
המשתמש מורשה ב- LimitOrderProtocol
חוזה.
דפוס הניצול השני, החמור יותר, כולל את נתיב הקוד השני שבו takingAmount
הערך מצוין. היצרן הזדוני יחכה באופן דומה להזמנת מילוי שציינה א takingAmount
ערך שיופיע ב-mempool. הם יפעילו את העסקה מראש ויכריחו את makingAmount
ערך שהוחזר על ידי _callGetMakerAmount
פונקציה להיות גבוהה יותר משני ה remainingMakerAmount
ו thresholdAmount
. הם גם היו קובעים את takingAmount
החזיר ערך על ידי _callGetTakerAmount
להיות הכמות של takerAsset
נכס מותר על LimitOrderProtocol
על ידי הלוקח. כאשר העסקה של הלוקח תצא לפועל, היא תעבור לקטוע את makingAmount
ערך ולאחר מכן לחשב מחדש את takingAmount
ערך. עם זאת, לא מובטח שחישוב מחדש זה יהיה נמוך יותר, ובמקרה זה ירוקן את הלוקח מכל takerAsset
שהתירו בחוזה. בנתיב הקוד הזה, ה thresholdAmount
הערך הוא להבטיח כי makingAmount
אינו נמוך מדי, אז לוקחים את כל הלוקחים takerAsset
הנכס אינו מסומן. הכספים שאבדו מוגבלים לסכום ה takerAsset
נכס שהמשתמש מותר ב- LimitOrderProtocol
חוזה.
ניצול זה אינו אפשרי ללא פקודות חלקיות, וליתר דיוק, הזמנות חלקיות עם זדון getMakerAmount
ו getTakerAmount
מימושים.
הנושא המרכזי של ה thresholdAmount
בדיקת הערך היא שהוא מכסה רק צד אחד של ההחלפה, אך ניתן לתמרן את הצד השני באמצעות הפעלה קדמית. אין הבטחות שהערך שהנוטל הציע במקור נשאר ללא שינוי. שקול להסיר makingAmount
חיתוך משני נתיבי הקוד והחזרה אם ההזמנה לא יכולה לתמוך במילוי גדול כפי שהתבקש. על ידי כך, ה thresholdAmount
ניתן להשתמש כדי להגביל במידה מספקת את הצד השני של ההחלפה ולהימנע מהתנהגות בלתי צפויה, אפילו בפקודות זדוניות.
עדכון: קבוע ב משיכת בקשה מס '83.
חומרה בינונית
[M01] ארגומנטים סטטיים שעברו לאחר ארגומנטים דינמיים
ב OrderMixin
החוזה, getTakerAmount
ו getMakerAmount
שדות בתים משמשים כארגומנטים עבור ה- _callGetTakerAmount
ו _callGetMakerAmount
פונקציות. שיחות אלו מספקות דרך לחשב צד אחד של ההחלפה על סמך הצד השני, והן מאפשרות למשתמשים למלא הזמנות חלקית.
השמיים getTakerAmount
/getMakerAmount
שדות הם משתנים דינמיים וארוזים לפני ה- takerAmount
ו makerAmount
ערכים ב _callGetTakerAmount
ו _callGetMakerAmount
פונקציות. זה אפשרי עבור יצרן זדוני לספק יותר נתונים מהצפוי ב- getTakerAmount
וgetMakerAmount
שדות לדחוף את takerAmount
ו makerAmount
בתים מעבר למקום שבו הם משוערים להיות בעת פענוח בפונקציה הבאה. זה מאפשר ליצרן להזיז את כמות הכרטיסים או היצרן שעברו בבתים מלאים ימינה ואפילו להחליף אותם לחלוטין אם מסופקים 32 בתים נוספים של נתונים.
משתמשים כבר צריכים לסקור ידנית את getTakerAmount
ו getMakerAmount
שדות בסדר, אבל די קשה לזהות את הטכניקה הזו. כמו כן ראוי לציין, התקפה זו חלה אפילו על בעלי אמון פנימי getMakerAmount
ו getTakerAmount
פונקציות. עבור רוב ההתקפות, מתן סכום סף סביר ימנע אובדן כספים.
כדי למנוע זאת, שקול לקודד את הארגומנטים הסטטיים לפני הארגומנטים הדינמיים כדי להימנע ממתן לארגומנטים הדינמיים שיטה לשלוט על הארגומנטים הסטטיים.
עדכון: לא קבוע. צוות 1 אינץ' אמר:
נקפיד במיוחד על אימות קבלנים. אנו ננסה ליישם אימות שפיות של מגברים ב-sdk שלנו שיעזור בסינון הזמנות שעלולות להיות זדוניות.
[M02] פקודות ERC721 ניתנות למניפולציה
אפשר להחליף יותר מסתם ERC20s דרך OrderMixin
על ידי פריסת חוזה שחולק את אותו בורר פונקציות כמו של IERC20 transferFrom
, ומתן חוזה זה בתור makerAsset
או takerAsset
בהזמנה.
ה-proxies מחוץ לתחום, כלומר, ERC721Proxy
, ERC721ProxySafe
, ו ERC1155Proxy
חוזים פועלים לפי דפוס זה כדי לספק תמיכה ERC721
ו ERC1155
אסימונים. מכיוון שיש לקרוא ל-proxies עם אותו דפוס כמו IERC20 transferFrom
שיחה, החתימה חייבת להתחיל עם address from
, address to
ו uint256 amount
. כל דבר אחר שהשליחים דורשים ניתן להעביר לאחר, ומוגדר בהזמנה כ makerAssetData
ו takerAssetData
.
ERC1155s יכולים באופן טבעי להעביר מספר מרובים מאותם אסימוני מזהה בבת אחת, כלומר ERC1155Proxy
החוזה עושה שימוש ב amount
שדה. מצד שני, ERC721
אין שימוש ברור עבור amount
שדה. מכיוון שהם מייצגים אסימונים שאינם ניתנים לשינוי, מזהה אסימון ספציפי יהיה קיים רק אחד, מה שהופך את amount
שדה חסר תועלת. בגלל זה, היישום עבור שניהם ERC721Proxy
ו ERC721ProxySafe
חוזים משתמשים בדרישות amount
שדה בתור tokenId
במקום.
עומס יתר זה של amount
פרמטר יוצר אפשרות למילוי חלקי ERC721
הזמנות על מנת לרכוש אסימונים רשומים בנפרד במחירים מוזלים. לדוגמה, יכול להיות מקרה שבו למשתמש יחיד יש מספר ERC721
s של אותו חוזה מותר להעביר על ידי ERC721Proxy
חוזה ומפרט אותם בסדרי הגבלה נפרדים.
אם צווי הגבלה מספקים גם את getMakerAmount
ו getTakerAmount
שדות, ניתן יהיה למלא אותם חלקית ERC721
הזמנות. מאז ההזמנה amount
השדה למעשה מתאים ל- tokenId
, משתמש זדוני יכול למקם מילוי חלקי על ERC721
עם ה-tokenId הגבוה יותר, וכתוצאה מכך א makingAmount
/takingAmount
של ERC721
שיכול להתאים לנמוך יותר tokenId
. התוצאה היא ה ERC721
עם התחתון tokenId
יועבר במחיר של (higher tokenId price) * (lower tokenId's id) / (higher tokenId's id)
.
לניצול הזה יש כמה דרישות:
- מְרוּבֶּה
ERC721
s מאותו חוזה לאפשר על אחד מהםERC721
ייפוי כוח על ידי בעלים יחיד. - הזמנה פתוחה עבור אחד מה
ERC721
זה לא הנמוך ביותרtokenId
מבין המותרים. - מילוי חלקי מותר בהזמנה.
כדי להסיר לחלוטין את האפשרות של חלקית ERC721
ממלא, שקול להפריד את amount
ו tokenId
טיעונים. בין אם הטיעונים מופרדים או לא, שקול גם לתעד זאת כדי להתריע בפני משתמשים על התנהגות זו וכדי להימנע מדפוס זה בעתיד.
עדכון: קבוע ב משיכת בקשה מס '59.
[M03] הנחות עשרוניות לא מתועדות
השמיים LimitOrderProtocol
החוזה יורש את ChainlinkCalculator
חוזה באמצעות ה OrderMixin
חוֹזֶה. חוזה זה חושף שתי פונקציות כדי לאפשר את השימוש באורקל של Chainlink במהלך פרדיקטים לבדוק והחיפוש של ה כמות יצרן/כמות הלוקח.
עם זאת, החוזה מניח הנחות לא מתועדות לגבי מספר העשרונים שהאורקלים של Chainlink צריכים לדווח בהם, כמו גם מספר העשרונים שהפרמטרים של הפונקציה צריכים להכיל. בתרחישים מסוימים, זה עלול להוביל להתנהגויות בלתי צפויות, כולל תמחור שגוי של נכסים ואובדן לא מכוון של כספים.
ליתר דיוק, לאורך כל החוזה ההנחה הסמויה היא שהאורקלים של Chainlink ידווחו עם דיוק של 18 עשרונים. עם זאת, לא כל האורקלים של Chainlink דווח עם מספר זה של עשרונים. למעשה, אם האורקל מדווח על צמד אסימונים שהוא במונחים של מטבע (לדוגמה, דולר), יהיה לו רק 8 עשרונים של דיוק. מכיוון שאין הגבלות על אשר ניתן להשתמש באורקלים, אין להניח הנחות מרומזות לגבי מספר העשרונים שהם ידווחו איתם.
בקשר לכך, קיימת הנחה מרומזת שה- amount
פרמטר עבור ChainlinkCalculator
פונקציות ישתמשו ב-18 עשרונים, יחד עם ההצהרה המפורשת המטעה ש- singlePrice
פונקציה Calculates price of token relative to ETH scaled by 1e18
. במציאות, אפילו עם אורקל זה עושה דוח עם 18 עשרונים, ערך ההחזר של ה singlePrice
הפונקציה תתאים לפי מספר העשרונים של ה- amount
פרמטר, שאינו בהכרח 18 עשרונים.
באופן דומה, doublePrice
הפונקציה מניחה ששני אורקלים של Chainlink ידווחו עם אותו מספר עשרוניות, מה שיגרום לסטייה של התוצאה של הפונקציה מהציפיות.
שקול תיעוד מפורש של הנחות לגבי מספר העשרונים שהפרמטרים וערכי ההחזר צריכים להיות במונחים של. יתרה מזאת, שקול להגביל חישובים התלויים באורקל ששוברים את ההנחות הללו, או שהחישובים הרלוונטיים יקחו בחשבון את מספר העשרונים בפועל.
עדכון: קבוע ב משיכת בקשה מס '75.
חומרה נמוכה
[L01] קבועים לא מוצהרים במפורש
יש כמה מקרים של ערכים מילוליים בשימוש עם משמעות בלתי מוסברת בבסיס הקוד. לדוגמה:
- ב
OrderMixin
החוזה,_remaining
המיפוי עמוס מדי מבחינה סמנטית (כפי שהוסבר בגיליון עומס סמנטי של מיפוי) כדי לעקוב אחר כמות הנכס שנותר עבור הזמנה שמלאה חלקית כמו גם אם הזמנה מולאה לחלוטין. באופן ספציפי,0
פירושו שלא בוצע מילוי הקשור להזמנה,1
פירושו שכבר לא ניתן למלא הזמנה, וכל דבר גדול מ1
פירושו שיש עוד סכום המשויך להזמנה שניתן למלא. - ב
ChainlinkCalculator
חוזה, הערך המילולי1e18
משמש ב-singlePrice
פונקציה.
כדי לשפר את קריאות הקוד ולהקל על הפירוק מחדש, שקול להגדיר קבוע עבור כל מספר קסם, לתת לו שם ברור ומובן מאליו. עבור ערכים מורכבים, שקול להוסיף הערה מוטבעת המסבירה כיצד הם חושבו או מדוע הם נבחרו.
עדכון: קבוע ב משיכת בקשה מס '75 ו משיכת בקשה מס '76.
[L02] צדדים זדוניים עלולים למנוע ביצוע פקודות מותרות
השמיים OrderMixin
חוזה מאפשר למשתמשי יצרן להגיש פקודות מותרות כך שניתן לבצע אותם בעסקה אחת, במקום שתצטרך לבצע עסקה נפרדת לצורך אישורים. כמו כן, מקבלי הזמנות יכולים להגיש אישור משלהם במהלך מילוי ההזמנה לאותה מטרה.
אולם משום שהיתר היוצר מצוי בתוך ה להזמין, הן ההיתרים של היוצר והן של הלוקח יהיו נגישים בזמן שעסקת מילוי ההזמנה נמצאת ב-mempool. זה יאפשר לכל משתמש זדוני לקחת את ההיתרים הללו ולבצע אותם בחוזי הנכסים המתאימים תוך הפעלת עסקת המילוי מראש. כי בהיתרים אלו יש א nonce
כדי למנוע התקפת הוצאה כפולה, עסקת המילוי של ההזמנה תיכשל כתוצאה מניסיון להשתמש באותו אישור ששימש רק במהלך ההרצה.
למרות שאין סיכון אבטחה, והיצרן יכול ליצור הזמנה חדשה ולאשר מראש את העסקה, מתקפה זו בהחלט יכולה להשפיע על השימושיות של הזמנות מותרות. אכן, תוקף בעל מוטיבציה יכול לחסום את כל פקודות מותרות עם התקפה זו. שקול לאמת אם ההיתר כבר הוגש, או אם הקצבה מספיקה, במהלך מילוי ההזמנה. שקול גם ליידע את המשתמשים על ההתקפה האפשרית הזו במהלך הרכבת ההזמנה.
עדכון: לא קבוע. צוות 1 אינץ' אומר:
היו לנו בדיקות אישור בעבר, אבל החלטנו לפשט את זרימת ההיתרים כדי פשוט לחזור על אישורים לא מוצלחים. נחשוב על הדרכים להודיע ליצרנים על הנושא.
[L03] קוד משוכפל
ישנם מקרים של קוד משוכפל בתוך בסיס הקוד. שכפול קוד יכול להוביל לבעיות בהמשך מחזור חיי הפיתוח ומשאיר את הפרויקט נוטה יותר להכנסת שגיאות. שגיאות כאלה יכולות להופיע בטעות כאשר שינויים בפונקציונליות אינם משוכפלים בכל מופעי הקוד שאמורים להיות זהים. דוגמאות לקוד משוכפל כוללות:
במקום לשכפל קוד, שקול להחזיק רק בחוזה או ספרייה אחת המכילה את הקוד המשוכפל ולהשתמש בו בכל פעם שהפונקציונליות המשוכפלת נדרשת.
עדכון: מקובע חלקית פנימה משיכת בקשה מס '60.
[L04] חבילת בדיקות שגויה או מטעה
ישנם מקרים בחבילת הבדיקות בהם הבדיקות חורגות מההתנהגות המצופה שלהן. לדוגמה:
- השמיים
ChainlinkCalculator
החוזה עובר בירושה על ידיOrderMixin
חוֹזֶה. עם זאת, במהלך הבדיקותAmountCalculator.arbitraryStaticCall
הפונקציה משמשת לקריאת ה-ChainlinkCalculator
חוזה כחוזה חיצוני ועצמאי. למרות שהתוצאה היא זו שצפויה, הבדיקה צריכה לשקף את ההתנהגות עם העיצוב הנוכחי של המערכת ומקרה השימוש הצפוי על ידי קריאהChainlinkCalculator
מתפקד ישירות מבלי להשתמש בקריאה הסטטית השרירותית. - למרות שחוזי ה-proxy היו מחוץ לתחום, שמנו לב שכאשר בדקנו את הפרוטוקול עם נכסי ERC721,
ERC721Proxy
החוזה אינו משמש להחלפת הנכסים שבו חבילת בדיקות.
מכיוון שחבילת הבדיקה עצמה נמצאת מחוץ לתחום הביקורת, אנא שקול לבדוק היטב את חבילת הבדיקה כדי לוודא שכל הבדיקות פועלות בהצלחה בהתאם למפרטי הפרוטוקול.
עדכון: קבוע ב משיכת בקשה מס '57, משיכת בקשה מס '59, ו משיכת בקשה מס '61.
[L05] טעויות והשמטות באירועים
לאורך בסיס הקוד, אירועים נפלטים בדרך כלל כאשר מתבצעים שינויים רגישים בחוזים. עם זאת, לאירועים רבים חסרים פרמטרים שצורפו לאינדקס ו/או חסרים פרמטרים חשובים. לדוגמה:
יש גם פעולות רגישות שחסרות בהן אירועים, כגון:
שקול להוסיף יותר אינדקס אירועים קיימים ולהוסיף פרמטרים חדשים היכן שהם חסרים. כמו כן, שקול לפלוט את כל האירועים בצורה כה מלאה עד שניתן יהיה להשתמש בהם כדי לבנות מחדש את מצב החוזה על ידי שירותים מחוץ לרשת.
עדכון: לא קבוע. עם זאת, צוות ה-1 אינץ' הוסיף orderRemaining
פרמטר ל OrderCanceled
אירוע ב משיכת בקשה מס '62.
צוות 1 אינץ' אומר:
מצאנו שרק תת-קבוצה מוגבלת של נתונים נדרשת כדי לספק את צרכי החזית. במקרה של ניתוח נרחב, כל השדות המוצעים זמינים באמצעות מעקב. ל
OrderRFQMixin
אנו מצפים מעושי שוק לבנות דרך מתוחכמת משלהם לעקוב אחר ההזמנות שבוטלו.
[L06] שינויים באחסון במהלך פליטת אירוע
ב NonceManager
חוזה, כאשר ה NonceIncreased
אירוע נפלט, גם האינונס של שולח ההודעה גדל.
ביצוע פעולות מרובות בו-זמנית יכול להפוך את בסיס הקוד לקשה יותר לנמק לגביו, נוטה יותר לשגיאות, ועלול להוביל להתעלמות או לאי-הבנה של פעולות.
כדי לשפר את הכוונה הכללית, הקריאות והבהירות של הקוד, שקול להגדיל את ערך ה-nonce לפני פליטת האירוע.
עדכון: קבוע ב משיכת בקשה מס '63.
[L07] מתודולוגיות פענוח לא עקביות עלולות לגרום לאי-התאמות בתוצאות
כדי לתמוך בכל ההרחבה והגמישות שלו, פרוטוקול Limit Order צריך להתמודד באופן שגרתי עם נתוני בתים דינמיים וערכי החזר שרירותיים מחוזים חיצוניים. כתוצאה מכך, הפרוטוקול כולל א ArgumentsDecoder
ספריה כדי להמיר ביעילות רבה יותר ערכי בתים דינמיים לסוגי נתונים בסיסיים. עם זאת, ספרייה זו אינה בשימוש בלעדי, ובמקרים מסוימים abi.decode
משמש במקום זאת. בנוסף, חלק מהחוזים משתמשים abi coder v1
בזמן שאחרים משתמשים abi coder v2
. הראשון מתפקד בצורה דומה יותר ל- ArgumentsDecoder
הספרייה, ואילו האחרונה מבצעת בדיקות נוספות בעת פענוח.
השימוש הלא עקבי במתודולוגיות הפענוח השונות הללו עלול לגרום לאי התאמה עדינים בין הכוונה להתנהגות בפועל של בסיס הקוד.
למשל, simulateCalls
הפונקציה משתמשת רק ב- ArgumentsDecoder.decodeBool
פוּנקצִיָה. אם ה simulateCalls
הפונקציה משמשת לבדיקת קריאות שיבוצעו בחלק הפרדיקט של הזמנה, ואז התוצאות שלה יכולות לסטות ממה שקורה בפועל כאשר תנאי הפרדיקט מוערכים, מכיוון שמשתמשים במתודולוגיות פענוח שונות.
אז, למשל, אם פרדיקט עושה חיצוני staticcall
לפונקציה כלשהי שמחזירה א uint256
ערך גדול מאחד ולא מהצפוי bool
, אז השיחה הזו תחזור, מכיוון שערך ההחזר הוא מפוענח עם abi coder v2
's abi.decode
שלא יקבל ערכים כמו bool
. עם זאת, אם אותה שיחה בדיוק מתבצעת עם simulateCalls
, ואז זה פשוט יסומן כ true
, כי decodeBool
מתייחס לכל ערך גדול מאפס כאל true
.
לעשות simulateCalls
הפונקציה משקפת באופן מלא את ההתנהגות של קריאות פרדיקט בפועל, שקול לשנות אותה לשימוש abi.decode
.
עדכון: קבוע ב משיכת בקשה מס '82.
[L08] היעדר אימות קלט
השמיים fillOrderToWithPermit
ו fillOrderTo
פונקציות של OrderMixin
חוזה, כמו גם את fillOrderRFQToWithPermit
ו fillOrderRFQTo
פונקציות של OrderRFQMixin
חוזה, אל תאמת את target
פרמטר כתובת.
זה מאפשר למשתמש להעביר בשוגג את כתובת האפס וכתוצאה מכך לנעול את הנכסים שהוא אמור לקבל לאחר מילוי הזמנה.
כדי להבטיח שמשתמשים לא ינעלו בטעות את הכספים שלהם, שקול לאמת את זה target
כתובת אינה שווה לכתובת האפס בפונקציות המצוטטות.
עדכון: קבוע ב משיכת בקשה מס '78.
[L09] כיסוי בדיקת יחידה נמוך
כיסוי הבדיקה ליחידה לכל הפרויקט הוא בסביבות 75%, כאשר לחלק מהחוזים כיסוי נמוך במיוחד.
בהתחשב בחשיבותן של בדיקות יחידה לאימות קוד ולמניעת רגרסיות בעת עיבוד מחדש ופיתוח של תכונות חדשות, אנו מעודדים להגדיל באופן משמעותי את כיסוי בדיקות היחידה ל-95% לפחות, ולכלול מקרי קצה המכסים אפילו מצבים לא סבירים.
עדכון: לא קבוע.
[L10] תיעוד מוטבע מטעה או חלקי
לאורך בסיס הקוד זוהו כמה מקרים של תיעוד מוטבע מטעה ו/או חלקי ויש לתקן אותם.
להלן מקרים של תיעוד מוטבע מטעה:
- ב
ChainlinkCalculator
החוזה,singlePrice
פונקציה NatSpec@notice
תג אומר את זהCalculates price of token relative to ETH scaled by 1e18
, אבל למעשה, התוצאה שלו היא ערך ofamount
אסימונים בקנה מידה לפי1e18
, כאשר האורקל עשוי שלא לדווח במונחים של ETH (עבור זוג שאינו כולל ETH, למשל). - ב
OrderRFQMixin
החוזה,invalidatorForOrderRFQ
פונקציה NatSpec@return
תג מטעה, מכיוון שייתכן שהציטוט לא מולא כדי שהביט המבטל בהתאמה נקבע. יכול להיות שההזמנה גם בוטלה. - על קווים 147, 165, ו 188 of
OrderMixin.sol
, ה-NatSpec@param
התגים אינם דקדוקיים. - באינטרנט 20 of
ERC1155Proxy.sol
, ה@notice
תג מציין שה-hash המחושב הוא תוצאה של hashing שלfunc_733NCGU
פונקציה, היכן שהיא צריכה להיותfunc_301JL5R
לתפקד במקום.
להלן מקרים של תיעוד מוטבע חלקי:
- פונקציות ב
AmountCalculator
החוזה אינו מתאר אף אחד מהפרמטרים. - ב
ChainlinkCalculator
החוזה,singlePrice
וdoublePrice
פונקציות אינן מתארות את כל הפרמטרים. - ב
ImmutableOwner
חוזה, למשתנה הציבורי ולשינוי אין NatSpec. - ב
InteractiveNotificationReceiver
החוזה,notifyFillOrder
הפונקציה אינה מתארת אף אחד מהפרמטרים. - ב
LimitOrderProtocol
החוזה,DOMAIN_SEPARATOR
לפונקציה אין NatSpec. - אירועים ומיפויים ב
NonceManager
אין NatSpec. - ב
OrderRFQMixin
חוֹזֶה,cancelOrderRFQ*
פונקציות אינן מתארות את ערכי ההחזרה. - ב
OrderMixin
חוזה, מספר פונקציות חסרות NatSpec מלא. - באינטרנט 168 of
OrderMixin.sol
ובקו 71 ofOrderRFQMixin.sol
, הוא חסר את@dev
תָג. - פונקציות ב
PredicateHelper
החוזה אינו מתאר את כל הפרמטרים.
תיעוד מוטבע ברור הוא הבסיסי להתוויית כוונות הקוד. חוסר התאמה בין התיעוד המוטבע והיישום עלול להוביל לתפיסות שגויות חמורות לגבי האופן שבו המערכת צפויה להתנהג. שקול לתקן שגיאות אלו כדי למנוע בלבול עבור מפתחים, משתמשים ומבקרים כאחד.
עדכון: קבוע חלקית. תיעוד מטעה המוזכר ב משיכת בקשה מס '75 ו משיכת בקשה מס '77.
צוות 1 אינץ' אומר:
תיקנו מסמכים מטעים. השלמת המסמכים תיעשה מאוחר יותר.
[L11] הזמנות DoS אפשריות בעת שימוש בווים
השמיים OrderMixin
החוזה מיישם פונקציונליות למילוי הזמנות חילופיות גנריות מחוץ לרשת, שיכולות להיות להן תנאים להצלחתן. במהלך מילוי ההזמנה, ההזמנה יכולה בדוק את תנאי ה"פרדיקט" המוגדרים מראש לפני המשך הביצוע.
עם זאת, מכיוון שתנאי היסוד הללו עלולים למקד את ההיגיון של כל חוזה שרירותי, יצרן זדוני יכול להערים על מקבלי הזמנה להאמין שהזמנה מתנהגת כהלכה ושהיא תקפה כאשר בודקים אותה מחוץ לשרשרת, אך לאחר מכן להיכשל כאשר מנסים למלא את אותה הזמנה על השרשרת. שינוי זה בהתנהגות הפרדיקטים יכול להיעשות על ידי הפעלה מקדימה של מצב משתנה כלשהו שבו תלויות הפרדיקטים, על ידי בחינת הגז שנשלח או אפילו אילו כתובות מעורבות בשיחה, או על ידי היגיון אחר.
יתר על כן, אם היוצר הגדיר א אינטראקציה במהלך ההחלפה, ה interactionTarget
החוזה עלול לחזור מעצמו או לבטל את ההקצבה כדי למנוע מילוי הזמנה מוצלח, מה שמוביל למעשה לאותה תוצאה כמו פרדיקטים זדוניים.
למרות שנכסים לא יהיו בסיכון, למשתמשים או לבוטים שימצאו הזמנה חיובית יהיה הנטל המוגבר בניסיון לזהות הזמנות ספאם מסוג זה שעלולות להיראות לגיטימיות על פני השטח. במקרה שהם לא יצליחו לזהות הזמנות מסוג זה, הם ייגרמו בעלויות גז מבוזבז. כדי לצמצם את כמות הזמנות הספאם, שקול להגביל את היעדים הזמינים עבור ה-hooks האלה. שקול גם להזהיר משתמשים לגבי אפשרות זו לפני שהם מנסים למלא הזמנות.
עדכון: לא קבוע. צוות 1 אינץ' אומר:
אנחנו מטפלים בזה בקצה האחורי שלנו ונחשוב על הדרכים להודיע למקבלים אפשריים על הנושא.
[L12] עיגול יכול להיות שלילי עבור taker
ב OrderMixin
ו OrderRFQMixin
חוזים, כאשר הזמנה מתמלאת והלוקח מספק רק א makingAmount
or takingAmount
סכום, הפרוטוקול מנסה לחשב את הסכום המקביל של ההחלפה.
ישנן שתי בעיות בחישובים אלה, הראשונה היא שאין תיעוד או היגיון המגבילים את מספר העשרונים שבהם צריך להשתמש בפרמטרי הכמות, שאליהם התייחסנו ב- הנחות עשרוניות לא מתועדות נושא.
הנושא השני הוא שבמהלך החישובים הללו, הפרוטוקול מתעגל לטובת היצרן. בעיית העיגול עלולה להחמיר מאוד כאשר ההנחות העשרוניות המשתמעות נשברות, אך גם כאשר הכל במונחים הצפויים, עיגול יתרחש בסכומים קטנים ומשונים.
שקול לאפשר ללוקח לציין כמות מינימלית של makerAsset
נכס שהם מוכנים לקבל יחד עם סכום מקסימלי של takerAsset
נכס שהם מוכנים להחליף, כך שהקבלה של כל עיגול תהיה מפורשת יותר.
עדכון: לא קבוע. צוות 1 אינץ' אומר:
כמות הסף צריכה להספיק להגנה של הלוקח.
[L13] טיפול בפקודות סותרות כאשר חסרים פרמטרים
ב OrderMixin
החוזה, fillOrderTo
הפונקציה מבצעת שיחות פנימיות ל- _callGetMakerAmount
ו _callGetTakerAmount
מתפקד בכל פעם שמנסים מילוי או את makingAmount
או takingAmount
הפרמטרים הם אפס, בהתאמה, או אם makingAmount
הערך גדול מה- remainingMakerAmount
ערך.
השמיים _callGetMakerAmount
ו _callGetTakerAmount
שיחות יובילו להחזרות אם ההזמנה לא נוצרה עם getMakerAmount
or getTakerAmount
פרמטרים, בהתאמה, ומילוי חלקי מתבצע.
An תגובה מוטבעת לצד _callGetMakerAmount
ו תגובה מוטבעת לצד _callGetTakerAmount
טוענים כי "מותרים רק מילוי שלמים" אם ההזמנה לא נוצרה עם getMakerAmount
or getTakerAmount
פרמטרים.
עם זאת, ישנם נתיבי קוד שעבורם זה לא חל, מכיוון שנתיבים אלה אינם בודקים את ה length
s של שניהם getMakerAmount
ו getTakerAmount
פרמטרים.
באופן ספציפי, כש taker
מציין א takerAmount
ערך עבור הזמנה שיש לה רק א getMakerAmount
, אלא אם כן התקשרו אליו getMakerAmount
מחזיר סכום גדול מ remainingMakerAmount
, ניתן לבצע מילוי חלקי בניגוד לתיעוד המוטבע.
זה משאיר את הכוונה של נתיבי הקוד האלה לא ברורה. אם זו ההתנהגות הצפויה, שקול לשנות את התיעוד המוטבע כך שיהיה מפורש יותר. אם זו התנהגות לא מכוונת, שקול לבדוק תמיד את האורך של שני ה getMakerAmount
ו getTakerAmount
פרמטרים בו זמנית כך שהיישום מחזק את ההתנהגות המתוארת בתיעוד המוטבע.
עדכון: קבוע ב משיכת בקשה מס '79.
[L14] שימוש בשיחות Chainlink שהוצאו משימוש
השמיים ChainlinkCalculator
החוזה מיועד לשמש לשאילתות של אורקל של Chainlink. זה עושה זאת באמצעות שיחות אליהם latestTimestamp
ו latestAnswer
שיטות, שניהם הוצאו משימוש. למעשה, השיטות כבר אינן קיימות ב-API של צוברי Chainlink נכון לגרסה שלוש.
כדי למנוע אי התאמה עתידית אפשרית עם אורקל של Chainlink, שקול להשתמש ב- latestRoundData
במקום זאת.
עדכון: קבוע ב משיכת בקשה מס '67.
הערות ומידע נוסף
[N01] לא מייבא ממשקים
השמיים AggregatorInterface
נראה שהממשק הוא קבוצת משנה של קוד שהועתק ממנו ChainLink
מאגר הקודים הציבורי של. הממשק המלא כלול ב ChainLink
חבילת npm של חוזה.
במידת האפשר, כדי להפחית את הפוטנציאל לאי התאמה בממשק ולבעיות כתוצאה מכך, במקום להגדיר מחדש ו/או לשכתב ממשקים של פרויקט אחר, שקול להשתמש בממשקים המותקנים דרך חבילות ה-npm הרשמיות שלהם במקום זאת.
עדכון: קבוע ב משיכת בקשה מס '66.
[N02] תלות פרויקט שהוצאה משימוש
במהלך ההתקנה של התלות של הפרויקט, NPM מזהירה שאחת החבילות המותקנות, Highlight
, "לא יתמכו עוד ולא יקבל עדכוני אבטחה בעתיד".
למרות שלא סביר שחבילה זו עלולה לגרום לסיכון אבטחה, שקול לשדרג את התלות המשתמשת בחבילה זו לגרסה מתוחזקת.
עדכון: קבוע ב משיכת בקשה מס '64.
[N03] שיטות קריאות חיצוניות לצפייה אינן קריאות סטטיות
לאורך רוב בסיס הקוד, הפרוטוקול מבצע במפורש שיחות חיצוניות דרך OpenZeppelin's functionStaticCall
שיטה להגביל את האפשרות לשינויי מדינה כאשר הם לא צפויים או לא רצויים. עם זאת, ב- ChainlinkCalculator
חוזה, למרות הכוונה לבצע שיחות חיצוניות רק ל view
שיטות על אורקל של Chainlink, השיחות החיצוניות ב- singlePrice
ו doublePrice
פונקציות אינן נעשות באמצעות מפורש staticcall
s.
אמנם לא זיהינו חששות אבטחה מיידיים הנובעים מכך, אך כדי לצמצם את משטח ההתקפה, לשפר את העקביות ולהבהיר כוונה, שקול להשתמש במפורש staticcall
s, עבור כל השיחות החיצוניות אל view
פונקציות ב ChainlinkCalculator
חוזה.
עדכון: לא קבוע. צוות 1 אינץ' אומר:
אנו חושבים שסיבוך התחביר מבטל שיפורים בעקביות.
[N04] לא נכשל מוקדם עם הזמנות לא חוקיות
ב OrderMixin
החוזה, fillOrderTo
הפונקציה מטפלת במצב המיוחד כאשר הזמנה לא הוגשה בעבר (remainingMakerAmount == 0
), אך הוא אינו מטפל במפורש בתנאי כאשר ההזמנה אינה תקפה עוד (remainingMakerAmount == 1
).
בתרחיש האחרון, הפונקציה תחזור בסופו של דבר, אך רק לאחר שריפת כמויות לא טריוויאליות של גז. כדי להבהיר את הכוונה, להגביר את הקריאות ולהפחית את השימוש בגז, שקול לטפל במפורש בתרחיש של הזמנה לא חוקית לקראת תחילת הפונקציה.
עדכון: קבוע ב משיכת בקשה מס '68.
[N05] חוזי עוזרים לא סומנו כמופשטים
בסולידיטי, מילת המפתח abstract
משמש עבור חוזים שאינם חוזים פונקציונליים בפני עצמם, או שאינם מיועדים לשמש ככאלה. במקום זאת, abstract
חוזים עוברים בירושה על ידי חוזים אחרים במערכת כדי ליצור חוזים פונקציונליים עצמאיים.
בכל בסיס הקוד, ישנן דוגמאות לחוזי עוזרים שאינם מסומנים כמופשטים, למרות העובדה שהם לא נועדו לפרוס בעצמם. למשל, ה AmountCalculator
, ChainlinkCalculator
, ImmutableOwner
, NonceManager
, ו PredicateHelper
חוזים מורכבים כולם מקבוצת בסיס של פונקציות שנועדו לשמש חוזים בירושה.
שקול לסמן חוזי עוזר כ abstract
כדי לציין בבירור שהם נועדו אך ורק להוסיף פונקציונליות לחוזים היורשים אותם.
עדכון: לא קבוע. צוות 1 אינץ' אומר:
ניתן לפרוס את אותם עוזרים בנפרד. הם עוברים בירושה רק לצורך חיסכון בגז.
[N06] סדר פונקציות לא עקבי
בכל בסיס הקוד, סדר הפונקציות בדרך כלל עוקב אחר ה- הזמנה מומלצת במדריך סגנון סולידיות, שהוא: constructor
, fallback
, external
, public
, internal
, private
.
עם זאת, ב- OrderMixin
החוזה, public
checkPredicate
הפונקציה חורגת ממדריך הסגנון, חוצה את external
פונקציות.
כדי לשפר את קריאותו הכוללת של הפרויקט, שקול לתקן את סדר הפונקציות בכל בסיס הקוד, כפי שהומלץ על ידי מדריך סגנון Solidity.
עדכון: קבוע ב משיכת בקשה מס '69.
[N07] זרימת מילוי הזמנה לא עקבית
השמיים OrderMixin
ו RFQOrderMixin
חוזים שניהם מטפלים במילוי הזמנות חתומות, אך זרימת ההזמנות הכללית בין שני החוזים אינה עקבית.
OrderMixin
's fillOrderTo
הפונקציה עוקבת אחר הזרימה הכללית הזו (פסאודו-קוד):
if ((takingAmount == 0) == (makingAmount == 0))
else if (takingAmount == 0)
else (handle makingAmount == 0) THEN swapTokens
ואילו RFQOrderMixin
דומה fillOrderRFQTo
הפונקציה עוקבת אחר הזרימה הזו (פסאודו-קוד):
if (takingAmount == 0 && makingAmount == 0)
else if (takingAmount == 0)
else if (makingAmount == 0)
else revert THEN swapTokens
אין תובנות מהתיעוד מדוע התנאי הראשון בכל אחת משתי הפונקציות הללו שונה, או מדוע takingAmount
ו makingAmount
לא שניהם יכולים להיות אפס בפונקציה האחרונה. כמו כן, המקרה בו הן א makingAmount
וכן takingAmount
מסופקים הרבה יותר קל לנמק ב fillOrderRFQTo
פונקציה, מכיוון שהיא מטופלת בצורה ברורה בגמר else
לַחסוֹם.
כדי להבהיר את הכוונה ולהגביר את הקריאות הכוללת של הקוד, שקול לתקן את זרימת ההזמנה הכללית על פני שני החוזים הללו, או לתעד במפורש מדוע קיימים ההבדלים.
עדכון: לא קבוע. צוות 1 אינץ' אומר:
זה נובע מפונקציות תמחור מותאמות אישית בהזמנות מוגבלות. מאז
getMakerAmount
יכול להיות שונה מהותית מgetTakerAmount
, חשבנו שעדיף לא להגדיר אפשרות ברירת מחדל עבור הלוקח, כי זה כנראה יבלבל אותם במקרים שבהם המקבלים יהיו שונים.
[N08] הודעות השגיאה מעוצבות בצורה לא עקבית או מטעות
לאורך בסיס הקוד, ה require
ו revert
הודעות שגיאה, שנועדו להודיע למשתמשים על התנאים המסוימים הגורמים לכשל בעסקה, נמצאו בפורמט לא עקבי.
לדוגמה, כל אחת מהודעות השגיאה בשורות 85 של OrderMixin.sol
, 16 של ERC721ProxySafe.sol
, ו 26 של Permitable.sol
להשתמש בסגנון אחר.
בנוסף, כמה הודעות שגיאה מטעות:
הודעות שגיאה נועדו להודיע למשתמשים על תנאים כושלים, ולכן עליהם לספק מספיק מידע כדי שניתן יהיה לבצע תיקונים מתאימים לאינטראקציה עם המערכת. הודעות שגיאה לא אינפורמטיביות פוגעות מאוד בחוויית המשתמש הכוללת, ובכך מורידות את איכות המערכת. יתר על כן, הודעות שגיאה בפורמט לא עקבי עלולות ליצור בלבול מיותר. לכן, שקול לסקור את כל בסיס הקוד כדי לוודא שכל require
ו revert
להצהרה יש הודעת שגיאה בפורמט עקבי, מדויק, אינפורמטיבי וידידותי למשתמש.
עדכון: מקובע חלקית פנימה משיכת בקשה מס '81.
[N09] שימוש לא עקבי במשתני החזר בעלי שם
קיים שימוש לא עקבי במשתני החזר בעלי שם ב- OrderMixin
חוֹזֶה. כמה פונקציות להחזיר משתנים בעלי שם, אחרים להחזיר ערכים מפורשים, ואחרים להכריז על משתנה החזרה עם שם אבל לעקוף אותו עם הצהרת החזרה מפורשת.
שקול לאמץ גישה עקבית לערכי החזרה בכל בסיס הקוד על-ידי הסרת כל משתני ההחזרה בעלי שם, הכרזה מפורשת עליהם כמשתנים מקומיים והוספת הצהרות ההחזר הנחוצות במידת הצורך. זה ישפר גם את המפורש וגם את הקריאות של הקוד, וזה עשוי גם לעזור להפחית רגרסיות במהלך מחזירי קוד עתידיים.
עדכון: קבוע ב משיכת בקשה מס '73.
[N10] חישוב הגיבוב של ההזמנה אינו פתוח ל-API
השמיים external
פונקציות remaining
, remainingRaw
ו remainingsRaw
כולם מצפים ל-hash להזמנה לפעולה מוצלחת.
עם זאת, פונקציית העוזר _hash
, שמחזיר את ה-hash של הזמנה, יש private
רְאוּת. המשמעות היא שמשתמשים יצטרכו לארוז חלקים מההזמנות וממחרוזות הדומיין באופן ידני כדי לקבל את ה-hash של הזמנה.
כדי למנוע את הפוטנציאל לטעויות בעת חישוב גיבוב הזמנה וכדי לספק למשתמשים שיטה להפקת ה-hash של הזמנה, שקול להרחיב את החשיפה של _hash
פונקציה ל public
ושחזור השם ל hash
כדי להיות עקבי עם שאר הקוד.
עדכון: קבוע ב משיכת בקשה מס '74.
[N11] עומס סמנטי של מיפוי
השמיים _remaining
מיפוי ב OrderMixin
החוזה עומס יתר על המידה מבחינה סמנטית כדי לעקוב אחר מצב ההזמנות וכמות הנכסים הנותר הזמינה עבור הזמנות אלו.
שלושת המדינות שהיא יכולה לקחת על עצמה הן:
0
: הגיבוב של ההזמנה עדיין לא נראה.1
: ההזמנה בוטלה או מולאה לחלוטין.- כל
uint
גדול מ1
: השארmakerAmount
זמין למילוי בהזמנה פלוס1
.
עומס סמנטי זה דורש עטיפה ופריקה של ערך זה במהלך lookup
, cancellation
, initialization
, ו storage
.
עומס יתר סמנטי וההיגיון הדרוש כדי לאפשר אותו עלולים להיות מועדים לשגיאות ויכולים להפוך את בסיס הקוד לקשה יותר להבנה ולהגיון לגביו, הוא עשוי גם לפתוח את הדלת לרגרסיות בעדכונים עתידיים לקוד.
כדי לשפר את קריאות הקוד, שקול לעקוב אחר מצב ההשלמה של הזמנות במיפוי נפרד.
עדכון: לא קבוע. צוות 1 אינץ' ציטט שתיקון יעלה את עלויות הגז עבור fillOrder
פונקציה.
[N12] צווים עם היתר מאפשרים קריאה לחוזים שרירותיים
השמיים OrderMixin
החוזה יורש את Permitable
חוזה כדי לאפשר מילוי הזמנה בעסקה אחת עם נכסים שמקבלים כאלה permit
קריאות לשינוי הקצבאות.
עם זאת, שיחות ל Permitable
חוזה אל תאמת אם היעד הוא נכס מותר וגם לא אם הוא אפילו נכס, שעלול לאפשר למשתמש זדוני להעביר את הכתובת של חוזה שרירותי שעלול לבצע שיחה נוספת לפני השלמת מילוי ההזמנה.
למרות שהחוזה הוא מוגן מפני כניסה חוזרת, צמצום משטח ההתקפה ומניעת קריאה של חוזים חיצוניים במהלך הביצוע מומלצת תמיד. שקול להגביל את הנכס המעורב בהיתר לנכסים המעורבים בהזמנה או לרשימת היתרים של נכסים עבור הפרוטוקול.
עדכון: לא קבוע. צוות 1 אינץ' אומר:
OrderMixin
אין למעשה מידע על אסימונים בפועלmakerAsset
וtakerAsset
לפעמים יש פרוקסי או חוזי ביניים אחרים ומידע על אסימונים בפועל מאוחסן בכמה בתים שרירותיים. אז אין דרך מעשית להגביל איזה נכסpermit
נקרא.
[N13] solhint
לעולם אינו מופעל מחדש
לאורך בסיס הקוד, ישנם כמה solhint-disable
הצהרות, במיוחד אלה ברשת 23 ובקו 41 of RevertReasonParser.sol
, שאינם מסתיימים עם solhint-enable
.
בעד מפורשות ולהגביל ככל האפשר בעת השבתה solhint
, שקול להשתמש solhint-disable-line
or solhint-disable-next-line
במקום זאת, בדומה לקו 16 של אותו קובץ.
עדכון: קבוע ב משיכת בקשה מס '72.
[N14] שגיאות הקלדה
בסיס הקוד מכיל את שגיאות ההקלדה הבאות:
בנוסף של הפרויקט README
(מחוץ לביקורת זו) מכיל את שגיאות ההקלדה הבאות:
שקול לתקן שגיאות הקלדה אלה כדי לשפר את קריאות הקוד.
עדכון: קבוע ב משיכת בקשה מס '71 ו משיכת בקשה מס '77.
[N15] שימוש ב uint
במקום uint256
כדי להעדיף מפורשות, כל המקרים של uint
יש להכריז כ uint256
. בפרט, אלה ב- for
לולאות על קווים 98 ו 119 of OrderMixin.sol
וקווים 16 ו 30 of PredicateHelper.sol
.
עדכון: קבוע ב משיכת בקשה מס '70.
מסקנות
נמצאו 3 בעיות בחומרה גבוהה. כמה שינויים הוצעו כדי לפעול לפי שיטות עבודה מומלצות ולצמצם את משטח ההתקפה הפוטנציאלי.
- &
- 7
- אודות
- גישה
- פי
- חֶשְׁבּוֹן
- לרוחב
- לפעול
- פעולות
- נוסף
- כתובת
- יתרון
- תעשיות
- מאפשר
- כְּבָר
- למרות
- כמויות
- אנליזה
- API
- גישה
- טיעונים
- סביב
- נכס
- נכסים
- בדיקה
- עורפי
- ההתחלה
- להיות
- הטוב ביותר
- שיטות עבודה מומלצות
- קצת
- בוטים
- לִבנוֹת
- שיחה
- אשר
- מקרים
- לגרום
- שרשרת
- שינוי
- בדיקה
- בדיקות
- קוד
- מורכב
- מצב
- בלבול
- בניה
- מכיל
- חוזה
- חוזים
- תיקונים
- עלויות
- יכול
- זוג
- יוצר
- מַטְבֵּעַ
- נוֹכְחִי
- נתונים
- עסקה
- מניעת שירות
- פריסה
- עיצוב
- מפתחים
- צעצועי התפתחות
- DID
- נבדלים
- אחר
- תחום
- לְהַכפִּיל
- דינמי
- מוקדם
- אדג '
- לעודד
- במיוחד
- ETH
- אירוע
- אירועים
- הכל
- דוגמה
- חליפין
- צפוי
- ניסיון
- לנצל
- תכונות
- שדות
- בסופו של דבר
- ראשון
- לסדר
- גמישות
- תזרים
- לעקוב
- מצא
- מלא
- פונקציה
- כספים
- עתיד
- מִשְׂחָק
- גז
- כללי
- נתינה
- גדול
- מדריך
- טיפול
- שירים
- has has
- יש
- לעזור
- גָבוֹהַ
- מאוד
- איך
- HTTPS
- היברידי
- לזהות
- פְּגִיעָה
- ליישם
- חשוב
- יבוא
- כלול
- כולל
- להגדיל
- גדל
- מידע
- מידע
- תשתית
- תובנות
- כוונה
- אינטרס
- מִמְשָׁק
- מעורב
- בעיות
- IT
- גָדוֹל
- גדול יותר
- עוֹפֶרֶת
- מוביל
- סִפְרִיָה
- מוגבל
- קו
- נְזִילוּת
- ברשימה
- רשימות
- מקומי
- נראה
- בדיקה
- גדול
- יצרן
- עשייה
- שוק
- ממפול
- ראי
- מודל
- רוב
- הכי פופולארי
- כלומר
- תכונות חדשות
- לא פטרייתי
- ללא אסימונים
- רשמי
- לפתוח
- תפעול
- אפשרות
- אורקל
- להזמין
- הזמנות
- אחר
- בעלים
- תבנית
- פופולרי
- להציג
- מניעה
- מחיר
- תמחור
- פְּרָטִי
- תהליך
- פּרוֹיֶקט
- פרויקטים
- .
- פרוטוקול
- לספק
- מספק
- פרוקסי
- ציבורי
- לפרסם
- לִרְכּוֹשׁ
- איכות
- להעלות
- מציאות
- להפחית
- הסתמכות
- לדווח
- דוחות לדוגמא
- מאגר
- דרישות
- REST
- תוצאות
- החזרות
- סקירה
- הסיכון
- סיבובים
- הפעלה
- Sdk
- אבטחה
- שירותים
- סט
- משותף
- שיתופים
- משמרת
- דומה
- פָּשׁוּט
- קטן
- חכם
- חוזים חכמים
- So
- מוּצָקוּת
- דואר זבל
- במיוחד
- הוצאה
- מסחרי
- התחלה
- מדינה
- הצהרה
- הברית
- מצב
- אחסון
- סגנון
- הוגש
- הצלחה
- מוצלח
- בהצלחה
- תמיכה
- נתמך
- משטח
- מתג
- מערכת
- יעד
- מבחן
- בדיקות
- בדיקות
- דרך
- בכל
- זמן
- יַחַד
- אסימון
- מטבעות
- לעקוב
- מעקב
- עסקה
- סומך
- ייחודי
- עדכונים
- us
- שמישות
- ש״ח
- USDT
- משתמשים
- תועלת
- ערך
- לצפיה
- ראות
- לחכות
- מה
- רשימה לבנה
- בתוך
- לְלֹא
- תיק עבודות
- ראוי
- אפס