Thunder Loan
AI First Flight #7
Beginner FriendlyFoundryDeFiOracle
EXP
View results
Previous Next
Submission Details
Severity: high
Valid

deposit() erroneously calls updateExchangeRate, inflating the exchange rate even though no flash loan fee was ever earned

messadeneami

Root + Impact

Description

ThunderLoan::deposit is meant to let liquidity providers mint AssetToken shares in exchange for underlying tokens. On every single call to deposit, however, the function also calls getCalculatedFee and then assetToken.updateExchangeRate(calculatedFee):

function deposit(IERC20 token, uint256 amount) external revertIfZero(amount) revertIfNotAllowedToken(token) {
AssetToken assetToken = s_tokenToAssetToken[token];
uint256 exchangeRate = assetToken.getExchangeRate();
uint256 mintAmount = (amount * assetToken.EXCHANGE_RATE_PRECISION()) / exchangeRate;
emit Deposit(msg.sender, token, amount);
assetToken.mint(msg.sender, mintAmount);
​
uint256 calculatedFee = getCalculatedFee(token, amount);
// @> a plain deposit is not a flash loan, no fee was ever earned, yet the exchange rate is bumped anyway
assetToken.updateExchangeRate(calculatedFee);
​
token.safeTransferFrom(msg.sender, address(assetToken), amount);
}

updateExchangeRate is supposed to be called exactly once per flash loan, inside repay, to distribute the fee the borrower paid across existing LPs:

function updateExchangeRate(uint256 fee) external onlyThunderLoan {
uint256 newExchangeRate = s_exchangeRate * (totalSupply() + fee) / totalSupply();
if (newExchangeRate <= s_exchangeRate) {
revert AssetToken__ExchangeRateCanOnlyIncrease(s_exchangeRate, newExchangeRate);
}
s_exchangeRate = newExchangeRate;
emit ExchangeRateUpdated(s_exchangeRate);
}

Because deposit calls this same function with a "fee" computed from the deposit amount even though no flash loan happened and no fee was actually collected from anyone, the exchange rate is inflated on every deposit with no real backing asset to justify the increase. updateExchangeRate also hard-reverts if the new rate is not strictly greater than the old one (AssetToken__ExchangeRateCanOnlyIncrease), so this isn't just a cosmetic accounting bug — it can make legitimate deposit() calls revert entirely once the rate has already been pushed near a boundary by repeated deposits, and more importantly it permanently corrupts the redemption math (redeem uses getExchangeRate to compute how much underlying each share is worth), meaning later depositors redeem against a rate that includes value that was never actually deposited by anyone.

Confirming this is a real bug and not intended behavior: the protocol's own upgraded version, ThunderLoanUpgraded.sol, removes these two lines from deposit entirely:

function deposit(IERC20 token, uint256 amount) external revertIfZero(amount) revertIfNotAllowedToken(token) {
AssetToken assetToken = s_tokenToAssetToken[token];
uint256 exchangeRate = assetToken.getExchangeRate();
uint256 mintAmount = (amount * assetToken.EXCHANGE_RATE_PRECISION()) / exchangeRate;
emit Deposit(msg.sender, token, amount);
assetToken.mint(msg.sender, mintAmount);
// @> getCalculatedFee + updateExchangeRate calls removed here in the fixed version
token.safeTransferFrom(msg.sender, address(assetToken), amount);
}

The fix simply deletes the fee-calculation and exchange-rate-update lines from deposit, leaving updateExchangeRate to be called only from the flash loan flow (repay) where a fee is actually collected. This confirms the original behavior in ThunderLoan.sol is a genuine accounting bug, not an intentional design choice.

Risk

Likelihood: High

  • This triggers on every single normal deposit() call, with no special preconditions, no attacker needed, and no unusual market state required. It is the default behavior of the protocol's most basic user-facing function.

Impact: High

  • Every deposit silently mints "fee" value out of thin air and bakes it into the exchange rate, permanently corrupting the share-to-underlying conversion used by redeem. Over time depositors redeem against an exchange rate that overstates the real backing per share, meaning later liquidity providers can be shortchanged when withdrawing, and the protocol's accounting diverges further from reality with every deposit. The ExchangeRateCanOnlyIncrease revert condition also means repeated deposits can eventually cause legitimate deposit calls to unexpectedly revert.

Proof of Concept

Add the following test to the ThunderLoan test suite (e.g. ThunderLoanTest.t.sol) and run with forge test:

