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

UUPS upgrade to ThunderLoanUpgraded silently corrupts the flash-loan fee from 0.3% to 100% via a storage slot collision

comphygxp

Root + Impact

Description

  • ThunderLoan.sol's own state variables are declared in this order: s_tokenToAssetToken -> s_feePrecision (=1e18) -> s_flashLoanFee (=3e15, i.e. 0.3%) -> s_currentlyFlashLoaning. Each of these occupies its own storage slot in the proxy.

  • ThunderLoanUpgraded.sol (explicitly in-scope per the README: "We are planning to upgrade from the current ThunderLoan contract to the ThunderLoanUpgraded contract. Please include this upgrade in scope") removes s_feePrecision entirely, replacing it with a constant FEE_PRECISION that consumes no storage slot, and leaves no gap in its place. The new declaration order becomes s_tokenToAssetToken -> s_flashLoanFee -> s_currentlyFlashLoaning.

  • Because a UUPS upgrade only swaps the implementation bytecode and never touches, clears, or migrates the proxy's own storage, every variable after the deleted slot physically shifts up by one slot. The variable named s_flashLoanFee in the new implementation now physically occupies the slot that used to belong to s_feePrecision - so after upgrading, s_flashLoanFee silently reads back 1e18 (100%) instead of the actually-configured 3e15 (0.3%).

  • Confirmed directly from the compiler's own storage layout output (forge inspect ThunderLoan storage-layout vs forge inspect ThunderLoanUpgraded storage-layout): s_feePrecision sits at slot 203 and s_flashLoanFee at slot 204 in ThunderLoan; in ThunderLoanUpgraded, s_flashLoanFee sits at slot 203 - a direct, physical collision, not a manual/inferred claim.

  • This fires deterministically for every caller the instant the owner performs the standard, documented upgradeTo() call - no attacker action, no special timing, and no unusual preconditions are required. It corrupts the core fee-calculation logic used inside flashloan(), making flash loans effectively unusable at the advertised rate (an honest borrower who funds exactly the documented 0.3% fee cannot repay, since the protocol now demands 100%).

// ThunderLoan.sol - BEFORE upgrade (slots shown from forge inspect storage-layout)
mapping(IERC20 => AssetToken) private s_tokenToAssetToken; // slot 202
@> uint256 private s_feePrecision; // slot 203 (=1e18)
@> uint256 private s_flashLoanFee; // slot 204 (=3e15, 0.3%)
mapping(IERC20 => bool) private s_currentlyFlashLoaning; // slot 205
​
// ThunderLoanUpgraded.sol - AFTER upgrade: s_feePrecision removed, NO gap left
mapping(IERC20 => AssetToken) private s_tokenToAssetToken; // slot 202
@> uint256 private s_flashLoanFee; // slot 203 <- physically the OLD s_feePrecision slot, silently reads 1e18
mapping(IERC20 => bool) private s_currentlyFlashLoaning; // slot 204
uint256 public constant FEE_PRECISION = 1e18; // constant, no slot

Risk

Likelihood:

  • Reason 1 // Triggers automatically and deterministically the moment the owner executes the standard, officially-documented, in-scope upgrade call (upgradeTo) - no attacker action or special timing needed.

  • Reason 2 // The upgrade path is explicitly called out as in-scope by the project's own README, so this is not a hypothetical/out-of-scope future event - it is the exact operation reviewers were asked to evaluate.

Impact:

  • Impact 1 // Core business logic (flash-loan fee calculation) is deterministically corrupted from 0.3% to 100%, making flashloan() effectively unusable for any honest borrower using the documented fee.

  • Impact 2 // The corruption is silent - no revert, no event, no error - so integrators and the protocol itself have no on-chain signal that the fee schedule just changed by over 300x.

Proof of Concept

