feat(protocol): modularize the inbox by adding hooks and native eth bond support

[AnshuJalan]

This PR refactors the inbox to add some hooks that we found useful to make the Inbox modular. Using these hooks, developers of rollup protocols can extend the inbox and add custom features without having to modify the base contract.

Examples of such custom features (from Surge repo) are linked below:

• Finality Gadget: An extension to detect proof conflicts are allow forced upgrades of verifiers.
• Chain Rollback: An extension that allows rolling back the chain incase finalizations have come to a halt due to a prover bug.

Developers can also "chain" multiple features in a single inbox like this: https://github.com/NethermindEth/surge-taiko-mono/blob/v3.0.0-deployment-scripts/packages/protocol/contracts/layer1/surge/deployments/internal-devnet/SurgeInbox.sol

Additionally, this PR also adds the support for using native eth as bonds, enabled by allowing _bondToken to be a address(0)

[ggonzalez94]

I like this. I think the idea is clean and pretty helpful.

I left a couple comments on abstractions I believe we can improve, and will submit a small PR for an idea of how to improve one of them.

If we go this route, I think we should consider what other things make sense to make possible to override safely.

• the whole activation flow. New chains should not have a need for this, and even existing chains will likely want to deal with it differently.
• Event emission? other chains might require a different event structure

[claude]

Claude finished @AnshuJalan's task in 2m 0s —— View job

---

Code Review Complete

Summary

This PR successfully modularizes the Inbox contract through hooks and adds native ETH bond support. The implementation is generally solid with good test coverage. I've identified several security considerations and potential improvements below.

---

Positive Aspects

1. Clean hook pattern: The before/after hooks (_beforePropose, _afterPropose, _beforeProve, _afterProve) provide clean extension points without compromising core logic
2. Good PR feat(protocol): use a different hook for forced inclusions #21128 integration: The _buildForcedInclusions hook is well-designed and addresses previous reviewer concerns about overriding _scheduleBlocks
3. Comprehensive test coverage: InboxBondManagerNativeETH.t.sol provides excellent coverage for native ETH bonds with 17 test cases
4. Minimal gas impact: Gas increases are negligible (27-154 gas per operation), acceptable for the added modularity

---

Security Concerns

1. Reentrancy Risk with Native ETH (Medium Severity)

Location: LibBonds.sol:104 and Inbox.sol:677

Native ETH transfers via sendEtherAndVerify can trigger external code execution. While nonReentrant modifiers protect the public entry points (deposit, withdraw, propose, prove), the internal _dequeueAndProcessForcedInclusions at line 677 sends ETH without reentrancy protection:

_feeRecipient.sendEtherAndVerify(totalFees * 1 gwei);

If _feeRecipient is msg.sender (the proposer), a malicious contract could potentially exploit this during the _propose execution flow before state updates complete.

Recommendation:

• Move forced inclusion fee transfers to after all state updates (consider using a pull-payment pattern)
• Or ensure _feeRecipient transfers happen after _coreState updates at line 426-428

2. msg.value Validation Gap (Low Severity)

Location: Inbox.sol:244-255

The deposit and depositTo functions are marked payable but only validate msg.value inside LibBonds.deposit. If _bondToken is accidentally misconfigured (non-zero when it should be zero), users could lose ETH sent with the transaction.

function deposit(uint64 _amount) external payable nonReentrant {
    _bondStorage.deposit(IERC20(_bondToken), msg.sender, msg.sender, _amount, true);
}

Recommendation: Add explicit validation at the function level:

if (_bondToken == address(0)) {
    require(msg.value > 0, "ETH required");
} else {
    require(msg.value == 0, "No ETH expected for ERC20 bonds");
}

3. Virtual Function Security (Medium Severity)

Location: Inbox.sol:841-852

Making _handleProofVerification virtual allows derived contracts to completely bypass proof verification. While this enables the custom verifier use case mentioned in comments, it's a significant security risk.

function _handleProofVerification(...) internal view virtual {
    IProofVerifier(_proofVerifier)
        .verifyProof(_proposalAge, LibHashOptimized.hashCommitment(_commitment), _proof);
}

Concerns:

• A malicious or buggy override could skip verification entirely
• No way to enforce that verification actually occurred
• State has already been updated (line 509) before verification (line 522)

Recommendations:

