6 december 2021
Introductie
De overeenstemmen team vroeg ons om een reeks contracten te beoordelen en te controleren met als uiteindelijk doel de gemeenschappelijke bestuurscontracten te verbeteren en ze meer flexibiliteit te geven. We hebben de code bekeken en publiceren nu onze resultaten.
Systeem overzicht
Het systeem is gebaseerd op drie hoofdcontracten:
- A
SafeGuard
contract sjabloon. Dit contract is deadmin
eenTimelock
contract, en voegt een laag modulaire rollen toe aan deTimelock
's acties. Dit contract definieert verschillende rollen die afzonderlijke verantwoordelijkheid en toegang hebben over de status van een voorstel (wachtrij, annulering en uitvoering). - A
SafeGuardFactory
contract implementeert een nieuweSafeGuard
en een bijbehorende nieuweTimelock
contract. Vervolgens stelt het het tijdslotadres in deSafeGuard
contract en registreert de nieuweSafeGuard
in deRegistry
. - A
Registry
die een lijst bevat van geïmplementeerdeSafeGuards
met hun corresponderende versienummer.
De SafeGuardFactory
zal in principe spawnen nieuwe SafeGuard
wanneer het wordt genoemd. EEN SafeGuard
is een omslag Timelock
operaties die verschillende en gescheiden rollen toevoegt voor elke actie. Rollen zijn gestructureerd door het gebruik van OpenZeppelin's AccessControlEnumerable
contract dat meer flexibiliteit en compatibiliteit geeft aan de multisig-portefeuilles en zakelijke use-cases waarbij meerdere adressen dezelfde gedeelde rol kunnen hebben.
update: In het PR # 10, heeft het Tally-team besloten om de Registry
contract. De lijst met ingezette beveiligingen wordt nu opgeslagen in de SafeGuardFactory
contract, in de safeGuards
opsombare set, samen met hun versie opgeslagen in de safeGuardVersion
in kaart brengen.
rollen
De SafeGuard
contract definieert de volgende rollen:
update: De CREATOR_ROLE
rol is verwijderd als een oplossing voor het probleem L02.
strekking
We hebben commit gecontroleerd b2c63a9dfc4090be13320d999e7c6c1d842625d3
van de safeguard
opslagplaats. In scope zijn de slimme contracten in de contracts
map. echter, de mocks
directory werd als buiten het bereik beschouwd.
Veronderstellingen
Het is niet de bedoeling dat het systeem kan worden geüpgraded. De Registry
adres is ingesteld in de constructeur van de SafeGuardFactory
en elk SafeGuard
kan de Timelock
slechts één keer instellen. Dit betekent dat als een nieuwe Registry
wordt ingezet een nieuwe SafeGuardFactory
moet ook worden ingezet. Als de Timelock
implementatiewijzigingen, de oude SafeGuard
s zullen verouderd raken en er zullen nieuwe moeten worden ingezet.
Bovendien is het systeem sterk afhankelijk van de implementatie van een Timelock
contract dat buiten het bereik van deze audit werd geacht. Het team heeft de implementatie van de nog niet afgerond Timelock
contract. Op het moment van schrijven van dit rapport, de implementatie van timelock door de Compound wordt gebruikt in het project.
De codebase is in de loop van een week door twee auditors gecontroleerd en hier presenteren we onze bevindingen.
Kritieke ernst
Geen.
Hoge ernst
[H01] ETH kan in de worden vergrendeld Timelock
contract
De Tally
team oorspronkelijk hun implementaties gebaseerd op de grond van de GovernorBravoDelegate
Samengestelde overeenkomst.
Tijdens deze audit heeft de Tally
team ontdekt een beperking in de gouverneur van Compound, waar ETH rechtstreeks naar de Timelock
is niet beschikbaar voor gebruik door governance-voorstellen, en hoewel het niet permanent vastzit, moet er een uitgebreide oplossing worden gevonden.
Dit komt omdat de implementatie van de gouverneur vereist dat alle waarde van een voorstel wordt bijgevoegd als: msg.value
door het account dat de uitvoering activeert, op geen enkele manier de Timelock
ETH-fondsen.
De hetzelfde probleem werd later geïdentificeerd in de SafeGuard
uitvoering en het team is op de hoogte van het probleem en is bezig het op te lossen.
Overweeg bij het oplossen van het probleem de aanpak te gebruiken geadopteerd door de OpenZeppelin-bibliotheek voor hetzelfde nummer.
update: Vast in commit 7337db227edda83533be586135d96ddac4f5bf29
.
[H02] SafeGuardFactory kan worden ingevroren
De Registry
contract is bedoeld om alle SafeGuards
dat de SafeGuardFactory
produceert. Het heeft de externe register
functie die voor dit doel wordt gebruikt.
Op hetzelfde moment, de SafeGuardFactory
heeft, in zijn constructor, de toewijzing van de local registry
waarde naar de invoerwaarde. Er is geen mogelijkheid om de waarde van de . te wijzigen registry
variabel en om deze reden, als een nieuwe Registry
wordt ingezet, moet er ook een nieuwe fabriek komen.
De SafeGuardFactory
heeft createSafeGuard
functie, verantwoordelijk voor de eerste het inzetten van een nieuwe SafeGuard
dan een nieuwe Timelock
met het adres van de SafeGuard
as admin
, dan het instellen van de timelock
variabele van de SafeGuard
contract en tot slot het registreren van de SafeGuard
in het register.
Het probleem is dat elke oproep naar createSafeGuard
kan worden gedwongen te falen door een aanvaller die het deterministische adres van de nieuwe . direct kan registreren SafeGuard
vóór de oprichting ervan. Wanneer een contract creëert een new
instantie, zijn nonce wordt verhoogd en het adres waar het nieuwe exemplaar van het contract zou worden geïmplementeerd, kan worden bepaald door het oorspronkelijke contractadres en zijn nonce. Daarom kan een aanvaller vooraf veel van de adressen berekenen waar de nieuwe SafeGuards
worden ingezet en registreren die adressen in de Registry
door de te bellen register
functie. Dit zou resulteren in de oproepen naar: createSafeGuard
naar terugkeren omdat de Registry
bevat het adres al.
Om te voorkomen dat externe actoren publiekelijk de Register
contract, overweeg dan om de toegang tot de register
functie om oproepen uitsluitend door de SafeGuardFactory
.
update: Vast in PR # 10. Het Tally-team heeft de Registry
contract.
Middelmatige ernst
Geen.
Lage ernst
[L01] Uitgeschreven code
De Registry
contract omvat: een becommentarieerde regel code. Overweeg om het uit de codebase te verwijderen om de leesbaarheid te verbeteren.
update: Vast in PR # 10 en begaan 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L02] De beheerder van SafeGuard kan de rol van maker aan elk adres toewijzen
De SafeGuard
contract definieert de rol van een CREATOR_ROLE
die, zoals de naam al doet vermoeden, wordt toegewezen aan de maker van de beveiliging.
Echter, door een beroep te doen op de grantRole
functie van de AccessControlEnumerable
contract in de OpenZeppelin-contractbibliotheek kan een beheerder deze rol aan elk adres toekennen. Dit kan voor verwarring zorgen omdat de maker van de SafeGuard
kan alleen de zijn SafeGuardFactory
.
Door de hele codebase is deze rol alleen gebruikt om te voorkomen dat gebruikers interactie hebben met de setTimelock
functie van de SafeGuard
contract. Door het ontwerp zorgt het systeem ervoor dat: setTimelock
functie kan maar één keer worden gebeld, van in de SafeGuardFactory
contract.
Overweeg het verwijderen van de CREATOR_ROLE
rol van de SafeGuard
contract en het gebruik van de onlyOwner
verandering in de setTimelock
functie.
update: Vast in PR # 10.
[L03] Onjuiste interfacedefinitie en implementatie
De ISafeGuard
interface definieert niet de queueTransactionWithDescription
functie geïmplementeerd in de SafeGuard
contract, en tegelijkertijd definieert het de __abdicate, __queueSetTimelockPendingAdmin en __executeSetTimelockPendingAdmin functies, maar ze worden niet geïmplementeerd.
Om de correctheid en consistentie in de codebase te verbeteren, kunt u overwegen om de ISafeGuard
interface die precies overeenkomt met de SafeGuard
implementatie.
update: Vast in commit 7fd27df16fc879d990d36a167a0b6e719e578558
.
[L04] Ontbrekende docstrings
Bij sommige contracten en functies in de codebase ontbreekt documentatie. Bijvoorbeeld, sommige functies in de SafeGuard
contract.
Bovendien gebruiken sommige docstrings informele taal, zoals die boven de setTimelock
functie in de SafeGuard
contract.
Dit belemmert het begrip van de beoordelaars van de bedoeling van de code, wat van fundamenteel belang is om niet alleen de veiligheid, maar ook de correctheid correct te beoordelen. Bovendien verbeteren docstrings de leesbaarheid en vereenvoudigen ze het onderhoud. Ze moeten expliciet het doel of de bedoeling van de functies uitleggen, de scenario's waarin ze kunnen falen, de rollen die ze mogen aanroepen, de geretourneerde waarden en de uitgezonden gebeurtenissen.
Overweeg om alle functies (en hun parameters) die deel uitmaken van de openbare API van de contracten grondig te documenteren. Functies die gevoelige functionaliteit implementeren, zelfs als deze niet openbaar zijn, moeten ook duidelijk worden gedocumenteerd. Overweeg bij het schrijven van docstrings de: Ethereum Natural-specificatieformaat (NatSpec).
update: Gedeeltelijk gefixeerd PR # 10. De juiste docstrings zijn toegevoegd aan verschillende functies in de codebasis. Overweeg echter om naast de huidige wijzigingen de volgende wijzigingen aan te brengen:
- Toevoegen
description
de@param
in de docstring hierbovenqueueTransactionWithDescription
functie - Toevoegen
@param
in de docstring boven decreateSafeGuard
functie inSafeGuardFactory
contract - Toevoegen
@return
in docstrings boven de functies inSafeGuardFactory
contract.
[L05] Nutteloze of herhaalde code
Er zijn plaatsen in de codebase waar code wordt herhaald of niet nodig is. Enkele voorbeelden zijn:
- Lijnen 29-32 van de
Registry
contract zijn nutteloos, omdat de_add
functie van deEnumerableSet
contract voert deze controles al uit tegen de reeds ingestelde waarden. - Lijnen 62, 67, 73 en 78 van de
SafeGuard
contract herhalen allemaal exact dezelfde operatie. Overweeg het in te kapselen in een interne functie om dubbele code te voorkomen. - Lijnen 62-63 en 67-68 of
SafeGuard
worden herhaald. Overweeg ze in te kapselen in een enkele interne functie. - Het gebruik van
gasleft
om aan te geven hoeveel gas moet worden doorgestuurd in de aanroep van de functieexecuteTransaction
is onnodig. Op dat moment van executie wordt namelijk het volledige gas dat overblijft gebruikt om de executie voort te zetten. Als dit niet voor de duidelijkheid is, overweeg dan om degas
parameter van de oproep.
Overweeg om de voorgestelde fix toe te passen om een schonere code te produceren en de consistentie en modulariteit van de codebase te verbeteren.
update: Vast in PR # 10 en begaan 7fd27df16fc879d990d36a167a0b6e719e578558
.
Opmerkingen en aanvullende informatie
[N01] Inconsistente stijl
Er zijn enkele plaatsen in de codebasis waar stijlverschillen de leesbaarheid beïnvloeden, waardoor het moeilijker wordt om de code te begrijpen. Enkele voorbeelden zijn:
- De
Registry
contract gebruikt verschillende stijlen voor docstrings in het hele contract. - De
SafeGuard
contract zendt een gebeurtenis uit wanneer:queueTransactionWithDescription
wordt aangeroepen, maar er worden geen gebeurtenissen uitgezonden in andere functies die met transacties te maken hebben. - In het
SafeGuard
contract, soms waarde wordt gebruikt als benoemde parameter en soms _waarde is gebruikt.
Rekening houdend met de waarde die een consistente coderingsstijl toevoegt aan de leesbaarheid van het project, kunt u overwegen een standaard coderingsstijl af te dwingen met behulp van lintertools, zoals Solhint.
update: Vast in PR # 10 en begaan 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N02] Ontbrekende licentie
De volgende contracten binnen de codebase missen een SPDX-licentie-ID.
Om compilerwaarschuwingen te dempen en de consistentie in de codebase te vergroten, kunt u overwegen een licentie-ID toe te voegen. Overweeg tijdens het doen te verwijzen naar: spdx.dev richtlijnen.
update: Vast in PR # 10 en begaan 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N03] De afhankelijkheid van OpenZeppelin Contract is niet vastgezet
Om onverwacht gedrag te voorkomen bij het breken van wijzigingen worden vrijgegeven in toekomstige updates van het Bibliotheek van OpenZeppelin Contracts, overweeg dan om de versie van deze afhankelijkheid vast te zetten in het package.json bestand.
update: Vast in PR # 10.
[N04] Solidity-compilerversie is niet vastgezet
Overweeg in de hele codebasis de versie van de . vast te zetten Solidity-compiler naar de laatste stabiele versie. Dit zou moeten helpen voorkomen dat onverwachte bugs worden geïntroduceerd als gevolg van incompatibele toekomstige releases. Om een specifieke versie te kiezen, moeten ontwikkelaars rekening houden met zowel de functies van de compiler die nodig zijn voor het project als: de lijst met bekende bugs geassocieerd met elke Solidity-compilerversie.
update: Vast in PR # 10.
[N05] Typefout
Op verschillende plaatsen in de codebasis is het woord role
is verkeerd gespeld als rol
. Een voorbeeld hiervan is in de docstring binnen de constructor
van de SafeGuard
contract.
Overweeg deze typefouten te corrigeren om de leesbaarheid van de code te verbeteren.
update: Gedeeltelijk gefixeerd PR # 10. Terwijl de spelling van role
is gecorrigeerd, moet de opmerking "stel beheerdersrol in op een gedefinieerd beheerdersadres" zijn "stel beheerdersrol in op een gedefinieerd beheerdersadres". Bovendien is "uitvoeren" verkeerd gespeld in de SafeGuard
contract op lijn 69, lijn 82, lijn 96 en lijn 110 en "beschikbaar" is verkeerd gespeld op lijn 70, lijn 83, lijn 97, lijn 111. Overweeg ook om informele woorden zoals: "ga" in SafeGuard
contract met formele alternatieven zoals "gaan naar".
[N06] Verklaar uint als uint256
Er zijn verschillende gebeurtenissen in de codebase waar variabelen worden gedeclareerd van uint
gegevenstype in plaats van uint256
. Bijvoorbeeld de eta
variabele in de QueueTransactionWithDescription
gebeurtenis van de SafeGuard
contract.
Om explicietheid te bevorderen, zijn alle gevallen van uint
moet worden gedeclareerd als uint256
.
update: Vast in PR # 10 en begaan 7fd27df16fc879d990d36a167a0b6e719e578558
.
[N07] Ongebruikte import
De SafeGuard
contract importeert de console
contract, maar gebruikt het nooit.
Overweeg om ongebruikte importen te verwijderen om de leesbaarheid van de code te verbeteren.
update: Vast in PR # 10.
Conclusies
Er zijn één hoge en verschillende andere kleine kwetsbaarheden gevonden en er zijn aanbevelingen en oplossingen voorgesteld.
- &
- toegang
- Account
- over
- Actie
- acties
- Extra
- adres
- beheerder
- Alles
- al
- Hoewel
- api
- nadering
- rond
- controleren
- Eigenlijk
- wezen
- bugs
- bedrijfsdeskundigen
- Bellen
- gevallen
- Veroorzaken
- verandering
- lading
- code
- codering
- Gemeen
- Samenstelling
- verwarring
- overweging
- bevat
- voortzetten
- contract
- contracten
- kon
- schepper
- Actueel
- gegevens
- omgang
- Design
- ontwikkelaars
- anders
- ontdekt
- uitwerken
- ETH
- Event
- EVENTS
- voorbeeld
- fabriek
- Voordelen
- Tot slot
- Voornaam*
- Bepalen
- Flexibiliteit
- gevonden
- functie
- fondsen
- toekomst
- GAS
- Vrijgevigheid
- doel
- bestuur
- Gouverneur
- richtlijnen
- met
- hulp
- hier
- Hoge
- Hoe
- HTTPS
- geïmplementeerd
- Laat uw omzet
- meer
- Interface
- IT
- bekend
- taal
- laatste
- Bibliotheek
- Vergunning
- Lijn
- Lijst
- lokaal
- opgesloten
- keek
- maken
- Match
- modulaire
- multisig
- Overige
- presenteren
- project
- voorstel
- publiek
- publiceren
- registreren
- Releases
- verslag
- bewaarplaats
- Resultaten
- beoordelen
- veiligheid
- reeks
- het instellen van
- gedeeld
- slim
- Slimme contracten
- stevigheid
- Land
- stijl
- system
- Door
- overal
- niet de tijd of
- tools
- spoor
- Transacties
- updates
- us
- gebruikers
- waarde
- kwetsbaarheden
- Portemonnees
- week
- WIE
- binnen
- woorden
- het schrijven van