September 22, 2022
This security assessment was prepared by OpenZeppelin, protecting the open economy.
We audited the following set of pull requests that introduce improvements and bug fixes to the UMAprotocol/protocol repository:
OptimisticGovernor contract from being used as a mastercopy for proxy contracts.VotingV2 contract that allows delegates to claim rewards and restake tokens on behalf of the voter that owns the delegated account.DesignatedVotingV2 contract by allowing the voter to be their own delegate.VotingV2 contract size back under the bytecode limit for deployment.VotingV2 back to the previous Voting contract in cases where VotingV2 does not have the price information.VotingV2 contract to forward the retrieval of rewards to the previous Voting contract, in order to allow users to claim their rewards post migration to V2.VotingV2 contract. The VotingUpgraderV2 contract ensures the atomicity of the upgrade process.The primary focus of these modifications is on the UMA Data Verification Mechanism (DVM) 2.0 system. PR #4135 is notable because it reverts some suggested changes from the prior UMA DVM 2.0 audit in order to reduce the VotingV2 contract size.
Review of changes to the following contracts was in-scope:
OptimisticGovernor.solStaker.solVotingV2.solDesignatedVotingV2.solStakerInterface.solEmergencyProposer.solGovernorV2.solEmergencyProposerTest.solStaker.solVotingV2.solVotingV2Test.solMinimumVotingAncillaryInterface.solVotingV2Interface.solVotingV2.solVotingV2Test.solVotingV2.solVotingUpgraderV2.solHere we present our findings.
setQuorum and setMinimumWaitTimeThe EmergencyProposer contract is intended to function as an emergency recovery mechanism by allowing any user to construct a transaction that can bypass the VotingV2 contract. To submit a proposal, a user needs to post a large bond which is defined by the quorum state variable. The user then needs to wait until the minimumWaitTime has passed. However, there is a lack of validation when setting quorum and minimumWaitTime, which are crucial for correct functioning of the contract.
The setQuorum function does not validate the size of the bond relative to the supply of the token used for the quorum value. In the case that the quorum is mistakenly set to be larger than the total token supply, no emergency proposal could be created.
The setMinimumWaitTime function does not validate that the newMinimumWaitTime is within a reasonable time frame. In the case that minimumWaitTime is mistakenly set to be a large interval consisting of multiple months or even years, no emergency proposal would pass until the time frame could expire.
If either of these two variables are set incorrectly, the EmergencyProposer contract would be rendered useless, and would need to be fixed via the voting process defined in the VotingV2 contract. This is counter to the goal of the EmergencyProposer contract, which is supposed to be able to bypass the VotingV2 contract in case it was to break.
Consider adding validations on setMinimumWaitTime and setQuorum. The newMinimumWaitTime time should be capped to a reasonable time frame, aligned with the desired time requirements for passing emergency proposals. For setQuorum we suggest that at a minimum a check should be added to ensure the newQuorum value is not larger than the token supply. Also consider further limiting the quorum amount to not exceed a certain fraction of the total token supply beyond which it would be unrealistic for a user or group of users to control.
Update: Fixed as of commit 9000d0aa644f3340deeb4eaa704d7432a7e55dc9 in PR #4153. newQuorum is now checked to ensure it is smaller than the total supply of tokens, and newMinimumWaitTime is checked to ensure it is not longer than 4 weeks.
The EmergencyProposal contract is used to construct an emergency recovery transaction that bypasses the standard voting process. The intention is to provide an alternate execution path that can be utilized to fix an incorrectly configured VotingV2 contract.
Any user can propose an emergency transaction via the emergencyPropose function, but the size of the required bond is expected to be large enough that it is unlikely that an individual or the UMA team would be able to sufficiently fund the bond amount on their own. Proposals are executed using the emergencyExecute function in the GovernorV2 contract. The executor address has the sole ability to call this function, giving it veto power over any proposal. The UMA team controls the executor address.
There is currently no restriction that would prevent using the standard voting process to create a transaction that results in the GovernorV2 contract calling the emergencyPropose function to create a proposal. While this is highly unlikely as this transaction would need to pass a standard voting process, this could potentially introduce undesired effects and deviate from the expected functioning of the contract.
An example of an unexpected interaction is that a GovernorV2 emergency proposal can not be slashed by using the slashProposal function, as the slash mechanism transfers the slashed amount to the GovernorV2 address.
To avoid unexpected or undesirable behavior, consider preventing the GovernorV2 contract from being able to execute the emergencyPropose function.
Update: Fixed as of commit 41a03995087d2e63d047075582ef49783074ba48 in PR #4151.
public visibilityIn the VotingV2 contract, the function _getPriceFromPreviousVotingContract allows the current voting contract to query and retrieve prices from a previous voting contract. However, the function is defined with public visibility. As a result, this function can be directly called and could be used to bypass the _requireRegisteredContract check used by the onlyRegisteredContract modifier on hasPrice and getPrice. This would unintentionally allow users to access price details from a previous voting contract.
Consider changing the visibility of _getPriceFromPreviousVotingContract from public to private.
Update: Fixed as of commit 4f1c1d741d67959ab69acdf8ff70f7eb4d65f429 in PR #4154.
setExecutorIn the EmergencyProposal contract, the function setExecutor allows the owner of the contract to set a new executor. However, the function does not validate that the newExecutor address is not the zero address before updating the value.
Consider adding validation to prevent an unintended change that sets the executor to the zero address.
Update: Resolved, not an issue. Ability to set executor to zero is a feature of the UMA platform. UMA’s statement:
The audit fix proposes adding address validation to the setExecutor function such that this value can’t be set to the zero address. We choose to do nothing for this issue as being able to set the executor to the zero address is a feature: this acts to disable the emergencyProposer system, thereby disabling the emergency logic. This could be, for example, an upgrade path for the emergency proposal system or a path forward if governance deems the emergency proposal logic redundant.
Several functions within the codebase have missing or incomplete documentation:
In PR #4110:
In PR #4118:
_previousVotingContract parameter has no corresponding @param line in the VotingV2 constructor docstring_getPriceFromPreviousVotingContract function in VotingV2 has no docstringIn PR #4128:
emergencyPropose function in EmergencyProposer returns a value but its docstring has no corresponding @return lineIn PR #4135:
getCurrentTime function in VotingV2Test has no docstringcommitVote function in VotingV2Test lost its docstring when it was relocated from VotingV2.solrevealVote function in VotingV2Test lost its docstring when it was relocated from VotingV2.solcommitAndEmitEncryptedVote function in VotingV2Test lost its docstring when it was relocated from VotingV2.solgetPendingPriceRequestsArray function in VotingV2Test has no docstringgetPriceRequestStatuses function in VotingV2Test lost its docstring when it was relocated from VotingV2.solretrieveRewardsOnMigratedVotingContract function in VotingV2 returns a value but its docstring has no corresponding @return lineConsider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions that implement sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec). Also consider providing explanatory comments for all events, structs, and storage variables that indicate their intended purpose.
Update: Partially fixed as of commit e729b6ba86529598326ec80083cdfbaf8541b94c in PR #4155. UMA’s statement:
It has been decided not to add docstring in the test contracts as this is not done through the rest of the repo.
require statementsWithin VotingV2.sol there are require statements that lack error messages. For instance:
require statement on line 257 of VotingV2.solrequire statement on line 265 of VotingV2.solrequire statement on line 486 of VotingV2.solrequire statement on line 704 of VotingV2.solrequire statement on line 928 of VotingV2.solrequire statement on line 964 of VotingV2.solrequire statement on line 971 of VotingV2.solWe are aware error messages were removed to reduce the contract size, however their absence makes interacting or debugging the contract more difficult.
Consider including specific, informative error messages in require statements to improve overall code clarity and to facilitate troubleshooting whenever a requirement is not satisfied.
Update: Fixed as of commit a88a022a9a5b02c9cd0526c7fc2e72a094d02813 in PR #4173.
onlyIfNotMigrated enables new price requests on migrated contractsThe modifier onlyIfNotMigrated and its underlying private function _requireNotMigrated are used to ensure that the voting contract being called is the most recent voting contract deployed. This prevents users from requesting prices and voting on price requests on an outdated voting contract. However, onlyIfNotMigrated is missing from the functions requestGovernanceAction and signalRequestsAsSpamForDeletion. Both of these functions will call _requestPrice, which initiates a new price request. Consequently, users will not be able to vote on this request due to the onlyIfNotMigrated modifier being present on commitVote, and could potentially be slashed as a result.
Consider adding the onlyIfNotMigrated modifier to the functions requestGovernanceAction and signalRequestsAsSpamForDeletion.
Update: Fixed as of commit e62b25249441fc827f3fd6a336ed31d887c93dad in PR #4157.
Pull request #4117 adds a new retrieveRewardsOnMigratedVotingContract function to the VotingV2 contract, in order to enable voters to claim their rewards from the previous voting contract. However, this pull request does not include any test coverage for the newly added function.
Without an automated test for this function, manual testing is required to determine if the current implementation matches the functions’s expected behavior, which is error prone and deviates from the goal of continuous integration.
Consider adding test coverage for the new retrieveRewardsOnMigratedVotingContract function.
Update: Resolved, not an issue. Tests were included but fell outside the scope of the audit. UMA’s statement:
Test coverage was actually already added for this function, but in a separate PR to when the logic was added. This was added in an adjacent PR where we proposed to decrease the contract bytecode. The test for this logic can be found here.
executeEmergencyProposalIn the EmergencyProposal contract, the executeEmergencyProposal function iterates through the transactions submitted on the proposal and executes them. However, only the first element in the array is able to execute a payable transaction, as the first element of the proposed transactions will transfer the entirety of the contract ETH balance, meaning that subsequent transactions will have a value of zero.
Consider documenting this behavior thoroughly as it may not be obvious or intuitive to the proposal submitter.
Update: Fixed as of commit 3b6239aa8b38610247674c952ba7379de898d4b6 in PR #4158. Further clarification provided by UMA regarding the behavior of the executeEmergencyProposal function:
The emergency proposer forwards its entire balance (presumably the amount sent by the caller) to the governor in the first transaction. However, the governor could spend that balance over multiple transactions. The only requirement is that the execution transaction has the total of all ETH sent in all emergency execution transactions.
require statements with multiple conditionsThroughout the codebase there are require statements that contain multiple conditions. For instance:
require statement on line 202 of Staker.solrequire statement on line 267 of VotingV2.solrequire statement on line 762 of VotingV2.solrequire statement on line 801 of VotingV2.solrequire statement on line 986 of VotingV2.solBundling multiple conditions into a single require statement reduces the contract size, but also prevents being able to provide a separate error message for each failure case.
Consider isolating each condition in its own require statement, to facilitate specific user-friendly error messages for every required condition.
Update: Acknowledged, and will not fix. UMA’s statement:
We won’t add this suggestion because of bytecode limitations.
There are two locations where the Solidity version used is ^0.8.0:
This version string allows compilation with any version of Solidity from 0.8.0 up to the latest release. This may lead to unexpected behavior if the code is deployed with a different Solidity version than was used during testing. Further, allowing old versions of Solidity leaves the code potentially vulnerable to known security bugs which have already been patched. The official guidance is to always use the latest Solidity release when deploying contracts. When a bug is discovered that affects a range of Solidity versions, the general policy of the Solidity team is to only apply the fix to the latest release (i.e. no backporting of security updates).
Consider locking the version pragma to the same Solidity version used during development and testing. Also consider setting this version to be the latest release.
Update: Fixed as of commit abb30bb58fdf222a6d28363922b179043cea755c in PR #4159 and commit 3be4710c3868ca553e8ab87a815febf939fe021f in PR #4178._
In the VotingUpgraderV2 contract, the mutable state variables existingGovernor, newGovernor, existingVoting, newVoting, and finder are only set once within the constructor. Consider declaring these state variables as immutable to increase gas efficiency and enhance the clarity of your codebase.
Update: Fixed as of commit 1c77938f762f7bc55a387dbccab11b9a86a464bf in PR #4160.
The codebase contains several occurrences of misleading or incorrect documentation:
In the contract VotingUpgraderV2: – The comment on line 41 “Existing governor is the only one who can initiate the upgrade” appears to be a copy & paste error from line 38 which is identical. Consider rewriting the comment. – The comment on line 52 “Addresses to upgrade” does not apply to the newVoting variable on line 53. Consider rewriting the comment. – The comment on line 62 “Removes an address from the whitelist” does not apply to the VotingUpgraderV2 constructor. Consider rewriting the comment.
Update: Fixed as of commit 77c2370cd2d86920429e658c157941c019fbf697 in PR #4152 and commit 3be4710c3868ca553e8ab87a815febf939fe021f in PR #4178.
Non-explicit imports are used inside the codebase, which reduces code readability and could lead to conflicts between names defined locally and the ones imported. This is especially important if many contracts are defined within the same Solidity files or the inheritance chains are long.
Following the principle that clearer code is better code, consider using named import syntax (import {A, B, C} from "X") to explicitly declare which contracts are being imported.
For example, the DesignatedVotingV2 contract’s import block can be rewritten more explicitly as:
import { MultiCaller } from "../../common/implementation/MultiCaller.sol";
import { Stakeable, StakerInterface } from "../../common/implementation/Stakeable.sol";
import { FinderInterface } from "../interfaces/FinderInterface.sol";
import { OracleInterfaces } from "./Constants.sol";
The usage of explicit imports makes it clear that StakerInterface is provided by Stakeable.sol, and that OracleInterfaces is provided by Constants.sol.
Update: Acknowledged, and will not fix. UMA’s statement:
Although it is a good suggestion, we have decided not to incorporate it for consistency with the rest of the imports in the other contracts.
setDelegate and setDelegatorIn the Staker contract, the functions setDelegate and setDelegator use the nonReentrant modifier. This is not required as these two functions are only updating the state and cannot be exploited through a reentrancy attack.
Consider removing the nonReentrant modifier from these functions.
Update: Fixed as of commit 04dac164d4c8b026403aecb75730c7bc24f4400c in PR #4156.
Consider making the following changes to eliminate unnecessary code:
Staker.sol:
override keyword can be removed from the withdrawAndRestake functionVotingV2.sol:
uint256(i) casts can be replaced with the variable i because i is already declared as a uint256 type on line 976(rewards) can be changed to rewardsUpdate: Fixed as of commit d5ab183d3a37a2114db7c3a6269ae66c932d1286 in PR #4162.
In the EmergencyProposer contract, the following events all contain a uint256 lockedTokens parameter:
In each case, when these events are emitted, the value of lockedTokens remains constant for a given proposal, which effectively provides no information because the value never changes.
Update: Acknowledged, and will not fix. UMA’s statement:
The value of lockedTokens is constant across all events of the same emergency proposal during its lifecycle. However, to facilitate individual reading of the events, we have opted to leave it as is.
Several overloaded functions were deprecated and removed from the VotingV2 contract in order to reduce the total bytecode size, which included the commitVote, revealVote, and commitAndEmitEncryptedVote functions. While these functions were removed from the version of the VotingV2 contract that will be deployed, they were added to the VotingV2Test contract to ensure testing was not impacted by the removal. However, having two different versions of the voting contracts causes drift, which can make maintenance of the contracts more time consuming. Additionally, discrepancies between test code and production code can increase the likelihood of bugs not being caught during testing.
Consider removing the functions added to the VotingV2Test contract that are no longer present in the VotingV2 contract, and update tests accordingly.
Update: Acknowledged, and will not fix. UMA’s statement:
We have decided not to incorporate these changes. Updates to the tests could take a long time compared to the outcome.
Consider addressing the following typographical errors:
EmergencyProposer.sol (PR #4128):
Staker.sol (PR #4110):
VotingV2.sol (PR #4117):
VotingV2.sol (PR #4118):
_timerAddress parameter and should be deletedVotingV2.sol (PR #4139):
VotingV2.js (PR #4139):
Update: Fixed as of commit c311e0a53c8b3a0ddb00fcea79341baca42d142a in PR #4165.
The internal function _getPriceOrError in the VotingV2 contract returns three values: * bool to indicate success or failure * uint256 to indicate price * string which can be used to provide more context when _getPriceOrError returns false
Per the comment on _getPriceOrError, the string return value should be non-empty only when the function returns false. However, the additional logic to query prices from a previous voting contract via _getPriceFromPreviousVotingContract may return a non-empty string even when the price was successfully retrieved. Because the string message is only used in the error case, users will never see the “Returned from previous contract” string.
Consider returning an empty string when _getPriceOrError returns true.
Update: Fixed as of commit 0aef75d2fe05488c31b462484c57277ee30e9cb0 in PR #4163.
executeEmergencyProposalIn the EmergencyProposer contract, the executeEmergencyProposal function has public visibility. This is not required as the function is not called within the contract.
Consider changing the visibility of the executeEmergencyProposal function to external.
Update: Fixed as of commit fd4813895c1f113f71b03ac9cf79c5ab22710514 in PR #4166.
In the EmergencyProposer contract, the following files are imported but not used:
To improve readability and avoid confusion, consider removing the unused imports.
Update: Fixed as of commit a16bed5abe1de184c689ece8bb5e56e3c183f3e8 in PR #4164.
Within the EmergencyProposer contract, several variables are defined but not used:
finder state variable is initialized in the constructor, but otherwise is not usedcurrentId state variable is never usedTo improve readability and avoid confusion, consider removing the unused variables.
Update: Fixed as of commit 5f7567e7c9686ae68565a09fe4db49a2cd29a2f4 in PR #4167.
A single medium severity issue was found. Some recommendations were also made to improve the quality of the codebase.
There were multiple changes regarding VotingV2 that were done to reduce the contract bytecode size as it was reaching the limit. However, some of these changes included removing errors from require statements or grouping multiple conditions into a single statement. These changes make it more difficult for a user to debug failures when interacting with a contract and make the codebase less readable.