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

Upgrade regression — critical bugs persist in ThunderLoanUpgraded

loyaltoffee360

Description

Normal behavior

A planned upgrade should either fix known critical issues or at minimum avoid introducing new ones. The ThunderLoanUpgraded implementation is explicitly in scope and intended to replace ThunderLoan.

Specific issue

ThunderLoanUpgraded retains the oracle manipulation vector (F-01), the deposit-instead-of-repay flash-loan callback bug (F-02), and the fee-on-transfer accounting flaw (F-04). It also introduces a storage collision that corrupts the flash-loan fee to 100% (F-03) and removes the deposit-side fee accrual (F-06). The only original issue that is no longer reachable via deposit() is the tiny-deposit DoS (F-07), but the flash-loan zero-fee DoS remains.

// src/upgradedProtocol/ThunderLoanUpgraded.sol
​
// Storage collision: old s_feePrecision (1e18) is read as s_flashLoanFee
mapping(IERC20 => AssetToken) public s_tokenToAssetToken;
@> uint256 private s_flashLoanFee; // now holds 1e18
uint256 public constant FEE_PRECISION = 1e18;
​
// Same manipulable spot oracle as original
function getCalculatedFee(IERC20 token, uint256 amount) public view returns (uint256 fee) {
uint256 valueOfBorrowedToken = (amount * getPriceInWeth(address(token))) / FEE_PRECISION;
@> fee = (valueOfBorrowedToken * s_flashLoanFee) / FEE_PRECISION; // 100% fee
}
​
// Same flash-loan callback without guard
function flashloan(address receiverAddress, IERC20 token, uint256 amount, bytes calldata params) external {
// ...
uint256 fee = getCalculatedFee(token, amount);
assetToken.updateExchangeRate(fee);
s_currentlyFlashLoaning[token] = true;
assetToken.transferUnderlyingTo(receiverAddress, amount);
@> receiverAddress.functionCall(...); // receiver can still call deposit()
// ...
}
​
// deposit() no longer accrues fee to 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;
emit Deposit(msg.sender, token, amount);
assetToken.mint(msg.sender, mintAmount);
// missing: updateExchangeRate(calculatedFee)
token.safeTransferFrom(msg.sender, address(assetToken), amount);
}

Risk

Likelihood

  • The upgrade path is explicitly in scope and described in the README.

  • The ThunderLoanUpgraded implementation is intended to be deployed as the new logic contract.

Impact

  • All critical findings from the original contract remain exploitable.

  • The storage collision makes the protocol strictly worse by charging 100% fees.

  • LPs stop earning deposit-side fees after the upgrade.


Proof of Concept

The PoC upgrades the proxy to ThunderLoanUpgraded and demonstrates that F-01, F-02, F-03, F-04, and F-06 are still present, while F-07 is only partially fixed.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import { ThunderLoan } from "src/protocol/ThunderLoan.sol";
import { ThunderLoanUpgraded } from "src/upgradedProtocol/ThunderLoanUpgraded.sol";
import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
​
contract UpgradeRegressionTest {
ThunderLoan thunderLoan;
IERC20 token;
address attacker = address(0x1);
address owner = address(0x2);
ThunderLoanUpgraded upgraded;
​
function _upgrade() internal {
ThunderLoanUpgraded impl = new ThunderLoanUpgraded();
vm.prank(thunderLoan.owner());
UUPSUpgradeable(address(thunderLoan)).upgradeTo(address(impl));
upgraded = ThunderLoanUpgraded(address(thunderLoan));
}
​
function test_H13_F01_F03_oracleManipulation_stillPresent() public {
_upgrade();
uint256 fairFee = upgraded.getCalculatedFee(token, 100e18);
manipulateOracleDown();
uint256 manipulated = upgraded.getCalculatedFee(token, 100e18);
@> assertLt(manipulated, fairFee / 100, "F-01: oracle undercharge persists after upgrade");
}
​
function test_H13_F03_storageCollision_notFixed() public {
_upgrade();
@> assertEq(upgraded.getFee(), 1e18, "F-03: storage collision corrupts fee to 100%");
@> assertEq(upgraded.getCalculatedFee(token, 100e18), 100e18, "F-03: fee calc broken");
}
}

Recommended Mitigation

Do not deploy ThunderLoanUpgraded until all underlying issues are fixed and the storage layout is audited. Required fixes:

  1. Restore s_feePrecision or add a storage gap to prevent the collision.

  2. Replace the spot oracle with a manipulation-resistant source.

  3. Block deposit() and redeem() during active flash loans.

  4. Use balance-delta accounting for deposits.

  5. Restore updateExchangeRate(calculatedFee) in deposit() or document the intentional removal.

// Do not deploy until these fixes are in place
function deposit(IERC20 token, uint256 amount) external nonRevert {
if (s_currentlyFlashLoaning[token]) revert ThunderLoan__FlashLoanInProgress();
// ... balance-delta accounting ...
assetToken.updateExchangeRate(calculatedFee); // restore fee accrual
}
Updates

