Summary

When you are upgrading contracts using a proxy, you cannot swap the order of the storage variables around in the initial contract compared to the upgraded contract. Proxy contracts, like the one used in thunder loan use delegatecall to forward calls to the appropriate function in the logic contracts, and delegatecall uses the position of variables in storage to find them rather than their names, which can lead to the overwrite of variables if you change their order from one logic contract to the next.

You can add new storage variables at the end of the list but you can't move the existing storage variables around. The ThunderLoan.sol contract lists s_feePrecision followed by s_flashLoanFee whereas the ThunderLoanUpgraded.sol contract lists s_flashLoanFee followed by FEE_PRECISION.

Vulnerability Details

In ThunderLoan.sol, these storage variables are in this order:

In ThunderLoanUpgraded.sol, the storage variables are in this order:

Impact

After upgrading to ThunderLoanUpgraded, the proxy contract will expect to find the fee precision variable at the storage slot it was written to in ThunderLoan but it will find the flash loan fee variable and vice versa. When using ThunderLoanUpgraded, you will have the wrong values for flash loan fee and fee precision. Also, since fee precision is 1e18 you will be charging a massive fee to anyone trying to get a flash loan (from .3% to 100%), so you have basically bricked your flash loan function since people won't use a flash loan protocol that charges such a huge fee.

Tools Used

Manual review

Recommendations

Put the storage variables in the same order in ThunderLoanUpgraded as they were in ThunderLoan