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

Storage collision between ThunderLoan and ThunderLoanUpgraded corrupts s_flashLoanFee and s_currentlyFlashLoaning after upgrade

blockend

Storage collision between ThunderLoan and ThunderLoanUpgraded corrupts s_flashLoanFee and s_currentlyFlashLoaning after upgrade

Description

  • Upgrading a UUPS proxy's implementation must preserve the exact storage variable ordering of the previous implementation, since storage is read by slot position, not by variable name.

  • ThunderLoanUpgraded removes s_feePrecision (replacing it with a constant, which occupies no storage slot) but leaves the rest of the variable order unchanged. This shifts s_flashLoanFee and s_currentlyFlashLoaning each up by one storage slot, causing them to read and write the wrong underlying data after the proxy is upgraded.

// ThunderLoan.sol
mapping(IERC20 => AssetToken) public s_tokenToAssetToken; // slot 0
uint256 private s_feePrecision; // slot 1
uint256 private s_flashLoanFee; // slot 2
mapping(IERC20 => bool) private s_currentlyFlashLoaning; // slot 3
​
// ThunderLoanUpgraded.sol
mapping(IERC20 => AssetToken) public s_tokenToAssetToken; // slot 0
// @> s_feePrecision removed; FEE_PRECISION is now a constant (no slot)
uint256 private s_flashLoanFee; // @> now slot 1, was slot 2
uint256 public constant FEE_PRECISION = 1e18;
mapping(IERC20 => bool) private s_currentlyFlashLoaning; // @> now slot 2, was slot 3

Risk

Likelihood:

  • This triggers automatically and unconditionally the moment the owner performs the intended, in-scope upgrade to ThunderLoanUpgraded — no attacker action is required.

Impact:

  • s_flashLoanFee silently becomes 1e18 (the old s_feePrecision value) instead of 3e15, making getCalculatedFee charge a 100% fee and effectively breaking all flash loans.

  • s_currentlyFlashLoaning reads/writes into what was the old fee slot, corrupting flash loan state tracking and potentially breaking repay() or the loan-active flag.

Proof of Concept

This test deploys ThunderLoanUpgraded, upgrades the existing proxy to point to it, then reads the fee through the new implementation. Because s_feePrecision was removed from the storage layout instead of being kept as a placeholder, every variable declared after it shifts up by one slot. The test shows that after upgrading, getFee() no longer returns the 3e15 (0.3%) fee that was set during initialize() — it instead returns 1e18, which is the value that used to live in the s_feePrecision slot. This confirms s_flashLoanFee is now silently reading the wrong storage slot, with no revert or error to signal the corruption.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import { Test, console } from "forge-std/Test.sol";
import { BaseTest, ThunderLoan } from "./BaseTest.t.sol";
import { AssetToken } from "../../src/protocol/AssetToken.sol";
import { MockFlashLoanReceiver } from "../mocks/MockFlashLoanReceiver.sol";
import { DepositOverRepay } from "../mocks/DepositOverRepay.sol";
import { ThunderLoanUpgraded } from "../../src/upgradedProtocol/ThunderLoanUpgraded.sol";
​
contract ThunderLoanTest is BaseTest {
uint256 constant AMOUNT = 10e18;
uint256 constant DEPOSIT_AMOUNT = AMOUNT * 100;
address liquidityProvider = address(123);
address user = address(456);
MockFlashLoanReceiver mockFlashLoanReceiver;
​
function setUp() public override {
super.setUp();
vm.prank(user);
mockFlashLoanReceiver = new MockFlashLoanReceiver(address(thunderLoan));
}
​
function testStorageCollisionAfterUpgrade() public setAllowedToken hasDeposits {
uint256 feeBeforeUpgrade = thunderLoan.getFee();
assertEq(feeBeforeUpgrade, 3e15); // 0.3%, set in initialize()
​
ThunderLoanUpgraded upgraded = new ThunderLoanUpgraded();
vm.prank(thunderLoan.owner());
thunderLoan.upgradeTo(address(upgraded));
​
uint256 feeAfterUpgrade = ThunderLoanUpgraded(address(thunderLoan)).getFee();
​
// s_flashLoanFee now reads the old s_feePrecision slot (1e18) instead of 3e15
assertEq(feeAfterUpgrade, 1e18);
assertNotEq(feeAfterUpgrade, feeBeforeUpgrade);
}
}

Recommended Mitigation

Preserve the original storage layout — keep s_feePrecision in place (even if unused) rather than deleting it, or append new/changed variables only at the end of the layout.

mapping(IERC20 => AssetToken) public s_tokenToAssetToken;
​
- uint256 private s_flashLoanFee;
- uint256 public constant FEE_PRECISION = 1e18;
+ uint256 private s_feePrecision;
+ uint256 private s_flashLoanFee;
​
mapping(IERC20 token => bool currentlyFlashLoaning) private s_currentlyFlashLoaning;
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!