ThunderLoan::deposit raises the exchange rate by a fee that was never earned, so claims exceed assets before a single flash loan is taken

Description

• The exchange rate is the protocol's promise to liquidity providers: amountUnderlying = shares * exchangeRate / 1e18. It exists to distribute flash-loan fees — the README says AssetTokens "gain interest over time depending on how often people take out flash loans", and AssetToken::updateExchangeRate is documented as "it should always go up, never down". It may only rise when new underlying actually arrives as revenue.

• deposit calls it with the fee that would be charged if the deposited amount were borrowed. No such fee is ever collected. The depositor hands over exactly amount, but the per-share claim of every holder is raised as though amount + fee had arrived.

The ordering matters as much as the call itself: mint happens before the rate is raised, so the depositor buys shares at the old rate and is immediately credited with the increase. Every deposit therefore both dilutes the backing and hands the depositor an unearned gain — which is exactly the mechanism by which one liquidity provider ends up taking another's money.

redeem pays out the inflated book value with no solvency check whatsoever:

so the shortfall is not shared proportionally. It lands entirely on whoever withdraws last.

On the fact that ThunderLoanUpgraded::deposit does not contain these two lines (src/upgradedProtocol/ThunderLoanUpgraded.sol:146-154): that is evidence the lines are unintended, not evidence the issue is already handled. ThunderLoan is the implementation that is live and holding funds, both files are in scope, and the upgrade that would remove these lines is itself broken by the storage-layout collision I am reporting separately — so "just upgrade" is not an available remediation.

Risk

Likelihood:

• Every deposit triggers it. No attacker, no race, no configuration — the protocol becomes insolvent through its intended, documented happy path, starting with the first liquidity provider.

• Nothing surfaces it. deposit succeeds, the share balance looks correct, and the problem only appears on the way out.

• The project's own suite passes 9/9 and contains no test for redeem at all. test/fuzz/Invariant.t.sol names the exact property this breaks — "2. The protocol should never lose liquidity provider deposits, it should always go up" — but the file is an empty contract Invariant { } with the invariants left as comments, so it was never enforced.

Impact:

• The protocol is insolvent from the first deposit. One LP deposits 1,000 with zero flash loans ever taken: the AssetToken holds exactly 1,000 and the protocol believes it owes 1,003.

• Liquidity providers steal from each other, decided purely by exit order. With two 1,000-token depositors, the one who exits first collects 1,004.51 — 4.51 more than they put in — and the second is left with 995.49 against their 1,000 principal. The 4.51 comes directly out of the second LP's deposit.

• The second LP cannot execute a full withdrawal at all. Their book claim is 1,001.50 against 995.49 actually present, so redeem(theirEntireShareBalance) reverts; they can only recover by computing a reduced amount, and the remainder of their shares is permanently unbacked.

• redeem(type(uint256).max) — the only "withdraw everything" path the contract offers — always reverts, for every LP, in every configuration, because it resolves to the caller's full share balance.

• The shortfall grows by getCalculatedFee(token, amount) on every deposit, so it scales with cumulative deposit volume rather than being a one-off rounding artifact.

Scope note, stated precisely so it is not overclaimed: a sole liquidity provider is not locked out of their principal. Their full-balance redeem reverts, but a reduced redeem returns 999.999999999999999999 of their 1,000 and strands 2.99 shares' worth of unbacked claim. The real loss requires a second liquidity provider — which is the normal case for a pool.

Proof of Concept

Two scenarios: what a sole LP actually experiences, and where the money really goes once there is more than one.

Save as test/PoC_H02.t.sol and run forge test --mt test_H02 -vv:

Output:

No flash loan is taken in either scenario. The 3-token gap in the first is exactly getCalculatedFee(token, 1000e18) — a fee on a loan that never happened — and in the second it is 4.51 tokens moved from one honest liquidity provider to another.

Recommended Mitigation

A deposit is not revenue. Remove both lines; the exchange rate should move only where a fee is actually collected.

This is exactly the shape of ThunderLoanUpgraded::deposit, so it is a known-good form for this codebase.

Because the rate is already inflated on any deployed instance, and AssetToken is created with new (:234) rather than behind a proxy, s_exchangeRate cannot be corrected after the fact by any existing code path — an onlyThunderLoan corrective setter, or deploying AssetToken upgradeably, is needed for remediation on a live pool.

Independently of the fix, redeem should refuse to pay out more than the AssetToken holds, so any future accounting error degrades into a clear failure instead of a first-come-first-served race:

And implement the invariant the repository already names: totalSupply() * getExchangeRate() / 1e18 <= underlying.balanceOf(assetToken) fails on the very first deposit.