Celo Contracts Audit

The cLabs team working on the Celo platform asked us to review and audit the smart contracts for the protocol. We examined the code, and here we publish our findings.

About Celo

The Celo platform implemented a new blockchain protocol and a node software forked from Geth. It has a proof-of-stake based rewards system, built-in stablecoins, governance procedures, and a system for linking accounts and phone numbers. The main goal of Celo is to make the blockchain easily accessible for anyone with a mobile phone.

The contracts are divided into four main groups: common, governance, identity, and stability. Each have their own folder of the same name within the contracts folder.

The governance folder handles the election of validators, which verify transactions and earn rewards for participating in the Celo network. Governance also provides a means to make proposals, which are transactions that are voted on and then executed by the Celo protocol. It also contains mechanisms for reward distribution and enforcing penalties for validator misbehavior.

The identity folder handles the linking of accounts to cell phone numbers or other unique identifiers, as well as escrowing funds sent to accounts that have not yet been created.

The stability folder manages the stable token (Celo Dollars) and the reserve intended to keep this stable token collateralized. These files also contain the means for on-chain exchanges of both the native asset of the protocol (Celo Gold) and Celo Dollars.

Finally, the common folder contains resources used in multiple places throughout the protocol. A registry of all contracts and addresses is found here, which is used by most contracts in the protocol to find and access other contracts. Utilities like FractionUtil and FixidityLib exist here, as well as frequently needed contracts like GoldToken, which manages the Celo Gold.

Elections and Validators

The protocol encourages the creation of groups of validators, which are then voted for during every epoch to validate transactions on the Celo blockchain. To register as a validator, a user must lock up a certain amount of Celo Gold. Validators cannot be elected without being part of a validator group. This was chosen as a design feature in order to minimize the information gap between voters and validators. It also incentivizes good behavior of validators, since they risk being ejected from an otherwise good group if they misbehave.

To vote, Celo users lock up Celo Gold, and then they can apply this Celo Gold as votes to different validator groups. Roughly once a day, an election is held to determine the validators for the next day. Validator groups are elected, and then validator seats are distributed to them via the d’Hondt method. For every seat that a group receives, 1 validator account from the group is officially elected as a validator for the following epoch.

Validators and/or validator groups can be slashed if they act maliciously or are found to have too little up-time. This mechanism takes away a portion of their locked Celo Gold, and can forcibly remove validators from groups.

Importantly, users and validators receive rewards when voting for and participating in winning validator groups. Validators and groups that win elections receive rewards, as well as the voters for those groups. However, those who vote for losing groups receive nothing.

Proposal and Hotfixes

Users can create proposals, which are predefined transactions that can then be voted on. If these proposals are voted on and approved, they will be executed by the Governance contract, which is expected to be the owner of the mainnet Celo network.

The governance system also can execute hotfixes, transactions to fix errors that have been detected in the network. This process requires off-chain and on-chain coordination. First, the cLabs team will need to publicly disclose the hash of the transaction that will be executed. Secondly, two thirds of the Celo validators will need to whitelist this hash for deployment. The hotfix can only be triggered after it is whitelisted by the validators and approved by the approver.

Attestations and Identity

Celo implements a mapping which ties accounts to cell phone numbers, as well as an on-and-off chain system to verify that account owners are who they say they are. This is done via the attestations system, which basically involves validators sending secret SMS messages to cell numbers, and then account owners proving they have received them.

Privileged Roles

There are two roles with far-reaching power. The first is the aforementioned virtual machine (VM), identified by the address(0). The VM has the ability to make protocol-level changes. The VM is a powerful account, having the ability to do things like increase the total supply of Celo Gold or modify its oracles. The VM should only perform calls in an automated and predictable fashion, as dictated by the protocol.

The other role is the approver, which must approve all proposals before they can be executed. Initially, the approver is planned to be a multi-sig contract. The approver account does not receive rewards for participation.

The cLabs team plans to progressively decentralize the control of the approver and to protect control of the VM such that it is strictly limited to its intended functionality.

About the audit

The audit was divided in two phases. The first phase started in January 2020 and included a detailed review of all the smart contracts in the celo-monorepo up to that moment. The second phase started in March 2020 and covered all the changes to the smart contracts since the first phase started.

All the smart contracts within the contracts folder of the celo-monorepo are included in the audit scope. All other code within the repository was assumed to work correctly, as stated by the cLabs team and the Celo Docs. In particular, the specific details on how the virtual machine is implemented in the node software was out of scope of this audit, and the report was made assuming that it works as intended by the developers. Moreover, oracles were not a part of the current codebase and were not reviewed by OpenZeppelin.

2 critical and 4 high-severity issues were found. You can see the reports of phase 1 and phase 2 for extensive details of every vulnerability found and how they have been fixed or mitigated.

We are impressed by the handling of this complex monorepo by the cLabs team. They are following very good processes to keep their project healthy. Of special interest for us during this phase are the small and focused pull requests, with proper descriptions and tags, links to reported issues, peer reviews and extensive automated testing.

Looking at how the cLabs team has carefully implemented fixes and new features during the time we have been auditing the smart contracts gives us confidence in that they are ready to publish the system, to constantly help monitor and update it, and to grow the repository in the direction required by the ecosystem.

Before starting the deep dive into the smart contracts, we performed a general health check of the project which we are now including in this full report.


Project Overview

(as of January 9th, 2020)

Summary

The Celo project has a healthy number of active contributors and dedicated maintainers for every package.

The documentation inside and outside of the repository is good and the community has plenty of ways to get news about the project and to communicate with the cLabs team.

However, some areas of concern were identified:

  • It has many open issues, pull requests and branches without activity.
  • It has code and configuration files written in many different languages, which increases the learning curve to collaborate in some of the packages.
  • The unit test coverage report does not cover the whole project.
  • It has no documented security policy or instructions on how to communicate with the Celo developers to communicate a security issue.

Below we present our overview of the project.

We reported some health concerns and other security vulnerabilities and quality issues during the phase 1 of the audit.

Repository

The repository is hosted in GitHub, as part of the Celo organization.

It has 25 watchers, 77 stars and 48 forks.

The repository is a monorepo with 24 packages managed by lerna. This audit is limited to the smart contracts in the protocol package.

License

The project is free software.

The protocol package is released under the GNU Lesser General Public License version 3.

The rest of the project is released under the Apache License v2.0.

Languages

1345 files are written in TypeScript.
This is the majority of the source code in the repository.

There are also 116 files written in Solidity.
This audit is limited to the Solidity code.

There are configuration files written in JSON, YAML, XML, INI, and Windows Resource File, small code files written in Bash, Javascript, Qt Linguist, Java, Groovy, Python, Objective C, CSS, DOS Batch, Prolog, Sass, and C++, documentation files written in Markdown, and Dockerfiles.

Code Contributors

Total code contributors: 53

Maintainers

(According to the CODEOWNERS file)

  • Nam Chu Hoai (nambrot), with 99 commits.
  • J M Rossy (jmrossy), with 94 commits.
  • Aaron DeRuvo (aaronmgdr), with 77 commits.
  • Trevor Porter (tkporter), with 64 commits.
  • Ashish Bhatia (ashishb), with 64 commits. Inactive since november 2019.
  • Asa Oines (asaj), with 63 commits.
  • Mariano Cortesi (mcortesi), with 42 commits.
  • Connor McEwen (cmcewen), with 42 commits.
  • Anna K (annakaz), with 40 commits.
  • Tim Moreton (timmoreton), with 25 commits.
  • Audrey Penven (yerdua), with 17 commits.
  • Martin (m-chrzan), with 17 commits.

Frequent Contributors (during the last year)

  • Jean Regisser (jeanregisser), with 57 commits.
  • Victor “Nate” Graf (nategraf), with 30 commits.
  • Sami Mäkelä (mrsmkl), with 25 commits.
  • Pedro Gutiérrez (Pedro-vk), with 16 commits.
  • Yorke Rhodes (yorhodes), with 14 commits.

Past Frequent Contributors (now inactive)

Dependencies

The protocol package has 36 dependencies and 16 development dependencies.

Issues

Closed: 871.

Open:

Pull Requests

Closed: 1107.

Open:

  • Total: 55.
  • Without activity for a week: 22.
  • Without a reply: 12.
  • With more than 10 comments: 10.

Releases

Releases are tagged with Semantic Versioning.

Number of releases so far: 10.

First release on github: wallet v1.3.1 on July 23, 2019.

Release cadence: No stable release cadence. No release in the last month.

Every release includes the files:

  • Source code (zip)
  • Source code (tar.gz)

Projects

Celo uses GitHub projects to manage the pending tasks.

The relevant projects for this audit are Betanet 2019 and Betanet 2020.

Together, these projects have 199 tasks in the To do, 10 In progress, 11 with Review in progress, and 56 Done.

Branches

There are 144 branches in the organization repository. 122 are active and 21 are stale.

Development happens in the master branch.

Contributing

The project has documented guidelines for code contributors. There are also instructions to build the project.

The protocol package uses solhint and tslint to follow a consistent code style.

The project requires contributors to respect the Celo Community Code of Conduct.

Testing

Unit tests for the contracts of the protocol package are defined in the test directory. They are run by Circle CI on Github pull requests.

There is a test coverage report, but it only includes the mobile package.

There are two test networks: Alfajores and Baklava.

Documentation

The project has a main README on the root of the source code repository. It includes general information about Celo, links to documentation, issues, contributing guide, community resources, and the licence. It also has a brief explanation of all the packages of the project.

There is a main documentation website aimed at developers.

Installation

The mobile wallet application can be installed from the Android and iOS app stores.

No installers or binary files have been published for the nodes.

Docker images are available to easily connect to the Baklava test network.

Communication

The people from the project have multiple ways to communicate with its users:

Update from the cLabs team:
The primary place of communication is Discord (with 1359 members).
The more active Twitter account is @CeloOrg (with 1223 tweets and 5799 followers).
(as of April 29th, 2020)

Security

The project does not have a Security Policy document.
There are no instructions for responsible disclosure of security vulnerabilities. There is no Bug Bounty program.

Audits

The project has not published any audit reports yet.


The commit for the first phase of the audit is 7be22605e172ca536c028e408b147aab83202e4a. All code within the contracts folder is included in the audit scope.

Summary

Overall, we were quite pleased with the quality of the code we were presented. We were happy to find that the cLabs team had thoroughly considered some of the more complex nuances of the incentive and governance structure of their contracts, including how to manage ownership of privileged roles. We valued that the Celo Docs were up-to-date and had detailed descriptions of the protocol’s intended functionality. We found that in some places, the code could have a bit more consistency and input validation, but generally we were impressed with the technical depth of the contracts.

Vulnerabilities

Below, we list all vulnerabilities identified in the Celo contracts.

Update: The cLabs team made some fixes and comments based on our recommendations. We address below the fixes introduced in individual pull requests. Our analysis of the mitigations disregards any other changes to the code base. Note that at the time of this writing, not all pull requests have been merged.

Critical

[C01] Funds of the Escrow contract can be locked by anyone

