Thunder Loan

AI First Flight #7

Beginner FriendlyFoundryDeFiOracle

EXP

View results

Previous Next

Submission Details

Severity: high

Valid

ThunderLoanUpgraded drops the s_feePrecision storage slot, so after the planned upgrade s_flashLoanFee reads V1's precision value (1e18) and the flash-loan fee rises from 0.3% to 100% of the borrowed value

friedsocks233

Root + Impact

Description

The README (L57) plans an upgrade of the proxy from ThunderLoan to ThunderLoanUpgraded. That upgrade should keep all existing proxy state, so the flash-loan fee stays at 3e15 / 1e18 (0.3%) and getCalculatedFee keeps charging 0.3% of the borrowed amount's WETH value.
V1 stores s_feePrecision (set to 1e18) and s_flashLoanFee (set to 3e15) in consecutive storage slots directly after s_tokenToAssetToken. V2 removes s_feePrecision and replaces it with the constant FEE_PRECISION, which takes no storage slot. V2's s_flashLoanFee therefore lands in the slot where V1 stored s_feePrecision, and every later variable moves up one slot. After upgradeTo, getFee() returns 1e18 and getCalculatedFee charges 100% of the borrowed value. Nothing on the upgrade path corrects this: _authorizeUpgrade only checks onlyOwner, and initialize cannot run a second time.

// src/protocol/ThunderLoan.sol L93-L99 (V1 layout currently held by the proxy)

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;

// src/protocol/ThunderLoan.sol L143-L144 (values written by V1 initialize)

s_feePrecision = 1e18;

s_flashLoanFee = 3e15; // 0.3% ETH fee

// src/upgradedProtocol/ThunderLoanUpgraded.sol L93-L99 (V2 layout)

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;

// src/upgradedProtocol/ThunderLoanUpgraded.sol L244-L249 (V2 reads the shifted slot)

function getCalculatedFee(IERC20 token, uint256 amount) public view returns (uint256 fee) {

//slither-disable-next-line divide-before-multiply

uint256 valueOfBorrowedToken = (amount * getPriceInWeth(address(token))) / FEE_PRECISION;

//slither-disable-next-line divide-before-multiply

fee = (valueOfBorrowedToken * s_flashLoanFee) / FEE_PRECISION;

}

Risk

Likelihood:

The trigger is the owner's own planned upgradeTo(ThunderLoanUpgraded), which the README announces and puts in scope. No attacker and no owner mistake are needed, and every proxy upgraded from V1 gets the wrong fee.
Neither _authorizeUpgrade (an empty onlyOwner function) nor initialize (already consumed on the proxy) checks or resets the layout, so the wrong fee is live from the upgrade transaction onward.
It is not rated High because it depends on a single privileged, one-time action, and the owner can restore the fee with updateFlashLoanFee(3e15) once the problem is noticed (the PoC confirms this). That limits how long the wrong fee stays in effect.

Impact:

The flash-loan fee goes from 3e15 to 1e18, 333 times the intended rate. getCalculatedFee on a 100e18 loan of a token priced 1:1 in WETH returns 100e18 (100%) instead of 0.3e18. Flash loans budgeted for the normal 0.3% fee revert with ThunderLoan__NotPaidBack, so the protocol's core function stops working and LP fee income stops until the owner intervenes.
Borrowers lose funds directly: a receiver that repays amount + the quoted fee from its own balance pays 100e18 instead of 0.3e18 for the same 100e18 loan.
LP principal is not lost; the inflated fee goes to LPs through the exchange rate. As side effects, getFeePrecision() no longer exists on the proxy, and the old 3e15 value is orphaned in the slot V2 uses as the s_currentlyFlashLoaning mapping base. That slot is harmless while all flags read false.

Proof of Concept

The PoC (test/PoC_TL-03.t.sol) puts ThunderLoan behind an ERC1967Proxy and initializes it. It allows a mock token priced 1:1 in WETH, deposits LP funds, and then has the legitimate owner call upgradeTo(new ThunderLoanUpgraded()). The tests assert the following:

