UUPSUpgradeable Vulnerability Post-mortem

General Announcements
1

In early September, we received two independent reports of a vulnerability in the UUPSUpgradeable base contract of the OpenZeppelin Contracts library, first released in version 4.0 in April, 2021.

In this post, we share an overview of the contract and the vulnerability, executed mitigation strategies, and the implemented fix. To the best of our knowledge, we successfully executed mitigations and no project suffered loss of funds.

Thank you Raymond Yeh of Bluejay Finance for disclosing this vulnerability to us, including a working proof-of-concept of the exploit, on September 3rd. Additional thanks to Ashiq Amien of Iosiro who independently detected and reported the same issue just six days later. Finally, thank you to the Tenderly team, for helping us in track affected projects; and to Dedaub for promptly volunteering help, as well.

Upgrade Proxies

OpenZeppelin Contracts includes several proxy contracts for enabling upgradeability. All are based on the same concept: each upgradeable deployment consists of an implementation contract, which holds the code to be executed, and a proxy contract, which holds the state. Whenever a user calls the proxy contract, the proxy delegates execution to the implementation contract. In order to upgrade the contract, the proxy is pointed to a different implementation contract, thus changing the code being executed but preserving the same address and state. You can read more about how this pattern works here.

The two most popular patterns in the library are the Transparent proxy and the UUPS proxy. As described here, the main difference between them is where the upgrade logic resides. In the Transparent proxy, the upgrade logic is in the proxy. In the UUPS pattern, the proxy delegates all calls to the implementation, which handles both the business logic and upgrade logic. This allows users to decide the authorization logic used for upgrades, such as Ownable or AccessControl, or any other custom solution. To facilitate this, the library provides a base UUPSUpgradeable contract that implementation contracts should extend from.

The internal upgrade logic in UUPSUpgradeable performs several tasks. Besides changing the implementation contract stored in the proxy, it also executes a DELEGATECALL to the new implementation to atomically execute any migration function, as part of an upgradeToAndCall action.

After this, the contract performs a rollback test. In order to make sure that the new implementation contract has valid upgrade logic, it executes another DELEGATECALL to upgrade back into the current version. If it succeeds, the contract finally re-upgrades to the new implementation. This prevents accidentally upgrading to a contract without upgrade logic, which would freeze the proxy in that version.

The code for this upgrade operation is the following, which is implemented in the ERC1967Upgrade base contract.

function _upgradeToAndCallSecure(address newImplementation, bytes memory data, bool forceCall) internal {
  address oldImplementation = _getImplementation();

  // Initial upgrade and setup call
  _setImplementation(newImplementation);
  if (data.length > 0 || forceCall) {
    Address.functionDelegateCall(newImplementation, data);
  }

  // Perform rollback test if not already in progress
  StorageSlot.BooleanSlot storage rollbackTesting = StorageSlot.getBooleanSlot(_ROLLBACK_SLOT);

  if (!rollbackTesting.value) {
    // Trigger rollback using upgradeTo from the new implementation
    rollbackTesting.value = true;
    Address.functionDelegateCall(
      newImplementation,
      abi.encodeWithSignature("upgradeTo(address)", oldImplementation)
    );

    rollbackTesting.value = false;

    // Check rollback was effective
    require(oldImplementation == _getImplementation(), "ERC1967Upgrade: upgrade breaks further upgrades");

    // Finally reset to the new implementation and log the upgrade
    _upgradeTo(newImplementation);
  }
}

The UUPSUpgradeable contract extends ERC1967Upgrade, by exposing this upgrade internal method behind a customizable access control check.

Vulnerability

The vulnerability lies in the DELEGATECALL instructions in the upgrade function, exposed by the UUPSUpgradeable base contract. As described here, a DELEGATECALL can be exploited by an attacker by having the implementation contract call into another contract that SELFDESTRUCTs itself, causing the caller to be destroyed.

Given an UUPS implementation contract, an attacker can initialize it and appoint themselves as upgrade administrators. This allows them to call the upgradeToAndCall function on the implementation directly, instead of on the proxy, and use it to DELEGATECALL into a malicious contract with a SELFDESTRUCT operation.

If the attack is successful, any proxy contracts backed by this implementation become unusable, as any calls to them are delegated to an address with no executable code. Furthermore, since the upgrade logic resided in the implementation and not the proxy, it is not possible to upgrade the proxy to a valid implementation. This effectively bricks the contract, and impedes access to any assets held on it.

Mitigation

The mitigation for this vulnerability is simple: initializing the implementation contract. If the implementation is initialized, an attacker can no longer pass the authorization check of calling upgradeToAndCall in it. We shared this on September 9th as a public security advisory before disclosing the fix and vulnerability, to minimize its impact.

Before sharing the advisory, we sent transactions to initialize more than 150 uninitialized UUPS implementation contracts across Ethereum Mainnet, Polygon, xDAI, Binance, and Avalanche. We found no instances on Fuse, Fantom, Arbitrum, or Optimism. These transactions were all sent from EOA 0x37E8d216c3f6c79eC695FBD0cB9842e62fB84370, and through a batching contract at 0x310fAC62C976d8F6FDFA34332a56EA1a05493b5b. We relayed the transaction on mainnet privately using Taichi to protect against generalized frontrunning bots.

