We reviewed all changes to production Solidity files in the following pull requests:
The following contracts were in scope:
A summary of the changes in the pull requests:
setSpreadaddTokengetPendingWithdrawal function in order to allow single withdrawal lookupsThese changes, along with those from Part 1 of this audit, comprise cLab’s Release 7 for the celo-monorepo.
Here we present our findings.
Prior to pull request #9367, the addToken function in the Reserve contract checked that an oracle exists for the token being added, and the oracle returns a non-zero exchange rate. These checks ensured that every token in the _tokens array had a corresponding oracle.
With the oracle checks removed from addToken by this pull request, it is possible to enter a state where the token has been added but the oracle doesn’t exist yet. However, the getReserveRatio function in the Reserve contract assumes that every token has a corresponding oracle that returns non-zero exchange rate values. If no oracle exists for a specific token, the converted price calculation will result in a divide-by-zero error.
The following contracts also contain code which can incorrectly divide by zero if queried with an oracle that is returning zero:
getGasPriceMinimum of GasPriceMinimumgetTargetTotalEpochPaymentsInGold of EpochRewardsConsider implementing additional logic that excludes tokens without oracles from the reserve ratio calculation, as well as including checks to ensure helpful errors are thrown rather than divide-by-zero errors.
Update: Partially fixed. The getReserveRatio function was fixed in PR #9527. Both getGasPriceMinimum and getTargetTotalEpochPaymentsInGold remain unchanged. Celo’s statement for this issue:
Updating
getGasPriceMinimumis not critical, as the only reasonable behavior when there is no oracle report is to revert (could be nicer to fail with a relevant require message, but this is an edge case). UpdatinggetTargetTotalEpochPaymentsInGoldis not necessary as it only ever converts from cUSD (not other cStables), which already has an oracle rate.
Pull request #9252 changes the behavior of the setSpread function in Exchange.sol, but does not update the version reported by the getVersionNumber function.
Consider incrementing the patch number returned by getVersionNumber in order to adhere to the smart contract release process.
Update: Fixed in PR #8334.
The online Celo docs state that before fully activating a new stable token, it is required to have at least one oracle report. The documentation points to line number 223 in the Reserve contract’s addToken function as the enforcer of this requirement. Pull request #9367 removed that code from the Reserve contract, invalidating the online documentation.
Consider updating the online Celo documentation to accurately reflect the new behavior of the addToken function.
Update: Fixed in PR #312 of the celo-org/docs repository.
In pull request #9252, code was added in order to ensure newly set spread values are valid. The change updated the setSpread function in the Exchange contract.
Similar to the Exchange contract, the GrandaMento contract includes an implementation of setSpread which already has a bounds check, however the two implementations differ in terms of logic and error messages.
In favor of consistent code across the repository, consider updating the code to make both implementations match.
Update: Fixed in PR #9459.
Pull request #9369 introduced a new getPendingWithdrawal function to address the possibility that the existing getPendingWithdrawals function can run out of gas if the number of pending withdrawals is excessive. In the pull request, a new test case for getPendingWithdrawal was added in lockedgold.ts, but the corresponding test for getPendingWithdrawals was removed in the process, even though this function is still in use. Furthermore, there are not matching test cases for both functions.
To improve code coverage, consider restoring the test that was removed and providing equivalent test cases for both functions.
Update: Fixed in PR #9460.
No critical or high severity issues have been found. Recommendations and fixes have been proposed to improve code quality, minimize errors, and address uncommon but possible operating conditions which could result in error.