Skip to content

Commit

Permalink
Add customizable fee receiver to ERC20FlashMint (#3327)
Browse files Browse the repository at this point in the history
Co-authored-by: Mazen Khalil <mazen@immunityledger.org>
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
  • Loading branch information
3 people committed May 6, 2022
1 parent 07b1b47 commit 3b9381d
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -5,6 +5,7 @@
* `Clones`: optimize clone creation ([#3329](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3329))
* `TimelockController`: Migrate `_call` to `_execute` and allow inheritance and overriding similar to `Governor`. ([#3317](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3317))
* `CrossChainEnabledPolygonChild`: replace the `require` statement with the custom error `NotCrossChainCall`. ([#3380](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3380))
* `ERC20FlashMint`: Add customizable flash fee receiver. ([#3327](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3327))

## 4.6.0 (2022-04-26)

Expand Down
28 changes: 28 additions & 0 deletions contracts/mocks/ERC20FlashMintMock.sol
Expand Up @@ -5,6 +5,9 @@ pragma solidity ^0.8.0;
import "../token/ERC20/extensions/ERC20FlashMint.sol";

contract ERC20FlashMintMock is ERC20FlashMint {
uint256 _flashFeeAmount;
address _flashFeeReceiverAddress;

constructor(
string memory name,
string memory symbol,
Expand All @@ -13,4 +16,29 @@ contract ERC20FlashMintMock is ERC20FlashMint {
) ERC20(name, symbol) {
_mint(initialAccount, initialBalance);
}

function mint(address account, uint256 amount) public {
_mint(account, amount);
}

function setFlashFee(uint256 amount) public {
_flashFeeAmount = amount;
}

function flashFee(address token, uint256 amount) public view virtual override returns (uint256) {
super.flashFee(token, amount);
return _flashFeeAmount;
}

function setFlashFeeReceiver(address receiver) public {
_flashFeeReceiverAddress = receiver;
}

function flashFeeReceiver() public view returns (address) {
return _flashFeeReceiver();
}

function _flashFeeReceiver() internal view override returns (address) {
return _flashFeeReceiverAddress;
}
}
18 changes: 17 additions & 1 deletion contracts/token/ERC20/extensions/ERC20FlashMint.sol
Expand Up @@ -43,6 +43,16 @@ abstract contract ERC20FlashMint is ERC20, IERC3156FlashLender {
return 0;
}

/**
* @dev Returns the receiver address of the flash fee. By default this
* implementation returns the address(0) which means the fee amount will be burnt.
* This function can be overloaded to change the fee receiver.
* @return The address for which the flash fee will be sent to.
*/
function _flashFeeReceiver() internal view virtual returns (address) {
return address(0);
}

/**
* @dev Performs a flash loan. New tokens are minted and sent to the
* `receiver`, who is required to implement the {IERC3156FlashBorrower}
Expand Down Expand Up @@ -73,8 +83,14 @@ abstract contract ERC20FlashMint is ERC20, IERC3156FlashLender {
receiver.onFlashLoan(msg.sender, token, amount, fee, data) == _RETURN_VALUE,
"ERC20FlashMint: invalid return value"
);
address flashFeeReceiver = _flashFeeReceiver();
_spendAllowance(address(receiver), address(this), amount + fee);
_burn(address(receiver), amount + fee);
if (fee == 0 || flashFeeReceiver == address(0)) {
_burn(address(receiver), amount + fee);
} else {

This comment has been minimized.

Copy link
@eugenioclrc

eugenioclrc Jun 14, 2023

I think is more appropiate to do;

diff --git a/contracts/token/ERC20/extensions/ERC20FlashMint.sol b/contracts/token/ERC20/extensions/ERC20FlashMint.sol
index 09c20bac..a6c663a3 100644
--- a/contracts/token/ERC20/extensions/ERC20FlashMint.sol
+++ b/contracts/token/ERC20/extensions/ERC20FlashMint.sol
@@ -117,7 +117,9 @@ abstract contract ERC20FlashMint is ERC20, IERC3156FlashLender {
         }
         address flashFeeReceiver = _flashFeeReceiver();
         _spendAllowance(address(receiver), address(this), amount + fee);
-        if (fee == 0 || flashFeeReceiver == address(0)) {
+        if (fee == 0) {
+            _burn(address(receiver), amount);
+        } else if (flashFeeReceiver == address(0)) {
             _burn(address(receiver), amount + fee);
         } else {
             _burn(address(receiver), amount);

This comment has been minimized.

Copy link
@Amxx

Amxx Jun 14, 2023

Collaborator

Can you explain why you think that would be better ?

This comment has been minimized.

Copy link
@eugenioclrc

eugenioclrc Jun 14, 2023

Hi @Amxx , my two cents here;

 if (fee == 0 || flashFeeReceiver == address(0)) {
    _burn(address(receiver), amount + fee); // this could be end up doing `amount + 0`
} else {
    _burn(address(receiver), amount);
    _transfer(address(receiver), flashFeeReceiver, fee);
}

Current implementation has an if with two cases, but i think it should have three cases with this order;

  1. If fee is 0, burn the loan amount
  2. if flashFeeReceiver address(0), burn the loan amount plus fee
  3. else burn amount, and transfer from receiver to flashFeeReceiver the fee

What do you think about this?

uint256 amountWithFee = amount + fee;
_spendAllowance(address(receiver), address(this), amountWithFee);
if (fee == 0) {
    _burn(address(receiver), amount);
} else if (flashFeeReceiver == address(0)) {
    _burn(address(receiver), amountWithFee);
} else {
    _burn(address(receiver), amount);
    _transfer(address(receiver), flashFeeReceiver, fee);
}

And perhaps this could be easier to read, instead of three cases just cache amount + fee;

uint256 amountWithFee = amount + fee;
_spendAllowance(address(receiver), address(this), amountWithFee);
if (fee == 0 || flashFeeReceiver == address(0)) {
    _burn(address(receiver), amountWithFee);
} else {
    _burn(address(receiver), amount);
    _transfer(address(receiver), flashFeeReceiver, fee);
}

This comment has been minimized.

Copy link
@Amxx

Amxx Jun 14, 2023

Collaborator

Both the current code and your proposal achieve the exact same behavior.
I don't think have 3 cases makes it easier to read. I would even go further and say it decreases the readability (and possibly the resulting codesize)

Caching amountWithFee is a possibility, but again, I'm not sure if makes things any clearer. The gas saving are negligeable.

_burn(address(receiver), amount);
_transfer(address(receiver), flashFeeReceiver, fee);
}
return true;
}
}
56 changes: 55 additions & 1 deletion test/token/ERC20/extensions/ERC20FlashMint.test.js
Expand Up @@ -8,7 +8,7 @@ const ERC20FlashMintMock = artifacts.require('ERC20FlashMintMock');
const ERC3156FlashBorrowerMock = artifacts.require('ERC3156FlashBorrowerMock');

contract('ERC20FlashMint', function (accounts) {
const [ initialHolder, other ] = accounts;
const [ initialHolder, other, anotherAccount ] = accounts;

const name = 'My Token';
const symbol = 'MTKN';
Expand Down Expand Up @@ -40,6 +40,12 @@ contract('ERC20FlashMint', function (accounts) {
});
});

describe('flashFeeReceiver', function () {
it('default receiver', async function () {
expect(await this.token.flashFeeReceiver()).to.be.eq(ZERO_ADDRESS);
});
});

describe('flashLoan', function () {
it('success', async function () {
const receiver = await ERC3156FlashBorrowerMock.new(true, true);
Expand Down Expand Up @@ -86,5 +92,53 @@ contract('ERC20FlashMint', function (accounts) {
// _mint overflow reverts using a panic code. No reason string.
await expectRevert.unspecified(this.token.flashLoan(receiver.address, this.token.address, MAX_UINT256, data));
});

describe('custom flash fee & custom fee receiver', function () {
const receiverInitialBalance = new BN(200000);
const flashFee = new BN(5000);

beforeEach('init reciever balance & set flash fee',async function () {
this.receiver = await ERC3156FlashBorrowerMock.new(true, true);
const receipt = await this.token.mint(this.receiver.address, receiverInitialBalance);
await expectEvent(receipt, 'Transfer', { from: ZERO_ADDRESS, to: this.receiver.address, value: receiverInitialBalance });
expect(await this.token.balanceOf(this.receiver.address)).to.be.bignumber.equal(receiverInitialBalance);

await this.token.setFlashFee(flashFee);
expect(await this.token.flashFee(this.token.address, loanAmount)).to.be.bignumber.equal(flashFee);
});

it('default flash fee receiver', async function () {
const { tx } = await this.token.flashLoan(this.receiver.address, this.token.address, loanAmount, '0x');
await expectEvent.inTransaction(tx, this.token, 'Transfer', { from: ZERO_ADDRESS, to: this.receiver.address, value: loanAmount });
await expectEvent.inTransaction(tx, this.token, 'Transfer', { from: this.receiver.address, to: ZERO_ADDRESS, value: loanAmount.add (flashFee)});
await expectEvent.inTransaction(tx, this.receiver, 'BalanceOf', { token: this.token.address, account: this.receiver.address, value: receiverInitialBalance.add(loanAmount) });
await expectEvent.inTransaction(tx, this.receiver, 'TotalSupply', { token: this.token.address, value: initialSupply.add (receiverInitialBalance).add(loanAmount) });

expect(await this.token.totalSupply()).to.be.bignumber.equal(initialSupply.add(receiverInitialBalance).sub(flashFee));
expect(await this.token.balanceOf(this.receiver.address)).to.be.bignumber.equal(receiverInitialBalance.sub(flashFee));
expect(await this.token.balanceOf(await this.token.flashFeeReceiver())).to.be.bignumber.equal('0');
expect(await this.token.allowance(this.receiver.address, this.token.address)).to.be.bignumber.equal('0');
});

it('custom flash fee receiver', async function () {
const flashFeeReceiverAddress = anotherAccount;
await this.token.setFlashFeeReceiver(flashFeeReceiverAddress);
expect(await this.token.flashFeeReceiver()).to.be.eq(flashFeeReceiverAddress);

expect(await this.token.balanceOf(flashFeeReceiverAddress)).to.be.bignumber.equal('0');

const { tx } = await this.token.flashLoan(this.receiver.address, this.token.address, loanAmount, '0x');
await expectEvent.inTransaction(tx, this.token, 'Transfer', { from: ZERO_ADDRESS, to: this.receiver.address, value: loanAmount });
await expectEvent.inTransaction(tx, this.token, 'Transfer', { from: this.receiver.address, to: ZERO_ADDRESS, value: loanAmount });
await expectEvent.inTransaction(tx, this.token, 'Transfer', { from: this.receiver.address, to: flashFeeReceiverAddress, value: flashFee });
await expectEvent.inTransaction(tx, this.receiver, 'BalanceOf', { token: this.token.address, account: this.receiver.address, value: receiverInitialBalance.add(loanAmount) });
await expectEvent.inTransaction(tx, this.receiver, 'TotalSupply', { token: this.token.address, value: initialSupply.add (receiverInitialBalance).add(loanAmount) });

expect(await this.token.totalSupply()).to.be.bignumber.equal(initialSupply.add(receiverInitialBalance));
expect(await this.token.balanceOf(this.receiver.address)).to.be.bignumber.equal(receiverInitialBalance.sub(flashFee));
expect(await this.token.balanceOf(flashFeeReceiverAddress)).to.be.bignumber.equal(flashFee);
expect(await this.token.allowance(this.receiver.address, flashFeeReceiverAddress)).to.be.bignumber.equal('0');
});
});
});
});

0 comments on commit 3b9381d

Please sign in to comment.