The Escrow contract allows users to send payments to other users who do not yet have a public and private key pair or an address. Given two different actors, Alice and Bob, with Alice being a current participant of the Celo protocol and Bob a person who does not possess Celo Gold, Alice could use the transfer function to send a payment to Bob to an address specified in the paymentId parameter. This paymentId is used to save in storage the state of the whole process in the escrowedPayments mapping.
To withdraw the amount of money Alice has sent him, Bob uses the withdraw function, which is in charge of recovering from storage the state of the payment into the payment variable and, after validating that the transaction is correct, sending the funds to Bob’s address.

Nevertheless, as there is not any validation on whether the paymentId has been used before to generate another transfer, it is possible for anyone to call a second time the transfer function with the same paymentId as the initial one and overwrite the storage of that transaction before anyone uses the withdraw function to claim the funds. This will result in the lock of the user’s funds in the Escrow contract. When the victim tries to use the withdraw function, the retrieved state of paymentId will only correspond to the second transfer.

Consider modifying the logic of the contract to be able to handle multiple escrows for a specific paymentId. Alternatively, consider preventing the creation of an escrow that will overwrite an existing escrow.

Update: Fixed in pull request #2797. The transfer now reverts if the paymentId has been used before. A regression unit test was added in pull request #3351.

[C02] Any owner of the Multisig can execute unconfirmed transactions

The Celo project has a custom implementation of a MultiSig wallet. This contract gives allowance to a group of accounts, known as owners, to submit, confirm, and execute transactions to other smart contracts, or to itself. It also defines a maximum number of 50 owners.

For a transaction to be executed, there must be a minimum number of owners that have to confirm it after it is submitted. This minimum is defined by the required variable.

The contract provides a group of functions for adding new owners, removing and replacing already existing owners. When a new owner is added, it will be pushed to the owners array, and will be mapped in isOwner mapping to true. Some restrictions defined in these functions are:

  • An already existing owner cannot be added twice
  • An owner cannot be removed if they are not part of the multisig
  • An existing owner cannot be replaced by another existing owner
  • A non-existent owner cannot be replaced by another owner

The isOwner and owners variables can get out of sync when the last added owner is replaced. In this case it is possible to set the newOwner as true and the owner as false in the isOwner mapping, but not replace it in the owners array.

When this happens, the whole logic of the contract gets broken as these two variables are used throughout the contract interchangeably to check whether a specific account possess ownership over the wallet, and whether they have confirmed transactions to be executed. The Multisig wallet will:

  • Allow any owner to execute unconfirmed transactions
  • Not be able to reach the requirement value in future transactions, and therefore not be able to execute transactions at all
  • Not be able to fully remove an owner from the Multisig
  • Change the number of required confirmations without enough confirmers
  • Show an unconfirmed transaction as confirmed
  • Show an unreal number of confirmations per transaction
  • Show an incorrect list of owners
  • Not be able to reach the MAX_OWNER_COUNT limit

Step-by-step proof-of-concept exploits for some of the scenarios listed above can be found in this private gist.

Consider reimplementing the replaceOwner function so it can handle the replacement of the last added owner, by removing the -1 in the break condition of the for loop.

Update: Fixed in pull request #2808. The replaceOwner function now handles the replacement of the last added owner. A regression unit test was added in pull request #3351. cLabs’ statement for this issue:

The team working on Celo chose to use the Gnosis multisig implementation, rather than build a new one, in order to use proven, audited code. But as mentioned on N18, there’s no actual process to verify this on a regular basis, which will be addressed as stated in the N18 response.

High

[H01] The approver role can prevent their own removal

The setApprover function allows Celo governance to set the approver for proposals. However, it has an onlyOwner modifier on it, meaning that the only way to access it is via a proposal. Such a proposal cannot be voted on if the proposal has not been approved by the approver. Thus, if the approver wishes, they can block all proposals with calls to setApprover, preventing themselves from being removed from power.

Consider implementing a special type of proposal for changing the approver. This type of proposal should not require approver approval, and should only be able to externally call setApprover.

Update: Not Fixed. cLabs’ statement for this issue:

The approver role is not an individual but rather a separate contract (e.g a multisig) that represents a set of individuals authorized to perform a “committer” like role. As per most open source projects, the approvers themselves self-govern and handle the addition and removal of new members. Since the approver role itself is a pointer to this contract, changing the approver role itself is more like removing all approvers, and as such the design intentionally requires approval on any proposal that does this.

[H02] Commissions can deceive members of a validator group

The updateCommission function of the Validators contract can be called at any time by the account under which a group is registered. This will update the fraction of rewards paid to the group owner versus the members of the group. Since this can be called at any time, it is possible for the group owner to call this immediately before rewards are distributed, setting the fraction paid to themselves to 1.0, which pays the group owner the total rewards and pays validators in the group nothing. The validators and voters supporting this group may not realize a change like this has been made until it is too late.

Consider implementing a time-locking mechanism or a commission history, such that changes to the commission only take effect after the epoch in which they are initiated.

Update: Fixed in pull request #2913. The cLabs team has implemented functions to queue commission updates and then apply them after a specified number of blocks.

[H03] Elected validators of a group can be chosen by the validator group registrant

The electValidatorSigners function of the Election contract is responsible for the election of validators within validator groups. After calculating the amount of seats each validator group receives, it uses the getTopGroupValidators function defined in the Validators contract to get the elected members of the group. In this specific case, the headN function is internally used by getTopGroupValidators to get the first numMembersElected[i] elements of the electionGroups[i] validator group.

As a result, this return value will be saved in the electedGroupValidators variable, thus assigning the validators to the electedValidators mapping and establishing them as validators of a certain epoch.

The problem arises from the assumption that the members list is ordered by any value. This list can be reordered by the registrant of the group using the reorderMember function. This function allows the registrant of a validator group, which is any address that makes use of the registerValidatorGroup function, to benefit whichever validator they want by ordering them in the head of the member list.

Two general issues result from this:

  • Users naively joining validator groups assuming they are secure or fair will be subject to the registrant decision on whether they will be able to be picked as validators, which could lower the user’s earnings over time.
  • A registrant or multiple colluding registrants can instantaneously reorder their group’s validators to give those validators monetary rewards, and, with enough colluding validators, control over the protocol’s consensus mechanism. This action would be publicly visible after the fact, so groups that have a reputation for doing this would likely lose voter support over time.

Consider modifying the members list’s type to match the AddressSortedLinkedList type in which the validators in a group are assigned by a specific value, which could be for example, the amount of locked gold of the validators. Moreover, if the ability to reorder validators in a group is important for specific use cases, such as organizations that want to change the specific machines or keys under which they validate in the case of hardware or connectivity failure, consider modifying the codebase so that this freely reorderable functionality is specifically opt-in, and not the default configuration.

Update: Not Fixed. cLabs’ statement for this issue:

A single entity that controls 2/3rds of validator groups can indeed take over the network. Such an attack would require 2/3rds of the votes to achieve, and the primary defense against this attack is the high vote threshold required. One needs to assume that a compromised validator group compromises all of the members of that validator group. The remedy for a malicious validator group is voting, and the add/remove/reorder operations of a validator group are not intended to be resilient against a malicious registrant.

The critical question here is, how does the mechanism of validator groups affect the overall security of the system? To explore this, let us consider a spectrum where the maximum validator group size (currently set to 5) is varied between the extremes of 1 and unlimited.

At one extreme, the maximum validator group size were to be lowered to 1, the system would become equivalent to the direct election of validators. One would expect a single person to own both the validator group and the validator. Validator groups would merely be a layer of indirection to allow swapping out down nodes or doing system upgrades.

At the other extreme, if there were no limits on validator group size, the system would become similar to parliamentary democracy. A single political party (validator group) could get a large amount of control, which would be less decentralized. However, larger validator groups have some advantages. A larger validator group is better able to do thorough vetting and security audits of validators than individual voters. It can also respond more quickly to outages or attacks. Changing significant numbers of votes is a slow process, and validator groups add an element of agility to the network. Voters might also find it easier to research a larger validator group than an individual validator. The security of the system is fundamentally derived from voting, so making voting easier increases security.

It is open to debate what the correct limit should be. The election of 5 errs close to the direct election extreme while hopefully maintaining some of the benefits larger validator groups.

Medium

[M01] Expired attestations lock funds

The Attestation contract allows users to request an attestation that they own a specific telephone number. To do so, a user must call the request function, transferring funds from their ERC20 account to the Attestation contract.

For the flow to be completed, the user must call the selectIssuers function and, after receiving the attestation via SMS, call the complete function before the attestationExpiryBlocks amount of blocks have passed.

If the user was not able to complete this request, or if the issuers have any problem sending the attestation to the user’s SMS within the time it took the network to produce the attestationExpiryBlocks, users’ funds will be locked forever in the Attestation contract. The validateAttestationCode function, which is called by the complete function, will revert on line 514 once the attestation has expired.

Consider implementing specific functionality to let users withdraw their money in case the attestation cannot be completed.

Update: Not an issue.
The implemented behavior is correct according to the specification. Pull request #3437 adds a comment to make this clearer. cLabs’ statement for this issue:

The loss of the attestation fee in the event of an incomplete attestation is a necessary requirement for the security of the protocol. There are two negative consequences that would result from the ability for users to withdraw their funds:
– Most importantly, it would allow malicious validators to spam other validators and cause them to send text messages for which they would not be able to be compensated through the completion of the attestation at virtually no cost to the attacker. Only without the fee being recoverable can the protocol incur a cost to the attacker.
– It would reduce the cost for attackers to just request a lot of attestations, but only complete the ones that they can control validators for. For the remaining ones, they would just refund themselves with the attestation fee.

Thus M01 is not a bug, but a deliberate protocol design decision.

[M02] Functions in StableToken do not emit Transfer events

Both debitFrom and creditTo functions of the StableToken contract work in a very similar way to the mint and burn functions of an ERC20 token in the sense that when those are called by the VM, the total supply of the token will increase or decrease.

The ERC20 specification states that on the transfer function “A token contract which creates new tokens should trigger a Transfer event with the _from address set to 0x0 when tokens are created”, while the StableToken does not trigger such an event in the creditTo function.

Similarly, the ERC20 specification does not mention anything about events when burning tokens, but emitting a Transfer event with the _to address set to 0x0 has become a de-facto practice found in widely used ERC20 tokens, such as the OpenZeppelin’s ERC20 implementation.

When tokens are burned, the StableToken‘s burn and debitFrom functions are not triggering the Transfer event and thus the amount of burned tokens is not being properly logged.

Clients attempting to reconstruct the entire history of transferred tokens by parsing Transfer event logs should take this issue into account to avoid mismatches in expected token balances.

Consider emitting the Transfer event inside the burn, debitFrom, and creditTo functions after those operations are successfully registered.

Update: Fixed in pull request #2805. The burn function now emits the Transfer event. The creditTo function was renamed to creditGasFees. It now calls the _creditGas internal function which emits the Transfer event. The debitFrom function was renamed to debitGasFees. In pull request #3438 comments were added to explain why the debitGasFees and the creditGasFees functions only emit the Transfer events for the net gas payments.

