Implement recommendations from 5.0 audit Phase 1B (#4502)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco Giordano <fg@frang.io>

.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.

.changeset/hip-goats-fail.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`VestingWallet`: Fix revert during 1 second time window when duration is 0.

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();

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];

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.
      *

contracts/mocks/AddressFnPointersMock.sol

@@ -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 {}
-}

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;
+    }
+}

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());
+    }
+}

contracts/mocks/UpgreadeableBeaconMock.sol

@@ -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;
-    }
-}

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();
     }
 }

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) {

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 {}
 }

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);
 }

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);
     }
 }

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";

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();

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.
      */

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) {
-            return returndata;
+    function verifyCallResult(bool success, bytes memory returndata) internal pure returns (bytes memory) {
+        if (!success) {
+            _revert(returndata);
         } else {
-            _revert(returndata, customRevert);
+            return returndata;
         }
     }
 
     /**
-     * @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();
         }
     }

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) {

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 () {

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;

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;

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],
+          );
+        });
+      });
     });
   });
 });

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);
     });

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);
     });
   });
 });