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

Upgrading proxy to bad implementation contract breaks storage

wafflemakr

Summary

Upgrading the proxy to new implementation ThunderLoanUpgraded breaks the storage, as the new ThunderLoanUpgraded::s_flashLoanFee variable will point to ThunderLoan::s_feePrecision initial slot

Vulnerability Details

ThunderLoan is the current implementation of the proxy contract. It defines two variables in storage as follows:

uint256 private s_feePrecision;
uint256 private s_flashLoanFee; // 0.3% ETH fee

However, the new implementation ThunderLoanUpgraded has an issue, as the s_flashLoanFee is now pointing to the slot of s_feePrecision

uint256 private s_flashLoanFee; // 0.3% ETH fee
uint256 public constant FEE_PRECISION = 1e18;

Impact

High. When the contract is upgraded, s_flashLoanFee now read the value stored in that slot, which was initialized as 1e18 in the first implementation. This means that flash loan fee will be 100% fee in ETH (s_flashLoanFee/FEE_PRECISION), and not 0.3% ETH fee as intended. This is basically a DoS to users that want to keep making flash loans.

Proof of Concept

function testUpgradeBreakStorage() public {
// Before
ThunderLoanUpgraded newImpl = new ThunderLoanUpgraded();
uint256 feeBefore = thunderLoan.getFee();
uint256 feePrecisionBefore = thunderLoan.getFeePrecision();
​
// When
thunderLoan.upgradeTo(address(newImpl));
ThunderLoanUpgraded thunderLoanUpgraded = ThunderLoanUpgraded(address(proxy));
​
// Checks
// flashloan fee changes after upgrade
assertNotEq(thunderLoanUpgraded.getFee(), feeBefore);
// flashloan fee now points to feePrecision storage slot
assertEq(thunderLoanUpgraded.getFee(), feePrecisionBefore);
​
uint256 depositAmount = 1 ether;
assertEq(thunderLoanUpgraded.getCalculatedFee(IERC20(weth), depositAmount), depositAmount);
}

Running the test will output:

forge test --mt testUpgradeBreakStorage -vvv
[⠒] Compiling...
No files changed, compilation skipped
​
Running 1 test for test/unit/AuditTest.t.sol:AuditTest
[PASS] testUpgradeBreakStorage() (gas: 3085204)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.28ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

  • Foundry

Recommendations

If the idea is to leave the precision as constant, add a storage variable to occupy the slot of s_feePrecision so storage reads are not affected. Ideally, keep the previous storage layout intact.

+ uint256 private leaveOneSlote;
uint256 private s_flashLoanFee; // 0.3% ETH fee
uint256 public constant FEE_PRECISION = 1e18;
Updates

Lead Judging Commences

0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

storage collision on upgrade

Support

FAQs

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