Skip to content

Document fallback handler modules behavior#6422

Open
luiz-lvj wants to merge 4 commits intoOpenZeppelin:masterfrom
luiz-lvj:erc7579-fallback-handlers
Open

Document fallback handler modules behavior#6422
luiz-lvj wants to merge 4 commits intoOpenZeppelin:masterfrom
luiz-lvj:erc7579-fallback-handlers

Conversation

@luiz-lvj
Copy link
Copy Markdown
Contributor

Fixes L-05

Fallback handler modules can be reported as installed for selectors that will never reach fallback

Root Cause: _installModule stores arbitrary selector -> handler pairs without reachability checks; selectors handled by concrete functions or receive() bypass fallback, but isModuleInstalled still returns true.

Toy example:

Install a fallback module using a selector equal to execute(bytes32,bytes).
isModuleInstalled returns true for that selector.
Calling execute(...) routes to the concrete function, not to fallback(bytes), so the module never executes.
Location: draft-AccountERC7579.sol#L239-L276

AccountERC7579 supports the ERC-7579 fallback-handler module type by routing unknown calls through fallback(bytes) into _fallback(). Installed handlers are stored in the _fallbacks mapping keyed by selector and configured by _installModule.

For MODULE_TYPE_FALLBACK, _installModule accepts an arbitrary selector extracted from initData via _decodeFallbackData and stores it without checking whether that selector can ever reach fallback. Any selector colliding with a concrete public/external function (for example execute) will always be dispatched to that function by Solidity, making the installed fallback handler unreachable. In addition, since Account defines receive(), empty-calldata Ether transfers never reach fallback, so a handler installed for 0x00000000 cannot cover the receive() path. However, isModuleInstalled reports fallback modules as installed based only on _fallbacks[selector] == module, which can create a false signal for off-chain registries or policies relying on selector-level reachability.

Consider rejecting installation for selectors that are known to be implemented by the account itself, and consider disallowing 0x00000000 when receive() is present. Consider documenting that isModuleInstalled(MODULE_TYPE_FALLBACK, ...) reflects configuration state only and does not imply that calls to that selector will dispatch to fallback.

This PR adds the ERC7579InvalidFallbackSelector for the 0x00000000 selector and documents the behavior for the cases where the seletor is already present in the contract.

PR Checklist

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

@luiz-lvj luiz-lvj requested a review from a team as a code owner March 20, 2026 18:01
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 20, 2026

⚠️ No Changeset found

Latest commit: 3a2eadf

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
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 20, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 880c9296-8b4d-436f-bc51-a9ca3667fafb

📥 Commits

Reviewing files that changed from the base of the PR and between 45f032d and a9f6a18.

📒 Files selected for processing (3)
  • contracts/account/extensions/draft-AccountERC7579.sol
  • contracts/account/utils/draft-ERC7579Utils.sol
  • test/account/extensions/AccountERC7579.behavior.js

Walkthrough

This PR adds validation to the ERC7579 account abstraction module system to prevent installation of fallback modules with invalid (zero) selectors. A new custom error ERC7579InvalidFallbackSelector() is introduced to ERC7579Utils. The _installModule function for MODULE_TYPE_FALLBACK now checks that the decoded selector is non-zero before installation proceeds. Documentation is updated to clarify fallback handler behavior regarding function-selector collisions and reachability.

Possibly related PRs

Suggested labels

bug, ignore-changeset, Account Abstraction

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: documenting fallback handler modules behavior in response to L-05 issue.
Description check ✅ Passed The description is directly related to the changeset, clearly explaining the issue being addressed, the root cause, and the specific changes made.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Tip

CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.

Add a .trivyignore file to your project to customize which findings Trivy reports.

@gonzaotc
Copy link
Copy Markdown
Contributor

I think the comment is enough for this one and restricting the zero selector may be an overkill. The rationale is that the zero selector is as invalid as any other colisioning selector, as we cannot predict them. Wdyt? @ernestognw

@ernestognw
Copy link
Copy Markdown
Member

ernestognw commented Mar 25, 2026

I think the comment is enough for this one and restricting the zero selector may be an overkill. The rationale is that the zero selector is as invalid as any other colisioning selector, as we cannot predict them. Wdyt? @ernestognw

I agree the comment is enough. There are many functions identified for the 0x00000000 selector and I understand some of them are mined to purposely get the 0 selector. The reason is that empty calldata is cheaper to process. Although I don't have a use case in the context of accounts, I don't want to restrict anyone who wants to mine the zero selector for this very purpose.

@Amxx
Copy link
Copy Markdown
Collaborator

Amxx commented Mar 25, 2026

Not only would I not restrict the selector 0x00000000, but I would also consider overriding the receive() function (that is defined in Account.sol to something like

receive() external payable virtual override {
    if (_fallbackHandler(0x00000000) != address(0)) _fallback();
}

@luiz-lvj
Copy link
Copy Markdown
Contributor Author

@Amxx I don't think we should override the receive function. Since fallback is called even if the selector is 0x00000000 when the calldata is empty, then the selector will actually work. The only behavior that would not be supported is if the module is actually on the receive function. Do you think this is a possible expected behavior? We would have to come up with some way to differentiate the module with 0x00000000 as a selector and receive.

@luiz-lvj luiz-lvj requested review from Amxx and ernestognw March 27, 2026 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants