Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move upgradeToAndCallUUPS to UUPSUpgradeable #4356

Merged
merged 14 commits into from
Jun 17, 2023
5 changes: 5 additions & 0 deletions .changeset/hip-beds-provide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': major
---

Move the logic to validate ERC-1822 during an upgrade from `ERC1967Utils` to `UUPSUpgradeable`.
54 changes: 0 additions & 54 deletions contracts/mocks/proxy/UUPSLegacy.sol

This file was deleted.

6 changes: 6 additions & 0 deletions contracts/mocks/proxy/UUPSUpgradeableMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,9 @@ contract UUPSUpgradeableUnsafeMock is UUPSUpgradeableMock {
ERC1967Utils.upgradeToAndCall(newImplementation, data, false);
}
}

contract UUPSUnsupportedProxiableUUID is UUPSUpgradeableMock {
function proxiableUUID() external pure override returns (bytes32) {
return keccak256("invalid UUID");
}
}
34 changes: 0 additions & 34 deletions contracts/proxy/ERC1967/ERC1967Utils.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
pragma solidity ^0.8.20;

import "../beacon/IBeacon.sol";
import "../../interfaces/IERC1967.sol";
import "../../interfaces/draft-IERC1822.sol";
import "../../utils/Address.sol";
import "../../utils/StorageSlot.sol";

Expand Down Expand Up @@ -33,9 +31,6 @@ library ERC1967Utils {
*/
event BeaconUpgraded(address indexed beacon);

// This is the keccak-256 hash of "eip1967.proxy.rollback" subtracted by 1
bytes32 private constant _ROLLBACK_SLOT = 0x4910fdfa16fed3260ed0e7147f7cc6da11a60208b5b9406d12a635614ffd9143;

/**
* @dev Storage slot with the address of the current implementation.
* This is the keccak-256 hash of "eip1967.proxy.implementation" subtracted by 1, and is
Expand All @@ -59,11 +54,6 @@ library ERC1967Utils {
*/
error ERC1967InvalidBeacon(address beacon);

/**
* @dev The storage `slot` is unsupported as a UUID.
*/
error ERC1967UnsupportedProxiableUUID(bytes32 slot);

/**
* @dev Returns the current implementation address.
*/
Expand Down Expand Up @@ -103,30 +93,6 @@ library ERC1967Utils {
}
}

/**
* @dev Perform implementation upgrade with security checks for UUPS proxies, and additional setup call.
*
* Emits an {IERC1967-Upgraded} event.
*/
function upgradeToAndCallUUPS(address newImplementation, bytes memory data, bool forceCall) internal {
// Upgrades from old implementations will perform a rollback test. This test requires the new
// implementation to upgrade back to the old, non-ERC1822 compliant, implementation. Removing
// this special case will break upgrade paths from old UUPS implementation to new ones.
if (StorageSlot.getBooleanSlot(_ROLLBACK_SLOT).value) {
_setImplementation(newImplementation);
} else {
try IERC1822Proxiable(newImplementation).proxiableUUID() returns (bytes32 slot) {
if (slot != IMPLEMENTATION_SLOT) {
revert ERC1967UnsupportedProxiableUUID(slot);
}
} catch {
// The implementation is not UUPS
revert ERC1967InvalidImplementation(newImplementation);
}
upgradeToAndCall(newImplementation, data, forceCall);
}
}

/**
* @dev Storage slot with the admin of the contract.
* This is the keccak-256 hash of "eip1967.proxy.admin" subtracted by 1, and is
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
pragma solidity ^0.8.19;

import "../ERC1967/ERC1967Proxy.sol";
import "../../interfaces/IERC1967.sol";

/**
* @dev Interface for {TransparentUpgradeableProxy}. In order to implement transparency, {TransparentUpgradeableProxy}
Expand Down
36 changes: 28 additions & 8 deletions contracts/proxy/utils/UUPSUpgradeable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable {
*/
error UUPSUnauthorizedCallContext();

/**
* @dev The storage `slot` is unsupported as a UUID.
*/
error UUPSUnsupportedProxiableUUID(bytes32 slot);

