ERC3156 Flash loans extension improvement

[mazenkhalil]

🧐 Motivation
The current flash loans implementation suggests to burn the fee of the flash loan with no easy way for implementers to override this default behavior. e.g. If the token is capped, this leads to shrink the total supply over time [considering the fee is higher than 0].

This behavior is not well documented and the Wizard doesn't consider this.

📝 Details
I would suggest to implement two strategies for the flash loan fee processing. Where implementers explicitly set the right strategy for their use case.

1. BURN: The loaned amount and the fee amount get burnt (current implementation).
2. HOLD: The loaned amount get burnt and the fee amount get transferred to the contract itself (address(this)).
3. CUSTOM: The fee amount to be transferred to different address (Breaks the trust?)

[Amxx]

Hello @mazenkhalil

I agree that having the ability to easily override/reconfigure the contract so that fees are transferred to a particular address (not burning them) would be an interesting addition.

I'm not exactly sure how we would like this to be implemented, considering burning the amount and the fee saves gas, and doing that in two steps (one of which could be overridden) would be more expensive.

[frangio]

This is a great point. We need to decide how we will expose this functionality. Am I right that we can simply model this as the ability to customize the fee receiver? We can say that if the receiver is 0 the fee is burned, and otherwise the fee is transferred to that account. So we could add an internal virtual function that people can override to customize:

function _flashFeeReceiver(address token) internal view virtual returns (address) {
    return address(0);
}

Regarding @Amxx's concern about implementation, we if the receiver is zero we can burn it all together.

[mazenkhalil]

I have discovered another interesting prone-to-error flow.

    function flashFee(address token, uint256 amount) public view virtual override returns (uint256) {
        require(token == address(this), "ERC20FlashMint: wrong token");
        // silence warning about unused variable without the addition of bytecode.
        amount;
        return 0;
    }

The flashFee function is suppose to be overloaded in order to update the flash fee amount. The thing is the validation defined in the function itself. Basically as a developer, I would expect the function to act as getter returning the fee amount while keeping all validations in place. But overriding this function leads to overriding the default validations as well.
Any suggestion?

[frangio]

@mazenkhalil It's a valid point. Can you open a new issue about it?

[Amxx]

Basically as a developer, I would expect the function to act as getter returning the fee amount while keeping all validations in place.

Please don't forget the ERC requirements

The flashFee function MUST return the fee charged for a loan of amount token. If the token is not supported flashFee MUST revert.

[mazenkhalil]

The point is the validation within the function that can be mistakenly overridden by implementers. More details about this can be found on issue #3331