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

A storage collision in ThunderLoanUpgraded sets the flash loan fee to 100%, rendering the contract unusable.

eliab256

Root + Impact

Description

  • ThunderLoan uses an upgradeable UUPS proxy pattern where storage layout must remain consistent across versions. In V1, `s_feePrecision` is declared as a state variable occupying slot X+1, followed by `s_flashLoanFee` at slot X+2, and both are used in `getCalculatedFee()` to compute the flash loan fee.

  • In `ThunderLoanUpgraded`, `s_feePrecision` is removed as a state variable and replaced by a `constant` named `FEE_PRECISION`, which does not occupy a storage slot. This shifts `s_flashLoanFee` from slot X+2 to slot X+1, causing it to read the stale value of the old `s_feePrecision` (1e18) instead of the intended `s_flashLoanFee` (3e15), resulting in a fee of 100% of the borrowed amount.

/*//////////////////////////////////////////////////////////////
STATE VARIABLES
//////////////////////////////////////////////////////////////*/
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;
​
mapping(IERC20 token => bool currentlyFlashLoaning)
private s_currentlyFlashLoaning;

Risk

Likelihood:

  • This problem will occur as soon as this logig is implemented

Impact:

  • Setting the fees for flash loan to 100% make the flashloan unusable and protocol become useless

Proof of Concept

The test verifies :
it confirms the correct fee of 0.3% (3e15) before the upgrade.
After the upgrade, it demonstrates that s_flashLoanFee shifts to the slot previously occupied by s_feePrecision, causing getFee() to return 1e18 instead of 3e15.
Finally, it proves that getCalculatedFee() now returns an amount equal to the entire borrowed amount, confirming a 100% fee that makes the protocol unusable for any borrower.


contract POCtest is Test {
ThunderLoan thunderLoanImplementation;
MockPoolFactory mockPoolFactory;
ERC1967Proxy proxy;
ThunderLoan thunderLoan;
​
ERC20Mock weth;
ERC20Mock tokenA;
ERC20Mock tokenB;
ERC20Mock6Decimals tokenWith6Decimals;
AssetToken assetToken6Decimals;
AssetToken assetTokenA;
AssetToken assetTokenB;
​
address depositer = makeAddr("depositer");
address flahLoanReceiver = makeAddr("flashLoanReceiver");
address flashLoanAttacker = makeAddr("flashLoanAttacker");
​
function setUp() public virtual {
thunderLoan = new ThunderLoan();
mockPoolFactory = new MockPoolFactory();
​
weth = new ERC20Mock();
tokenA = new ERC20Mock();
tokenB = new ERC20Mock();
tokenWith6Decimals = new ERC20Mock6Decimals();
​
mockPoolFactory.createPool(address(tokenA));
mockPoolFactory.createPool(address(tokenWith6Decimals));
proxy = new ERC1967Proxy(address(thunderLoan), "");
thunderLoan = ThunderLoan(address(proxy));
thunderLoan.initialize(address(mockPoolFactory));
​
assetTokenB = thunderLoan.setAllowedToken(
IERC20(address(tokenB)),
true
);
​
assetToken6Decimals = thunderLoan.setAllowedToken(
IERC20(address(tokenWith6Decimals)),
true
);
​
tokenA.mint(depositer, 50000 * 10 ** tokenA.decimals()); //fund depositer with tokenA
​
//fund depositer with token with 6 decimals
tokenWith6Decimals.mint(
depositer,
5000 * 10 ** tokenWith6Decimals.decimals()
); // 5000 tokens in 6-decimal representation
}
function testDOSdueToupgradeImplementationAndSettingFeesAtOneUndredPercent() public{
// SETUP: allow tokenA and fund the depositer
assetTokenA = thunderLoan.setAllowedToken(IERC20(address(tokenA)),true);
​
vm.startPrank(depositer);
tokenA.approve(address(thunderLoan), 50000e18);
thunderLoan.deposit(IERC20(address(tokenA)), 50000e18);
vm.stopPrank();
​
// STEP 1: verify fee BEFORE upgrade → should be 0.3%
uint256 borrowAmount = 1000e18;
uint256 feeBefore = thunderLoan.getCalculatedFee(
IERC20(address(tokenA)),
borrowAmount
);
uint256 feeRawBefore = thunderLoan.getFee(); // should be 3e15
​
console.log("=== BEFORE UPGRADE ===");
console.log("s_flashLoanFee slot value : ", feeRawBefore); // 3e15
console.log("Calculated fee on 1000e18 : ", feeBefore); // ~3e15
​
assertEq(feeRawBefore, 3e15);
​
// STEP 2: upgrade to ThunderLoanUpgraded
ThunderLoanUpgraded thunderLoanUpgradedImplementation = new ThunderLoanUpgraded();
​
vm.prank(thunderLoan.owner());
thunderLoan.upgradeTo(address(thunderLoanUpgradedImplementation));
​
// Cast proxy to upgraded interface
ThunderLoanUpgraded thunderLoanUpgraded = ThunderLoanUpgraded(
address(proxy)
);
​
// STEP 3: verify fee AFTER upgrade → reads s_feePrecision (1e18) instead of s_flashLoanFee (3e15) due to storage collision
uint256 feeRawAfter = thunderLoanUpgraded.getFee(); // reads wrong slot
uint256 feeAfter = thunderLoanUpgraded.getCalculatedFee(
IERC20(address(tokenA)),
borrowAmount
);
​
console.log("=== AFTER UPGRADE ===");
console.log("s_flashLoanFee slot value : ", feeRawAfter); // 1e18 ← collision
console.log("Calculated fee on 1000e18 : ", feeAfter); // = borrowAmount
​
// Storage collision: s_flashLoanFee now reads old s_feePrecision = 1e18
assertEq(feeRawAfter, 1e18);
// Fee is now 100% of borrowed amount, not 0.3%
assertNotEq(feeAfter, feeBefore);
assertEq(feeAfter, borrowAmount);
}}

