OpenZeppelin audited the UMAprotocol/managed-oracle repository at commit fc03083. The files that were diff-audited were compared against their versions present in the UMAprotocol/protocol repository at commit 6a23be1.
In scope were the following files:
src
├── common
│ ├── implementation
│ │ ├── AddressWhitelist.sol (diff)
│ │ └── DisableableAddressWhitelist.sol
│ └── interfaces
│ └── DisableableAddressWhitelistInterface.sol
└── optimistic-oracle-v2
└── implementation
├── ManagedOptimisticOracleV2.sol
└── OptimisticOracleV2.sol (diff)
The final state of the audited codebase, including all implemented resolutions, is reflected in commit 5b33321.
The UMA Optimistic Oracle protocol is designed for the efficient and rapid resolution of data requests on-chain. It operates on a "propose and dispute" model, whereby a proposed value is accepted as true after a set "liveness" period, provided no one challenges it. A proposer stakes a bond to submit a price, and if another party believes that the price is incorrect, they can also stake a bond to dispute it.
An undisputed proposal is settled optimistically, rewarding the proposer, while a disputed price is escalated to UMA's Data Verification Mechanism (DVM) for ultimate resolution, which then determines the winner and allocates the bonds and rewards accordingly. This core protocol is extended by the ManagedOptimisticOracleV2, which introduces a layer of administrative control, allowing designated managers to customize parameters like bond sizes and liveness periods for specific requests.
The ManagedOptimisticOracleV2 contract implements a multi-tiered, role-based access-control system to govern the oracle's operational parameters. At the highest level, the DEFAULT_ADMIN_ROLE holds the exclusive authority to upgrade the contract's implementation. Below this, the REGULAR_ADMIN role is responsible for managing system-wide settings, which includes configuring the default proposer and requester whitelists, and setting constraints such as the maximum bond and minimum liveness period. The contract also introduces a REQUEST_MANAGER role, which is managed by the REGULAR_ADMIN. This role has the ability to apply custom configurations to individual price requests, allowing it to override default parameters by setting specific bond amounts, liveness periods, and proposer whitelists on a per-request basis.
The DisableableAddressWhitelist contract extends the base AddressWhitelist contract by introducing a mechanism to toggle its enforcement. When enforcement is disabled, all addresses are considered whitelisted, effectively bypassing the underlying whitelist checks.
The AddressWhitelist contract was updated to ensure compatibility with the latest OpenZeppelin version and to improve its extensibility for inheriting contracts. The OptimisticOracleV2 contract was refactored to support upgradeability using the UUPS proxy pattern, and several of its functions have been made virtual to allow for extension by child contracts like ManagedOptimisticOracleV2. This refactoring also modernized the code by replacing the SafeMath library with Solidity's native overflow protection.
As mentioned above, the ManagedOptimisticOracleV2 contract involves multiple roles, and the security model of this system relies on a layered approach to access control and a set of critical trust assumptions regarding its administrators and configurations. The entities in charge of these roles are expected to follow best practices for account security, including robust key management and multi-factor authentication where applicable. The integrity and proper functioning of the protocol are contingent upon these assumptions always holding true.
DEFAULT_ADMIN_ROLE, REGULAR_ADMIN, and REQUEST_MANAGER roles operate in the best interest of the protocol and its users. The owner of the AddressWhitelist and DisableableAddressWhitelist contracts is trusted to manage the whitelist entries and enforcement status appropriately.requesterWhitelist, defaultProposerWhitelist, and any customProposerWhitelists are critical for controlling access and participation.DisableableAddressWhitelist mechanism's isEnforced state is assumed to be managed correctly and in accordance with the desired access policies.ManagedOptimisticOracleV2 contract are expected to match the implementation of the DisableableAddressWhitelist contract.The ManagedOptimisticOracleV2 contract introduces per-currency maximum bond amounts that are stored in the maximumBonds mapping. These limits can be set either during initialization by the contract initializer or later by an authorized admin. If no maximum bond is explicitly set for a given currency, the mapping returns its default value of zero. As a result, any non-zero bond passed to the contract will fail validation via the _validateBond() function, effectively preventing the usage of that currency until configured.
If this behavior is intentional, consider clearly documenting it in both the code and user-facing documentation. Alternatively, instead of defaulting to rejection, consider changing the logic such that currencies with unset maximum bond values have no enforced limit.
Update: Resolved in pull request #22 at commit ab33c3e. The UMA team added two comments to clarify the default rejection of a currency when no bond range is configured.
RequestManagerAdded and RequestManagerRemoved Events Can Be Wrongfully EmittedThe addRequestManager and removeRequestManager functions of the ManagedOptimisticOracleV2 contract are responsible for assigning and revoking the REQUEST_MANAGER role and are implemented through the grantRole and revokeRole functions of AccessControlUpgradeable.
These functions also emit the RequestManagerAdded and RequestManagerRemoved events unconditionally, regardless of whether the role was actually granted or revoked. This means that if addRequestManager is called for an address that already has the REQUEST_MANAGER role, or removeRequestManager is called for an address that does not, the corresponding event will still be emitted, which could be misleading for off-chain services that rely on these events to track state changes. Furthermore, these events are redundant, considering the fact that the AccessControlUpgradeable contract already emits the RoleGranted and RoleRevoked events.
Consider removing the RequestManagerAdded and RequestManagerRemoved events entirely.
Update: Resolved in pull request #23 at commit 46774d9.
The ManagedOptimisticOracleV2 contract includes several functions that accept external contract addresses expected to implement the DisableableAddressWhitelistInterface, such as the setDefaultProposerWhitelist, setRequesterWhitelist, and requestManagerSetProposerWhitelist functions. However, these functions do not verify whether the provided addresses actually implement the expected interface, which can result in runtime errors or incorrect behavior if an invalid contract is passed.
Consider supporting the ERC-165 standard in both the AddressWhitelist and DisableableAddressWhitelist contracts to facilitate safe and standardized interface detection. In addition, consider enforcing interface compliance through runtime checks to ensure that the provided whitelist contracts implement the DisableableAddressWhitelistInterface.
Update: Resolved in pull request #17 at commit 7bb517c. The DisableableAddressWhitelist contract was replaced with DisabledAddressWhitelist, thereby changing the implementation from an owner-controlled toggle for the enforcement of the list to a hard-coded implementation that can be set in the ManagedOptimisticOracleV2 contract. Both the DisabledAddressWhitelist and AddressWhitelist contract now extend ERC165 while ManagedOptimisticOracleV2 performs runtime checks whenever a whitelist is set.
The in-scope contracts, including OptimisticOracleV2 and ManagedOptimisticOracleV2, implement a sophisticated system for managing optimistic price requests. This system involves complex state transitions, financial stakes in the form of bonds, and a multi-layered access-control model. As such, the security and reliability of dependent protocols are critically reliant on the correctness of this logic.
Throughout the repository, there is an absence of a dedicated test suite for the contracts in scope. Without automated tests, there is no formal, repeatable process to verify that the contracts function as intended. Verifying the correctness of the intricate state machine, access-control logic, and financial calculations is left to manual review, which is error-prone and time-consuming. Furthermore, the lack of a regression suite means that future changes to the codebase could inadvertently introduce critical vulnerabilities without being detected.
Consider implementing a comprehensive test suite that covers the functionality of the scoped contracts. This suite should include unit tests for individual functions, integration tests to validate the interactions between contracts, and fork tests to simulate behavior in a realistic on-chain environment. The tests should validate expected outcomes, proper handling of edge cases, and adherence to the access control model. Establishing robust test coverage would significantly improve the maintainability and security posture of the system.
Update: Resolved in pull request #25 at commit 31b47fd. Tests for the ManagedOptimisticOracleV2 contract were added.
The AddressWhitelist contract provides functionality for managing lists of approved addresses. It uses a mapping to store the status of an address and a separate, dynamic array, whitelistIndices, to store every address ever added. The getWhitelist function iterates through this array to return a list of currently active members.
The current implementation of the whitelist has several drawbacks:
removeFromWhitelist function only changes the status of an address to Out but does not remove it from the whitelistIndices array. This "soft delete" causes the array to grow indefinitely, leading to an increasing gas cost for enumeration.getWhitelist function iterates through this ever-growing array twice, making it highly inefficient and creating a potential out-of-gas condition if the list becomes too large, as commented.None, Out, and In) is not only redundant compared to only two (Out and In), it also introduces an inconsistent state. First, if an address is removed from the whitelist despite having never been added, its status is set to Out. Second, if the address is then added to the whitelist, the address is not added to whitelistIndices but still marked as In with an event emitted, although the address will not be included in getWhitelist.EnumerableSet library.Consider refactoring the AddressWhitelist contract to use the EnumerableSet.AddressSet library provided by OpenZeppelin. Be aware that this adoption would shift the storage layout. As such, upgradeable contracts depending on AddressWhitelist should be handled with care.
Update: Resolved in pull request #24 at commit 81bd067.
The setMinimumLiveness function of the ManagedOptimisticOracleV2 contract allows the admin to set a new global minimum liveness without validating that the value falls within the bounds enforced by the underlying OptimisticOracleV2. If minimumLiveness is set above the maximum value accepted by OptimisticOracleV2, subsequent attempts of a requester or request manager to set custom liveness values will fail, resulting in a DoS for that functionality.
While this scenario is unlikely to materialize in practice, as it requires a misconfigured call from a privileged admin, the risk can be fully mitigated by adding validation. Thus, consider updating the _setMinimumLiveness function to enforce that the new value respects the bounds accepted by OptimisticOracleV2, preventing any accidental misconfiguration.
Update: Resolved in pull request #7 at commit bd0fe60.
The OptimisticOracleV2.sol contract is designed to be upgradeable and currently uses the storage gap pattern (uint256[998] private __gap;) to reserve space for future state variables. This is a widely used technique to prevent storage collisions in child contracts when the parent contract is upgraded with new variables.
However, while functional, the storage gap pattern is a manual and somewhat opaque method for managing upgradeability. It does not provide a clear, structured approach for how storage should be organized, especially in contracts with complex inheritance. This can make reasoning about the overall storage layout difficult for developers and auditors.
Consider adopting the namespaced storage pattern defined in EIP-7201. This standard provides a robust and explicit convention for managing storage layouts in upgradeable contracts. By grouping related state variables into structs and assigning them a unique, deterministic storage location based on a namespace ID, EIP-7201 makes the storage layout modular and easier to understand. Adopting this standard would improve the long-term maintainability and developer ergonomics of the contract's upgradeability strategy.
Update: Acknowledged, not resolved. The team stated:
This is a pretty involved change and will break storage assumptions of the contracts already deployed (since last week) which are still using
__gapinstead. Thanks for bringing this up though, we will consider using this pattern in future contracts.
view FunctionsThroughout the OptimisticOracleV2 contract, external view functions such as getRequest and getState are protected with the nonReentrantView modifier. This modifier prevents these functions from being called back into during the execution of a state-changing function, ensuring that they cannot read from a temporarily inconsistent state.
In the ManagedOptimisticOracleV2 contract, new external view functions have been introduced, namely getCustomProposerWhitelist and getProposerWhitelistWithEnforcementStatus. However, these functions, do not use the nonReentrantView modifier. This creates an inconsistency with the established security pattern of the parent contract. While no direct exploit was identified from this omission due to robust state checks elsewhere, it represents a deviation from parent contract.
Consider adding the nonReentrantView modifier to all new external view functions in ManagedOptimisticOracleV2. This would enforce a consistent security posture across the entire contract system, improve clarity for developers and auditors, and harden the contract against potential re-entrancy vectors that could arise from future integrations.
Update: Resolved in pull request #28 at commit 77b6f82.
Throughout the codebase, multiple opportunities for code improvement were identified:
_getManagedRequestId and getManagedRequestId functions into a single, public version.ManagedOptimisticOracleV2Events and ManagedOptimisticOracleV2 contracts are redundantly split and do not improve legibility as they exist within the same file. Consider moving the ManagedOptimisticOracleV2Events contract into a separate file, or simply removing it and adding the events directly in the ManagedOptimisticOracleV2 contract.address that are redundant, such as the conversion of msg.sender in the requestPrice function, or the conversions of requester in the proposePriceFor, disputePriceFor, and _settle functions. Consider removing these unnecessary casts.AddressWhitelist contract nor in theManagedOptimisticOracleV2 contract. Consider enhancing mapping documentation by using named parameters.newImplementation name of the address parameter in the _authorizeUpgrade function is unused, consider removing it.isSet boolean flag in the CustomBond and CustomLiveness structs is unnecessary because its state can be inferred from the value of amount or liveness, respectively. A zero value for either of these fields is not a sensible setting, so it can be used to signify that no custom value has been set, while any non-zero value indicates a custom setting is active. Removing this redundant boolean simplifies the logic and saves an extra storage slot for each struct, reducing the overall gas cost.Consider addressing the identified instances to improve the quality and maintainability of the codebase.
Update: Resolved in pull request #27 at commit dc580dd.
Throughout the codebase, multiple opportunities for naming improvements were identified:
REGULAR_ADMIN and REQUEST_MANAGER roles could be renamed to REGULAR_ADMIN_ROLE and REQUEST_MANAGER_ROLE.liveness field of the InitializeParams struct could be renamed to defaultLiveness.To improve the legibility of the codebase, consider addressing the suggestions made above.
Update: Resolved in pull request #16 at commit acba673 and pull request #5 at commit f747326. The team stated:
We'd like to stick to whitelist naming for legacy reasons. Many systems use the whitelist naming. We will consider renaming in the future.
Since Solidity version 0.8.4, custom errors provide a cleaner and more cost-efficient way to explain to users why an operation failed.
The OptimisticOracleV2 and ManagedOptimisticOracleV2 contracts use require and revert messages.
For conciseness and gas savings, consider replacing require and revert messages with custom errors.
Update: Resolved in pull request #8 at commit 7d085a8.
Providing a specific security contact (such as an email address or an ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite 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. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.
The OptimisticOracleV2, ManagedOptimisticOracleV2, AddressWhitelist, and DisableableAddressWhitelist contracts do not have a security contact.
Consider adding a NatSpec comment containing a security contact above each contract definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Update: Resolved in pull request #10 at commit 210c517.
Throughout the codebase, multiple instances of functions with unnecessarily permissive visibility were identified:
In ManagedOptimisticOracleV2:
_setMaximumBond function with internal visibility could be limited to private._setMinimumLiveness function with internal visibility could be limited to private._setDefaultProposerWhitelist function with internal visibility could be limited to private._setRequesterWhitelist function with internal visibility could be limited to private._getManagedRequestId function with internal visibility could be limited to private._validateBond function with internal visibility could be limited to private._getEffectiveProposerWhitelist function with internal visibility could be limited to private.In OptimisticOracleV2:
_getId function with internal visibility could be limited to private._getState function with internal visibility could be limited to private._getOracle function with internal visibility could be limited to private._getStore function with internal visibility could be limited to private._getIdentifierWhitelist function with internal visibility could be limited to private._getTimestampForDvmRequest function with internal visibility could be limited to private._stampAncillaryData function with internal visibility could be limited to private.To better convey the intended use of functions, consider changing the functions' visibility to be only as permissive as required. Otherwise, consider documenting that the contracts are expected to be extended by third party developers.
Update: Resolved in pull request #26 at commit ce47fb4.
DisableableAddressWhitelist Is Not Enforced By DefaultThe DisableableAddressWhitelist contract extends the AddressWhitelist contract and introduces the isEnforced flag to control whether the whitelist logic is active or not. By default, this flag is not explicitly set during construction and therefore defaults to false, meaning the whitelist is not enforced, and any address will pass the check. This creates a possibility for the owner to mistakenly assume that the whitelist is active without explicitly enabling it.
Consider modifying the DisableableAddressWhitelist contract to set the isEnforced flag to true by default, ensuring that whitelist behavior is enforced unless explicitly disabled.
Update: Resolved in pull request #6 at commit 2a816e1. The UMA team stated:
Not relevant anymore, as we moved from
DisableableAddressWhitelistto two separate contracts:DisabledAddressWhitelist+AddressWhiteList, which have clear behaviours consistent with their names
Throughout the codebase, multiple instances of misleading or incomplete documentation were identified:
@return NatSpec of the getCustomProposerWhitelist function should be changed to @return DisableableAddressWhitelistInterface, as per the implementation.@dev NatSpec of the getManagedRequestId function is incorrect.Consider addressing the above instances to improve the clarity of the codebase.
Update: Resolved in pull request #9 at commit b8a7ea2. The first suggestion does no longer apply with the removal of the DisableableAddressWhitelistInterface.
This report concludes a three-day audit of the managed-oracle protocol. Our analysis indicates that the protocol's design and implementation are fundamentally sound. We have identified several areas where code improvements can be made, and these have been detailed as findings in this report. These suggestions aim to enhance the robustness and efficiency of the existing codebase.
The codebase is well-commented and thoroughly documented, which greatly facilitated the review process. We would also like to commend the UMA team for their proactive approach in conducting their own internal audit, which had already identified several of the same points that we have raised. It was a pleasure to collaborate with them, and we appreciate their commitment to security and code quality.