Skip to content

Conversation

@ifelsedeveloper
Copy link

@ifelsedeveloper ifelsedeveloper commented Dec 26, 2025

Change Summary

What does this PR change?

Implements audit recommendations from the 2024-12-26 Aqua Protocol Audit Report. Addresses 2 Low severity findings and 8 Informational findings to improve code quality, documentation, and prevent edge-case issues.

Related Issue/Ticket:


Changes Implemented

Low Severity Fixes

ID Finding Change
L-1 ReentrancyGuard pragma version mismatch Updated pragma from ^0.8.0 to ^0.8.24 to match transient storage requirements
L-2 dock() undefined behavior with empty tokens array Added require(tokens.length > 0, EmptyTokensArray()) check

Informational Fixes

ID Finding Change
I-1 Shipped event doc says "revoked" Fixed to "shipped"
I-2 Inconsistent "App" vs "Strategy" terminology Standardized to "app" with interface-level documentation
I-3 Pushed event mixes terminology Fixed to use "app" consistently
I-4 safeBalances() missing revert documentation Added @dev tag with specific error reference
I-5 Custom errors not in interface Moved all errors to IAqua interface with NatSpec
I-6 Typo "abi enocoded" Fixed to "abi encoded"
I-7 Missing NatSpec for Balance library Added comprehensive documentation
I-8 Missing NatSpec for Multicall contract Added comprehensive documentation

Findings NOT Addressed (by design)

ID Finding Reason
M-1 ship() can be called multiple times with different tokens Deferred - requires design decision
I-9 Consider OpenZeppelin's ReentrancyGuardTransient Deferred - custom implementation preferred

Files Modified

File Changes
src/interfaces/IAqua.sol Added custom errors with NatSpec, fixed event docs, standardized terminology
src/Aqua.sol Removed duplicate errors (now inherited from interface), added empty array check
src/libs/ReentrancyGuard.sol Updated pragma, added NatSpec documentation
src/libs/Balance.sol Added comprehensive NatSpec documentation
src/libs/Multicall.sol Added NatSpec documentation
test/Aqua.t.sol Updated error references from Aqua.* to IAqua.*

Testing & Verification

How was this tested?

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

Test Results:

Ran 26 tests in 2 test suites
├── test/AquaStorageTest.t.sol: 8 passed
└── test/Aqua.t.sol: 18 passed

Suite result: ok. 26 passed; 0 failed; 0 skipped

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

Risk Mitigation
Breaking change for external integrations using error selectors Errors are now in IAqua interface instead of Aqua contract. External code referencing Aqua.ErrorName must update to IAqua.ErrorName
New EmptyTokensArray() revert in dock() Low risk - prevents undefined behavior. Only affects calls with empty arrays which were already no-ops
Pragma update in ReentrancyGuard No runtime impact - only affects compilation. Already compiled with 0.8.30 in main contracts

Breaking Changes:

  • Error selector references changed from Aqua.* to IAqua.*

Migration Required:

  • None for on-chain contracts
  • Off-chain code using error selectors needs update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant