Root + Impact

Description

• ThunderLoan.deposit() mints shares to the depositor at the current exchangeRate, then additionally calls getCalculatedFee(token, amount) and assetToken.updateExchangeRate(calculatedFee) to credit a "fee" into the exchange rate - but deposit() only ever transfers in amount. Nobody ever transfers the extra fee tokens into the AssetToken contract for a plain deposit.

• AssetToken.updateExchangeRate()'s formula newRate = oldRate * (totalSupply + fee) / totalSupply assumes the fee portion of the numerator is backed by real assets. That assumption only holds in the flashloan() path, where the fee must arrive in the same transaction or the whole call reverts via the ending-balance check. In the deposit() path this precondition does not hold, so exchangeRate is inflated with nothing backing it.

• The result: totalSupply * exchangeRate (what the protocol owes all LPs) systematically drifts above token.balanceOf(address(assetToken)) (what the protocol actually holds) - with zero attacker and zero oracle manipulation required. Two ordinary, honest deposit() calls back-to-back already produce a real deficit, and an earlier LP who redeems first extracts more than they put in, leaving a later, fully honest LP unable to redeem the full value of their own shares (redeem() reverts - real fund loss / DoS, not just a bad number).

• This is also independently weaponizable: since calculatedFee is derived from OracleUpgradeable.getPriceInWeth()'s live spot price (manipulable in a single transaction via MockTSwapPool-style AMMs), an attacker can spike the price, deposit to mint a hugely inflated exchange-rate bump, restore the price, then redeem a small fraction of their new shares for far more underlying than they put in - directly at the expense of existing LPs' principal.

• Confirms this is a known-bad pattern: src/upgradedProtocol/ThunderLoanUpgraded.sol::deposit() has already removed the getCalculatedFee/updateExchangeRate calls entirely, while flashloan()/repay() logic is otherwise unchanged - strong evidence the fee-on-deposit call was recognized internally as wrong.

Risk

Likelihood:

• Reason 1 // No attacker, no timing, and no oracle manipulation is needed - two consecutive, fully honest deposit() calls at a constant, non-manipulated price already produce the deficit, as shown in the PoC below.

• Reason 2 // The deficit is monotonic and compounds with every subsequent deposit, so it is not a one-off edge case but a systemic, guaranteed drift toward insolvency the longer the pool is used normally.

Impact:

• Impact 1 // Honest LPs' redeemable principal is put at direct risk: a later, fully honest depositor's redeem() reverts because the vault's real balance no longer covers what the exchange rate says they are owed.

• Impact 2 // Combined with oracle-price manipulation, an attacker can weaponize the same bug to extract real underlying value from existing LPs' principal in a single transaction.

Proof of Concept

Ran with forge test --match-path "test/PoC_1.t.sol" -vvv: [PASS] test_PassiveDriftCausesInsolvency() (gas: 785549). Key logs: after LP_1 deposits 1000 tokenA, real balance = 1000e18 but owed-to-LPs already = 1003e18 (a 3e18 phantom fee credited from a single honest deposit). After LP_2 also deposits 1000 tokenA, real balance = 2000e18 but owed-to-LPs = 2006.009e18 (deficit 6.009e18) - and the oracle price (MockTSwapPool) never changed the entire time. LP_1 then redeems first and receives 1004.5e18 (more than their 1000e18 principal). LP_2, a purely honest, later depositor, is entitled to 1001.5e18 but the vault only has 995.5e18 left - LP_2's redeem() call reverts (vm.expectRevert() assertion passes), proving a real fund-loss/DoS, not a cosmetic accounting artifact. Full regression suite: 19/19 passing, no regressions.

Recommended Mitigation

Remove the getCalculatedFee/updateExchangeRate calls from deposit() entirely - deposits should mint shares 1:1 at the current exchange rate with no fee credited, matching what ThunderLoanUpgraded.sol::deposit() already does. updateExchangeRate should only ever be invoked from a path where the extra funds are guaranteed to have arrived in the same transaction (i.e. inside flashloan(), after the ending-balance check passes). Add an invariant test asserting assetToken.totalSupply() * assetToken.getExchangeRate() / EXCHANGE_RATE_PRECISION() <= token.balanceOf(address(assetToken)) holds after every state-changing call, as a CI guard against reintroducing this class of bug.