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

`ThunderLoanUpgraded` reorders storage, so after the upgrade `s_flashLoanFee` reads the old `s_feePrecision` slot and every flash loan is charged a 100% fee

augustwh11

The upgrade turns s_feePrecision into a constant, which deletes a storage slot and shifts s_flashLoanFee onto it

Description

  • This is a UUPS proxy: the proxy keeps the storage, the implementation only supplies the code. An upgrade is therefore only safe if the new implementation declares its state variables in exactly the same order, so that each name keeps pointing at the slot that already holds its value. The README asks explicitly for this upgrade to be reviewed.

  • ThunderLoanUpgraded promotes s_feePrecision to constant FEE_PRECISION. Constants live in bytecode, not storage, so the variable that followed it moves up one slot. s_flashLoanFee therefore starts reading the slot that still contains s_feePrecision's value, 1e18 — and 1e18 is exactly the fee denominator, so the fee ratio becomes 1e18 / 1e18 = 1: 100% of the borrowed value.

// src/protocol/ThunderLoan.sol:93-99 (current implementation - the one holding funds)
mapping(IERC20 => AssetToken) public s_tokenToAssetToken; // slot 202
@> uint256 private s_feePrecision; // slot 203 <- initialised to 1e18
@> uint256 private s_flashLoanFee; // 0.3% ETH fee // slot 204 <- initialised to 3e15
mapping(IERC20 token => bool currentlyFlashLoaning) private s_currentlyFlashLoaning; // slot 205
// src/upgradedProtocol/ThunderLoanUpgraded.sol:93-99 (the upgrade)
mapping(IERC20 => AssetToken) public s_tokenToAssetToken; // slot 202
@> uint256 private s_flashLoanFee; // 0.3% ETH fee // slot 203 <- now reads 1e18
@> uint256 public constant FEE_PRECISION = 1e18; // <- no slot at all
mapping(IERC20 token => bool currentlyFlashLoaning) private s_currentlyFlashLoaning; // slot 204

forge inspect ... storageLayout confirms the slot numbers directly:

ThunderLoan ThunderLoanUpgraded
202 s_tokenToAssetToken 202 s_tokenToAssetToken
203 s_feePrecision 203 s_flashLoanFee <-- collision
204 s_flashLoanFee 204 s_currentlyFlashLoaning
205 s_currentlyFlashLoaning

initialize is protected by initializer and cannot be re-run after the upgrade, so there is no path that corrects the value on the way through:

// src/upgradedProtocol/ThunderLoanUpgraded.sol:139-144
function initialize(address tswapAddress) external initializer {
...
@> s_flashLoanFee = 3e15; // @> unreachable after the upgrade - `initializer` already consumed
}

The fee then flows straight into the quote every borrower is charged:

// src/upgradedProtocol/ThunderLoanUpgraded.sol:244-249
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; // @> s_flashLoanFee == FEE_PRECISION
}

Every guard the protocol has passes this value. updateFlashLoanFee only rejects newFee > FEE_PRECISION (:251-256), and 1e18 is exactly at the bound, so nothing anywhere flags the state as invalid.

Risk

Likelihood:

  • Certain, not probable. It is not triggered by an attacker or by a race — it is the deterministic consequence of performing the upgrade the README asks to be reviewed. The moment upgradeTo lands, the fee is wrong for every subsequent call.

  • Nothing warns. The upgrade transaction succeeds, getFee() returns a plausible-looking 1e18, and the OpenZeppelin upgrade-safety plugin is not used anywhere in the repository, so no tooling is in the way either.

Impact:

  • Borrowers are charged the entire loan. On a 100-token loan the fee goes from 0.3 to 100 tokens — 333× the intended rate.

  • This is not merely "flash loans stop working". A receiver contract that approves amount + fee and repays what it is quoted — which is exactly what the project's own test/mocks/MockFlashLoanReceiver.sol does, and the shape every real integrator copies — will hand over its whole balance without any of its own logic noticing. Existing integrations built against the pre-upgrade contract lose funds on their next call.

  • Anyone whose receiver cannot cover 100% of the loan simply reverts, so the flash-loan product is dead for everyone else. The protocol's only revenue mechanism is destroyed either way.

  • The owner can repair it with updateFlashLoanFee(3e15), but only after noticing, and only after the borrowers who called first have already paid.

Proof of Concept

Deploy ThunderLoan behind a proxy exactly as script/DeployThunderLoan.s.sol does, upgrade to ThunderLoanUpgraded, and read the fee back. The raw slot is read with vm.load so the collision is visible directly and not inferred.

