OpenZeppelin Reentrancy Bugfix Review

Summary
Whitehat Zb3 submitted a critical reentrancy vulnerability in OpenZeppelin’s TimelockController contract on August 21, 2021, which affected a project hosted on Immunefi’s bug bounty platform. The project, which has elected to remain anonymous, has paid the whitehat an undisclosed amount (including a bonus for anonymity), and OpenZeppelin has graciously paid the whitehat a bounty of $25,000 for their contribution to community security and issued a patch.
This is the only critical vulnerability that OpenZeppelin has ever had in its open source smart contract library to the best of its knowledge. The vulnerability has been patched in the affected project, and OpenZeppelin has released an updated contract version which fixes the vulnerability. All projects using TimelockController should migrate.
Immunefi and OpenZeppelin proceeded to responsibly disclose the vulnerability to as many of the affected projects as possible, identifying and contacting 20 high risk projects out of a total of 316 instances of the TimelockController across numerous chains. We’d also like to thank the Dedaub and Tenderly teams for helping us find affected projects and are proud to announce that we found no cases of malicious exploitation of the vulnerability and no critical instances of the vulnerability containing funds at risk.
Vulnerability Analysis
OpenZeppelin’s TimelockController contract is a standard smart contract used across DeFi. The way the TimelockController works is that it introduces a delay for privileged actions to help prevent rogue or compromised admin actions and also allows project users an advanced look at upcoming changes.
In terms of the technical details of TimelockController, a PROPOSER
can use the schedule
function to schedule a particular call to be made from the timelock contract to a target contract chosen by the PROPOSER
, and after a delay elapses, the EXECUTOR
pushes the call through with the execute
function. Calls cannot be executed until the delay elapses. That is the essential function of the TimelockController. It allows for calls from the contract to be transparent and introduces oversight through a delay, which can protect users against the project maintainers suddenly going rogue.
The vulnerable code (modified for clarity) is listed below:
function executeBatch( address[] calldata targets, uint256[] calldata values, bytes[] calldata datas, bytes32 predecessor, bytes32 salt) public payable virtual onlyRoleOrOpenRole(EXECUTOR_ROLE) { require(targets.length == values.length); require(targets.length == datas.length); bytes32 id = hashOperationBatch(targets, values, datas, predecessor, salt); require(predecessor == bytes32(0) || isOperationDone(predecessor)); for (uint256 i = 0; i < targets.length; ++i) { _call(id, i, targets[i], values[i], datas[i]); } require(isOperationReady(id)); _timestamps[id] = _DONE_TIMESTAMP;}
Calls are dependent on each other, so they cannot be executed unless their predecessors have been executed. The code require(predecessor == bytes32(0) || isOperationDone(predecessor));
checks to see if the previous call has been executed, and require(isOperationReady(id));
checks to see if the calls were approved.
But the key problem here is that the check to see if the call was approved occurs after the call was already executed, so the check can be bypassed.
The EXECUTOR
, who is not the PROPOSER
, can make calls that essentially hijack the TimelockController and set the delay to 0, remove the existing PROPOSERs
, set themselves as the PROPOSER
, and make arbitrary calls to other contracts (taking them over, too). This is possible because the TimelockController is self-governing. That is, the ADMIN
role, which controls all the other roles, is assigned to the TimelockController itself. This means that any changes to the parameters of the timelock (such as adding or removing EXECUTOR
or PROPOSER
or changing the minimum delay) must themselves go through the timelock and be visible to anyone before being executed. The EXECUTOR
can cause the TimelockController to make calls to itself.
When working in Solidity, it is best practice to do Checks Effects Interactions. For any given function call, the first thing to do is check to make sure the call is authorized. Once all the checks are completed, the state can be updated, e.g., changing the balances, transferring the ownership of an NFT, etc. Only after those checks have been made can a call go out to an untrusted contract. The main purpose is to avoid reentrancy.
The step by step method to execute an attack is as follows:
1. The attacker gives themselves permission to propose
2. The attacker 0s out the delay parameter
3. The attacker sets an arbitrary call to be approved
Vulnerability Fix
OpenZeppelin has produced a fix available here. All projects using TimelockController should migrate immediately. The fix involved adding the isOperationReady(id)
check before the call is executed. Although it appears redundant, the isOperationReady(id)
check is required both before and after the call in order to prevent reentrancy.
Acknowledgements
We’d like to thank whitehat Zb3 for their impressive and important find. OpenZeppelin has also paid out a bounty of $25,000 to the whitehat and issued a patch for the timelock contract.
Thanks also to the OpenZeppelin for their speedy response and assistance in responsibly disclosing to users of their library. Dedaub and Tenderly also played an invaluable role in the disclosure process, finding instances of the contract across chains. OpenZeppelin’s diligence in maintaining the library and in protecting their users is a great example of community stewardship. We look forward to working with them on grander initiatives in the near future.
If you’d like to start bug hunting, we got you. Check out the Web3 Security Library, and start earning rewards on Immunefi — the leading bug bounty platform for web3 with the world’s biggest payouts.
If you’re interested in letting DeFi’s leading security community protect your project with a bug bounty, visit the Immunefi services page and fill out the form to launch your bug bounty on Immunefi.