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

When using a proxy pattern and changing the implementation contract, existing storage variables in both contracts have to reside in same storage slots

zxarcs

Summary

State variables are not in the same storage memory slots.
After contract upgrade, both storage memory at index one and at index two will be overwritten with bad data.

Details

In part, storage memory slots for the current ThunderLoan.sol contract looks like this:

slot n: token-to-asset-token mapping
slot n+1: fee precision
slot n+2: flash loan fee
// other state variables

In part, storage memory slots for the planned ThunderLoanUpgraded.sol contract looks like this:

slot n: token-to-asset-token mapping
slot n+1: flash loan fee // this will overwrite the fee precision above
// fee precision not a storage memory variable any more since it is marked as constant
slot n+2: is token currently flash loaning mapping
// other state variables

When the proxy contract uses delegatecall low level function, it will execute logic from one of these implementation contracts and update the storage memory of the proxy contract itself.

This means that (before the upgrade) when flash loan fee is updated (at some point), the proxy's storage slot at that specific location will be updated.

After the upgrade, fee precision is changed to a constant meaning that it will not be part of the contract storage memory any more as it was in previous contract version. That storage slot will now instead be associated flash loan fee and (once first flash loan executes) flash loan fee with data related to s_currentlyFlashLoaning mapping which will be 0x0...0 since the initial storage slot for mappings is 0x0...0 (with mapping data stored elsewhere).

This can introduce many unexpected issues because wrong state variables are being updated.

Filename

src/upgradedProtocol/ThunderLoanUpgraded.sol

Permalinks

https://github.com/Cyfrin/2023-11-Thunder-Loan/blob/8539c83865eb0d6149e4d70f37a35d9e72ac7404/src/upgradedProtocol/ThunderLoanUpgraded.sol#L96-L97

Impact

After protocol update to ThunderLoanUpgraded.sol contract, multiple state variables will be overwritten leading to many issues, know and unknown.

Recommendations

Do not change fee precision to a constant. This will introduce many issues, as explained above.
In addition, do not change order of state variables in ThunderLoanUpgraded.sol.
Keep state variables in same storage memory slots in ThunderLoanUpgraded.sol as they are in ThunderLoan.sol

Note: Additional changes will need to be made to FEE_PRECISION. Recommended to keep the same implementation with s_feePrecision as in previous version of the contract in ThunderLoan.sol.

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

POC

src/upgradedProtocol/ThunderLoanUpgraded.sol

Change is needed in ThunderLoanUpgraded.sol so that the initialize function can run. See H-4

function initialize(address tswapAddress) external reinitializer(2) {
__Ownable_init();
__UUPSUpgradeable_init();
__Oracle_init(tswapAddress);
s_flashLoanFee = 3e15; // 0.3% ETH fee
}

test/unit/ThunderLoanTest.t.sol

import { ThunderLoanUpgraded } from "src/upgradedProtocol/ThunderLoanUpgraded.sol";
//...
​
function testUpgradeSetsWrongFeePrecisionAndFee() public {
uint256 feePrecisionBeforeUpgrade;
uint256 feePrecisionAfterUpgrade;
​
console.log("address(thunderLoan) slots");
for (uint256 i = 0; i < 250; i++) {
bytes32 slot = vm.load(address(proxy), bytes32(uint256(i)));
//console.logBytes32(slot);
uint256 slotUint = uint256(slot);
​
if (i == 203) {
feePrecisionBeforeUpgrade = slotUint;
}
​
if (slotUint > 0) {
console.log("index %s value %s", i, slotUint);
}
}
// output:
// index 0 value 1
// index 51 value 728815563385977040452943777879061427756277306518
// index 201 value 263400868551549723330807389252719309078400616203
// index 203 value 1000000000000000000 <- s_feePrecision
​
// deploy the new version and upgrade proxy
ThunderLoanUpgraded thunderLoanV2Implementation = new ThunderLoanUpgraded();
thunderLoan.upgradeToAndCall(
address(thunderLoanV2Implementation), abi.encodeWithSignature("initialize(address)", address(mockPoolFactory))
);
​
console.log("address(thunderLoanV2Implementation) slots");
for (uint256 i = 0; i < 250; i++) {
bytes32 slot = vm.load(address(proxy), bytes32(uint256(i)));
//console.logBytes32(slot);
uint256 slotUint = uint256(slot);
​
if (i == 203) {
feePrecisionAfterUpgrade = slotUint;
}
​
if (slotUint > 0) {
console.log("index %s value %s", i, slotUint);
}
}
// output:
// index 0 value 1
// index 51 value 728815563385977040452943777879061427756277306518
// index 201 value 917977473271046311748067258655037622845262362008
// index 203 value 3000000000000000 <- s_feePrecision (overwritten by s_flashLoanFee)
​
assertNotEq(feePrecisionBeforeUpgrade, feePrecisionAfterUpgrade);
}
​

Note: Not clear if when this POC is ran, storage variables will be at exact same index like they were for me. The point will still stand though because which ever storage index they are in the fee precision will be overwritten when the update happens.

Tools Used

  • Manual Audit

  • Foundry

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.