feat: add beforeShip hook for native ETH wrapping

[deacix]

Add IShipHook interface and modify ship() to support native ETH:

• Add IShipHook interface with beforeShip() callback
• Make ship() payable to accept native ETH
• Call beforeShip hook on the app when ETH is sent
• App can wrap ETH to WETH and send to maker

This enables apps (like Fusion SwapVM) to handle native ETH in a single transaction: user sends ETH with ship(), app wraps it to WETH and sends to maker, then normal Aqua flow continues.

Flow:

1. User calls Aqua.ship{value: X}(app, strategy, [WETH,...], [X,...])
2. Aqua calls app.beforeShip{value: X}(maker, strategyHash, tokens, amounts)
3. App wraps ETH to WETH and transfers to maker
4. Aqua continues with normal ship flow (virtual balance tracking)
5. Aqua.pull() works normally (maker now has WETH)

Change Summary

What does this PR change?

Related Issue/Ticket:

Testing & Verification

How was this tested?

• Unit tests
• Integration tests
• Manual testing (describe steps)
• Verified on staging

Risk Assessment

Risk Level:

• Low - Minor changes, no operational impact
• Medium - Moderate changes, limited impact, standard rollback available
• High - Significant changes, potential operational impact, complex rollback

Risks & Impact

[claude]

Claude finished @deacix's task —— View job

---

Review of PR #39: Add beforeShip hook for native ETH wrapping

I've analyzed the changes and here's my comprehensive feedback:

Review Checklist:

• Read repository conventions (CLAUDE.md)
• Analyze code changes in all modified files
• Assess code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Identify security concerns
• Review test coverage

Code Quality and Best Practices ✅

The code follows good practices overall:

• Clear interface design with IShipHook for extensibility
• Proper use of modifiers and visibility specifiers
• Consistent error handling with custom errors
• Good documentation with NatSpec comments

Suggestions:

1. Consider adding more detailed documentation about the security implications of implementing IShipHook in the interface comments
2. The ShipHookFailed error is duplicated in both IAqua.sol:40-42 and Aqua.sol:41-44. Remove the duplicate from Aqua.sol

Potential Bugs or Issues ⚠️

1. Hook Failure Handling: The hook call at Aqua.sol:53 could revert for various reasons:

   • App doesn't implement IShipHook interface
   • Hook reverts internally
   • Gas exhaustion

   Consider using a try-catch pattern to provide better error context:

   try IShipHook(app).beforeShip{value: msg.value}(msg.sender, strategyHash, tokens, amounts) returns (bool success) {
       require(success, ShipHookFailed(app));
   } catch {
       revert ShipHookFailed(app);
   }

2. Interface Check: There's no verification that the app actually implements IShipHook before calling it when ETH is sent. This could lead to confusing reverts.

Performance Considerations 🔄

1. The hook adds an external call when ETH is sent, which increases gas costs. This is acceptable given the functionality benefits.
2. Consider documenting expected gas overhead for hook implementations to help integrators plan accordingly.

Security Concerns 🔒

Critical Issues:

1. Reentrancy Risk: The beforeShip hook is called before state changes (balance updates). This creates a reentrancy vector where a malicious app could:

   • Call back into ship() or other Aqua functions
   • Manipulate state before the original ship completes

   Recommendation: Move the hook call after the state changes, or add a reentrancy guard.

2. ETH Handling Trust: The design trusts apps to properly handle ETH. A malicious or buggy app could:

   • Keep the ETH instead of wrapping to WETH
   • Send WETH to wrong address
   • Not send the expected amount to the maker

   Recommendation: Consider implementing verification that the maker received the expected WETH amount.

3. No App Validation: Any address can be passed as app, and if ETH is sent, it will attempt to call beforeShip on it. This could lead to ETH being sent to contracts that don't handle it properly.

Test Coverage 🧪

The PR indicates test coverage is pending (all test checkboxes unchecked). This is a critical gap for a feature handling ETH.

Required Test Scenarios:

1. Happy path: ETH sent, hook wraps to WETH, ship completes
2. Hook returns false
3. Hook reverts
4. Reentrancy attempts during hook execution
5. App doesn't implement IShipHook interface
6. Zero ETH scenarios
7. Gas limit edge cases

