Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ERC3156 extension of ERC20 (flash minting and lending) #2543

Merged
merged 25 commits into from
Apr 6, 2021
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
b263508
add ERC3156 extension of ERC20
Amxx Feb 25, 2021
6130c24
ERC3165 documentation
Amxx Mar 1, 2021
0a6f007
rename ERC3165 to ERC20FlashMint + testing for flashloan
Amxx Mar 1, 2021
5f9a11c
update ERC20 documentation
Amxx Mar 1, 2021
d1a156e
add changelog entry
Amxx Mar 10, 2021
e346ef6
ERC20 extension documentation
Amxx Mar 15, 2021
ac7d0d9
Update contracts/token/ERC20/README.adoc
Amxx Mar 18, 2021
a0f4762
Update contracts/token/ERC20/README.adoc
Amxx Mar 18, 2021
14f12e7
Update contracts/token/ERC20/README.adoc
Amxx Mar 18, 2021
0a0a6f4
Update contracts/token/ERC20/extensions/draft-ERC20FlashMint.sol
Amxx Mar 18, 2021
1650488
Update test/token/ERC20/extensions/draft-ERC20FlashMint.test.js
Amxx Mar 18, 2021
1cb7ef0
Update test/token/ERC20/extensions/draft-ERC20FlashMint.test.js
Amxx Mar 18, 2021
90c774d
Update contracts/mocks/ERC3156FlashBorrowerMock.sol
Amxx Mar 18, 2021
b1c1f6c
address PR comments
Amxx Mar 18, 2021
9178432
fix typos in readme
Amxx Mar 25, 2021
3cec88e
Update contracts/token/ERC20/extensions/draft-ERC20FlashMint.sol
Amxx Mar 26, 2021
d64e6e6
Update contracts/token/ERC20/extensions/draft-ERC20FlashMint.sol
Amxx Mar 26, 2021
6b9bcdc
Update contracts/token/ERC20/extensions/draft-ERC20FlashMint.sol
Amxx Mar 26, 2021
67df750
Update contracts/token/ERC20/extensions/draft-ERC20FlashMint.sol
Amxx Mar 26, 2021
5f7617b
Update contracts/token/ERC20/extensions/draft-ERC20FlashMint.sol
Amxx Mar 26, 2021
581b23b
flashFee revert is token is invalid
Amxx Mar 26, 2021
3039307
Merge remote-tracking branch 'Amxx/feature/ERC3156' into feature/ERC3156
Amxx Mar 26, 2021
17db268
fix tests for ERC20FlashMint
Amxx Mar 26, 2021
684f85c
Add warning in the flash borrower mock
Amxx Mar 29, 2021
2e61d2a
fix typo
frangio Apr 6, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -5,6 +5,7 @@
* `IERC20Metadata`: add a new extended interface that includes the optional `name()`, `symbol()` and `decimals()` functions. ([#2561](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2561))
* `ERC777`: make reception acquirement optional in `_mint`. ([#2552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2552))
* `ERC20Permit`: add a `_useNonce` to enable further usage of ERC712 signatures. ([#2565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2565))
* `ERC20FlashMint`: add an implementation of the ERC3156 extension for flash-minting ERC20 tokens. ([#2543](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2543))

## 4.0.0 (2021-03-23)

Expand Down
66 changes: 66 additions & 0 deletions contracts/interfaces/IERC3156.sol
@@ -0,0 +1,66 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

/**
* @dev Interface of the ERC3156 FlashBorrower, as defined in
* https://eips.ethereum.org/EIPS/eip-3156[ERC-3156].
*/
interface IERC3156FlashBorrower {
/**
* @dev Receive a flash loan.
* @param initiator The initiator of the loan.
* @param token The loan currency.
* @param amount The amount of tokens lent.
* @param fee The additional amount of tokens to repay.
* @param data Arbitrary data structure, intended to contain user-defined parameters.
* @return The keccak256 hash of "ERC3156FlashBorrower.onFlashLoan"
*/
function onFlashLoan(
address initiator,
address token,
uint256 amount,
uint256 fee,
bytes calldata data
) external returns (bytes32);
}

/**
* @dev Interface of the ERC3156 FlashLender, as defined in
* https://eips.ethereum.org/EIPS/eip-3156[ERC-3156].
*/
interface IERC3156FlashLender {
/**
* @dev The amount of currency available to be lended.
* @param token The loan currency.
* @return The amount of `token` that can be borrowed.
*/
function maxFlashLoan(
address token
) external view returns (uint256);

/**
* @dev The fee to be charged for a given loan.
* @param token The loan currency.
* @param amount The amount of tokens lent.
* @return The amount of `token` to be charged for the loan, on top of the returned principal.
*/
function flashFee(
address token,
uint256 amount
) external view returns (uint256);

/**
* @dev Initiate a flash loan.
* @param receiver The receiver of the tokens in the loan, and the receiver of the callback.
* @param token The loan currency.
* @param amount The amount of tokens lent.
* @param data Arbitrary data structure, intended to contain user-defined parameters.
*/
function flashLoan(
IERC3156FlashBorrower receiver,
address token,
uint256 amount,
bytes calldata data
) external returns (bool);
}
47 changes: 47 additions & 0 deletions contracts/mocks/ERC3156FlashBorrowerMock.sol
@@ -0,0 +1,47 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;


import "../token/ERC20/IERC20.sol";
import "../interfaces/IERC3156.sol";
import "../utils/Address.sol";

contract ERC3156FlashBorrowerMock is IERC3156FlashBorrower {
bytes32 constant internal RETURN_VALUE = keccak256("ERC3156FlashBorrower.onFlashLoan");

bool immutable _enableApprove;
bool immutable _enableReturn;

event BalanceOf(address token, address account, uint256 value);
event TotalSupply(address token, uint256 value);

constructor(bool enableReturn, bool enableApprove) {
_enableApprove = enableApprove;
_enableReturn = enableReturn;
}

function onFlashLoan(
address /*initiator*/,
address token,
uint256 amount,
uint256 fee,
bytes calldata data
) public override returns (bytes32) {
require(msg.sender == token);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is useless if you don't verify that you trust msg.sender.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a mock, its not designed to be used outside our testing suit anyway.

The argument used to be that doing a functionCall with arbitrary data to an arbitrary address (line 38) is bad, and we should not show that. Someone could use this endpoint with an arbitrary token address, 0 amount, 0 fee, and an arbitrary payload, which could transfer token held by the receiver.

I "fixed" that by saying that (within our testsuite) only the token should advertize onFlashLoan ... which is not how the ERC might me used in the broader sens, but corresponds to the usage we push in this example.

TLDR: I disagree, it prevents line 38 being used to do arbitrary calls to arbitrary address ... anyone using this method to do anything nasty will only be able to do it to themselves.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm concerned about is that coding flash borrowers that are safe is not trivial. Coders using the OZ flash lending implementation might come into the OZ repo to see how you did it, and they will find an unsafe implementation of a flash borrower.

Going that extra mile and making sure that your flash borrower is a decent example on how to code one would be useful for the community.

My mock flash borrower is not great itself, but it could quickly be polished up, and I was able to fully test the reference implementation with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I share these concerns. I think the mock should be either absurdly wrong or have a huge warning, or it should be correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hear your concern, and I do agree we should have extensive documentation in this source code saying this is just for testing, and should not be considered a good example of how to do FlashBorrower.

That being said, I'm having a hard time to see which security issues are not covered by the require(msg.sender == token) check. Do you have anything in mind ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, you might be right. I can't think of any scenario where require (token == msg.sender) let's a malicious lender through. It might be protection enough for a borrower that only accepts loans from flash minters, and not from generic flash lenders.

A bit of weird scenario to discriminate lenders on their internal implementation, but maybe not unsafe.


emit BalanceOf(token, address(this), IERC20(token).balanceOf(address(this)));
emit TotalSupply(token, IERC20(token).totalSupply());

if (data.length > 0) {
// WARNING: This code is for testing purposes only! Do not use.
Address.functionCall(token, data);
Amxx marked this conversation as resolved.
Show resolved Hide resolved
}

if (_enableApprove) {
IERC20(token).approve(token, amount + fee);
}

return _enableReturn ? RETURN_VALUE : bytes32(0);
}
}
17 changes: 17 additions & 0 deletions contracts/mocks/ERC3156Mock.sol
@@ -0,0 +1,17 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;


import "../token/ERC20/extensions/draft-ERC20FlashMint.sol";

contract ERC20FlashMintMock is ERC20FlashMint {
constructor (
string memory name,
string memory symbol,
address initialAccount,
uint256 initialBalance
) ERC20(name, symbol) {
_mint(initialAccount, initialBalance);
}
}
21 changes: 11 additions & 10 deletions contracts/token/ERC20/README.adoc
Expand Up @@ -15,21 +15,22 @@ There a few core contracts that implement the behavior specified in the EIP:

Additionally there are multiple custom extensions, including:

* {ERC20Permit}: gasless approval of tokens.
* {ERC20Snapshot}: efficient storage of past token balances to be later queried at any point in time.
* {ERC20Burnable}: destruction of own tokens.
* {ERC20Capped}: enforcement of a cap to the total supply when minting tokens.
* {ERC20Pausable}: ability to pause token transfers.
* {ERC20Snapshot}: efficient storage of past token balances to be later queried at any point in time.
* {ERC20Permit}: gasless approval of tokens (standardized as ERC2612).
* {ERC20FlashMint}: token level support for flash loans through the minting and burning of ephemeral tokens (standardized as ERC3156).

Finally, there are some utilities to interact with ERC20 contracts in various ways.

* {SafeERC20}: a wrapper around the interface that eliminates the need to handle boolean return values.
* {TokenTimelock}: hold tokens for a beneficiary until a specified time.

The following related EIPs are in draft status and can be found in the drafts directory.
The following related EIPs are in draft status.

- {IERC20Permit}
- {ERC20Permit}
- {ERC20FlashMint}

NOTE: This core set of contracts is designed to be unopinionated, allowing developers to access the internal functions in ERC20 (such as <<ERC20-_mint-address-uint256-,`_mint`>>) and expose them as external functions in the way they prefer. On the other hand, xref:ROOT:erc20.adoc#Presets[ERC20 Presets] (such as {ERC20PresetMinterPauser}) are designed using opinionated patterns to provide developers with ready to use, deployable contracts.

Expand All @@ -43,22 +44,22 @@ NOTE: This core set of contracts is designed to be unopinionated, allowing devel

== Extensions

{{ERC20Snapshot}}

{{ERC20Pausable}}

{{ERC20Burnable}}

{{ERC20Capped}}

{{ERC20Pausable}}

{{ERC20Snapshot}}

== Draft EIPs

The following EIPs are still in Draft status. Due to their nature as drafts, the details of these contracts may change and we cannot guarantee their xref:ROOT:releases-stability.adoc[stability]. Minor releases of OpenZeppelin Contracts may contain breaking changes for the contracts in this directory, which will be duly announced in the https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/CHANGELOG.md[changelog]. The EIPs included here are used by projects in production and this may make them less likely to change significantly.

{{IERC20Permit}}

{{ERC20Permit}}

{{ERC20FlashMint}}

== Presets

These contracts are preconfigured combinations of the above features. They can be used through inheritance or as models to copy and paste their source code.
Expand Down
73 changes: 73 additions & 0 deletions contracts/token/ERC20/extensions/draft-ERC20FlashMint.sol
@@ -0,0 +1,73 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "../../../interfaces/IERC3156.sol";
import "../ERC20.sol";

/**
* @dev Implementation of the ERC3156 Flash loans extension, as defined in
* https://eips.ethereum.org/EIPS/eip-3156[ERC-3156].
*
* Adds the {flashLoan} method, which provides flash loan support at the token
* level. By default there is no fee, but this can be changed by overriding {flashFee}.
*/
abstract contract ERC20FlashMint is ERC20, IERC3156FlashLender {
bytes32 constant private RETURN_VALUE = keccak256("ERC3156FlashBorrower.onFlashLoan");

/**
* @dev Returns the maximum amount of tokens available for loan.
* @param token The address of the token that is requested.
* @return The amont of token that can be loaned.
*/
function maxFlashLoan(address token) public view override returns (uint256) {
return token == address(this) ? type(uint256).max - ERC20.totalSupply() : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have clear reaons to use ERC20.totalSupply here instead of totalSupply?

I'm not saying we should change anything but I'd like to understand the reasoning, and maybe it would be a good idea to document it in a comment here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that, since totalSupply can on overridden, and what we really care about is the mint not overflowing if someone wanted to flashMint, then te best way to protect _mint overflowing is to use the internal bookkeeping variables.

But If you'd rather use the overloaded version I have no strong argument. (worst case scenario some flashMint call revert, but that would not break the ERC20 token.

}

/**
* @dev Returns the fee applied when doing flash loans. By default this
* implementation has 0 fees. This function can be overloaded to make
* the flash loan mechanism deflationary.
* @param token The token to be flash loaned.
* @param amount The amount of tokens to be loaned.
* @return The fees applied to the corresponding flash loan.
*/
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;
}

/**
* @dev Performs a flash loan. New tokens are minted and sent to the
* `receiver`, who is required to implement the {IERC3156FlashBorrower}
* interface. By the end of the flash loan, the receiver is expected to own
* amount + fee tokens and have them approved back to the token contract itself so
* they can be burned.
* @param receiver The receiver of the flash loan. Should implement the
* {IERC3156FlashBorrower.onFlashLoan} interface.
* @param token The token to be flash loaned. Only `address(this)` is
* supported.
* @param amount The amount of tokens to be loaned.
* @param data An arbitrary datafield that is passed to the receiver.
* @return `true` is the flash loan was successfull.
*/
function flashLoan(
IERC3156FlashBorrower receiver,
address token,
uint256 amount,
bytes calldata data
)
public virtual override returns (bool)
{
uint256 fee = flashFee(token, amount);
_mint(address(receiver), amount);
require(receiver.onFlashLoan(msg.sender, token, amount, fee, data) == RETURN_VALUE, "ERC20FlashMint: invalid return value");
uint256 currentAllowance = allowance(address(receiver), address(this));
require(currentAllowance >= amount + fee, "ERC20FlashMint: allowance does not allow refund");
_approve(address(receiver), address(this), currentAllowance - amount - fee);
_burn(address(receiver), amount + fee);
return true;
}
}
90 changes: 90 additions & 0 deletions test/token/ERC20/extensions/draft-ERC20FlashMint.test.js
@@ -0,0 +1,90 @@
/* eslint-disable */

const { BN, constants, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers');
const { expect } = require('chai');
const { MAX_UINT256, ZERO_ADDRESS, ZERO_BYTES32 } = constants;

const ERC20FlashMintMock = artifacts.require('ERC20FlashMintMock');
const ERC3156FlashBorrowerMock = artifacts.require('ERC3156FlashBorrowerMock');

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

const name = 'My Token';
const symbol = 'MTKN';

const initialSupply = new BN(100);
const loanAmount = new BN(10000000000000);

beforeEach(async function () {
this.token = await ERC20FlashMintMock.new(name, symbol, initialHolder, initialSupply);
});

describe('maxFlashLoan', function () {
it('token match', async function () {
expect(await this.token.maxFlashLoan(this.token.address)).to.be.bignumber.equal(MAX_UINT256.sub(initialSupply));
});

it('token mismatch', async function () {
expect(await this.token.maxFlashLoan(ZERO_ADDRESS)).to.be.bignumber.equal('0');
});
});

describe('flashFee', function () {
it('token match', async function () {
expect(await this.token.flashFee(this.token.address, loanAmount)).to.be.bignumber.equal('0');
});

it('token mismatch', async function () {
await expectRevert(this.token.flashFee(ZERO_ADDRESS, loanAmount), 'ERC20FlashMint: wrong token');
});
});

describe('flashLoan', function () {
Amxx marked this conversation as resolved.
Show resolved Hide resolved
it('success', async function () {
const receiver = await ERC3156FlashBorrowerMock.new(true, true);
const { tx } = await this.token.flashLoan(receiver.address, this.token.address, loanAmount, '0x');

expectEvent.inTransaction(tx, this.token, 'Transfer', { from: ZERO_ADDRESS, to: receiver.address, value: loanAmount });
expectEvent.inTransaction(tx, this.token, 'Transfer', { from: receiver.address, to: ZERO_ADDRESS, value: loanAmount });
expectEvent.inTransaction(tx, receiver, 'BalanceOf', { token: this.token.address, account: receiver.address, value: loanAmount });
expectEvent.inTransaction(tx, receiver, 'TotalSupply', { token: this.token.address, value: initialSupply.add(loanAmount) });

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

it ('missing return value', async function () {
const receiver = await ERC3156FlashBorrowerMock.new(false, true);
await expectRevert(
this.token.flashLoan(receiver.address, this.token.address, loanAmount, '0x'),
'ERC20FlashMint: invalid return value',
);
});

it ('missing approval', async function () {
const receiver = await ERC3156FlashBorrowerMock.new(true, false);
await expectRevert(
this.token.flashLoan(receiver.address, this.token.address, loanAmount, '0x'),
'ERC20FlashMint: allowance does not allow refund',
);
});

it ('unavailable funds', async function () {
const receiver = await ERC3156FlashBorrowerMock.new(true, true);
const data = this.token.contract.methods.transfer(other, 10).encodeABI();
await expectRevert(
this.token.flashLoan(receiver.address, this.token.address, loanAmount, data),
'ERC20: burn amount exceeds balance',
);
});

it ('more then maxFlashLoan', async function () {
const receiver = await ERC3156FlashBorrowerMock.new(true, true);
const data = this.token.contract.methods.transfer(other, 10).encodeABI();
// _mint overflow reverts using a panic code. No reason string.
await expectRevert.unspecified(this.token.flashLoan(receiver.address, this.token.address, MAX_UINT256, data));
});
});
});