Ran with forge test --match-path "test/PoC_3.t.sol" -vv: all 3 tests pass. test_FeeIsCorrectBeforeUpgrade confirms the baseline (getFee()==3e15, getFeePrecision()==1e18). test_UpgradeCorruptsFeeToOneHundredPercent performs the official thunderLoan.upgradeTo(address(newThunderLoanUpgraded)) upgrade with zero storage migration, then shows the same proxy's getFee() now returns 1e18 (100%) instead of 3e15, and getCalculatedFee(tokenA, 100e18) returns 100e18 (100% of principal) instead of the expected 0.3e18. test_UpgradeBreaksFlashloanForHonestBorrower funds an honest MockFlashLoanReceiver with exactly the documented 0.3% fee, performs the same official upgrade, and shows flashloan(100e18) reverts because the corrupted fee now demands the entire loan amount - a real, deterministic breakage of live functionality, not just a dangling getter. Full regression suite passing, no regressions.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import { BaseTest } from "./unit/BaseTest.t.sol";
import { ThunderLoan } from "../src/protocol/ThunderLoan.sol";
import { ThunderLoanUpgraded } from "../src/upgradedProtocol/ThunderLoanUpgraded.sol";
import { AssetToken } from "../src/protocol/AssetToken.sol";
import { MockFlashLoanReceiver } from "./mocks/MockFlashLoanReceiver.sol";
​
contract PoC_3_StorageCollision is BaseTest {
MockFlashLoanReceiver mockFlashLoanReceiver;
​
function setUp() public override {
super.setUp();
thunderLoan.setAllowedToken(tokenA, true);
mockFlashLoanReceiver = new MockFlashLoanReceiver(address(thunderLoan));
}
​
function test_FeeIsCorrectBeforeUpgrade() public {
assertEq(thunderLoan.getFee(), 3e15, "pre-upgrade fee should be 0.3%");
assertEq(thunderLoan.getFeePrecision(), 1e18, "pre-upgrade precision should be 1e18");
}
​
function test_UpgradeCorruptsFeeToOneHundredPercent() public {
assertEq(thunderLoan.getFee(), 3e15);
​
ThunderLoanUpgraded newImplementation = new ThunderLoanUpgraded();
thunderLoan.upgradeTo(address(newImplementation));
ThunderLoanUpgraded upgraded = ThunderLoanUpgraded(address(proxy));
​
uint256 feeAfterUpgrade = upgraded.getFee();
assertEq(
feeAfterUpgrade,
1e18,
"post-upgrade getFee() reads the OLD s_feePrecision slot value (1e18) due to storage collision"
);
​
AssetToken assetToken = upgraded.getAssetFromToken(tokenA);
tokenA.mint(address(assetToken), 1_000e18);
​
uint256 loanAmount = 100e18;
uint256 calculatedFee = upgraded.getCalculatedFee(tokenA, loanAmount);
assertEq(calculatedFee, loanAmount, "post-upgrade fee is 100% of the loan, not 0.3%");
}
​
function test_UpgradeBreaksFlashloanForHonestBorrower() public {
tokenA.mint(address(this), 1_000e18);
tokenA.approve(address(thunderLoan), 1_000e18);
thunderLoan.deposit(tokenA, 1_000e18);
​
ThunderLoanUpgraded newImplementation = new ThunderLoanUpgraded();
thunderLoan.upgradeTo(address(newImplementation));
ThunderLoanUpgraded upgraded = ThunderLoanUpgraded(address(proxy));
​
uint256 loanAmount = 100e18;
uint256 expectedFeeAtDocumentedRate = (loanAmount * 3e15) / 1e18;
tokenA.mint(address(mockFlashLoanReceiver), expectedFeeAtDocumentedRate);
​
vm.expectRevert();
upgraded.flashloan(address(mockFlashLoanReceiver), tokenA, loanAmount, "");
​
uint256 corruptedFee = upgraded.getCalculatedFee(tokenA, loanAmount);
assertEq(corruptedFee, loanAmount, "flashloan effectively unusable: fee == 100% of principal");
}
}

Recommended Mitigation

// ThunderLoanUpgraded.sol
mapping(IERC20 => AssetToken) private s_tokenToAssetToken;
+ uint256 private __deprecated_feePrecision_gap; // keep slot to avoid storage collision
uint256 private s_flashLoanFee;
mapping(IERC20 => bool) private s_currentlyFlashLoaning;
+ uint256 public constant FEE_PRECISION = 1e18;
- uint256 public constant FEE_PRECISION = 1e18; // (previously declared before s_flashLoanFee, causing the shift)

  1. Do not delete s_feePrecision outright - keep an equal-sized placeholder in its exact original position (e.g. rename to __deprecated_feePrecision_gap) so s_flashLoanFee and s_currentlyFlashLoaning keep their pre-upgrade physical slots; add the new FEE_PRECISION constant after all existing state variables, not in place of one.

  2. Make storage-layout diffing a mandatory CI gate before any upgrade ships: run forge inspect <OldImpl> storage-layout and forge inspect <NewImpl> storage-layout and fail the build on any slot mismatch, or use OpenZeppelin's @openzeppelin/upgrades-core validation tooling.

  3. If this has already shipped, deploy a corrective implementation whose reinitializer explicitly re-writes the correct s_flashLoanFee value (e.g. 3e15), since a normal upgrade does not re-run initialize().

  4. General rule for all future upgrades: state variables are append-only. Never delete or reorder an existing variable; replace removals with an equal-sized __gap, and always add new variables at the end of the existing list.

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!