[M03] Users can avoid some slashing penalties by front-running

The slash function of the LockedGold contract is called whenever any type of slashing occurs. It will decrement the account’s non-voting locked gold balance, as well as the accounts’ active and pending votes if needed. However, an attacker can take advantage of the protocol in specific circumstances, when slashed accounts have more than the locked gold requirement and the slashing penalty is greater than the locked gold requirement.

In order to avoid some slashing penalties, a user can simply front-run the call to slash with calls to the functions revokePending and revokeActive of the Election contract, and the unlock function of the LockedGold contract. These first two functions will turn pending or active votes into nonvoting account balance, while the unlock function will turn the nonvoting account balance into PendingWithdrawal objects.

Aside from locked gold requirements for validators, a user can unlock all of their other votes and account balances, turning them into PendingWithdrawals. The slash function does not attempt to decrement from these, so a user can avoid some slashing penalties by doing so. If a user sees that they are being slashed before the slasher’s transaction is mined, the user can construct a transaction that moves their locked gold to a PendingWithdrawal and send it with a higher gas cost, front-running the slashing transaction.

It should be noted that there is little incentive for a validator or validator group to have any amount of locked gold above the required amount, so if they plan to be malicious, they will likely rid their accounts of anything other than the minimum amount of locked gold ahead of time, rather than taking a chance on trying to front-run.

Consider adding functionality to slash that will also decrement from pendingWithdrawals.

Update: Not fixed. cLabs’ statement for this issue:

The current implementation limits the slashing penalty to be the min(penalty, accountTotalLockedGold(validator)) so an account will never be slashed more than an account’s locked gold balance.

The intent is for penalties to never exceed the lockedGoldRequirement for any account.

For non-validators or validators/groups slashed via governance, they may still front-run their slashing by withdrawing. This is being tracked in celo-org/celo-monorepo#2887.

[M04] Undocumented assembly blocks

The Proxy.sol contract includes a couple of assembly blocks. In particular, the fallback function uses a large block of assembly for delegating calls from the Proxy contract to the implementation contract that lives in the IMPLEMENTATION_SLOT.

As this is a low-level language that is harder to parse by readers, consider including extensive documentation regarding the rationale behind its use, clearly explaining what every single assembly instruction does. This will make it easier for users to trust the code, for reviewers to verify it, and for developers to build on top of it or update it.

While this does not pose a security risk per se, it is one of the most critical parts of the system, as it is in charge of the communication between the proxy and the implementation using the proxy’s storage for preserving it between contract upgrades.

Note that the use of assembly discards several important safety features of Solidity, which may render the code less safe and more error-prone. Hence, consider implementing thorough tests which cover all potential use cases of these functions to ensure they behave as expected.

Update: Fixed in pull request #2895. Inline comments have been added to the assembly blocks.

[M05] Missing counter addition

In the Attestations contract, the addIncompleteAttestations function creates a currentIndex variable before entering a while condition.

If the random validator has already been selected as an issuer for a specific request, then the attestation.status will be different from AttestationStatus.None and the while loop will restart, attempting to find new random validators until the transaction either runs out of gas or the currentIndex variable reaches the value of unselectedRequest.attestationsRequested.

The while loop condition will always be true if the attestationsRequested parameter of the request is bigger than the current amount of validators.

There is a specific attack vector that can trigger this behavior: An attacker uses the Escrow contract to generate a transfer for a victim with the minAttestations parameter’s value higher than the current amount of validators. With the intention of generating the minimum amount of attestations to withdraw the money, the victim will make use of the Attestations contract to generate a request with the attestationsRequested parameter equal to the number of minAttestations used by the attacker. Finally, when the victim executes the selectIssuers function with the corresponding identifier, the transaction will fail with an out of gas error.

Consider picking validators, which is now done via the seed variable, from a list of unselected validators, which can only include validators that have not already provided an attestation. Additionally, consider including a check either in transfer or selectIssuers that requires that the number of attestations does not exceed the total number of validators. Alternatively, consider modifying the currentIndex variable, or some other variable inside the if condition in such a way that the while loop can eventually finish its execution. If the latter is chosen, consider the effects of successfully finishing execution of addIncompleteAttestations, such as the deletion of the relevant request afterwards.

Update: Fixed in pull request #2794. Now there is a maximum number of attestations, there is a new check to require that the number of attestations does not exceed the number of issuers, and the list of issuers is reduced after every iteration.

[M06] Lack of input validations

Across the repository, there are multiple situations where the input is not checked before using it in a function. In particular:

  • The _setOwner function in the Proxy contract does not check that the new owner of the proxy is not the zero address. This would result in the permanent loss of control over the proxy ownership if the zero address is passed by mistake. Also note that some Ethereum clients may default to sending null parameters if none are specified.
  • The setWalletAddress function in the Accounts contract allows to change the walletAddress for the owned account. There are however are no checks on the address passed as parameter, which could in particular be the zero address.
  • The increaseAllowance, decreaseAllowance and approve functions in the StableToken and the GoldToken contracts do not check that the spender is not the zero address, and the transferFrom function in both contracts does not check that the zero address, which in Celo represents the VM, will not transfer tokens on other’s behalf.
  • The Governance contract is in charge of important parameters of the Celo blockchain. In particular, the minDeposit variable holds the value of the minimum amount of Gold to submit a proposal. Although in the initialize function the minDeposit variable is checked not to be 0, the same condition is not checked in the setMinDeposit function. If the minDeposit is set to 0 by the administrator, it could lead to an insecure blockchain state where an attacker can send a lot of proposals to the network without having to spend Celo Gold.

Consider requiring that these addresses are not the zero address, restricting integer values when appropriate, and consider adding more checks along the code for proper input validation.

Update: Fixed in pull request #2807. The cLabs team has implemented input validation for functions increaseAllowance() and approve(), which together prevent setting an allowance greater than 0 for address(0). decreaseAllowance() and transferFrom() can then no longer be used by address(0) to spend any amount. The cLabs team has also implemented the recommended fixes for Proxy.sol and Governance.sol. They have added a comment in setWalletAddress explaining what it means for a wallet address to be set to 0.

[M07] Elements with equal value within a Sorted List can be arbitrarily ordered

When using the SortedLinkedList library, the update function makes use of the remove and the insert function. After some validations , the insert function uses the isValueBetween function, which will return true if the lesserKey is 0 or if the value associated with lesserKey is less or equal than value or if the greaterKey equals 0 or the value associated with greaterKey is greater than or equal to value

This will allow any update operation to move a specific item in the list to the spot directly before or after an item if the value is the same for both.

Throughout the code there are two specific places in which this behavior can be used to manipulate the Celo protocol. Both the queue variable in the Governance contract and the eligible variable in the Election contract belong to the SortedLinkedList.List type. The first one saves the proposals in the order they will be dequeued and the second one holds the eligible validator groups ordered by number of votes, establishing the winners of an election. By using the upvote or the revokeUpvote functions in the first case, or using the vote,revokePending or revokeActive functions in the second case, a participant of the Celo protocol can order themselves at any position as a selected proposal or validator by matching the amount of votes of the selected element in the list and by using specially crafted lesser and greater parameters.

Consider reviewing if this is the intended case of the library and documenting it, explaining the risk and attack vectors that may arise from this behavior.

Update: Not Fixed. cLabs’ statement for this issue:

There are four such examples of sorted lists:
1. The list of election eligible validator groups, ordered by vote total. Here, ties are very unlikely, as votes are denominated in wei. Furthermore, ties would only have an effect if the groups that are tied spanned the boundary defined by ​maxElectableValidators​. Otherwise, the ordering of groups that are tied in the number of votes does not affect election results, even if those groups happen to elect validators.
2. The list of governance proposals, ordered by upvote total. Here, ties are very unlikely, as votes are denominated in wei. Furthermore, ties would only have an effect if the tied proposals spanned the boundary defined by concurrentProposals​. Even if this was the case, the ramifications would be minor, as the tied proposal(s) that didn’t get dequeued would become the proposal(s) with the highest number of upvotes, making it extremely likely the proposal would get dequeued next.
3. The list of oracle reports for a given token, ordered by price. Here, the unstable sorting doesn’t matter, as the same median value will be returned regardless of how ties are ordered.
4. The list of oracle reports for a given token, ordered by timestamp. Here, the unstable sorting doesn’t matter, as the same median value will be returned regardless of how ties are ordered.

[M08] The reveal process can get stuck for a proposer

The Random contract provides randomness functions for the block proposer. To prevent the randomness from being known in advance, the contract uses a commit and reveal process for the upcoming block. In this way, the contract stores the last commitment under the commitments mapping, and at the beginning of a new block, this commitment is revealed.

The revealAndCommit function is in charge of this process. It is only callable by the VM, and redirects the functionality to the internal _revealAndCommit function. There, it checks if the proposer had submitted a commitment in the past, and if that condition is true, it will check if the revealed randomness was zero or not. If the revealed randomness is zero, the transaction will revert.

The problem might occur if the function call from the VM passes a commitment derived from a zero randomness as a parameter. As the hash of bytes32(0) is not zero, if in the first revealAndCommit call the newCommitment parameter was 0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563 (the bytes32(0) hash), then in the following call, when the randomness is revealed, the requirement in L70 will revert although the stored commitment corresponds to the input randomness.

These calls are made by the node client twice: when the client checks that the randomness is correct and when the proposer reveals the randomness. In both cases, when the transaction reverts, both the process and commitNewWork functions from the blockchain client will return an error.

Update: Fixed in pull request #2795. Now a commitment of 0 randomness will revert.

[M09] Contracts storage layout can be corrupted on upgradeable contracts

The Celo protocol has an unstructured storage proxy pattern system implemented, very similar to the one proposed in the OpenZeppelin SDK.

This upgradeability system consists of a Proxy contract which users interact with directly and that is in charge of forwarding transactions to and from a second contract. This second contract contains the logic, commonly known as the implementation contract. This approach has two main considerations to be aware of:

  • Storage collisions between the proxy contract and the implementation contract: There can be collisions between the state variables defined for the proxy, such as the IMPLEMENTATION_POSITION, the OWNER_POSITION, and the state variables defined for the logic contract. This is well managed by Celo, by reserving a pseudo-random slot for both the logic contracts’ implementation and owner address. This way, they are guaranteed to not clash with state variables allocated by the compiler, since they depend on the hash of a string that does not start with a storage index.
  • Storage collisions between different implementation versions: There can be storage collisions between different versions of the same implementation contract. Some possible scenarios are:
  • When changing the variables order in the contract
  • When removing the non-latest variable defined in the contract
  • When changing the type of a variable
  • When introducing a new variable before any existing one
  • In some cases, when adding a new field to a struct in the contract

The unstructured storage proxy mechanism does not safeguard against this situation by itself, and there are no validations being done by the script that is in charge of upgrading contracts , thus there is no certainty that storage layout will remain safe after an upgrade. Violating any of these storage layout restrictions will cause the upgraded version of the contract to have its storage values mixed up, and can lead to critical errors in Celo protocol’s contracts.

