Skip to content

Unstoppable

Statement

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.

Analysis

According to the statement our mission is to try to stop the pool for offering flash loans. No need to extract money, empty an account or anything else. Just, freeze the contract.

Understanding contracts

There are two contracts:

  • UnstoppableLender.sol: Contract which implements the logic of the lending flash loans.
  • ReceiverUnstoppable.sol: Contract which implements the logic of receiving funds from the lending pool.

Let's first analyze the lending pool contract:

This contract implements a pool of ERC20 tokens (damnValuableToken). In fact, the address of the underlying token is provided in the constructor.

It has two interesting functions:

  • depositTokens: Function that implements the deposit of tokens into the pool. Updating some internal variables.
  • flashLoan: Function that implements the lending logic. 
1.  function depositTokens(uint256 amount) external nonReentrant {
2.        require(amount > 0, "Must deposit at least one token");
3.        // Transfer token from sender. Sender must have first approved them.
4.        damnValuableToken.transferFrom(msg.sender, address(this), amount);
5.        poolBalance = poolBalance + amount;
6.    }

Some comments about this function:

  1. It first checks that the amount of damnValuableToken (ERC20 token) to be deposited is greater than 0.
  2. It performs a transfer from the address calling this function (the wallet doing the deposit) to the current pool. In order to be able to execute such functionality, the wallet should've already provided with permissions to do that (allowance)
  3. It stores and updates an internal variable poolBalance which holds the current amount of tokens that the pool has.

Now, let's move on to the following function:

0. function flashLoan(uint256 borrowAmount) external nonReentrant {
1.        require(borrowAmount > 0, "Must borrow at least one token");
2.
3.        uint256 balanceBefore = damnValuableToken.balanceOf(address(this));
4.        require(balanceBefore >= borrowAmount, "Not enough tokens in pool");
5.
6.        // Ensured by the protocol via the `depositTokens` function
7.        assert(poolBalance == balanceBefore);
8.        
9.        damnValuableToken.transfer(msg.sender, borrowAmount);
10.        
11.        IReceiver(msg.sender).receiveTokens(address(damnValuableToken), borrowAmount);
12.        
13.        uint256 balanceAfter = damnValuableToken.balanceOf(address(this));
14.        require(balanceAfter >= balanceBefore, "Flash loan hasn't been paid back");
18. }

Some notes about this code snippet:

  1. It has the nonReentrant modifier. So, no reentrancy possible.
  2. In the first three lines, it basically checks that the borrowed amount provided as a parameter is valid. In order to do so, it checks if it’s greater than zero and that it doesn't exceed the amount of tokens the lending pool has. If it does exceed, it reverts.
  3. The 7th line is a bit controversial and weird. It checks that the amount of damnValuableTokens that the pool currently has is the same as the internal variable that is updated in the depositTokens() function. Why is this carried out? Why would someone try to store the actual balance if this information is already stored in the ERC20 token contract? Remember that calls to that function are free because they are reading the blockchain and not writing anything.
  4. Line 11 calls the receiveToken() function from the ReceiverUnstoppable contract. This is where the borrower would implement the logic to leverage the loan received.
  5. Line 13-14 basically ensures that the loan has been repaid (with fees).

Final solution

According to the exercise statement we had to find a way to stop the pool offering loans. Line 7th seems very controversial as it is comparing two values that should be always the same but they are managed in different ways. If it was only possible to change one of them in order to desync them... Well.. in fact it is actually possible. The depositTokens() is adding the deposited value to the internal value each time this function is called. On the other side, the ERC20 balanceOf() function, checks the amount of tokens that the address has according to the ERC20 contract. So.. is the depositTokens() the only way to deposit/send damnValuableToken tokens in this pool? Clearly, no. What somebody could do is just to send 1 damnValuableToken to this pool address and this will make "update" the balance of the pool's damnValuableTokens but the poolBalance will remain the same.

By desyncing these two variables, line 7th will always fail and therefore this function will always revert, stopping the pool from giving flash loans.

Exploit

I modified the unstoppable.challenge.js with: 

 it('Exploit', async function () {
    await this.token.connect(attacker).transfer(this.pool.address, 1)
});

Fix

As part of my learning phase of Smart Contracts Security, I like to think how I could solve this issue. However, in this particular case I think that the poolBalance variable is useless in this context. I'd remove it and therefore remove the check in line 7th.