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

Thunder Loan — UUPS Storage Collision Silently Prices Flash Loans at 100%

bizgudboy

Description

The planned UUPS cut-over from ThunderLoan to ThunderLoanUpgraded remaps s_flashLoanFee onto the leftover v1 s_feePrecision slot (1e18). After upgradeTo, getFee() returns 1e18 instead of 3e15, and getCalculatedFee prices every flash loan at 100% of the WETH-valued notional instead of the documented 0.3%.

The upgraded initialize cannot repair this. Both versions gate initialization with OpenZeppelin's initializer modifier rather than reinitializer(2), so a second initialize on the already-initialized ERC1967 proxy reverts Initializable: contract is already initialized and never writes 3e15. upgradeTo does not call updateFlashLoanFee, and UUPS _authorizeUpgrade is onlyOwner with no storage migration.

This is the README-planned production upgrade (upgradeTo(ThunderLoanUpgraded) on a live v1-initialized proxy). No reentrancy, unusual token, or extra privilege is required. Any later borrower then pays the remapped 100% fee.

Deep Dive

v1 layout writes two adjacent fee slots

ThunderLoan stores two child uint256s immediately after s_tokenToAssetToken:

// src/protocol/ThunderLoan.sol
mapping(IERC20 => AssetToken) public s_tokenToAssetToken;
​
// The fee in WEI, it should have 18 decimals. Each flash loan takes a flat fee of the token price.
uint256 private s_feePrecision;
uint256 private s_flashLoanFee; // 0.3% ETH fee
​
mapping(IERC20 token => bool currentlyFlashLoaning) private s_currentlyFlashLoaning;

initialize writes both slots and marks the proxy initialized:

function initialize(address tswapAddress) external initializer {
__Ownable_init();
__UUPSUpgradeable_init();
__Oracle_init(tswapAddress);
s_feePrecision = 1e18;
s_flashLoanFee = 3e15; // 0.3% ETH fee
}

v1 fee math divides by s_feePrecision twice:

function getCalculatedFee(IERC20 token, uint256 amount) public view returns (uint256 fee) {
uint256 valueOfBorrowedToken = (amount * getPriceInWeth(address(token))) / s_feePrecision;
fee = (valueOfBorrowedToken * s_flashLoanFee) / s_feePrecision;
}

With getPriceInWeth == 1e18 (the MockTSwapPool fixture, and any 1:1-WETH asset), a 10e18 borrow yields 10e18 * 3e15 / 1e18 = 3e16 — 0.3%.

v2 drops the precision variable and a constant takes no slot

ThunderLoanUpgraded deletes s_feePrecision and reintroduces precision as a constant:

// src/upgradedProtocol/ThunderLoanUpgraded.sol
mapping(IERC20 => AssetToken) public s_tokenToAssetToken;
​
// The fee in WEI, it should have 18 decimals. Each flash loan takes a flat fee of the token price.
uint256 private s_flashLoanFee; // 0.3% ETH fee // line 96 — first child uint256
uint256 public constant FEE_PRECISION = 1e18;
​
mapping(IERC20 token => bool currentlyFlashLoaning) private s_currentlyFlashLoaning;

Solidity constants are inlined at compile time and consume no storage. The compiler therefore assigns s_flashLoanFee the slot that still holds leftover v1 s_feePrecision (1e18). The in-progress mapping also slides up one slot onto the old s_flashLoanFee cell.

| Child slot (after inherited OZ / Oracle storage) | ThunderLoan (v1) | Value after v1 initialize | ThunderLoanUpgraded (v2) |
| --- | --- | --- | --- |
| N | s_tokenToAssetToken | token → AssetToken | s_tokenToAssetToken (unchanged) |
| N+1 | s_feePrecision | 1e18 | **s_flashLoanFee** |
| N+2 | s_flashLoanFee | 3e15 | s_currentlyFlashLoaning |
| N+3 | s_currentlyFlashLoaning | mapping | (vacated) |

FEE_PRECISION is absent from this table because it has no slot.

Why every write path fails after upgradeTo

v2 initialize is still the only place that would assign the intended fee, and it still uses initializer:

function initialize(address tswapAddress) external initializer {
__Ownable_init();
__UUPSUpgradeable_init();
__Oracle_init(tswapAddress);
s_flashLoanFee = 3e15; // 0.3% ETH fee
}

On a production proxy that already executed v1 initialize, OpenZeppelin's initializer sees _initialized == 1 and reverts Initializable: contract is already initialized. UUPS authorization does not migrate storage:

function _authorizeUpgrade(address newImplementation) internal override onlyOwner { }

updateFlashLoanFee is the only remaining post-init writer, and upgradeTo never invokes it. After the planned cut-over:

getFee() == 1e18 // leftover v1 s_feePrecision, now aliased as s_flashLoanFee
FEE_PRECISION == 1e18 // new constant

v2 fee math then collapses to identity:

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;
}
fee = (amount * priceWeth / 1e18) * 1e18 / 1e18
= amount * priceWeth / 1e18
= 100% of the WETH-valued notional

For the fixture price 1e18 and a 10e18 borrow, getCalculatedFee jumps from 3e16 to 10e18 (333×). TSwap / MockTSwap only scales the WETH notional; it does not change the 1e18 / 1e18 rate.

flashloan consumes that fee twice:

  1. assetToken.updateExchangeRate(fee) immediately credits LPs as if a full-notional fee had been earned (newRate = oldRate * (supply + fee) / supply).

  2. The receiver must return amount + fee, or the call reverts ThunderLoan__NotPaidBack.

Exploitation

No attacker-crafted calldata is required. The triggering act is the documented owner upgrade; any later borrower pays the remapped fee.