Additional Recommendations

1. Events: Consider emitting an event when the hook is called successfully for better observability
2. Access Control: Document clearly that any app can implement this hook - ensure this aligns with the security model
3. Upgrade Path: Consider how existing apps will handle this change (backward compatibility is maintained ✅)

Summary

The implementation provides useful functionality for ETH wrapping, but has significant security concerns that should be addressed before merging:

• Must fix: Reentrancy protection
• Should fix: Better error handling and app validation
• Must have: Comprehensive test coverage

The core concept is sound, but the implementation needs security hardening before production use.

[claude]

Claude finished @deacix's task —— View job

---

Review of PR #39: Add beforeShip hook for native ETH wrapping

I've analyzed the changes and here's my comprehensive feedback:

Review Checklist:

• Read repository conventions (CLAUDE.md)
• Analyze code changes in all modified files
• Assess code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Identify security concerns
• Review test coverage

Code Quality and Best Practices ✅

The code follows good practices overall:

• Clear interface design with IShipHook for extensibility
• Proper use of modifiers and visibility specifiers
• Consistent error handling with custom errors
• Good documentation with NatSpec comments
• Well-structured hook system with flags for flexibility

Minor Issues:

1. The ShipHookFailed error is duplicated in both IAqua.sol:40-42 and Aqua.sol:48-50. Consider removing the duplicate from Aqua.sol since it's already defined in the interface.

Potential Bugs or Issues ⚠️

1. Hook Failure Handling: The hook call at Aqua.sol:74 could revert for various reasons:

   • App doesn't implement IShipHook interface
   • Hook reverts internally
   • Gas exhaustion

   Consider using a try-catch pattern to provide better error context:

   try IShipHook(app).beforeShip{value: msg.value}(maker, strategyHash, tokens, amounts) returns (bool success) {
       require(success, ShipHookFailed(app, HOOK_BEFORE));
   } catch {
       revert ShipHookFailed(app, HOOK_BEFORE);
   }

2. Interface Check: There's no verification that the app actually implements IShipHook before calling it when hooks are enabled. This could lead to confusing reverts.

Performance Considerations 🔄

1. The hook system adds external calls when hooks are enabled, which increases gas costs. This is acceptable given the functionality benefits.
2. The flag-based system is efficient - no overhead when hooks aren't used.
3. Consider documenting expected gas overhead for hook implementations to help integrators plan accordingly.

Security Concerns 🔒

Critical Issues:

1. Reentrancy Risk: The beforeShip hook is called at line 74 before state changes (balance updates at lines 86-90). This creates a reentrancy vector where a malicious app could:

   • Call back into ship() or other Aqua functions
   • Manipulate state before the original ship completes

   Recommendation: Move the hook call after the state changes, or add a reentrancy guard using OpenZeppelin's ReentrancyGuard.

2. ETH Handling Trust: The design trusts apps to properly handle ETH. A malicious or buggy app could:

   • Keep the ETH instead of wrapping to WETH
   • Send WETH to wrong address
   • Not send the expected amount to the maker

   Recommendation: Consider implementing verification that the maker received the expected WETH amount after the hook.

3. No App Validation: Any address can be passed as app, and if hooks are enabled, it will attempt to call hook methods on it. This could lead to unexpected behavior if the contract doesn't handle these calls properly.

4. afterShip Hook Risk: The afterShip hook (lines 94-101) is called after state changes, which is safer from a reentrancy perspective, but it can still perform external calls that might fail and cause the entire transaction to revert after balances are already updated in memory.

Test Coverage 🧪

The PR indicates test coverage is pending (all test checkboxes unchecked). This is a critical gap for a feature handling ETH and external calls.

Required Test Scenarios:

1. Happy path: ETH sent with HOOK_BEFORE flag, hook wraps to WETH, ship completes
2. Hook returns false
3. Hook reverts
4. ETH sent without HOOK_BEFORE flag (should revert)
5. Reentrancy attempts during hook execution
6. App doesn't implement IShipHook interface
7. Zero ETH scenarios with hooks
8. Gas limit edge cases
9. Both hooks enabled (HOOK_BOTH)
10. Only afterShip hook enabled

Additional Recommendations

