Originally published at: https://blog.openzeppelin.com/compound-open-oracle-audit/
Compound’s Open Oracle System is a protocol aiming to provide an on-chain price oracle based on trusted reporting sources. The oracle allows reporters to post prices of assets and calculates the median price out of all reported samples, then providing the median price as the official reference price for the asset.
Audit history and current scope
In this audit we looked into all Solidity contracts inside the Compound Open Oracle System repository. The audited commit is
e7a928334e5e454a88eec38e4ee1be5ee3b13f08. The exact files included in this scope are:
All off-chain systems related to the Open Oracle were left out of scope. Moreover, we did not audit the oracle’s integration with Compound nor any other project in the ecosystem.
We have previously audited several phases of the Compound Protocol with details below. Yet note that this is our first audit of the Open Oracle System.
- A subset of Compound’s contracts in commit
f385d71983ae5c5799faae9b2dfea43e5cf75262of Compound’s public repository. Read the report’s summary.
- A patch that introduced a time delay for critical admin functions and the ability to pause others. That patch is reflected in commit
681833a557a282fba5441b7d49edb05153bb28ecof Compound’s public repository. Read the report.
- A refactor of the core
CTokencontract with the purpose of accommodating underlying tokens that may extract a fee when transferring tokens (e.g., USDT). This refactor is presented in commit
2535734126c7c26e9bc452f27f45c5408acff71fof Compound’s private repository. The report is not available to the public yet.
- The difference between the code at commit
2535734126c7c26e9bc452f27f45c5408acff71fof Compound’s private repository and commit
bcf0bc7b00e289f9b661a0ae934626e018188040of their public repository. These changes introduced the ability to handle underlying ERC20 tokens whose implementations can be upgraded (e.g., DAI). The report is not available to the public yet.
- The difference between commit
9ea64ddd166a78b264ba8006f688880085eeed13in Compound’s public repository. The audit included changes to the
JumpRateModelcontract and the two newly added files
DAIInterestRateModel.sol. Read the report.
- An alpha version of Compound’s Governance System including the Compound Governance Token (COMP) together with the core governance contracts in initial commit
6858417c91921208c0b3ff342b11065c09665b1b, as well as a follow-up commit
f5976a8a1dcf4e14e435e5581bade8ef6b5d38ea. The report is not available to the public yet.
Compound’s Open Oracle revolves around a consortium of reporters publishing signed prices for a set of assets. This is achieved by signing messages containing information such as asset prices and timestamp. The oracle validates the submitted data and saves it to a storage mapping which holds a struct containing both the asset’s price and timestamp. The official Reference Price for each asset is essentially the median of the prices from each trusted reporter, intended to be computed by the oracle whenever a price change from these trusted reporters occurs.
The individual prices reported by each source are always available and verifiable. Prices are attached to a timestamp at the time of signing, and updates are only ever stored if they represent a later time according to their source. Anyone can post prices to the oracle, however only those coming from trusted sources are used to calculate the median price of an asset.
Following we present our findings, in order of importance.
[H01] Inconsistencies in stored data may lead to incorrect median price
DelFiPrice contract has a public
prices mapping intended to store the median price for each asset. Following the specification, an asset is referenced by its symbol (a string in uppercase letters) and the corresponding median price is stored as a
uint64 value. Anyone can post prices on chain, but only those signed by trusted sources are taken into account when calculating the median price. There are two ways to post an asset’s price on chain.
The first method is via the external
put function of the
OpenOraclePriceData contract. Given a signature and the associated message containing the price and the asset’s symbol (among other things), the function will first recover the source from the message and signature, and then update the asset’s price for this particular source. Price data for each source and symbol is kept in the contract’s storage in private
data mapping. Note that prices updated via this method do not trigger a recalculation of the official (median) price hold in the public
prices mapping of the
DelFiPrice contract. As a consequence, if at this point a user queries an asset’s price reading the
prices mapping, the returned median price will be incorrect. Furthermore, if the user queries the asset’s price using the
medianPrice function, the returned median price may be inaccurate as well, since it will not be compared against the anchor price to validate it.
The other way to submit prices is via the external
postPrices function of the
DelFiPrice, which allows to report a batch of prices (with associated signatures) of one or more assets in a single transaction. Internally,
postPrices calls the
put function. In this case, the
postPrices function also correctly recalculates the median price of each asset passed in the
symbols parameter, updating the corresponding entries in the
prices mapping if the median is within safe bounds. However, if there is a mismatch between the assets referenced in the
symbols array and the assets which prices were updated, then some median prices will not be recalculated.
According to the specification, “Whenever a post happens, the official (median) price is recalculated”. This is not strictly followed by the current implementation, leaving the door open for potential data inconsistencies in the registered prices and their medians. Taking into account that reporting accurate median prices is the fundamental purpose of the system, we understand this to be a High severity issue. Consider applying the necessary modifications to strictly follow the specification. In particular, there should be a single reliable way to post prices on chain, and a single reliable way to query the median price for an asset.
[M01] Potentially inaccurate median price during sudden, extreme, price variations
The “official” price reported by the Open Oracle consists of the median price calculated over a set of individual prices reported by trusted sources. As it is known, the actual prices of most cryptocurrency-related assets are highly volatile, prone to suffer sudden, extreme and unpredictable fluctuations in short periods of time.
Since the prices reported by the Open Oracle are based on the median of a set of individual prices, the oracle needs at least 50% of the individual prices to be accurate for the median to be accurate as well. In an event of a sudden, extreme, asset price variation, the median price reported by the oracle would only reflect the new price once 50% of the sources report on the new price. As a consequence, the Open Oracle may not reflect sudden, extreme, variations in a timely manner. This behavior becomes more problematic at times when the Ethereum network is highly congested, since transactions reporting newer prices will take longer than usual to be mined. Projects relying on the Open Oracle feed must be aware of the associated risks during times of high price volatility, and implement the necessary logic to adapt to potential delays in the prices reported by the oracle.
To better reflect price variations, consider not only calculating the median of the individual prices reported for a given asset, but also computing their standard deviation. This should favor the oracle’s transparency, and help projects querying the Open Oracle better understand the actual distribution of the prices, thus allowing them to take more informed decisions.
[M02] Lack of minimum sample size requirement for official price calculation
To obtain the official price for an asset, Compound’s Open Oracle takes all reported price data from trusted sources, calculates a median price using the
medianPrice function of the
DelFiPrice contract, and finally supplies this median price as the asset’s price.
However, this process does not take into account the sample size of the prices when calculating the median price of an asset, which may render the oracle vunerable to a price manipulation attack when the sample size is low (see [N09] Considerations on rogue sources note for a more detailed description).
Consider either implementing a minimum sample size requirement or returning the sample size together with the oracle’s official price, so as to raise awareness on the potential risks when the sample size is dangerously low.
[M03] Function put allows storing invalid signed data
put function of the
OpenOraclePriceData is used to submit signed price data. It first recovers the signing address from the given signature and message by means of the inherited
source function, and then uses the recovered address (i.e., the source) to update the price and timestamp under the corresponding source address and asset symbol.
source function uses the
ecrecover precompile to recover the signing address. If there is an error during the recovering process,
ecrecover will return the zero address (as stated in its documentation). Since the returned address is never validated in the
put function, this could lead to submitting invalid data with a successful transaction. While this does not pose a security risk, since data would be written in an entry corresponding to the zero address, the function should fail earlier to favor a more predictable behavior.
Consider ensuring the address returned by the
source function is not the zero address before continuing execution in the
[M04] Oracle might fail if anchor price is not updated fast enough
DelFiPrice contract implements a safety mechanism to disregard reports that, while coming from trusted sources, may lead to a drastic variation of the official median price reported by the oracle. The mechanism is based on an anchor price (reported by a trusted
anchor address), together with upper and lower bounds that calculated median prices are checked against. Should the ratio between a new median price and the anchor price be out-of-bounds, the official median price of the asset would not be updated. Both the address that reports the anchor price and the tolerance allowed between the anchor and the median are set during construction.
In an scenario of high price volatility, a trusted source may report a sharp variation of an asset’s price. As a consequence, the newly calculated median price may easily fall out of the acceptable range defined by the anchor price. Unless the anchor value is rapidly updated by the trusted
anchor address, the oracle will never accept the new median price, thus causing the oracle to fail.
The address in charge of reporting the anchor price plays a fundamental role in the Open Oracle system, particularly in times of steep price variations. While the way this address is managed is out of this audit’s scope, we strongly advise closely monitoring price fluctuations and trends, so as to be able to quickly update the anchor price record and ensure the oracle prices can always be correctly updated.
[L01] Commented out code
OpenOracleData contract includes commented out lines of code without giving developers enough context on why those lines have been discarded, thus apparently providing them with little to no value at all.
As the purpose of these lines is unclear and may confuse future developers and external contributors, consider removing them from the code base. If they are to provide some sort of documentation of an interface, consider extracting them to a separate document where a deeper and more thorough explanation could be included.
[L02] Lack of indexed parameters in events
None of the parameters in the events defined in the
DelFiPrice contract are indexed. Consider indexing event parameters to avoid hindering the task of off-chain services searching and filtering for specific events.
[L03] Incomplete docstrings
- Docstrings for the
postPricesfunction of the
DelFiPricecontract are missing documentation for the
symbolsparameter. Note that they should state the format in which symbols are expected to be submitted (i.e., in uppercases according to the specification).
- Docstrings in the
OpenOracleViewcontract should include a link to the “Open Oracle standard specification” mentioned.
putfunction of the
OpenOraclePriceDatacontract expect the
messageparameter to be formatted in a specific way that is not fully documented in the parameter’s docstrings. In particular, it is never mentioned that
messagemust contain the string “prices” to be valid.
[L04] Use of magic constants
Consider defining a constant state variable for every magic constant, giving it a clear and self-explanatory name. For complex values, consider also adding an inline comment explaining how they were calculated or why they were chosen. All of this will allow for added readability, easing the code’s maintenance.
Notes & Additional Information
[N01] Inconsistent coding style for function parameters
There is an inconsistent use of underscores in the names of function parameters. While some parameters include an underscore and the end of their names, others do not. To favor readability throughout the code base, consider always following a consistent coding style.
[N02] Redundant assignment of sorted price array
medianPrice function of the
DelFiPrice contract calls the private
sort function to sort the
postedPrices in-memory array, then assigning the result of the operation to a new
sortedPrices array. In Solidity, arrays are reference types, so the
postedPrices array will be sorted after calling the
sort function. Therefore, for a more gas-efficient execution, consider removing both the return statement from the
sort function and the unnecessary assignment to the
[N03] Undocumented system design for untrusted price sources
The Open Oracle is designed to allow anyone to submit asset prices by calling the
postPrices function of the
DelFiPrice contract or the
put function of the
OpenOraclePriceData contract. However, only prices reported from trusted sources, defined in the
sources array, are taken into account to calculate the official median price of an asset. This system design renders prices reported from untrusted reporters pointless, though at the same time allows them to submit prices. While this seems to be inline with the system’s intended behavior, a clearer explanation addressing this (either in code or external documentation) might be beneficial for users, developers and auditors alike.
[N04] Lack of explicit visibility in state variables
lowerBoundAnchorRatio of the
DelFiPrice contract are implicitly using the default visibility. To favor readability, consider explicitly declaring the visibility of all state variables.
datamapping of the
OpenOraclePriceDatacontract should be renamed with a more self-explanatory name that states what kind of data the mapping stores. Similarly, the
putfunctions can also benefit from more self-explanatory naming.
dataaddress of the
OpenOracleViewcontract should be renamed to clearly denote it refers to a contract address.
sourcefunction of the
OpenOracleDatacontract should be renamed to
PriceUpdatedevent of the
DelFiPricecontract should be renamed to
pricesmapping of the
DelFiPricecontract should be renamed to
[N06] Declare uint as uint256
To favor explicitness, all instances of
uint should be declared as
[N07] Unnecessary casting operations
In line 54 of
data state variable is first casted to
address and then to the
OpenOraclePriceData type. However,
data is already stored as an
OpenOraclePriceData type. Therefore, consider removing the unnecessary casting operations.
[N08] Unused named return variable
medianPrice function of the
DelFiPrice contract declares a named return variable called
median. Given that it is not used, consider removing this variable.
[N09] Considerations on rogue sources
According to its specification, Compound’s Open Oracle relies on a consortium of trusted sources that make asset prices available on known https endpoints. Anyone can query these public web endpoints, obtaining the signed data that is later expected to be posted on Ethereum smart contracts. Anyone can post data on chain, but only data signed by the trusted sources will be considered when calculating the median price for an asset.
Each source will have an associated Ethereum address under which their prices will be registered on chain. The number of sources is not enforced in any way at the smart contract level. More interestingly, the array holding all trusted sources’ addresses is set at deployment and cannot be later modified.
The oracle’s “official” price for an asset is the median of a set of prices reported by the trusted sources. Therefore, it only takes 50% of the sources to go rogue in order to manipulate the oracle’s price at will. The cartelized sources will have total control of the oracle’s official price, even if the remaining 50% of the sources keep reporting the real prices of an asset. It is unclear how many sources the Open Oracle will trust, but the lower the number of sources, the easier it would be to conduct this attack. While a conglomerate of malicious sources may sound unlikely at first, one should take into account that the incentives for a set of sources to take control over the oracle increase with the number of projects fully relying on the oracle’s prices. If initially trusted sources happen to go rogue and manipulate prices before the Compound team notices, all on-chain finance systems trusting the Open Oracle may suffer severe financial losses.
The Compound team is aware of the potential risk of rogue sources, and according to the specification “The consortium operates on a trust-but-verify principle. If reporters use timestamps too far in the future or report prices which deviate too far from the norm, they will likely be voted out of the Compound Reference Price view”. At the time of the audit there is no governance mechanism in place to “vote out” a trusted source. As a consequence, the only available way to replace sources is to re-deploy the
DelFiPrice contract with a new set of sources.
So as to rapidly detect and mitigate any malicious behavior, we strongly advise closely monitoring the prices and timestamps posted on chain, tracking the
Write event triggered after every price update. Furthermore, consider outlining a response and recovery plan so as to act quickly in case an attempt to compromise the Open Oracle is detected.
No critical and one high severity issue were found. Some changes were proposed to follow best practices and reduce potential attack surface.