Celo Contracts Audit - Release 7 - Part 1 - OpenZeppelin blog

Table of Contents

Summary

Type

DeFi

Timeline

From 2022-01-17

To 2022-01-24

Languages

Solidity

Total Issues

7 (7 resolved)

Scope

We reviewed all changes to production Solidity files in the following pull requests:

• PR 9041 up to commit e763f08
• PR 8993 up to commit 0fea046
• PR 8334 up to commit cae8504

Overview of changes

These pull requests:

• Define a new collection of contracts to represent the Brazilian REAL as a stable token
• Introduce a mechanism for validators to delegate a fraction of their payments to a beneficiary address
• Allow the Exchange contracts to be deployed without the corresponding stable token address, which can be provided by the contract owner once it is known

Findings

Here we present our findings.

Medium Severity

Invalid Beneficiary

The setPaymentDelegation function of the Accounts contract allows a validator to set the zero address as their beneficiary with a non-zero payment fraction. In this scenario, the reward distribution mechanism will attempt to mint tokens to the zero address, which will revert. Consequently, neither the validator nor their group will receive the epoch payment.

Consider preventing validators from setting a zero beneficiary with a non-zero payment fraction. Additionally, in the interest of clearly signaling user intentions, consider introducing a deletePaymentDelegation function so the setPaymentDelegation function can disallow any zero beneficiary.

Update: Fixed in pull request #9283.

Low Severity

Version not incremented

PR #8993 adds new rewards delegation functionality to the Accounts and Validators contracts, but does not increment the version functions. Consider incrementing the MINOR version of both contracts in line with the contract versioning system.

Update: Fixed in pull request #9256. The PR also increments the MAJOR version of the ExchangeBRL contract.

Notes & Additional Information

Comments should be linted

In PR #9041, both the ExchangeBRL and StableTokenBRL contracts have inconsistent indentation.

Consider linting the comments in order to increase readability of the codebase.

Update: Fixed in pull request #9285.

Inadequate NatSpec

We identified the following instances of incorrect or incomplete NatSpec comments:

• in the getPaymentDelegation function of the Accounts contract:
  • the @param account statement is missing
  • the @return statement describes both values returned as a single value instead of two separate values
  • the @return statement should note that the fraction parameter is a FixidityLib value, or has 24 decimals of precision
• in the initialize function of the Exchange contract:
  • the @param stableTokenIdentifier statement is in the wrong order.
• throughout the codebase, the getVersionNumber functions share a common issue:
  • the @return statement describes all four values returned as a single value instead of four separate values
  • during this review we identified this issue in the ExchangeBRL, StableTokenBRL, Validators, Exchange, and ExchangeEUR contracts
  • this issue is replicated across the codebase and is found in code outside the scope of this review

In order to keep the codebase well documented, consider updating the NatSpec comments.

Update: Partially fixed in pull request #9270. The remaining fixes are being tracked in issue #9242 and issue #9268.

The Celo team states:

The issue with @return statements exists throughout the contracts code base, including many contracts that were outside the scope of this audit. For consistency, and to get them all in one go, we’ll keep singular @returns for now, but have created #9268 to fix this throughout our smart contracts in the immediate future.

Recalculated constant

The grandamento test file imports the SECONDS_IN_A_WEEK variable, but recalculates its value multiple times. For improved code clarity, consider using the constant.

Update: Fixed in pull request #9269.

Typographical errors

The codebase contains the following typographical errors:

• FixidyLib should be FixidityLib
• vaidator should be validator

Consider correcting these errors to improve code readability.

Update: Fixed in pull request #9250.

Improper use of solhint-disable

solhint-disable will disable a feature of solhint until it is re-enabled with solhint-enable or until the end of the file. Using solhint-disable with an unmatched solhint-enable creates code that is prone to errors when updating it in the future.

In PR #9041, both the ExchangeBRLProxy and StableTokenBRLProxy contracts use solhint-disable without a following solhint-enable. Consider matching each instance of solhint-disable with solhint-enable or instead use solhint-disable-next-line to ensure clean, developer friendly code.

Update: Issue is scheduled for a future fix, it is tracked by issue #9245.

The Celo team states:

Agreed that solhint-disable-next-line would be better. That said, the current risk of these unmatched solhint-disableds causing problems is minimal – these contracts exist solely to create new named proxies, and are meant to have exactly the code of the original Proxy contract, so they inherit from it and add nothing new, and we don’t expect ever modifying them to add anything new.

Conclusions

No critical or high severity issues were found. Some changes were proposed to follow best practices and reduce potential attack surface.