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

Constant update of exchange rate during funds deposit leads to catastrophic scenarios

0xscratch

Root + Impact

  • Root: The ThunderLoan::deposit function also updates the exchange rate whenever any liquidity provider deposits their funds. Whereas the exchange rate should only be updated whenever a user takes a flash loan.

  • Impact: This will lead to some extra profits for the early depositors as the exchange rate will go up like crazy, thus letting them earn the profits of later depositors as well.

Description

  • Normal Behaviour:

    • The whole system of Thunder Loan works around the liquidity providers and the users who take flash loans. Liquidity Providers deposit the assets and earn interest accordingly. On the other hand, users take flash loans and pay some fees.

    • The point is, every time a user takes a flash loan, the fees those users pay are actually provided to the liquidity providers themselves by updating the exchange rate according to those fees.

  • Issue:

    • Here's the plot twist: the deposit function also calculates fees and updates the exchange rate whenever any liquidity provider deposits their funds.

    • This means, even if no user has yet taken a flash loan, the exchange rate keeps increasing, which will mint fewer asset tokens for the further liquidity providers. And it will be a huge loss to the late comers because in a redeem situation, the early liquidity providers will eat all their portions due to the high increased exchange rate.

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; // `mintAmount` depends on exchangeRate
emit Deposit(msg.sender, token, amount);
assetToken.mint(msg.sender, mintAmount);
@> uint256 calculatedFee = getCalculatedFee(token, amount);
@> assetToken.updateExchangeRate(calculatedFee); // Increases with the fees
token.safeTransferFrom(msg.sender, address(assetToken), amount);
}
​
...
​
function redeem(
IERC20 token,
uint256 amountOfAssetToken
)
external
revertIfZero(amountOfAssetToken)
revertIfNotAllowedToken(token)
{
// ...
// tokens to be redeemed depend on the exchange rate as well
@> uint256 amountUnderlying = (amountOfAssetToken * exchangeRate) / assetToken.EXCHANGE_RATE_PRECISION();
​
// ...
}

Risk

  • Likelihood: High

    • The exchange rate gets updated every time a liquidity provider deposits some funds

  • Impact: High

    • Mints very less asset tokens as liquidity providers start coming in due to that chain reaction

    • A lot of users will lose their funds in this whole process

    • Much of the benefit is taken by the early depositors

    • Puts the whole system at risk, financially.

Proof of Concept

  1. Here's how the exploit scenario unfolds:

    • Initially, the exchange rate will be 1e18, which was set by the contract itself.

    • As LP1 deposits some funds of tokenA, an equal amount of AssetToken gets minted to the LP1, but the exchange rate increases.

    • Now, when LP2 deposits the same amount of funds as LP1, he will receive fewer AssetToken as compared to LP1 due to the increased exchange rate. Even now, the exchange rate increases furthermore.

    • No flash loan happens (let's say). And LP1 decides to redeem his underlying token, i.e. tokenA. Due to the increased exchange rate, he will get a lot more underlying tokens by burning his AssetToken.

    • Whereas, LP2 will get an error when he tries to redeem using all his AssetToken. This is because LP1 eats up some funds of LP2 as well due to the weird increase in the exchange rate.


  2. Add the following test into test/unit/ThunderLoanTest.t.sol:

    function test__depositUpdatesExchangeRate() public setAllowedToken hasDeposits {
    // First liquidity provider already deposited tokenA
    // Let's see what the exchange rate is
    console.log("ExchangeRate after LP1 deposits: ", thunderLoan.getAssetFromToken(tokenA).getExchangeRate());
    ​
    // The initial exchange rate was 1e18, but it has now increased...
    ​
    // Let's visualise another liquidity provider deposits tokenA
    address liquidityProvider2 = address(789);
    ​
    vm.startPrank(liquidityProvider2);
    tokenA.mint(liquidityProvider2, DEPOSIT_AMOUNT);
    tokenA.approve(address(thunderLoan), DEPOSIT_AMOUNT);
    thunderLoan.deposit(tokenA, DEPOSIT_AMOUNT);
    vm.stopPrank();
    ​
    // Now the exchange rate has increased
    console.log("ExchangeRate after LP2 deposits: ", thunderLoan.getAssetFromToken(tokenA).getExchangeRate());
    ​
    // Let's see what both liquidity providers get if they choose to redeem the underlying token
    // Also, make sure that no flash loan has taken place yet
    vm.prank(liquidityProvider);
    thunderLoan.redeem(tokenA, type(uint256).max); // means redeem everything
    ​
    vm.prank(liquidityProvider2);
    vm.expectRevert(); // fails because `liquidityProvider1` gained some portion of `liquidityProvider2` due to the increased exchange rate
    thunderLoan.redeem(tokenA, type(uint256).max);
    ​
    console.log("Balance of LiquidityProvider 1: ", tokenA.balanceOf(liquidityProvider));
    console.log("Balance of LiquidityProvider 2: ", tokenA.balanceOf(liquidityProvider2));
    ​
    }

  3. Run it using:

    forge test --mt test__depositUpdatesExchangeRate -vv

  4. Logs:

    Ran 1 test for test/unit/ThunderLoanTest.t.sol:ThunderLoanTest
    [PASS] test__depositUpdatesExchangeRate() (gas: 1691146)
    Logs:
    ExchangeRate after LP1 deposits: 1003000000000000000
    ExchangeRate after LP2 deposits: 1004506753369945082
    Balance of LiquidityProvider 1: 1004506753369945082000
    Balance of LiquidityProvider 2: 0
    ​
    Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.58ms (2.99ms CPU time)

Recommended Mitigation

Simply remove the fee calculation and any update of the exchange rate within the deposit function.

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);
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 8 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!