diff --git a/.changeset/proud-seals-complain.md b/.changeset/proud-seals-complain.md new file mode 100644 index 00000000000..35df4777e27 --- /dev/null +++ b/.changeset/proud-seals-complain.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`BeaconProxy`: Use an immutable variable to store the address of the beacon. It is no longer possible for a `BeaconProxy` to upgrade by changing to another beacon. diff --git a/contracts/proxy/ERC1967/ERC1967Utils.sol b/contracts/proxy/ERC1967/ERC1967Utils.sol index f7caaa68486..3ad93ff3c01 100644 --- a/contracts/proxy/ERC1967/ERC1967Utils.sol +++ b/contracts/proxy/ERC1967/ERC1967Utils.sol @@ -161,10 +161,13 @@ library ERC1967Utils { } /** - * @dev Perform beacon upgrade with additional setup call. Note: This upgrades the address of the beacon, it does - * not upgrade the implementation contained in the beacon (see {UpgradeableBeacon-_setImplementation} for that). + * @dev Change the beacon and trigger a setup call. * * Emits an {IERC1967-BeaconUpgraded} event. + * + * CAUTION: Invoking this function has no effect on an instance of {BeaconProxy} since v5, since + * it uses an immutable beacon without looking at the value of the ERC-1967 beacon slot for + * efficiency. */ function upgradeBeaconToAndCall(address newBeacon, bytes memory data, bool forceCall) internal { _setBeacon(newBeacon); diff --git a/contracts/proxy/beacon/BeaconProxy.sol b/contracts/proxy/beacon/BeaconProxy.sol index 94e697ca4d9..93e30688465 100644 --- a/contracts/proxy/beacon/BeaconProxy.sol +++ b/contracts/proxy/beacon/BeaconProxy.sol @@ -12,8 +12,14 @@ import {ERC1967Utils} from "../ERC1967/ERC1967Utils.sol"; * * The beacon address is stored in storage slot `uint256(keccak256('eip1967.proxy.beacon')) - 1`, so that it doesn't * conflict with the storage layout of the implementation behind the proxy. + * + * CAUTION: The beacon address can only be set once during construction, and cannot be changed afterwards. + * You must ensure that you either control the beacon, or trust the beacon to not upgrade the implementation maliciously. */ contract BeaconProxy is Proxy { + // An immutable address for the beacon to avoid unnecessary SLOADs before each delegate call. + address private immutable _beacon; + /** * @dev Initializes the proxy with `beacon`. * @@ -27,12 +33,20 @@ contract BeaconProxy is Proxy { */ constructor(address beacon, bytes memory data) payable { ERC1967Utils.upgradeBeaconToAndCall(beacon, data, false); + _beacon = beacon; } /** * @dev Returns the current implementation address of the associated beacon. */ function _implementation() internal view virtual override returns (address) { - return IBeacon(ERC1967Utils.getBeacon()).implementation(); + return IBeacon(_getBeacon()).implementation(); + } + + /** + * @dev Returns the beacon. + */ + function _getBeacon() internal view virtual returns (address) { + return _beacon; } }