January 10, 2023
This security assessment was prepared by OpenZeppelin.
We audited the UMAprotocol/protocol repository at the fed1c5677b5e764eb0b3e59817941c129e6c26f7 commit.
In scope were the following contracts:
packages/core/contracts/optimistic-asserter
├── implementation
│ ├── OptimisticAsserter.sol
│ └── escalation-manager
│ ├── FullPolicyEscalationManager.sol
│ └── BaseEscalationManager.sol
└── interfaces
├── OptimisticAsserterCallbackRecipientInterface.sol
├── OptimisticAsserterInterface.sol
└── EscalationManagerInterface.sol
UMA’s OptimisticAsserter contract uses the same incentive mechanism as the Optimistic Oracle to quickly resolve claims that could be resolved by the (slow) Data Verification Mechanism (DVM). The main difference is that rather than being responses to previous requests, claims are asserted directly.
In both cases, the claim asserter posts an associated bond, and there is a short time window where someone else can dispute the claim and post their own bond. Disputes are adjudicated by the existing DVM and the loser’s bond is sacrificed and partially transferred to the winner. If the DVM is functioning correctly, its decision is predictable, so there is no reason to post invalid claims or make invalid disputes. If the claim is not disputed within the time window, it can be assumed to be valid without invoking the DVM.
The OptimisticAsserter contract also allows the process for resolving claims to be customized by the claim asserter. Specifically, they can choose:
This allows the contract to be used in a much larger set of contexts. The last three properties are decided by assigning an Escalation Manager, which is a contract that can choose these policies on a per-assertion basis.
Even when the “disregard oracle” option is chosen, the dispute still needs to be resolved by the oracle in order to distribute the bond. The flag simply indicates that the asserter will not wait for the resolution and will likely create a new assertion instead.
The OptimisticAsserter contract has an owner that can choose the fraction of the loser’s bond that is sent to the Store contract when a dispute is resolved by the DVM. The owner can also set the default bond currency and liveness parameters, which apply whenever the claim asserter chooses to use the default values. Consequently, asserters must implicitly trust the owner not to front-run their assertion with unreasonable parameters.
The FullPolicyEscalationManager contract, intended to be used as an escalation manager, also has an owner that has complete control over which assertions it will accept, the oracle results, the disputer whitelist, and whether the oracle results are disregarded.
Given the extensive customization possibilities, there are several ways for a claim asserter to create invalid assertions that will nevertheless not be disputed. For example, they could choose an unreasonably small dispute window, a prohibitively large bond, callbacks that revert dispute operations, or even an escalation manager that provides a biased oracle or refuses valid disputers. Consequently, potential disputers and anyone who relies on a resolved claim should ensure the assertion was fairly structured. In many cases, the relevant participants (claim asserters or escalation manager owners) will be contracts that can be vetted for reasonable configurations.
Here we present our findings.
When creating an assertion, the minimum bond depends on the current value of the configurable burn percentage. Asserters and disputers would reasonably expect their reward to be the bond reduced by the burnedBondPercentage at the time the assertion was originally made. However, in contrast to other configurable parameters, the burn percentage is not cached, resulting in the actual reward being reduced by the most recent value at the time of settlement.
Consider caching the burn percentage and using it consistently throughout the life of an assertion.
Update: Acknowledged, not resolved. The UMA team stated:
We’ve decided to not make this change. The implications of the issue presented here is that the voters vote to change the bond burn percentage after an assertion is disputed with the goal of stealing it from the disputer (for example setting the bond burn percentage to 100% after an assertion is made). We feel that this is a non-issue as the reputational damage of the DVM doing this would destroy the project and the cost to the protocol would be more than the value extracted from taking the reward.
Additionally, we are at the stack depth for the assertion object and adding an additional property to an assertion (bond burn percentage at assertion time) would require additional data structures or other refactors to accommodate this that we don’t want to do at this time.
The setArbitrationResolution function in the FullPolicyEscalationManager contract can be called multiple times for the same requestId. This would allow the contract owner to change the price for a given requestId, invalidating the resolvedPrice result in the OptimisticAsserter that was used when the settleAssertion function was called.
Consider restricting the setArbitrationResolution function to only allow setting the resolution once per requestId.
Update: Resolved in pull request #4319 with commit 76bed2a.
In the BaseEscalationManager contract, the following public functions may benefit from access control:
Currently anyone can call these functions, which can result in them executing state-changing logic in an unintended context.
Consider implementing appropriate access control on these functions so that only the expected instance(s) of the OptimisticAsserter contract can call them.
Update: Resolved in pull request #4334 with commit dd99434.
Consider fixing the following docstrings and inline comments:
OptimisticAsserter.sol, the comment on line 88 claims the caller is the asserter, but that is not necessarily true.FullPolicyEscalationManager.sol, comments on line 161, line 171, and line 180 for the respective setDisputeCallerInWhitelist, setWhitelistedAssertingCallers, and setWhitelistedAsserters functions state “Adds a … to the whitelist”. In reality, the functions can both add and remove from the whitelists.Update: Resolved in pull request #4320 with commit b943b23.
Several contracts and functions in the codebase lack complete documentation:
assertionResolvedCallback has undocumented assertionId and assertedTruthfully parameters.assertionDisputedCallback has an undocumented assertionId parameter.setDisputeCallerInWhitelist has an undocumented value parameter.setWhitelistedAssertingCallers has an undocumented value parameter.setWhitelistedAsserters has an undocumented value parameter.assertTruthWithDefaults has an undocumented asserter parameter.assertTruth has an undocumented return value.getCurrentTimestamp has an undocumented return value.internal are undocumented.Incomplete documentation hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security, but also correctness. Additionally, docstrings improve readability and facilitate maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned, and the events emitted.
Consider thoroughly documenting all functions and their parameters. When writing docstrings, especially for publicly-exposed functions, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #4342 with commit c831b8c.
When a currency is whitelisted and cached in the OptimisticAsserter contract, it is assumed that the Store contract will have a final fee set for the currency address. If the final fee has not been set, then assertions can be posted without a bond.
Prior to whitelisting a currency, consider adding a check that ensures the final fee from the Store contract is non-zero in the syncUmaParams and _validateAndCacheCurrency functions.
Update: Acknowledged, not resolved. The UMA team stated:
We’ve decided to not make this change. There should never be collateral currencies added that don’t have a final fee by DVM. If this was to occur, it would not pose too much of an issue within the system as 0 bonded assertions don’t really enable you to do much other than spam the DVM. if you were to spam the DVM with this technique we can delete the requests at the DVM level.
Added to this (and more importantly) there is a feature of having zero-sized bonds in one important situation: testnets. It is really convenient to not have to force users on testnets to approve and pay collateral currencies when making simple tests as this strictly increases the friction with using the system.
The requestPrice and getPrice functions in the BaseEscalationManager contract are stubs that do not perform any oracle actions. Additionally, the getPrice function has a return type of int256 but does not return any value. In its current state, this base contract does not implement a fully functional escalation manager.
Consider making the BaseEscalationManager contract abstract and removing the function body of the getPrice function to force implementation by derived contracts.
Update: Acknowledged, not resolved. The UMA team stated:
We think it’s better for this to not be abstract as it makes derived contracts easier and shorter to implement; if you don’t care about a particular feature then you just do nothing and it defaults to the disabled/simple behavior. For example if you don’t care about disabling disputes then the base implementation just returns true for
isDisputeAllowed. By making this abstract we’d force implementing contracts to have to declare and implement things, even if they don’t care about that functionality.
In OptimisticAsserter.sol, the defaultIdentifier constant is not declared using UPPER_CASE format.
According to the Solidity Style Guide, constants should be named in all capital letters with underscores separating words. For better readability, consider following this convention.
Update: Acknowledged, not resolved. The UMA team stated:
While it goes against the solidity styling guide, we’d like to keep this lowercase so that the accessor function
optimisticAsserter.defaultIdentifier()looks the same as the other assessors of similar type (optimisticAsserter.defaultCurrency()andoptimisticAsserter.defaultLiveness()).
The OptimisticAsserter contract is designed to allow disputed assertions to be arbitrated by either the UMA Data Verification Mechanism (DVM) or an escalation manager contract specified by the asserter. The arbitrateViaEscalationManager setting controls whether the DVM or escalation manager will act as the oracle for arbitration.
Even when an escalation manager is chosen instead of the DVM, the assertTruth function still performs checks that are DVM-specific:
require(_validateAndCacheIdentifier(identifier), "Unsupported identifier");
require(_validateAndCacheCurrency(address(currency)), "Unsupported currency");
require(bond >= getMinimumBond(address(currency)), "Bond amount too low");
These are restrictions put in place in order to interact with the DVM, but they are not necessarily required by a generic escalation manager. These restrictions force all escalation managers to use the same currencies as the DVM for bond payment, and also impose a minimum bond amount based on the DVM final fee that is unrelated to each escalation manager’s costs. The identifiers supported by each escalation manager must also match those supported by the DVM.
Additionally, while the existing design allows the DVM to collect a fee for providing oracle services, it does not allow an escalation manager to collect any fee to cover its potential cost for providing the same service. Moreover, since the usage fee and burn amount are coupled, none of the bond is burned when the escalation manager is used as an oracle.
To provide a more generic and flexible design, consider uncoupling escalation managers from the constraints of the DVM and allowing all oracles to specify their supported identifiers, currencies, and fees. Applying an equal treatment to all oracles should allow for the removal of special-case logic for handling different oracle types.
Update: Resolved in pull request #4343 with commit 25d22a9. The oracleFee computed in settleAssertion is now collected in all dispute cases, even when the escalation manager is used as the oracle, to ensure that there is always a fee associated with any dispute. Additionally, the UMA team stated:
The feature of using escalation manager for dispute arbitration is designed only as a fallback mechanism to allow integrating partners to unplug from UMA DVM if it is considered under risk of being corrupted. This is not meant to be feature-full replacement of existing UMA Oracle system, thus we kept the same DVM constraints also when arbitrating disputes within the escalation manager. Otherwise it would require escalation managers to implement their own whitelisting and fee management mechanisms that is too much overhead just for the fallback scenario.
Within the codebase, some events do not have their parameters indexed. Indexing event parameters facilitates the task of off-chain services searching and filtering for specific events. In particular, the following events may benefit from the addition of the indexed keyword to certain parameters:
In FullPolicyEscalationManager.sol:
DisputeCallerWhitelistSet event: Consider indexing address disputeCaller.AssertingCallerWhitelistSet event: Consider indexing address assertingCaller.AssertingWhitelistSet event: Consider indexing address asserter.Update: Resolved in pull request #4323 with commit 6ad9643.
The argument to the getMinimumBond function in the OptimisticAsserterInterface interface is named currencyAddress, but it is named currency in the OptimisticAsserter contract on line 358.
For clarity, consider renaming one of the arguments so the interface matches the implementation.
Update: Resolved in pull request #4324 with commit dffb20e.
In contrast to the AssertionMade and AssertionSettled events, the AssertionDisputed event does not emit the calling address. Consider including it for completeness.
Update: Resolved in pull request #4325 with commit 9c37198.
All of the contract files in the escalation-manager directory lack SPDX license identifiers.
To 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 #4326 with commit 5e36c85.
The multicall function of the MultiCaller contract is declared payable but does not accept any ETH. In the interest of clarity, consider removing the payable keyword.
Update: Resolved in pull request #4327 with commit 1d416c9.
The _isDisputeAllowed function in the OptimisticAsserter contract may call isDisputeAllowed on an optional EscalationManagerInterface contract. In practice, the policy is to not validate disputers when no escalation manager is specified, so the function will exit early instead of invoking a function on the zero address.
Nevertheless, in the interest of local reasoning, consider adding a zero-address check on the em address prior to using it to make external function calls. Also consider swapping line 449 and line 450 so that the em address assignment is only made when this variable is required.
Update: Resolved in pull request #4336 with commit 9543282.
Consider addressing the following instances of redundant code within the codebase:
override keyword can be removed from all functions in the BaseEscalationManager contract.requestId is computed on line 89 and line 155 in the FullPolicyEscalationManager contract. Consider deduplicating this code by creating a shared getRequestId function. Additionally, consider making this function publicly available for user convenience.FullPolicyEscalationManger contract, the require statement in the configureEscalationManager function can be simplified:require(!_blockByAsserter || (_blockByAsserter && _blockByAssertingCaller), "Cannot block only by asserter");Execution can only reach the (_blockByAsserter && _blockByAssertingCaller) condition if the first condition !_blockByAsserter evaluates to false, which means the second condition will always evaluate to (true && _blockByAssertingCaller). This further reduces to just _blockByAssertingCaller.assertions[assertionId].escalationManagerSettings.escalationManager happens on lines 435, 443, 449, 471, 472, 480, and 481 in the OptimisticAsserter contract. Consider modifying the existing _getEscalationManager function to return an address type, and using it in all the identified locations other than line 435 to perform the escalationManager lookup. This change would also enable the removal of the address() cast that occurs on line 414 where _getEscalationManager is currently used._callbackOnAssertionResolve and _callbackOnAssertionDispute functions within the OptimisticAsserter contract, the lookup of assertions[assertionId].callbackRecipient and assertions[assertionId].escalationManagerSettings.escalationManager happens twice within each function’s scope. In each case, consider storing the value obtained from the first lookup in a local variable for reuse.return statement in the assertTruth function is redundant because assertionId is a named return variable in the function specification. Consider removing this return statement.Update: Partially resolved in pull request #4335 with commit 59b13d3. The _isDisputeAllowed function still performs the lookup assertions[assertionId].escalationManagerSettings.escalationManager instead of using the _getEscalationManager function.
In Solidity, functions with public visibility can be used both internally and externally. In the case a function is only ever used externally, it is best practice to limit the visibility to external rather than public. Below is a list of functions that have public visibility that can be limited to external visibility in the OptimisticAsserter contract:
To improve the explicitness of the codebase, and to reduce gas usage, consider changing the visibility of these functions to external.
Update: Resolved in pull request #4337 with commit bbe05e1.
Consider correcting the following typographical errors:
In BaseEscalationManager.sol:
In FullPolicyEscalationManager.sol:
In OptimisticAsserter.sol:
In OptimisticAsserterInterface.sol:
Update: Resolved in pull request #4339 with commit 97be5e6.
In EscalationManagerInterface.sol, the import OptimisticAsserterInterface.sol is unused and could be removed.
Consider removing unused imports to avoid confusion that could reduce the overall clarity and readability of the codebase.
Update: Resolved in pull request #4338 with commit e4f47ee.
Within the OptimisticAsserter contract, named return variables are used inconsistently. Most external and public functions in this contract that return a value use a named return variable, but stampAssertion, getMinimumBond, and getCurrentTime do not assign names. None of the internal functions use named variables, making them consistent with each other but inconsistent with the external/public functions. Additionally, the following functions have unused named variables: assertTruthWithDefaults, settleAndGetAssertionResult, getAssertion, and getAssertionResult.
When choosing to use named return variables, consider using them in a consistent manner throughout all functions within the same contract.
Update: Resolved in pull request #4341 with commit 59364ce. The unused named return variables were removed.
The OptimisticAsserter and FullPolicyEscalationManager contracts use the constant value 1e18 in the settleAssertion and getPrice functions without documenting the value’s meaning.
Literal values in the codebase without an explained meaning make the code harder to understand and maintain, thus hindering the experience of developers, auditors, and external contributors alike. To improve the code’s readability and facilitate refactoring, consider defining a constant for every magic number, giving it a clear and self-explanatory name.
Update: Resolved in pull request #4340 with commit 5ba00f8. The OptimisticAsserter and FullPolicyEscalationManager contracts now independently define a numericalTrue constant equal to 1e18 and provide an explanatory comment.
No critical or high severity issues were found. Several recommendations were proposed to follow best practices and reduce the potential attack surface. We also recommend implementing monitoring and/or alerting functionality (see Appendix).
While audits help in identifying potential security risks, the UMA team is encouraged to also incorporate automated monitoring of on-chain contract activity into their operations. Ongoing monitoring of deployed contracts helps in identifying potential threats and issues affecting the production environment.
_validateAndCacheIdentifier and _validateAndCacheCurrency functions in the OptimisticAsserter contract store price identifiers, whitelisted currencies, and associated final fee amounts so they don’t need to be retrieved again from the rest of the UMA system. However, these parameters can be invalidated by the removeSupportedIdentifier, removeFromWhitelist, and setFinalFee functions. Consider monitoring the SupportedIdentifierRemoved, RemovedFromWhitelist, and NewFinalFee events to detect potential cache invalidations. A follow-on automated action would be to call syncUmaParams and pass in the relevant identifier or currency value to be updated.AssertionMade event for unusually high bond values, which may be indicative of suspicious activity. Because bond values much higher than the minimum required bond may be encountered during normal operation of the protocol, this will likely require some data gathering in order to determine where to set the outlier threshold.AdminPropertiesSet event.