_Follow along with the video lesson:_ --- ### Exploit - Storage Collision - Write Up ::image{src='/security-section-6/47-exploit-storage-collision-write-up/exploit-storage-collision-write-up1.png' style='width: 100%; height: auto;'} Picking up where we left off, let's dive right into the write up for this `storage collision` finding, together. Begin with our template, as we always do. I've included it below, but it's always available on the [**course repo**](https://github.com/Cyfrin/sc-exploits-minimized)! ``` ### [S-#] TITLE (Root Cause -> Impact) **Description:** **Impact:** **Proof of Concept:** **Recommended Mitigation:** ``` Add our template to our findings.md and we'll start with the title! ### Title What's the severity of our storage collision vulnerability? - **Impact:** `High` - Fees post upgrade will be drastically different resulting in unexpected calculations. Storage collision is really bad. - **Likelihood:** `Medium/Low` - If the upgraded contract is implemented as it is, this error will be applied to all fee calculations. The issue is contingent on the upgrade being used as-is. There may be argument for this to be a medium, but given the impact in `ThunderLoan`, I'm going to stick with my assessment of `high severity`. Our title is actually going to identify an impact we didn't even go over together, but this storage collision actually affects more than the fee variable. We have to remember that changing how many storage slots are used may impact any assignment that comes later in the 'array'! In the case of ThunderLoan and significant affect of this is that the `s_currentlyFlashloaning` mapping is impacted, which freezes the protocol entirely. As such, our title might look something like ``` ### [H-3] Mixing up variable location causes storage collisions in ThunderLoan::s_flashLoanFee and ThunderLoan::s_currentlyFlashLoaning ``` ### Description and Impact Our description should clearly outline the problem and what's causing it. ```` **Description:** `ThunderLoan.sol` has two variables in the following order: ```javascript uint256 private s_feePrecision; uint256 private s_flashLoanFee; // 0.3% ETH fee ``` However, the expected upgraded contract `ThunderLoanUpgraded.sol` has them in a different order. ```javascript uint256 private s_flashLoanFee; // 0.3% ETH fee uint256 public constant FEE_PRECISION = 1e18; ``` Due to how Solidity storage works, after the upgrade, the `s_flashLoanFee` will have the value of `s_feePrecision`. You cannot adjust the positions of storage variables when working with upgradeable contracts. ```` And our impact should clearly illustrate the affect the vulnerability has or may have on the protocol. ``` **Impact:** After upgrade, the `s_flashLoanFee` will have the value of `s_feePrecision`. This means that users who take out flash loans right after an upgrade will be charged the wrong fee. Additionally the `s_currentlyFlashLoaning` mapping will start on the wrong storage slot. ``` We've already created our `proof of concept/proof of code` via the test we wrote for this. It's nice when finding the bug and reporting the bug overlap this way. ```` **Proof of Code:** <details> <summary>Code</summary> Add the following code to the `ThunderLoanTest.t.sol` file. ```javascript // You'll need to import `ThunderLoanUpgraded` as well import { ThunderLoanUpgraded } from "../../src/upgradedProtocol/ThunderLoanUpgraded.sol"; function testUpgradeBreaks() public { uint256 feeBeforeUpgrade = thunderLoan.getFee(); vm.startPrank(thunderLoan.owner()); ThunderLoanUpgraded upgraded = new ThunderLoanUpgraded(); thunderLoan.upgradeTo(address(upgraded)); uint256 feeAfterUpgrade = thunderLoan.getFee(); assert(feeBeforeUpgrade != feeAfterUpgrade); } ``` </details> You can also see the storage layout difference by running `forge inspect ThunderLoan storage` and `forge inspect ThunderLoanUpgraded storage` ```` And lastly, we want to provide a `recommended mitigation` for the issue. The root of this problem is kinda forked. Yes, the order of the variables is changing and this would impact referenced storage slots as we've discussed, but additionally, one of our storage variables is becoming a constant and being removed from storage entirely. This bumps everything in storage forward, causing our collision. We can mitigate this by reserving a blank storage slot to fill the place of `FEE_PRECISION` in our `ThunderLoanUpgraded` contract. **Recommended Mitigation:** Do not switch the positions of the storage variables on upgrade, and leave a blank if you're going to replace a storage variable with a constant. In `ThunderLoanUpgraded.sol`: ```diff - uint256 private s_flashLoanFee; // 0.3% ETH fee - uint256 public constant FEE_PRECISION = 1e18; + uint256 private s_blank; + uint256 private s_flashLoanFee; + uint256 public constant FEE_PRECISION = 1e18; ``` ### Wrap Up Whew! We just learnt a tonne about storage. We also learnt that proxies come with a degree of centralization that comes with it's own set of risks. These upgrades and the issues that come from them happen all too often. The risks associated with proxies often have them labelled as 'bad' or 'too risky', and they may very well be, the conversation is still on going. I encourage you to voice your opinion on it, on social media and jump into the conversation. See you in the next lesson!