TransparentUpgradeableProxy clashing selector calls may not be delegated in @openzeppelin/contracts

@@ -1,5 +1,10 @@
 # Changelog
 
+## 4.8.3 (2023-04-13)
+
+- `GovernorCompatibilityBravo`: Fix encoding of proposal data when signatures are missing.
+- `TransparentUpgradeableProxy`: Fix transparency in case of selector clash with non-decodable calldata or payable mutability. ([#4154](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4154))
+
 ## 4.8.2 (2023-03-02)
 
 - `ERC721Consecutive`: Fixed a bug when `_mintConsecutive` is used for batches of size 1 that could lead to balance overflow. Refer to the breaking changes section in the changelog for a note on the behavior of `ERC721._beforeTokenTransfer`.

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-// OpenZeppelin Contracts (last updated v4.8.0) (governance/compatibility/GovernorCompatibilityBravo.sol)
+// OpenZeppelin Contracts (last updated v4.8.3) (governance/compatibility/GovernorCompatibilityBravo.sol)
 
 pragma solidity ^0.8.0;
 
@@ -75,6 +75,11 @@ abstract contract GovernorCompatibilityBravoUpgradeable is Initializable, IGover
         bytes[] memory calldatas,
         string memory description
     ) public virtual override returns (uint256) {
+        require(signatures.length == calldatas.length, "GovernorBravo: invalid signatures length");
+        // Stores the full proposal and fallback to the public (possibly overridden) propose. The fallback is done
+        // after the full proposal is stored, so the store operation included in the fallback will be skipped. Here we
+        // call `propose` and not `super.propose` to make sure if a child contract override `propose`, whatever code
+        // is added their is also executed when calling this alternative interface.
         _storeProposal(_msgSender(), targets, values, signatures, calldatas, description);
         return propose(targets, values, _encodeCalldata(signatures, calldatas), description);
     }
@@ -130,8 +135,7 @@ abstract contract GovernorCompatibilityBravoUpgradeable is Initializable, IGover
         returns (bytes[] memory)
     {
         bytes[] memory fullcalldatas = new bytes[](calldatas.length);
-
-        for (uint256 i = 0; i < signatures.length; ++i) {
+        for (uint256 i = 0; i < fullcalldatas.length; ++i) {
             fullcalldatas[i] = bytes(signatures[i]).length == 0
                 ? calldatas[i]
                 : abi.encodePacked(bytes4(keccak256(bytes(signatures[i]))), calldatas[i]);

@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: MIT
+// OpenZeppelin Contracts (last updated v4.8.3) (interfaces/IERC1967.sol)
+
+pragma solidity ^0.8.0;
+
+/**
+ * @dev ERC-1967: Proxy Storage Slots. This interface contains the events defined in the ERC.
+ *
+ * _Available since v4.9._
+ */
+interface IERC1967Upgradeable {
+    /**
+     * @dev Emitted when the implementation is upgraded.
+     */
+    event Upgraded(address indexed implementation);
+
+    /**
+     * @dev Emitted when the admin account has changed.
+     */
+    event AdminChanged(address previousAdmin, address newAdmin);
+
+    /**
+     * @dev Emitted when the beacon is changed.
+     */
+    event BeaconUpgraded(address indexed beacon);
+}

@@ -4,17 +4,16 @@ pragma solidity ^0.8.0;
 import "../proxy/utils/Initializable.sol";
 
 /**
- * @dev Implementation contract with an admin() function made to clash with
- * @dev TransparentUpgradeableProxy's to test correct functioning of the
- * @dev Transparent Proxy feature.
+ * @dev Implementation contract with a payable admin() function made to clash with TransparentUpgradeableProxy's to
+ * test correct functioning of the Transparent Proxy feature.
  */
 contract ClashingImplementationUpgradeable is Initializable {
     function __ClashingImplementation_init() internal onlyInitializing {
     }
 
     function __ClashingImplementation_init_unchained() internal onlyInitializing {
     }
-    function admin() external pure returns (address) {
+    function admin() external payable returns (address) {
         return 0x0000000000000000000000000000000011111142;
     }
 

@@ -1,7 +1,7 @@
 {
   "name": "@openzeppelin/contracts-upgradeable",
   "description": "Secure Smart Contract library for Solidity",
-  "version": "4.8.2",
+  "version": "4.8.3",
   "files": [
     "**/*.sol",
     "/build/contracts/*.json",

@@ -1,9 +1,10 @@
 // SPDX-License-Identifier: MIT
-// OpenZeppelin Contracts (last updated v4.5.0) (proxy/ERC1967/ERC1967Upgrade.sol)
+// OpenZeppelin Contracts (last updated v4.8.3) (proxy/ERC1967/ERC1967Upgrade.sol)
 
 pragma solidity ^0.8.2;
 
 import "../beacon/IBeaconUpgradeable.sol";
+import "../../interfaces/IERC1967Upgradeable.sol";
 import "../../interfaces/draft-IERC1822Upgradeable.sol";
 import "../../utils/AddressUpgradeable.sol";
 import "../../utils/StorageSlotUpgradeable.sol";
@@ -17,7 +18,7 @@ import "../utils/Initializable.sol";
  *
  * @custom:oz-upgrades-unsafe-allow delegatecall
  */
-abstract contract ERC1967UpgradeUpgradeable is Initializable {
+abstract contract ERC1967UpgradeUpgradeable is Initializable, IERC1967Upgradeable {
     function __ERC1967Upgrade_init() internal onlyInitializing {
     }
 
@@ -33,11 +34,6 @@ abstract contract ERC1967UpgradeUpgradeable is Initializable {
      */
     bytes32 internal constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
 
-    /**
-     * @dev Emitted when the implementation is upgraded.
-     */
-    event Upgraded(address indexed implementation);
-
     /**
      * @dev Returns the current implementation address.
      */
@@ -111,11 +107,6 @@ abstract contract ERC1967UpgradeUpgradeable is Initializable {
      */
     bytes32 internal constant _ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;
 
-    /**
-     * @dev Emitted when the admin account has changed.
-     */
-    event AdminChanged(address previousAdmin, address newAdmin);
-
     /**
      * @dev Returns the current admin.
      */
@@ -147,11 +138,6 @@ abstract contract ERC1967UpgradeUpgradeable is Initializable {
      */
     bytes32 internal constant _BEACON_SLOT = 0xa3f0ad74e5423aebfd80d3ef4346578335a9a72aeaee59ff6cb3582b35133d50;
 
-    /**
-     * @dev Emitted when the beacon is upgraded.
-     */
-    event BeaconUpgraded(address indexed beacon);
-
     /**
      * @dev Returns the current beacon.
      */

@@ -56,6 +56,8 @@ The current implementation of this security mechanism uses https://eips.ethereum
 
 == ERC1967
 
+{{IERC1967}}
+
 {{ERC1967Proxy}}
 
 {{ERC1967Upgrade}}

@@ -1,3 +1,4 @@
 [submodule "lib/forge-std"]
+	branch = v1
 	path = lib/forge-std
 	url = https://github.com/foundry-rs/forge-std

@@ -1 +1 @@
-Subproject commit eb980e1d4f0e8173ec27da77297ae411840c8ccb
+Subproject commit c2236853aadb8e2d9909bbecdc490099519b70a4

@@ -2,7 +2,7 @@
   "private": true,
   "name": "openzeppelin-solidity",
   "description": "Secure Smart Contract library for Solidity",
-  "version": "4.8.2",
+  "version": "4.8.3",
   "files": [
     "/contracts/**/*.sol",
     "/build/contracts/*.json",

@@ -1,12 +1,12 @@
 {
   "name": "openzeppelin-solidity",
-  "version": "4.8.2",
+  "version": "4.8.3",
   "lockfileVersion": 2,
   "requires": true,
   "packages": {
     "": {
       "name": "openzeppelin-solidity",
-      "version": "4.8.2",
+      "version": "4.8.3",
       "license": "MIT",
       "bin": {
         "openzeppelin-contracts-migrate-imports": "scripts/migrate-imports.js"

@@ -223,6 +223,21 @@ contract('GovernorCompatibilityBravo', function (accounts) {
     );
   });
 
+  it('with inconsistent array size for selector and arguments', async function () {
+    const target = this.receiver.address;
+    this.helper.setProposal(
+      {
+        targets: [target, target],
+        values: [0, 0],
+        signatures: ['mockFunction()'], // One signature
+        data: ['0x', this.receiver.contract.methods.mockFunctionWithArgs(17, 42).encodeABI()], // Two data entries
+      },
+      '<proposal description>',
+    );
+
+    await expectRevert(this.helper.propose({ from: proposer }), 'GovernorBravo: invalid signatures length');
+  });
+
   describe('should revert', function () {
     describe('on propose', function () {
       it('if proposal does not meet proposalThreshold', async function () {

@@ -6,6 +6,7 @@ const ImplV1 = artifacts.require('DummyImplementation');
 const ImplV2 = artifacts.require('DummyImplementationV2');
 const ProxyAdmin = artifacts.require('ProxyAdmin');
 const TransparentUpgradeableProxy = artifacts.require('TransparentUpgradeableProxy');
+const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableProxy');
 
 contract('ProxyAdmin', function (accounts) {
   const [proxyAdminOwner, newAdmin, anotherAccount] = accounts;
@@ -18,12 +19,13 @@ contract('ProxyAdmin', function (accounts) {
   beforeEach(async function () {
     const initializeData = Buffer.from('');
     this.proxyAdmin = await ProxyAdmin.new({ from: proxyAdminOwner });
-    this.proxy = await TransparentUpgradeableProxy.new(
+    const proxy = await TransparentUpgradeableProxy.new(
       this.implementationV1.address,
       this.proxyAdmin.address,
       initializeData,
       { from: proxyAdminOwner },
     );
+    this.proxy = await ITransparentUpgradeableProxy.at(proxy.address);
   });
 
   it('has an owner', async function () {

@@ -34,7 +34,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
 
   describe('implementation', function () {
     it('returns the current implementation address', async function () {
-      const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
+      const implementation = await this.proxy.implementation({ from: proxyAdminAddress });
 
       expect(implementation).to.be.equal(this.implementationV0);
     });
@@ -55,7 +55,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
         it('upgrades to the requested implementation', async function () {
           await this.proxy.upgradeTo(this.implementationV1, { from });
 
-          const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
+          const implementation = await this.proxy.implementation({ from: proxyAdminAddress });
           expect(implementation).to.be.equal(this.implementationV1);
         });
 
@@ -108,7 +108,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
           });
 
           it('upgrades to the requested implementation', async function () {
-            const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
+            const implementation = await this.proxy.implementation({ from: proxyAdminAddress });
             expect(implementation).to.be.equal(this.behavior.address);
           });
 
@@ -173,7 +173,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
           });
 
           it('upgrades to the requested version and emits an event', async function () {
-            const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
+            const implementation = await this.proxy.implementation({ from: proxyAdminAddress });
             expect(implementation).to.be.equal(this.behaviorV1.address);
             expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV1.address });
           });
@@ -199,7 +199,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
             });
 
             it('upgrades to the requested version and emits an event', async function () {
-              const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
+              const implementation = await this.proxy.implementation({ from: proxyAdminAddress });
               expect(implementation).to.be.equal(this.behaviorV2.address);
               expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV2.address });
             });
@@ -228,7 +228,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
               });
 
               it('upgrades to the requested version and emits an event', async function () {
-                const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
+                const implementation = await this.proxy.implementation({ from: proxyAdminAddress });
                 expect(implementation).to.be.equal(this.behaviorV3.address);
                 expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV3.address });
               });
@@ -274,7 +274,7 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
         });
 
         it('assigns new proxy admin', async function () {
-          const newProxyAdmin = await this.proxy.admin.call({ from: newAdmin });
+          const newProxyAdmin = await this.proxy.admin({ from: newAdmin });
           expect(newProxyAdmin).to.be.equal(anotherAccount);
         });
 
@@ -333,14 +333,23 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
       );
     });
 
-    context('when function names clash', function () {
+    describe('when function names clash', function () {
       it('when sender is proxy admin should run the proxy function', async function () {
-        const value = await this.proxy.admin.call({ from: proxyAdminAddress });
+        const value = await this.proxy.admin({ from: proxyAdminAddress, value: 0 });
         expect(value).to.be.equal(proxyAdminAddress);
       });
 
       it('when sender is other should delegate to implementation', async function () {
-        const value = await this.proxy.admin.call({ from: anotherAccount });
+        const value = await this.proxy.admin({ from: anotherAccount, value: 0 });
+        expect(value).to.be.equal('0x0000000000000000000000000000000011111142');
+      });
+
+      it('when sender is proxy admin value should not be accepted', async function () {
+        await expectRevert.unspecified(this.proxy.admin({ from: proxyAdminAddress, value: 1 }));
+      });
+
+      it('when sender is other value should be accepted', async function () {
+        const value = await this.proxy.admin({ from: anotherAccount, value: 1 });
         expect(value).to.be.equal('0x0000000000000000000000000000000011111142');
       });
     });

@@ -2,12 +2,14 @@ const shouldBehaveLikeProxy = require('../Proxy.behaviour');
 const shouldBehaveLikeTransparentUpgradeableProxy = require('./TransparentUpgradeableProxy.behaviour');
 
 const TransparentUpgradeableProxy = artifacts.require('TransparentUpgradeableProxy');
+const ITransparentUpgradeableProxy = artifacts.require('ITransparentUpgradeableProxy');
 
 contract('TransparentUpgradeableProxy', function (accounts) {
   const [proxyAdminAddress, proxyAdminOwner] = accounts;
 
   const createProxy = async function (logic, admin, initData, opts) {
-    return TransparentUpgradeableProxy.new(logic, admin, initData, opts);
+    const { address } = await TransparentUpgradeableProxy.new(logic, admin, initData, opts);
+    return ITransparentUpgradeableProxy.at(address);
   };
 
   shouldBehaveLikeProxy(createProxy, proxyAdminAddress, proxyAdminOwner);