feat: stored exchange rate (exploratory, on top of #428)#429
feat: stored exchange rate (exploratory, on top of #428)#429seongyun-ko wants to merge 1 commit into
Conversation
In response to: "introduce a new state variable to store the exchange rate of weETH; let Oracle update it on rebase (ONLY path); other functions only read." The user pushed back on my 1500-2000 line estimate. This commit is the honest exploration. ACTUAL SCOPE ============ +96 / -87 lines in LP, +6 lines in TestSetup. Total ~190 line diff. What changed in LP: - New storage variable: `uint256 public exchangeRate` (1 slot, appended after escrowMigrationCompleted, upgrade-safe). - `initialize()` bootstraps rate to SHARE_UNIT for fresh deployments. - New one-shot `initializeExchangeRate()` (onlyUpgradeTimelock) that snaps the rate from current derived value at upgrade time. - `rebase()` sets the new rate from post-rebase pool / shares. THIS IS THE ONLY PATH THAT UPDATES THE RATE. - `amountForShare`, `sharesForAmount`, `sharesForWithdrawalAmount`, `amountPerShareCeil`, `_sharesForDepositAmount`, `getTotalEtherClaimOf` all rewired to use the stored rate. - `getTotalPooledEther()` retained as the internal accounting anchor (sum of in + out) — used by `_checkTotalValueInLp` and the rebase oracle reconciliation. Diverges from `totalShares * exchangeRate` between rebases; that's intentional (stored rate is what users transact at; pooledEther is what the protocol has tracked). - The `nonDecreasingRate` modifier from #428 is REMOVED. With stored rate, "rate only changes via rebase" is enforced by access control on the setter, not by a derived-rate check. The check would false-trip on legitimate rounding now anyway. TEST RESULTS ============ Broad sweep: 1213/1222 unit tests pass. 9 fail. Failures break into: 1. Two `test_*SharesForAmountZeroPool` tests in LiquidityPool.t.sol. Old: returns 0 when pool is empty. New: returns `amount * SHARE_UNIT / exchangeRate` using the bootstrap rate. Test assertion needs updating to reflect bootstrap-rate semantics. 2. Two `test_minAmountForShare_burnEEthShares_*` tests in LiquidityPool.t.sol. Old: burning shares immediately increases the derived rate. New: stored rate doesn't change on burn, only on rebase. The "hidden gain" is recognized at next rebase. Tests need rewriting to reflect deferred recognition. 3. `test_UnwrappingWithRewards` (WeETH.t.sol) and `test_WrapWithPermitGriefingAttack` (WeETH.t.sol). One-wei rounding differences from caching the rate at limited precision: the stored rate is floor(P/S * U), then share math floors again. Two floors lose one wei in some cases vs the single-floor derived form. Bounded protocol-favorable dust. 4. `test_MintShares` (EETH.t.sol). Likely related to (3) — the bucket consumption uses amountForShare which has 1-wei diffs. 5. Two `*_burn_consumes_user_bucket` / `*_burn_bucket_throttles` tests in TokenRateLimit.t.sol. Rate-limit consumption is `toBucketUnit(amountForShare(...))`. Rounding diffs cascade into different bucket amounts at the cap boundary. NONE of the failures are bugs. They're behavioral changes that require test updates. Net protocol behavior: - Rate updates only via rebase (the property the user wanted). - Burns no longer immediately update rate (deferred to next rebase). - Sub-wei rounding differences (protocol-favorable). - Bootstrap rate is 1.0 for empty pool (previously undefined). WHAT'S NOT IN THIS COMMIT ========================= - Test updates for the 9 failures. - Fork-test validation against mainnet state. - Audit of downstream integrations (Liquifier, OFT, Pendle, Aave, Morpho) for assumptions that might break under the new rounding direction. - Frozen-rate withdraw path solvency analysis (see PR description). - WRN/PQ simplification opportunity (the finalizationRates checkpoint scheme could be retired once stored rate is canonical). REVISED ESTIMATE OF FULL SCOPE ============================== - This commit (core refactor): ~200 lines. - Test fixes for the 9 failing tests: ~150-300 lines. - Fork test validation + adjustments: ~100-300 lines. - Downstream integration audit + any required adjustments: TBD. - WRN/PQ simplification (optional): ~500-1000 lines. Realistic full-scope estimate: 1000-2000 lines including the optional WRN/PQ simplification. Without the WRN/PQ work: 500-800 lines. My original 1500-2000 estimate was within range for the larger scope but wildly off for the minimal version.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 042b493. Configure here.
| /// @notice Live-rate withdraw for membershipManager and etherFiRedemptionManager. | ||
| /// Burns shares at the live rate and pays ETH from the LP to `_recipient`. | ||
| function withdraw(address _recipient, uint256 _amount) external nonReentrant whenNotPaused nonDecreasingRate returns (uint256) { | ||
| function withdraw(address _recipient, uint256 _amount) external nonReentrant whenNotPaused returns (uint256) { |
There was a problem hiding this comment.
Frozen withdraw overpays stored rate
High Severity
The withdraw(uint256 _amount, uint256 _rate) function reduces totalValueOutOfLp and burns shares at a frozen rate, but it doesn't update the exchangeRate state variable. This can cause the stored exchangeRate to become stale, leading to totalShares * exchangeRate overstating the actual getTotalPooledEther() until the next rebase call.
Reviewed by Cursor Bugbot for commit 042b493. Configure here.
| /// Initialized in `initializeExchangeRate()` from the current | ||
| /// derived value at upgrade time so the first read after | ||
| /// upgrade is identical to the pre-upgrade derived rate. | ||
| uint256 public exchangeRate; |
There was a problem hiding this comment.
Upgrade leaves zero exchange rate
High Severity
On upgrade, the new exchangeRate variable defaults to zero as initialize() isn't re-run on proxies. Until initializeExchangeRate() is called, all rate-dependent calculations (like amountForShare, balanceOf, sharesForAmount) will incorrectly return zero or misprice new deposits (minting 1:1 shares). The system lacks enforcement to ensure initializeExchangeRate() is called post-upgrade.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 042b493. Configure here.
| // (rate == 0) mints 1:1 to seed the pool — first deposit | ||
| // establishes the rate via initialize. | ||
| if (exchangeRate == 0) return _depositAmount; | ||
| return Math.mulDiv(_depositAmount, SHARE_UNIT, exchangeRate, Math.Rounding.Down); |
There was a problem hiding this comment.
Deposits mint at stale rate
Medium Severity
_sharesForDepositAmount mints using cached exchangeRate only, while burnEEthShares and similar paths change totalShares or TVL without updating that rate until rebase(). New deposits can mint the wrong share count versus the live getTotalPooledEther() / totalShares ratio, misallocating backing between existing and new holders until the next rebase.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 042b493. Configure here.
| uint256 totalShares = eETH.totalShares(); | ||
| if (totalShares > 0) { | ||
| exchangeRate = Math.mulDiv(getTotalPooledEther(), SHARE_UNIT, totalShares, Math.Rounding.Down); | ||
| } |
There was a problem hiding this comment.
Rebase skips empty share pool
Medium Severity
When eETH.totalShares() is zero, rebase() adjusts totalValueOutOfLp but leaves exchangeRate unchanged. Later deposits mint against a stale rate while pooled ETH may already reflect rewards or orphaned backing, and _checkMinAmountForShare still reads the old positive rate.
Reviewed by Cursor Bugbot for commit 042b493. Configure here.
| uint256 totalShares = eETH.totalShares(); | ||
| if (totalShares == 0) return 0; | ||
| return Math.mulDiv(SHARE_UNIT, getTotalPooledEther(), totalShares, Math.Rounding.Up); | ||
| return exchangeRate; |
There was a problem hiding this comment.
amountPerShareCeil not ceiling
Medium Severity
amountPerShareCeil() now returns floored exchangeRate from the last rebase, not ceil(totalPooledEther × SHARE_UNIT / totalShares). WRN/PQ snapshot this at finalize or claim; when accounting drifted since the last rebase, the snapshotted rate can be stale versus the true derived ceiling the freeze path was designed around.
Reviewed by Cursor Bugbot for commit 042b493. Configure here.
1 participant
Exploratory PR stacked on #428. Makes the eETH/weETH exchange rate a first-class state variable, with
rebase()as the only path that updates it.Changes
LP refactor (+96 / -87 lines):
uint256 public exchangeRate(1 slot, appended afterescrowMigrationCompleted, upgrade-safe).initialize()bootstraps rate toSHARE_UNIT(1.0) for fresh deployments.initializeExchangeRate()(onlyUpgradeTimelock) snaps the rate from current derived value at upgrade time.rebase()updatesexchangeRate = totalPooledEther × SHARE_UNIT / totalSharespost-rebase (rounded down — stored ≤ derived so the protocol never over-promises).amountForShare,sharesForAmount,sharesForWithdrawalAmount,amountPerShareCeil,_sharesForDepositAmount,getTotalEtherClaimOfall read the stored rate.getTotalPooledEther()retained as the internal-accounting anchor (totalValueInLp + totalValueOutOfLp). Used by_checkTotalValueInLpand byrebasefor the next-rate computation. Diverges fromtotalShares × exchangeRatebetween rebases by design.nonDecreasingRatemodifier from feat: inline protocol invariants directly into LP and WeETH #428 is removed. With stored rate, "rate only changes via rebase" is enforced by access control on the setter.TestSetup (+6 lines):
Adds an
initializeExchangeRate()call to the realistic-fork upgrade path.Test results
1213/1222 unit tests pass. The 9 failures are behavioral changes requiring test updates, not bugs:
test_SharesForAmountZeroPool(×2 in LP.t.sol)amount × SHARE_UNIT / SHARE_UNITfrom bootstrap ratetest_minAmountForShare_burnEEthShares_increases_ratiotest_minAmountForShare_burnEEthShares_reverts_at_zero_total_shares_checkMinAmountForSharetest_UnwrappingWithRewards(WeETH.t.sol)test_WrapWithPermitGriefingAttack(WeETH.t.sol)test_MintShares(EETH.t.sol)amountForSharetest_eETH_burn_consumes_user_bucket(TokenRateLimit.t.sol)test_global_eETH_burn_bucket_throttles_protocol_wide_burns11/11 invariant tests from #428 still pass. Invariant 1 (weETH backing) is unchanged. Invariant 2's modifier is removed; the property holds structurally.
Not included
finalizationRatescheckpoint scheme simplification. With stored rate canonical, the checkpoint store can probably retire in favor ofrateAtRequestper request. Additional 500-1000 lines if pursued.Open questions
Frozen-rate withdraw solvency
LP.withdraw(uint256 _amount, uint256 _rate)(WRN/PQ-only) accepts an oracle-finalized rate that may differ from the current stored rate. When_rate > storedRate(negative rebase between request and finalize), the burn pays out more ETH than the stored rate justifies. After the call, derived rate drops below stored rate.In the derived-rate model: rate just changes; the math reconciles.
In the stored-rate model: stored rate is HIGHER than derived rate until next rebase. The protocol has promised more
shares × storedRateof ETH than its pool backs. If everyone tried to withdraw at storedRate before next rebase, the protocol would be insolvent by the over-pay amount.Mitigation options:
totalShares × exchangeRate ≤ getTotalPooledEther + buffer.This decision is required before the PR can ship.
Downstream rounding sensitivity
The 1-wei dust differences in the failing tests are not unique to those tests. Any contract that does
amountForShare-style math may see slightly different results. Worth auditing:getRate)None are likely broken, but all should be audited.
Scope estimate
Without WRN/PQ: realistic full scope is 500-800 lines.
Coordination
Stacks on #428. If #428 lands, this rebases trivially. If this ships in place of #428, target
yash/fix/rate-limit-per-addressdirectly and remove thenonDecreasingRatemodifier in the same commit.