Great, I’ll rename it accordingly… took a closer look at the fineprint today, some more feedback:
“If the loan cannot be executed
flashFee MUST revert.” - Since we’re already exposing flashSupply, is this really needed? I understand this this would be nice, but maybe change this to SHOULD so if we don’t, we’re still compliant? Flashloans aren’t the MAIN feature of the contract, and code space is limited and less code = more room for optimizations.
Keep in mind that is these functions are called as view functions, their values may no longer be valid by the time a transaction uses them and if they are called from another contract, you would wish to minimize gas usage, so this check is not relevant (as you’ve most likely already retrieved totalSupply), but just adds gas.
How will you know that the optional batchFlashLoan is implemented from another contract?
batchFlashLoan function MUST revert if the length of the
tokens arrays differ.” - I know these kind of checks are commonly used by Solidity devs, but unless there is a security risk or a missed param can cause loss of funds (such as transfer to address(0)), why include this? I don’t see any harm here. Using address(0) as token will fail, and 0 as amount will just loan nothing. I chose to simply assume the tokens length is correct, if there are more amounts, they will be ignored, if there are less, I’ll have to check what happens… not sure if solidity does bounds checking.
batchFlashLoan function MUST include a
fees argument to
onBatchFlashLoan with the fee to pay for each individual
amount lent.” - may want to mention that the order should be the same. I don’t care, but just for completeness.
In Rationale, is this out of date? "
flashFee reverts on unsupported tokens, because returning a numerical value would be incorrect."
And that brings us to the big dilemma: PULL or PUSH the returned funds…
- You can rely directly on
token.balanceOf(address(this)) in the lender for logic.
- The receiver must approve the tokens. If the receiver also approves tokens for another reason to the lender, some tokens will cause issues as the approve has to be changed to 0 before it can be set again.
- If users approve to the lender to interact with other functionality on that contract, users are at risk of a grieving attack stealing fees that could drain their entire balance. All the ways I can think of to mitigate this will add contracts/gas.
- More flexibility to optimize and reduce number of transfers needed.
- No need to approve. No risk of grieving.
- You have to keep track of the token balance expected in the contract. Some contracts do this already, such as UniSwap V2 (reserves), while other, such as cTokens? do not.
- Care must be taken that there are no reentrancy issues
- Care must be taken that a batch with the same token twice doesn’t cause issues.
Neither is perfect, but since we already track the balances in the contract, we are going with PUSH. Once we confirm that our code is secure, there are only upsides left for the user of the flashLoans (lower gas and more flexible). Initially we had pull, but the grieving attack issue made us switch. Happy to reconsider if someone can suggest a good solution to the grieving problem.
Anyway, great work on this EIP… not an easy one