1. Events: Consider emitting events when hooks are called successfully for better observability
2. Access Control: Document clearly that any app can implement these hooks - ensure this aligns with the security model
3. Backward Compatibility: Good job maintaining backward compatibility by making hooks optional via flags
4. Constants: Good use of hook flag constants for clarity

Summary

The implementation provides useful functionality for ETH wrapping and lifecycle hooks, but has significant security concerns that should be addressed before merging:

• Must fix: Reentrancy protection for beforeShip hook
• Should fix: Better error handling and app validation
• Must have: Comprehensive test coverage

The core concept and architecture are sound, but the implementation needs security hardening before production use.

[claude]

Claude finished @deacix's task —— View job

---

PR Review: Add beforeShip hook for native ETH wrapping

I've thoroughly analyzed the implementation. Here's my comprehensive feedback:

Review Checklist:

• Read repository conventions (CLAUDE.md)
• Analyze code changes in all modified files
• Assess code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Identify security concerns
• Review test coverage

Code Quality and Best Practices ✅

Strengths:

• Clean interface design with IShipHook for extensibility
• Good use of custom errors for gas efficiency
• Clear NatSpec documentation
• Proper use of constants and modifiers
• Follows established patterns in the codebase

Minor Issues:

1. Duplicate Error Declaration: ShipHookFailed error is defined in both IAqua.sol:40-43 and should be removed from one location (typically keep in interface only)
2. Hook Type Parameter: In IAqua.sol:42, the error uses uint8 hookType but the actual usage passes hook flag constants (HOOK_BEFORE=0x01, HOOK_AFTER=0x02) which might be confusing

Potential Bugs or Issues ⚠️

1. ETH Handling Edge Cases:

   • If msg.value > 0 but the app's beforeShip doesn't properly handle ETH, it could be locked
   • No validation that the app actually implements IShipHook before calling it

2. Hook Return Value:

   • The beforeShip hook returns bool success but there's no guidance on when to return false vs reverting
   • Consider if a try-catch pattern might be more appropriate for better error handling

3. Missing afterShip Error Handling:

   • The afterShip hook call (line 87) doesn't check for success/failure like beforeShip does
   • Should it also have error handling or is silent failure intended?

Performance Considerations 🔄

1. Gas Costs:

   • Hook calls add external calls which increase gas costs
   • The conditional checks for hooks are efficient using bitwise operations
   • Consider documenting expected gas overhead for integrators

2. Storage Access:

   • The implementation efficiently checks hook flags before making external calls
   • No unnecessary storage reads/writes

Security Concerns 🔒

Critical Issues:

1. Reentrancy Risk:

   • The beforeShip hook is called at line 66 BEFORE state changes (balance updates at lines 78-82)
   • This violates the checks-effects-interactions pattern and creates a reentrancy vector
   • Recommendation: Either move the hook call after state changes or add a reentrancy guard

2. Trust Assumptions:

   • The design trusts apps to properly handle ETH and wrap to WETH
   • No verification that the maker actually received the expected WETH
   • Consider adding a post-hook balance check for critical operations

3. Unchecked External Call:

   • The afterShip hook (line 87) is an unchecked external call that could fail silently
   • If this is intentional, document it clearly; otherwise, add error handling

4. Interface Implementation Check:

   • No verification that the app implements IShipHook before calling
   • Could use ERC-165 or a simple success check to validate

Test Coverage 🧪

Missing Tests - The PR indicates no tests have been written yet. Critical test scenarios needed:

1. Happy Path:

   • ETH sent with HOOK_BEFORE flag, properly wrapped to WETH
   • Both hooks called successfully
   • Only specific hooks called based on flags

2. Edge Cases:

   • ETH sent without HOOK_BEFORE flag (should revert)
   • Hook returns false
   • Hook reverts
   • App doesn't implement IShipHook
   • Zero ETH with HOOK_BEFORE flag

3. Security Tests:

   • Reentrancy attempts during hook execution
   • Gas griefing scenarios
   • Malicious hook implementations

Summary and Recommendations

The implementation provides valuable functionality for ETH handling, but has security concerns that must be addressed:

Must Fix Before Merging:

1. ⚠️ Reentrancy protection - Move hooks after state changes or add reentrancy guard
2. ⚠️ Comprehensive test coverage - This feature handles ETH and needs thorough testing
3. ⚠️ Error handling consistency - Decide on afterShip error handling approach

