UMA is a platform that allows users to enter trust-minimized financial contracts on the Ethereum blockchain. We previously audited the decentralized oracle component of the system. In this audit we reviewed a particular financial contract template that can be used within the system.
The audited commit is
e6eaa48124ae3f209fb117cf05eb18292cf26d21 and the scope includes all contracts in the
financial-templates directory. The
WETH9 contract was not included, since it is used only for testing.
Additionally, it should be noted that the financial template design depends on a number of economic and game-theoretic arguments and assumptions. These were explored to the extent that they clarified the intention of the code base, but we did not audit the mechanism design itself.
All external code and contract dependencies were assumed to work as documented.
All issues listed below have been fixed or accepted by the UMA team. During the review of the fixes for this audit, the UMA team made four additional changes to their code base. These were addressed in PR#1334, PR#1356, PR#1368 and PR#1395.
Our analysis of the mitigations assumes any pending pull request will be merged, but disregards all other unrelated changes to the code base.
Here we present our findings, along with the updates for each one of them.
Overall, we are satisfied with the security posture of the team and the code base’s overall health. As with the oracle audit, we are pleased to see the use of encapsulated functions and well-documented contracts. We must also highlight the team’s clear and transparent analysis of known risks, particularly in a system with a novel design.
We would also like to take this opportunity, like the previous audit, to highlight the UMA team’s outstanding software development practices in terms of mandatory code reviews, documentation and testing. As before, the reviewed fixes were implemented in encapsulated pull requests that included associated unit tests as well as design discussions occurring in public.
Users of the UMA system can deploy new financial contracts that issue synthetic ERC20 tokens that derive value from an external asset. In the audited template, there are whitelists for the supported underlying asset pairs, expiry dates and acceptable ERC20 collateral tokens. The asset whitelist is managed by the
Governor contract, described in our previous audit, and the collateral whitelist is set during deployment. Any user can choose a combination of these values to deploy a new financial contract.
Subsequently, anyone can take an over-collateralized loan of freshly minted synthetic tokens that will be redeemable for the value of the specified asset at the expiry date, paid in the collateral currency. At expiry, the loan can be repaid with synthetic tokens or by withholding some of the collateral. The synthetic tokens themselves can be freely traded.
One novel, and fundamental, feature of this system is the ability to encourage borrowers to maintain sufficient collateralization without on-chain access to a live price feed. This is achieved by limiting the amount of collateral that can be immediately withdrawn from a position and rewarding liquidators who find and challenge under-collateralized loans. If the liquidation itself is disputed, the price will be retrieved from the UMA oracle to resolve the dispute.
While we encourage innovative designs, the unknown price constraint introduces some surprising risks and behaviors that users should understand before participating. Many of them are listed in UMA’s documentation. We would additionally like to highlight:
- The documentation suggests that contract deployers can choose a larger period for withdrawals and disputes if they are worried about network congestion. However, in the audited template, these deadlines must be set to one hour.
- Any position, no matter how collateralized, can be liquidated (and immediately closed). If the liquidation is not disputed, all collateral will be lost. This means that even over-collateralized borrowers must constantly monitor their position, or rely on external dispute bots to challenge improper liquidations.
- UMA oracle fees are charged on the deposited collateral, not on the value of borrowed tokens. This introduces a disincentive for over-collateralization. The fees themselves are managed by the UMA governance mechanism, which must be trusted to choose and change them fairly.
- The deposit and withdrawal limits are restricted by the overall global collateralization ratio, which is somewhat manipulable and is expected to exceed the actual collateralization requirement.
- The UMA oracle has an indefinite resolve time, because failed votes can be repeatedly deferred until the next voting round. During this time, all open positions are frozen but they continue accruing fees.
As the ecosystem becomes more interconnected, understanding the external dependencies and assumptions has become an increasingly crucial component of system security. To this end, we would like to discuss how the financial contracts depend on the surrounding ecosystem.
Like the oracle, the financial template does not interact with external projects but it uses time-based logic, which means it is dependent on the availability of Ethereum in multiple ways:
- Undesirable actions, such as withdrawing too much collateral or liquidating colleteralized loans, are discouraged by the threat of these actions being noticed and corrected within an hour. If they are not detected or the transactions are not processed in time, some users will lose their collateral.
- Similarly, financial contracts must pay fees at least every week, on a rolling window, to avoid a late penalty.
- Additionally, as the deadline approaches, it becomes possible for miners to delay payment actions in order to collect this fee.
- Ongoing fees are specified as a percentage of collateral, but they compound with each fee-paying transaction. This implies that payment delays or long periods without transactions will result in higher fees (in addition to the late penalty, if it applies).
[H01] Insolvent initial position
The initial sponsor in any contract is free to open a position with any amount of tokens for any amount of collateral, even if it would be insolvent. Since it is non-economical to liquidate an insolvent position, they will not be liquidated. Although, the UMA team intends to liquidate insolvent positions, some discretion would be necessary (or UMA will become a money pump).
This has two important consequences. Firstly, if the sponsor sold their synthetic tokens for the nominal face value, the recipient would be unable to settle them upon contract expiration. Secondly, if there are any other users with open positions when the contract expires, their collateral becomes vulnerable to theft by the initial sponsor.
In both cases, the issue could be prevented by due diligence. Yet if all traders and borrowers were expected to check the collateralization ratio before using synthetic tokens, it would severely limit the usability of the system.
Unfortunately, the described situation can occur multiple times in the same financial contract if all positions are closed and a new one is opened. Nevertheless, the system must prevent insolvent contracts in order to function correctly.
Consider submitting a price request before the first position is opened (as well as in subsequent “initial” positions) to ensure it is collateralized. Should this be prohibitive, consider requiring the initial sponsor to submit a liquidation bond that must expire before they receive their tokens.
Alternatively, consider preventing users from completely withdrawing, redeeming or liquidating the final position. This would not prevent an initial insolvent position but it would strengthen the guarantee that a healthy contract will remain collateralized.
Update: The issue is acknowledged, but the implementation will remain unchanged to maintain the user experience. In the (abridged) words of the UMA team:
We believe the UX and complexity tradeoffs of a lockup period outweigh the downside of asking new participants to be aware of the solvency of the contract they’re participating in. As for a contract returning to this state, token holders can be assured that as long as they hold a single token, this initial state cannot be reached again. Similarly, a sponsor can be assured that collateral introduced while the contract is solvent cannot be taken unfairly […] We do think this issue should be solved, but we think that it would be better done through a more major re-architecture of the contract for V2.
[H02] Sponsors can force liquidation of insolvent positions
Liquidators specify a maximum collateral-to-token ratio to ensure they do not accidentally liquidate a position that is collateralized. However, they cannot indicate a minimum collateral-to-token ratio. If the liquidation is front-run in such a way that the target position becomes insolvent (not just under-collateralized), the liquidator will end up burning their tokens with insufficient compensation. We have identified two possible attack vectors:
- A) If the position about to be liquidated is the only open position, the sponsor can redeem their tokens (which reduces the global collateralization ratio to zero) and then create an insolvent position.
- B) If the liquidation is processed late enough, the sponsor might simply complete a slow withdrawal before being liquidated.
Consider including a minimum collateral-to-token ratio to prevent liquidators from accidentally liquidating an insolvent position.
Update: Fixed in PR#1351.
[M01] Sponsors can delay liquidations
Any under-collateralized sponsor can delay liquidation attempts by transferring their position to another address that they control before the liquidation transaction targeted at the old address is processed. This would cause the liquidation to fail.
In most cases, sponsors that recognize that are about to be liquidated would redeem their position. However, were sponsors able to successfully delay liquidation for long enough, they could keep the global collateralization ratio artificially depressed, potentially below the collateralization requirement. In this scenario, sponsors would be able to create under-collateralized positions or withdraw excessive collateral. In the extreme case, sponsors could delay liquidation until they were insolvent.
Consider delaying position transfers for a short time window, thereby treating them similar to slow withdrawals.
Update: Fixed in PR#1314. Transfers of positions are now delayed for the same duration as slow withdrawal requests.
[M02] Passed withdrawal request might not be withdrawable
requestWithdrawal function of the
PricelessPositionManager contract allows sponsors to submit a withdrawal request for a given amount of collateral. A request is considered passed once a certain period of time elapses, and the function ensures that the time at which a request is expected to pass is equal or lower than the contract’s expiration time.
In the corner case where the request’s pass time is equal to the contract’s expiry time, the request will be accepted, even though it will not be possible to actually execute it. This is due to the fact that passed requests are executed by the sponsor calling the
withdrawPassedRequest function, which can only be called before the contract’s expiration time (as it is marked with the
To avoid unexpected behaviors during withdrawal of collateral, consider modifying the
requestWithdrawal function to only accept requests whose pass time will be strictly before the contract’s expiration time.
Update: Fixed in PR#1386. A comment from the UMA team worth highlighting:
It should be noted, however, that the funds aren’t locked and can be withdrawn at final settlement. The contract can also undergo emergency shutdown, which will result in pending/passed withdrawal requests being locked until settlement anyway. We think this change makes sense to make the inequalities consistent, but being able to withdraw after the liveness period is, nonetheless, not guaranteed.
[M03] Unregistered parties
Registry contract tracks the parties associated with each registered financial contract. It provides mechanisms for financial contracts to initialize, add, remove and query the party members.
ExpiringMultiParty contract does not inform the registry when its parties change. In fact, the
ExpiringMultiPartyCreator sets the initial party member to the address that triggers the deployment, whether or not that address is a party to the financial contract.
Consider updating the
Registry contract whenever the
ExpiringMultiParty contract’s participants change.
Update: Fixed in PR#1353. The participants are no longer tracked in the
Registry contract. Note that the party-related functions still exist in the
Registry, but they do not apply to
[M04] Possible mismatch between price identifier and collateral
ExpiringMultiPartyCreator contract allows the deployer to choose the price identifier and collateral token. If these are chosen to be inconsistent with each other, the price returned by the oracle will not match the expected token price, which could confuse users and lead to unexpected behavior. Consider maintaining an approved mapping between collateral tokens and price identifiers to enforce consistency between these values.
Update: Not an issue. There are use cases of the UMA contracts where the price identifier may not match the collateral token. In the words of the UMA team:
While many products will have matching price quote currencies and collateral tokens, we want to allow creating exotic / complex risk exposures if users prefer.
[L01] Violation of Checks-Effects-Interactions pattern
Solidity recommends the usage of the Checks-Effects-Interactions pattern to avoid potential security vulnerabilities, such as reentrancy. However, the
withdrawPassedRequest function of the
PricelessPositionManager contract executes an external call to do the token transfer before modifying the storage to reset the withdrawal request. If the call is made to a malicious contract (or an ERC777 that executes arbitrary receive hooks), this pattern could lead to a reentrancy scenario.
Even though the external call is expected to be to a trusted contract on the collateral token whitelist, consider always following the “Checks-Effects-Interactions” pattern.
Update: Fixed in PR#1307. Additionally, PR#1334 introduces reentrancy guards to all public and external functions.
[L02] Lack of input validation
- The constructor does not validate whether
_expirationTimestampis in the past.
depositfunction does not validate whether the
collateralAmountis non-zero. If it is zero, the function will emit the ERC20 events and the
requestWithdrawalfunction does not validate whether
collateralAmountexceeds the sponsor’s collateral. In such a scenario the request would be clearly illegitimate and could be reverted.
withdrawPassedRequestfunction does not validate whether calling sponsors have an active withdrawal request. If they do not, the function will emit the
payFeesfunction does not validate whether the
lastPaymentTimeis the current
time. In other words, it does not validate if a previous fee-paying transaction already occurred in the current block. If it did, the function will emit the
VALID_EXPIRATION_TIMESTAMPSarray only initializes 16 out of the 17 elements. This means that zero will be considered a valid expiration timestamp.
Consider implementing the necessary logic where appropriate to validate all relevant parameters.
Update: Fixed in PR#1316.
[L03] Negative prices are not supported
The UMA oracle reports prices as
int value, which may be negative. However, the
PricelessPositionManager contract does not support negative prices and simply replaces negative values with zero. This mismatch may lead to errors or unexpected behavior.
Consider modifying either the oracle or the financial contract so that they are compatible.
Update: This is the expected behavior. In the words of the UMA team:
We’ve decided not to support negative prices in this version of ExpiringMultiParty because the incentives break down when a token becomes a liability rather than an asset. However, we want to leave the DVM a bit more general to allow negative prices for other use cases outside of positively-valued synthetic token contracts. We set a negative price to 0 in this contract to prevent the contract from locking up any funds in the case that an error in parameterization or at the DVM level causes a negative price to be returned. We expect that any price identifier used by the ExpiringMultiParty will always return nonnegative price values from the DVM.
[L04] Unsafe addition
requestWithdrawal function of the
PricelessPositionManager contract does not check for overflows when computing the withdrawal request’s pass time. Consider using OpenZeppelin’s
SafeMath library for all integer calculations.
Update: Fixed in PR#1304.
[L05] Erroneous docstrings and comments
Several docstrings and comments throughout the code base were found to be erroneous. In particular:
- Line 209 of
FeePayer.solrepeatedly mentions “collateralToRemove” instead of “collateralToAdd”.
- Line 213 of
FeePayer.solshould say “collateral added” instead of “collateral removed”.
- Line 215 of
FeePayer.solreferences a non-existent
- Line 34 of
PricelessPositionManager.solshould mention the initial settlement as another state-changing event.
- Line 249 of
PricelessPositionManager.solshould clarify that the withdrawn amount might be reduced to the collateral left in the contract.
- Line 70 of
minSponsorTokensas a parameter for the
Liquidatableconstructor, but it is actually a parameter for the constructor of the
- Line 415 of
Liquidatable.solsays “the dispute failed”, even though there was no dispute.
Update: Fixed in PR#1352.
[L06] Missing error messages in require statements
There are several
require statements without error messages in the audited code base. For instance, only three
require statements include messages in the
Consider including specific and informative error messages in all
require statements to favor readability and ease debugging.
Update: Fixed in PR#1364.
Notes & Additional Information
[N01] Missing upper bound constraint during fee payment
payFees function of the
FeePayer contract does not compare the profit from corruption value (stored in the
_pfc local variable) to the total amount of fees paid before dividing them, as is done in the
_pfc be lower than
totalPaid (a local variable representing the total amount of fees being paid), the subsequent calculation of the
cumulativeFeeMultiplier value would underflow, reverting the transaction in the SafeMath
Following the “fail early and loudly” principle, consider explicitly restricting the upper bound of the
totalPaid local variable.
Update: Addressed in PR#1338. The UMA team decided to refactor the
payRegularFees function (originally called
payFees) to reduce or waive unaffordable fees rather than revert the transaction. This ensures funds will not be trapped in the contract, but the cumulative fee multiplier will be set to zero and the contract will no longer work as intended. We strongly suggest thoroughly testing this dangerous scenario, in particular ensuring that users can no longer deposit funds into the defunct contract to prevent accidental loss of funds. Additionally, we suggest explicitly checking for this condition, rather than relying on division-by-zero errors to prevent new deposits.
[N02] Inefficient timestamp validation
_isValidTimestamp function of the
ExpiringMultiPartyCreator contract iterates over a fixed-length array of known expiration timestamps to check whether the provided expiration timestamp is valid. However, this iteration might not be efficient in terms of gas costs. For a more efficient lookup of valid timestamps, consider replacing the
VALID_EXPIRATION_TIMESTAMPS array with a
bool mapping where accepted timestamps are set to
Update: Fixed in PR#1308. It should be noted that timestamps are defined using the
uint32 type, but then implicitly casted and stored as
[N03] Repeated function calls to read Profit from Corruption value
_payFinalFees function of the
FeePayer contract is unnecessarily calling the
pfc function twice. The first time, in line 134, the function is called and its return value stored in a local variable
_pfc. However, the function does not reuse the local variable and instead calls
pfc again in line 140.
To reduce gas costs during execution, consider reusing the
_pfc local variable instead of calling the
pfc function again.
Update: Fixed in PR#1339.
[N04] Undocumented prior approval of ERC20 tokens
There are functions that execute transfers of ERC20 tokens from the caller’s balance using the standard
transferFrom function. The caller, in these cases, must first approve the contract that is moving the funds. Neither the existing external documentation nor the functions’ docstrings explicitly state this requirement. In particular:
createLiquidationfunctions need the
msg.senderto first approve the contract to move an amount of collateral.
createLiquidationfunctions need the
msg.senderto first approve the contract to move an amount of synthetic tokens.
So as to clearly document their assumptions, consider expanding the functions’ docstrings stating that the caller must first approve enough tokens for the functions to work as expected.
Update: Fixed in PR#1319.
[N05] Unnecessary public getter for state variable
An inline comment in the
PricelessPositionManager contract states that the
rawTotalPositionCollateral state variable should not be used directly. However, the state variable’s visibility is currently
public, and Solidity will therefore create a public getter for it.
To avoid mismatches between code and documentation, consider restricting access by removing the
public keyword from the
rawTotalPositionCollateral state variable.
Update: The UMA team decided not to follow our suggestion to ease testing and inspection after deployment.
[N06] Lack of indexed parameters in events
CreatedExpiringMultiParty event defined in the
ExpiringMultiPartyCreator contract, and the
LiquidationWithdrawn event defined in the
Liquidatable contract, do not index any of their parameters. Consider indexing event parameters to avoid hindering the task of off-chain services searching and filtering for specific events.
Update: Fixed in PR#1317.
[N07] Function missing view modifier
For added readability, consider declaring as
_computeFinalFees function of the
FeePayer contract, since it does not modify the contract’s storage.
Update: Fixed in PR#1315.
[N08] Unused repeated function
PricelessPositionManager contract, the function
_getStoreAddress is not called nor used in any way.
Moreover, the contract itself inherits from the
FeePayer contract which already implements the
_getStore function with the same purpose.
To favor simplicity, consider removing the
_getStoreAddress function from the
Update: Fixed in PR#1303.
[N09] Code repetition
There are some examples of repeated code blocks performing conceptually atomic operations.
- Lines 114 to 115 and 140 to 141 are both reducing the
cumulativeFeeMultiplierby a given amount.
- Lines 278 to 279 and 292 to 293 are both resetting the withdrawal request.
- Lines 184 to 185 and 316+319 are ensuring individual and global consistency when increasing collateral balances.
- Lines 210+215, 269 to 270, 356+362 and 534+542 are ensuring individual and global consistency when decreasing collateral balances.
To favor reusability, consider refactoring these repeated operations into private functions.
Update: Fixed in PR#1300.
[N10] Inconsistent NatSpec usage
The code base uses the Ethereum Natural Specification (NatSpec) format inconsistently.
For example, in the FeePayer contract, the function
_getCollateral is using single line comments format, other functions like
_getStore have no comments at all, while others such as
_payFinalFees are following the NatSpec format correctly.
So as to favor consistency, readability and improve the quality of the project’s documentation, consider always following the Ethereum Natural Specification format.
Update: Fixed in PR#1345. The UMA team adopted a style guide where the NatSpec format is reserved exclusively for public and external functions. Single line comments are optionally used for the remaining functions.
[N11] Missing docstrings in sensitive functions
Some internal and private functions implementing sensitive functionality are not commented. See for example
_deleteSponsorPosition functions in the
PricelessPositionManager contract. The lack of documentation might hinder reviewers’ understanding of the code’s intention, which is fundamental to assess not only security, but also correctness. Additionally, docstrings improve readability and ease maintenance.
Consider thoroughly documenting all non-trivial functions, even if they are not part of the contracts’ public API. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Fixed in PR#1343.
[N12] Naming issues
The following functions and variables may benefit from better naming:
FeePayershould be renamed to indicate fee-adjustment operations.
payFeesfunction of the
FeePayercontract should be renamed to
PricelessPositionManagershould be renamed to indicate the possibility of augmenting an existing position.
transferPositionOwnershipor similar. The
Transferevent should be renamed accordingly.
Update: Fixed in PR#1341. The
create function has not been renamed but its docstrings have been improved to better reflect the function’s behavior.
[N13] TODOs in code
There are “TODO” comments in the code base that should be removed and instead tracked in the project’s backlog of issues. See for examples:
Update: Fixed in PR#1327.
[N14] Typographical errors
- Line 148: “constrainments” should be “constraints”.
- Line 30: there is an extra space before “Finder”.
- Line 82: “more” is misspelled.
- Line 195: “cumulativeFeeMultiplier” is misspelled.
- Line 210: “cumulativeFeeMultiplier” is misspelled.
- Line 95: “multiplier” is misspelled.
- Line 98: “multiplier” is misspelled.
- Line 126: “DisputeSucceeded” should be “disputeSucceeded”.
- Line 147: there is an unmatched backtick symbol.
- Line 185: “tokens” is misspelled.
- Line 288: “whose” is misspelled.
- Line 370: the word “is” is missing.
- Line 371: “from times” should be “times from”.
- Line 39: “is” should be “as”.
- Line 66: “a” should be “an”.
- Line 196: “requestWithdrawal” is misspelled.
- Line 298: “the” should be removed.
- Line 322: “mint the caller” should be “mint to the caller”.
- Line 377: “Burns” should be “burns”.
- Line 8: there is an extra space before “who”.
- Line 12: it should say “return it to the caller”.
- Line 13: “adding new roles” should be “assigning the roles”.
- Line 16: “tokens” should be “token’s”.
Update: Fixed in PR#1298.
[N15] Unnecessary imports
Liquidatable contracts, consider removing the imports statement for the
Testable contract, as this contract is never used in any of them. Similarly, consider removing the unused import for the
FixedPoint library in the
Update: Fixed in PR#1299 and PR#1236.
Originally, no critical and 2 high severity issues were found. Some changes were proposed to follow best practices and reduce potential attack surface. We later reviewed all fixes applied by the UMA team and all the most relevant issues have been already fixed.