ERC7579: prevent installing/uninstalling without proper initData (#5974)

Co-authored-by: ernestognw <[email protected]>

Author: Amxx

CHANGELOG.md

@@ -11,6 +11,7 @@
 - `SignerERC7702` is renamed as `SignerEIP7702`. Imports and inheritance must be updated to that new name and path. Behavior is unmodified.
 - `ERC721Holder`, `ERC1155Holder`, `ReentrancyGuard` and `ReentrancyGuardTransient` are flagged as stateless and are no longer transpiled. Developers using their upgradeable variants from `@openzeppelin/contracts-upgradeable` must update their imports to use the equivalent version available in `@openzeppelin/contracts`.
 - Update minimum pragma to 0.8.24 in `Votes`, `VotesExtended`, `ERC20Votes`, `Strings`, `ERC1155URIStorage`, `MessageHashUtils`, `ERC721URIStorage`, `ERC721Votes`, `ERC721Wrapper`, `ERC721Burnable`, `ERC721Consecutive`, `ERC721Enumerable`, `ERC721Pausable`, `ERC721Royalty`, `ERC721Wrapper`, `EIP712`, `ERC4626` and `ERC7739`. ([#5726](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5726))
+- `AccountERC7579`: Installing and uninstalling fallback modules now require the corresponding `initData` and `deInitData` arguments to be at least 4 bytes long (matching the selector to which the fallback module is registered). It now reverts with `ERC7579CannotDecodeFallbackData` instead of treating the missing bytes as `0x00`.
 
 ### Deprecation
 

certora/specs/Account.spec

@@ -205,7 +205,7 @@ invariant absentExecutorIsNotStored(address module, uint256 index)
 */
 // This guarantees that at most one fallback module is active for a given initData (i.e. selector)
 rule fallbackModule(address module, bytes initData) {
-    assert isModuleInstalled(FALLBACK_TYPE(), module, initData) <=> getFallbackHandler(getDataSelector(initData)) == module;
+    assert isModuleInstalled(FALLBACK_TYPE(), module, initData) <=> (initData.length >= 4 && getFallbackHandler(getDataSelector(initData)) == module);
 }
 
 rule moduleManagementRule(env e, method f, calldataarg args, uint256 moduleTypeId, address module, bytes additionalContext)

contracts/account/extensions/draft-AccountERC7579.sol

@@ -68,6 +68,9 @@ abstract contract AccountERC7579 is Account, IERC1271, IERC7579Execution, IERC75
     /// @dev The account's {fallback} was called with a selector that doesn't have an installed handler.
     error ERC7579MissingFallbackHandler(bytes4 selector);
 
+    /// @dev The provided initData/deInitData for a fallback module is too short to extract a selector.
+    error ERC7579CannotDecodeFallbackData();
+
     /// @dev Modifier that checks if the caller is an installed module of the given type.
     modifier onlyModule(uint256 moduleTypeId, bytes calldata additionalContext) {
         _checkModule(moduleTypeId, msg.sender, additionalContext);
@@ -380,11 +383,13 @@ abstract contract AccountERC7579 is Account, IERC1271, IERC7579Execution, IERC75
      * https://github.com/erc7579/erc7579-implementation/blob/16138d1afd4e9711f6c1425133538837bd7787b5/src/MSAAdvanced.sol#L296[ERC7579 reference implementation].
      *
      * This is not standardized in ERC-7579 (or in any follow-up ERC). Some accounts may want to override these internal functions.
+     *
+     * NOTE: This function expects the signature to be at least 20 bytes long. Panics with {Panic-ARRAY_OUT_OF_BOUNDS} (0x32) otherwise.
      */
     function _extractSignatureValidator(
         bytes calldata signature
     ) internal pure virtual returns (address module, bytes calldata innerSignature) {
-        return (address(bytes20(signature[0:20])), signature[20:]);
+        return (address(bytes20(signature)), signature[20:]);
     }
 
     /**
@@ -399,6 +404,7 @@ abstract contract AccountERC7579 is Account, IERC1271, IERC7579Execution, IERC75
     function _decodeFallbackData(
         bytes memory data
     ) internal pure virtual returns (bytes4 selector, bytes memory remaining) {
+        require(data.length > 3, ERC7579CannotDecodeFallbackData());
         return (bytes4(data), data.slice(4));
     }
 

test/account/extensions/AccountERC7579.behavior.js

@@ -167,6 +167,17 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) {
         });
       }
 
+      it('should revert when installing a fallback module with an initData that is not long enough to encode a function selector', async function () {
+        const instance = this.modules[MODULE_TYPE_FALLBACK];
+        await expect(
+          this.mockFromEntrypoint.installModule(MODULE_TYPE_FALLBACK, instance, '0x'),
+        ).to.be.revertedWithCustomError(this.mock, 'ERC7579CannotDecodeFallbackData');
+
+        await expect(
+          this.mockFromEntrypoint.installModule(MODULE_TYPE_FALLBACK, instance, '0x123456'),
+        ).to.be.revertedWithCustomError(this.mock, 'ERC7579CannotDecodeFallbackData');
+      });
+
       withHooks &&
         describe('with hook', function () {
           beforeEach(async function () {
@@ -240,6 +251,17 @@ function shouldBehaveLikeAccountERC7579({ withHooks = false } = {}) {
         });
       }
 
+      it('should revert when uninstalling a fallback module with an initData that is not long enough to encode a function selector', async function () {
+        const instance = this.modules[MODULE_TYPE_FALLBACK];
+        await expect(
+          this.mockFromEntrypoint.uninstallModule(MODULE_TYPE_FALLBACK, instance, '0x'),
+        ).to.be.revertedWithCustomError(this.mock, 'ERC7579CannotDecodeFallbackData');
+
+        await expect(
+          this.mockFromEntrypoint.uninstallModule(MODULE_TYPE_FALLBACK, instance, '0x123456'),
+        ).to.be.revertedWithCustomError(this.mock, 'ERC7579CannotDecodeFallbackData');
+      });
+
       it('should revert uninstalling a module of type MODULE_TYPE_FALLBACK if a different module was installed for the provided selector', async function () {
         const instance = this.modules[MODULE_TYPE_FALLBACK];
         const anotherInstance = await ethers.deployContract('$ERC7579ModuleMock', [MODULE_TYPE_FALLBACK]);