UMA is a platform that allows users to enter trust-minimized financial contracts on the Ethereum blockchain. We previously audited the decentralized oracle, a particular financial contract template, some ad hoc pull requests, the Perpetual Multiparty template, various incremental pull requests over a longer engagement , the insured bridge, and a new governance proposal.
The UMA Oval contracts provide protocols a way to capitalize on OEV (Oracle Extractable Value) produced by Chainlink price updates. Serving as intermediaries between protocols and Chainlink, Oval contracts allow the release of price updates to occur in MEV-Share auctions with the protocol as the benefactor. The oval adapters are designed to adhere to the existing interfaces used by protocols, ensuring a smooth and seamless integration.
We audited the UMAprotocol/oev-contracts repository at commit 0f2c186. Some links in the issues descriptions might be inaccessible as the originally audited repository was private. We confirm that all the necessary fixes are included in the public UMAprotocol/oval-contracts repository at commit 205823f.
In scope were the following contracts:
src
├── DiamondRootOval.sol
├── Oval.sol
├── adapters
| ├── destination-adapters
| │ ├── BaseDestinationAdapter.sol
| │ ├── ChainlinkDestinationAdapter.sol
| │ └── UniswapAnchoredViewDestinationAdapter.sol
| ├── source-adapters
| │ ├── ChainlinkSourceAdapter.sol
| │ ├── SnapshotSource.sol
| │ └── UniswapAnchoredViewSourceAdapter.sol
| └── lib
| └── DecimalLib.sol
└── controllers
├── BaseController.sol
└── ImmutableController.sol
The economic value generated by blockchain oracles, for example, price updates leading to money market liquidations, is currently paid out to validators who participate in MEV. The potential for innovation lies in redefining MEV extraction within the MEV supply chain, changing the benefactor from validators to decentralized applications (dApps) responsible for generating Oracle Extractable Value (OEV).
The Oval system is designed to use MEV-Share because it can leverage the already existing Flashbots infrastructure and trust, thereby limiting the amount of extra components that need to be built and maintained. This is important as any kind of high-speed order flow auction system will likely require a centralized actor to facilitate the connection between actors.
The Oval system works by submitting a transaction that pulls the latest price from Chainlink into the Oval contract. In order to backrun the price update, liquidators submit their bundle to an Oval node. Then, the Oval node places the price update transaction in front of the bundle directly within the Flashbots auction. The Oval contracts control which value is released to the downstream protocol to protect from other front-runners stealing this MEV and to keep it within the MEV-Share redistribution flow.
The system consists of four main smart contract types. These are as follows:
SnapshotSource adapter is a contract to use in conjunction with a source adapter that needs to snapshot historic data.Each time a Chainlink price update leads to a meaningful change in a downstream integration (e.g., price update that creates a liquidation), a transaction can be sent to the Oval system to pull the current price from the oracle via the source adapter and make it available to the integration via the destination adapter. This transaction is placed in front of the searcher's bundles by the Oval node and submitted to the MEV-Share system to make sure that the extracted OEV is appropriately distributed among the searcher, the builder and the integrated protocol.
The current Chainlink oracle adapters in the codebase are developed as prototype adapters. These adapters can be customized separately to meet each protocol's standards. For every new integration with the Oval system, the system's components are deployed according to the specific configuration chosen for that integration.
The action of pulling in the latest prices from the source oracle adapters into the Oval contract can only be executed by a set of permissioned addresses. This requirement is an artifact of using MEV-Share as the back-running auction mechanism. When an MEV-Share transaction is submitted, it is up to the transaction sender (the "user" who is back-run) to specify two main things: the kickback address and the kickback percentage. Oval is designed to kick back the rewards to the integrating protocol. The current MEV-Share implementation provides no way to verify the kickback parameters on-chain, so there is no ability to ensure that the kickback parameters are set correctly. To simplify this problem, a set of trusted actors is introduced who are allowed to unlock the latest value for the Oval contract. It is up to this set of trusted actors to set the kickback parameters correctly.
Each protocol has its own integration of the Oval deployment and is responsible for setting the addresses of the updaters in favor of the protocol. The permissions to add or remove actors from the permissioned set of updaters could be configured by that protocol's governance. We assume that the updaters will not act maliciously.
This design introduces two major failure modes that need to be engineered around. The modes in consideration are:
Both amount to the same failure mode: a delay in the liveness of updating the system. The proposed solution involves tracking an internal timer to record the last update time for each integration, and then comparing this with the time at which the Chainlink oracle was last updated. If the delta exceeds a defined threshold, then the Oval system should gracefully release the new price to the destination adapters. To achieve this, the Integration Controller exposes a function that returns such a threshold.
Another potential point of failure is the reliability of the oracle's pricing data. The source oracles should be chosen carefully by the governance of the protocol integrating the Oval system. Therefore, we assume that the integrated oracles work as expected.
Two main privileged roles were found in the system:
Owner: Two contracts were found to be ownable. The following is an explanation of each owner's role in their respective contract:
BaseController contract is allowed to set or remove an updater address, setting the time window that bounds how long the permissioned actor has to unlock the latest value after a new source update is posted and setting the maximum number of historical source updates to traverse when looking for a historic value in the past.UniswapAnchoredViewDestinationAdapter sets the mapping between cTokens and Ovals.In the Oval contract, if the unlockLatestValue function has been called within the lockWindow, the internalLatestData function will return the most recent price from the unlockLatestValue call. If the unlockLatestValue function has not been called during the lockWindow, the internalLatestData function will return the most recent value that is at least lockWindow old.
In the BaseController contract, the setLockWindow function is used to change the value returned from lockWindow. Suppose the setLockWindow function is used to extend the lockWindow so that the duration is longer than the time passed since the last update. In that case, the oracle will switch back to returning the cached values, changing to the previous price abruptly. Additionally, suppose the setLockWindow function is used to reduce the lockWindow so that the duration is shorter than the time passed since the last update. In that case, the oracle will return the values from the source oracle, changing the values abruptly.
Both cases can create an opportunity for users aiming to exploit MEV to sandwich the setLockWindow calls, taking advantage of sudden price changes.
Consider restricting the change in the setLockWindow function so that the return value from the internalLatestData function remains the same after setting the new value of the lock-window.
Update: Partially resolved in pull request #63 at commit a90bef5. The Risk Labs team stated:
Implemented the recommended fix in restricting the
setLockWindowfunction so that the return value from theinternalLatestDatafunction remains the same after setting the new value of the lock-window.
When changing the value of lockWindow_, after unlocking the latest value, if setLockWindow is called during the current lockWindow_ but is called outside the newLockWindow period, the second call to internalLatestData might return the most recent value that is at least lockWindow_ old, which could be different than the first call to internalLatestData causing the transaction to fail. The same issue is true if setLockWindow is called outside the current lockWindow_ period but is called during the newLockWindow period.
However, suppose on the Ethereum mainnet, if the lockWindow_ is being reduced from 60 seconds to a value ranging from 0 to 11 seconds, which is less than the 12 seconds block time (ATTOW), a malicious user might prevent the setLockWindow call from being included in the same block as the unlockLatestValue. If the setLockWindow transaction is added to a following block but called within 60 seconds of the unlockLatestValue call, the transaction might fail from being executed if the underlying source oracle is updated.
Although the proposed fix restricts from setting a newLockWindow if a change in the internalLatestData occurs, consider always ensuring that setLockWindow will be called during a valid period after unlocking the latest value.
The IAggregatorV3Source interface inherits from the IAggregatorV3 interface which implements Chainlink's deprecated functions latestAnswer and latestTimestamp. Moreover, the IAggregatorV3Source interface itself implements Chainlink's deprecated latestRound function.
Additionally, the UniswapAnchoredViewSourceAdapter contract in getLatestSourceData is using Chainlink's deprecated latestTimestamp function.
The usage of Chainlink's deprecated functions in ChainlinkDestinationAdapter contracts is justified to respect the interface of the protocols that already implement the deprecated Chainlink interface and to ease the switch to the newly developed Oval-based contract without introducing breaking changes to the destination protocol. However, the ChainlinkSourceAdapter contracts should follow the up-to-date interface from the Chainlink docs.
Consider replacing Chainlink's deprecated functions with the appropriate up-to-date counterparts. Moreover, consider not importing the IAggregatorV3 interface into the IAggregatorV3Source interface to avoid future mistakes and separate both codebases for source and destination adapters.
Update: Resolved in pull request #93 at commit 35d5f7c.
Throughout the codebase the following instances of incorrect documentation have been identified:
DecimalLib contains the following errors:
uint256 is 2^256 - 1 not 2^265.2^256 is approximately equal to 1.2e77, but not equal._tryLatestRoundDataAt function comment suggests that the function is trying to get the latest data as of the requested timestamp. If it is not available, the function will return the earliest data. However, the _searchRoundDataAt function comment states that the function might return newer data than the requested timestamp. Both aforementioned comments appear to contradict each other.Consider resolving these instances of incorrect documentation to improve the clarity and readability of the codebase.
Update: Resolved in pull request #66 at commit 623ca26, and in pull request #89 at commit 17ffd3b.
In the DecimalLib library, both convertingDecimals functions 1 and 2 take an input number answer scaled at iDecimals and return this number scaled at oDecimals.
If iDecimals < oDecimals and answer * 10^(oDecimals - iDecimals) > type(uint256).max, the convertingDecimals functions will revert.
Despite the low likelihood of this occurrence, consider including a code comment explaining this case as a precautionary measure.
Update: Resolved in pull request #64 at commit ecbdcd0.
In the DecimalLib library, both convertingDecimals functions 1 and 2 take an input number answer scaled at iDecimals and return this number scaled at oDecimals.
Suppose a protocol allows users to buy a token for a certain price which is returned by the oracle. Suppose that this protocol expects an output decimals number smaller than the input decimals. When downscaling the answer (i.e., when iDecimals > oDecimals), the conversion can lead to a loss of precision. In the worst case, if the answer is small enough, the conversion can return zero, allowing users to buy the token for a value of zero.
Consider adding a warning comment to highlight this issue for clients integrating this oracle.
Update: Resolved in pull request #61 at commit 9670e08.
The source adapter contracts interact with the incoming oracle interface that is ingested by the Oval system. The destination adapter contracts represent the standardized output interface that the Oval system exposes. In other words, the ChainlinkSourceAdapter is responsible for fetching the data from Chainlink, and the ChainlinkDestinationAdapter will return this data to the protocols implementing the Oval system. The data returned by ChainlinkDestinationAdapter should return the output decimals to match the decimals expected by the protocol. Optionally, if the source oracle's answer is based on a different decimals number than expected, the answer is converted using the DecimalLib library.
However, within the source and destination contracts of Chainlink, the data is converted twice. Once when fetched by the source adapter and another time when returned by the destination adapter.
For example, when calling tryLatestDataAt in the ChainlinkSourceAdapter, the answer returned is converted from the sourceDecimals to the expected output decimals. In the ChainlinkDestinationAdapter contract, when calling either latestAnswer, latestTimestamp, or latestRoundData, the answer returned from the internalLatestData is converted again.
Moreover, when converting from a higher to lower decimal output and then from lower to higher again, the data returned will be altered and will not match the original answer.
Consider handling the conversion in a single contract to avoid conversion duplication and data loss.
Update: Acknowledged, not resolved. The Risk Labs team stated:
We decided to not do anything in response to this issue. Specifically, we want all internal units within the OEVShare contracts to operate at 18 decimals to keep logic consistent and decimal independent within the OEVShare contracts. It also enables us to have different input and output decimals, by mixing and matching sources and destination adapters. Regarding loss of precision and the risk therein: this would only be the case if the input decimals are greater than 18 decimals. None of the feeds we want to use OEVshare on are more than 18 so there is no risk in this regard.
Throughout the codebase, there are several parts that do not have docstrings:
Consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Acknowledged, not resolved. The Risk Labs team stated:
No changes were made on this as these are public state variables. We historically do not document them with NatSpec.
revert StatementWithin the getTokenConfigByCToken function of UniswapAnchoredViewDestinationAdapter.sol, there is a revert statement on line 55 that lacks an error message.
Consider including specific, informative error messages in revert statements to improve the overall clarity of the codebase and avoid potential confusion when the contract reverts.
Update: Resolved in pull request #67 at commit 2628f26.
Throughout the codebase, instances of functions that are updating the state without an event emission were found. The contracts which could benefit from the addition of event emissions include:
constructor function in ChainlinkDestinationAdapter.sol.constructor function in ImmutableController.sol.constructor function in ImmutableController.sol.constructor function in ImmutableController.sol.constructor function in ChainlinkSourceAdapter.sol.constructor function in ChainlinkSourceAdapter.sol.snapshotData function in SnapshotSource.sol.constructor function in UniswapAnchoredViewSourceAdapter.sol.constructor function in UniswapAnchoredViewSourceAdapter.sol.constructor function in UniswapAnchoredViewSourceAdapter.sol.syncAggregatorSource function in UniswapAnchoredViewSourceAdapter.sol.unlockLatestValue function in OevShare.sol.setOevOracle function in UniswapAnchoredViewDestinationAdapter.sol.setOevOracle function in UniswapAnchoredViewDestinationAdapter.sol.constructor function in UniswapAnchoredViewDestinationAdapter.sol.setLockWindow function in BaseController.sol.setMaxTraversal function in BaseController.sol.setUpdater function in BaseController.sol.Consider emitting events whenever there are state changes to make the platform more verbose and improve its on-chain readability.
Update: Resolved in pull request #77 at commit b199370. The Risk Labs team stated:
Implemented the recommended fix in emitting events whenever there are state changes.
Throughout the codebase there are multiple functions that have variables that were unnecessary cast. For instance:
cToken variable in the syncAggregatorSource function in the UniswapAnchoredViewSourceAdapter contract.cToken variable in the getLatestSourceData function in the UniswapAnchoredViewSourceAdapter contract.To avoid clarify the intent and improve the readability of the codebase, consider removing these unnecessary casts.
Update: Resolved in pull request #69 at commit 7beb464.
Throughout the codebase, there are multiple state variables that have been shadowed. For instance:
lastUnlockTime state variable in the Oval contract is shadowed on line 36 in BaseController and line 35 in ImmutableController.To improve the overall clarity, intent, and readability of the codebase, consider renaming variables that shadow any state variables.
Update: Resolved in pull request #70 at commit f43507b.
The use of non-explicit imports in the codebase can decrease the clarity of the code, and may create naming conflicts between locally defined and imported variables. This is particularly relevant when multiple contracts exist within the same Solidity files or when inheritance chains are long.
Throughout the codebase, global imports are being used:
DiamondRootOval.solOval.solBaseDestinationAdapter.solDecimalLib.solSnapshotSource.solChainlinkDestinationAdapter.solChainlinkSourceAdapter.solUniswapAnchoredViewDestinationAdapter.solUniswapAnchoredViewSourceAdapter.solBaseController.solImmutableController.solFollowing the principle that clearer code is better code, consider using named import syntax (import {A, B, C} from "X") to explicitly declare which contracts are being imported.
Update: Resolved in pull request #78 at commit 3b9c6d9.
Providing a specific security contact, such as an email or ENS, within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice proves beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. Additionally, if the contract incorporates third-party libraries and a bug surfaces in these, it becomes easier for the creators of those libraries to make contact, inform the code owners about the problem, and provide mitigation instructions.
Every contract in the codebase omits the security-contract tag.
Consider adding a NatSpec comment on top of the contract definition with a security contact. Using the @custom:security-contact convention is recommended as it has been adopted by the Openzeppelin Wizard and the ethereum-lists.
Update: Partially resolved in pull request #81 at commit 96db8e3. The Risk Labs team only added the security tag to the OevShare contract.
Named return variables are a way to declare variables that are meant to be used within a function's body for the purpose of being returned as the function's output. They are an alternative to explicit in-line return statements.
Throughout the codebase, there are multiple instances of unused named return variables. For instance:
roundId return variable in the latestRoundData function in ChainlinkDestinationAdapter.sol.answer return variable in the latestRoundData function in ChainlinkDestinationAdapter.sol.startedAt return variable in the latestRoundData function in ChainlinkDestinationAdapter.sol.answeredInRound return variable in the latestRoundData function in ChainlinkDestinationAdapter.sol.answer return variable in the internalLatestData function in Oval.sol.timestamp return variable in the internalLatestData function in Oval.sol.Consider either using or removing any unused named return variables.
Update: Resolved in pull request #73 at commit 2063f8e.
Throughout the codebase, there are files that lack SPDX license identifiers. For instance:
DiamondRootOval.solBaseDestinationAdapter.solDecimalLib.solSnapshotSource.solUniswapAnchoredViewDestinationAdapter.solTo avoid legal issues regarding copyright and follow best practices, consider adding SPDX license identifiers to files as suggested by the Solidity documentation.
Update: Resolved in pull request #74 at commit b5f63e6.
Within ChainlinkSourceAdapter.sol, the state variable PHASE_MASK lacks an explicitly declared visibility.
For clarity, consider always explicitly declaring the visibility of variables, even when the default visibility matches the intended visibility.
Update: Resolved in pull request #79 at commit 3a1141b.
Throughout the codebase there are multiple instances of typographical errors:
Please note that this list is not an exhaustive list and more cases may be found. Consider fixing these and any other typographical errirs to improve the readability of the documentation.
Update: Resolved in pull request #75 at commit 1efe162.
Throughout the codebase, there are multiple unused state variables. For instance:
decimals state variable in the BaseDestinationAdapter contractWhile these are all abstract contracts, it may be best to define the decimals variables in the actual implementation.
To improve the overall clarity, intentionality, and readability of the codebase, consider standardizing and moving the decimals definitions to their respective implementation contracts.
Update: Resolved in pull request #80 at commit 950e0c0.
UPPER_CASE FormatThroughout the codebase, there are constants not using UPPER_CASE format. For instance:
lockWindow_ constant declared on line 15 in ImmutableController.solmaxTraversal_ constant declared on line 16 in ImmutableController.solchainlinkSource constant declared on line 14 in ChainlinkSourceAdapter.solsourceDecimals constant declared on line 15 in ChainlinkSourceAdapter.soluniswapAnchoredViewSource constant declared on line 16 in UniswapAnchoredViewSourceAdapter.solcToken constant declared on line 17 in UniswapAnchoredViewSourceAdapter.solsourceDecimals constant declared on line 18 in UniswapAnchoredViewSourceAdapter.soldecimals constant declared on line 12 in BaseDestinationAdapter.solAccording to the Solidity Style Guide, constants should be named with all capital letters with underscores separating words. For better readability, consider following this convention.
Update: Resolved in pull request #76 at 7a9b631.
ChainlinkSourceAdapter Does Not Correctly Validate Historical Data in _tryLatestRoundDataAtWhen validating the data returned from _searchRoundDataAt in the ChainlinkSourceAdapter._tryLatestRoundDataAt function, the implementation checks updatedAt for uninitialized historical data.
Update: Resolved in pull request #54 at commit b2807eb.
The Risk Labs team provided incredible supplemental documentation addressing the functionality and design decisions of the protocol. The code itself was very clean, well thought-out, and properly documented.