OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.97k stars 11.8k forks source link

Add EIP3156 FlashLoan support (ERC20 extension) #2540

Closed Amxx closed 3 years ago

Amxx commented 3 years ago

🧐 Motivation ERC3156 is now final

📝 Details Provide an extension for ERC20 that implements this ERC.

frangio commented 3 years ago

This sounds nice. Have the EIP or an implemenation been audited? Are there production deployments?

Amxx commented 3 years ago

I'm not 100% sure, @albertocuestacanada might be able to tell use more. Maybe yield uses it.

Amxx commented 3 years ago

there is this ERC3156 compatible uniswap wrapper: https://ftmscan.com/address/0xa89a83890cc5d43d710846cefcb4a41007a37347#code

alcueca commented 3 years ago

Hi there!

As all EIPs that make it to Final, ERC3156 has been thoroughly audited pro-bono by an all-star team, including your own Austin Williams.

There are two ERC3156 compliant wrappers in mainnet and fantom, MakerDAO's MIP-25 is compliant, audited, and scheduled to go live soon, and Cover Protocol is also a compliant borrower. WETH10 is scheduled to go live soon and is also a compliant lender.

Yield doesn't use it in v1, but v2 will implement it for sure.

alcueca commented 3 years ago

Please note that the reference implementation is here: https://github.com/albertocuestacanada/ERC3156

Not to be confused with the ERC3156-Wrappers, which are a personal project: https://github.com/albertocuestacanada/ERC3156-Wrappers

Amxx commented 3 years ago

Implementation proposal: https://github.com/Amxx/openzeppelin-contracts/blob/feature/ERC3156/contracts/token/ERC20/extensions/draft-ERC3156.sol

I just wonder why the return value for IERC3156FlashBorrower.onFlashLoan is bytes32(keccak256("ERC3156FlashBorrower.onFlashLoan")) and not bytes4(IERC3156FlashBorrower(0).onFlashLoan.selector) as most other ERC do ...

alcueca commented 3 years ago

The return value is a sentinel to make sure the lender didn't accidentally call the fallback function. bytes32(keccak256("ERC3156FlashBorrower.onFlashLoan")) sounds like a reasonable choice, returning the function signature would be a bit weak. Is returning the signature as a sentinel a thing?

Maybe you should give a hand to @MicahZoltu in reviewing ERCs. That's quite a detail to know about.

Amxx commented 3 years ago

Its true that returning a bytes4 is "weaker". This is the de-facto standard for many eip however (721, 777, 1155, 1271, ...)

MicahZoltu commented 3 years ago

I don't see why the seed for a particular sentinel needs to be standardized? It is just "a well known value" and its actual contents don't matter at all.

Ungolim commented 3 years ago

any updates on this?

Amxx commented 3 years ago

PR #2543 is basically good to go. Should be in 4.1 release.