# Damn Vulnerable DeFi: Naive Receiver 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 offering quite expensive flash loans of Ether, which has 1000 ETH in balance._ > > _You also see that a user has deployed a contract with 10 ETH in balance, capable of interacting with the lending pool and receiveing flash loans of ETH._ > > _Drain all ETH funds from the user’s contract. Doing it in a single transaction is a big plus ;)_ Okay, so this problem isn’t even asking us to drain the cool expensive flash loan with 1k ETH; it just wants us to see if we can drain a separate contract, attached to an EOA that’s interacting with the flash loan contract. > NaiveReceiverLenderPool.sol ![An image of the NaiveReceiverLenderPool smart contract](https://storage.googleapis.com/papyrus_images/3e327f1413d2230628ce85f606df5c8262d5f7c3690c5f35b21dfa83a99adbf7.png) An image of the NaiveReceiverLenderPool smart contract > Sidenote: The first line of code is omitted here but it specifies the Solidity pragma as version \*`^0.6.0` Right off the bat, we notice a few libraries brought in to handle issues like Reentrancy, arithmetic under/over flows, and a library for dealing with different address sources in Ethereum (contracts and EOAs). Cool. As we read down the contract, I notice something that kind of gives me an immediate pause because who wants to pay a fee of 1ETH to take out a loan? That’s a pretty wild fee. Not only that but the variable for the fee is defined as both `private` and `constant` , meaning not only will this fee not be (easily) queryable and the value also cannot be changed after it is deployed. _Note:_ `constant` and `immutable` keywords both specify a type of state variable on the Ethereum blockchain. `constant` keywords can’t be changed and must be set at creation while `immutable` can be changed within the constructor at deployment. Also, EVM doesn’t reserve storage for variables that are given these flags and instead just puts their value in place where the variable is called. (Not really sure what this really means yet will update later). ### fixedFee This is pretty straightforward, we have an external function that doesn’t do anything but read and return the `FIXED_FEE` `constant`variable that was defined earlier in the contract. Not much to gauge from this. ### flashLoan This shit is nonReentrant. Okay so any hopes I had initially of calling flashLoan multiple times to bankrupt the contract might be for naught. This f(x) is going to be called externally and it’s expecting just two arguments that it’ll process — namely, a `payable address` and a `uint256` known respectively as `borrower` and the `borrowAmount` and it does a couple of important checks right out the gate with these two pieces of info. First, we see that the f(x) is checking the current balance of the pool and then verifying that the amount being requested as the `borrowAmount` isn’t more than what’s actually available within the contract. Next, the f(x) is going to do a check on the `borrower` that was provided to ensure that this function is being called by a contract. > Sidenote: The `isContract`_method relies on_ `extcodesize`, which returns 0 for contracts in construction, since the code is only stored at the end of the constructor execution. `EXTCODESIZE` returns 0 if it is called from the constructor of a contract. So if you are using this in a security sensitive setting, you would have to consider if this is a problem. \*\*\*_It is unsafe to assume that an address for which this function returns false is an externally-owned account (EOA) and not a contract. Among others,_ `isContract` will return `false` for the following types of addresses: \\n — an externally-owned account \\n — a contract in construction \\n — an address where a contract will be created \\n — an address where a contract lived, but was destroyed After all of those checks clear out, the contract attempts to make a call to the `borrower` contract with a `call` that attempts to call a f(x) on the other end by the name of `receiveEther` that accepts a `uint256` as an argument with the `NaiveReceiverLendingPool`passing in its `FIXED_FEE` as the argument. We do two final checks for this f(x) where we check if `success` of our external call is true and then we make sure that we were given our refund for the flashLoan at the end which will either succeed and everything is squared away or it fails and all the changes get reverted. Nothing too big jumps out from any of this so far, but I’m very curious to see the f(x) on the other end that’s handling reception of the ETH as well as the payment of the fee. ![Picture of the fallback function within NaiveRecieverLendingPool](https://storage.googleapis.com/papyrus_images/9b37d993b8067978930b3b310a36d8d66b401d1a8328886f7adeed15310ec037.png) Picture of the fallback function within NaiveRecieverLendingPool ### fallback This just looks like a basic fallback function for allowing the contract to accept deposits. No logic is found in the function, so I won’t spend too much time thinking about this one. > FlashLoanReceiver.sol ![An image of the FlashLoanReceiver smart contract](https://storage.googleapis.com/papyrus_images/64832537c2474c3f017b61bb4366119a347d10665438218d7b7e6a63552908d6.png) An image of the FlashLoanReceiver smart contract Ah, beautiful. There doesn’t seem to be any sort of guard for reentrancy imported here. Maybe we can use that? We are still using SafeMath and Address but this time we’re specifically using it for `address payable` and then immediately there’s this `address payable private pool` state that doesn’t get an instantiation from jump. ### constructor Here we have a `public` constructor that is just taking `address payable pool` as its argument and setting `pool` to be an instance of the contract that’s being pointed to with the `poolAddress` variable, in our case the `NaiveReceiverLenderPool` ### receiveEther This is what we came for. We have a function that can be called by anyone to send value to this smart contract, and we also attach a `fee` argument here that maps to what we passed in as our `FIXED_FEE` and immediately makes a check to be sure that the address that is sending this call is matching the `pool` instance that was created in the constructor when the contract was deployed. We see that the f(x) creates `amountToBeRepaid` that takes whatever value is sent over for the loan request plus a fee of 1ETH as defined by `FIXED_FEE` , and then we make a quick check to be certain that the amount requested in addition to the `FIXED_FEE` is available in the contract to actually pay back the `NaiveReceiverLenderPool` at the end of this call. Some action is executed and then the money is returned to the lending pool; however, **the issue here is that the** `Receiver` **has absolutely no way to _reject_ incoming loans that are called with their address. What this means is that, when we look at** `NaiveReceiverLenderPool` **we can see that the function to call flashLoan is only flagged to be called externally (by anyone) and we can supply an address of our choosing to accept the flashLoan by passing it into** `borrower` **, and in this case we can bankrupt** `Receiver` **entirely by asking for flash loans that are getting hit with a fee.** ### \_executeActionDuringFlashLoan This is just some general `internal` f(x) that only `Receiver` can call in order to pull off whatever trading insight they believe they’ve reasoned upon. Nothing to see here beyond the confines of your own imagination. ### fallback A fallback for just depositing money into the contract. Solution -------- ![An image of the JS snippet used to verify exploit](https://storage.googleapis.com/papyrus_images/6a880e179987898958192fb9e83a3e729eb7a3ca16d1edbc6df1b3ffa1e6243f.png) An image of the JS snippet used to verify exploit So the solution here is to just call `flashLoan` from our wallet, and we don’t even have to specify a loan amount as long as we have a 1 ETH `FIXED_FEE` , so we just drain the contract with 10 calls. I’m going to play around with the draining the contract in 1 transaction; I have a feeling there are a few options. ### Reflection This one made me feel bad 😞 I didn’t like draining some random person’s funds that’s cruel, but I did learn quite a bit about securing your contracts and making sure that receiving ether from a contract is always a more explicit flow for the `Receiver` — a few things I’d have done differently: 1. A check within the `flashLoan` f(x) inside of `NaiveReceiverLendingPool`to be sure that `borrower` is `msg.sender` would be one change I’d make here to ensure that we aren’t actually letting random people request loans for others. 2. We should probably add a check in this current `FlashLoanReceiver` to make sure that the trade was profitable or else we should revert like so `require(savedAmountFromExecuteFlashLoan > amountToBeRepaid + gasLimit)` This way we have the benefit of simply reverting which will cause `success` in `NaiveReceiverLendingPool` to be false which will cause the following `require` clause to fail. ### Conclusion This one was mostly just about considering the effects of withdrawal design and how dangerous it can be to not affirm origins of f(x) calls. --- *Originally published on [blokboy](https://paragraph.com/@blokboy/damn-vulnerable-defi-naive-receiver)*