The rate update adds two quantities that are not in the same unit, so the credit is too large by a factor of the current exchange rate

Description

• s_exchangeRate means underlying per share, scaled by 1e18 — that is exactly how deposit (ThunderLoan.sol:150) and redeem (ThunderLoan.sol:174) use it. totalSupply() is therefore a quantity of shares, while fee is a quantity of underlying tokens.

• The formula adds them together:

Expanding what the code computes against what it must compute:

The two agree only while R == 1e18. For every rate above that, the credit is too large by exactly the factor R / 1e18. Note that the author's own worked example in the comment block (:83-88) is computed at rate = 1, which is precisely the one case where the bug is invisible.

The error is self-reinforcing: an over-credit raises R, which makes the next over-credit larger. Book value compounds while the real balance only accrues the fees that were actually paid.

redeem then hands out that book value with no solvency check at all:

so the shortfall is not shared — it lands entirely on whoever withdraws last.

AssetToken.sol is not modified by the upgrade, so ThunderLoanUpgraded inherits this verbatim.

Risk

Likelihood:

• Every flash loan, forever, with no attacker and no unusual conditions. The rate leaves 1e18 on the first fee ever collected, and from the second fee onward every single credit is wrong.

• Nothing bounds or resets it. s_exchangeRate is private with no setter, and AssetToken is deployed with new (ThunderLoan.sol:234) rather than behind a proxy, so the drift cannot be corrected even by the protocol's own operators.

• Invisible to the test suite: the project has no redeem test, and test/fuzz/Invariant.t.sol is an empty contract Invariant { } whose stated property — "The protocol should never lose liquidity provider deposits" — was never implemented.

Impact:

• The protocol becomes progressively insolvent under completely honest operation. Measured on a clean deployment after 100 ordinary flash loans, all properly repaid: the AssetToken holds 2,698.5 tokens while its shares are booked at 2,834.3 — a 5.03% shortfall that no one stole.

• The loss is a transfer between liquidity providers, decided by exit order. In the run below the LP who exits first collects 1,417.14 tokens — more than their fair half of the 2,698.5 actually held — and the second LP's redeem reverts, leaving them 1,281.37 against an identical book claim. One LP is 68 tokens richer and the other 68 poorer, purely for being second.

• Because it is order-dependent and irreversible, the rational response for every LP is to withdraw before the others, so the defect converts ordinary withdrawals into a race.

• No attacker profits directly, which is why I put impact at Medium rather than High. Impact Medium × Likelihood High ⇒ High on the CodeHawks matrix.

Proof of Concept

Deliberately run on a fresh ThunderLoanUpgraded deployment: its deposit (:146-154) does not call updateExchangeRate, and a fresh deployment has no storage collision, so nothing here can be attributed to either of those. The borrower is completely honest — it repays principal plus the full quoted fee through repay() every time.

Save as test/PoC_H05.t.sol and run forge test --mt test_H05 -vv:

Output:

Loan #1 shows a deficit of exactly zero, because the rate is still 1e18 and the two formulas coincide there. From loan #2 onward the gap opens and compounds — this is the signature of the unit error rather than of rounding, which would not grow.

The two liquidity providers hold identical claims of 1,417.14. The first to leave gets all of it; the second is left 135.77 short and cannot execute a full withdrawal.

Recommended Mitigation

Convert the fee into share-space before adding it, so both terms are in the same unit.

The explicit zero-supply guard is worth adding at the same time: totalSupply() is used as a divisor on the current line too, so an empty pool panics with a division-by-zero rather than a named error.

Two further hardening items, independent of the formula itself:

• redeem should never pay out more than the AssetToken holds — add if (amountUnderlying > token.balanceOf(address(assetToken))) revert ... so an accounting error degrades into a clear failure rather than a first-come-first-served race.

• Implement the invariant that test/fuzz/Invariant.t.sol already names. totalSupply() * getExchangeRate() / 1e18 <= underlying.balanceOf(assetToken) fails on the second flash loan and would have caught this immediately.