Under V1, getFee() == 3e15, raw slot 203 == 1e18 and raw slot 204 == 3e15.
After the upgrade the storage is unchanged, but V2 getFee() == 1e18, getCalculatedFee(token, 100e18) == 100e18, and initialize reverts.
A flash loan that succeeds on V1 with a 1e18 fee buffer reverts on V2 with ThunderLoan__NotPaidBack.
A receiver that repays amount + fee pays 0.3e18 on V1 and 100e18 on V2.
The owner can restore the fee with updateFlashLoanFee(3e15).
Run it with forge test --match-path test/PoC_TL-03.t.sol -vv.

File test/PoC_TL-03.t.sol; run FOUNDRY_FFI=false forge test --match-path test/PoC_TL-03.t.sol -vv.

// SPDX-License-Identifier: MIT

pragma solidity 0.8.20;

import { Test, console } from "forge-std/Test.sol";

import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

import { ERC20Mock } from "@openzeppelin/contracts/mocks/ERC20Mock.sol";

import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

import { ThunderLoan } from "../src/protocol/ThunderLoan.sol";

import { ThunderLoanUpgraded } from "../src/upgradedProtocol/ThunderLoanUpgraded.sol";

import { AssetToken } from "../src/protocol/AssetToken.sol";

import { MockPoolFactory } from "./mocks/MockPoolFactory.sol";

/// Surface shared by V1 and V2, so one receiver works against both implementations.

interface ITLRepay {

function repay(IERC20 token, uint256 amount) external;

}

/// A flash-loan receiver that repays `amount + fee` out of what it holds (the borrowed

/// amount plus a pre-funded fee buffer). If the buffer cannot cover the fee it repays

/// everything it has, so an under-budgeted loan fails on ThunderLoan's own

/// NotPaidBack check rather than on an ERC20 balance error.

