We diff-audited the lidofinance/dual-governance repository at the HEAD commit 46d667e against the BASE commit 08e622a.
The following files were in scope:
contracts
├── DualGovernance.sol
├── EmergencyProtectedTimelock.sol
├── Escrow.sol
├── Executor.sol
├── ImmutableDualGovernanceConfigProvider.sol
├── ResealManager.sol
├── TimelockedGovernance.sol
├── committees
│ ├── HashConsensus.sol
│ ├── ProposalsList.sol
│ ├── TiebreakerCoreCommittee.sol
│ └── TiebreakerSubCommittee.sol
├── interfaces
│ ├── IDualGovernance.sol
│ ├── IDualGovernanceConfigProvider.sol
│ ├── IEmergencyProtectedTimelock.sol
│ ├── IEscrowBase.sol
│ ├── IExternalExecutor.sol
│ ├── IGovernance.sol
│ ├── IOwnable.sol
│ ├── IRageQuitEscrow.sol
│ ├── IResealManager.sol
│ ├── ISealable.sol
│ ├── ISignallingEscrow.sol
│ ├── IStETH.sol
│ ├── ITiebreaker.sol
│ ├── ITiebreakerCoreCommittee.sol
│ ├── ITimelock.sol
│ ├── IWithdrawalQueue.sol
│ └── IWstETH.sol
├── libraries
│ ├── AssetsAccounting.sol
│ ├── DualGovernanceConfig.sol
│ ├── DualGovernanceStateMachine.sol
│ ├── DualGovernanceStateTransitions.sol
│ ├── EmergencyProtection.sol
│ ├── EnumerableProposals.sol
│ ├── EscrowState.sol
│ ├── ExecutableProposals.sol
│ ├── ExternalCalls.sol
│ ├── Proposers.sol
│ ├── Resealer.sol
│ ├── SealableCalls.sol
│ ├── Tiebreaker.sol
│ ├── TimelockState.sol
│ └── WithdrawalsBatchesQueue.sol
└── types
├── Duration.sol
├── ETHValue.sol
├── IndexOneBased.sol
├── PercentD16.sol
├── SharesValue.sol
└── Timestamp.sol
Update: The fix review for this audit has been completed, and all pull requests addressing the reported issues have been merged at commit 3e0f1ae.
The Dual Governance system enables Lido stakers to veto the governance proposals passed by LDO holders. By depositing stETH, wstETH, or Lido Withdrawal NFTs into an Escrow, stakers can delay proposals while retaining the option to exit the protocol. The system transitions between Normal, Veto Signalling, Veto Cooldown, and Rage Quit states which, in turn, determines whether proposal submission and execution are allowed. Tiebreaker logic handles emergency pauses, allowing a special committee to unpause protocol contracts and finalize pending proposals if a deadlock arises.
The Dual Governance contracts were previously audited by OpenZeppelin. This audit focused on changes implemented since the initial review, including:
EmergencyExecutionCommittee and EmergencyActivationCommittee contracts.The admin executor is an instance of the Executor contract, which is set up with special privileges. Previously, this contract had the following capabilities:
ResealManager addressEmergencyProtectedTimelock contractWith this update, the following changes to privileged roles were made:
cancelAllPendingProposals function. Previously, this function was only callable by a proposer that was associated with the admin executor.ResealManager address was previously immutable in the DualGovernance contract. Now, this address can be set by the admin executor.It is assumed that all parties who operate and manage the roles of the system act in the best interest of the protocol, follow best practices to protect their access, and are trained to follow security procedures in the event of an emergency.
Details of the roles involved can be found in the system specification.
The setMinAssetsLockDuration function sets the minimum lock duration of the Escrow contract. This function performs a check to ensure that the new lock duration does not exceed the maximum lock duration, which is passed to the function as the MAX_MIN_ASSETS_LOCK_DURATION constant.
However, this maximum duration check is not performed during the initialization of the Escrow contract. Thus, the lock duration only depends on the DualGovernance configuration, which the Escrow contract is set up with as a minimum lock duration.
Consider passing down the maximum lock duration value to the initialize function and calling the internal setMinAssetsLockDuration function.
Update: Resolved in pull request #256 at commit 30015d3.
In Resealer.sol, the setResealManager function prevents the new manager from being set to the zero address, whereas the setResealCommittee function does not have this check.
Consider adding a check to the setResealCommittee function to ensure that the committee is not set to the zero address.
Update: Acknowledged, not resolved. The Lido team stated:
Acknowledged. This is the expected behavior.
In the future, there is a chance that the use of
ISealablecontracts will be abandoned, so it is necessary to have the ability to disable the reseal functionality.ResealManageris an essential part ofDualGovernance, so if it is set to the zero address, it may cause reverts in the system's functioning. Therefore, it was decided that the reseal functionality can be disabled by setting the committee to the zero address.
ResealManager Could Make Use of SealableCalls LibraryThe Tiebreaker contract uses the SealableCalls library function callGetResumeSinceTimestamp to catch potential reverts and return a boolean indicating whether the call succeeded. In contrast, the ResealManager contract calls the ISealable interface directly, which could cause a generic revert.
Consider using the SealableCalls library in ResealManager to revert more specifically.
Update: Acknowledged, not resolved. The Lido team stated:
Acknowledged. This is the expected behavior.
For the Tiebreaker functionality, it is critically important to handle reverts of
ISealable, for example, in cases where the contract implementation has been updated and no longer conforms to theISealableinterface. ForResealManager, it is necessary to explicitly determine whether a transaction has reverted or not.
Throughout the codebase, multiple opportunities for code quality improvement were identified:
abi.encodeWithSelector which is not type-safe. Instead, consider using abi.encodeCall.Escrow.sol, the lockUnstETH, unlockUnstETH, and markUnstETHFinalized functions perform actions on the unstETHIds input array. However, markUnstETHFinalized does not revert given an empty input array, unlike the other functions. For consistency, consider implementing the same check in markUnstETHFinalized as well.setProposerExecutor and unregisterProposer functions of DualGovernance.sol ensure that they can only be called by the admin executor and that the admin executor remains with a valid proposer afterwards. This is done by querying the admin executor address once from the timelock and then once by using msg.sender. Consider using msg.sender instead of querying the timelock in setProposerExecutor to save some gas.Consider applying the above suggestions to improve the quality of the codebase.
Update: Partially resolved in pull request #258 at commit f721d2f. The Lido team stated:
We decided to not change everything related to
abi.encodeWithSelector.
In the DualGovernanceConfig library, the firstSealRageQuitSupport field of the Context struct is described as the following:
The percentage of the total stETH supply that must be reached in the Signalling Escrow to transition Dual Governance from the Normal state to the VetoSignalling state
However, the VetoSignalling state can also be accessed from the Veto Cooldown and Rage Quit states.
Consider updating the documentation to reflect the specification.
Update: Resolved in pull request #260 at commit ca3ea75.
The cancelAllPendingProposals function of the TimelockedGovernance contract does not describe the purpose of its boolean return value. This value indicates the success of the function.
For improved code clarity, consider documenting the return value of cancelAllPendingProposals.
Update: Resolved in pull request #261 at commit d03d9d1.
Some of the publicly declared constants and function signatures in the provided interfaces do not align with their respective implementation contracts.
For the IDualGovernance interface:
registerProposer, unregisterProposer, isProposer, and getProposer functions, the parameter account should be proposerAccount.isExecutor function, the address parameter should be executor.setProposalsCanceller and getProposalsCanceller functions are not defined.Additionally:
DUAL_GOVERNANCE public constant is not defined in the IEscrowBase interface.MIN_EXECUTION_DELAY public constant is not defined in the IEmergencyProtectedTimelock interface.To avoid any unexpected behavior, ensure that all contracts correctly implement their public interfaces.
Update: Partially resolved in pull request #262 at commit d4fde93. The Lido team stated:
We decided to not add constants to interfaces.
calldata Instead of memoryWhen working with external function parameters, reading arguments directly from calldata is more gas-efficient than storing them in memory. As a read-only region containing the arguments of incoming external calls, calldata is cheaper and more gas-efficient for these parameters.
Throughout the codebase, multiple instances where function parameters could use calldata instead of memory were identified:
Escrow.sol, the unstETHIds parameters [1, 2, 3].HashConsensus.sol, the membersToRemove parameter.Consider using calldata as the data location for the parameters of external functions to optimize gas usage.
Update: Acknowledged, not resolved.
Throughout the codebase, multiple instances of typographical errors were identified:
TimelockState.sol, "configuration related the state" should be "configuration related to the state".Proposers.sol, "Checks if an proposerAccount" should be "Checks if a proposerAccount".Consider correcting the above-listed typographical errors to improve the clarity and readability of the codebase.
Update: Resolved in pull request #265 at commit c19a82b.
After conducting a thorough review of the modifications made to the Dual Governance contracts, we identified several opportunities for minor enhancements to the codebase. However, due to the complexity of the system we recommend applying further testing methods like Formal Verification and Fuzzing. Throughout the audit, the Lido team was highly responsive, promptly addressing our inquiries and providing us with comprehensive documentation and detailed explanations about the system’s design and functionality.