First Flight #3: Thunder Loan

Summary

The flashloan function is subject to reentrancy. The reason is because the exchange rate is updated (increasing it to account for the fees accrued during a flash loan) before the external call is made to MockFlashLoanReceiver.sol. There is also no need to call updateExchangeRate where it is called because it isn't used for the rest of the function.

Vulnerability Details

The relevant part of the flashloan function is here where updateExchangeRate is called before the external call is made:

Impact

You open yourself up for a reentrancy attack which could drain funds. Granted this is mostly mitigated by the fact that there is a post check that the ending balance of the tokens in AssetToken.sol is not less than the starting balance plus the fee. But that check doesn't mean that an attacker couldn't figure out how to drive up their amount of asset tokens using flash loan but still return starting balance plus fee of underlying tokens and then withdraw more than their fair share of tokens afterward.

Tools Used

Manual review

Recommendations

Move the call to updateExchangeRate to the end of 'flashloan' or, better yet, as i suggested in another finding, don't use the concept of exchange rate to calculate how many tokens/asset tokens depositors get upon deposit and redemption because it is unnecessary complication.