From f715365ec40515159ac5f4a4d6f85625ab3b405d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Fri, 4 Aug 2023 14:23:38 -0600 Subject: [PATCH] Implement recommendations from 5.0 audit Phase 1B (#4502) Co-authored-by: Hadrien Croubois Co-authored-by: Francisco Giordano --- .changeset/fifty-owls-retire.md | 5 + .changeset/hip-goats-fail.md | 5 + contracts/finance/VestingWallet.sol | 5 +- contracts/metatx/ERC2771Context.sol | 32 ++++- contracts/metatx/ERC2771Forwarder.sol | 112 +++++++++++---- contracts/mocks/AddressFnPointersMock.sol | 50 ------- contracts/mocks/CallReceiverMock.sol | 12 ++ contracts/mocks/UpgradeableBeaconMock.sol | 27 ++++ contracts/mocks/UpgreadeableBeaconMock.sol | 12 -- contracts/proxy/ERC1967/ERC1967Proxy.sol | 10 +- contracts/proxy/ERC1967/ERC1967Utils.sol | 15 +- contracts/proxy/Proxy.sol | 9 -- contracts/proxy/beacon/IBeacon.sol | 2 +- contracts/proxy/beacon/UpgradeableBeacon.sol | 2 +- contracts/proxy/transparent/ProxyAdmin.sol | 2 +- .../TransparentUpgradeableProxy.sol | 20 ++- contracts/proxy/utils/UUPSUpgradeable.sol | 7 +- contracts/utils/Address.sol | 132 ++++-------------- contracts/utils/Strings.sol | 6 +- test/metatx/ERC2771Context.test.js | 6 +- test/metatx/ERC2771Forwarder.t.sol | 6 +- test/metatx/ERC2771Forwarder.test.js | 29 +++- test/proxy/ERC1967/ERC1967Utils.test.js | 12 ++ test/proxy/beacon/UpgradeableBeacon.test.js | 7 + test/utils/Address.test.js | 44 +----- 25 files changed, 286 insertions(+), 283 deletions(-) create mode 100644 .changeset/fifty-owls-retire.md create mode 100644 .changeset/hip-goats-fail.md delete mode 100644 contracts/mocks/AddressFnPointersMock.sol create mode 100644 contracts/mocks/UpgradeableBeaconMock.sol delete mode 100644 contracts/mocks/UpgreadeableBeaconMock.sol diff --git a/.changeset/fifty-owls-retire.md b/.changeset/fifty-owls-retire.md new file mode 100644 index 000000000..118fad421 --- /dev/null +++ b/.changeset/fifty-owls-retire.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`Address`: Removed the ability to customize error messages. A common custom error is always used if the underlying revert reason cannot be bubbled up. diff --git a/.changeset/hip-goats-fail.md b/.changeset/hip-goats-fail.md new file mode 100644 index 000000000..5cfe2ef79 --- /dev/null +++ b/.changeset/hip-goats-fail.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`VestingWallet`: Fix revert during 1 second time window when duration is 0. diff --git a/contracts/finance/VestingWallet.sol b/contracts/finance/VestingWallet.sol index 840837d27..2e8fdab82 100644 --- a/contracts/finance/VestingWallet.sol +++ b/contracts/finance/VestingWallet.sol @@ -19,6 +19,9 @@ import {Context} from "../utils/Context.sol"; * * By setting the duration to 0, one can configure this contract to behave like an asset timelock that hold tokens for * a beneficiary until a specified time. + * + * NOTE: When using this contract with any token whose balance is adjusted automatically (i.e. a rebase token), make sure + * to account the supply/balance adjustment in the vesting schedule to ensure the vested amount is as intended. */ contract VestingWallet is Context { event EtherReleased(uint256 amount); @@ -154,7 +157,7 @@ contract VestingWallet is Context { function _vestingSchedule(uint256 totalAllocation, uint64 timestamp) internal view virtual returns (uint256) { if (timestamp < start()) { return 0; - } else if (timestamp > end()) { + } else if (timestamp >= end()) { return totalAllocation; } else { return (totalAllocation * (timestamp - start())) / duration(); diff --git a/contracts/metatx/ERC2771Context.sol b/contracts/metatx/ERC2771Context.sol index 4a2df6e3d..1084afb15 100644 --- a/contracts/metatx/ERC2771Context.sol +++ b/contracts/metatx/ERC2771Context.sol @@ -18,15 +18,36 @@ abstract contract ERC2771Context is Context { /// @custom:oz-upgrades-unsafe-allow state-variable-immutable address private immutable _trustedForwarder; + /** + * @dev Initializes the contract with a trusted forwarder, which will be able to + * invoke functions on this contract on behalf of other accounts. + * + * NOTE: The trusted forwarder can be replaced by overriding {trustedForwarder}. + */ /// @custom:oz-upgrades-unsafe-allow constructor - constructor(address trustedForwarder) { - _trustedForwarder = trustedForwarder; + constructor(address trustedForwarder_) { + _trustedForwarder = trustedForwarder_; } + /** + * @dev Returns the address of the trusted forwarder. + */ + function trustedForwarder() public view virtual returns (address) { + return _trustedForwarder; + } + + /** + * @dev Indicates whether any particular address is the trusted forwarder. + */ function isTrustedForwarder(address forwarder) public view virtual returns (bool) { - return forwarder == _trustedForwarder; + return forwarder == trustedForwarder(); } + /** + * @dev Override for `msg.sender`. Defaults to the original `msg.sender` whenever + * a call is not performed by the trusted forwarder or the calldata length is less than + * 20 bytes (an address length). + */ function _msgSender() internal view virtual override returns (address sender) { if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) { // The assembly code is more direct than the Solidity version using `abi.decode`. @@ -39,6 +60,11 @@ abstract contract ERC2771Context is Context { } } + /** + * @dev Override for `msg.data`. Defaults to the original `msg.data` whenever + * a call is not performed by the trusted forwarder or the calldata length is less than + * 20 bytes (an address length). + */ function _msgData() internal view virtual override returns (bytes calldata) { if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) { return msg.data[:msg.data.length - 20]; diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 93e334622..6f8e000d2 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.20; +import {ERC2771Context} from "./ERC2771Context.sol"; import {ECDSA} from "../utils/cryptography/ECDSA.sol"; import {EIP712} from "../utils/cryptography/EIP712.sol"; import {Nonces} from "../utils/Nonces.sol"; @@ -26,6 +27,16 @@ import {Address} from "../utils/Address.sol"; * transactions in the mempool. In these cases the recommendation is to distribute the load among * multiple accounts. * + * WARNING: Do not approve this contract to spend tokens. Anyone can use this forwarder + * to execute calls with an arbitrary calldata to any address. Any form of approval may + * result in a loss of funds for the approving party. + * + * NOTE: Batching requests includes an optional refund for unused `msg.value` that is achieved by + * performing a call with empty calldata. While this is within the bounds of ERC-2771 compliance, + * if the refund receiver happens to consider the forwarder a trusted forwarder, it MUST properly + * handle `msg.data.length == 0`. `ERC2771Context` in OpenZeppelin Contracts versions prior to 4.9.3 + * do not handle this properly. + * * ==== Security Considerations * * If a relayer submits a forward request, it should be willing to pay up to 100% of the gas amount @@ -82,6 +93,11 @@ contract ERC2771Forwarder is EIP712, Nonces { */ error ERC2771ForwarderExpiredRequest(uint48 deadline); + /** + * @dev The request target doesn't trust the `forwarder`. + */ + error ERC2771UntrustfulTarget(address target, address forwarder); + /** * @dev See {EIP712-constructor}. */ @@ -90,15 +106,15 @@ contract ERC2771Forwarder is EIP712, Nonces { /** * @dev Returns `true` if a request is valid for a provided `signature` at the current block timestamp. * - * A transaction is considered valid when it hasn't expired (deadline is not met), and the signer - * matches the `from` parameter of the signed request. + * A transaction is considered valid when the target trusts this forwarder, the request hasn't expired + * (deadline is not met), and the signer matches the `from` parameter of the signed request. * * NOTE: A request may return false here but it won't cause {executeBatch} to revert if a refund * receiver is provided. */ function verify(ForwardRequestData calldata request) public view virtual returns (bool) { - (bool alive, bool signerMatch, ) = _validate(request); - return alive && signerMatch; + (bool isTrustedForwarder, bool active, bool signerMatch, ) = _validate(request); + return isTrustedForwarder && active && signerMatch; } /** @@ -186,31 +202,42 @@ contract ERC2771Forwarder is EIP712, Nonces { */ function _validate( ForwardRequestData calldata request - ) internal view virtual returns (bool alive, bool signerMatch, address signer) { - signer = _recoverForwardRequestSigner(request); - return (request.deadline >= block.timestamp, signer == request.from, signer); + ) internal view virtual returns (bool isTrustedForwarder, bool active, bool signerMatch, address signer) { + (bool isValid, address recovered) = _recoverForwardRequestSigner(request); + + return ( + _isTrustedByTarget(request.to), + request.deadline >= block.timestamp, + isValid && recovered == request.from, + recovered + ); } /** - * @dev Recovers the signer of an EIP712 message hash for a forward `request` and its corresponding `signature`. - * See {ECDSA-recover}. + * @dev Returns a tuple with the recovered the signer of an EIP712 forward request message hash + * and a boolean indicating if the signature is valid. + * + * NOTE: The signature is considered valid if {ECDSA-tryRecover} indicates no recover error for it. */ - function _recoverForwardRequestSigner(ForwardRequestData calldata request) internal view virtual returns (address) { - return - _hashTypedDataV4( - keccak256( - abi.encode( - _FORWARD_REQUEST_TYPEHASH, - request.from, - request.to, - request.value, - request.gas, - nonces(request.from), - request.deadline, - keccak256(request.data) - ) + function _recoverForwardRequestSigner( + ForwardRequestData calldata request + ) internal view virtual returns (bool, address) { + (address recovered, ECDSA.RecoverError err, ) = _hashTypedDataV4( + keccak256( + abi.encode( + _FORWARD_REQUEST_TYPEHASH, + request.from, + request.to, + request.value, + request.gas, + nonces(request.from), + request.deadline, + keccak256(request.data) ) - ).recover(request.signature); + ) + ).tryRecover(request.signature); + + return (err == ECDSA.RecoverError.NoError, recovered); } /** @@ -232,12 +259,16 @@ contract ERC2771Forwarder is EIP712, Nonces { ForwardRequestData calldata request, bool requireValidRequest ) internal virtual returns (bool success) { - (bool alive, bool signerMatch, address signer) = _validate(request); + (bool isTrustedForwarder, bool active, bool signerMatch, address signer) = _validate(request); // Need to explicitly specify if a revert is required since non-reverting is default for // batches and reversion is opt-in since it could be useful in some scenarios if (requireValidRequest) { - if (!alive) { + if (!isTrustedForwarder) { + revert ERC2771UntrustfulTarget(request.to, address(this)); + } + + if (!active) { revert ERC2771ForwarderExpiredRequest(request.deadline); } @@ -247,7 +278,7 @@ contract ERC2771Forwarder is EIP712, Nonces { } // Ignore an invalid request because requireValidRequest = false - if (signerMatch && alive) { + if (isTrustedForwarder && signerMatch && active) { // Nonce should be used before the call to prevent reusing by reentrancy uint256 currentNonce = _useNonce(signer); @@ -269,6 +300,33 @@ contract ERC2771Forwarder is EIP712, Nonces { } } + /** + * @dev Returns whether the target trusts this forwarder. + * + * This function performs a static call to the target contract calling the + * {ERC2771Context-isTrustedForwarder} function. + */ + function _isTrustedByTarget(address target) private view returns (bool) { + bytes memory encodedParams = abi.encodeCall(ERC2771Context.isTrustedForwarder, (address(this))); + + bool success; + uint256 returnSize; + uint256 returnValue; + /// @solidity memory-safe-assembly + assembly { + // Perform the staticcal and save the result in the scratch space. + // | Location | Content | Content (Hex) | + // |-----------|----------|--------------------------------------------------------------------| + // | | | result ↓ | + // | 0x00:0x1F | selector | 0x0000000000000000000000000000000000000000000000000000000000000001 | + success := staticcall(gas(), target, add(encodedParams, 0x20), mload(encodedParams), 0, 0x20) + returnSize := returndatasize() + returnValue := mload(0) + } + + return success && returnSize >= 0x20 && returnValue > 0; + } + /** * @dev Checks if the requested gas was correctly forwarded to the callee. * diff --git a/contracts/mocks/AddressFnPointersMock.sol b/contracts/mocks/AddressFnPointersMock.sol deleted file mode 100644 index dc4edc075..000000000 --- a/contracts/mocks/AddressFnPointersMock.sol +++ /dev/null @@ -1,50 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -import {Address} from "../utils/Address.sol"; - -/** - * @dev A mock to expose `Address`'s functions with function pointers. - */ -contract AddressFnPointerMock { - error CustomRevert(); - - function functionCall(address target, bytes memory data) external returns (bytes memory) { - return Address.functionCall(target, data, _customRevert); - } - - function functionCallWithValue(address target, bytes memory data, uint256 value) external returns (bytes memory) { - return Address.functionCallWithValue(target, data, value, _customRevert); - } - - function functionStaticCall(address target, bytes memory data) external view returns (bytes memory) { - return Address.functionStaticCall(target, data, _customRevert); - } - - function functionDelegateCall(address target, bytes memory data) external returns (bytes memory) { - return Address.functionDelegateCall(target, data, _customRevert); - } - - function verifyCallResultFromTarget( - address target, - bool success, - bytes memory returndata - ) external view returns (bytes memory) { - return Address.verifyCallResultFromTarget(target, success, returndata, _customRevert); - } - - function verifyCallResult(bool success, bytes memory returndata) external view returns (bytes memory) { - return Address.verifyCallResult(success, returndata, _customRevert); - } - - function verifyCallResultVoid(bool success, bytes memory returndata) external view returns (bytes memory) { - return Address.verifyCallResult(success, returndata, _customRevertVoid); - } - - function _customRevert() internal pure { - revert CustomRevert(); - } - - function _customRevertVoid() internal pure {} -} diff --git a/contracts/mocks/CallReceiverMock.sol b/contracts/mocks/CallReceiverMock.sol index 281afacfe..e371c7db8 100644 --- a/contracts/mocks/CallReceiverMock.sol +++ b/contracts/mocks/CallReceiverMock.sol @@ -59,3 +59,15 @@ contract CallReceiverMock { return "0x1234"; } } + +contract CallReceiverMockTrustingForwarder is CallReceiverMock { + address private _trustedForwarder; + + constructor(address trustedForwarder_) { + _trustedForwarder = trustedForwarder_; + } + + function isTrustedForwarder(address forwarder) public view virtual returns (bool) { + return forwarder == _trustedForwarder; + } +} diff --git a/contracts/mocks/UpgradeableBeaconMock.sol b/contracts/mocks/UpgradeableBeaconMock.sol new file mode 100644 index 000000000..354ac02f0 --- /dev/null +++ b/contracts/mocks/UpgradeableBeaconMock.sol @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {IBeacon} from "../proxy/beacon/IBeacon.sol"; + +contract UpgradeableBeaconMock is IBeacon { + address public implementation; + + constructor(address impl) { + implementation = impl; + } +} + +interface IProxyExposed { + // solhint-disable-next-line func-name-mixedcase + function $getBeacon() external view returns (address); +} + +contract UpgradeableBeaconReentrantMock is IBeacon { + error BeaconProxyBeaconSlotAddress(address beacon); + + function implementation() external view override returns (address) { + // Revert with the beacon seen in the proxy at the moment of calling to check if it's + // set before the call. + revert BeaconProxyBeaconSlotAddress(IProxyExposed(msg.sender).$getBeacon()); + } +} diff --git a/contracts/mocks/UpgreadeableBeaconMock.sol b/contracts/mocks/UpgreadeableBeaconMock.sol deleted file mode 100644 index 4bee5c0f2..000000000 --- a/contracts/mocks/UpgreadeableBeaconMock.sol +++ /dev/null @@ -1,12 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; - -import {IBeacon} from "../proxy/beacon/IBeacon.sol"; - -contract UpgradeableBeaconMock is IBeacon { - address public implementation; - - constructor(address impl) { - implementation = impl; - } -} diff --git a/contracts/proxy/ERC1967/ERC1967Proxy.sol b/contracts/proxy/ERC1967/ERC1967Proxy.sol index d4104ccfb..df3a61369 100644 --- a/contracts/proxy/ERC1967/ERC1967Proxy.sol +++ b/contracts/proxy/ERC1967/ERC1967Proxy.sol @@ -14,17 +14,17 @@ import {ERC1967Utils} from "./ERC1967Utils.sol"; */ contract ERC1967Proxy is Proxy { /** - * @dev Initializes the upgradeable proxy with an initial implementation specified by `_logic`. + * @dev Initializes the upgradeable proxy with an initial implementation specified by `implementation`. * - * If `_data` is nonempty, it's used as data in a delegate call to `_logic`. This will typically be an encoded + * If `_data` is nonempty, it's used as data in a delegate call to `implementation`. This will typically be an encoded * function call, and allows initializing the storage of the proxy like a Solidity constructor. * * Requirements: * * - If `data` is empty, `msg.value` must be zero. */ - constructor(address _logic, bytes memory _data) payable { - ERC1967Utils.upgradeToAndCall(_logic, _data); + constructor(address implementation, bytes memory _data) payable { + ERC1967Utils.upgradeToAndCall(implementation, _data); } /** @@ -34,7 +34,7 @@ contract ERC1967Proxy is Proxy { * https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call. * `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc` */ - function _implementation() internal view virtual override returns (address impl) { + function _implementation() internal view virtual override returns (address) { return ERC1967Utils.getImplementation(); } } diff --git a/contracts/proxy/ERC1967/ERC1967Utils.sol b/contracts/proxy/ERC1967/ERC1967Utils.sol index bd2108096..b6f705e54 100644 --- a/contracts/proxy/ERC1967/ERC1967Utils.sol +++ b/contracts/proxy/ERC1967/ERC1967Utils.sol @@ -31,8 +31,7 @@ library ERC1967Utils { /** * @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 - * validated in the constructor. + * This is the keccak-256 hash of "eip1967.proxy.implementation" subtracted by 1. */ // solhint-disable-next-line private-vars-leading-underscore bytes32 internal constant IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc; @@ -94,8 +93,7 @@ library ERC1967Utils { /** * @dev Storage slot with the admin of the contract. - * This is the keccak-256 hash of "eip1967.proxy.admin" subtracted by 1, and is - * validated in the constructor. + * This is the keccak-256 hash of "eip1967.proxy.admin" subtracted by 1. */ // solhint-disable-next-line private-vars-leading-underscore bytes32 internal constant ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; @@ -133,7 +131,7 @@ library ERC1967Utils { /** * @dev The storage slot of the UpgradeableBeacon contract which defines the implementation for this proxy. - * This is bytes32(uint256(keccak256('eip1967.proxy.beacon')) - 1) and is validated in the constructor. + * This is the keccak-256 hash of "eip1967.proxy.beacon" subtracted by 1. */ // solhint-disable-next-line private-vars-leading-underscore bytes32 internal constant BEACON_SLOT = 0xa3f0ad74e5423aebfd80d3ef4346578335a9a72aeaee59ff6cb3582b35133d50; @@ -153,12 +151,12 @@ library ERC1967Utils { revert ERC1967InvalidBeacon(newBeacon); } + StorageSlot.getAddressSlot(BEACON_SLOT).value = newBeacon; + address beaconImplementation = IBeacon(newBeacon).implementation(); if (beaconImplementation.code.length == 0) { revert ERC1967InvalidImplementation(beaconImplementation); } - - StorageSlot.getAddressSlot(BEACON_SLOT).value = newBeacon; } /** @@ -184,7 +182,8 @@ library ERC1967Utils { } /** - * @dev Reverts if `msg.value` is not zero. + * @dev Reverts if `msg.value` is not zero. It can be used to avoid `msg.value` stuck in the contract + * if an upgrade doesn't perform an initialization call. */ function _checkNonPayable() private { if (msg.value > 0) { diff --git a/contracts/proxy/Proxy.sol b/contracts/proxy/Proxy.sol index 3644197fa..005a876ac 100644 --- a/contracts/proxy/Proxy.sol +++ b/contracts/proxy/Proxy.sol @@ -56,7 +56,6 @@ abstract contract Proxy { * This function does not return to its internal call site, it will return directly to the external caller. */ function _fallback() internal virtual { - _beforeFallback(); _delegate(_implementation()); } @@ -67,12 +66,4 @@ abstract contract Proxy { fallback() external payable virtual { _fallback(); } - - /** - * @dev Hook that is called before falling back to the implementation. Can happen as part of a manual `_fallback` - * call, or as part of the Solidity `fallback` or `receive` functions. - * - * If overridden should call `super._beforeFallback()`. - */ - function _beforeFallback() internal virtual {} } diff --git a/contracts/proxy/beacon/IBeacon.sol b/contracts/proxy/beacon/IBeacon.sol index f5e9f7981..56477c92a 100644 --- a/contracts/proxy/beacon/IBeacon.sol +++ b/contracts/proxy/beacon/IBeacon.sol @@ -10,7 +10,7 @@ interface IBeacon { /** * @dev Must return an address that can be used as a delegate call target. * - * {BeaconProxy} will check that this address is a contract. + * {UpgradeableBeacon} will check that this address is a contract. */ function implementation() external view returns (address); } diff --git a/contracts/proxy/beacon/UpgradeableBeacon.sol b/contracts/proxy/beacon/UpgradeableBeacon.sol index 81ce50902..a7816a1e6 100644 --- a/contracts/proxy/beacon/UpgradeableBeacon.sol +++ b/contracts/proxy/beacon/UpgradeableBeacon.sol @@ -51,7 +51,6 @@ contract UpgradeableBeacon is IBeacon, Ownable { */ function upgradeTo(address newImplementation) public virtual onlyOwner { _setImplementation(newImplementation); - emit Upgraded(newImplementation); } /** @@ -66,5 +65,6 @@ contract UpgradeableBeacon is IBeacon, Ownable { revert BeaconInvalidImplementation(newImplementation); } _implementation = newImplementation; + emit Upgraded(newImplementation); } } diff --git a/contracts/proxy/transparent/ProxyAdmin.sol b/contracts/proxy/transparent/ProxyAdmin.sol index c0e6780f4..cd03fb939 100644 --- a/contracts/proxy/transparent/ProxyAdmin.sol +++ b/contracts/proxy/transparent/ProxyAdmin.sol @@ -16,7 +16,7 @@ contract ProxyAdmin is Ownable { * and `upgradeAndCall(address,bytes)` are present, and `upgradeTo` must be used if no function should be called, * while `upgradeAndCall` will invoke the `receive` function if the second argument is the empty byte string. * If the getter returns `"5.0.0"`, only `upgradeAndCall(address,bytes)` is present, and the second argument must - * be the empty byte string if no function should be called, being impossible to invoke the `receive` function + * be the empty byte string if no function should be called, making it impossible to invoke the `receive` function * during an upgrade. */ string public constant UPGRADE_INTERFACE_VERSION = "5.0.0"; diff --git a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol index b51d2973e..77ed5fe73 100644 --- a/contracts/proxy/transparent/TransparentUpgradeableProxy.sol +++ b/contracts/proxy/transparent/TransparentUpgradeableProxy.sol @@ -45,6 +45,9 @@ interface ITransparentUpgradeableProxy is IERC1967 { * implement transparency without decoding reverts caused by selector clashes between the proxy and the * implementation. * + * NOTE: This proxy does not inherit from {Context} deliberately. The {ProxyAdmin} of this contract won't send a + * meta-transaction in any way, and any other meta-transaction setup should be made in the implementation contract. + * * IMPORTANT: This contract avoids unnecessary storage reads by setting the admin only during construction as an immutable variable, * preventing any changes thereafter. However, the admin slot defined in ERC-1967 can still be overwritten by the implementation * logic pointed to by this proxy. In such cases, the contract may end up in an undesirable state where the admin slot is different @@ -75,18 +78,25 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { constructor(address _logic, address initialOwner, bytes memory _data) payable ERC1967Proxy(_logic, _data) { _admin = address(new ProxyAdmin(initialOwner)); // Set the storage value and emit an event for ERC-1967 compatibility - ERC1967Utils.changeAdmin(_admin); + ERC1967Utils.changeAdmin(_proxyAdmin()); + } + + /** + * @dev Returns the admin of this proxy. + */ + function _proxyAdmin() internal virtual returns (address) { + return _admin; } /** * @dev If caller is the admin process the call internally, otherwise transparently fallback to the proxy behavior. */ function _fallback() internal virtual override { - if (msg.sender == _admin) { - if (msg.sig == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { - _dispatchUpgradeToAndCall(); - } else { + if (msg.sender == _proxyAdmin()) { + if (msg.sig != ITransparentUpgradeableProxy.upgradeToAndCall.selector) { revert ProxyDeniedAdminAccess(); + } else { + _dispatchUpgradeToAndCall(); } } else { super._fallback(); diff --git a/contracts/proxy/utils/UUPSUpgradeable.sol b/contracts/proxy/utils/UUPSUpgradeable.sol index 7ad0c9abc..f08e61c1e 100644 --- a/contracts/proxy/utils/UUPSUpgradeable.sol +++ b/contracts/proxy/utils/UUPSUpgradeable.sol @@ -25,7 +25,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { * and `upgradeToAndCall(address,bytes)` are present, and `upgradeTo` must be used if no function should be called, * while `upgradeToAndCall` will invoke the `receive` function if the second argument is the empty byte string. * If the getter returns `"5.0.0"`, only `upgradeToAndCall(address,bytes)` is present, and the second argument must - * be the empty byte string if no function should be called, being impossible to invoke the `receive` function + * be the empty byte string if no function should be called, making it impossible to invoke the `receive` function * during an upgrade. */ string public constant UPGRADE_INTERFACE_VERSION = "5.0.0"; @@ -126,7 +126,10 @@ 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. + * @dev Performs an implementation upgrade with a security check for UUPS proxies, and additional setup call. + * + * As a security check, {proxiableUUID} is invoked in the new implementation, and the return value + * is expected to be the implementation slot in ERC1967. * * Emits an {IERC1967-Upgraded} event. */ diff --git a/contracts/utils/Address.sol b/contracts/utils/Address.sol index e3a71313c..fd22b05ab 100644 --- a/contracts/utils/Address.sol +++ b/contracts/utils/Address.sol @@ -54,8 +54,10 @@ library Address { * plain `call` is an unsafe replacement for a function call: use this * function instead. * - * If `target` reverts with a revert reason, it is bubbled up by this - * function (like regular Solidity function calls). + * If `target` reverts with a revert reason or custom error, it is bubbled + * up by this function (like regular Solidity function calls). However, if + * the call reverted with no returned reason, this function reverts with a + * {FailedInnerCall} error. * * Returns the raw returned data. To convert to the expected return value, * use https://solidity.readthedocs.io/en/latest/units-and-global-variables.html?highlight=abi.decode#abi-encoding-and-decoding-functions[`abi.decode`]. @@ -66,23 +68,7 @@ library Address { * - calling `target` with `data` must not revert. */ function functionCall(address target, bytes memory data) internal returns (bytes memory) { - return functionCallWithValue(target, data, 0, defaultRevert); - } - - /** - * @dev Same as {xref-Address-functionCall-address-bytes-}[`functionCall`], but with a - * `customRevert` function as a fallback when `target` reverts. - * - * Requirements: - * - * - `customRevert` must be a reverting function. - */ - function functionCall( - address target, - bytes memory data, - function() internal view customRevert - ) internal returns (bytes memory) { - return functionCallWithValue(target, data, 0, customRevert); + return functionCallWithValue(target, data, 0); } /** @@ -95,28 +81,11 @@ library Address { * - the called Solidity function must be `payable`. */ function functionCallWithValue(address target, bytes memory data, uint256 value) internal returns (bytes memory) { - return functionCallWithValue(target, data, value, defaultRevert); - } - - /** - * @dev Same as {xref-Address-functionCallWithValue-address-bytes-uint256-}[`functionCallWithValue`], but - * with a `customRevert` function as a fallback revert reason when `target` reverts. - * - * Requirements: - * - * - `customRevert` must be a reverting function. - */ - function functionCallWithValue( - address target, - bytes memory data, - uint256 value, - function() internal view customRevert - ) internal returns (bytes memory) { if (address(this).balance < value) { revert AddressInsufficientBalance(address(this)); } (bool success, bytes memory returndata) = target.call{value: value}(data); - return verifyCallResultFromTarget(target, success, returndata, customRevert); + return verifyCallResultFromTarget(target, success, returndata); } /** @@ -124,20 +93,8 @@ library Address { * but performing a static call. */ function functionStaticCall(address target, bytes memory data) internal view returns (bytes memory) { - return functionStaticCall(target, data, defaultRevert); - } - - /** - * @dev Same as {xref-Address-functionCall-address-bytes-string-}[`functionCall`], - * but performing a static call. - */ - function functionStaticCall( - address target, - bytes memory data, - function() internal view customRevert - ) internal view returns (bytes memory) { (bool success, bytes memory returndata) = target.staticcall(data); - return verifyCallResultFromTarget(target, success, returndata, customRevert); + return verifyCallResultFromTarget(target, success, returndata); } /** @@ -145,82 +102,48 @@ library Address { * but performing a delegate call. */ function functionDelegateCall(address target, bytes memory data) internal returns (bytes memory) { - return functionDelegateCall(target, data, defaultRevert); - } - - /** - * @dev Same as {xref-Address-functionCall-address-bytes-string-}[`functionCall`], - * but performing a delegate call. - */ - function functionDelegateCall( - address target, - bytes memory data, - function() internal view customRevert - ) internal returns (bytes memory) { (bool success, bytes memory returndata) = target.delegatecall(data); - return verifyCallResultFromTarget(target, success, returndata, customRevert); + return verifyCallResultFromTarget(target, success, returndata); } /** - * @dev Tool to verify that a low level call to smart-contract was successful, and revert (either by bubbling - * the revert reason or using the provided `customRevert`) in case of unsuccessful call or if target was not a contract. + * @dev Tool to verify that a low level call to smart-contract was successful, and reverts if the target + * was not a contract or bubbling up the revert reason (falling back to {FailedInnerCall}) in case of an + * unsuccessful call. */ function verifyCallResultFromTarget( address target, bool success, - bytes memory returndata, - function() internal view customRevert + bytes memory returndata ) internal view returns (bytes memory) { - if (success) { - if (returndata.length == 0) { - // only check if target is a contract if the call was successful and the return data is empty - // otherwise we already know that it was a contract - if (target.code.length == 0) { - revert AddressEmptyCode(target); - } + if (!success) { + _revert(returndata); + } else { + // only check if target is a contract if the call was successful and the return data is empty + // otherwise we already know that it was a contract + if (returndata.length == 0 && target.code.length == 0) { + revert AddressEmptyCode(target); } return returndata; - } else { - _revert(returndata, customRevert); } } /** - * @dev Tool to verify that a low level call was successful, and revert if it wasn't, either by bubbling the - * revert reason or with a default revert error. + * @dev Tool to verify that a low level call was successful, and reverts if it wasn't, either by bubbling the + * revert reason or with a default {FailedInnerCall} error. */ - function verifyCallResult(bool success, bytes memory returndata) internal view returns (bytes memory) { - return verifyCallResult(success, returndata, defaultRevert); - } - - /** - * @dev Same as {xref-Address-verifyCallResult-bool-bytes-}[`verifyCallResult`], but with a - * `customRevert` function as a fallback when `success` is `false`. - * - * Requirements: - * - * - `customRevert` must be a reverting function. - */ - function verifyCallResult( - bool success, - bytes memory returndata, - function() internal view customRevert - ) internal view returns (bytes memory) { - if (success) { + function verifyCallResult(bool success, bytes memory returndata) internal pure returns (bytes memory) { + if (!success) { + _revert(returndata); + } else { return returndata; - } else { - _revert(returndata, customRevert); } } /** - * @dev Default reverting function when no `customRevert` is provided in a function call. + * @dev Reverts with returndata if present. Otherwise reverts with {FailedInnerCall}. */ - function defaultRevert() internal pure { - revert FailedInnerCall(); - } - - function _revert(bytes memory returndata, function() internal view customRevert) private view { + function _revert(bytes memory returndata) private pure { // Look for revert reason and bubble it up if present if (returndata.length > 0) { // The easiest way to bubble the revert reason is using memory via assembly @@ -230,7 +153,6 @@ library Address { revert(add(32, returndata), returndata_size) } } else { - customRevert(); revert FailedInnerCall(); } } diff --git a/contracts/utils/Strings.sol b/contracts/utils/Strings.sol index 0037eee1b..24c22d53d 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -10,7 +10,7 @@ import {SignedMath} from "./math/SignedMath.sol"; * @dev String operations. */ library Strings { - bytes16 private constant _SYMBOLS = "0123456789abcdef"; + bytes16 private constant _HEX_DIGITS = "0123456789abcdef"; uint8 private constant _ADDRESS_LENGTH = 20; /** @@ -34,7 +34,7 @@ library Strings { ptr--; /// @solidity memory-safe-assembly assembly { - mstore8(ptr, byte(mod(value, 10), _SYMBOLS)) + mstore8(ptr, byte(mod(value, 10), _HEX_DIGITS)) } value /= 10; if (value == 0) break; @@ -68,7 +68,7 @@ library Strings { buffer[0] = "0"; buffer[1] = "x"; for (uint256 i = 2 * length + 1; i > 1; --i) { - buffer[i] = _SYMBOLS[localValue & 0xf]; + buffer[i] = _HEX_DIGITS[localValue & 0xf]; localValue >>= 4; } if (localValue != 0) { diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index 0ec8d98dd..522b05cd1 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -36,7 +36,11 @@ contract('ERC2771Context', function (accounts) { }); it('recognize trusted forwarder', async function () { - expect(await this.recipient.isTrustedForwarder(this.forwarder.address)); + expect(await this.recipient.isTrustedForwarder(this.forwarder.address)).to.equal(true); + }); + + it('returns the trusted forwarder', async function () { + expect(await this.recipient.trustedForwarder()).to.equal(this.forwarder.address); }); context('when called directly', function () { diff --git a/test/metatx/ERC2771Forwarder.t.sol b/test/metatx/ERC2771Forwarder.t.sol index 189ed6ac5..3256289ea 100644 --- a/test/metatx/ERC2771Forwarder.t.sol +++ b/test/metatx/ERC2771Forwarder.t.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; import {ERC2771Forwarder} from "contracts/metatx/ERC2771Forwarder.sol"; -import {CallReceiverMock} from "contracts/mocks/CallReceiverMock.sol"; +import {CallReceiverMockTrustingForwarder, CallReceiverMock} from "contracts/mocks/CallReceiverMock.sol"; struct ForwardRequest { address from; @@ -40,7 +40,7 @@ contract ERC2771ForwarderMock is ERC2771Forwarder { contract ERC2771ForwarderTest is Test { ERC2771ForwarderMock internal _erc2771Forwarder; - CallReceiverMock internal _receiver; + CallReceiverMockTrustingForwarder internal _receiver; uint256 internal _signerPrivateKey; uint256 internal _relayerPrivateKey; @@ -52,7 +52,7 @@ contract ERC2771ForwarderTest is Test { function setUp() public { _erc2771Forwarder = new ERC2771ForwarderMock("ERC2771Forwarder"); - _receiver = new CallReceiverMock(); + _receiver = new CallReceiverMockTrustingForwarder(address(_erc2771Forwarder)); _signerPrivateKey = 0xA11CE; _relayerPrivateKey = 0xB0B; diff --git a/test/metatx/ERC2771Forwarder.test.js b/test/metatx/ERC2771Forwarder.test.js index 26726e8df..209c84b2f 100644 --- a/test/metatx/ERC2771Forwarder.test.js +++ b/test/metatx/ERC2771Forwarder.test.js @@ -7,14 +7,13 @@ const { constants, expectRevert, expectEvent, time } = require('@openzeppelin/te const { expect } = require('chai'); const ERC2771Forwarder = artifacts.require('ERC2771Forwarder'); -const CallReceiverMock = artifacts.require('CallReceiverMock'); +const CallReceiverMockTrustingForwarder = artifacts.require('CallReceiverMockTrustingForwarder'); contract('ERC2771Forwarder', function (accounts) { const [, refundReceiver, another] = accounts; const tamperedValues = { from: another, - to: another, value: web3.utils.toWei('0.5'), data: '0x1742', deadline: 0xdeadbeef, @@ -41,7 +40,7 @@ contract('ERC2771Forwarder', function (accounts) { this.alice.address = web3.utils.toChecksumAddress(this.alice.getAddressString()); this.timestamp = await time.latest(); - this.receiver = await CallReceiverMock.new(); + this.receiver = await CallReceiverMockTrustingForwarder.new(this.forwarder.address); this.request = { from: this.alice.address, to: this.receiver.address, @@ -97,6 +96,10 @@ contract('ERC2771Forwarder', function (accounts) { }); } + it('returns false with an untrustful to', async function () { + expect(await this.forwarder.verify(this.forgeData({ to: another }).message)).to.be.equal(false); + }); + it('returns false with tampered signature', async function () { const tamperedsign = web3.utils.hexToBytes(this.requestData.signature); tamperedsign[42] ^= 0xff; @@ -169,6 +172,14 @@ contract('ERC2771Forwarder', function (accounts) { }); } + it('reverts with an untrustful to', async function () { + const data = this.forgeData({ to: another }); + await expectRevertCustomError(this.forwarder.execute(data.message), 'ERC2771UntrustfulTarget', [ + data.message.to, + this.forwarder.address, + ]); + }); + it('reverts with tampered signature', async function () { const tamperedSig = web3.utils.hexToBytes(this.requestData.signature); tamperedSig[42] ^= 0xff; @@ -360,6 +371,18 @@ contract('ERC2771Forwarder', function (accounts) { }); } + it('reverts with at least one untrustful to', async function () { + const data = this.forgeData({ ...this.requestDatas[this.idx], to: another }); + + this.requestDatas[this.idx] = data.message; + + await expectRevertCustomError( + this.forwarder.executeBatch(this.requestDatas, this.refundReceiver, { value: this.msgValue }), + 'ERC2771UntrustfulTarget', + [this.requestDatas[this.idx].to, this.forwarder.address], + ); + }); + it('reverts with at least one tampered request signature', async function () { const tamperedSig = web3.utils.hexToBytes(this.requestDatas[this.idx].signature); tamperedSig[42] ^= 0xff; diff --git a/test/proxy/ERC1967/ERC1967Utils.test.js b/test/proxy/ERC1967/ERC1967Utils.test.js index cce874cd9..975b08d81 100644 --- a/test/proxy/ERC1967/ERC1967Utils.test.js +++ b/test/proxy/ERC1967/ERC1967Utils.test.js @@ -9,6 +9,7 @@ const ERC1967Utils = artifacts.require('$ERC1967Utils'); const V1 = artifacts.require('DummyImplementation'); const V2 = artifacts.require('CallReceiverMock'); const UpgradeableBeaconMock = artifacts.require('UpgradeableBeaconMock'); +const UpgradeableBeaconReentrantMock = artifacts.require('UpgradeableBeaconReentrantMock'); contract('ERC1967Utils', function (accounts) { const [, admin, anotherAccount] = accounts; @@ -155,6 +156,17 @@ contract('ERC1967Utils', function (accounts) { await expectEvent.inTransaction(receipt.tx, await V2.at(this.utils.address), 'MockFunctionCalled'); }); }); + + describe('reentrant beacon implementation() call', function () { + it('sees the new beacon implementation', async function () { + const newBeacon = await UpgradeableBeaconReentrantMock.new(); + await expectRevertCustomError( + this.utils.$upgradeBeaconToAndCall(newBeacon.address, '0x'), + 'BeaconProxyBeaconSlotAddress', + [newBeacon.address], + ); + }); + }); }); }); }); diff --git a/test/proxy/beacon/UpgradeableBeacon.test.js b/test/proxy/beacon/UpgradeableBeacon.test.js index 4c58f1740..0737f6fdf 100644 --- a/test/proxy/beacon/UpgradeableBeacon.test.js +++ b/test/proxy/beacon/UpgradeableBeacon.test.js @@ -20,6 +20,13 @@ contract('UpgradeableBeacon', function (accounts) { this.beacon = await UpgradeableBeacon.new(this.v1.address, owner); }); + it('emits Upgraded event to the first implementation', async function () { + const beacon = await UpgradeableBeacon.new(this.v1.address, owner); + await expectEvent.inTransaction(beacon.contract.transactionHash, beacon, 'Upgraded', { + implementation: this.v1.address, + }); + }); + it('returns implementation', async function () { expect(await this.beacon.implementation()).to.equal(this.v1.address); }); diff --git a/test/utils/Address.test.js b/test/utils/Address.test.js index beded18e1..57453abd5 100644 --- a/test/utils/Address.test.js +++ b/test/utils/Address.test.js @@ -3,7 +3,6 @@ const { expect } = require('chai'); const { expectRevertCustomError } = require('../helpers/customError'); const Address = artifacts.require('$Address'); -const AddressFnPointerMock = artifacts.require('$AddressFnPointerMock'); const EtherReceiver = artifacts.require('EtherReceiverMock'); const CallReceiverMock = artifacts.require('CallReceiverMock'); @@ -12,7 +11,6 @@ contract('Address', function (accounts) { beforeEach(async function () { this.mock = await Address.new(); - this.mockFnPointer = await AddressFnPointerMock.new(); }); describe('sendValue', function () { @@ -141,14 +139,6 @@ contract('Address', function (accounts) { await expectRevert.unspecified(this.mock.$functionCall(this.target.address, abiEncodedCall)); }); - it('bubbles up error if specified', async function () { - await expectRevertCustomError( - this.mockFnPointer.functionCall(this.target.address, '0x12345678'), - 'CustomRevert', - [], - ); - }); - it('reverts when function does not exist', async function () { const abiEncodedCall = web3.eth.abi.encodeFunctionCall( { @@ -254,14 +244,6 @@ contract('Address', function (accounts) { [], ); }); - - it('bubbles up error if specified', async function () { - await expectRevertCustomError( - this.mockFnPointer.functionCallWithValue(this.target.address, '0x12345678', 0), - 'CustomRevert', - [], - ); - }); }); }); @@ -305,14 +287,6 @@ contract('Address', function (accounts) { recipient, ]); }); - - it('bubbles up error if specified', async function () { - await expectRevertCustomError( - this.mockFnPointer.functionCallWithValue(this.target.address, '0x12345678', 0), - 'CustomRevert', - [], - ); - }); }); describe('functionDelegateCall', function () { @@ -355,28 +329,12 @@ contract('Address', function (accounts) { recipient, ]); }); - - it('bubbles up error if specified', async function () { - await expectRevertCustomError( - this.mockFnPointer.functionCallWithValue(this.target.address, '0x12345678', 0), - 'CustomRevert', - [], - ); - }); }); describe('verifyCallResult', function () { it('returns returndata on success', async function () { const returndata = '0x123abc'; - expect(await this.mockFnPointer.verifyCallResult(true, returndata)).to.equal(returndata); - }); - - it('reverts with return data and error', async function () { - await expectRevertCustomError(this.mockFnPointer.verifyCallResult(false, '0x'), 'CustomRevert', []); - }); - - it('reverts expecting error if provided onRevert is a non-reverting function', async function () { - await expectRevertCustomError(this.mockFnPointer.verifyCallResultVoid(false, '0x'), 'FailedInnerCall', []); + expect(await this.mock.$verifyCallResult(true, returndata)).to.equal(returndata); }); }); });