Add internal _flashFee without validation to ERC20FlashMint

[mazenkhalil]

🧐 Motivation

I have came across the below function where implementers can to override to customize the functionality.
The thing is the validation within the base implementation. 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.

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

Similar case can be seen in the below function. But this time the base implementation has assumption than can be overloaded as well.

    function maxFlashLoan(address token) public view virtual override returns (uint256) {
        return token == address(this) ? type(uint256).max - ERC20.totalSupply() : 0;
    }

I believe such flow (validation/assumption within override functions) is dangerous and should be addressed to mitigate the risks that could arise in implementation contracts.

[frangio]

A fix for this would be to define an internal function _flashFee to be the one that should be overriden.

[mazenkhalil]

Is it ok to make a major change? Adding _flashFee function would require implementers to change their current implementation upon upgrade. (considering that override modifier will be removed from current function)

[frangio]

Just seeing the question in the last comment.

(considering that override modifier will be removed from current function)

The override modifier shouldn't be removed.

[nirban256]

Hey @frangio I would like to work on this issue, can I start working on it?

[frangio]

Yes go ahead.

[nirban256]

ok

[nirban256]

Hey @frangio do I need to write the tests or is it fine if I push the code directly?

[Amxx]

the new code need to be covered by test. Existing one may or may not be enough. In this case, we will probably want some small tests.
Also, we will need a changelog entry.

[nirban256]

the new code need to be covered by test. Existing one may or may not be enough. In this case, we will probably want some small tests. Also, we will need a changelog entry.

What type of test will be needed?
Like the function _flashFee is calling another function flashFee inside it which is returning the amount, so what type of test needs to be written like the amount that is being returned by the flashFee function is equal to the amount that is received by the _flashFee function?

[frangio]

@nirban256 Please just do your best effort to test it and open a PR, we will leave feedback in the PR.

[nirban256]

ok @frangio