Thunder Loan
AI First Flight #7
Beginner FriendlyFoundryDeFiOracle
EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: high
Invalid

Storage collision between ThunderLoan and ThunderLoanUpgraded inflates the flash loan fee from 0.3% to 100% after upgrade

wittytiger689

Root + Impact

Description

The upgrade target reorders storage. ThunderLoan declares s_feePrecision before s_flashLoanFee, while ThunderLoanUpgraded removes s_feePrecision and replaces it with a constant. Constants are embedded in bytecode and occupy no storage slot, so every variable declared after it shifts up by one slot.

// src/protocol/ThunderLoan.sol
mapping(IERC20 => AssetToken) public s_tokenToAssetToken; // slot N+0
@> uint256 private s_feePrecision; // slot N+1
uint256 private s_flashLoanFee; // slot N+2
mapping(IERC20 token => bool) private s_currentlyFlashLoaning; // slot N+3
// src/upgradedProtocol/ThunderLoanUpgraded.sol
mapping(IERC20 => AssetToken) public s_tokenToAssetToken; // slot N+0
@> uint256 private s_flashLoanFee; // slot N+1 - reads old s_feePrecision
@> uint256 public constant FEE_PRECISION = 1e18; // constants occupy NO slot
mapping(IERC20 token => bool) private s_currentlyFlashLoaning; // slot N+2 - reads old fee slot

A proxy keeps its storage across an upgrade while the implementation's variable-to-slot mapping is recompiled from the new source. After the upgrade, s_flashLoanFee therefore reads the slot that used to hold s_feePrecision (1e18), and s_currentlyFlashLoaning reads the slot that used to hold the fee.

Risk

Likelihood: High

This is not an edge case but the intended deployment path. The protocol documentation states: "We are planning to upgrade from the current ThunderLoan contract to the ThunderLoanUpgraded contract." Performing that documented upgrade triggers the bug with certainty.

Impact: High

The flash loan fee becomes 100% of the borrowed value the instant the upgrade executes, because s_flashLoanFee and FEE_PRECISION both equal 1e18, so fee = value * 1e18 / 1e18 = value. Every flash loan then demands repaying double the borrowed amount, breaking the protocol's core function for all users.

On a proxy this is unrecoverable without a further upgrade, and the corrupted s_currentlyFlashLoaning slot additionally breaks repay().

Proof of Concept

Deploy behind an ERC1967Proxy, record the fee, perform the documented upgrade, read the fee again.

function test_Finding3_UpgradeCorruptsFlashLoanFee() public {
uint256 feeBefore = thunderLoan.getFee(); // 3e15 (0.3%)
uint256 precisionBefore = thunderLoan.getFeePrecision(); // 1e18
​
ThunderLoanUpgraded upgraded = new ThunderLoanUpgraded();
thunderLoan.upgradeToAndCall(address(upgraded), "");
​
uint256 feeAfter = ThunderLoanUpgraded(address(proxy)).getFee();
​
assertEq(feeBefore, 3e15);
assertEq(precisionBefore, 1e18);
assertEq(feeAfter, precisionBefore); // the fee now reads the OLD feePrecision slot
}
​
function test_Finding3_FeeBecomesOneHundredPercent() public {
ThunderLoanUpgraded upgraded = new ThunderLoanUpgraded();
thunderLoan.upgradeToAndCall(address(upgraded), "");
​
ThunderLoanUpgraded tl = ThunderLoanUpgraded(address(proxy));
tl.setAllowedToken(IERC20(address(tokenA)), true);
​
uint256 borrow = 10e18;
uint256 fee = tl.getCalculatedFee(IERC20(address(tokenA)), borrow);
​
assertEq(fee, borrow); // the fee equals 100% of the borrowed amount
}

Both tests pass. Result:

BEFORE upgrade - s_flashLoanFee : 3000000000000000 (3e15 = 0.3%)
BEFORE upgrade - s_feePrecision : 1000000000000000000 (1e18)
AFTER upgrade - s_flashLoanFee : 1000000000000000000 (the old precision value)
fee inflated by factor: 333
​
borrow amount: 10000000000000000000 (10 tokens)
fee charged : 10000000000000000000 (10 tokens = 100%)

Recommended Mitigation

Never remove or reorder existing storage variables in an upgradeable contract. The proxy's storage is fixed at the layout of the first implementation, so a later implementation must preserve that layout exactly: variables may only be appended at the end, and any variable that is no longer used must keep its slot occupied by a placeholder rather than being deleted.

Replacing a storage variable with a constant is the specific trap here, because a constant looks like a declaration in the same position but consumes no slot, silently shifting everything below it.

mapping(IERC20 => AssetToken) public s_tokenToAssetToken;
- uint256 public constant FEE_PRECISION = 1e18;
+ uint256 private __deprecated_feePrecision; // keep the slot occupied
uint256 private s_flashLoanFee;
mapping(IERC20 token => bool) private s_currentlyFlashLoaning;

Declare FEE_PRECISION as a constant elsewhere in the contract body (after all storage variables), and add an automated storage-layout check to CI, such as the OpenZeppelin Upgrades plugin, so a layout-incompatible upgrade cannot be deployed.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 1 hour ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!