this is the contract FlashLoanReceiver i used for the test:

contract FlashLoanReceiver {
IThunderLoanFixed private immutable i_thunderLoan;
​
constructor(address thunderLoan) {
i_thunderLoan = IThunderLoanFixed(thunderLoan);
}
​
//amount 1
function requestFlashLoan(
address _underlyingToken,
uint256 amount
) external {
i_thunderLoan.flashloan(address(this), _underlyingToken, amount, "");
}
​
function executeOperation(
address token,
uint256 amount,
uint256 fee,
address initiator,
bytes calldata params
) external {
IERC20(token).approve(address(i_thunderLoan), amount + fee);
i_thunderLoan.repay(IERC20(token), amount + fee);
}
}

Recommended Mitigation

To preserve the storage layout across upgrades, `s_feePrecision` should not be removed as a state variable. Instead, it can be retained in its original slot and simply left unused, or renamed to signal its deprecated status. This ensures that `s_flashLoanFee` remains at slot X+2 as expected, reading the correct value of `3e15` after the upgrade. Alternatively, the constant `FEE_PRECISION` should be added BEFORE the existing variables to maintain slot consistency — but the safest approach is always to never remove or reorder existing state variables.

contract ThunderLoanUpgraded is Initializable, OwnableUpgradeable, UUPSUpgradeable, OracleUpgradeable {
​
mapping(IERC20 => AssetToken) public s_tokenToAssetToken;
​
- uint256 private s_flashLoanFee;
- uint256 public constant FEE_PRECISION = 1e18;
​
+ uint256 private s_feePrecision; // slot X+1: retained to preserve storage layout
+ uint256 private s_flashLoanFee; // slot X+2: now correctly reads 3e15
+ uint256 public constant FEE_PRECISION = 1e18; // no slot consumed
​
mapping(IERC20 token => bool currentlyFlashLoaning) private s_currentlyFlashLoaning;
}
Updates

Lead Judging Commences

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