Table of Contents
- From 2022-01-17
- To 2022-01-24
- Total Issues
- 7 (7 resolved)
We reviewed all changes to production Solidity files in the following pull requests:
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
Exchangecontracts to be deployed without the corresponding stable token address, which can be provided by the contract owner once it is known
Here we present our findings.
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.
Version not incremented
PR #8993 adds new rewards delegation functionality to the
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
Notes & Additional Information
Comments should be linted
Consider linting the comments in order to increase readability of the codebase.
Update: Fixed in pull request #9285.
We identified the following instances of incorrect or incomplete NatSpec comments:
- in the
getPaymentDelegationfunction of the
- in the
initializefunction of the
@param stableTokenIdentifierstatement is in the wrong order.
- throughout the codebase, the
getVersionNumberfunctions share a common issue:
@returnstatement describes all four values returned as a single value instead of four separate values
- during this review we identified this issue in the
- 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.
The Celo team states:
The issue with
@returnstatements 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.
Update: Fixed in pull request #9269.
The codebase contains the following typographical errors:
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
StableTokenBRLProxy contracts use
solhint-disable without a following
solhint-enable. Consider matching each instance of
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:
solhint-disable-next-linewould 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.
No critical or high severity issues were found. Some changes were proposed to follow best practices and reduce potential attack surface.