1. Consider the approach from review comments: Make IVerifier interface more flexible with bytes params rather than making verification overridable
2. If virtual is necessary, add a post-verification hook instead: _afterProofVerification(bool verified) where base contract ensures verified == true
3. Document this security implication prominently in code comments and audit reports

---

Code Quality Issues

4. Inconsistent Type Casting

Location: Inbox.sol:58, 70, 146, 150, 245, 250, 255

_proofVerifier changed from typed IProofVerifier to address, and _bondToken from IERC20 to address. However, the code casts back to types at usage:

address internal immutable _proofVerifier;  // Line 58
// ...
IProofVerifier(_proofVerifier).verifyProof(...)  // Line 850

This loses compile-time type safety for unclear benefit.

Recommendation: Keep strong typing unless there's a specific reason. If address storage is needed for flexibility, document why.

5. Missing NatSpec for Virtual Functions

Location: Inbox.sol:807-852

Virtual/internal functions like _buildForcedInclusions and _handleProofVerification lack proper NatSpec documentation explaining:

• When/why to override
• Security considerations
• Expected behavior contracts

Per CLAUDE.md Solidity NatSpec requirements, internal functions should have @dev, @param, and @return tags.

Example needed:

/// @dev Handles proof verification by delegating to the proof verifier contract.
/// @dev WARNING: Overriding this function can bypass proof verification entirely.
///      Only override if you fully understand the security implications.
/// @param _proposalAge Seconds passed since the proposal was submitted
/// @param _commitment The commitment containing the batch transitions to verify
/// @param _proof The encoded proof data to verify against the commitment
function _handleProofVerification(...) internal view virtual { ... }

6. Confusing Function Naming (Minor)

Location: Inbox.sol:825-835

_buildForcedInclusions doesn't "build" forced inclusions—it populates a pre-allocated array with consumed queue items. Consider renaming to _populateForcedInclusions or _consumeAndPopulateForcedInclusions for clarity.

---

Gas Optimization

Gas increases are minimal (27-154 gas), which is acceptable. The hook indirection adds one internal call overhead per operation. No optimization needed unless gas becomes critical.

---

Test Coverage Assessment

Excellent: The new InboxBondManagerNativeETH.t.sol file comprehensively tests:

• ✅ Deposit/withdraw with ETH
• ✅ msg.value validation
• ✅ Edge cases (zero address, insufficient ETH)
• ✅ Withdrawal delays and cancellation
• ✅ Config validation

Missing: No tests for the new hook functions. Consider adding tests for contracts that override hooks to ensure the extension mechanism works as intended.

---

Design Feedback

The hook pattern is clean and the forced inclusion abstraction from PR #21128 addressed previous concerns well. However, making _handleProofVerification virtual is concerning from a security standpoint. Consider:

1. Alternative approach: Use a strategy pattern with a registered verifier interface that must be set by governance, rather than allowing arbitrary override
2. Safety net: Add an immutable bool requireVerification flag that cannot be disabled in production deployments

---

Recommendations Priority

High Priority:

1. Fix reentrancy risk with forced inclusion fee transfers (Security Redesign the rollup protocol with new zkEVM proof assumptions #1)
2. Address virtual proof verification security concern (Security refactor based on team feedback #3)
3. Add proper NatSpec documentation for virtual functions (Quality Impl LibTxList.sol #5)

Medium Priority:
4. Add msg.value validation at function level (Security #2)
5. Add tests for hook overrides
6. Consider reverting type changes unless there's a documented reason (Quality #4)

Low Priority:
7. Improve function naming (Quality #6)

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ac21b3a6de

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[ggonzalez94]

Looks great! Left a few minor nits, and a suggestion for how to deal with the verifier without needing to make that function virtual

[AnshuJalan]

Marked this as do.not.merge for now, since there are a couple more developments on our end regarding modularization. I'll get back to you on this.

CC: @ggonzalez94

[ggonzalez94]

Marked this as do.not.merge for now, since there are a couple more developments on our end regarding modularization. I'll get back to you on this.

CC: @ggonzalez94

Sounds good. If they are strictly additions of top I would still try to merge this so that the PR does not become too big. Also let's not forget to go back using gwei for the bond, since wei is too small and it would break our shasta deployment

[AnshuJalan]

Yeah, so I am reverting to wei and not making any new changes in this PR. Will raise a separate one after a discussion

[AnshuJalan]

@ggonzalez94 Need your re-approval