Lead Judging Commences

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

[H-01] Storage Collision during upgrade

## Description The thunderloanupgrade.sol storage layout is not compatible with the storage layout of thunderloan.sol which will cause storage collision and mismatch of variable to different data. ## Vulnerability Details Thunderloan.sol at slot 1,2 and 3 holds s_feePrecision, s_flashLoanFee and s_currentlyFlashLoaning, respectively, but the ThunderLoanUpgraded at slot 1 and 2 holds s_flashLoanFee, s_currentlyFlashLoaning respectively. the s_feePrecision from the thunderloan.sol was changed to a constant variable which will no longer be assessed from the state variable. This will cause the location at which the upgraded version will be pointing to for some significant state variables like s_flashLoanFee to be wrong because s_flashLoanFee is now pointing to the slot of the s_feePrecision in the thunderloan.sol and when this fee is used to compute the fee for flashloan it will return a fee amount greater than the intention of the developer. s_currentlyFlashLoaning might not really be affected as it is back to default when a flashloan is completed but still to be noted that the value at that slot can be cleared to be on a safer side. ## Impact 1. Fee is miscalculated for flashloan 1. users pay same amount of what they borrowed as fee ## POC 2 ``` function testFlashLoanAfterUpgrade() public setAllowedToken hasDeposits { //upgrade thunderloan upgradeThunderloan(); uint256 amountToBorrow = AMOUNT * 10; console.log("amount flashloaned", amountToBorrow); uint256 calculatedFee = thunderLoan.getCalculatedFee( tokenA, amountToBorrow ); AssetToken assetToken = thunderLoan.getAssetFromToken(tokenA); vm.startPrank(user); tokenA.mint(address(mockFlashLoanReceiver), amountToBorrow); thunderLoan.flashloan( address(mockFlashLoanReceiver), tokenA, amountToBorrow, "" ); vm.stopPrank(); console.log("feepaid", calculatedFee); assertEq(amountToBorrow, calculatedFee); } ``` Add the code above to thunderloantest.t.sol and run `forge test --mt testFlashLoanAfterUpgrade -vv` to test for the second poc ## Recommendations The team should should make sure the the fee is pointing to the correct location as intended by the developer: a suggestion recommendation is for the team to get the feeValue from the previous implementation, clear the values that will not be needed again and after upgrade reset the fee back to its previous value from the implementation. ##POC for recommendation ``` // function upgradeThunderloanFixed() internal { thunderLoanUpgraded = new ThunderLoanUpgraded(); //getting the current fee; uint fee = thunderLoan.getFee(); // clear the fee as thunderLoan.updateFlashLoanFee(0); // upgrade to the new implementation thunderLoan.upgradeTo(address(thunderLoanUpgraded)); //wrapped the abi thunderLoanUpgraded = ThunderLoanUpgraded(address(proxy)); // set the fee back to the correct value thunderLoanUpgraded.updateFlashLoanFee(fee); } function testSlotValuesFixedfterUpgrade() public setAllowedToken { AssetToken asset = thunderLoan.getAssetFromToken(tokenA); uint precision = thunderLoan.getFeePrecision(); uint fee = thunderLoan.getFee(); bool isflanshloaning = thunderLoan.isCurrentlyFlashLoaning(tokenA); /// 4 slots before upgrade console.log("????SLOTS VALUE BEFORE UPGRADE????"); console.log("slot 0 for s_tokenToAssetToken =>", address(asset)); console.log("slot 1 for s_feePrecision =>", precision); console.log("slot 2 for s_flashLoanFee =>", fee); console.log("slot 3 for s_currentlyFlashLoaning =>", isflanshloaning); //upgrade function upgradeThunderloanFixed(); //// after upgrade they are only 3 valid slot left because precision is now set to constant AssetToken assetUpgrade = thunderLoan.getAssetFromToken(tokenA); uint feeUpgrade = thunderLoan.getFee(); bool isflanshloaningUpgrade = thunderLoan.isCurrentlyFlashLoaning( tokenA ); console.log("????SLOTS VALUE After UPGRADE????"); console.log("slot 0 for s_tokenToAssetToken =>", address(assetUpgrade)); console.log("slot 1 for s_flashLoanFee =>", feeUpgrade); console.log( "slot 2 for s_currentlyFlashLoaning =>", isflanshloaningUpgrade ); assertEq(address(asset), address(assetUpgrade)); //asserting precision value before upgrade to be what fee takes after upgrades assertEq(fee, feeUpgrade); // #POC assertEq(isflanshloaning, isflanshloaningUpgrade); } ``` Add the code above to thunderloantest.t.sol and run with `forge test --mt testSlotValuesFixedfterUpgrade -vv`. it can also be tested with `testFlashLoanAfterUpgrade function` and see the fee properly calculated for flashloan

Support

FAQs

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

Give us feedback!