UMA is a platform that allows users to enter trust-minimized financial contracts on the Ethereum blockchain. One component of the system is a decentralized oracle, which was the subject of this audit.
The audited commit is
9d403ddb5f2f07194daefe7da51e0e0a6306f2c4 and the scope includes all production contracts in the following directories:
The following directories were not included:
Additionally, it should be noted that the oracle 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. Lastly, the oracle includes a mechanism for voters to store encrypted votes on the Ethereum blockchain. The encryption and decryption procedure occurs client-side and was not part of the audit.
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 highlighted three additional non-severe issues they had identified in their code base. These were addressed in PR#1246, PR#1267 and PR#1262.
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 happy with the security posture of the team and the health of the code base. We are pleased to see the use of mostly small, encapsulated functions and well-documented contracts. We also appreciate that the UMA team has considered the various privileged roles and their security implications.
We must highlight the UMA team’s outstanding software development practices in terms of mandatory code reviews, documentation and testing. Their developers hold insightful discussions in the open, which greatly improves the project’s transparency and allows their community to witness and get involved in the development process of the protocol. Most, if not all, reviewed fixes were done in encapsulated pull requests, always accompanied by relevant unit tests to confirm behavior where appropriate.
Finally, we applaud the team’s effort to have a public bug bounty program.
Users of the UMA system can enter into financial contracts that distribute funds according to the future price of an asset. Whenever the contract requires the asset price at a given timestamp (for instance, to ensure both parties are still correctly collateralized or to close a disputed contract at expiration), it will send a price request to the UMA oracle. All financial contracts pay a fee to use the oracle.
The UMA oracle layers economic incentives over decentralized data aggregation in order to achieve consensus on the price. Price requests are batched in sequential rounds. In each round, the price of all outstanding requests are voted on using a commit-reveal scheme. In order to participate, voters must obtain UMA voting tokens (an ERC20), and their vote is weighted by their balance. If a minimum threshold of voters (by token count) is reached and more than half of them vote for the same value, this value is the resolution of the price request. If neither condition is met, the price remains unresolved until the next round, where it is voted on again.
All successful voters are rewarded, per price request, with freshly minted tokens in proportion to their stake. Non-voters or incorrect voters are implicitly punished through inflation of the token supply.
As described in the whitepaper, the price of the voting token itself is supported by a buyback mechanism, where the oracle fees are used to purchase UMA tokens on the open market whenever the value of corrupting the oracle is greater than the cost of obtaining majority voting power. This should undermine the incentive to corrupt the oracle.
There are a number of privileged roles that exercise wide-ranging powers over the protocol.
Most importantly, the Risk Labs Foundation controls the wallet that can withdraw oracle fees. They intend to use these fees for the buyback mechanism but this is not currently programmatically enforced. The intention is to encapsulate the buyback logic in a smart contract but until then, token holders must trust the Risk Labs Foundation to perform this function quickly and correctly.
Additionally, there is a
Governor contract that manages the system in a number of ways:
- It can shutdown or remargin any contract within the system. This is intended as an emergency safety mechanism.
- It can replace the implementation of crucial contracts within the system.
- It can decide which prices are supported by the oracle.
- It can decide which addresses can register new financial contracts (implicitly deciding which financial contracts are supported).
- It can set the oracle fees.
- It can replace the address that can withdraw oracle fees.
- It can set the inflation rate per vote.
- It can set the quorum threshold.
- It can set the rewards expiration timeout.
- It can migrate the whole voting contract to another implementation.
Any action that can be undertaken by the
Governor contract must first be proposed by the Risk Labs Foundation and then ratified by the token holders using the oracle voting mechanism. This has two interesting implications:
- Token holders cannot use the smart contract to initiate an action of their own accord, but they have veto power over any action. The Risk Labs Foundation intends to solicit proposals using an external mechanism, and advance those that have greater than 5% token holder support.
- Since the
Governorcontract relies on the correct functioning of the UMA oracle, it may not be able to correct a problem with the oracle itself.
As the ecosystem becomes more interconnected, understanding the external dependencies and assumptions have become an increasingly crucial component of system security. To this end, we would like to discuss how the UMA oracle depends on the surrounding ecosystem.
Fortunately, this is quite straightforward since the oracle does not interact with external projects. However, it does use time-based logic, which means it is dependent on the availability of Ethereum in multiple ways:
- After a price is resolved, voters have a time limit to collect their rewards. If they are temporarily unable to submit this transaction (for instance, during a period of high Ethereum congestion), the rewards will be lost.
- Similarly, if a voter is unable to commit or reveal within a particular round’s time window, those votes will not be counted.
- Votes that were not included in the round they were made may end up being confirmed in subsequent rounds. In such cases, the voter would typically choose to do that anyway.
[H01] Votes can be duplicated
The Data Verification Mechanism uses a commit-reveal scheme to hide votes during the voting period. The intention is to prevent voters from simply voting with the majority. However, the current design allows voters to blindly copy each other’s submissions, which undermines this goal.
In particular, each commitment is a masked hash of the claimed price, but is not cryptographically tied to the voter. This means that anyone can copy the commitment of a target voter (for instance, someone with a large balance) and submit it as their own. When the target voter reveals their salt and price, the copycat can “reveal” the same values. Moreover, if another voter recognizes this has occurred during the commitment phase, they can also change their commitment to the same value, which may become an alternate Schelling point.
Consider including the voter address within the commitment to prevent votes from being duplicated. Additionally, as a matter of good practice, consider including the relevant timestamp, price identifier and round ID as well to limit the applicability (and reusability) of a commitment.
Update: Fixed in PR#1217. The hash now includes all relevant context to avoid duplication of votes and limit their applicability and reusability.
[H02] Governance actions that require ETH cannot be practically executed
Governor contract allows the account with the
Proposer role to propose new governance actions through the
propose function. Each proposal is made up of a number of actions, each action represented with the
Transaction struct. An action is essentially an external call from the
Governor contract to an address, passing arbitrary data and value.
Actions that require Ether for their execution (i.e., its
value field is greater than zero) may be difficult to execute and produce unexpected failures since there is no straightforward way to deposit ETH into the
Governor contract. None of the functions implemented by the contract (nor its ancestors) are marked as
payable. It must also be noted that while several scenarios are tested in the
Governor.js file, the case where a proposal involves a transfer of ETH is not covered.
Consider implementing the necessary functionality so that the
Governor contract can receive ETH before executing a proposal. This should allow actions that require Ether to be easily triggered from the contract, preventing unexpected behaviors.
Update: Fixed in PR#1191, with a caveat worth mentioning. ETH must now be deposited in the
Governor contract when each transaction within a proposal is executed (via the
executeProposal function) and not when it is proposed (via the
propose function). This means that an approved proposal that requires ETH might never be executed if no actor is willing to pay for it.
[H03] Any governance action could be executed multiple times
executeProposal function of the
Governor contract can execute a transaction listed in an approved proposal. A proposal can contain multiple transactions, each of them expected to be executed a single time in the order they appear listed in the
Once a transaction is executed successfully, the
executeProposal function deletes the transaction from the proposal so that it cannot be executed twice. However, the removal is done after the external call is made, which fails to follow the Checks-Effects-Interactions pattern. This can be leveraged by any contract called by the
Governor to execute a reentrancy attack, where the callee would reenter the
executeProposal function and trigger the same transaction multiple times.
It must be noted that while the reentrancy attack is indeed feasible, the likelihood of it actually occurring and having a negative impact in the UMA protocol is diminished by fact that:
- A proposal can only be proposed by the account with the trusted
Proposerrole, which will decide the transactions included in the proposal. It is unlikely that the
Proposerconstructs a malicious transaction that invokes a contract that does a reentrancy.
- Even if the
Proposerdoes include a malicious proposal, it would have to be voted by the Governance system before it can be executed. This process should involve reviewing and validating that the proposal does not contain unexpected, potentially malicious transactions that may harm the system.
To ensure a proposal’s transaction cannot be executed multiple times leveraging a reentrancy attack, consider modifying the
executeProposal function of the
Governor contract to tightly follow the Checks-Effects-Interactions pattern. In particular, the removal of the executed transaction must occur before the external call.
Update: Fixed in PR#1200. The
executeProposal function now correctly follows the suggested Checks-Effects-Interactions pattern.
[M01] Token holders can react to revealed vote
In each voting round the token balances are snapshotted during the first reveal transaction. If this transaction is front-run, it allows voters to buy or sell tokens based on the contents of the first revealed vote. They may use this to change the stake associated with their vote when they discover whether it matches another voter.
Consider introducing a dedicated transaction to snapshot token balances at the start of the reveal phase so that they can be locked before the first reveal.
Update: Fixed in PR#1238. The risk of the front-running described in the issue still exists, but a new function
snapshotCurrentRound has been added to give users an alternative way of freezing the relevant variables before the first reveal in an isolated transaction. The UMA team is aware of the tradeoff between security and user-experience for this particular case. In their words:
This is certainly possible, but unlikely due to the difficulty in doing it correctly and the minimal reward for doing so. For that reason, it seemed making the transaction optional was a reasonable compromise between UX and security. If this were to become a problem, bots could be created to call the snapshot transaction as early as possible in the reveal period.
[M02] Fragile implementation of conversion from integer to UTF-8 representation
_uintToBytes function of the
Governor contract intends to convert any given integer number into its UTF-8 bytes representation, returned as a
bytes32 type. Even though the function works as intended, we consider its implementation to be dangerously fragile for the following reasons:
- Misleading function name: the function does not return a
bytestype, but rather a
- Unclear intention: the fact that the function attempts to build a UTF-8 representation of the passed integer is not clear at all without thorough manual inspection.
- Untested implementation: no unit tests were found for this function. Given the code was copied, and adapted, from an answer in an online forum, the lack of tests is particularly risky.
- Lack of documentation: the function operates at a byte level to reproduce the UTF-8 representation of an integer, but none of the low-level steps are explained.
- Hardcoded values: the function uses multiple hardcoded values without explaining what they represent.
- Failing silently: the function implicitly discards any digits after the 32nd, failing silently at representing large numbers and producing output collisions.
Consider refactoring, documenting and testing the
_uintToBytes function of the
Governor contract to address these shortcomings.
Update: Fixed in PR#1204.
[M03] Discrepancies with the whitepaper
There are some discrepancies between the code and the whitepaper. Some (and possibly all) of these are known and acknowledged by the UMA team but we list them for completeness.
- The whitepaper claims that price requests must be for a datetime within the last two voting periods. In fact, they are restricted to any time in the past.
- The whitepaper claims the balances are snapshotted at the beginning of a commitment voting period. In fact, they are snapshotted when the first voter reveals their hash.
- The whitepaper claims that when more than 50% of voters cannot agree, the median is returned as the result. In fact, the vote is simply delayed until the next round.
- The whitepaper claims that accurate voters increase their token ownership by a fixed factor
revery voting period. In fact, the increase occurs for every vote.
- The whitepaper describes a mechanism to prevent parasitic contracts from using the oracle without reporting the potential profit from corruption or paying the fees. This mechanism is not in use. The whitepaper does not actually claim that the UMA DVM uses this mechanism but we believe that this fact is worth clarifying. It should also be noted that the system currently has an emergency shutdown mechanism that could be used to nullify any registered contract (and implicitly, the parasitic contracts that depend on its price requests). However, this mechanism does not distinguish between registered contracts that are intentionally created to be parasitized, and legitimate contracts that use popular price feeds, and are subsequently unknowingly parasitized by other contracts. In the latter case, the emergency shutdown mechanism may not be appropriate.
Update: All points acknowledged. Regarding points (1), (2) and (4), the whitepaper will be updated to reflect the implementation. Point (3) will be implemented in the future, and the whitepaper will be updated to clarify. As of point (5), UMA does not currently intend to implement this mechanism, and the whitepaper will be updated to make it explicit.
[M04] Financial contract registration with repeated parties might lead to inconsistent behaviors
registerContract function of the
Registry contract takes an array
parties containing a list of addresses. However, the function never validates whether the passed array contains the same party address multiple times. This means that the
for loop in lines 96 to 100 can register the same contract address multiple times for a party in its corresponding
partyMap[parties[i]].contracts array (see line 97), though only registering the index of the last added contract address in the
partyMap[parties[i]].contractIndex mapping (see line 99).
The described behavior can be leveraged to take the
Registry contract to an inconsistent state, where if multiple repeated parties appear in financial contracts, it may become impossible to fully remove a registered party. Refer to this unit test for a step-by-step reproduction of the issue. Note that in the showcased example, the
Register contract ends up in an inconsistent state where:
- When querying the registered contracts for a party, a contract
contract1appears as registered.
- When querying the
isPartyMemberOfContractfor the same party and contract
contract1, the party does not appear as a member of the contract.
- When trying to remove the party from the contract, the transaction is reverted, even if the party has that contract registered as shown in (1).
Financial contract registration in the
Registry is restricted to known accounts with the
ContractCreator role, so it would not be possible for everyone to leverage the described behavior maliciously. Nevertheless, the system’s behavior might be affected if repeated parties are registered accidentally.
Consider replacing the body of the loop with a call to the
addPartyToContract function. This would perform the consistency checks and avoid duplicating logic across the
addPartyToContract functions. It would also ensure the
PartyAdded event is emitted for the initial parties as well. Afterwards, related unit tests should be added to ensure the system behaves as intended.
Update: Fixed in PR#1194. An internal
_addPartyToContract function has been added, which is called when a contract is registered and when a new party is added to an existing contract. Related unit tests have been added as well.
[L01] Possibly miscalculated late penalty
computeRegularFee function of the
Store contract, the late penalty fee is calculated using the duration of time that the contract is paying for. It does not include any notion of the current time or the due date. This only makes sense if
startTime is the time that the fee started accruing and
endTime is the current time. If this is a requirement, it should be specified and enforced. Otherwise, contracts that use this function to calculate fees for different periods of time may be misinformed about the late penalty.
Depending on how the function is intended to be used, consider replacing the
endTime parameter with the current block time, or moving the late penalty fee calculation into its own separate function, and documenting the intended use either way.
Update: Fixed in PR#1237. The late penalty is now scaled by the difference between
startTime and the current time. Importantly, in the expected use case, it now scales quadratically with the number of unpaid weeks. The regular fee is unchanged.
[L02] Externally-owned accounts can be registered as contracts in Finder contract
changeImplementationAddress function of the
Finder contract is a privileged function (only called by the
owner address) in charge of registering contract addresses under a given interface name. However, while the function is only intended to register contract addresses, there is currently no validation on whether the address provided is actually a contract.
While this issue does not pose a security risk, consider using the
isContract function of OpenZeppelin’s
Address utility library to ensure the address being registered is indeed a contract.
Update: The issue is acknowledged, but the implementation will remain unchanged. Addressing the issue would introduce an unnecessary burden on existing token holders to migrate to a new contract.
[L03] Roles cannot be renounced
The role management scheme implemented in the
MultiRole contract does not include a way for accounts to renounce the roles they are granted. This might become problematic in a scenario where an account wishes to renounce a role after the trusted device holding the private keys has been compromised.
Consider allowing accounts to lose their granted privileges by renouncing any of their roles. For reference, follow the development of the
AccessControl contract in the OpenZeppelin Contracts library.
Update: Fixed in PR#1247. Users can renounce their shared roles. Users are intentionally prevented from renouncing exclusive roles as a safety mechanism.
[L04] Missing error messages in require statements
There are several
require statements without error messages. For examples, see line 29 in
Withdrawable.sol, and lines 56, 70, 71 and 125 in
Consider including specific and informative error messages in all
require statements to favor readability and ease debugging.
Update: Fixed in PR#1259. The
require statements now contain error messages.
[L05] Lookup key strings are not centrally defined
Known UMA contracts are tracked in the
interfacesImplemented mapping of the
Finder contract. New entries can be added by a privileged address via the
changeImplementationAddress function, and the
getImplementationAddress function acts as a public getter to query the registry providing a string-type key. While this registry is used by several different contracts to get the addresses of legitimate UMA contracts, the strings used as keys to query the registry are not centrally defined. The identified strings are:
This issue does not pose a security risk, but the approach taken is error-prone and difficult to maintain. Therefore, consider factoring out all mentioned constant strings to a single library, which can be then imported in the necessary contracts. This will ease maintenance and make the code more resilient to future changes.
Update: Fixed in PR#1241 and PR#1320. The code base uses the new
OracleInterfaces library to reference contracts within the system.
[L06] Use of arbitrary numbers as role identifiers is prone to error
MultiRole contract uniquely identifies registered roles using a
uint data type. Contracts inheriting from
MultiRole are then expected to define specific roles and associate them with a number. For example, the
ExpandedERC20 contract defines a
Roles enum where the
Owner element would be identified with 0,
Minter with 1 and
Burner with 2.
This approach might be error-prone when dealing with multiple contracts implementing different roles. For instance, the role allowed to withdraw from a
Withdrawable contract might also be identified with 0, thus semantically overloading the identifier
0, which would mean different things depending on which contract it references.
Consider uniquely labeling roles with identifiers that are more self-explanatory and less prone to confusion. In particular, consider using hashes of meaningful strings as role identifiers (e.g.,
keccak256("MINTER_ROLE")). For reference, follow the development of the
AccessControl contract in the OpenZeppelin Contracts library.
Update: Acknowledged, but the implementation will remain unchanged. Addressing this issue would necessitate a major change to the
[L07] Not failing loudly
removeSupportedIdentifier functions of the
IdentifierWhitelist contract do not revert when the expected state changes do not take place.
Following the “fail loudly” principle, consider modifying both functions so that transactions are reverted when an identifier cannot be effectively added / removed.
Update: Acknowledged, but the implementation will remain unchanged. The suggested mitigation introduces an additional risk, since the
Governor contract adds and removes a price identifier and an unexpected failure may disable it.
[L08] Lack of input validation
setWeeklyDelayFeefunction does not validate whether the
newWeeklyDelayFeeparameter is less than 1.
initfunction does not validate that the
phaseLengthparameter is greater than zero. This is important to avoid unexpected divisions by zero in other functions of the library.
addMemberfunction does not do a null address check like the
resetMemberfunction of the
Consider implementing the necessary logic where appropriate to validate all relevant user-controlled input.
Update: Fixed in PR#1212. The suggested validations are implemented.
[L09] Repeated reward events
retrieveRewards function, a
RewardRetrieved event with zero tokens is emitted when the reward is expired or incorrect. Additionally, after a correct vote is processed and cleared, if it is processed again it will fail the validity test and be treated as an incorrect vote. In any of these situations, the same vote can be processed multiple times, emitting a redundant
RewardRetrieved event every time.
Following the “fail early” principle, consider checking that the
revealHash is non-zero before processing a vote. This should help prevent emitting the
RewardRetrieved event unnecessarily.
Update: Fixed in PR#1240.
[L10] Transfer of ETH may unexpectedly fail
withdraw function of the
Withdrawable contract allows authorized callers to withdraw any amount of ETH from the contract. The transfer of Ether is executed with Solidity’s
transfer function, which forwards a limited amount of gas to the receiver. Should the receiver be a contract with a fallback function that needs more than 2300 units of gas to execute, the transfer of Ether would inevitably fail. After the Istanbul hard fork, it has become a recommended practice not to use
transfer to avoid hard dependencies on specific gas costs.
To avoid unexpected failures in the withdrawal of Ether, consider replacing
transfer with the
sendValue function available in the OpenZeppelin Contracts library.
Update: Fixed in PR#1225.
[L11] Ever-growing array of pending price requests
Voting contract tracks all price requests still to be resolved in its
pendingPriceRequests array. Once a price request is considered resolved, the
_resolvePriceRequest function attempts to remove the corresponding request from
pendingPriceRequests. This is done by first replacing the request with the last item in the array and then updating the associated
priceRequests mapping to keep indices updated and consistent. However, the function falls short at fully cleaning the
pendingPriceRequests array. In particular, while the elements are properly deleted, the array’s length is never decreased.
It must be noted that an ever-growing
pendingPriceRequests array does not cause any security issues. Yet the array will be unnecessarily filled with zero elements, which would have a negative impact on the
getPendingPriceRequests function’s gas efficiency.
Consider decreasing the length of the
pendingPriceRequests array every time an element is deleted. Furthermore, consider adding related unit tests to ensure this unexpected behavior is not reintroduced in future modification to the code base.
Update: Fixed in PR#1207.
[L12] Erroneous comments
- In line 38 of
Registry.sol, the comment should start with “The address of each financial contract is mapped to its index…”.
- In line 48 of
Registry.sol, “their associated FinancialContract struct” should say “their associated Party struct”.
- In line 76 of
Registry.sol, the comment is an incomplete thought.
- In line 127 of
Registry.sol, “to” should be “from”.
- In line 25 of
VoteTiming.sol, “floor(timestamp/phaseLength)” should say “floor(timestamp/roundLength)”.
- In line 22 of
DesignatedVoting.sol, an inline comment claims that the
Ownerrole can set the
Withdrawerrole, but actually the
Withdrawerroles are permanently the same role.
- In line 55 of
minVoteThresholdis described as an inclusive bound but it is actually an exclusive bound. Similarly, the comment in line 72 should say “exceeded” instead of “met”.
- In line 549 of
Voting.sol, the comment references a non-existent
- In line 71 of
Governor.sol, there is a redundant
- In line 154 of
Governor.sol, the comment states the proposal data will be zeroed after execution but the
Update: Partially fixed in PR#1213. The comment in line 127 of
Registry.sol has been fixed in PR#1206. The erroneous comment in line 25 of
VoteTiming.sol has not been addressed.
[L13] Nonexistent role can be set in Withdrawable contract
setWithdrawRole function of the
Withdrawable contract can be used by derived contracts to set the role identifier allowed to withdraw Ether or tokens. However, the function does not validate whether the passed
roleId argument is already registered. While the function’s docstrings explicitly state that the role must exist, this should be checked programmatically to avoid errors.
Consider using the available
onlyValidRole modifier to effectively validate that the
roleId argument exists.
Update: Fixed in PR#1226.
[L14] Lack of event emission after fee update
setFinalFee functions of the
Store contract do not emit an event after updating the weekly delay and final fees. Given the important role fees play in the UMA system, consider logging an event after potentially sensitive modifications are applied.
Update: Fixed in PR#1214.
[L15] Actions in proposals might fail silently when target is an externally-owned account
executeProposal function of the
Governor contract internally calls the private
_executeCall function to execute the external call involved in a proposal’s action. In turn,
_executeCall uses the low-level
call function to pass arbitrary data to the target address. However, the function never validates whether the address being called is actually a contract.
The intended use case of the
Governor contract is to manage the UMA system contracts so proposals will typically include transactions to those contracts, and will be vetted by the UMA DVM. Nevertheless, any assumptions should be represented by explicit constraints in the code. If the transaction contains data but the target is an externally owned account, the external call will still be considered successful (i.e.,
_executeCall will return
true) even though no code was executed.
Consider validating proposals during registration to ensure that, if any data is included in an action, the target address is indeed a deployed contract.
Update: Fixed in PR#1242. Proposals are rejected if they include transactions that would send data to an externally owned account.
[L16] Unsafe addition in reward’s expiration time calculation
Voting contract calculates the reward’s expiration time using an unsafe addition. In practice, this is unlikely to overflow. Nevertheless, consider using OpenZeppelin’s
SafeMath library for all integer calculations.
Update: Fixed in PR#1235.
[L17] Immutable designated voting association
DesignatedVotingFactory contract is designed to let a voting address (typically a hot wallet) register an associated
DesignatedVoting contract that lets them vote on behalf of an owner (typically a cold wallet). However, once it is set, there is no mechanism to remove or replace the association within the
While this does not pose a security issue, it does prevent the mapping from tracking changes to the
DesignatedVoting contract, which may degrade user experience.
Consider including a
clearDesignatedVoting function to remove the association.
Update: UMA acknowledges this issue, but the implementation will remain unchanged. According to UMA, addressing it would introduce an unnecessary burden on existing token holders to migrate to a new contract.
[L18] Using unstable version of OpenZeppelin Contracts may break functionality
The OpenZeppelin Contracts library is imported as
"@openzeppelin/contracts": "^3.0.0-beta.0". This is an unstable, pre-production version of the library, which may include unexpected breaking changes between releases until it is stabilized. For example, since the dependency is not pinned, the latest
3.0.0-rc.0 version is now installed instead of the
3.0.0-beta.0 version. This first release candidate includes breaking changes from the previous beta version, such as contracts moved to other folders (e.g.,
ERC20Snapshot), or functions changing names and visibility. Some of these changes have broken imports in the UMA project and contracts now fail to compile correctly. Furthermore, the previously public
snapshot function of the
ERC20Snapshot contract has been restricted to
internal, and its name has changed to
Initially, consider pinning the imported dependency to a fixed version to avoid installing undesired versions that may break functionality. More importantly, before moving into production, consider waiting for the final stable release of OpenZeppelin Contracts 3.0.
Update: Fixed in PR#1254 and PR#1280. The project is now using OpenZeppelin Contracts v3.0.0.
[L19] Redundant and fragile ERC20 transfer
payOracleFeesErc20 function of the
Store contract allows the caller to deposit ERC20 tokens into the
Store contract. As the transfer of tokens is executed via the ERC20
transferFrom function, the caller must have previously approved the tokens to the
Store contract. The
payOracleFeesErc20 function will attempt to transfer all approved tokens from the caller to the contract, so the amount to be transferred is programmatically determined by querying the ERC20
This implementation can be considered suboptimal for a few reasons. Firstly, the approve-then-pull pattern is typically used as a workaround so that contracts can respond to receiving tokens. Since the
payOracleFeesErc20 function does not emit any events or perform any internal actions, the workaround is unnecessary. Secondly, the allowance is not typically considered to be equivalent to a payment. It is common practice to assign a high allowance and allow contracts to retrieve the required amount when necessary. Lastly, there are some ERC20 tokens that use an allowance of
2 ** 256 - 1 as a flag to indicate “infinite” allowance. Two well-known examples include the DAI token and Compound Finance’s CTokens. In such a case,
payOracleFeesErc20 will be unable to retrieve tokens when granted an “infinite” allowance.
Consider removing the
payOracleFeesErc20 function in favor of direct ERC20 transfers. Alternatively, consider letting the caller determine how many tokens are to be transferred from their accounts to the
Update: Fixed in PR#1255. The
payOracleFeesErc20 function explicitly accepts the amount to transfer.
[L20] Tests not passing
The testing suite finishes with 4 failing tests. These tests are:
1) Contract: scripts/Voting.js Intrinio price: AssertionError: expected 0 to equal 1 + expected - actual -0 +1 at Context. (test/scripts/Voting.js:248:12) at processTicksAndRejections (internal/process/next_tick.js:81:5) 2) Contract: scripts/Voting.js Constant price: AssertionError: expected 1 to equal 0 + expected - actual -1 +0 at Context. (test/scripts/Voting.js:316:12) at processTicksAndRejections (internal/process/next_tick.js:81:5) 3) Contract: scripts/Voting.js Numerator/Denominator: AssertionError: expected 2 to equal 1 + expected - actual -2 +1 at Context. (test/scripts/Voting.js:352:12) at processTicksAndRejections (internal/process/next_tick.js:81:5) 4) Contract: scripts/Voting.js Only batches up to the maximum number of commits or reveals that can fit in one block: There should be 0 pending requests during pre-commit phase + expected - actual -3 +0 at Context. (test/scripts/Voting.js:380:12) at processTicksAndRejections (internal/process/next_tick.js:81:5)
As the test suite was left outside of the audit’s scope, please consider thoroughly reviewing the test suite to make sure all tests run successfully. Furthermore, it is advisable to only merge code that neither breaks the existing tests nor decreases coverage. Lastly, consider adding a command to the
package.json file for running tests.
Update: Not an issue. As UMA correctly pointed out, these tests do pass if the environment variables are set correctly.
[L21] Unvetted function call
withdrawErc20 function in the
Withdrawable contract and the
payOracleFeesErc20 function in the
Store contract both call an unvetted function (
transferFrom respectively) on a user-specified address. If the address is chosen maliciously, this could result in arbitrary code being executed within another contract on behalf of the the UMA contract. This does not directly affect the UMA system but it does introduce an unexpected behavior. To improve predictability, consider whitelisting or vetting the contracts that can be passed to the
Update: This issue is acknowledged because, according to UMA, the suggested mitigation would introduce too much complexity for minimal gain.
[L22] Use of uninitialized state variables in fee calculation
computeRegularFee function of the
Store contract is in charge of computing the regular oracle fees that a contract should pay, given a period of time in seconds and the profit from corruption. The calculation depends on two state variables
weeklyDelayFee, which should be initialized by the owner of the
Store contract calling the
setWeeklyDelayFee functions respectively. However, these two state variables are not initialized upon construction of the
Store contract, and the
computeRegularFee function does not validate whether they are already initialized. As a result, all regular fees calculated before the owner sets the
weeklyDelayFee state variables will inevitably be zero.
To avoid unexpected behaviors, consider initializing the
weeklyDelayFee state variables during construction of the
Update: Fixed in PR#1256. The variables are set in the constructor of the
Notes & Additional Information
[N01] Unexpected initial round
As explained in the comment for the
computeCurrentRoundId function of the
VoteTiming library, the round ID depends on the global timestamp but not on the lifetime of the system. Although this is a reasonable choice to simplify the implementation, it has the surprising consequence that the initial round ID starts at an arbitrary number (that increments, as expected, for subsequent rounds) instead of zero or one. To favor readability, consider stating this behavior explicitly in the function’s comments.
Update: Fixed in PR#1272.
[N02] 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 example line 65 of
ResultComputation.sol, line 7 of
ContractCreator.sol or line 194 of
Update: Fixed in PR#1250.
[N03] Use of uint type
Several variables are declared as
uint throughout the code base. To favor explicitness, consider changing all instances of
Update: Fixed in PR#1230.
[N04] Typographical errors
Line 30 should say “will succeed” instead of “will be succeed”.
Lines 31, 45 and
62 have the same unclear parameter description.
Line 12 should say “will succeed” instead of “will be succeed”.
- Line 189 should say “an exclusive” instead of “a exclusive”.
Line 190 should say “initialMember” instead of “initialMembers”.
- Lines 13, 21 and 30 contain the extra word “of”.
Line 31 misspells “identifier”.
Line 127 should say “from the calling contract” instead of “to the calling contract”.
- Line 55 has the extra word “been”.
Line 95 has the extra word “correctly”.
- Line 215 has an extra word “of”.
- Line 279 says “time” instead of “type”.
- Line 279 should say “an identifier” instead of “a identifier”.
- Line 280 misspells the word “list”.
- Lines 314 and 346 have the extra word “is”.
- Line 396 references
EncryptedSender.solbut there is no such file in the repository.
- Lines 605 and 614 should say “round’s” instead of “rounds”.
Line 711 misspells the word “snapshotted”.
- Line 46 has the extra word “is”.
- Line 47 describes the salt parameter in a confusing way.
- Line 56 should say “array of structs” instead of “struct”.
- Line 106 describes a named return parameter (
totalRewardToIssue) but the name is not part of the function signature.
Update: Fixed in PR#1206.
[N05] Inconsistent use of SafeERC20 contract
transferFrom operations executed in line 29 of
Withdrawable.sol and line 71 of
Store.sol respectively are correctly wrapped in
require statements to handle ERC20 contracts that return
false upon failure. Yet, in other parts of the code base (out of this audit’s scope), it was noticed that OpenZeppelin’s
SafeERC20 library is used for ERC20 operations.
To be consistent throughout the code base and avoid confusion, consider modifying the
Store contracts to use the
Update: Fixed in PR#1205.
[N06] Inconsistencies in coding style
Minor deviations from the Solidity Style Guide were found. For example, internal functions in lines 37 and 47 of
Withdrawable.sol should start with an underscore. Additionally, while some functions parameters start with an underscore, others do not.
Taking into consideration how much value a consistent coding style adds to the project’s readability, enforcing a standard coding style with help of linter tools such as Solhint is recommended.
Update: Fixed in PR#1271. The related
STYLE.md file has been updated accordingly in PR#1286.
[N07] Naming issues
resetMemberfunction should be renamed to
getMemberfunction should be renamed to
addMemberfunction should be renamed to
removeMemberfunction should be renamed to
ownerparameter of the
createWithdrawRolefunction should be renamed to
computeFinalFeefunction should be renamed as it does not actually compute anything. If keeping the name for consistency, this should be explicitly stated in docstrings to avoid confusions.
weeklyDelayFeevariables should be renamed to
- The contract name
EncryptedStoreis misleading, as it does not actually enforce any kind of encryption. It should be renamed to
_uintToBytesfunction should be renamed to
getPendingRequestsfunction should be renamed to
getActiveRequestsor similar, since it only returns active price requests. In particular, it does not return requests scheduled to be resolved in a future round.
divRawvariable should be renamed to
aScaledor similar, since it has nothing to do with division.
Update: Fixed in PR#1227, PR#1231 and PR#1204. The
MultiRole library will remain unchanged, as well as the
getPendingRequests function of the
[N08] Potentially differing time sources in testing environment
Testable contract is a base contract from which all contracts that need to mock time changes in testnets should inherit. If several contracts of the UMA protocol derive from the
Testable contract, then for each change in time all contracts must be updated at the same timestamp to maintain a consistent time across the system. Otherwise there might exist multiple time sources with differing times, potentially causing unexpected failures during testing. Consider implementing a pattern that keeps a single source of time in testing environments.
Update: Fixed in PR#1236. All contracts use a shared
Timer contract during testing to obtain the mocked time.
[N09] Named return variables
Named return variables are used inconsistently throughout the code base. For example, the
computeRoundEndTime functions of the
VoteTiming library have named return variables in the function signature but return their results directly.
Consider removing all named return variables, explicitly declaring them as local variables, and adding the necessary
return statements where appropriate. This should improve both explicitness and readability of the project.
Update: Fixed in PR#1229. Named returns are only consistently used in functions that return multiple values.
[N10] Consider warning voters about salt reuse
When voters reveal their previously committed vote using the
revealVote function of the
Voting contract, they must provide the salt used to mask the vote hash. Since transaction data is public, the salt will be revealed with the vote. While this is the system’s expected behavior, it assumes that voters will never reuse the same pair of salt and price. Otherwise their commits could be easily disclosed in advance.
Since this is a common mistake, consider adding user-friendly documentation, both in docstrings and external documentation, stating the risks of reusing salts in the commit and reveal voting scheme.
Update: Fixed in PR#1273.
[N11] Hardcoded condition in require statement
To favor simplicity, consider replacing the
require statement in line 114 of
MultiRole.sol with a
Update: Fixed in PR#1228.
[N12] Inconsistent NatSpec usage
The code base uses the Ethereum Natural Specification (NatSpec) format inconsistently. In particular the
@return tags are often missing. Consider adding the missing tags to all contracts and functions.
Update: Fixed in PR#1270.
[N13] Redundant inheritance from MultiRole contract
Store inherit from both
Withdrawable contracts. However,
Withdrawable already inherits from
MultiRole. To favor simplicity and avoid confusion, consider removing the redundant inheritance from
MultiRole in the
Update: Fixed in PR#1203. It should be noted that the related import statements were not removed.
[N14] Untested, undocumented behavior of late penalty fee
computeRegularFee function of the
Store contract computes a penalty fee that is to be paid if the regular fee is overdue. The penalty fee is expected to be computed per overdue week and currently is always floored. For example, if the regular fee is more than a week overdue but less than two, the penalty fee will be computed as if a single week had passed. While this appears to be the system’s expected behavior, it was found to be undocumented and barely tested, which may cause confusions in developers, auditors and users alike.
Consider explicitly documenting the behavior of the penalty fee and adding relevant unit tests to ensure the system behaves as intended.
Update: Fixed in PR#1251.
[N15] Unused encrypted vote
When a voter commits to a price, they can optionally include an encrypted version of their vote to be stored in the
Voting contract. This value is never decrypted, verified against their hash or processed in any way within the EVM. Instead, it simply uses the EVM as a temporary storage location so that voters do not need to save their vote contents between the commit and reveal phase. Consider emitting an event with the encrypted vote instead, which would likely be more gas efficient than modifying EVM storage.
Update: Fixed in PR#1231. The
EncryptedStore contract has been removed, and encrypted votes are logged using the
EncryptedVote event of the
[N16] Base contracts not marked as abstract
All base contracts that are not intended to be instantiated directly, such as
ContractCreator should be marked as
abstract to favor readability and avoid unintended usage.
Update: Fixed in PR#1201.
[N17] Implicitly merged withdraw roles
withdrawErc20 functions of the
Withdrawable contract, the caller is also the recipient of the funds. For increased flexibility, consider allowing the withdrawer to specify the recipient address.
Update: The UMA team has decided not to move forward with our recommendation.
Originally, no critical and three 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.