contract BufferedReceiver {

address public immutable tl;

uint256 public lastFeeQuoted;

constructor(address _tl) {

tl = _tl;

}

function executeOperation(

address token,

uint256 amount,

uint256 fee,

address, /* initiator */

bytes calldata /* params */

)

external

returns (bool)

{

lastFeeQuoted = fee;

uint256 owed = amount + fee;

uint256 bal = IERC20(token).balanceOf(address(this));

uint256 pay = owed < bal ? owed : bal;

IERC20(token).approve(tl, pay);

ITLRepay(tl).repay(IERC20(token), pay);

return true;

}

contract PoC_TL03 is Test {

// ERC1967 implementation slot: bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1)

bytes32 constant IMPL_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;

// Slots of ThunderLoan's own state variables behind the OZ 4.9.3 base layout.

uint256 constant SLOT_MAP = 202; // s_tokenToAssetToken (V1 and V2)

uint256 constant SLOT_203 = 203; // V1: s_feePrecision | V2: s_flashLoanFee

uint256 constant SLOT_204 = 204; // V1: s_flashLoanFee | V2: s_currentlyFlashLoaning base

uint256 constant SLOT_205 = 205; // V1: s_currentlyFlashLoaning base | V2: unused

address owner = makeAddr("owner");

address lp = makeAddr("liquidityProvider");

ERC20Mock tokenA;

MockPoolFactory poolFactory;

address proxy;

ThunderLoan v1; // proxy viewed through the V1 ABI

uint256 constant LP_DEPOSIT = 1000e18;

uint256 constant LOAN = 100e18;

uint256 constant FEE_BUFFER = 1e18; // generous for a 0.3% fee on 100e18 (0.3e18)

function setUp() public {

tokenA = new ERC20Mock();

poolFactory = new MockPoolFactory();

poolFactory.createPool(address(tokenA)); // MockTSwapPool: 1 tokenA = 1 WETH

ThunderLoan impl = new ThunderLoan();

proxy = address(new ERC1967Proxy(address(impl), ""));

v1 = ThunderLoan(proxy);

vm.prank(owner);

v1.initialize(address(poolFactory));

vm.prank(owner);

v1.setAllowedToken(IERC20(address(tokenA)), true);

tokenA.mint(lp, LP_DEPOSIT);

vm.startPrank(lp);

tokenA.approve(proxy, LP_DEPOSIT);

v1.deposit(IERC20(address(tokenA)), LP_DEPOSIT);

vm.stopPrank();

}

function _upgradeAsOwner() internal returns (ThunderLoanUpgraded v2) {

ThunderLoanUpgraded v2Impl = new ThunderLoanUpgraded();

vm.prank(owner);

v1.upgradeTo(address(v2Impl)); // the README's planned upgrade, done by the legitimate owner

assertEq(address(uint160(uint256(vm.load(proxy, IMPL_SLOT)))), address(v2Impl), "upgrade did not happen");

v2 = ThunderLoanUpgraded(proxy);

}

function _load(uint256 slot) internal view returns (uint256) {

return uint256(vm.load(proxy, bytes32(slot)));

}

/// Limb 1 / attack path: the fee read by V2 is V1's precision slot (1e18), i.e. 100%.

function test_upgradeMovesFeeFrom0_3PercentTo100Percent() public {

// ---- Baseline: V1 behaves as intended ----

assertEq(v1.getFee(), 3e15, "V1 fee should be 0.3%");

assertEq(v1.getFeePrecision(), 1e18, "V1 precision");

assertEq(v1.getCalculatedFee(IERC20(address(tokenA)), LOAN), 3e17, "V1 fee on 100e18 = 0.3e18");

// Raw layout under V1

assertEq(_load(SLOT_203), 1e18, "V1 slot 203 = s_feePrecision");

assertEq(_load(SLOT_204), 3e15, "V1 slot 204 = s_flashLoanFee");

assertEq(address(uint160(_load(SLOT_MAP))), address(0), "mapping base slot holds no value");

console.log("V1 getFee() :", v1.getFee());

console.log("V1 getCalculatedFee(100e18) :", v1.getCalculatedFee(IERC20(address(tokenA)), LOAN));

// ---- The planned owner upgrade ----

ThunderLoanUpgraded v2 = _upgradeAsOwner();

// Storage itself is untouched by the upgrade ...

assertEq(_load(SLOT_203), 1e18);

assertEq(_load(SLOT_204), 3e15);

// ... but V2 interprets slot 203 as s_flashLoanFee.

assertEq(v2.FEE_PRECISION(), 1e18);

assertEq(v2.getFee(), 1e18, "V2 fee now reads V1's precision slot");

assertEq(v2.getFee(), _load(SLOT_203), "V2 s_flashLoanFee == slot 203");

assertEq(v2.getFee() / 3e15, 333, "fee is 333x the intended 0.3%");

uint256 v2Fee = v2.getCalculatedFee(IERC20(address(tokenA)), LOAN);

assertEq(v2Fee, LOAN, "V2 fee on a 100e18 loan = 100e18 (100% of the borrowed value)");

// LP state survived (mapping at 202 did not move): token still allowed, same AssetToken.

assertTrue(v2.isAllowedToken(IERC20(address(tokenA))));

// Nothing on the upgrade path resets the fee: initialize cannot run again.

vm.prank(owner);

vm.expectRevert(bytes("Initializable: contract is already initialized"));

v2.initialize(address(poolFactory));

// Limb 4 (informational): getFeePrecision() no longer exists on the proxy.

(bool ok,) = proxy.staticcall(abi.encodeWithSignature("getFeePrecision()"));

assertFalse(ok, "getFeePrecision() should be gone in V2");

// Limb 3: old 3e15 is orphaned at slot 204, which V2 uses only as the flags mapping base.

assertEq(_load(SLOT_204), 3e15, "old fee value orphaned in V2's mapping base slot");

assertFalse(v2.isCurrentlyFlashLoaning(IERC20(address(tokenA))), "flags read false at rest");

console.log("V2 getFee() :", v2.getFee());

console.log("V2 getCalculatedFee(100e18) :", v2Fee);

}

/// Limb 1: a borrower that budgets the normal 0.3% fee succeeds on V1 and reverts with

/// NotPaidBack on V2 for the identical loan.

function test_flashLoanThatWorksOnV1RevertsOnV2() public {

IERC20 token = IERC20(address(tokenA));

AssetToken asset = v1.getAssetFromToken(token);

// ---- Baseline on V1 ----

BufferedReceiver r1 = new BufferedReceiver(proxy);

tokenA.mint(address(r1), FEE_BUFFER);

v1.flashloan(address(r1), token, LOAN, "");

assertEq(r1.lastFeeQuoted(), 3e17, "V1 quoted 0.3e18");

assertEq(tokenA.balanceOf(address(r1)), FEE_BUFFER - 3e17, "V1 borrower paid 0.3e18");

// ---- Upgrade ----

_upgradeAsOwner();

// ---- Same loan, same budget, on V2 ----

BufferedReceiver r2 = new BufferedReceiver(proxy);

tokenA.mint(address(r2), FEE_BUFFER);

uint256 startingBalance = tokenA.balanceOf(address(asset));

uint256 v2Fee = LOAN; // 100% of a 1:1-priced token

// Receiver repays all it holds (LOAN + FEE_BUFFER) but the pool now demands LOAN + 100e18.

vm.expectRevert(

abi.encodeWithSelector(

ThunderLoanUpgraded.ThunderLoan__NotPaidBack.selector,

startingBalance + v2Fee,

startingBalance + FEE_BUFFER

)

);

ThunderLoanUpgraded(proxy).flashloan(address(r2), token, LOAN, "");

console.log("V2 required ending balance :", startingBalance + v2Fee);

console.log("V2 receiver could return :", startingBalance + FEE_BUFFER);

}

/// Limb 2: a receiver that repays the quoted `fee` from a held buffer pays 100e18 instead of

/// 0.3e18 for the same 100e18 loan.

function test_borrowerOverpaysQuotedFeeAfterUpgrade() public {

IERC20 token = IERC20(address(tokenA));

// V1 cost for a 100e18 loan

BufferedReceiver r1 = new BufferedReceiver(proxy);

tokenA.mint(address(r1), 200e18);

v1.flashloan(address(r1), token, LOAN, "");

uint256 v1Cost = 200e18 - tokenA.balanceOf(address(r1));

assertEq(v1Cost, 3e17, "V1 cost = 0.3e18");

_upgradeAsOwner();

// V2 cost for the same loan

BufferedReceiver r2 = new BufferedReceiver(proxy);

tokenA.mint(address(r2), 200e18);

ThunderLoanUpgraded(proxy).flashloan(address(r2), token, LOAN, "");

uint256 v2Cost = 200e18 - tokenA.balanceOf(address(r2));

assertEq(v2Cost, 100e18, "V2 cost = 100e18 (100% of the loan)");

assertEq(v2Cost / v1Cost, 333, "borrower pays 333x the intended fee");

console.log("V1 cost of 100e18 loan :", v1Cost);

console.log("V2 cost of 100e18 loan :", v2Cost);

}

/// Context (mitigation, not a refutation): the owner can repair the fee after the fact.

function test_ownerCanRestoreFeeAfterTheFact() public {

ThunderLoanUpgraded v2 = _upgradeAsOwner();

assertEq(v2.getFee(), 1e18);

vm.prank(owner);

v2.updateFlashLoanFee(3e15);

assertEq(v2.getFee(), 3e15);

assertEq(v2.getCalculatedFee(IERC20(address(tokenA)), LOAN), 3e17);

}

Recommended Mitigation

--- a/src/upgradedProtocol/ThunderLoanUpgraded.sol

+++ b/src/upgradedProtocol/ThunderLoanUpgraded.sol

@@ -93,7 +93,8 @@

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_blank; // former s_feePrecision slot; keep it so the V1 storage layout is preserved

uint256 private s_flashLoanFee; // 0.3% ETH fee

uint256 public constant FEE_PRECISION = 1e18;

mapping(IERC20 token => bool currentlyFlashLoaning) private s_currentlyFlashLoaning;

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 1 hour 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

Rewards breakdown

This contest has a flat reward structure with the following payouts:

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being reviewed by our AI judge. Results will be available in a few minutes.

View all submissions

Rewards Distribution

The contest is complete and the rewards are being distributed.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!