feat: ERC4626 lib (SC-1233) #188
Conversation
WalkthroughThe PR introduces a new ERC4626Lib library that encapsulates ERC4626 token operations (deposit, withdraw, redeem) with integrated rate-limit management. MainnetController is refactored to delegate these operations to the library, converting EXCHANGE_RATE_PRECISION from a constant to a public accessor. AaveLib receives minor formatting adjustments. Changes
Sequence DiagramssequenceDiagram
actor User
participant MC as MainnetController
participant E4626L as ERC4626Lib
participant Proxy as IALMProxy
participant Token as ERC4626Token
participant RL as RateLimits
rect rgb(220, 240, 255)
note over User,RL: deposit() flow
User->>MC: depositERC4626(token, amount)
MC->>E4626L: deposit(proxy, token, amount, maxExchangeRate, rateLimits, rateLimitId)
E4626L->>E4626L: ApproveLib.approve(asset → proxy)
E4626L->>Proxy: depositERC4626(token, amount)
Proxy->>Token: transferFrom + deposit
Token-->>Proxy: shares
E4626L->>E4626L: getExchangeRate(shares, assets)
E4626L->>E4626L: assert(exchangeRate <= maxExchangeRate)
E4626L->>RL: decreaseRateLimit(rateLimitId, amount)
E4626L-->>MC: shares
MC-->>User: shares
end
rect rgb(220, 255, 220)
note over User,RL: withdraw() flow
User->>MC: withdrawERC4626(token, amount)
MC->>E4626L: withdraw(proxy, token, amount, rateLimits, withdrawId, depositId)
E4626L->>RL: decreaseRateLimit(withdrawId, amount)
E4626L->>Proxy: withdrawERC4626(token, amount)
Proxy->>Token: withdraw
Token-->>Proxy: shares
E4626L->>RL: increaseRateLimit(depositId, assets)
E4626L-->>MC: shares
MC-->>User: shares
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
deluca-mike
force-pushed
the
sc-1233-erc4626-lib
branch
from
7a87195 to
0b598fe
Compare
|
Coverage after merging sc-1233-erc4626-lib into dev will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/MainnetController.sol(3 hunks)src/libraries/AaveLib.sol(1 hunks)src/libraries/ERC4626Lib.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T16:47:34.627Z
Learnt from: supercontracts
Repo: sparkdotfi/spark-alm-controller PR: 170
File: src/MainnetController.sol:1153-1181
Timestamp: 2025-10-16T16:47:34.627Z
Learning: In src/MainnetController.sol, the USDS token always returns true on successful transfer operations, so it does not require the empty return data handling that some non-standard ERC20 tokens (like USDT) need.
Applied to files:
src/MainnetController.sol
⏰ 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). (3)
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: build
🔇 Additional comments (9)
src/libraries/AaveLib.sol (1)
9-9: LGTM!The formatting adjustment to the import statement is purely cosmetic with no functional impact.
src/libraries/ERC4626Lib.sol (3)
45-72: LGTM!The withdraw function correctly:
- Triggers withdraw rate limit decrease before the operation
- Performs the withdrawal via proxy
- Increases the deposit rate limit after withdrawal (allows re-depositing)
- Returns the shares burned
The logic and rate limit ordering are correct for the withdraw pattern where the assets amount is specified upfront.
74-101: LGTM!The redeem function correctly performs the redemption before rate limiting. This ordering is necessary because:
redeemspecifies shares (input), not assets (output)- The assets amount is only known after the redemption completes
- Rate limits must use the actual assets received
This differs from
withdraw(where assets are known upfront) but is the correct pattern forredeem.
103-111: LGTM!The getExchangeRate function correctly handles all edge cases:
- Returns 0 for zero assets (handles both 0/0 and 0/shares cases)
- Reverts for zero shares with non-zero assets (infinite exchange rate is invalid)
- Calculates the exchange rate with proper precision scaling
The logic is sound and safe.
src/MainnetController.sol (5)
25-25: LGTM!The import of ERC4626Lib is correctly added to support the refactored ERC4626 operations.
297-308: LGTM!The refactoring to use
ERC4626Lib.getExchangeRatefor calculating the max exchange rate is correct and improves code reusability. This eliminates duplication of the exchange rate calculation logic.Note: This depends on fixing the visibility of
EXCHANGE_RATE_PRECISIONin ERC4626Lib (see comment on ERC4626Lib.sol line 15).
455-468: LGTM!The refactoring to delegate ERC4626 deposits to the library is clean and correct. All necessary parameters (proxy, token, amount, maxExchangeRate, rateLimits, rateLimitId) are properly passed through.
470-483: LGTM!The refactoring to delegate ERC4626 withdrawals to the library is correct. Both rate limit IDs (withdraw and deposit) are properly passed to handle the bidirectional rate limit updates.
485-498: LGTM!The refactoring to delegate ERC4626 redemptions to the library is correct. The function properly passes the shares and both rate limit IDs, returning the assets received from the redemption.
|
|
||
| library ERC4626Lib { | ||
|
|
||
| uint256 internal constant EXCHANGE_RATE_PRECISION = 1e36; |
There was a problem hiding this comment.
Critical: Change visibility to public for external access.
The constant is declared as internal but MainnetController.sol line 501 attempts to access it as ERC4626Lib.EXCHANGE_RATE_PRECISION. Internal constants in libraries cannot be accessed from outside the library, causing a compilation error.
Apply this diff to fix the visibility:
- uint256 internal constant EXCHANGE_RATE_PRECISION = 1e36;
+ uint256 public constant EXCHANGE_RATE_PRECISION = 1e36;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint256 internal constant EXCHANGE_RATE_PRECISION = 1e36; | |
| uint256 public constant EXCHANGE_RATE_PRECISION = 1e36; |
🤖 Prompt for AI Agents
In src/libraries/ERC4626Lib.sol around line 15, the constant
EXCHANGE_RATE_PRECISION is declared internal but is referenced externally as
ERC4626Lib.EXCHANGE_RATE_PRECISION; change its visibility to public so it can be
accessed from other contracts/libraries by replacing the visibility specifier
with public (i.e., make the constant public) and recompile to ensure no other
visibility issues remain.
| function deposit( | ||
| address proxy, | ||
| address token, | ||
| uint256 amount, | ||
| uint256 maxExchangeRate, | ||
| address rateLimits, | ||
| bytes32 rateLimitId | ||
| ) external returns (uint256 shares) { | ||
| IRateLimits(rateLimits).triggerRateLimitDecrease( | ||
| RateLimitHelpers.makeAddressKey(rateLimitId, token), | ||
| amount | ||
| ); | ||
|
|
||
| // Approve asset to token from the proxy (assumes the proxy has enough of the asset). | ||
| ApproveLib.approve(IERC4626(token).asset(), proxy, token, amount); | ||
|
|
||
| // Deposit asset into the token, proxy receives token shares, decode the resulting shares. | ||
| shares = abi.decode( | ||
| IALMProxy(proxy).doCall( | ||
| token, | ||
| abi.encodeCall(IERC4626(token).deposit, (amount, proxy)) | ||
| ), | ||
| (uint256) | ||
| ); | ||
|
|
||
| require(getExchangeRate(shares, amount) <= maxExchangeRate, "MC/exchange-rate-too-high"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM with minor note.
The deposit function correctly:
- Triggers rate limit decrease before the deposit operation (security best practice)
- Handles approvals via ApproveLib
- Validates exchange rate after receiving shares
- Returns the shares received
Minor note: The error message uses "MC/" prefix which is MainnetController-specific. Consider using "ERC4626/" or a generic prefix for better library reusability.
🤖 Prompt for AI Agents
In src/libraries/ERC4626Lib.sol around lines 17 to 43, the require revert
message uses the MainnetController-specific prefix "MC/" which reduces
reusability of this library; change the revert string to a library-appropriate
prefix such as "ERC4626/exchange-rate-too-high" (or a generic
"LIB/exchange-rate-too-high") so the error clearly identifies this library,
updating the require(...) call accordingly.
| function EXCHANGE_RATE_PRECISION() external pure returns (uint256) { | ||
| return ERC4626Lib.EXCHANGE_RATE_PRECISION; | ||
| } |
There was a problem hiding this comment.
Critical: Compilation error due to accessing internal library constant.
This function attempts to access ERC4626Lib.EXCHANGE_RATE_PRECISION, which is declared as internal in the library (ERC4626Lib.sol line 15). Internal constants in libraries cannot be accessed from external contracts, causing a compilation failure.
Resolution: Change the visibility of EXCHANGE_RATE_PRECISION to public in ERC4626Lib.sol (see comment on that file).
Compatibility note: Converting the previous public constant to an external pure function maintains backward compatibility for external callers, as both are accessed via function call syntax.
🤖 Prompt for AI Agents
In src/MainnetController.sol around lines 500-502, the function is failing
compilation because it accesses ERC4626Lib.EXCHANGE_RATE_PRECISION which is
declared internal in ERC4626Lib (line ~15); open ERC4626Lib.sol and change the
EXCHANGE_RATE_PRECISION declaration to be public (or replace the internal
constant with an external pure function that returns the same uint256 value) so
external contracts can reference it; after changing visibility, recompile and
keep the external accessor in MainnetController as-is to preserve backward
compatibility.
Summary by CodeRabbit
Release Notes