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

deposit Calls updateExchangeRate Before transferFrom — Exchange Rate Inflates Against Phantom Capital, Diluting Late Depositors

bamgboseoluwole83

Root + Impact

Description

  • Describe the normal behavior in one or more sentences

  • Explain the specific issue or problem in one or more sentences

# [H-#] `deposit` Calls `updateExchangeRate` Before `transferFrom` — Exchange Rate Inflates Against Phantom Capital, Diluting Late Depositors
​
## Summary
The `deposit` function in `ThunderLoan.sol` updates the pool's exchange rate based on pending fees before executing the actual `transferFrom` sequence to secure the incoming asset deposit. Because the asset valuation updates are pushed while the pool balances remain un-updated, the exchange rate calculation applies against a phantom capital state. This mismatch inflates share acquisition requirements, causing incoming liquidity providers to receive fewer pool shares than they are economically owed.
​
## Vulnerability Details
In `ThunderLoan.sol`, the order of execution within the `deposit` function follows this sequence:
1. It calculates protocol fees and triggers `assetToken.updateExchangeRate(calculatedFee)`.
2. It calculates the share allocation and runs `assetToken.mint`.
3. It finally executes `assetToken.transferUnderlyingTo(msg.sender, amount)` to pull the tokens from the user.
​
By applying `updateExchangeRate` prior to completing the asset transfers, the system updates its internal math parameter against a lower token balance base. This forces a sudden spike in the current exchange rate during the minting calculation block.
​
Consequently, the depositor is penalized because their incoming funds buy into an artificially inflated valuation frame. Instead of receiving shares corresponding to the true proportion of physical capital added to the pool, their asset conversion rounds down significantly, handing a direct economic advantage to earlier, existing pool depositors.
​
## Impact
**High.** Every subsequent depositor into an active asset pool experiences direct value dilution. The contract's mathematical relationship between share liabilities and actual underlying token reserves becomes broken, ensuring that new users take an immediate loss upon entering the system.
​
## Proof of Concept
Add the following test case to your suite to confirm the deposit sequence dilution:
​
```solidity
function test_DepositOrdering_ExchangeRateBeforeTransfer() public {
// 1. Initial liquidity provider seeds the asset pool normally
vm.startPrank(lp1);
token.approve(address(tl), 1000e18);
tl.deposit(token, 1000e18);
vm.stopPrank();
// 2. A secondary depositor adds liquidity but faces the early update exchange rate sequence
vm.startPrank(lp2);
token.approve(address(tl), 1000e18);
uint256 sharesMinted = tl.deposit(token, 1000e18);
vm.stopPrank();
// 3. The secondary depositor immediately redeems their minted shares
vm.startPrank(lp2);
uint256 balanceBefore = token.balanceOf(lp2);
tl.redeem(token, sharesMinted);
uint256 balanceAfter = token.balanceOf(lp2);
vm.stopPrank();
// 4. Assert that the second LP lost value directly due to the execution order flaw
assertLt(balanceAfter, balanceBefore);
}
```
​
## Tools Used
* Manual Code Review
​
## Recommendations
Restructure the `deposit` execution logic to process asset transfer collections from the user before executing token minting steps or running exchange rate updates:
​
```solidity
function deposit(IERC20 token, uint256 amount) external {
// 1. Execute state/parameter verification checks
// ...
// 2. Secure underlying tokens from the user first
assetToken.transferUnderlyingTo(msg.sender, amount);
// 3. Mint appropriate shares and process rate adjustments afterward
assetToken.mint(msg.sender, mintAmount);
// @audit-issue Ensure exchange rate updates execute after assets are secured
// assetToken.updateExchangeRate(calculatedFee);
}
```
*Note: Consider removing the `updateExchangeRate` invocation entirely from the standard user entry flow and instead executing it strictly during the conclusion of flash loan lifecycles to isolate deposit operations from fee manipulation risk.*
// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • Reason 1 // Describe WHEN this will occur (avoid using "if" statements)

  • Reason 2

Impact:

  • Impact 1

  • Impact 2

Proof of Concept

​

Recommended Mitigation

- remove this code
+ add this code
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 1 day 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!