We reviewed Pull Request #4816 of the UMAprotocol/protocol repository. The scope includes the Solidity contract changes for OracleBaseTunnel (from which OracleRootTunnel inherits), OracleChildTunnel, and OracleSpoke, scripts within packages/scripts/src/admin-proposals, and changes to tests in packages/core/test/hardhat.
Additionally, we reviewed the plan outlined in the proposal (UMIP-185) intended to be used to perform the upgrade across Ethereum, Polygon, Optimism, Arbitrum, Base, and Blast networks.
When handling Oracle requests on chains other than Ethereum mainnet, additional ancillary data is appended to the bridged data. This data is intended to be resolved by off-chain components and relayed to Ethereum mainnet. The proposed change involves compressing ancillary data for Oracle requests when bridged to Ethereum mainnet by sending only a hash of the data along with metadata (childBlockNumber, childOracle, childRequester, childChainId), as detailed in proposal. This aims to reduce gas costs for both off-chain components and users (voters). The upgrade involves deploying new versions of the OracleRootTunnel, OracleChildTunnel, and OracleSpoke contracts and updating relevant system parameters via governance.
New request IDs are calculated using the requester address. Similarly, hasPrice and getPrice use msg.sender as the requester to retrieve a price. As a result, if the OptimisticOracleV2 implementation is upgraded in the future, the new contract instance will not be able to access prices submitted through the previous implementation.
Since this issue only occurs if the OptimisticOracleV2 contract is replaced, consider whether this scenario is possible and if changes are needed in OracleSpoke or OracleChildTunnel.
Update: Resolved in commit 3b1a2b4 of pull request #4816 by removing the requester from the child request ID derivation. The team stated:
As we see it, the only theoretical drawback is someone could frontrun OOv2 dispute by requesting/proposing/disputing the same id/time/ancillary data from OOv1 which would make it ambiguous for DVM voters to figure out the true origin of the request. But one can argue its not absolutely necessary as answer should not depend on who's asking. And this attack is not possible in practice as our OO implementations append
ooRequesterfield, so you cannot spoof the ancillary data. Hence, we opted to simplify by using the same inherited_encodePriceRequestmethod and potentially making it easier to handle future OO upgrades.
There are several comments that could be updated to enhance clarity:
"its" could be changed to "it is" here.
"ancillary data" could be changed to "compressed ancillary data" here, as parentRequestId is calculated based on compressed data.
This and this comment state that compressed data is only returned when the original data exceeds a threshold, but in the current implementation, ancillary data is always compressed.
Update: Resolved at commit 8e05fe6 of the same pull request.
The resolveLegacyRequest function of the OracleSpoke contract allows setting a price for any childRequester as long as the legacy request has been resolved. As a result, the ResolvedLegacyRequest event may be emitted multiple times. It is also possible to set new price entries this way, but they are indexed by a keccak256 hash, avoiding potential collisions.
Consider whether repeated event emissions might be problematic for off-chain components.
Update: Resolved. The team stated:
Previous
OracleSpokedid not include requester in its stamped ancillary data, so we cannot limit theresolveLegacyRequestto any single one requester. Only maybe hardcoding known requester addresses and checking them, but even that still could have more thanResolvedLegacyRequestevents emitted. This though should not impact the functioning of OO as the main reason for this event was to check if a particular legacy child request has been resolved to new format. Off-chain one can recalculatepriceRequestIdfor a known valid requester and filter events accordingly.
The verification script is missing a check on whether the CONTRACT_CREATOR role has not been retained after the upgrade has been performed.
Consider adding a validation step to ensure that no unneeded roles are retained.
Update: Resolved in commit e452d64.
Consider running the DAO proposal simulation on a forked environment and evaluating the correctness of the state after the upgrade.
Update: Scripts to simulate the upgrade on a forked environment have been included, along with verifications.
Currently, the only end-to-end test available for Oracle requests and resolving is the one performed for the Polygon network. However, corresponding end-to-end tests for rollup chains are not present. Even though the existing unit tests cover the codebase extensively, we recommend also adding end-to-end tests to improve cross-chain mechanics coverage, configuration validation, and regression testing for new changes.
Update: The team stated:
We acknowledge the recommendation and will create an end-to-end test for the Oracle request/resolving flow for rollup networks that utilize the hub and spoke mechanism.
The reviewed pull request and proposal introduce changes to compress ancillary data in Oracle requests, aiming to reduce gas consumption. Only minor issues were found, all of which have been addressed. The DAO proposal was also reviewed and confirmed to include all necessary steps, as outlined in the UMIP, to upgrade the required components with the new contract implementations.