Should Consider:

1. Interface implementation validation
2. Post-hook balance verification for critical operations
3. Try-catch pattern for better error context
4. Gas overhead documentation

The core design is solid, but the implementation needs security hardening before production deployment.

[claude]

Claude finished @deacix's task —— View job

---

Code Review: Add beforeShip hook for native ETH wrapping

I've thoroughly analyzed this PR. Here are my findings:

Todo List:

• Read CLAUDE.md for repository conventions
• Analyze IShipHook.sol interface design
• Review IAqua.sol interface updates
• Analyze Aqua.sol implementation for security and correctness
• Review test coverage in Aqua.t.sol
• Review test updates in AquaStorageTest.t.sol
• Write comprehensive review feedback

Code Quality and Best Practices ✅

Strengths:

• Clean interface design with clear separation of concerns
• Well-documented with comprehensive NatSpec comments
• Good use of constants for hook flags (HOOK_BEFORE, HOOK_AFTER, HOOK_BOTH)
• Follows existing code patterns and conventions

Minor Issues:

1. Duplicate Error Declaration: The ShipHookFailed error is declared in both IAqua.sol:40-43 and Aqua.sol:48-50 (though I don't see it in the Aqua.sol file shown, so this may have been fixed)

Potential Bugs or Issues ⚠️

1. Missing Interface Validation: The implementation doesn't verify that the app actually implements IShipHook before calling hook methods. This could lead to confusing reverts on addresses that don't implement the interface.

2. afterShip Error Handling: The afterShip hook call at Aqua.sol:87 doesn't check return values or handle errors, unlike beforeShip which requires success. This inconsistency should be documented or made consistent.

3. Hook Failure Modes: The beforeShip hook returns a boolean for success, but there's no clear guidance on when to return false vs reverting. Consider documenting the expected behavior.

Performance Considerations 🔄

1. Gas Costs: Hook calls add external calls which increase gas costs. This is acceptable for the added functionality.
2. Efficient Flag Checks: Good use of bitwise operations for checking hook flags.
3. No Unnecessary Storage: The implementation efficiently avoids additional storage slots.

Security Concerns 🔒

Critical Issues:

1. Reentrancy Vulnerability: The beforeShip hook is called at line 66 before state changes (balance updates at lines 78-82). This violates the checks-effects-interactions pattern and creates a serious reentrancy risk. A malicious app could:

   • Call back into ship() or other Aqua functions
   • Manipulate state before the original ship completes

   Recommendation: Either move the hook call after state changes OR add OpenZeppelin's ReentrancyGuard.

2. Trust Model for ETH Handling: The design trusts apps to properly wrap ETH to WETH. There's no verification that:

   • The correct amount of WETH was sent to the maker
   • The app actually wrapped the ETH vs keeping it

   Recommendation: Consider adding a balance check to verify the maker received the expected WETH.

3. Unchecked External Call Risk: Any address can be passed as app, and if hooks are enabled, external calls will be made to it. This could lead to griefing or unexpected behavior.

Test Coverage 🧪

Critical Gap: The PR shows no test implementation for the new hook functionality. The test files have been updated to pass the new hooks parameter (with value 0), but there are no tests for:

1. Missing Test Scenarios:

   • ETH sent with HOOK_BEFORE flag (happy path)
   • ETH sent without HOOK_BEFORE flag (should revert)
   • Hook returns false
   • Hook reverts
   • Reentrancy attempts
   • Both hooks enabled (HOOK_BOTH)
   • Gas limit edge cases

2. Security Tests Needed:

   • Reentrancy protection validation
   • Malicious hook implementations
   • Interface validation failures

Summary and Recommendations

The feature design is solid and adds valuable functionality for ETH handling. However, there are critical security issues that must be addressed:

Must Fix Before Merging:

1. ⚠️ Reentrancy protection - This is a critical security vulnerability
2. ⚠️ Comprehensive test coverage - No tests exist for the new functionality
3. ⚠️ ETH validation - Add verification that ETH is properly handled

Should Consider:

1. Interface implementation checks before calling hooks
2. Consistent error handling between beforeShip and afterShip
3. Documentation of trust assumptions and security considerations

The implementation needs security hardening and thorough testing before it's ready for production use.

[claude]

Claude finished @deacix's task —— View job

---

PR Review: Add beforeShip hook for native ETH wrapping

I've thoroughly analyzed the implementation. Here's my comprehensive feedback:

Review Checklist:

• Read repository conventions (CLAUDE.md not found)
• Analyze IShipHook.sol interface design
• Review IAqua.sol interface updates
• Analyze Aqua.sol implementation for security and correctness
• Review test coverage in Aqua.t.sol
• Review test updates in AquaStorageTest.t.sol
• Compile comprehensive review feedback

Code Quality and Best Practices ✅

Strengths:

• Clean interface design with IShipHook for extensibility
• Good use of custom errors for gas efficiency
• Clear NatSpec documentation
• Proper use of constants and modifiers
• Follows established patterns in the codebase

Minor Issues:

1. Duplicate Error Declaration: ShipHookFailed error is defined in both IAqua.sol:40-43 and Aqua.sol:48-50. Remove the duplicate from Aqua.sol since it's already imported from the interface.

Potential Bugs or Issues ⚠️

1. ETH Handling Edge Cases:

   • No validation that the app actually implements IShipHook before calling it (line 66)
   • Could result in confusing reverts or locked ETH

2. Hook Return Value Semantic:

   • The beforeShip returns bool success but there's no clear guidance on when to return false vs reverting
   • Consider documenting the expected behavior

3. Inconsistent Error Handling:

   • beforeShip checks the return value and reverts on failure (line 72)
   • afterShip (line 87) has no error handling - is this intentional?

Performance Considerations 🔄

1. Gas Costs:

   • Hook calls add ~30-50k gas for external calls
   • The bitwise flag checks are efficient
   • No unnecessary storage operations
   • Gas snapshot shows reasonable increases

2. Optimization Opportunities:

   • Consider documenting expected gas overhead for hook implementations
   • The implementation is already well-optimized

Security Concerns 🔒

Critical Issues:

1. Reentrancy Risk ⚠️:

   • The beforeShip hook is called at line 66 BEFORE state changes (balance updates at lines 78-82)
   • This violates the checks-effects-interactions pattern and creates a reentrancy vector
   • A malicious app could:
     • Call back into ship() or other Aqua functions
     • Manipulate state before the original ship completes
   • Recommendation: Either:
     • Move the hook call after state changes, OR
     • Add OpenZeppelin's ReentrancyGuard modifier to ship()

2. Trust Assumptions:

   • The design trusts apps to properly handle ETH and wrap to WETH
   • No verification that the maker actually received the expected WETH
   • Consider adding balance checks after the hook if critical

3. Unchecked External Call:

   • The afterShip hook (line 87) could fail silently
   • If this is intentional for backwards compatibility, document it clearly

4. No Interface Validation:

   • No check if app implements IShipHook before calling
   • Could use ERC-165 or try-catch for safer calls

Test Coverage 🧪

Critical Gap: No tests for the new hook functionality!

The test files show:

• Existing tests updated to pass 0 for the hooks parameter
• No new tests for ETH handling, hook execution, or edge cases

Required Test Scenarios:

1. Happy path: ETH sent with HOOK_BEFORE, properly wrapped
2. ETH sent without HOOK_BEFORE flag (should revert)
3. Hook returns false
4. Hook reverts
5. Reentrancy attempts during hooks
6. App doesn't implement IShipHook
7. Both hooks enabled (HOOK_BOTH)
8. Gas limit edge cases

Summary and Recommendations

The implementation provides valuable ETH handling functionality, but has critical security issues:

Must Fix Before Merging:

1. ⚠️ Reentrancy protection - Add modifier or reorder operations
2. ⚠️ Comprehensive test coverage - No hook tests exist!
3. ⚠️ Remove duplicate error declaration in Aqua.sol:48-50

Should Consider:

1. Interface implementation validation
2. Consistent error handling for afterShip
3. Post-hook balance verification
4. Try-catch pattern for better UX

The core design is solid, but needs security hardening and thorough testing before production use.

[claude]

Claude finished @deacix's task —— View job

---

Code Review: Add beforeShip hook for native ETH wrapping

I've thoroughly analyzed this PR. Here's my comprehensive feedback:

Review Checklist:

• Read repository conventions (CLAUDE.md not found)
• Analyze interface design (IShipHook.sol)
• Review implementation in Aqua.sol
• Evaluate security considerations
• Assess test coverage
• Compile comprehensive feedback

Code Quality and Best Practices ✅

Strengths:

• Clean interface design with IShipHook for extensibility
• Good use of custom errors for gas efficiency
• Clear NatSpec documentation on both interfaces and implementation
• Proper use of constants for hook flags (HOOK_BEFORE, HOOK_AFTER, HOOK_BOTH)
• Follows established patterns in the codebase
• Well-structured tests with comprehensive coverage

Potential Bugs or Issues ⚠️

1. Missing Interface Validation: The implementation at Aqua.sol:66 doesn't verify that the app actually implements IShipHook before calling hook methods. This could lead to confusing reverts when an address that doesn't implement the interface is used with hooks enabled.

2. Inconsistent Error Handling:

   • beforeShip checks return value and reverts on failure (Aqua.sol:72)
   • afterShip (Aqua.sol:87) has no error handling - if this is intentional, it should be documented

Performance Considerations 🔄

1. Gas Costs: Hook calls add external calls which increase gas costs - this is acceptable for the added functionality
2. Efficient Implementation:
   • Bitwise flag operations are gas-efficient
   • No unnecessary storage operations
   • Gas snapshot shows reasonable increases

Security Concerns 🔒

Critical Issue: Reentrancy Risk ⚠️

The beforeShip hook is called at line 66 BEFORE state changes (balance updates at lines 78-82). This violates the checks-effects-interactions pattern and creates a serious reentrancy vulnerability. A malicious app could:

• Call back into ship() or other Aqua functions
• Manipulate state before the original ship completes
• Potentially drain funds or corrupt state

Recommendation: Add OpenZeppelin's ReentrancyGuard modifier to the ship() function, or move the hook call after state changes (though this would change the semantics).

Other Security Considerations:

1. Trust Model for ETH Handling: The design trusts apps to properly wrap ETH to WETH. There's no verification that:

   • The correct amount of WETH was sent to the maker
   • The app actually wrapped the ETH vs keeping it

   This is acceptable if apps are trusted/verified, but should be documented.

2. Unchecked External Call: Any address can be passed as app, and if hooks are enabled, external calls will be made. Consider documenting that only trusted apps should use hooks.

Test Coverage ✅

Excellent test coverage! The AquaHooks.t.sol file includes 18 comprehensive tests covering:

• All hook flag combinations (NONE, BEFORE, AFTER, BOTH)
• ETH wrapping functionality
• Hook failure scenarios (returns false, reverts)
• Non-hook app behavior
• Multiple tokens with ETH
• Fuzz testing for hook flags and ETH amounts

The tests demonstrate good practices and thoroughly validate the implementation.

Summary and Recommendations

The implementation is well-designed and adds valuable ETH handling functionality. The code quality is high and test coverage is comprehensive. However, there is one critical issue that must be addressed:

Must Fix Before Merging:

1. ⚠️ Reentrancy protection - Add ReentrancyGuard to prevent reentrancy attacks through the beforeShip hook

Should Consider:

1. Interface validation before calling hooks (try-catch pattern)
2. Document the trust assumptions for ETH handling
3. Clarify if afterShip error handling omission is intentional

Nice to Have:

1. Consider emitting events when hooks are successfully called for better observability

Overall, this is a solid implementation that just needs the reentrancy issue addressed before it's production-ready.

[claude]

Claude finished @deacix's task —— View job

---

Code Review: Add beforeShip hook for native ETH wrapping

I've thoroughly analyzed this PR. Here's my comprehensive feedback:

Review Checklist:

• Read repository conventions
• Analyze interface design (IShipHook.sol)
• Review implementation in Aqua.sol
• Evaluate security considerations
• Assess test coverage
• Compile comprehensive feedback

Code Quality and Best Practices ✅

Strengths:

• Clean, well-documented interface design with clear separation of concerns
• Excellent use of custom errors for gas efficiency
• Good use of constants for hook flags (HOOK_NONE, HOOK_BEFORE, HOOK_AFTER, HOOK_BOTH)
• Follows established patterns in the codebase
• Clear NatSpec documentation throughout

Minor Issue:

• The ShipHookFailed error is declared in IAqua.sol:40-43. The error uses uint8 hookType parameter, which could be more descriptive (consider using an enum or documenting that 1=beforeShip, 2=afterShip).

Potential Bugs or Issues ⚠️

1. Missing Interface Validation: The implementation doesn't verify that the app actually implements IShipHook before calling hook methods (Aqua.sol:67 and :88). This could lead to confusing reverts when an address that doesn't implement the interface is used with hooks enabled.

2. Inconsistent Error Handling:

   • beforeShip checks return value and reverts on failure (Aqua.sol:73)
   • afterShip (Aqua.sol:88) has no error handling - if this is intentional for backwards compatibility, it should be documented

3. Hook Failure Semantics: The beforeShip hook returns a boolean for success, but there's no clear guidance on when to return false vs reverting. This should be documented in the interface.

Performance Considerations 🔄

1. Gas Overhead: Hook calls add ~30-50k gas for external calls. This is acceptable for the added functionality.
2. Efficient Implementation:
   • Bitwise flag checks are gas-efficient
   • No unnecessary storage operations
   • Gas snapshot shows reasonable increases
3. Optimization: The implementation is already well-optimized. Consider documenting expected gas overhead for integrators.

Security Concerns 🔒

✅ Reentrancy Protection - RESOLVED

The implementation correctly uses OpenZeppelin's ReentrancyGuardTransient modifier on the ship() function (Aqua.sol:16,54). This prevents reentrancy attacks through the hooks. The tests confirm this protection works as expected (tests show reentrancy attempts are blocked).

Other Security Considerations:

1. Trust Model for ETH Handling: The design trusts apps to properly wrap ETH to WETH. There's no verification that:

   • The correct amount of WETH was sent to the maker
   • The app actually wrapped the ETH vs keeping it

   This is acceptable if apps are trusted/verified, but should be documented clearly.

2. Unchecked External Call: Any address can be passed as app, and if hooks are enabled, external calls will be made. This could lead to griefing or unexpected behavior. Consider documenting that only trusted apps should use hooks.

3. ETH Validation: Good implementation requiring HOOK_BEFORE flag when ETH is sent - prevents accidental ETH loss.

Test Coverage ✅

Excellent test coverage! The AquaHooks.t.sol file includes 18 comprehensive tests covering:

• All hook flag combinations (NONE, BEFORE, AFTER, BOTH) ✓
• ETH wrapping functionality with proper WETH handling ✓
• Hook failure scenarios (returns false, reverts) ✓
• Non-hook app behavior and interface validation ✓
• Multiple tokens with ETH scenarios ✓
• Strategy hash verification ✓
• Fuzz testing for hook flags and ETH amounts ✓
• Reentrancy protection verification ✓

The tests demonstrate thorough validation of all edge cases and security scenarios.

Summary and Recommendations

This is a well-designed and well-implemented feature that adds valuable ETH handling functionality to Aqua. The code quality is high, and the test coverage is comprehensive.

Strengths:

• ✅ Reentrancy protection properly implemented
• ✅ Clean, extensible design with hook flags
• ✅ Comprehensive test coverage
• ✅ Good gas efficiency
• ✅ Proper ETH validation

Recommendations for Improvement:

1. Add interface validation before calling hooks (use try-catch pattern)
2. Document the trust assumptions for ETH handling in the interface comments
3. Clarify if afterShip error handling omission is intentional
4. Consider emitting events when hooks are successfully called for better observability
5. Document expected behavior for beforeShip return value (false vs revert)

Overall, this is a solid implementation that just needs minor documentation improvements. The security concerns have been properly addressed with the reentrancy guard.

[claude]

Claude finished @deacix's task —— View job

---

PR Review: Add beforeShip hook for native ETH wrapping

I've thoroughly analyzed this PR. Here's my comprehensive feedback:

Code Quality and Best Practices ✅

Strengths:

• Well-structured interface design with clear separation of concerns
• Excellent documentation with comprehensive NatSpec comments explaining the hook system
• Good use of constants for hook flags (HOOK_NONE, HOOK_BEFORE, HOOK_AFTER, HOOK_BOTH)
• Clear error handling with custom errors
• Follows existing code patterns and conventions

Minor Issues:

• None found. The code is clean and well-organized.

Potential Bugs or Issues ⚠️

1. Missing Interface Validation:

   • The implementation doesn't verify that the app implements IShipHook before calling hook methods (Aqua.sol:70 and :97)
   • This is documented as intentional in the comments to save gas (~2600 gas per call)
   • Apps must ensure they only set hook flags if they implement the interface
   • This is a reasonable trade-off but could lead to confusing reverts

2. Inconsistent Error Handling:

   • beforeShip checks return value and reverts on failure (Aqua.sol:78)
   • afterShip has no return value or error handling - any revert propagates directly
   • This is documented as intentional and follows common callback patterns
   • The design is sound but worth noting

Performance Considerations 🔄

1. Gas Overhead:

   • Hook calls add ~30-50k gas for external calls
   • The implementation is optimized with bitwise flag checks
   • No unnecessary storage operations or ERC-165 checks
   • Gas snapshot shows reasonable increases

2. Efficient Design:

   • Flags allow selective hook usage (no overhead when not used)
   • Direct hook calls without interface checks save ~2600 gas per call

Security Concerns 🔒

✅ Reentrancy Protection - RESOLVED

The critical reentrancy issue from previous reviews has been properly addressed:

• Aqua.sol:16 uses OpenZeppelin's ReentrancyGuardTransient modifier
• ship() function at line 54 has the nonReentrant modifier
• Tests confirm reentrancy attempts are blocked (AquaHooks.t.sol:608-672)
• The tests show that attempts to re-enter from both beforeShip and afterShip hooks fail as expected

Other Security Considerations:

1. Trust Model for ETH Handling:

   • The design trusts apps to properly wrap ETH to WETH
   • No verification that the correct amount was sent to the maker
   • This is acceptable for trusted apps but should be clearly documented
   • The interface docs explain the expected behavior well

2. ETH Validation:

   • Good implementation requiring HOOK_BEFORE flag when ETH is sent (Aqua.sol:60-62)
   • Prevents accidental ETH loss

3. Hook Execution Order:

   • beforeShip called before state changes (line 69)
   • afterShip called after state changes (line 96)
   • This is the correct pattern for their intended use cases

Test Coverage ✅

Excellent test coverage! The AquaHooks.t.sol file includes 18 comprehensive tests:

1. Hook Flag Tests: All flag combinations tested (NONE, BEFORE, AFTER, BOTH) ✓
2. ETH Wrapping Tests: Happy path and edge cases ✓
3. Error Scenarios: Hook failures, reverts, false returns ✓
4. Interface Validation: Non-hook app behavior ✓
5. Strategy Hash Verification: Correct hash passed to hooks ✓
6. Multiple Tokens: ETH with other tokens ✓
7. Fuzz Tests: Random hook flags and ETH amounts ✓
8. Security Tests: Comprehensive reentrancy protection tests ✓

The tests are well-structured, cover all edge cases, and properly validate the security measures.

Summary and Recommendations

This is a well-designed and well-implemented feature that adds valuable ETH handling functionality to Aqua. The code quality is high, test coverage is comprehensive, and the critical reentrancy issue has been properly addressed.

Strengths:

• ✅ Clean, extensible hook system with flag-based control
• ✅ Proper reentrancy protection with ReentrancyGuardTransient
• ✅ Excellent test coverage including security scenarios
• ✅ Good gas optimization decisions
• ✅ Clear documentation of design decisions and trade-offs

Recommendations for Consideration:

1. Consider documenting the trust assumptions more prominently in the main contract comments
2. The intentional lack of interface validation is well-documented but could be highlighted in integration guides
3. Consider adding events when hooks are successfully executed for better observability

Overall, this is a solid implementation that's ready for production use. The security concerns have been addressed, and the design decisions are well-reasoned and documented.

Todo List:

• Read CLAUDE.md for repository conventions (not found)
• Analyze IShipHook.sol interface design
• Review IAqua.sol interface updates
• Examine Aqua.sol implementation for security and correctness
• Assess test coverage in AquaHooks.t.sol
• Check for reentrancy protection (critical issue from previous reviews)
• Compile comprehensive review feedback