Implement recommendations from 5.0 audit Phase 1B

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