diff --git a/CHANGELOG.md b/CHANGELOG.md index a9955665a2c..42c44fa5666 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/certora/specs/Account.spec b/certora/specs/Account.spec index 638cdc838dd..941621e6e58 100644 --- a/certora/specs/Account.spec +++ b/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) diff --git a/contracts/account/extensions/draft-AccountERC7579.sol b/contracts/account/extensions/draft-AccountERC7579.sol index 41ebf2fed20..98f877aaeb9 100644 --- a/contracts/account/extensions/draft-AccountERC7579.sol +++ b/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,6 +383,8 @@ 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 @@ -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)); } diff --git a/test/account/extensions/AccountERC7579.behavior.js b/test/account/extensions/AccountERC7579.behavior.js index d531db7e046..e02fdec58d0 100644 --- a/test/account/extensions/AccountERC7579.behavior.js +++ b/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]);