ERC7579: prevent installing/uninstalling without proper initData

[Amxx]

Followup to #5961

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

⚠️ No Changeset found

Latest commit: 6c8aaca

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[coderabbitai]

Walkthrough

• Certora spec: fallbackModule rule now guards selector comparison with initData.length >= 4 before using getDataSelector(initData).
• Contract draft-AccountERC7579.sol: adds errors ERC7579CannotDecodeFallbackData and ERC7579InvalidModuleSignature. Enforces signature length > 19 in _extractSignatureValidator(), and data length > 3 in _decodeFallbackData(); both revert with the new errors on failure.
• Tests: add cases ensuring installing/uninstalling a fallback module reverts with ERC7579CannotDecodeFallbackData when initData is too short (e.g., 0x, 0x123456).

Possibly related PRs

• Prevent revert in isModuleInstalled for fallback modules on short additionalContext #5961: Adds guards to prevent reading 4-byte selectors from too-short calldata/context, aligning with the new fallback decoding checks.

Suggested labels

ignore-changeset

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title clearly and concisely describes the main change by specifying that ERC7579 fallback module installation and uninstallation now require valid initData. It focuses on the primary prevention of operations without proper initData and omits extraneous details. It's specific enough for team members to understand the intent at a glance.
Description Check	✅ Passed	The description references the related PR and includes a checklist indicating that tests have been added, which aligns with the changeset’s focus on error conditions and coverage. Though brief, it is directly related to the pull request’s purpose of preventing invalid fallback module operations. Therefore, it is relevant and passes the lenient criteria.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

test/account/extensions/AccountERC7579.behavior.js (1)

170-263: Consider adding tests for signature validation with short signatures.

While the test coverage for fallback module data validation is thorough, consider adding tests for isValidSignature with signatures shorter than 20 bytes to fully exercise the new ERC7579InvalidModuleSignature error path in _extractSignatureValidator.

Example test:

it('should handle short signatures gracefully in isValidSignature', async function () {
  const hash = ethers.randomBytes(32);
  const shortSig = '0x1234'; // Less than 20 bytes
  
  // Should return 0xffffffff (invalid signature magic value)
  await expect(this.mock.isValidSignature(hash, shortSig))
    .to.eventually.equal('0xffffffff');
});

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 765b288 and 6c8aaca.

📒 Files selected for processing (3)

• certora/specs/Account.spec (1 hunks)
• contracts/account/extensions/draft-AccountERC7579.sol (3 hunks)
• test/account/extensions/AccountERC7579.behavior.js (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

test/account/extensions/AccountERC7579.behavior.js (1)

test/helpers/erc7579.js (1)

• MODULE_TYPE_FALLBACK (5-5)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: tests-foundry
• GitHub Check: tests-upgradeable
• GitHub Check: slither
• GitHub Check: coverage
• GitHub Check: tests
• GitHub Check: halmos

🔇 Additional comments (6)

contracts/account/extensions/draft-AccountERC7579.sol (3)

71-75: LGTM! Clear error declarations for input validation.

The new error declarations are well-documented and appropriately scoped:

• ERC7579CannotDecodeFallbackData: Used when initData/deInitData is too short to extract a 4-byte selector
• ERC7579InvalidModuleSignature: Used when signature is too short to extract a 20-byte module address

---

393-394: LGTM! Defensive input validation for signature parsing.

The length check ensures at least 20 bytes are available before extracting address(bytes20(signature)). While isValidSignature already guards this call (line 192), the added check makes _extractSignatureValidator more robust since it's internal virtual and could be called from derived contracts without the outer guard.

---

409-410: LGTM! Essential input validation for fallback data decoding.

The check prevents attempting to extract a selector from data shorter than 4 bytes. Without this guard, bytes4(data) would pad with zeros (yielding an incorrect selector) and data.slice(4) would cause an out-of-bounds access. The explicit validation provides a clear error message.

certora/specs/Account.spec (1)

208-208: LGTM! Spec correctly guards selector extraction.

The updated assertion now requires initData.length >= 4 before evaluating getDataSelector(initData), aligning with the contract's new validation in _decodeFallbackData. This prevents undefined behavior in formal verification when initData is too short to contain a selector.

test/account/extensions/AccountERC7579.behavior.js (2)

170-179: LGTM! Comprehensive test coverage for installation validation.

The test correctly verifies that installing a fallback module with initData shorter than 4 bytes (both empty 0x and 3-byte 0x123456) reverts with ERC7579CannotDecodeFallbackData, ensuring the new validation logic works as expected.

---

254-263: LGTM! Comprehensive test coverage for uninstallation validation.

The test correctly verifies that uninstalling a fallback module with initData shorter than 4 bytes reverts with ERC7579CannotDecodeFallbackData, providing symmetric coverage with the installation tests.