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:
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:
None.
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.
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:
Consider including a minimum collateral-to-token ratio to prevent liquidators from accidentally liquidating an insolvent position.
Update: Fixed in PR#1351.
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.
The 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 onlyPreExpiration modifier).
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.
The 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.
However, the 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 ExpiringMultiParty contracts.
The 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.
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.
In the PricelessPositionManager contract:
_expirationTimestamp is in the past.deposit function does not validate whether the collateralAmount is non-zero. If it is zero, the function will emit the ERC20 events and the Deposit event unnecessarily.requestWithdrawal function does not validate whether collateralAmount exceeds the sponsor’s collateral. In such a scenario the request would be clearly illegitimate and could be reverted.withdrawPassedRequest function does not validate whether calling sponsors have an active withdrawal request. If they do not, the function will emit the RequestWithdrawalExecuted event unnecessarily.In the FeePayer contract:
payFees function does not validate whether the lastPaymentTime is 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 RegularFeesPaid event unnecessarily.In the ExpiringMultiPartyCreator contract:
VALID_EXPIRATION_TIMESTAMPS array 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.
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.
The 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.
Several docstrings and comments throughout the code base were found to be erroneous. In particular:
FeePayer.sol repeatedly mentions “collateralToRemove” instead of “collateralToAdd”.FeePayer.sol should say “collateral added” instead of “collateral removed”.FeePayer.sol references a non-existent totalPositionCollateral variable.PricelessPositionManager.sol should mention the initial settlement as another state-changing event.PricelessPositionManager.sol should clarify that the withdrawn amount might be reduced to the collateral left in the contract.Liquidatable.sol identifies minSponsorTokens as a parameter for the Liquidatable constructor, but it is actually a parameter for the constructor of the PricelessPositionManager contract.Liquidatable.sol says “the dispute failed”, even though there was no dispute.Update: Fixed in PR#1352.
There are several require statements without error messages in the audited code base. For instance, only three require statements include messages in the PricelessPositionManager contract.
Consider including specific and informative error messages in all require statements to favor readability and ease debugging.
Update: Fixed in PR#1364.
The 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 _payFinalFees function.
Should _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 sub operation.
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.
The _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 uint256 to bool mapping where accepted timestamps are set to true.
Update: Fixed in PR#1308. It should be noted that timestamps are defined using the uint32 type, but then implicitly casted and stored as uint256.
The _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.
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:
deposit, create, dispute and createLiquidation functions need the msg.sender to first approve the contract to move an amount of collateral.redeem, settleExpired and createLiquidation functions need the msg.sender to 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.
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.
The 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.
For added readability, consider declaring as view the _computeFinalFees function of the FeePayer contract, since it does not modify the contract’s storage.
Update: Fixed in PR#1315.
In the 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 PricelessPositionManager contract.
Update: Fixed in PR#1303.
There are some examples of repeated code blocks performing conceptually atomic operations.
In FeePayer.sol:
cumulativeFeeMultiplier by a given amount.In PricelessPositionManager.sol:
To favor reusability, consider refactoring these repeated operations into private functions.
Update: Fixed in PR#1300.
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.
Some internal and private functions implementing sensitive functionality are not commented. See for example _reduceSponsorPosition and _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.
The following functions and variables may benefit from better naming:
_getCollateral and _convertCollateral functions in FeePayer should be renamed to indicate fee-adjustment operations.payFees function of the FeePayer contract should be renamed to payRegularFees, payOngoingFees, payRegularAndPenaltyFees or similar.priceIdentifer variable in PricelessPositionManager should be priceIdentifier.EndedSponsor event in PricelessPositionManager should be EndedSponsorship or EndedSponsorPosition.create function of PricelessPositionManager should be renamed to indicate the possibility of augmenting an existing position.transfer function of PricelessPositionManager should be transferPositionOwnership or similar. The Transfer event 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.
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.
ExpiringMultiPartyCreator.sol:FeePayer.sol:Liquidatable.sol:PricelessPositionManager.sol:SyntheticToken.sol:TokenFactory.sol:Update: Fixed in PR#1298.
In the PricelessPositionManager and 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 ExpiringMultiParty contract.
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.