First Flight #3: Thunder Loan

Summary

ThunderLoan contract, just like ThunderLoanUpgraded, inherits many other contracts :

This means ThunderLoan and ThunderLoanUpgraded share the same storage organisation relating to all inherited contracts. They both have state variables of inherited contracts, including useful variables and placeholders.

Respect of storage layout is of paramount importance in upgradeable contracts.
The problem comes from ThunderLoan and ThunderLoanUpgraded themselves, because they don't declare the same storage variables.

Vulnerability Details

Besides storage variables inherited from other contracts, ThunderLoan declares in its storage the following variables :

whereas ThunderLoanUpgraded contract declares in its storage the following variables:

Indeed, s_flashLoanFee variable declaration of ThunderLoan contract has been replaced by uint256 public constant FEE_PRECISION = 1e18; declaration in ThunderLoanUpgraded contract. This means FEE_PRECISION is not stored in storage, but directly in the runtime bytecode of the contract. This will save gas for deployment and usage of ThunderLoanUpgraded contract but this will break the storage of the proxy because of storage collisions.

This means that initialize function of ThunderLoanUpgraded contract, when executing s_flashLoanFee = 3e15, will set to the value 3e15 the slot in the proxy that previously contained s_feePrecision value.

Also, currentlyFlashLoaning mapping in the ThunderLoanUpgraded contract will enter in collision with s_flashLoanFee value stored in the proxy. Storage slot value of a mapping declaration should always be 0, and this won't be the case in this situation.

Impact

The impact of this vulnerability is MEDIUM as it creates storage collisions but without serious consequences. Storage collisions should always be avoided by a correct implementation of upgradeable contracts.
In this specific case, the storage slot of the proxy that contained s_feePrecision value is replaced by the value of s_flashLoanFee. This won't cause any issue, since any sload instruction that will retrieve s_flashLoanFee in the upgraded implementation will return the correct value, since s_feePrecision has been overwritten.

currentlyFlashLoaning mapping, which storage slot value in the proxy at the index of declaration is 0 in the non upgraded context, will now have the value of s_flashLoanFee after deploying the upgraded contract. This is because currentlyFlashLoaning is now the 2nd storage variable declared, instead of the 3rd one (ignoring all state variables inherited, which are necessarily the same in both non upgraded and upgraded contracts).

But the most important thing here is that the storage slot of the mapping has been modified.
Since location of elements stored in mappings is :

All previous elements stored in the mapping wouldn't be accessible anymore, as mappingSlot is now different (minus one the previous slot).

In our case, this is not as disastrous as it could be, since currentlyFlashLoaning only stores boolean values for the duration of the flashloan. This means outside any transaction ordering a flashloan, all values in this mapping will be 0, as expected.

Tools Used

Manual

Recommendations

I suggest to keep the same storage declaration in both ThunderLoan and ThunderLoanUpgraded implementation contracts. This way, the proxy that delegatecalls to these contracts won't have any collision when upgrading from ThunderLoan to ThunderLoanUpgraded.