Beginner FriendlyFoundryDeFiOracle
100 EXP
View results
Submission Details
Severity: high
Valid

Storage collisions between different versions of the ThunderLoan contract occur

Summary

When writing new versions of your contracts, either due to new features or bug fixing, there is an additional restriction to observe: you cannot change the order in which the contract state variables are declared, nor their type. Violating any of these storage layout restrictions will cause the upgraded version of the contract to have its storage values mixed up, and can lead to critical errors in your application.

Vulnerability Details

For example, in the upgraded contract, the s_flashLoanFee store is the same slot for s_feePrecision in the in the contract upgraded before.

/*//////////////////////////////////////////////////////////////
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_feePrecision;
uint256 private s_flashLoanFee; // 0.3% ETH fee
/*//////////////////////////////////////////////////////////////
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

Impact

The upgraded version of the contract to have its storage values mixed up, and can lead to critical errors in application.

POC

we show the value of s_flashLoanFee changed after upgraded.

// 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 {ThunderLoanUpgraded} from "../../src/upgradedProtocol/ThunderLoanUpgraded.sol";
contract ThunderLoanTestUpgraded is BaseTest {
uint256 constant AMOUNT = 10e18;
uint256 constant DEPOSIT_AMOUNT = AMOUNT * 100;
address liquidityProvider = address(123);
address liquidityProvider2 = address(124);
address user = address(456);
MockFlashLoanReceiver mockFlashLoanReceiver;
function setUp() public override {
super.setUp();
vm.prank(user);
mockFlashLoanReceiver = new MockFlashLoanReceiver(address(thunderLoan));
}
function testUpgradeError() public{
uint256 feeUpgradedBefore = thunderLoan.getFee();
console.log(feeUpgradedBefore);
//upgraded
ThunderLoanUpgraded thunderLoanV2 = new ThunderLoanUpgraded();
thunderLoan.upgradeTo(address(thunderLoanV2));
uint256 feeUpgradedAfter = thunderLoan.getFee();
console.log(feeUpgradedAfter);
assertEq(feeUpgradedBefore,feeUpgradedAfter);
}
}

we run

forge test --match-contract ThunderLoanTestUpgraded -vv

we get

[FAIL. Reason: Assertion failed.] testUpgradeError() (gas: 3083993)
Logs:
3000000000000000
1000000000000000000
Error: a == b not satisfied [uint]
Left: 3000000000000000
Right: 1000000000000000000
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.82ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Tools Used

forge

Recommendations

Do not change the order in which the contract state variables are declared, nor their type when write upgraded version contract.

Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year 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.