/**
* @dev Check that the execution is being performed through a delegatecall call and that the execution context is
* a proxy contract with an implementation (as defined in ERC1967) pointing to self. This should only be the case
Expand All @@ -35,12 +40,10 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable {
* fail.
*/
modifier onlyProxy() {
if (address(this) == __self) {
// Must be called through delegatecall
revert UUPSUnauthorizedCallContext();
}
if (ERC1967Utils.getImplementation() != __self) {
// Must be called through an active proxy
if (
address(this) == __self || // Must be called through delegatecall
ERC1967Utils.getImplementation() != __self // Must be called through an active proxy
) {
Amxx marked this conversation as resolved.
Show resolved Hide resolved
revert UUPSUnauthorizedCallContext();
}
_;
Expand Down Expand Up @@ -81,7 +84,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable {
*/
function upgradeTo(address newImplementation) public virtual onlyProxy {
_authorizeUpgrade(newImplementation);
ERC1967Utils.upgradeToAndCallUUPS(newImplementation, new bytes(0), false);
_upgradeToAndCallUUPS(newImplementation, new bytes(0), false);
}

/**
Expand All @@ -96,7 +99,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable {
*/
function upgradeToAndCall(address newImplementation, bytes memory data) public payable virtual onlyProxy {
_authorizeUpgrade(newImplementation);
ERC1967Utils.upgradeToAndCallUUPS(newImplementation, data, true);
_upgradeToAndCallUUPS(newImplementation, data, true);
}

/**
Expand All @@ -110,4 +113,21 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable {
* ```
*/
function _authorizeUpgrade(address newImplementation) internal virtual;

/**
* @dev Perform implementation upgrade with security checks for UUPS proxies, and additional setup call.
*
* Emits an {IERC1967-Upgraded} event.
*/
function _upgradeToAndCallUUPS(address newImplementation, bytes memory data, bool forceCall) private {
try IERC1822Proxiable(newImplementation).proxiableUUID() returns (bytes32 slot) {
if (slot != ERC1967Utils.IMPLEMENTATION_SLOT) {
revert UUPSUnsupportedProxiableUUID(slot);
}
ERC1967Utils.upgradeToAndCall(newImplementation, data, forceCall);
} catch {
// The implementation is not UUPS
revert ERC1967Utils.ERC1967InvalidImplementation(newImplementation);
}
}
}
80 changes: 57 additions & 23 deletions test/proxy/utils/UUPSUpgradeable.test.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
const { expectEvent } = require('@openzeppelin/test-helpers');
const { web3 } = require('@openzeppelin/test-helpers/src/setup');
const { getSlot, ImplementationSlot } = require('../../helpers/erc1967');
const { getAddressInSlot, ImplementationSlot } = require('../../helpers/erc1967');
const { expectRevertCustomError } = require('../../helpers/customError');

const ERC1967Proxy = artifacts.require('ERC1967Proxy');
const UUPSUpgradeableMock = artifacts.require('UUPSUpgradeableMock');
const UUPSUpgradeableUnsafeMock = artifacts.require('UUPSUpgradeableUnsafeMock');
const UUPSUpgradeableLegacyMock = artifacts.require('UUPSUpgradeableLegacyMock');
const NonUpgradeableMock = artifacts.require('NonUpgradeableMock');
const UUPSUnsupportedProxiableUUID = artifacts.require('UUPSUnsupportedProxiableUUID');
const Address = artifacts.require('$Address');

contract('UUPSUpgradeable', function () {
before(async function () {
this.implInitial = await UUPSUpgradeableMock.new();
this.implUpgradeOk = await UUPSUpgradeableMock.new();
this.implUpgradeUnsafe = await UUPSUpgradeableUnsafeMock.new();
this.implUpgradeNonUUPS = await NonUpgradeableMock.new();
this.implUnsupportedUUID = await UUPSUnsupportedProxiableUUID.new();
this.helper = await Address.new();
});

beforeEach(async function () {
Expand All @@ -26,6 +28,7 @@ contract('UUPSUpgradeable', function () {
const { receipt } = await this.instance.upgradeTo(this.implUpgradeOk.address);
expect(receipt.logs.filter(({ event }) => event === 'Upgraded').length).to.be.equal(1);
expectEvent(receipt, 'Upgraded', { implementation: this.implUpgradeOk.address });
expect(await getAddressInSlot(this.instance, ImplementationSlot)).to.be.equal(this.implUpgradeOk.address);
});

it('upgrade to upgradeable implementation with call', async function () {
Expand All @@ -37,13 +40,64 @@ contract('UUPSUpgradeable', function () {
);
expect(receipt.logs.filter(({ event }) => event === 'Upgraded').length).to.be.equal(1);
expectEvent(receipt, 'Upgraded', { implementation: this.implUpgradeOk.address });
expect(await getAddressInSlot(this.instance, ImplementationSlot)).to.be.equal(this.implUpgradeOk.address);

expect(await this.instance.current()).to.be.bignumber.equal('1');
});

it('calling upgradeTo on the implementation reverts', async function () {
await expectRevertCustomError(
this.implInitial.upgradeTo(this.implUpgradeOk.address),
'UUPSUnauthorizedCallContext',
[],
);
});

it('calling upgradeToAndCall on the implementation reverts', async function () {
await expectRevertCustomError(
this.implInitial.upgradeToAndCall(
this.implUpgradeOk.address,
this.implUpgradeOk.contract.methods.increment().encodeABI(),
),
'UUPSUnauthorizedCallContext',
[],
);
});

it('calling upgradeTo from a contract that is not an ERC1967 proxy (with the right implementation) reverts', async function () {
await expectRevertCustomError(
this.helper.$functionDelegateCall(
this.implUpgradeOk.address,
this.implUpgradeOk.contract.methods.upgradeTo(this.implUpgradeUnsafe.address).encodeABI(),
),
'UUPSUnauthorizedCallContext',
[],
);
});

it('calling upgradeToAndCall from a contract that is not an ERC1967 proxy (with the right implementation) reverts', async function () {
await expectRevertCustomError(
this.helper.$functionDelegateCall(
this.implUpgradeOk.address,
this.implUpgradeOk.contract.methods.upgradeToAndCall(this.implUpgradeUnsafe.address, '0x').encodeABI(),
),
'UUPSUnauthorizedCallContext',
[],
);
});

it('rejects upgrading to an unsupported UUID', async function () {
await expectRevertCustomError(
this.instance.upgradeTo(this.implUnsupportedUUID.address),
'UUPSUnsupportedProxiableUUID',
[web3.utils.keccak256('invalid UUID')],
);
});

it('upgrade to and unsafe upgradeable implementation', async function () {
const { receipt } = await this.instance.upgradeTo(this.implUpgradeUnsafe.address);
expectEvent(receipt, 'Upgraded', { implementation: this.implUpgradeUnsafe.address });
expect(await getAddressInSlot(this.instance, ImplementationSlot)).to.be.equal(this.implUpgradeUnsafe.address);
});

// delegate to a non existing upgradeTo function causes a low level revert
Expand All @@ -63,24 +117,4 @@ contract('UUPSUpgradeable', function () {
otherInstance.address,
]);
});

it('can upgrade from legacy implementations', async function () {
const legacyImpl = await UUPSUpgradeableLegacyMock.new();
const legacyInstance = await ERC1967Proxy.new(legacyImpl.address, '0x').then(({ address }) =>
UUPSUpgradeableLegacyMock.at(address),
);

const receipt = await legacyInstance.upgradeTo(this.implInitial.address);

const UpgradedEvents = receipt.logs.filter(
({ address, event }) => address === legacyInstance.address && event === 'Upgraded',
);
expect(UpgradedEvents.length).to.be.equal(1);
frangio marked this conversation as resolved.
Show resolved Hide resolved

expectEvent(receipt, 'Upgraded', { implementation: this.implInitial.address });

const implementationSlot = await getSlot(legacyInstance, ImplementationSlot);
const implementationAddress = web3.utils.toChecksumAddress(implementationSlot.substr(-40));
expect(implementationAddress).to.be.equal(this.implInitial.address);
});
});