This security assessment was prepared by OpenZeppelin.
We audited the UMAprotocol/protocol repository at the 3687d9b commit.
In scope were the following contracts:
contracts
└── gnosis-zodiac
└── implementation
└── OptimisticGovernor.sol
The Optimistic Governor contract is meant to control an "avatar" that complies with the Gnosis Zodiac framework. The target use case is that the avatar is a Gnosis Safe that holds DAO funds and is capable of controlling DAO operations. The Optimistic Governor aims to allow anyone to control DAO funds and activities as long as their proposed activities conform to a set of natural language rules that the DAO has made publicly available. This is facilitated via the UMA Optimistic Oracle, which becomes the final arbiter in case proposals are contested for not conforming to DAO rules. In the case where proposals are not contested during their "liveness" period, they then become executable by anyone.
The main change is the migration to the Optimistic Oracle v3. As a consequence, it is possible to specify an escalation manager which enables replacing Data Verification Mechanism (DVM) dispute resolution with a custom dispute resolution logic and whitelisting asserters and disputers.
Additional changes:
Proposals are now deleted automatically and immediately upon a dispute. If the Optimistic Oracle is upgraded, anyone can delete all proposals that were submitted before the upgrade.
Proposal explanations and governance rules are now emitted with the proposal event to facilitate disputes and votes on the Optimistic Oracle v3.
Docstrings and styling were improved.
Some of the broader security concerns that were identified in the previous audit were addressed.
First and foremost, there is now less reliance on the DVM. Since proposals are deleted in case of a dispute, it is not possible to execute a malicious proposal even if the DVM is compromised.
Rules and explanations are now part of the proposal event, so the data availability of these items is no longer a concern.
However, there are some new security concerns, on top of those from the previous audit.
The ability to set an escalation manager provides a lot of flexibility, but should be configured with care because an unsafe configuration can break some of the trust assumptions and compromise the governance. It is the DAOs' responsibility to ensure the safety of any chosen escalation manager - not UMA's.
Transactions could be crafted to look safe or even desirable when called, but then be completely malicious if they are delegate-called. The context in which a call is executed comes down to a simple uint flag attached to a transaction. DAOs using the Optimistic Oracle need to be vigilant against potential transaction phishing attacks that use the wrong context for a call.
Finally, the Zodiac framework allows for additional modules, modifiers, and guards; it is essentially a smart contract middleware that can sit between EOA interactions with the Optimistic Governor and interactions with the avatar. These entire code chains must be well-understood, as they could potentially modify the final behavior of the avatar in ways that an inspection of the proposals and the Optimistic Governor cannot anticipate.
The Optimistic Governor contract has only one privileged role, which is that of the contract owner. The owner is able to set the bond amount and bond collateral token required to initiate a proposal. The owner can also set the rules that proposals must conform to, set the "liveness" time (the window during which a proposal is contestable), and set the identifier used by the Optimistic Oracle to support Optimistic Governance proposals. Finally, the owner can set the escalation manager and renounce or transfer ownership.
ProposalDeleted events can be emitted with non-existent assertionIdThe OptimisticGovernor contract's assertionDisputedCallback function does not validate whether its assertionId argument is valid (i.e., whether it is associated with a proposal). This allows a ProposalDeleted event to be emitted for any assertionId value, not just values that correspond to actual proposals.
A user can accomplish this by interacting with the OptimisticOracleV3 contract. The OptimisticOracleV3 contract's assertTruth function may be called by any user to assert truths that will be accepted as true unless disputed. This will result in the generation of a new assertionId that will be stored in the oracle's assertions mapping.
If assertTruth is called with the callbackRecipient set to zero and the escalationManager set to the OptimisticGovernor address, a subsequent call to disputeAssertion will call _callbackOnAssertionDispute, which in turn will call the OptimisticGovernor contract's assertionDisputedCallback function, passing the disputed assertionId, which exists in the assertions mapping of the oracle, but not in the proposalHashes mapping of the OptimisticGovernor. Regardless, the assertionId value is not checked and execution will enter the if condition on line 369 because the caller is the OptimisticOracle, and ultimately on line 374 the ProposalDeleted event will be emitted.
Consider adding a check in the assertionDisputedCallback function that ensures the assertionId value maps to a non-zero proposalHash, which ensures the assertion is associated with a proposal.
Update: Resolved in pull request #4486 at commit d880037.
The internal _sync function changes the OptimisticOracleV3 contract used by the OptimisticGovernor, but unlike other administrative setter functions, it does not emit an event when a change occurs.
Consider adding a new OptimisticOracleChanged event that is emitted by _sync whenever the finder contract returns a different address than that of the current optimisticOracleV3 variable.
Update: Resolved in pull request #4487 at commit f3ea7a6.
setEscalationManagerThe setEscalationManager administrative function in OptimisticGovernor does not perform any checks to validate that the _escalationManager address is a contract. Specifying an address where no contract code is deployed could result in unexpected behavior since the OptimisticOracleV3 contract expects that all escalation managers implement EscalationManagerInterface.
Consider using the existing _isContract function to validate the _escalationManager argument.
Update: Resolved in pull request #4488 at commit ed8f3fc.
In the OptimisticGovernor contract, the SetCollateral event, which can be used to emit an updated collateral value, is unused. The setCollateralAndBond function emits the SetBond event, which includes both the collateral token and the bond amount.
Consider renaming the SetBond event to SetCollateralAndBond, and removing the unused SetCollateral event.
Update: Resolved in pull request #4489 at commit cf6b68f.
Some functions in the OptimisticGovernor contract use leading underscores to name either input parameters or local variables in cases where there is no name collision with the contract's storage variables:
proposeTransactions: Input parameters _transactions and _explanationexecuteProposal: Input parameter _transactions and local variable _proposalHashdeleteProposalOnUpgrade: Input parameter _proposalHash_constructClaim: Input parameters _proposalHash and _explanationFor consistency and clarity, consider removing the leading underscore from these parameters, and also updating the corresponding docstrings. Alternatively, consider using the leading underscore consistently throughout the contract.
Update: Resolved in pull request #4490 at commit 8a2f6d5.
The executeProposal function has the payable modifier. However, any ETH sent is not used in the function and will be locked in the OptimisticGovernor contract.
Consider removing the payable attribute to avoid potentially locking ETH in the OptimisticGovernor contract.
Update: Resolved in pull request #4491 at commit f417256.
The proposalHashes and assertionIds mapping names misleadingly reflect the contents of one another (i.e., proposalHashes stores assertionId values, and assertionIds stores proposalHash values).
To increase clarity, consider swapping the names of the proposalHashes and assertionIds mappings, so that the variable names correspond to the values being stored.
Update: Resolved in pull request #4492 at commit c1e996c.
Consider making the following changes to eliminate redundant code:
OptimisticGovernor.sol, msg.sender is used on line 263 to transfer tokens from the proposer to the Optimistic Governor contract, but proposer has already been defined as msg.sender on line 234, so proposer can be used here.OptimisticGovernor.sol, line 310 computes the assertionId, but this value was already computed on line 307 inside a require statement. Lines 310 and 307 can be swapped, and proposalHashes[_proposalhash] inside the require statement can be replaced with assertionId.StakerInterface.sol, the setDelegate and setDelegator functions are marked virtual, but interface functions are implicitly virtual, so this keyword can be removed.Update: Resolved in pull request #4493 at commit 835a6ff. The StakerInterface.sol file was not changed. The UMA team stated:
The issue in
StakerInterface.solwas not fixed as it is not used by the Optimistic Governor, while the dependent VotingV2 contract is already deployed in production.
Four low-severity issues were found, and four notes were included to improve the quality of the codebase. Some changes were proposed regarding smart contract security best practices. In terms of system design, this iteration of the Optimistic Governor was found to have more flexibility and fewer security assumptions than the previous implementation.
While audits help in identifying potential security risks, the UMA team is encouraged to also incorporate automated monitoring of on-chain contract activity into their operations. Ongoing monitoring of deployed contracts helps in identifying potential threats and issues affecting the production environment.
To ensure no unexpected administrative actions are occurring, and to validate that correct values were used, consider monitoring all OptimisticGovernor administrative events (i.e., events emitted from functions with the onlyOwner modifier). In particular, consider monitoring:
OwnershipTransferred event for expected and unexpected changes of ownershipProposalExecuted events with identical proposalHash and assertionId values)Finder contract state, specifically a change in address for the Store, CollateralWhitelist, IdentifierWhitelist, and OptimisticOracleV3 contracts, which affect the behavior of the Optimistic GovernorSetCollateralAndBond and SetLiveness events, which may interfere with proper operation of the Governor