To identify UUPS instances, we searched through Upgraded events on all chains, which are emitted when a proxy is created. We then filtered proxies with ERC1967 implementation data and without admin, removing Transparent proxies from the mix. We then searched for occurrences of the DELEGATECALL opcode in the bytecode of the implementations obtained, and proceeded to analyze them. Additionally, we searched for the UUPSUpgradeable string across verified contracts on Etherscan, in case we had missed any with the previous search.

In order to identify the initialize function, which can vary between implementations depending on the parameters it receives, we extracted the calldata used when deploying a proxy for the implementation. This worked in most cases, but for others we had no recourse other than to reconstruct the initialize call manually.

To avoid accidentally granting access to an unsafe address when initializing a contract, we replaced anything resembling an address in the initialization calldata with a dummy contract. This contract implements several methods that were required by the initializers we reviewed, such as name, symbol, decimals, token0, token1, factory, and others.

Additionally, after executing the initialization, we selfdestructed the batching contract, since several initializers granted admin rights to msg.sender. This ensured that no address had control over any of the implementations we initialized.

Hotfix

A hotfix was shipped in Contracts 4.3.2 and in the upgrade-safe version of the library. The fix adds an onlyProxy modifier to the UUPSUpgradeable base contract preventing the upgrade functions to be called directly on the implementation.

address private immutable __self = address(this);

modifier onlyProxy() {
  require(address(this) != __self, "Function must be called through delegatecall");
  require(_getImplementation() == __self, "Function must be called through active proxy");
  _;
}

We also updated our documentation and guidelines to suggest initializing all implementation contracts upon deployment. Additionally, we modified the code generated by the Wizard to include a constructor that automatically initializes the implementation when deployed.

Next steps

Our team will thoroughly review the rollback test mechanism for the next release of the library. While the code provides solid guarantees against accidentally bricking the contract, it also requires the usage of the potentially dangerous DELEGATECALL operation. We will evaluate removing this check in favor of a simpler one, and adding more safeguards on the tooling side.

In terms of processes, we plan to increase the number of required reviews for all major features in Contracts, especially when it involves a code change that requires going against a security recommendation, as is the case here.

Last but not least, we are working on setting up active monitoring infrastructure to alert in the event of a deployment of a vulnerable uninitialized implementation contract. We will be sharing more news about this soon.

9 Likes
2

You can read Raymond's and Iosiro's writeups of the vulnerability here:

1 Like
3

Good you found this vulnerability.

Perhaps you can check what I proposed in Why not using ERC165 - IUUPSUpgradreable - #3 by gnarvaja as an alternative to doing the rollback test. Of course, it has the problem identified by @frangio in the reply but perhaps is safer than the current rollback test.

4

Yes, we have been discussing this approach. The issue mentioned by Fran can be circumvented by changing the returned value in supportsInterface depending on whether the call is madde on the implementation or not, which can be detected via immutable vars (see the fix that was added on the latest version).

We didn't want to introduce any major changes in this fix, but we'll evaluate this for the next big release.

1 Like
5

If you are thinking about major changes, perhaps you may want to consider adding protection on who can run initialize.
IMHO, leaving the possibility of having contracts that might be initialized by anyone even when they are implementation contracts it's a potential threat. Let's say someone initializes and hijacks an implementation contract deployed by a trusted address, and with social hacking methods tries to make people interact with that contract instead of the Proxy.

Following the same patch you did for 4.3.2, you can add an immutable variable registering the contract's creator address.

 address private immutable __self = address(this);                                                                    
+address private immutable __creator = msg.sender;

And then put a validation on initialize function that has to be called either from the proxy or from __creator.

6

The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.

1 Like
7

Great, much better.

It would be great if doing this change you can also support constructor args for setting immutable vars.

There is a pending issue in the plugins repo: https://github.com/OpenZeppelin/openzeppelin-upgrades/issues/312

Hopefully in the future I have the time and the skills to submit these proposed changes to your packages and I stop being so pedigüeño. :wink:

1 Like
8

Hi @spalladino

Thanks for describing this and Raymond as well for finding it.

Quick question.

You mention this:

Given an UUPS implementation contract, an attacker can initialize it and appoint themselves as upgrade administrators

Yes, anyone can call initialize, but how can they appoint themselves as upgrade administrators ? probably, _authorizeUpgrade will have a check who can update and who can't and if this decision doesn't come into play in the initialize function, then they won't be able to call it. Does that make sense ?

Thanks a lot.

9

True, but very often the initializer method is used to set up the upgrade administrator address, either to an address passed in as a parameter, or to msg.sender directly.

1 Like
10

Are we only supposed to add the _disableInitializers for UUPS Upgradable Implementations??

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
    _disableInitializers();
}
11

Pretty much. That disables initialization logic in the implementation, which is the one that can be destroyed via SELFDESTRUCT.

1 Like
12

Just to note, this exploit has been patched in the newest versions of OpenZeppelin 5.0 Right?

1 Like
13

Just to note, this exploit has been patched in the newest versions of OpenZeppelin 5.0 Right?

Patched since September 15, 2021, around the same time this post-mortem was released.

Note we'll never publish a post-mortem without being extremely sure there's no risk out there.

14

Is the beacon proxy vulnerable too? Must the constructor include _disableInitializer?