Governor contract of OpenZeppelin: Security concerns about the execution of the proposal
Security concerns about the execution of the proposal
Summary
The OpenZeppelin Governance contract enables users to create proposals for transaction execution, with successful execution requiring ETH. Payable transactions are accepted during proposal execution, creating a risk for users who may unintentionally lose their ETH by sending an amount exceeding the total proposal values. This vulnerability opens the door for malicious proposals to exploit surplus ETH within the governor, potentially resulting in the misappropriation of user-locked funds within the Governor contract.
Overview
The governance protocol is generally implemented in a special-purpose contract called “Governor”. The GovernorAlpha and GovernorBravo contracts designed by Compound have been very successful and popular so far. OpenZeppelin Contracts set out to build a modular system of Governor contracts so that forking is not needed, and different requirements can be accommodated by writing small modules using Solidity inheritance. The design of OpenZeppelin Governor requires minimal use of storage and results in more gas-efficient operation.
The `Governor` contract allows users to create proposals via the `propose` function. Members of the governance can vote on the proposal via the castVote function. The proposal can be executed via the `execute` function if the proposal is successful. This requires the quorum to be reached, the vote to be successful, and the deadline to be reached. Otherwise, call the `cancel` function to cancel a proposal. A proposal is cancellable by the proposer, but only while it is in a pending state.
The execution of the proposal accepts payable transactions
The execute function allows the proposal to be executed with ETH, depending on the proposal values provided as a parameter. However, the proposal execution accepts payable transactions without verifying whether the values parameter is equal to the ETH sent.
A user will lose their ETH if they send more than the total value of the proposal.
While the proposal execution necessitates ETH for successful completion, if a user attaches more ETH than the proposal requires, they will lose the surplus. In the example below, the user sends 2 ETH, but the proposal value is only 1 ETH. Consequently, the user incurs a loss of 1 ETH. Although this is a mistake made by the user, the contract should not permit users to lose their ETH.
Another proposal takes advantage of the redundant ETH, steals user-locked funds in the Governor contract
If the scenario described above occurs, another proposal can take advantage of the surplus ETH, potentially stealing user-locked funds within the Governor contract. In such cases, subsequent proposals may exploit the excess funds, executing with zero ETH or an amount less than the required proposal value, as the Governor holds sufficient ETH. As illustrated in the test below, the second proposal requires 1 ETH, but the executor does not send any ETH when executing it. Consequently, the subsequent proposal can exploit the surplus ETH within the Governor, leading to the misappropriation of user-locked funds.
The full code tests are available on Gist
OpenZeppelin response
The Governor doesn't perform a check to enforce that msg.value == sum(values). However, we think the risk is acceptable given that the executor is in full control of selecting the value to send, and the Governor can recover stuck funds.
The msg.value of a transaction is completely controlled by the user when signing it, so a user must deliberately choose to overpay for a proposal execution. This is also preventable since the required value is known before the execution.
Regardless, we agree it could be an issue if there's no way to get the funds out sent by mistake, but the funds can be taken out using the relay() and the execute() function itself (in a different proposal).
Although subsequent proposals can use the value stuck in the Governor, a proposal can also take out the value (or the executor using the relay() function). We agree this may be a risk for those users who sent value by mistake, but we emphasize that the value sent requires signing a transaction. Also, the DAO can propose getting the funds back if such value is a considerable amount, capping the potential loss of funds.
Additionally, in order to get a proposal that's able to extract value from a previous execution, it requires voting and quorum to be met and then some executor sending value by mistake. This combination of factors seems difficult to coordinate.
The affected user can recover the stuck funds by either proposing to transfer the ETH to another address or contacting the Governor admin to recover the funds, especially if the value is substantial. This situation is a concern for users because the resolution of such issues depends on the actions of other users.
The OpenZeppelin team agrees that there should be better documentation regarding this matter. Consequently, they will make an update to the documentation, providing more details on how ETH is handled within the contract.
Conclusion
Security is a shared responsibility between the contract developer and the user. The contract developer must ensure that users do not unintentionally lose their ETH. Meanwhile, users need to exercise caution when sending ETH to the contract, being fully aware of the contract's behavior and the associated risk of potential ETH loss. The contract developer should also implement mechanisms to facilitate the recovery of substantial stuck funds.