Consider checking whether there were changes in the storage layout before upgrading a contract by saving the storage layout of the implementation contract’s previous version and comparing it with the storage layout of the new one.

In addition to this, consider using the OpenZeppelin SDK, which already covers these scenarios among others, such as multilevel inheritance using a more complete version of the Initializable contract and the transparent proxy pattern.

Update: Not Fixed. cLabs’ statement for this issue:

As stated in the report, there’s a risk of performing not compatible upgrades that use a different storage layout. The actual upgrade script is a development one; on mainnet upgrade will be performed by governance proposal; and when this happens we’ll make sure there are programmatic tools for verifying contract upgrades compatibility. The TransparentProxy pattern is an interesting idea to implement. This can be done before or after mainnet with a governance proposal. To be able to use this, one would need to create a new contract (ContractManager) whose sole responsibility is to perform upgrades; since the Governance contract is the performing upgrade, and also makes other calls to the underlying implementation.

[M10] Not using OpenZeppelin contracts

OpenZeppelin maintains a library of standard, audited, community-reviewed, and battle-tested smart contracts.
Instead of always importing these contracts, the Celo project reimplements them in some cases, while in other cases it just copies portions of them.

This increases the amount of code that the cLabs team will have to maintain and misses all the improvements and bug fixes that the OpenZeppelin team is constantly implementing with the help of the community.
In particular, the following contracts and libraries are being reimplemented or copied:

Consider importing the OpenZeppelin contracts instead of reimplementing or copying them, and consider updating the library to its latest version.

Update: Partially fixed in pull request #2877. Now Celo is using version 2.5.0 of OpenZeppelin instead of copying the contracts to their repository, except for the ReentrancyGuard contract. This ReentrancyGuard version was copied into the Celo repository because the most recent version of ReentrancyGuard in OpenZeppelin requires its constructor to be called to work correctly. This does not happen when it is used through a proxy. However, note that the ReentrancyGuard version now in use by Celo worked with proxies just by coincidence, because the uint256 _guardCounter variable would work even if it is not initialized. The project openzeppelin-contracts-ethereum-package is the one designed to work with proxies. Consider importing this package instead, which includes a ReentrancyGuard version with an initializer function.

[M11] Users are not able to verify releases

Celo does not publish the checksums of its releases. Moreover, while some releases are signed (for example, alfajores-v0.3), some of them are left unsigned (for example, wallet-v1.4.2).

Publishing the checksum allows users to verify the integrity of the downloaded files, which may have been modified by attackers. On the other hand, signing the releases allows users to verify that they come from a trusted source. It is an important security measure in case the GitHub account of one of the maintainers gets compromised.

Even though we understand that the smart contracts in the scope of the engagement will not be executed in a user’s context, we decided to include this issue in the report as the celo-monorepo does not only contain the smart contracts, but also other applications, such as mobile clients.

Consider signing all the releases and publishing their corresponding SHA256 checksum. The article Managing commit signature verification explains more details about signatures in GitHub.

Update: Not fixed. cLabs’ statement for this issue:

All production releases of the project will include release notes detailed on GitHub. For each downloadable binary (currently only Docker images), a checksum and a signature generated via a securely custodied offline GPG key will be provided.

[M12] There is no security policy

The celo-monorepo project has no security policy.

The security policy includes instructions for the responsible disclosure of any security vulnerabilities found in the project.

Consider adding a policy to the repository, and adding links to it in the README.md file and in the Contributing guide.

Consider adding a method for secure and encrypted communication with the team, like an email address with its GPG key.

Update: Fixed in pull request #2906. The security policy document has been added.

[M13] Missing test coverage report

There is no automated test coverage report for the protocol package. Without this report it is impossible to know whether there are parts of the code never executed by the automated tests; so for every change, a full manual test suite has to be executed to make sure that nothing is broken or misbehaving.

Consider adding the test coverage report and making it reach at least 95% of the source code.

Update: Not Fixed. cLabs’ statement for this issue:

The celo-monorepo configuration includes a coverage that runs once a day. It’s not included on every PR/commit since for the moment it takes ~3 hours to run all tests with coverage enabled. This will be revisited before mainnet so as to improve the coverage tooling.

[M14] The relations between loops and their boundaries are not clear

There are several occurrences in the codebase where for loops are used to iterate an entire array.

This is a widely known problem in the Ethereum ecosystem, as it is common that loops in Smart Contracts result in an out of gas error when the number of iterations that the loop needs to execute is too big.

Some examples of this behavior are:

Also, the family of LinkedLists libraries makes heavy use of for loops for iterating the lists, as seen in the popN, headN, getElements and numElementsGreaterThan functions. This could also generate problems in other parts of the protocol that are using these functions under the hood.

Some examples of this behavior are:

Some examples of the implications arising from this issue are:

Consider setting a hard cap on the number of iterations that the smart contracts should execute. Likewise, when the iteration limit is bounded by invariant properties in the code, consider enforcing that those properties are invariant. Finally, consider thoroughly reviewing the usage of for loops in the protocol and closely analyzing the effects of failures that may arise from the lack of limits in these loops.

Update: Not Fixed. Issue #3420 was created to keep track of this. cLabs’ statement for this issue:

All of the listed “unbounded” loops are either bounded by input arguments, are external view calls intended to be called via RPC, or are bounded governable parameters.

In the first case, the caller is responsible for setting the arguments appropriately. For example, the “unbounded” loop in batchGetMetadataURL is bounded by the length of the input array. Clients that want to query metadata URLs (e.g. the Celo Wallet mobile application) can reduce the size of the batches being fetched if larger batches consume too much gas.