Save as test/PoC_H03.t.sol and run forge test --mt test_H03 -vv:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import { Test, console2 } from "forge-std/Test.sol";
import { ThunderLoan } from "../src/protocol/ThunderLoan.sol";
import { ThunderLoanUpgraded } from "../src/upgradedProtocol/ThunderLoanUpgraded.sol";
import { ERC20Mock } from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import { MockPoolFactory } from "./mocks/MockPoolFactory.sol";
​
contract PoC_H03 is Test {
ThunderLoan thunderLoan;
ERC20Mock tokenA;
​
function setUp() public {
MockPoolFactory factory = new MockPoolFactory();
tokenA = new ERC20Mock();
factory.createPool(address(tokenA));
thunderLoan = ThunderLoan(address(new ERC1967Proxy(address(new ThunderLoan()), "")));
thunderLoan.initialize(address(factory));
thunderLoan.setAllowedToken(tokenA, true);
}
​
function test_H03_storageCollisionAfterUpgrade() public {
uint256 feeBefore = thunderLoan.getFee();
uint256 calcBefore = thunderLoan.getCalculatedFee(tokenA, 100e18);
​
// slot 203 is s_feePrecision today, and becomes s_flashLoanFee after the upgrade
uint256 slot203 = uint256(vm.load(address(thunderLoan), bytes32(uint256(203))));
uint256 slot204 = uint256(vm.load(address(thunderLoan), bytes32(uint256(204))));
​
thunderLoan.upgradeTo(address(new ThunderLoanUpgraded()));
​
uint256 feeAfter = ThunderLoanUpgraded(address(thunderLoan)).getFee();
uint256 calcAfter = ThunderLoanUpgraded(address(thunderLoan)).getCalculatedFee(tokenA, 100e18);
​
console2.log("raw slot 203 (s_feePrecision) :", slot203);
console2.log("raw slot 204 (s_flashLoanFee) :", slot204);
console2.log("s_flashLoanFee before upgrade :", feeBefore);
console2.log("s_flashLoanFee after upgrade :", feeAfter);
console2.log("fee on a 100e18 loan, before :", calcBefore);
console2.log("fee on a 100e18 loan, after :", calcAfter);
​
assertEq(feeBefore, 3e15, "0.3% before");
assertEq(feeAfter, slot203, "after the upgrade the fee IS the old precision slot");
assertEq(feeAfter, 1e18, "which is 1e18");
assertEq(calcAfter, 100e18, "fee is now 100% of the borrowed amount");
assertEq(calcAfter * 1000 / calcBefore, 333_333, "333x the intended fee");
}
}

Output:

[PASS] test_H03_storageCollisionAfterUpgrade() (gas: 5660391)
Logs:
raw slot 203 (s_feePrecision) : 1000000000000000000
raw slot 204 (s_flashLoanFee) : 3000000000000000
s_flashLoanFee before upgrade : 3000000000000000
s_flashLoanFee after upgrade : 1000000000000000000
fee on a 100e18 loan, before : 300000000000000000
fee on a 100e18 loan, after : 100000000000000000000

The fee variable does not change value across the upgrade — it changes which slot it is, and lands on the one holding 1e18.

For completeness, s_currentlyFlashLoaning also shifts, from slot 205 to 204. That one is harmless: a mapping never reads its own slot, only keccak256(key . slot), and every entry is false between transactions anyway. The damage is confined to s_flashLoanFee.

Recommended Mitigation

Storage layout must be append-only across an upgrade. Keep the slot occupied and leave the constant out of the sequence.

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
- uint256 public constant FEE_PRECISION = 1e18;
+ /// @dev slot preserved from ThunderLoan.s_feePrecision - do NOT remove or reorder.
+ /// FEE_PRECISION replaces its use, but the slot must stay occupied.
+ uint256 private __deprecated_feePrecision;
+ // 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
​
mapping(IERC20 token => bool currentlyFlashLoaning) private s_currentlyFlashLoaning;
+
+ uint256 public constant FEE_PRECISION = 1e18;

Two process changes worth making alongside it, because this class of bug is invisible in code review but trivial to catch mechanically:

  • Add @openzeppelin/upgrades-core (or forge inspect <contract> storageLayout diffed in CI) to the upgrade path — the layout diff above is generated in one command and would have failed the build.

  • Add a test that upgrades a funded, initialised proxy and asserts that getFee(), owner() and getAssetFromToken() are unchanged afterwards. The repository currently has no such test.

Updates

Lead Judging Commences

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