Preconditions

  1. Deploy the ThunderLoan implementation and ERC1967Proxy(""), then ThunderLoan(proxy).initialize(tswapAddress) so s_feePrecision = 1e18, s_flashLoanFee = 3e15, and Initializable._initialized = 1.

  2. Owner setAllowedToken(token, true); LPs mint, approve, and deposit liquidity.

  3. Owner does not call updateFlashLoanFee(3e15) after upgrading.

Ordered trace

  1. View: getFee() == 3e15 and getCalculatedFee(token, 10e18) == 3e16 when getPriceInWeth == 1e18.

  2. Owner upgradeTo(address(new ThunderLoanUpgraded())) via UUPS onlyOwner _authorizeUpgrade.

  3. ThunderLoanUpgraded(proxy).initialize(tswapAddress) reverts Initializable: contract is already initialized.

  4. View: getFee() == 1e18 and getCalculatedFee(token, 10e18) == 10e18.

  5. Optional: flashloan(receiver, token, 10e18, params) demands a 10e18 fee and calls updateExchangeRate(10e18).

Foundry proof of concept

Uses the existing BaseTest proxy path (ThunderLoan impl + initialize(mockPoolFactory), MockTSwap price 1e18):

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import { BaseTest } from "./BaseTest.t.sol";
import { ThunderLoanUpgraded } from "../../src/upgradedProtocol/ThunderLoanUpgraded.sol";
​
contract UpgradeStorageCollisionTest is BaseTest {
function testUpgradeRemapsFlashLoanFeeOntoFeePrecision() public {
vm.prank(thunderLoan.owner());
thunderLoan.setAllowedToken(tokenA, true);
​
uint256 depositAmount = 1000e18;
tokenA.mint(address(this), depositAmount);
tokenA.approve(address(thunderLoan), depositAmount);
thunderLoan.deposit(tokenA, depositAmount);
​
uint256 amount = 10e18;
assertEq(thunderLoan.getFee(), 3e15);
assertEq(thunderLoan.getCalculatedFee(tokenA, amount), 3e16);
​
ThunderLoanUpgraded implV2 = new ThunderLoanUpgraded();
thunderLoan.upgradeTo(address(implV2));
ThunderLoanUpgraded loanV2 = ThunderLoanUpgraded(address(thunderLoan));
​
vm.expectRevert("Initializable: contract is already initialized");
loanV2.initialize(address(mockPoolFactory));
​
// Leftover v1 s_feePrecision is now read as s_flashLoanFee.
assertEq(loanV2.getFee(), 1e18);
assertEq(loanV2.getCalculatedFee(tokenA, amount), 10e18);
}
}

Observed after upgradeTo (no updateFlashLoanFee):

  • getFee() == 1000000000000000000

  • getCalculatedFee(tokenA, 10000000000000000000) == 10000000000000000000

  • pre-upgrade fee for the same notional was 30000000000000000

Impact

Severity: High.

The in-scope, owner-executed upgrade silently breaks the protocol's only fee invariant.

  • Economic DoS of the core function. A legitimate borrower must now source and repay a fee equal to 100% of the WETH-valued notional (the full borrowed amount when the oracle price is 1:1). flashloan stops being usable at the advertised 0.3% rate.

  • Forced wealth transfer. Any borrower who does repay transfers a full notional to LPs. updateExchangeRate applies that fee before repayment is checked, so a completed loan inflates the LP exchange rate by the entire notional. Later redeems extract the corresponding underlying.

  • No extra conditions. Reachability is the README-planned upgradeTo(ThunderLoanUpgraded) on a v1-initialized proxy. No reentrancy, no unusual ERC-20, and no malicious updateFlashLoanFee is required.

  • A post-hoc fee write is not a fix. Calling updateFlashLoanFee(3e15) after the fact would overwrite the remapped slot and restore a 0.3% rate, but that call is not part of the upgrade path, is easy to omit, and does not restore the shifted s_currentlyFlashLoaning slot. The layout remains permanently wrong for every future upgrade.

Liquidity stays in the protocol; borrowers and any integrator that assumes a 0.3% fee are the injured parties. Because this is the planned production cut-over, the bug ships the first time the owner upgrades.

Recommendation

Do not delete or reorder existing storage variables in a UUPS implementation. Promote precision to a constant without removing its slot, so s_flashLoanFee keeps the same cell it occupied in v1:

mapping(IERC20 => AssetToken) public s_tokenToAssetToken;
​
/// @dev Was `s_feePrecision`. Left in place so `s_flashLoanFee` keeps its v1 slot.
uint256 private __deprecated_feePrecision;
uint256 private s_flashLoanFee;
uint256 public constant FEE_PRECISION = 1e18;
​
mapping(IERC20 token => bool currentlyFlashLoaning) private s_currentlyFlashLoaning;

With that layout, leftover 1e18 stays unused, leftover 3e15 continues to be the fee, and no migration is required.

If a storage rewrite is ever needed, do not reuse initializer. Add a dedicated migrator and invoke it atomically with the upgrade:

function initializeV2() external reinitializer(2) {
s_flashLoanFee = 3e15;
}
​
// owner:
proxy.upgradeToAndCall(
address(newImpl),
abi.encodeCall(ThunderLoanUpgraded.initializeV2, ())
);

_authorizeUpgrade must not be treated as a migration hook; upgradeTo does not run implementation initializers.

Add a storage-layout diff to CI (forge inspect <Contract> storage-layout or the OpenZeppelin Upgrades plugin) and fail the build if any existing slot is removed, retyped, or reordered. Prefer ERC-7201 namespaced storage for new variables so later upgrades cannot collide with the v1 child layout.

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!