Examples:
* In the Accounts contract: L353
* In the Attestations contract: L133, L358, L360, L543
* In the Governance contract: L1026 (transaction array length set by proposer)
* In the Proposals library: L82, L121 ( (transaction array length set by proposer)
* In the Reserve contract: L116
* In the SortedOracles contract: L112
* In the StableToken contract: L124
* In the Validators contract L380, L912, L929

In the second case, the function exposes smart contract storage state via a view function. All of these functions were written to be called via RPC by a trusted client, in which gas provided can be made to exceed the block gas limit.

Examples:
* In the Accounts contract: L353
* In the Attestations contract: L420, L430
* In the Election contract: L805
* In the Governance contract: L879
* In the SortedOracles contract: L201, L232
* In the Validators contract: L341, L380, L804, L910, L912, L974, L929

In the third case, the length of the loop is bounded by a storage variable set via on-chain governance. Should the gas consumed by the loop grow too large, it can be lowered via on-chain governance.

Examples:
* In the DowntimeSlasher contract: L82
* In the Election contract: L224, L397, L848, L854, L878, L902, L944
* In the Governance contract: L917
* In the Reserve contract: L116, L275, L294
* In the Validators contract: L341, L380 (where needed, limited by the max group size), L804, L910 (where needed, limited by the max group size), L912 (where needed, limited by the max group size), L929 (where needed, limited by electableValidators.max)
* In the Election contract: L822, L826
* In the Governance contract: L965

Low

[L01] Abi.encodePacked function receives dynamic-sized parameters

The checkProofOfPossession function of the UsingPrecompiles contract uses both dynamic-sized variables blsKey and blsPop as the parameters of the encodePacked function.

As stated in the Solidity documentation webpage it is easy to craft collisions if more than one parameter of the encodePacked function is dynamically-sized.

Although no attack vector has been identified from this behavior, consider using abi.encode instead.

Update: Not fixed. cLabs’ statement for this issue:

Abi.encodePacked​ is used here to concatenate bytes before they are passed on to a precompile. The length of these arrays are checked in Validator._updateBlsPublicKey​. Perhaps it would be helpful to check that here or change the parameter types. On the other hand, it’s possible to call the precompile using any arguments without using this method.

[L02] Inconsistent quorum calculation

The Governance and DoubleSigningSlasher contracts are inconsistently calculating the number of validators required to constitute quorum.

In the byzantineQuorumValidatorsInCurrentSet function of the Governance contract, the quorum is calculated by doubling the total amount of validators, then dividing the obtained value by 3 and finally adding 1. On the other hand, in the minQuorumSize function of the DoubleSigningSlasher contract, the quorum is calculated by doubling the value of the total amount of validators, then adding 2 and dividing that value by 3. The return values of these functions differ when numberValidatorsInSet (for the minQuorumSize function) and numberValidatorsInCurrentSet (for the byzantineQuorumValidatorsInCurrentSet function) are equal to any integer multiple of 3. For example, with 21 validators, byzantineQuorumValidatorsInCurrentSet will return 15, while minQuorumSize will return 14.

Also worthy of note is that byzantineQuorumValidatorsInCurrentSet uses SafeMath functions while minQuorumSize does not.

Consider being consistent in the calculation of the quorum and thoroughly comment in the source code an explanation regarding how the quorum is being calculated. Consider also using SafeMath operations in minQuorumSize for better code consistency and security.

Update: Fixed in pull request #2796. The cLabs team has implemented a single quorum calculation.

[L03] Missing revert messages in require statements

There are several require statements in the project without revert messages. This behavior results in an increased difficulty of debugging the system.

The list of affected require statements is:

Consider including specific and informative revert messages in all require statements.

Update: Fixed in pull requests #2809 and #3272. The cLabs team has addressed all identified cases in Freezable.sol, Initializable.sol, UsingPrecompiles.sol, and FixidityLib.sol. FractionUtil.sol has been deleted from the repository.

[L04] Not using the registry EIP

The Ethereum Improvement Proposal (EIP) 1820 defines a standard registry.

Instead of using it, the cLabs team implemented its own simplified version of the Registry.

Deviating from the community accepted standards limits the opportunities to interoperate with other systems and to use shared implementations that are collectively maintained and already tested in mainnet by other projects.

Consider using the OpenZeppelin implementation of the EIP 1820 registry.

If there are important reasons to not use the EIP 1820 registry, consider documenting them.

Update: Not fixed. cLabs’ statement for this issue:

EIP 1820 serves a different purpose than the Celo Registry.
The Celo Registry is a central “address book” controlled by Governance that 1-1 maps component names to their implementations.
EIP 1820 allows the owner of any address to declare that that address implements a certain interface. There is only a “check if address A implements interface X” functionality.
There is no “lookup contract that is the implementation of X” functionality, which is what the Celo Registry implements.

[L05] Incumbency Bias possible in validator elections

The Celo protocol determines which validators are elected each epoch via a voting scheme. Validator seats are given to the top-voted groups every election. Rewards are distributed later on to every voter who voted for a winning validator group. Rewards are notably not given to voters for losing validator groups. The concept of incumbency bias refers to users choosing to only vote for the top groups, and never for the lower-ranked groups, in order to ensure they receive rewards. This is exacerbated by the existence of transaction fees, making voting for a losing group a costly endeavor. This of course could create a centralization concern for the protocol, by causing the same groups to win validator elections every time.

This vulnerability is listed as low severity because the cLabs team is aware of it and it is a necessary design feature to enable rewards based on validator performance.

This issue is difficult to solve with a single simple fix, as solutions could create even more issues. Although we cannot make a firm recommendation as a full analysis of incentives is not within the scope of this audit, consider the potential solutions of implementing losing-group base rewards, or a vote change pledge contract as discussed in OpenZeppelin/Celo communications. Note that while incumbency bias currently does tend slightly towards the centralization of the protocol, it does not necessarily break the protocol or go against its intended functionality.

Update: Not Fixed. cLabs’ statement for this issue:

This is a known issue, which has two potential mitigants. First, a vote change pledge contract can be built that allows users to commit their votes to a group provided that group meets a minimum threshold of commitments. Second, in the early stages of the network larger groups of gold holders (of more than 1% of staked gold) can play a catalyst role by using their voting power to lift promising new validators groups past the initial threshold to help them establish a track record to attract other voters.

[L06] Lack of validation may produce unexpected state in the distribution of epoch rewards

The _distributeEpochPaymentsFromSigner function of the Validators contract is used to distribute the rewards for the last epoch to a specific signer.

Any validator will be assigned to the group on address(0) when it is registered or when it is deregistered from its current validator group.

However, when the epoch rewards are distributed the case where the group variable is assigned to address(0) is not handled.

Consider strictly checking with a require or assert statement if a validator has been part of a proper validator group during the epoch reward distribution.

Update: Fixed in pull request #2931. The cLabs team has added a require which checks that the validator group is not address(0)

[L07] The minimum gas price calculation differs from documentation

The documentation defines the gas price minimum as gas_price_minimum' = gas_price_minimum * (1 + ((total_gas_used / block_gas_limit) − target_density) * adjustment_speed).

However, in the code the gas price minimum is defined as oldGasPriceMinimum * (1 + (adjustmentSpeed * (blockDensity - targetDensity))) + 1.

It is not clear what the last unit added is nor why it is being used.

Consider keeping your documentation updated to your current codebase and clearly stating how the gas price minimum is defined. For instance, consider adding the +1 that exists in the code to the documentation version. Having accurate documentation will improve developer experience and project auditability.

Update: Fixed in pull request #2889. The documentation has been updated.

[L08] Unused events

Line 120 of Validators.sol declares a ValidatorEpochPaymentSet event.

As it is never emitted, consider removing the declaration or emitting the event appropriately.

Update: Fixed in pull request #2811. The cLabs team has removed the unused event.

[L09] Lack of indexed parameters in events

Throughout the codebase, there are events defined without any indexed parameters. Some examples are:

  • Lines 44-52 in the Reserve contract.
  • Lines 121-123 in the Validators contract.
  • Line 43 in the SortedOracles contract.
  • Line 104-106 in the Election contract.
  • Line 20 in the BlockchainParameters contract.

Consider indexing event parameters when appropriate, to avoid hindering the task of off-chain services searching and filtering for specific events.

Update: Fixed in pull requests #2907 and #3125. Now the events have indexed parameters, except for the ones that just emit integer values.

[L10] Inconsistent validation of variables

The setter functions for important variables of the system are rigorously validated such as in the Validators contract , and not validated at all in some others such as in the SortedOracles contract.

Consider defining a consistent strategy for validation across the whole protocol, rather than validating only some important variables within the smart contract layer.

Update: Fixed in pull request #2813. Validations are now consistent. They have been moved to internal helpers and to setter functions.

[L11] Voting cap can be manipulated

The canReceiveVotes function of the Election contract determines whether more votes can be cast for a specific group or not. The result will be true, meaning the group can receive votes, if the proportion of votes they will have after the votes are cast is less than or equal to (numGroupMembers + 1) / min(maxElectableValidators, numRegisteredValidators) * getTotalLockedGold.

Since this calculation is based on the total locked Celo Gold, it is plausible that an attack could increase the locked gold tremendously, thus also significantly impacting the maximum number of votes that a group can have. One group increasing the voting cap by locking gold, then casting as many votes as possible for their group, could use this to win an election immediately before it ends. When there are many groups at the same voting cap, increasing the cap dramatically and then voting for your group could give you an advantage, with other groups having little time to do anything about it.

This is a vulnerability of low severity because the cost to perform it (amount of locked Celo Gold) has to be high. Additionally, even if the voting cap is changed, for other groups to get back to the voting cap (if they were at it before it was changed) it would only take a small fraction of the deposited attack amount. Finally, the voting cap is a mechanism intended to increase fairness in the election system, and in that regard it works well, so recommending not using it could be worse for the protocol than using it.

Consider making it clear to voters how the voting cap is determined, so that groups are not surprised by voting caps changing, especially near the end of elections.

Update: Not Fixed. cLabs’ statement for this issue:

This is possible, though there is little benefit for increasing a validator group’s votes past that vote cap because a group’s number of validator slots is also capped. Those votes past the cap would be wasted votes in terms of gaining more control of the system. The cost is also high because locking gold would distribute cap increases uniformly to all validator groups.So the impacts are low and the costs are high here. Making vote caps clearer will be a better experience for voters. One way to fix this would be to base vote caps on the total locked gold at the end of the prior election, rather than the current time. This will make last minute changes in the vote cap impossible and provide a better voting experience.

[L12] increaseSupply in GoldToken does not emit Transfer event

When the total supply of Celo Gold tokens increases, the increaseSupply function is called to update the totalSupply_ to keep it synchronized with the total amount of gold in the system. This function is not triggering the Transfer event, and thus the increase in the quantity of tokens is not being logged.

Clients attempting to reconstruct the entire history of transferred gold tokens by parsing Transfer event logs should take this issue into account to avoid mismatches in expected token balances. Additionally, emitting a Transfer event upon token creation is encouraged by the ERC20 spec.

Consider modifying the increaseSupply function so it receives the to address, and consider and emitting the Transfer event from address(0) to that address.

Update: Not Fixed. cLabs’ statement for this issue:

Because native transfers are not tracked through events, only those made through the ERC20 contracts, it should not be expected that a user could track all gold in existence by replaying event logs.

[L13] Fraction reduction is not applied in all methods

The FractionUtil library adds the functionality to use Fractions in contracts.

When operations with fractions are done, the result Fraction might have a greatest common divisor (GCD) greater than one. In this scenario, that Fraction can be reduced. This is called after finishing operations such as in the add, sub, and mul methods.

However, the div method does not reduce the Fraction once the operation ends, creating the possibility of having denominators and numerators that have a greater than one GCD.

Consider reducing the result of the div method after the operation is completed.

Update: Fixed in pull request #2890. The FractionUtil contract was removed. Now the only function that used it returns a tuple of (numerator, denominator).

[L14] Account’s name field may allow phishing scenarios

The Accounts contract allows any user to set a name for their account with the setName function. This function does not limit in any way the name an account can set, which could result in a lot of accounts using the name of famous people to deceive naive users.

Clients from the Celo network should not use this name as an unique identifier of the account or as a trusted piece of information. Moreover, this feature could be misused by attackers if there is no proper documentation on the risks this feature introduces to the network, such as phishing scenarios which are so common in the Blockchain space that there are published scientific papers on how to recognize addresses associated with phishing.

Consider analyzing and publicly documenting the risks associated with the ability to set a name for the accounts, not only for developers generating clients but also for end-users of the Celo network.

Update: Not Fixed. cLabs’ statement for this issue:

It is correct that an account’s name is fundamentally impossible to verify. Therefore, the name should be considered untrusted by users and developers. This will be clarified explicitly in the documentation and consider renaming the field to untrustedName. In anticipation of this problem, verifiable identity attributes have been added to the Metadata feature (https://docs.celo.org/celo-codebase/protocol/identity/metadata). With this, users can verify attributes such as DNS record ownership themselves. The community should actively encourage that user-facing UIs will utilize Metadata to reduce the surface area for phishing.

[L15] Failure of ecrecover function does not revert

The ecrecover function is called within the getSignerOfMessageHash function, which is in turn called by the getSignerOfAddress function. The ecrecover function returns the signer of a message, given the correct parameters v, r, and s. In the event of an invalid combination of parameters, the ecrecover function will return address(0). If misapplied, the results of getSignerOfAddress and getSignerOfMessageHash could make it appear that a valid signature of the address(0) was produced.

Currently, if a user calls the transfer function of the Escrow contract to lock some payment with paymentId set to address(0), any user can then call withdraw with paymentId set to address(0) and an invalid signature, and they will receive that escrowed payment.

If not used carefully, ecrecover could potentially give users some appearance of controlling address(0). Considering that the VM is assigned to address(0), a signature that appears to come from this address could have significant consequences. On the other hand, we should be able to safely assume that the private key for address(0) is never knowable, and therefore a valid signature from it is never producible.

Consider adding a require statement that ensures that the result of ecrecover != 0, so that in the future, any uses of it do not mistakenly give the impression that a valid signature of address(0) was produced.

Update: Fixed in pull request #2892. ecrecover now reverts if the signer address is 0.

[L16] Not using the SafeMath library

Several arithmetic operations in the codebase are not using the SafeMath library that would prevent arithmetic overflow and underflow issues.

As an example, we detailed some of the occurrences below:

While this issue does not pose a security risk, and it is very unlikely that these operations could cause undesired outcomes as no exploitable overflows or underflows were detected in the current implementation, this may not hold true in future changes to the codebase.

Consider using the SafeMath library in these and all other places where an arithmetic operation is involved.

Update: Fixed in pull request #2893. The cLabs team is now using the SafeMath library extensively.

[L17] Excessive indirection

LinkedList, SortedLinkedList, and SortedLinkedListWithMedian are part of a group of libraries for managing double linked lists throughout the project. For instance, they are being used for saving oracles’ reports in the SortedOracles contract to keep them in order and therefore, be able to calculate the rates’ median.

Every time a SortedLinkedListWithMedian variable is defined, the list structure within it has to wrap a SortedLinkedList for saving and managing integer values and thus maintain an order, and also the latter has to wrap a LinkedList for being able to access the basic structure and functionality of a double linked list.

Therefore, to access a particular element of the list from SortedLinkedListWithMedian, going through the SortedLinkedList and LinkedList is imperative, and this leads to confusing blocks of code such as in line 142 to pop an element, or line 242 to update the median.

While this does not pose a security risk per se, it introduces a lot of complexity to an important section of the code, is error prone and difficult to maintain in the long term.

Consider redesigning these libraries to remove excessive indirection, and consider documenting how these libraries work and what their purposes and responsibilities are.

[L18] Lack of checks for denominator

The FractionUtil library adds the functionality to use Fractions.

These Fractions consist of two numbers, an integer numerator and an integer denominator, where the denominator should not be zero or otherwise it could create a division by zero, which may cause a VM error: invalid opcode error during execution.

Nevertheless, the functions inside the library do not have checks for this issue and the output may not be correct in almost all functions. For instance, the reduce function would return the same Fraction when reducing multiples of (1,0), the equals function would return true when comparing (0,1) == (0,0), and the div function would return (y.denominator,0) for the pair (1,0) and (y.numerator, y.denominator).

This library is only used in the getOracleExchangeRate function from the Exchange contract. There, a new Fraction variable is created with the output from the medianRate function of the SortedOracles. In particular, the rateDenominator could be zero if the numRates(token) == 0 in the SortedOracle contract and no further checks will be done for this variable causing a division by zero in the getOracleExchangeRate function of the Exchange contract . If this Fraction is then used in any of the previously mentioned methods from the FractionUtil library, then the actual result will not be the expected value.

Consider adding checks to eliminate the possibility of haveing zero as denominator in all the places where the methods from the FractionUtil library are used, or adding those checks in the library methods themselves.

Update: Fixed in pull request #2890. The FractionUtil contract was removed. The fraction is now calculated by calling the div function from the SafeMath library, which checks that it is not 0.

[L19] Signers cannot change association once set

In the Accounts contract, it is possible to cause a mismatch between the authorizedBy and accounts[].signer.validator data structures.

If an account A calls authorizeValidatorSigner() successfully with a signature from address B, and then again with a signature from address C, address B will no longer be able to be a signer for any account. The second call to authorizeValidatorSigner() effectively removes B as a signer and replaces them with C.

Now, if account B attempts many actions in the code where a call checking validatorSignerToAccount(B) is made, execution will revert in the require on L242 of the Accounts contract, since authorizedBy[B] == A but accounts[A].signers.validator != B. There is no way for a fourth account, D, to authorize B as their signer, as authorizeValidatorSigner(B, vb, rb, sb) will fail upon reaching the call to isNotAuthorizedSigner called from line 436.

The most important thing to note here is that once a signer has been authorized, that address can never be authorized as a signer for any account again. Additionally, every account can have only 1 authorized validator signer.

Consider adding more information about signers and accounts to the documentation, including a specific mention of these properties, so that users are not confused.

Update: Fixed in pull request #2959. The cLabs team added documentation to explain that a deauthorized key cannot be reauthorized.

[L20] TODO in reward calculations can cause reverts

In line 244 of EpochRewards.sol there is a TODO comment along with a require that will always throw. The result of this is that any calls to getTargetGoldTotalSupply, or functions that call it, like getRewardsMultiplier and calculateTargetEpochPaymentAndRewards, will cause a revert after timeSinceInitialization >= SECONDS_LINEAR, which will evaluate to true roughly 15 years after platform launch.

Consider implementing the intended calculation and removing the TODO comment.

Update: Not fixed. cLabs’ statement for this issue:

The EpochRewards contract is upgradeable via on-chain governance. So long as a fix is deployed within the next 15 years, this should not be an issue.

[L21] Lack of assert statement may produce unexpected slash

In line 78 of SlasherUtil.sol, execution will return prematurely if the group of the validator being slashed has address(0). This should be impossible, and represents a user who was not a member of any group at the time in question. The comment on the same line corroborates this by stating this Should never be true.

Consider changing this line to assert(group != address(0)). This way, unexpected errors will revert, rather than making state changes to the code, and any errors in this regard will not result in slashing a potentially innocent victim.

Update: Fixed in pull request #2891. The statement was changed to an assert.

[L22] Not following the fail-loudly principle

The executeTransaction function from the MultiSig contract allows an owner to try to execute the desired transaction.

The first problem happens when the transaction does not have enough confirmations, in which the case call to executeTransaction will succeed but it will not emit any event nor change anything in storage.

The second problem comes when there are enough confirmations but there is an issue in the use of the external_call function. Because the returned value is inside an if conditional statement instead of a require or a revert statement, in case of an unsuccessful external call, the main transaction will succeed and an ExecutionFailure event will be emitted.

For instance, because the external_call function does not have a balance check, if the transaction being executed by external_call was supposed to send a value greater than the balance of the MultiSig contract, then the external call will fail, although the executeTransaction function will be completed.

A problem in the entire execution of a transaction should be as loud as possible by ending the transaction unsuccessfully, reverting it, and marking why it failed by the usage of a revert message.

To address the first problem, consider changing the if statement on line 231 to a require statement, including an error message indicating that the transaction had too few confirmations. For the second problem, consider changing the if statement on line 234 to a require statement to fail loudly in case something wrong happens with the external call, instead of emitting a failure event and continuing the execution.

Update: Fixed in pull request #2894. The executeTransaction function now reverts if the transactionId is not confirmed.

[L23] There is no stable release cadence

The last release of the celo-monorepo project was on October, 2019.
Before that, there have been from 1 to 3 releases per month, without any stable cadence.

Predictability is a very good feature for a software project.
It makes it possible to tell your users when to expect the features or fixes they are waiting for, it removes a lot of the uncertainty that can become problematic for maintainers, and over time it will allow to make estimates about the velocity of the team.

Consider setting a hard date for releases, once or twice per month, on the same days every month.

Update: Not fixed. Refer to the cLabs statement for [M11] Users are not able to verify releases.

[L24] Missing changelog

The celo-monorepo project has no changelog.

Keeping a comprehensive chronological log of the important changes of the project helps maintainers, contributors, and users to keep track of the progress of the project and to take action when breaking changes, important bug fixes, or new features are implemented.

Consider starting to record all the important changes in the project in a changelog file. Consider making a summary of the most recent changes on every release and adding a link to the relevant changelog entry for further details. As an example, see the changelog of the openzeppelin-contracts and its releases.

Update: Not fixed. Refer to the cLabs statement for [M11] Users are not able to verify releases.

[L25] There are many stale branches

There are 21 stale branches in the celo-monorepo repository.

When there are many open branches in the team repository, it is not clear which contain work in progress, which ones are experimental and which ones were discarded.

Consider creating the branches in the personal fork of the contributor instead of in the team repository, and deleting all the branches that were discarded or have already rot.

Consider splitting the work as much as possible so smaller branches can be landed into the development repository instead of remaining open for a long time. For work that requires a branch to be open for a long time and involves more than one contributor, consider documenting it on an issue or in the project board.

Update: Not Fixed. cLabs’ statement regarding this issue:

This will be cleaned up prior mainnet.

Notes

[N01] Unnecessary variable assignment

The Random contract defines and assigns the randomnessBlockRetentionWindow variable which will be used to set the amount of past randomness values to keep in the contract’s storage.

Since this variable is already set in the initialize function, the assignment in L18 is unnecessary.

Consider removing the unnecessary assignment to benefit conciseness and save gas.

Update: Fixed in pull request #2806. The cLabs team has removed the unnecessary assignment.

[N02] Not exhaustively following a consistent coding style

Deviations from the Solidity Style Guide were identified throughout the code base. Some examples are:

Taking into consideration how much value a consistent coding style adds to the project’s readability, enforcing a standard coding style is recommended.

[N03] Transparent proxy pattern

In the proxy pattern used in the Celo protocol, the Proxy contract is in charge of forwarding calls to the implementation contract. However, it will not be able to delegate calls to functions with the same function signature as one of its own functions.

Consider using the the transparent proxy pattern for increased generality.

Update: Not fixed. cLabs’ statement for this issue:

As mentioned in the response to M09, it’s an interesting idea but not of immediate implementation.
This will be analyzed as a future change.

[N04] Misleading and incomplete comments

Across the repository, multiple occurrences of comments that are not accurate or incomplete, had been found:

In the Governance contract:

In the FixidityLib library:

  • L213 does not show the expected answer for that test case.
  • L261-L264 discuss test cases for the reciprocal function. The comments test the extreme cases of inputting 0, fixed1() and the two “max” input values, but they fail to mention a test case for the input 1, which should result in the highest output value of the function.

In the Signatures library:

Consider updating the comments to more accurately describe the purpose and effect of the codebase.

Update: Fixed in pull request #2862. The comments have been updated.

[N05] Typos

There are multiple typos across the repository. Some examples are:

In the Accounts contract:

  • L424 says “of of” instead of “of”.

In the Election contract:

  • L120 says “acconut” instead of “account”.

In the SortedLinkedList library:

  • L169 says “greaerKey” instead of “greaterKey”.

In the Validators contract:

  • L322 says “goven” instead of “govern”.
  • L352 says “epoxh_score” instead of “epoch_score”.
  • L353 says “btween” instead of “between”.
  • L374 says “btween” instead of “between”.

In the UsingPrecompiles contract:

  • L188 says “correspoinding” instead of “corresponding”.

In the SortedOracles contract:

  • L249 says “alwas” instead of “always.”

Consider running codespell on pull requests.

Update: Fixed in pull request #2896. However, codespell is not being run in pull requests.

[N06] Lack of consistency

In the Accounts contract there are functions such as setName, setWalletAddress, or setAccountDataEncryptionKey that do not create a pointer to the Account struct element in the accounts mapping although other functions such as createAccount use a pointer to then assign the values.

To improve auditability and reduce the potential surface for errors, consider modifying these places to use a consistent development style in the code.

Update: Fixed in pull request #2897. Now the functions consistently assign the account to a variable.

[N07] Attestations may be vulnerable to SIM swapping attacks

The documentation for the identity part of the project explains the process to link a phone number to an account address, generating an association that the software that interacts with the Celo blockchain will consider as certified. This documentation also states that Attestations can theoretically be attacked, whether by a service provider or collusion with a validator.

However, it should be noted that SIM swapping attacks are not theoretical and that the risk of being able to associate a user with a telephone number is not minor, as it could generate theft of money and a loss of credibility in the Celo network.

Consider thoroughly analyzing the risk of SIM swapping attacks and explicitly documenting it, stating how these attacks can generate phishing scenarios or theft of funds, and how this kind of attack should be mitigated.

Update: Fixed in pull request #2956. The documentation has been corrected and detailed.

[N08] Trust assumptions on hotfixes

The Governance contract allows for specific transactions to be executed without a normal “proposal” taking place.

This functionality is labeled as “Hotfix” and is built upon a mixture of off-chain and on-chain activities. If a problem in the protocol is discovered by the cLabs team, they will determine what transactions to execute in order to fix it. Then, they will publicly disclose a hash for users to whitelist it. Notably, the full details of the transaction will not be published, which is by design. If this hash is whitelisted by 2/3rds of the current set of validators and approved by the approver role, then anyone will be able to execute it.

This gives the Celo community, with the help of the validators, a way to address bugs and vulnerabilities. However, it can also be abused to execute transactions on behalf of the Governance contract. The general public should know that there is no way to understand what is contained in each transaction given just its hash, and that there is no guarantee of which specific transactions will be executed in a Hotfix. So they will need to trust the Hotfix proposer and its intentions.

Consider informing users of the mechanics of the hotfix system, highlighting the importance of the trust relationship between community and the proposers.

Update: Fixed in pull request #2898.

[N09] Lack of explicit return case

The isConfirmed function from the MultiSig contract checks if a submitted transaction has enough confirmations to be considered confirmed.

If the transaction has at least the required number of confirmations, then the return value will be true.

The issue is that if the total number of confirmations is not enough to reach required, then the function will be returning by default the value false, although there is not an explicit return statement.

To improve readability of the code, consider adding the explicit return case when the total number of confirmations is not enough to change the status of the transaction to confirmed, instead of relying on the default value that will be returned by the function.

Update: Fixed in pull request #2863.

[N10] Some ERC20 functions are susceptible to frontrunning

The GoldToken contract and StableToken contract are ERC20 tokens and are based on the OpenZeppelin IERC20 implementation.

All the standard ERC20 functions are completely implemented in the GoldToken contract. Among them, the approve function is susceptible to front-running attacks. For example, assume that Alice approves Bob to spend 100 tokens. After the transaction is mined, Alice wants to reduce the approved value for Bob so she again calls the approve function with 50 tokens as a parameter. Bob sees the transaction in the mempool and, before this second transaction is mined, he spends the initial amount of 100 tokens. This can accomplished by him by submitting the “spend” transaction with a higher gasPrice than the second call to approve, which should get the spending transaction processed first. After both transactions are mined, Bob can now spend another 50 tokens on behalf of Alice.

Another case in which a front-running scenario happens is in the decreaseAllowance function, although its severity is less than in the approve function. In this case, an attacker could only front-run a transaction that Alice makes in decreaseAllowance to spend more than the new allowed value.

It should be noted that approve double-spending issue is a well-known problem with ERC20 tokens and that the decreaseAllowance front-running problem is not solvable. However, consider making sure users are informed of the risks of using approve and decreaseAllowance and to thoroughly document the attack vectors that these behavior create.

[N11] Unnecessary modifier in mint function

Both mint and _mint functions in the StableToken contract implement the updateInflationFactor modifier.

Given that mint only validates that the sender of the function call is the Exchange or the Validators contract before calling _mint, and given that some extra operations will be executed, consider removing the updateInflationFactor modifier from the mint function.

Update: Fixed in pull request #2864.

[N12] Incomplete docstrings

Some public and external functions are not properly documented using docstrings. Some examples are:

Consider checking that all the external and public functions are properly documented.

Update: Partially fixed in pull request #2778. A few parameters like list in AddressSortedLinkedList.getElements() or _maxAttestations in Attestations.initialize are still not documented.

[N13] Renaming suggestions

To favor explicitness and readability, several parts of the source code may benefit from better naming. Our suggestions are:

In the Accounts contract:

In the LockedGold contract:
* totalNonvoting to totalNonVoting.
* nonvoting to nonVoting.
* incrementNonvotingAccountBalance to incrementNonVotingAccountBalance.
* _incrementNonvotingAccountBalance to _incrementNonVotingAccountBalance.
* decrementNonvotingAccountBalance to decrementNonVotingAccountBalance.
* _decrementNonvotingAccountBalance to _decrementNonVotingAccountBalance.

In the Multisig contract:
* external_call to externalCall.

[N14] Broken links in comments

The comment on line 407 of the Governance contract contains this broken link, and the comment on line 414 of the StableToken contract contains this other broken link.

Consider changing these links to the correct ones, or consider removing these comments.

Update: Fixed in pull request #2900. The cLabs team has removed these comments from the code.

[N15] Redundant require Statements in vote()

Within the Governance contract, in the function vote, there are require statements that check for the same conditions. In particular, on lines 603 – 608, there are two require statements checking that value != Proposals.VoteValue.None and weight > 0.

Consider removing one of each redundant check so that it is only performed once. Consider keeping the code to one check per require to maximize the effectiveness of error messages from requires.

Update: Fixed in pull request #2614. The cLabs team has removed the redundant checks.

[N16] TODOs in code

There are “TODO” comments in the code base that should be removed and instead tracked in the project’s issues backlog. See for example:

Having well described TODO comments will make the process of tracking and solving them easier. Without that information, these comments might tend to rot and important information for the security of the system might be forgotten by the time it is released to production.

These TODO comments should at least have a brief description of the task pending to do, and a link to the corresponding issue in the project repository.

Consider updating the TODO comments, adding at least a brief description of the task pending to do, and a link to the corresponding issue in the project repository. In addition to this, for completeness, a signature and a timestamp can be added. For example:

// TODO: Move to uint128 if gas savings are significant enough.
// https://github.com/celo-org/celo-monorepo/issues/1338
// --asaj - 20200117

[N17] Different address nomenclature for precompiled contracts

The project uses precompiled contracts to perform intensive calculations outside the VM native language, Solidity.

The contracts that make use of these precompiled contracts are the GoldToken contract and the UsingPrecompiles contract. Although they both use the Transfer precompile, they both define its address in a different way.

Consider using the same value to make usage of these precompiled contracts within the project consistent, and reduce the surface for error.

Update: Fixed in pull request #2901. The GoldToken contract has been updated to be consistent with the other usages of the precompiled contracts address.

[N18] Check that imports are up-to-date

The Celo protocol builds off of several contracts not written by the cLabs team. Over time, it is likely that bugs will be found in these contracts by other teams, or improved functionality will be implemented. Before deployment, it is a good idea to check any contracts written by outside groups to ensure that any updates are at least considered. Some contracts written by other groups include the FixidityLib and the Multisig contracts, as well as various OpenZeppelin contracts.

Consider keeping a list of all contracts developed by outside groups, and periodically checking them for issues or updates.

Update: Partially fixed. The cLabs team has acknowledged this and begun creating a process for checking for 3rd-party updates.

[N19] Docs incorrect regarding proposal deposits

In the Celo docs it states:

Any user may submit a “Proposal” to the Governance smart contract, along with a commitment of Locked Celo Gold. This commitment is required to avoid spam proposals.

However, to create a proposal a user must send in regular Celo Gold, analogous to Ether, rather than Locked Gold, to create a proposal.

Consider changing the docs to state that creating proposals requires a transfer of regular Celo Gold.

Update: Fixed in pull request #2902. The documentation has been corrected and expanded.

[N20] Unnecessary checks

The FixidityLib library provides fixed point arithmetic with protection against overflow operations to smart contracts.

To represent these fixed point decimals there are maximum values that a variable can take.

One of the methods, newFixedFraction, allows to convert a two-uint256-parameters fraction representation into a fixed point variable. This method checks if the numerator and denominator are less than the maxNewFixed value and checks if the denominator is now zero. After that, both parameters are converted into Fractions by calling the newFixed method and then the division is returned by calling the divide method.

The issue comes from the fact that the mentioned checks are already done in the newFixed method and in the divide method, so implementing them again in the newFixedFraction method is unnecessary and a waste of gas.

Consider using the already implemented checks in the divide and newFixed methods instead of duplicating the same checks.

Update: Fixed in pull request #2865. The unnecessary checks have been removed.

[N21] Inconsistent use of onlyVm modifier

Throughout the code, many functions are designed to be only called by the “VM”, or address(0). In Validators.sol, there exists an onlyVm modifier. However, at different spots in the code, the same functionality is implemented with an inline require statement. The following places have been identified for this issue:

For better auditability and for the sake of code re-use, consider implementing an onlyVm modifier in place of such inline require statements.

Update: Fixed in pull request #2903. The cLabs team has added a contract containing an onlyVM modifier that is now used throughout the code.

[N22] Not using EIP 1967: Standard Proxy Storage Slots

In the Proxy contract, the implementation and owner storage slots positions are being defined without following the EIP 1967.

Even though EIP 1967 is still a draft, it is being used by Etherscan for distinguishing whether a contract is a proxy or not, and to read the implementation contract directly from the proxy. In addition to this, the EIP 1967 calculates the storage slots in a way that the preimage of the hash cannot be known, reducing the chances of a possible attack.

Consider changing the implementation and owner slot positions to match the standard.

Update: Fixed in pull request #2904.

[N23] Unused code

  • Lines 313-328 of the Reserve contract declare a private mintToken function which is not being used.
  • Lines 27-33 of the FractionUtil library declare an internal exists function which is not being used.
  • Lines 61-112 of the FixidityLib library declare maximum values for the different functions of the library. Nevertheless, maxUint256, maxFixedAdd, maxFixedMul, and maxFixedDividend are not used along the code of the project.

This increases the code complexity and the size of the bytecode to be deployed.

Consider removing all the unused functions, and consider using the maximum values to restrict the input along the code.

Update: Fixed in pull request #2905. The unused code has been removed, and relevant information has been moved to the docstrings. However, note that some docstrings now refer to the names of the removed functions.
Consider replacing these stale references with the information that was returned by the corresponding functions.

Conclusion

3 critical and 3 high-severity issues were found. Some changes were proposed to encourage code re-use and consistency. A number of issues were identified which relate to incentive design, some of which may simply be inherent to the protocol. The changes proposed in these cases should be considered carefully, and may not be necessary to implement. However, we report them so users and developers are aware of potential future issues and can keep themselves and their funds safe.


After a first phase of auditing, the cLabs team asked us to review and audit the recent changes in the smart contracts for their protocol.

The audited commit is db2e561e1f13220a98743a7999818e48a5089d13. All the changes to the contracts folder since commit 7be22605e172ca536c028e408b147aab83202e4a are included in the scope for this phase.

Outside of the scope for this phase are the unchanged parts of the contracts that we audited in the first phase.

Vulnerabilities

Below, we list all vulnerabilities identified in the Celo contracts.

Critical

None.

High

[H01] removeSlasher can cause mismatch between slashingWhitelist and slashingMap

It is possible to successfully call the removeSlasher function within LockedGold.sol with a mismatch between the input parameters slasherIdentifier and index (for example, with index corresponding to the index of a different identifier than slasherIdentifier within slashingWhitelist). Upon doing so, slashingWhitelist[index] will be deleted and slashingMap‘s entry corresponding to identifier will be changed to false.

This leads to two problems. The first is that the slasherWhitelist and slashingMap will be mismatched. The second is that the slashingMap[keyBytes] will now equal false, so future calls to this function with the same slasherIdentifier (from which keyBytes is derived) will revert, and the slasher may be able to be added to the whitelist twice.

This issue’s severity is minimized by the onlyOwner modifier on removeSlasher. As long as the owner can be trusted, this issue is less likely to cause problems. Additionally, the owner can perform certain combinations of calls to removeSlasher and addSlasher to fix any mismatch problems. Still, to avoid this entirely, consider adding a require that checks that slashingWhitelist[index] == keyBytes within the removeSlasher function.

Update: Fixed in pull request #3122.

Medium

[M01] Incorrect value of MAX_UINT

In line 38 of ReleaseGold.sol, there is a constant called MAX_UINT. This constant holds the value 0xffffffffffffffffffffffffffffffff, which is 32 hex digits or 128 bits. Since default uint types in solidity have 256 bits, the MAX_UINT value should be 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff (with 64 hexidecimal f‘s).

When the initialize function is called with an initialDistributionRatio of 1000 and the totalGrant is greater than this erroneous value of MAX_INT, it will result in some gold not being able to be withdrawn. In the same way, an erroneous distribution could happen in the setMaxDistribution function.

Consider changing MAX_UINT to the correct value. Furthermore, to be extra careful, consider setting maxDistribution to totalGrant and totalBalance, instead of using the maximum possible value which is usually going to be much larger than the total withdrawable amount.

Update: Fixed in pull request #3274. It is now forcing an underflow to get the maximum possible value for a uint256. Note however that this is not clear, and it might cause problems in the future if Solidity decides to change the default behavior for underflows.

[M02] Inputs are not checked in setReserveFraction

The setReserveFraction function of the Exchange contract has no checks on its input, which means it can set reserveFraction to values corresponding to a Fraction of greater than or less than 1.0.

In the function getUpdatedGoldBucket, the updatedGoldBucket is set to reserveFraction multiplied by the unfrozen reserve gold balance.

According to the documentation, “the Celo Gold bucket must remain smaller than the total reserve gold balance”. So, it follows that the reserveFraction should never be equal to or greater than 1.0.

Consider adding a require such that reserveFraction‘s value is restricted to being strictly less than 1.0 when being set.

Update: Fixed in pull request #3275.

Low

[L01] Duplicated onlyWhenFrozen modifier

The functions transfer and transferWithComment of the GoldToken contract have the onlyWhenNotFrozen modifier. They both call the internal _transfer function, which also has the onlyWhenNotFrozen modifier. This causes the modifier to be called twice during the execution of transfers.

Consider removing the onlyWhenNotFrozen modifier from the transfer and transferWithComment function, or removing it from the _transfer internal function, so it is called only once per transfer.

Update: Fixed in pull request #3123. The modifier was removed from the transfer and transferWithComment functions.

[L02] Interfaces are missing functions present in their implementations

Some interfaces are not fully matching their implementation. For example:

These mismatches between the interface and its implementation are confusing. It can mean that there are problems with the modeling of the system or that the updates to the implemenations and interfaces got out of sync.

Consider adding the missing functions to the corresponding interface, so there is one basic implementation that directly matches the interface specification. Alternatively, if there are reasons for these functions to be only in the implementations, for each case consider to add a new contract that inherits from the basic implementation, to move the extra functions to this contract, and to add to its docstrings the reason for extending the interface.

[L03] The initialize function is inconsistently declared in interfaces

The initialize function is declared in some interfaces (like in IExchange), but not in others (like in IAttestations).

The initialize function plays a role similar to the constructor, so consider not declaring it in the interfaces. If it is important to add it for some reason, consider consistently adding it to all interfaces.

Update: Fixed in pull request #3124. The interfaces no longer declare the initialize functions.

[L04] Lack of indexed parameters in events

Throughout the codebase, there are events defined without any indexed parameters. Some examples are:

Consider indexing event parameters when appropriate, to avoid hindering the task of off-chain services searching and filtering for specific events.

Update: Partially fixed in pull request #3134. The TransferWhitelist contract now has indexed parameters on its events.

[L05] Lack of input validation in the Heap library

Neither the heapifyDown nor the siftDown functions of the Heap library are validating that both array parameters have the same length.

This will not generate a security incident with the codebase as it is, since in the Election contract , the heapifyDown function is being called with the keys and votesForNextMember arrays as parameters, which will always have the same length.

However, this could introduce bugs and vulnerabilities in the future, if the library is used in a different part of the code or if the keys and votesForNextMember array have a different length.

Consider introducing a require statement validating the parameters of any of these functions so that the code will remain secure in the future.

Update: Fixed in pull request #3121.

[L06] The relations between loops and their boundaries are not clear

There are some occurrences in the codebase where for loops are used to iterate an entire array.

This is a widely known problem in the Ethereum ecosystem, as it is common that loops in Smart Contracts result in an out of gas error when the number of iterations that the loop needs to execute is too big.

  • In the getWhitelist function of the TransferWhitelist contract there are two loops iterating over the whitelist and the registeredContracts arrays correspondingly.
  • In the getAssetAllocationWeights function of the Reserve contract, there is a for loop iterating over the assetAllocationSymbols array.

Consider setting a hard cap on the number of iterations that the smart contracts should execute. Likewise, when the iteration limit is bounded by invariant properties in the code, consider enforcing that those properties are invariant. Finally, consider thoroughly reviewing the usage of for and while loops in the protocol and closely analyzing the effects of failures that may arise from the lack of limits in these loops.

Update: Not Fixed. Issue #3420 was created to keep track of this.

[L07] Unnecessary loop

In the electNValidatorSigners function of the Election contract, the contract uses two for loops to fill the keys and the votesForNextMember arrays. The index in both loops goes from 0 to the size of the electionGroups, so having them separately wastes gas and duplicates code unnecessarily.

Consider refactoring the code to fill these arrays in a single for loop.

Update: Fixed in pull request #3126.

[L08] There are two different whitelist getters

The TransferWhitelist contract is used to whitelist addresses.
Addresses can be added directly to the whitelist, or they can be added through the registry. Then, the getWhitelist function will query the registry to get the addresses of the registered contracts, will append them to the directly whitelisted address, and thus return all the whitelisted addresses.

Note that the directly added addresses are stored in the whitelist public state variable. Because this variable is public, the compiler will create its getter function whitelist. This automatic getter will return a different list than the one returned by the getWhitelist function.

To avoid confusion, consider making the whitelist state variable private.

Update: Fixed in pull request #3127.

[L09] Whitelist setters do not emit events

The TransferWhitelist contract is used to whitelist addresses.
Addresses and contract identifiers can be added individually by calling the addAddress and the addRegisteredContract functions. Or lists of addresses and contract identifiers can be set by calling the setWhitelist and the setRegisteredContracts functions.

When the items are added individually to the whitelist, the events WhitelistedAddress and WhitelistedRegistryId are emitted. However, when the lists are set, these events are not emitted. This could cause the event log to be misleading.

Consider also emitting events when lists are added to the whitelist. Ideally, when a list is set overwriting the previously whitelisted items, a removal event should be emitted for each of the items that is no longer in the whitelist. Then, a whitelisted event should be emitted for each of the new items added. Alternatively, one single event can be emitted when a list of items is set. But this can be ambiguous because looking at the log it will not be clear if the previous whitelisted items were kept or overwritten. In this case, consider thoroughly documenting the events and the functions to make the behavior clear.

Update: Fixed in pull request #3134. setWhitelist now emits the WhitelistedAddressRemoved event for all the previous addresses. It calls the addAddress function to add all the new ones, which emits the WhitelistedAddress event.

[L10] Lack of validation consistency

The functions addRegisteredContract and setRegisteredContracts of the TransferWhitelist contract are in charge of assigning the values of registeredContracts.

While the addRegisteredContract function validates that the parameter being added to registeredContracts is not address(0), the setRegisteredContracts function does not perform any validation.

Consider validating that none of the elements passed into setRegisteredContracts are address(0).

[L11] Removed account signers cannot be reauthorized

The functions removeVoteSigner, removeValidatorSigner and removeAttestationSigner of the Accounts contract remove authorized signers from the sender account, but the signers cannot be authorized again.

When signers are authorized through any of these functions, a call is made to the authorize function and they are added into the authorizedBy mapping. When signers are removed, they are removed from the account’s signer structure, but they are left in the authorizedBy mapping. Because during authorization the contract checks that authorized addresses are not in the authorizedBy mapping, this means that removed accounts cannot be authorized again.

If the remove functions are intended to be the inverse of the authorize functions (which is what might be expected just by reading the signatures and the docstrings), consider updating the authorizedBy mapping when an account is removed. If this is by design, consider documenting on the remove functions that removing an account authorization does not mean that it will be able to be authorized again.

Update: Fixed in pull requqest #3128. The functions now have clear comments about signers not able to be reauthorized.

[L12] Comments still refer to removed functions

In the FixidityLib library, the functions maxFixedMul and maxFixedDividend have been removed. However, they are still mentioned in the docstrings in lines 13, 100, 101, 105, 166, 236, 238, 239, 240, 242, and 243.

Consider removing these comments or changing them so they reflect what still exists in the file.

Update: Fixed in pull request #3143.

[L13] Duplicated code in updatePublicKeys

The updatePublicKeys function of the Validators contract updates the ECDSA and BLS public keys of a validator account atomically by calling the private functions _updateEcdsaPublicKey and _updateBlsPublicKey.

This updatePublicKeys function is duplicating some code from the external functions updateEcdsaPublicKey and updateEcdsaPublicKey.

Consider refactoring the external and private key update functions to avoid code duplication as much as possible.

Update: Fixed in pull request #3282.

[L14] maxDistribution can be set before the factory sends the gold

In the initialize function of the ReleaseGold contract, the scheduled release is used when setting the maxDistribution value because at this moment the factory has not sent the gold.

However, when the setMaxDistribution function is called, the getTotalBalance function is used despite the fact that there is nothing in this contract ensuring that the factory has already sent the gold.

Consider adding a require statement to avoid a change in the maxDistribution before getTotalBalance can return the correct value.
If this makes the design too complicated, consider at least adding a comment explaining that the setMaxDistribution function should not be called before the factory has sent the gold.

Update: Fixed in pull request #3276. Now setMaxDistribution reverts if totalBalance is 0.

[L15] Missing revert messages in require statements

In line 30 of ReentrancyGuard.sol there is a require statement without a revert message. This behavior results in an increased difficulty of debugging the system.

Consider including specific and informative revert messages in all require statements.

Update: Fixed in pull request #3277.

[L16] Unclear overwrite of commission update queue

In the queueCommissionUpdate function of the Validators contract the nextCommission and nextCommissionBlock for a group are updated.
When the nextCommissionBlock has passed, the updateCommission function can be called to update the group commission.

It is not clear what should happen when queueCommissionUpdate is called twice before updateCommission succeeds. Currently, the nextCommission and nextCommissionBlock are just overwritten. However, by calling it a queue it gives the impression that the values of the second call will be qeued to be applied after the values of the first call.

Consider documenting this case, explaining that the pending commission values will be overwritten. Since this is a queue of one element, consider renaming the function to something like setNextCommission.

Update: Fixed in pull request #3278.

Notes

[N01] Unnecessary imports

There are some import statements that are not used and can be removed. Namely:

Consider removing these unused imports for improved code auditability.

Update: Fixed in pull request #3136.

[N02] Renaming suggestions

Good naming is one of the keys for readable code, and to make the intention of the code clear for future changes. There are some names in the code that make it confusing or hard to understand.

Consider the following suggestions:

In the TransferWhitelist contract:

Update: The suggested renames were applied in pull request #3184.

[N03] Misleading comment

In the Reserve contract, L435 implies that the ratio needs to be greater than 2 in order for the protocol to transfer tax on Celo Gold but in the code the ratio can be greater than or equal to 2.

Consider updating the comment to more accurately describe the purpose and effect of the codebase.

Update: Fixed in pull request #3185.

[N04] MockReserve.sol has mintToken function

The MockReserve contract has a mintToken function, while the contract it is based on, Reserve, does not.

Consider removing mintToken from the MockReserve contract so it works better as a test double for the production contract.

Update: Fixed in pull request #3281.

Conclusion

One high severity vulnerability was identified within the changes to the code since our previous audit. Several lower severity vulnerabilities were found relating to style, efficiency, and consistency throughout the codebase. Overall, the OpenZeppelin team was quite pleased with the quality of the code and the receptiveness of the cLabs team to our previous audit phase.