UMA Continuous Audit

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 as well as the optimistic oracle and a new financial contract template. In this audit we are taking an iterative approach where we will review individual pull requests as they are developed by the UMA team. We will repeatedly update this report with any new findings for the duration of our engagement. Unless otherwise stated, the scope includes all solidity files affected by the specified pull requests.

Findings

To facilitate iterative reviews, these findings are listed in reverse chronological order.

Pull Requests 2828

This PR addressed the typographical errors and misleading comments that we noted in our review of 2668.

Pull Requests 2768

The PR introduces the KPIOptionsFinancialProductLibrary contract, which allows ExpiringMultiParty contracts to enforce a 2:1 collateral requirement before expiration. The details of the payout structure at expiration are deferred to the price identifier, which should be specified to ensure the price is capped at two collateral tokens per synthetic token.

Our only comment is to reiterate our previous recommendation about discarding the oracle price before expiration:

We believe that invoking the full oracle mechanism to achieve consensus on an unused parameter is unnecessarily wasteful. If modifying the contracts to bypass the oracle is infeasible, consider transforming the price identifier instead, so the oracle would be expected to return [a constant]. At the very least, this simplifies the data retrieval and validation requirements. It also enables future upgrades where the oracle can reuse constant results across different timestamps or simply return the constant if it is known ahead of time.

Update: The UMA team have acknowledged this and intend to address it in a pending upgrade to the ExpiringMultiParty contract.

Pull Requests 2744

This PR implements all of our recommendations from Pull Requests 2598 and 2672. We don’t have any comments.

Pull Requests 2598 and 2672

These PRs introduce and modify the MerkleDistributor contract. For simplicity, we reviewed the final result.

The contract has an owner that can easily distribute tokens to a large number of addresses by combining all transfer descriptions in a Merkle tree, publishing the root, and transferring the total number of funds to the contract. Subsequently, the tokens can be retrieved by providing a Merkle proof of an unclaimed transfer. The Merkle trees are not validated in any way, so the system assumes the contract owner behaves honestly.

The distributor allows anyone to prove and execute transfers on anyone elses behalf. A claimMulti function is provided to allow for multiple transfers to be claimed in a single transaction.

Since anyone can execute transfers on behalf of anyone else, it is possible for an attacker to “grief” a claimer by front-running their claim transaction with a transaction that performs the same transfer first. This will cause the honest user’s transaction to revert in the _verifyAndMarkClaimed function when it checks whether the claim has already occurred. This is not a major concern for single claims via the claim function, because the honest user ends up spending less gas than they normally would (because their transaction reverts early) and they still end up receiving their full reward.

However, this attack can be more problematic for the claimMulti function. The attacker can front run the call to claimMulti with a call to claim that performs only the last claim that the claimMulti call will attempt. This results in the entire claimMulti transaction reverting. In this way, an attacker can prevent an entire block of claims by making only a single claim. They may be motivated to do this, for example, to prevent large batches of new claims from being sold on the open market shortly after a new claim window opens.

If this is a concern, consider submitting all claimMulti transactions via a private transactions channel, such as SparkPool’s Taichi private endpoint.

Moreover, we have several recommendations to improve the quality of the code base:

  • The lastCreatedIndex variable name and comments suggest it indicates an existing window. In fact, it is the index that will be assigned to the next window. Consider updating the name and comments accordingly.
  • The WithdrawRewards event does not specify the reward currency. Consider including it as well.
  • The setWindow function performs the token transfer before emitting the event. If the reward token follows the ERC777 specification, it is possible that the caller has a tokensToSend hook that re-enters the setWindow function, which would cause the CreatedWindow events to be emitted in the wrong order. This may fall outside the threat model, since the function can only be called by the owner, who is assumed to behave honestly. Nevertheless, as a matter of good practice, consider emitting the event before performing the token transfer or introducing a reentrancy guard.
  • The code does not use Ethereum Natural Specification Format (NatSpec) comments. Consider following this specification on everything that is part of the contract’s public API.
  • The setWindow function implicitly assumes the caller has granted an appropriate allowance to the contract. Consider stating this explicitly in the function comments.
  • The setWindow function comment has an extra closing bracket.
  • The withdrawRewards function comment is missing the space in the phrase “in case”.

Update: All recommendations were implemented in Pull Request 2744.

Pull Request 2668

This PR introduces the CoveredCallFinancialProductLibrary contract, which allows ExpiringMultiParty contracts to mimic the payout structure of covered call options.

Before the option expires, the transformPrice function discards the oracle price and returns 1. We believe that invoking the full oracle mechanism to achieve consensus on an unused parameter is unnecessarily wasteful. If modifying the contracts to bypass the oracle is infeasible, consider transforming the price identifier instead, so the oracle would be expected to return 1. At the very least, this simplifies the data retrieval and validation requirements. It also enables future upgrades where the oracle can reuse constant results across different timestamps or simply return the constant if it is known ahead of time.

We would also like to note some typographical errors:

  • Line 77 says “an collateral token” instead of “a collateral token”
  • Line 86 says “cover call” instead of “covered call”

Lastly, while investigating this pull request, we identified a misleading comment in Line 454 of the Liquidatable contract that states the sponsor and disputer reward percentages cannot (cumulatively) exceed zero but the actual restriction is that they cannot exceed 1.

Update: The UMA team have acknowledged the suboptimal transformPrice function and intend to address it in a pending upgrade to the ExpiringMultiParty contract. The typographical errors and misleading comment are fixed in Pull Request 2828.

Pull Requests 2600

This PR removes restrictions in the DesignatedVotingFactory contract. It is now possible for voters to change the DesignatedVoting contract that they are associated with. We don’t have any comments.

Pull Requests 2395 and 2546

These PRs add additional parameters to the ProposePrice and DisputePrice events in the OptimisticOracle. They also remove unnecessary “//Event” comments preceding some event emissions. We don’t have any comments.