function testDepositInflatesExchangeRateWithNoFeeEarned() public setAllowedToken {
uint256 rateBefore = thunderLoan.getExchangeRate(address(tokenA));
​
vm.startPrank(liquidityProvider);
tokenA.mint(liquidityProvider, AMOUNT);
tokenA.approve(address(thunderLoan), AMOUNT);
thunderLoan.deposit(tokenA, AMOUNT);
vm.stopPrank();
​
uint256 rateAfter = thunderLoan.getExchangeRate(address(tokenA));
​
// No flash loan was ever taken out, no fee was ever paid by anyone,
// yet the exchange rate increased purely from calling deposit().
assertGt(rateAfter, rateBefore);
}

Running this shows rateAfter > rateBefore immediately after a plain deposit() call with zero flash loans executed — confirming the exchange rate is being inflated with no fee ever actually having been earned or paid by anyone.

Recommended Mitigation

Remove the getCalculatedFee / updateExchangeRate calls from deposit, exactly as already done in ThunderLoanUpgraded.sol:

function deposit(IERC20 token, uint256 amount) external revertIfZero(amount) revertIfNotAllowedToken(token) {
AssetToken assetToken = s_tokenToAssetToken[token];
uint256 exchangeRate = assetToken.getExchangeRate();
uint256 mintAmount = (amount * assetToken.EXCHANGE_RATE_PRECISION()) / exchangeRate;
emit Deposit(msg.sender, token, amount);
assetToken.mint(msg.sender, mintAmount);
​
- uint256 calculatedFee = getCalculatedFee(token, amount);
- assetToken.updateExchangeRate(calculatedFee);
​
token.safeTransferFrom(msg.sender, address(assetToken), amount);
}

updateExchangeRate should only ever be invoked from the flash loan repayment path, where a real fee has actually been collected from a borrower.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 2 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-02] Updating exchange rate on token deposit will inflate asset token's exchange rate faster than expected

# Summary Exchange rate for asset token is updated on deposit. This means users can deposit (which will increase exchange rate), and then immediately withdraw more underlying tokens than they deposited. # Details Per documentation: > Liquidity providers can deposit assets into ThunderLoan and be given AssetTokens in return. **These AssetTokens gain interest over time depending on how often people take out flash loans!** Asset tokens gain interest when people take out flash loans with the underlying tokens. In current version of ThunderLoan, exchange rate is also updated when user deposits underlying tokens. This does not match with documentation and will end up causing exchange rate to increase on deposit. This will allow anyone who deposits to immediately withdraw and get more tokens back than they deposited. Underlying of any asset token can be completely drained in this manner. # Filename `src/protocol/ThunderLoan.sol` # Permalinks https://github.com/Cyfrin/2023-11-Thunder-Loan/blob/8539c83865eb0d6149e4d70f37a35d9e72ac7404/src/protocol/ThunderLoan.sol#L153-L154 # Impact Users can deposit and immediately withdraw more funds. Since exchange rate is increased on deposit, they will withdraw more funds then they deposited without any flash loans being taken at all. # Recommendations It is recommended to not update exchange rate on deposits and updated it only when flash loans are taken, as per documentation. ```diff function deposit(IERC20 token, uint256 amount) external revertIfZero(amount) revertIfNotAllowedToken(token) { AssetToken assetToken = s_tokenToAssetToken[token]; uint256 exchangeRate = assetToken.getExchangeRate(); uint256 mintAmount = (amount * assetToken.EXCHANGE_RATE_PRECISION()) / exchangeRate; emit Deposit(msg.sender, token, amount); assetToken.mint(msg.sender, mintAmount); - uint256 calculatedFee = getCalculatedFee(token, amount); - assetToken.updateExchangeRate(calculatedFee); token.safeTransferFrom(msg.sender, address(assetToken), amount); } ``` # POC ```solidity function testExchangeRateUpdatedOnDeposit() public setAllowedToken { tokenA.mint(liquidityProvider, AMOUNT); tokenA.mint(user, AMOUNT); // deposit some tokenA into ThunderLoan vm.startPrank(liquidityProvider); tokenA.approve(address(thunderLoan), AMOUNT); thunderLoan.deposit(tokenA, AMOUNT); vm.stopPrank(); // another user also makes a deposit vm.startPrank(user); tokenA.approve(address(thunderLoan), AMOUNT); thunderLoan.deposit(tokenA, AMOUNT); vm.stopPrank(); AssetToken assetToken = thunderLoan.getAssetFromToken(tokenA); // after a deposit, asset token's exchange rate has aleady increased // this is only supposed to happen when users take flash loans with underlying assertGt(assetToken.getExchangeRate(), 1 * assetToken.EXCHANGE_RATE_PRECISION()); // now liquidityProvider withdraws and gets more back because exchange // rate is increased but no flash loans were taken out yet // repeatedly doing this could drain all underlying for any asset token vm.startPrank(liquidityProvider); thunderLoan.redeem(tokenA, assetToken.balanceOf(liquidityProvider)); vm.stopPrank(); assertGt(tokenA.balanceOf(liquidityProvider), AMOUNT); } ```

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!