# Damn Vulnerable DeFi: Unstoppable By [blokboy](https://paragraph.com/@blokboy) · 2021-10-05 --- _Warning: Solution to the challenge will be discussed here. Also note, I could be totally fucking wrong about everything I’m about to write. This is mostly just me long form “tweeting into the ether” about my smart contract journey. Proceed with caution._ Problem ------- > There’s a lending pool with a million DVT tokens in balance, offering flash loans for free. > > If only there was a way to attack and stop the pool from offering flash loans … > > You start with 100 DVT tokens in balance. So from jump, this particular problem is pretty clear that we don’t need to necessarily try to drain the contract or anything but just stop it from offering flash loans. I’m not really sure how that’s done without bankrupting the contract, so let’s take a look at the code… ![An image of the ReceiverUnstoppable smart contract](https://storage.googleapis.com/papyrus_images/27c3ffe5b3c474e4385a9a8a6f72ebe0ec0101f8e1659440dbaef94c2e7450cb.png) An image of the ReceiverUnstoppable smart contract Checking out the Receiver contract for the flash loan and I’m not noticing anything too wonky here… ### constructor Here we just set the `owner` of the contract to match the address that deployed the contract initially as well as set the `pool` (of type `UnstoppableLender`) to point to the contract we’re hoping to freeze. Nothing special to see here (or is there?). ### receiveTokens This is an external function meant to be called by `UnstoppableLender` in order to move tokens to the `ReceiverUnstoppable` , specifying two arguments the `tokenAddress` which is basically the address of the contract holding the state for a particular ERC20 that we are trying to transfer to `Receiver` , and an `amount` that details exactly how many tokens we should be sending. From the first line of this function, we see that in order for this function to not revert, the address that called this particular function must have come from the `pool` variable we set up in the constructor. > Sidenote: the `transfer` method is a method that looks like this for any ERC20, where we can move value from the contract to whichever address we specify `_to` with an appropriate `_value` and returns a `bool` indicating if the call was successful or not. > > `function transfer(address _to, uint256 _value) public returns (bool success)` After we satisfy the first condition, we move onto the second requirement that casts the ERC20 standard over the provided `tokenAddress` in the arguments (to ensure we have access to that `transfer` f(x) you see up there). So this seems to be pretty on par with a flash loan recipients’ obligations to their lender. As long as we’re getting tokens from the DVT pool and then sending them back at the end all of the state changes we attempt with this flash loan should be accepted. ### executeFlashLoan So this looks like a f(x) that’s meant to be called by the `owner` of the `ReceiverUnstoppable` contract that was set in the constructor and it only expects an `amount` to request in the form of a flash loan. The final line of this function appears to call on the `pool` object that’s pointing at the DVT contract to make a request for some ⚡️ loans. ![An image of the UnstoppableLender smart contract](https://storage.googleapis.com/papyrus_images/c02c923aaaa2e8ffd59cea49291302620135ce0c3c6965e7135278566c29ee8c.png) An image of the UnstoppableLender smart contract I’m seeing a lot of OpenZeppelin standards at the top which gives me very little faith that I’ll be draining this contract completely. I’m pretty new to this but typically seeing these libraries means I’m not gonna be able to rely on [reentrancy](https://medium.com/coinmonks/protect-your-solidity-smart-contracts-from-reentrancy-attacks-9972c3af7c21) or [arithmetic bs](https://medium.com/coinmonks/math-in-solidity-part-2-overflow-3cd7283714b4) here. There doesn’t seem to be anything about ownership of contract though… 🧐 I’m also noticing that this contract seems to have an interface for the `receiveTokens` f(x) that’s written out in our other contract of concern for this challenge. ### constructor So a few things, we’re using SafeMath to manage arithmetic and try to stop overflows, so it’s not likely that we’ll be able to fuck off `poolBalance` which is of type uint256, and then we have public access to the DamnValuableToken ERC20 implementation as well as the poolBalance that seems pretty well guarded although visible. Here, we just want to provide a token address that should point to the location of the DamnValuableToken ERC20 implementation and make sure it’s not pointing to the null address `address(0)` at which point we point our DamnValuableToken at the `tokenAddress` provided by `msg.sender` when they deployed the function. ### depositTokens Something about the use of a wrapper deposit function instead of using native functions within ERC20 standard feels intuitively off to me, but I’m also pretty ignorant as to what current best practices are for most shit like this right now. But this particular wrapper is expecting an `amount` to be provided by the address that calls this function and we are doing a check immediately to make sure that `msg.sender` isn’t calling this with zero tokens to deposit. > Sidenote: the `transferFrom` method is again part of the ERC20 standard, that differentiates from the previously inspected `transfer` only in that we are not explicitly deciding which address the value should be sourced from. Notice that in this particular call we need to specify this because we are transferring value between users _within_ the DamnValuableToken contract, so we need to know both parties in advance and cannot implicitly derive this from `msg.sender` . F(x) returns bool on success but should throw unless the `_from` account has deliberately authorized the sender of the message via some mechanism > > `function transferFrom(address _from, address _to, uint256 _value) public returns (bool success)` Then the `poolBalance` should be incremented to reflect the updated balance after you call this function as the last step. This is pretty weird because I don’t see any clear fallback function or anything that will manage incrementing `poolBalance` if we decide to send/transfer funds to `UnstoppableLender` through ERC20 methods rather than using this `depositTokens` wrapper. That seems weird but maybe this is a normal thing. ![An image of the flashLoan function within the Unstoppable Lender smart contract](https://storage.googleapis.com/papyrus_images/f3aec51497c7ffd0bea3858f91e96d0af0571c50283e266dfd6446c929805119.png) An image of the flashLoan function within the Unstoppable Lender smart contract ### flashLoan This seems to be the last stop on our journey and I’m seeing a lot of small clues being confirmed about `flashLoan` from up until this point. We could’ve guessed it was `external` or `public` since we were calling it from within our `ReceiverUnstoppable` contract, and I assumed it would be guarded against reentrancy which turns out to be a good assumption. And it looks like this function is taking in an `amount` argument which we could infer from the fact that we supplied an argument by this name from within `ReceiverUnstoppable` when we make the call to `executeFlashLoan` This f(x) is giving me an odd tingly sensation in my brain as I’m scanning it, I’m pretty certain it’s just me feeling excitement that the error must be in here through some sort of process of elimination. So we have a quick check to make sure that we aren’t trying to request a loan for zilch (although I wonder what kinda attack surface that enables), and then we get a read on how many DVT tokens our _current_ contract holds by checking its state within DVT. > Sidenote: `balanceOf` is another method that can be found as part of the ERC20 standard and it’s a pretty straightforward public view function that lets us specify an address that we would like to see the quantity of tokens within the contract allocated to them > > `function balanceOf(address _owner) public view returns (uint256 balance)` So boom, we check out balance by passing in `address(this)` and make sure that the `UnstoppableLender` actually has enough tokens to fulfill their part of the obligation for the ⚡️ loan…. and **the catch is on line 36.** We are asserting that `poolBalance` should match the reported state from `balanceBefore` — it’s really important to note here that balanceBefore is reading state directly from DVT contract while `poolBalance` is attempting to map directly to that `balanceBefore` variable. I think there are two ways I can stop the function given this line of code: 1. Use `transfer` or `send` methods to send one of our 10 DVT tokens to either the DVT contract specifying that we should increase the balance for `UnstoppableLender`’s address and so `balanceBefore` shouldn’t match with `poolBalance` after we’re done. 2. Same thing as (1) but sending DVT to `UnstoppableLender` contract directly (without using the `depositTokens` so that `poolBalance` doesn’t get incremented by value) and in this case `poolBalance` should be ahead of `balanceBefore` In either of these two cases, our assert should fail and flash loans should no longer be possible. Going forward, it looks like if the assertion clears then we are going to hit the DVT contract and ask it to transfer over tokens to `msg.sender` ’s address for the total borrow amount. Once that transfer is made and approved, we then call the `receiveTokens` interface with the `msg.sender` as the target to receive the tokens which are returned within the same call. Once we make exit the `IReceiver` call then we should have received back full payment for the loan which is verified by the last two lines of the the smart contract that are checking the balance after the flash loan has been provided. If the balance hasn’t been paid back all of the changes associated with these function calls would be reverted and the gas from sender would be lost in the ether :( Solution -------- ![](https://storage.googleapis.com/papyrus_images/217081ab4a88d249c8e8467940509b29d635ef58666038a8138e9e6580f978d7.png) The exploit method snippet depicted So this was all we had to do to solve this one. Just shoot a token to the pool. ### Reflection This was pretty fun for a first challenge. One thing that struck a personal cord for me with this one is to really reason about your safety checks and what exactly you’re trying to protect especially within functions that are making calls to potentially multiple other contracts/functions. It was kind of daunting just sitting on line 36 for a few minutes to really think about why my brain started freaking out a bit when it saw the line. I can think of a few workarounds for this one that might work, in order of my level of faith in the revision being functional: 1. We change the `poolBalance` value to instead reflect `address(this).balance` , so we are always operating with the current total balance in our `UnstoppableLender` and make sure that it matches with `balanceBefore` if we absolutely \*must \*check to be sure that separate states are matching 2. I probably wouldn’t even bother with trying to match a separate state within `UnstoppableLender` — as far as I can tell, we could’ve just checked to be sure `balanceBefore` wasn’t `0` and proceeded from there, but I could also be missing something in my analysis as to why we _need_ to have the `poolBalance` mapping to DVT balance 3. Write a fallback function that updates `poolBalance` anytime money is deposited into the contract. Something in my gut tells me this is bad, but I’ll have to experiment later ### Conclusion Thanks for sifting through this if you made it to the end. I don’t normally write out stuff like this, but I’m trying a new thing; and I would love to hear more feedback on how I can make this sort of shit more accessible and useful to others or just general improvements/critiques/comments/whatever. \\n \\n This is literally just a brain dump as I’m trying to learn more about this stuff and I really really appreciate any feedback or insights into what I got wrong or what I should revisit my mental model for! --- *Originally published on [blokboy](https://paragraph.com/@blokboy/damn-vulnerable-defi-unstoppable)*