From 9d06a1b64bcff51349d832222c165b03f9c1f316 Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 23 Feb 2023 08:52:10 -0300 Subject: [PATCH 1/9] Remove unused Solhint overrides (#4069) --- contracts/utils/cryptography/EIP712.sol | 3 --- 1 file changed, 3 deletions(-) diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index d0e52c39658..e06d0066b1a 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -38,7 +38,6 @@ abstract contract EIP712 is IERC5267 { bytes32 private constant _TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); - /* solhint-disable var-name-mixedcase */ // Cache the domain separator as an immutable value, but also store the chain id that it corresponds to, in order to // invalidate the cached domain separator if the chain id changes. bytes32 private immutable _cachedDomainSeparator; @@ -53,8 +52,6 @@ abstract contract EIP712 is IERC5267 { bytes32 private immutable _hashedName; bytes32 private immutable _hashedVersion; - /* solhint-enable var-name-mixedcase */ - /** * @dev Initializes the domain separator and parameter caches. * From a6b8366980d8b28cbe4be7f1798719f0fac4cac1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 23 Feb 2023 21:06:47 +0100 Subject: [PATCH 2/9] Improve ERC4626 event coverage (#4072) --- test/token/ERC20/extensions/ERC4626.test.js | 106 +++++++++++++++++++- 1 file changed, 101 insertions(+), 5 deletions(-) diff --git a/test/token/ERC20/extensions/ERC4626.test.js b/test/token/ERC20/extensions/ERC4626.test.js index 0c67a2fc7a9..2f7b99d38ae 100644 --- a/test/token/ERC20/extensions/ERC4626.test.js +++ b/test/token/ERC20/extensions/ERC4626.test.js @@ -66,6 +66,13 @@ contract('ERC4626', function (accounts) { to: recipient, value: parseShare(1), }); + + await expectEvent.inTransaction(tx, this.vault, 'Deposit', { + sender: holder, + owner: recipient, + assets: parseToken(1), + shares: parseShare(1), + }); }); it('mint', async function () { @@ -85,6 +92,13 @@ contract('ERC4626', function (accounts) { to: recipient, value: parseShare(1), }); + + await expectEvent.inTransaction(tx, this.vault, 'Deposit', { + sender: holder, + owner: recipient, + assets: parseToken(1), + shares: parseShare(1), + }); }); it('withdraw', async function () { @@ -104,6 +118,14 @@ contract('ERC4626', function (accounts) { to: constants.ZERO_ADDRESS, value: '0', }); + + await expectEvent.inTransaction(tx, this.vault, 'Withdraw', { + sender: holder, + receiver: recipient, + owner: holder, + assets: '0', + shares: '0', + }); }); it('redeem', async function () { @@ -123,6 +145,14 @@ contract('ERC4626', function (accounts) { to: constants.ZERO_ADDRESS, value: '0', }); + + await expectEvent.inTransaction(tx, this.vault, 'Withdraw', { + sender: holder, + receiver: recipient, + owner: holder, + assets: '0', + shares: '0', + }); }); }); @@ -171,6 +201,13 @@ contract('ERC4626', function (accounts) { to: recipient, value: expectedShares, }); + + await expectEvent.inTransaction(tx, this.vault, 'Deposit', { + sender: holder, + owner: recipient, + assets: depositAssets, + shares: expectedShares, + }); }); /** @@ -207,6 +244,13 @@ contract('ERC4626', function (accounts) { to: recipient, value: mintShares, }); + + await expectEvent.inTransaction(tx, this.vault, 'Deposit', { + sender: holder, + owner: recipient, + assets: expectedAssets, + shares: mintShares, + }); }); it('withdraw', async function () { @@ -226,6 +270,14 @@ contract('ERC4626', function (accounts) { to: constants.ZERO_ADDRESS, value: '0', }); + + await expectEvent.inTransaction(tx, this.vault, 'Withdraw', { + sender: holder, + receiver: recipient, + owner: holder, + assets: '0', + shares: '0', + }); }); it('redeem', async function () { @@ -245,6 +297,14 @@ contract('ERC4626', function (accounts) { to: constants.ZERO_ADDRESS, value: '0', }); + + await expectEvent.inTransaction(tx, this.vault, 'Withdraw', { + sender: holder, + receiver: recipient, + owner: holder, + assets: '0', + shares: '0', + }); }); }); @@ -292,6 +352,13 @@ contract('ERC4626', function (accounts) { to: recipient, value: expectedShares, }); + + await expectEvent.inTransaction(tx, this.vault, 'Deposit', { + sender: holder, + owner: recipient, + assets: depositAssets, + shares: expectedShares, + }); }); /** @@ -326,6 +393,13 @@ contract('ERC4626', function (accounts) { to: recipient, value: mintShares, }); + + await expectEvent.inTransaction(tx, this.vault, 'Deposit', { + sender: holder, + owner: recipient, + assets: expectedAssets, + shares: mintShares, + }); }); it('withdraw', async function () { @@ -351,6 +425,14 @@ contract('ERC4626', function (accounts) { to: constants.ZERO_ADDRESS, value: expectedShares, }); + + await expectEvent.inTransaction(tx, this.vault, 'Withdraw', { + sender: holder, + receiver: recipient, + owner: holder, + assets: withdrawAssets, + shares: expectedShares, + }); }); it('withdraw with approval', async function () { @@ -363,21 +445,35 @@ contract('ERC4626', function (accounts) { }); it('redeem', async function () { - expect(await this.vault.maxRedeem(holder)).to.be.bignumber.equal(parseShare(100)); - expect(await this.vault.previewRedeem(parseShare(100))).to.be.bignumber.equal(parseToken(1)); + const effectiveAssets = await this.vault.totalAssets().then(x => x.add(virtualAssets)); + const effectiveShares = await this.vault.totalSupply().then(x => x.add(virtualShares)); + + const redeemShares = parseShare(100); + const expectedAssets = redeemShares.mul(effectiveAssets).div(effectiveShares); - const { tx } = await this.vault.redeem(parseShare(100), recipient, holder, { from: holder }); + expect(await this.vault.maxRedeem(holder)).to.be.bignumber.equal(redeemShares); + expect(await this.vault.previewRedeem(redeemShares)).to.be.bignumber.equal(expectedAssets); + + const { tx } = await this.vault.redeem(redeemShares, recipient, holder, { from: holder }); await expectEvent.inTransaction(tx, this.token, 'Transfer', { from: this.vault.address, to: recipient, - value: parseToken(1), + value: expectedAssets, }); await expectEvent.inTransaction(tx, this.vault, 'Transfer', { from: holder, to: constants.ZERO_ADDRESS, - value: parseShare(100), + value: redeemShares, + }); + + await expectEvent.inTransaction(tx, this.vault, 'Withdraw', { + sender: holder, + receiver: recipient, + owner: holder, + assets: expectedAssets, + shares: redeemShares, }); }); From 6e88df28cb45712790c02b7aef7a866a3b9c5512 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 24 Feb 2023 00:06:22 +0100 Subject: [PATCH 3/9] Mark ERC777 and ERC1820 as deprecated (#4066) --- CHANGELOG.md | 2 ++ contracts/token/ERC777/ERC777.sol | 2 ++ contracts/token/ERC777/README.adoc | 2 ++ contracts/utils/introspection/ERC1820Implementer.sol | 2 ++ docs/modules/ROOT/pages/erc777.adoc | 2 ++ 5 files changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ad77fe681d..e882fd5167c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ - `ERC20Permit`: Added the file `IERC20Permit.sol` and `ERC20Permit.sol` and deprecated `draft-IERC20Permit.sol` and `draft-ERC20Permit.sol` since [EIP-2612](https://eips.ethereum.org/EIPS/eip-2612) is no longer a Draft. Developers are encouraged to update their imports. ([#3793](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3793)) - `Timers`: The `Timers` library is now deprecated and will be removed in the next major release. ([#4062](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4062)) +- `ERC777`: The `ERC777` token standard is no longer supported by OpenZeppelin. Our implementation is now deprecated and will be removed in the next major release. The corresponding standard interfaces remain available. ([#4066](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4066)) +- `ERC1820Implementer`: The `ERC1820` pseudo-introspection mechanism is no longer supported by OpenZeppelin. Our implementation is now deprecated and will be removed in the next major release. The corresponding standard interfaces remain available. ([#4066](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4066)) ## 4.8.1 (2023-01-12) diff --git a/contracts/token/ERC777/ERC777.sol b/contracts/token/ERC777/ERC777.sol index c1503c4dfc3..0e5af5d089f 100644 --- a/contracts/token/ERC777/ERC777.sol +++ b/contracts/token/ERC777/ERC777.sol @@ -25,6 +25,8 @@ import "../../utils/introspection/IERC1820Registry.sol"; * Additionally, the {IERC777-granularity} value is hard-coded to `1`, meaning that there * are no special restrictions in the amount of tokens that created, moved, or * destroyed. This makes integration with ERC20 applications seamless. + * + * CAUTION: This file is deprecated as of v4.9 and will be removed in the next major release. */ contract ERC777 is Context, IERC777, IERC20 { using Address for address; diff --git a/contracts/token/ERC777/README.adoc b/contracts/token/ERC777/README.adoc index 5012a31101f..ac326abe698 100644 --- a/contracts/token/ERC777/README.adoc +++ b/contracts/token/ERC777/README.adoc @@ -3,6 +3,8 @@ [.readme-notice] NOTE: This document is better viewed at https://docs.openzeppelin.com/contracts/api/token/erc777 +CAUTION: As of v4.9, OpenZeppelin's implementation of ERC-777 is deprecated and will be removed in the next major release. + This set of interfaces and contracts are all related to the https://eips.ethereum.org/EIPS/eip-777[ERC777 token standard]. TIP: For an overview of ERC777 tokens and a walk through on how to create a token contract read our xref:ROOT:erc777.adoc[ERC777 guide]. diff --git a/contracts/utils/introspection/ERC1820Implementer.sol b/contracts/utils/introspection/ERC1820Implementer.sol index ac5a884c02a..cf4b50498d4 100644 --- a/contracts/utils/introspection/ERC1820Implementer.sol +++ b/contracts/utils/introspection/ERC1820Implementer.sol @@ -12,6 +12,8 @@ import "./IERC1820Implementer.sol"; * declare their willingness to be implementers. * {IERC1820Registry-setInterfaceImplementer} should then be called for the * registration to be complete. + * + * CAUTION: This file is deprecated as of v4.9 and will be removed in the next major release. */ contract ERC1820Implementer is IERC1820Implementer { bytes32 private constant _ERC1820_ACCEPT_MAGIC = keccak256("ERC1820_ACCEPT_MAGIC"); diff --git a/docs/modules/ROOT/pages/erc777.adoc b/docs/modules/ROOT/pages/erc777.adoc index d79fbee28eb..4a0af16c396 100644 --- a/docs/modules/ROOT/pages/erc777.adoc +++ b/docs/modules/ROOT/pages/erc777.adoc @@ -1,5 +1,7 @@ = ERC777 +CAUTION: As of v4.9, OpenZeppelin's implementation of ERC-777 is deprecated and will be removed in the next major release. + Like xref:erc20.adoc[ERC20], ERC777 is a standard for xref:tokens.adoc#different-kinds-of-tokens[_fungible_ tokens], and is focused around allowing more complex interactions when trading tokens. More generally, it brings tokens and Ether closer together by providing the equivalent of a `msg.value` field, but for tokens. The standard also brings multiple quality-of-life improvements, such as getting rid of the confusion around `decimals`, minting and burning with proper events, among others, but its killer feature is *receive hooks*. A hook is simply a function in a contract that is called when tokens are sent to it, meaning *accounts and contracts can react to receiving tokens*. From 62dbb1b06a5e86ef72fb9e0716d37fd20dbe522d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 24 Feb 2023 15:48:30 +0100 Subject: [PATCH 4/9] Document clock modes for token and governor (#4058) Co-authored-by: Francisco --- contracts/interfaces/README.adoc | 13 ++- docs/modules/ROOT/pages/governance.adoc | 121 ++++++++++++++++++++++-- 2 files changed, 125 insertions(+), 9 deletions(-) diff --git a/contracts/interfaces/README.adoc b/contracts/interfaces/README.adoc index 5b4cedf9543..8748da8a46f 100644 --- a/contracts/interfaces/README.adoc +++ b/contracts/interfaces/README.adoc @@ -30,6 +30,11 @@ are useful to interact with third party contracts that implement them. - {IERC3156FlashLender} - {IERC3156FlashBorrower} - {IERC4626} +- {IERC4906} +- {IERC5267} +- {IERC5313} +- {IERC5805} +- {IERC6372} == Detailed ABI @@ -53,4 +58,10 @@ are useful to interact with third party contracts that implement them. {{IERC3156FlashBorrower}} -{{IERC4626}} \ No newline at end of file +{{IERC4626}} + +{{IERC5267}} + +{{IERC5805}} + +{{IERC6372}} diff --git a/docs/modules/ROOT/pages/governance.adoc b/docs/modules/ROOT/pages/governance.adoc index eb76321bfd8..cd7dd4e6f20 100644 --- a/docs/modules/ROOT/pages/governance.adoc +++ b/docs/modules/ROOT/pages/governance.adoc @@ -119,13 +119,15 @@ contract MyToken is ERC20, ERC20Permit, ERC20Votes, ERC20Wrapper { } ``` -NOTE:The only other source of voting power available in OpenZeppelin Contracts currently is xref:api:token/ERC721.adoc#ERC721Votes[`ERC721Votes`]. ERC721 tokens that don't provide this functionality can be wrapped into a voting tokens using a combination of xref:api:token/ERC721.adoc#ERC721Votes[`ERC721Votes`] and xref:api:token/ERC721Wrapper.adoc#ERC721Wrapper[`ERC721Wrapper`]. +NOTE: The only other source of voting power available in OpenZeppelin Contracts currently is xref:api:token/ERC721.adoc#ERC721Votes[`ERC721Votes`]. ERC721 tokens that don't provide this functionality can be wrapped into a voting tokens using a combination of xref:api:token/ERC721.adoc#ERC721Votes[`ERC721Votes`] and xref:api:token/ERC721Wrapper.adoc#ERC721Wrapper[`ERC721Wrapper`]. + +NOTE: The internal clock used by the token to store voting balances will dictate the operating mode of the Governor contract attached to it. By default, block numbers are used. Since v4.9, developers can override the xref:api:interfaces.adoc#IERC6372[IERC6372] clock to use timestamps instead of block numbers. === Governor Initially, we will build a Governor without a timelock. The core logic is given by the Governor contract, but we still need to choose: 1) how voting power is determined, 2) how many votes are needed for quorum, 3) what options people have when casting a vote and how those votes are counted, and 4) what type of token should be used to vote. Each of these aspects is customizable by writing your own module, or more easily choosing one from OpenZeppelin Contracts. -For 1) we will use the GovernorVotes module, which hooks to an IVotes instance to determine the voting power of an account based on the token balance they hold when a proposal becomes active. This module requires as a constructor parameter the address of the token. +For 1) we will use the GovernorVotes module, which hooks to an IVotes instance to determine the voting power of an account based on the token balance they hold when a proposal becomes active. This module requires as a constructor parameter the address of the token. This module also discovers the clock mode (ERC6372) used by the token and applies it to the Governor. For 2) we will use GovernorVotesQuorumFraction which works together with ERC20Votes to define quorum as a percentage of the total supply at the block a proposal’s voting power is retrieved. This requires a constructor parameter to set the percentage. Most Governors nowadays use 4%, so we will initialize the module with parameter 4 (this indicates the percentage, resulting in 4%). @@ -137,7 +139,7 @@ votingDelay: How long after a proposal is created should voting power be fixed. votingPeriod: How long does a proposal remain open to votes. -These parameters are specified in number of blocks. Assuming block time of around 12 seconds, we will set votingDelay = 1 day = 7200 blocks, and votingPeriod = 1 week = 50400 blocks. +These parameters are specified in the unit defined in the token's clock. Assuming the token uses block numbers, and assuming block time of around 12 seconds, we will have set votingDelay = 1 day = 7200 blocks, and votingPeriod = 1 week = 50400 blocks. We can optionally set a proposal threshold as well. This restricts proposal creation to accounts who have enough voting power. @@ -160,11 +162,11 @@ contract MyGovernor is Governor, GovernorCompatibilityBravo, GovernorVotes, Gove {} function votingDelay() public pure override returns (uint256) { - return 6575; // 1 day + return 7200; // 1 day } function votingPeriod() public pure override returns (uint256) { - return 46027; // 1 week + return 50400; // 1 week } function proposalThreshold() public pure override returns (uint256) { @@ -223,7 +225,6 @@ contract MyGovernor is Governor, GovernorCompatibilityBravo, GovernorVotes, Gove return super.supportsInterface(interfaceId); } } - ``` === Timelock @@ -261,7 +262,7 @@ const grantAmount = ...; const transferCalldata = token.interface.encodeFunctionData(‘transfer’, [teamAddress, grantAmount]); ``` -Now we are ready to call the propose function of the governor. Note that we don’t pass in one array of actions, but instead three arrays corresponding to the list of targets, the list of values, and the list of calldatas. In this case it’s a single action, so it’s simple: +Now we are ready to call the propose function of the Governor. Note that we don’t pass in one array of actions, but instead three arrays corresponding to the list of targets, the list of values, and the list of calldatas. In this case it’s a single action, so it’s simple: ```javascript await governor.propose( @@ -305,7 +306,7 @@ await governor.queue( ); ``` -This will cause the governor to interact with the timelock contract and queue the actions for execution after the required delay. +This will cause the Governor to interact with the timelock contract and queue the actions for execution after the required delay. After enough time has passed (according to the timelock parameters), the proposal can be executed. If there was no timelock to begin with, this step can be ran immediately after the proposal succeeds. @@ -319,3 +320,107 @@ await governor.execute( ``` Executing the proposal will transfer the ERC20 tokens to the chosen recipient. To wrap up: we set up a system where a treasury is controlled by the collective decision of the token holders of a project, and all actions are executed via proposals enforced by on-chain votes. + +== Timestamp based governance + +=== Motivation + +It is sometimes difficult to deal with durations expressed in number of blocks because of inconsistent or unpredictable time between blocks. This is particularly true of some L2 networks where blocks are produced based on blockchain usage. Using number of blocks can also lead to the governance rules being affected by network upgrades that modify the expected time between blocks. + +The difficulty of replacing block numbers with timestamps is that the Governor and the token must both use the same format when querying past votes. If a token is designed around block numbers, it is not possible for a Governor to reliably do timestamp based lookups. + +Therefore, designing a timestamp based voting system starts with the token. + +=== Token + +Since v4.9, all voting contracts (including xref:api:token/ERC20.adoc#ERC20Votes[`ERC20Votes`] and xref:api:token/ERC721.adoc#ERC721Votes[`ERC721Votes`]) rely on xref:api:interfaces.adoc#IERC6372[IERC6372] for clock management. In order to change from operating with block numbers to operating with timestamps, all that is required is to override the `clock()` and `CLOCK_MODE()` functions. + +```solidity +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.2; + +import "github.com/openzeppelin/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol"; +import "github.com/openzeppelin/openzeppelin-contracts/contracts/token/ERC20/extensions/ERC20Permit.sol"; +import "github.com/openzeppelin/openzeppelin-contracts/contracts/token/ERC20/extensions/ERC20Votes.sol"; + +contract MyToken is ERC20, ERC20Permit, ERC20Votes { + constructor() ERC20("MyToken", "MTK") ERC20Permit("MyToken") {} + + // Overrides IERC6372 functions to make the token & governor timestamp-based + + function clock() public view override returns (uint48) { + return uint48(block.timestamp); + } + + function CLOCK_MODE() public pure override returns (string memory) { + return "mode=timestamp"; + } + + // The functions below are overrides required by Solidity. + + function _afterTokenTransfer(address from, address to, uint256 amount) + internal + override(ERC20, ERC20Votes) + { + super._afterTokenTransfer(from, to, amount); + } + + function _mint(address to, uint256 amount) + internal + override(ERC20, ERC20Votes) + { + super._mint(to, amount); + } + + function _burn(address account, uint256 amount) + internal + override(ERC20, ERC20Votes) + { + super._burn(account, amount); + } +} +``` + +=== Governor + +The Governor will automatically detect the clock mode used by the token and adapt to it. There is no need to override anything in the Governor contract. However, the clock mode does affect how some values are interpreted. It is therefore necessary to set the `votingDelay()` and `votingPeriod()` accordingly. + +```solidity +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.2; + +import "@openzeppelin/contracts/governance/Governor.sol"; +import "@openzeppelin/contracts/governance/compatibility/GovernorCompatibilityBravo.sol"; +import "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol"; +import "@openzeppelin/contracts/governance/extensions/GovernorVotesQuorumFraction.sol"; +import "@openzeppelin/contracts/governance/extensions/GovernorTimelockControl.sol"; + +contract MyGovernor is Governor, GovernorCompatibilityBravo, GovernorVotes, GovernorVotesQuorumFraction, GovernorTimelockControl { + constructor(IVotes _token, TimelockController _timelock) + Governor("MyGovernor") + GovernorVotes(_token) + GovernorVotesQuorumFraction(4) + GovernorTimelockControl(_timelock) + {} + + function votingDelay() public pure virtual override returns (uint256) { + return 1 days; + } + + function votingPeriod() public pure virtual override returns (uint256) { + return 1 weeks; + } + + function proposalThreshold() public pure virtual override returns (uint256) { + return 0; + } + + // ... +} +``` + +=== Disclaimer + +Timestamp based voting is a recent feature that was formalized in EIP-6372 and EIP-5805, and introduced in v4.9. At the time this feature is released, governance tooling such as https://www.tally.xyz[Tally] does not support it yet. While support for timestamps should come soon, users can expect invalid reporting of deadlines & durations. This invalid reporting by offchain tools does not affect the onchain security and functionality of the governance contract. + +Governors with timestamp support (v4.9 and above) are compatible with old tokens (before v4.9) and will operate in "block number" mode (which is the mode all old tokens operate on). On the other hand, old Governor instances (before v4.9) are not compatible with new tokens operating using timestamps. If you update your token code to use timestamps, make sure to also update your Governor code. From d5581531deabf016552705eac9234a5d4c2935b1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 24 Feb 2023 15:49:10 +0100 Subject: [PATCH 5/9] Add a "fees" section to the ERC4626 guide (#4054) Co-authored-by: Francisco Giordano --- contracts/mocks/docs/ERC4626Fees.sol | 87 ++++++++++++++ contracts/mocks/token/ERC4646FeesMock.sol | 40 ++++++ docs/modules/ROOT/pages/erc4626.adoc | 22 ++++ scripts/prepare-docs.sh | 8 ++ test/token/ERC20/extensions/ERC4626.test.js | 127 ++++++++++++++++++++ 5 files changed, 284 insertions(+) create mode 100644 contracts/mocks/docs/ERC4626Fees.sol create mode 100644 contracts/mocks/token/ERC4646FeesMock.sol diff --git a/contracts/mocks/docs/ERC4626Fees.sol b/contracts/mocks/docs/ERC4626Fees.sol new file mode 100644 index 00000000000..8ff162953e9 --- /dev/null +++ b/contracts/mocks/docs/ERC4626Fees.sol @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../../token/ERC20/extensions/ERC4626.sol"; + +abstract contract ERC4626Fees is ERC4626 { + using Math for uint256; + + /** @dev See {IERC4626-previewDeposit}. */ + function previewDeposit(uint256 assets) public view virtual override returns (uint256) { + uint256 fee = _feeOnTotal(assets, _entryFeeBasePoint()); + return super.previewDeposit(assets - fee); + } + + /** @dev See {IERC4626-previewMint}. */ + function previewMint(uint256 shares) public view virtual override returns (uint256) { + uint256 assets = super.previewMint(shares); + return assets + _feeOnRaw(assets, _entryFeeBasePoint()); + } + + /** @dev See {IERC4626-previewWithdraw}. */ + function previewWithdraw(uint256 assets) public view virtual override returns (uint256) { + uint256 fee = _feeOnRaw(assets, _exitFeeBasePoint()); + return super.previewWithdraw(assets + fee); + } + + /** @dev See {IERC4626-previewRedeem}. */ + function previewRedeem(uint256 shares) public view virtual override returns (uint256) { + uint256 assets = super.previewRedeem(shares); + return assets - _feeOnTotal(assets, _exitFeeBasePoint()); + } + + /** @dev See {IERC4626-_deposit}. */ + function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal virtual override { + uint256 fee = _feeOnTotal(assets, _entryFeeBasePoint()); + address recipient = _entryFeeRecipient(); + + super._deposit(caller, receiver, assets, shares); + + if (fee > 0 && recipient != address(this)) { + SafeERC20.safeTransfer(IERC20(asset()), recipient, fee); + } + } + + /** @dev See {IERC4626-_deposit}. */ + function _withdraw( + address caller, + address receiver, + address owner, + uint256 assets, + uint256 shares + ) internal virtual override { + uint256 fee = _feeOnRaw(assets, _exitFeeBasePoint()); + address recipient = _exitFeeRecipient(); + + super._withdraw(caller, receiver, owner, assets, shares); + + if (fee > 0 && recipient != address(this)) { + SafeERC20.safeTransfer(IERC20(asset()), recipient, fee); + } + } + + function _entryFeeBasePoint() internal view virtual returns (uint256) { + return 0; + } + + function _entryFeeRecipient() internal view virtual returns (address) { + return address(0); + } + + function _exitFeeBasePoint() internal view virtual returns (uint256) { + return 0; + } + + function _exitFeeRecipient() internal view virtual returns (address) { + return address(0); + } + + function _feeOnRaw(uint256 assets, uint256 feeBasePoint) private pure returns (uint256) { + return assets.mulDiv(feeBasePoint, 1e5, Math.Rounding.Up); + } + + function _feeOnTotal(uint256 assets, uint256 feeBasePoint) private pure returns (uint256) { + return assets.mulDiv(feeBasePoint, feeBasePoint + 1e5, Math.Rounding.Up); + } +} diff --git a/contracts/mocks/token/ERC4646FeesMock.sol b/contracts/mocks/token/ERC4646FeesMock.sol new file mode 100644 index 00000000000..cd8f4101245 --- /dev/null +++ b/contracts/mocks/token/ERC4646FeesMock.sol @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../docs/ERC4626Fees.sol"; + +abstract contract ERC4626FeesMock is ERC4626Fees { + uint256 private immutable _entryFeeBasePointValue; + address private immutable _entryFeeRecipientValue; + uint256 private immutable _exitFeeBasePointValue; + address private immutable _exitFeeRecipientValue; + + constructor( + uint256 entryFeeBasePoint, + address entryFeeRecipient, + uint256 exitFeeBasePoint, + address exitFeeRecipient + ) { + _entryFeeBasePointValue = entryFeeBasePoint; + _entryFeeRecipientValue = entryFeeRecipient; + _exitFeeBasePointValue = exitFeeBasePoint; + _exitFeeRecipientValue = exitFeeRecipient; + } + + function _entryFeeBasePoint() internal view virtual override returns (uint256) { + return _entryFeeBasePointValue; + } + + function _entryFeeRecipient() internal view virtual override returns (address) { + return _entryFeeRecipientValue; + } + + function _exitFeeBasePoint() internal view virtual override returns (uint256) { + return _exitFeeBasePointValue; + } + + function _exitFeeRecipient() internal view virtual override returns (address) { + return _exitFeeRecipientValue; + } +} diff --git a/docs/modules/ROOT/pages/erc4626.adoc b/docs/modules/ROOT/pages/erc4626.adoc index 11c13ca9581..00d46a4d05d 100644 --- a/docs/modules/ROOT/pages/erc4626.adoc +++ b/docs/modules/ROOT/pages/erc4626.adoc @@ -191,3 +191,25 @@ stem:[\delta = 3], stem:[a_0 = 100], stem:[a_1 = 10^5] image::erc4626-attack-6.png[Inflation attack without offset=6] stem:[\delta = 6], stem:[a_0 = 1], stem:[a_1 = 10^5] + + +[[fees]] +== Custom behavior: Adding fees to the vault + +In an ERC4626 vaults, fees can be captured during the deposit/mint and/or during the withdraw/redeem steps. In both cases it is essential to remain compliant with the ERC4626 requirements with regard to the preview functions. + +For example, if calling `deposit(100, receiver)`, the caller should deposit exactly 100 underlying tokens, including fees, and the receiver should receive a number of shares that matches the value returned by `previewDeposit(100)`. Similarly, `previewMint` should account for the fees that the user will have to pay on top of share's cost. + +As for the `Deposit` event, while this is less clear in the EIP spec itself, there seems to be consensus that it should include the number of assets paid for by the user, including the fees. + +On the other hand, when withdrawing assets, the number given by the user should correspond to what he receives. Any fees should be added to the quote (in shares) performed by `previewWithdraw`. + +The `Withdraw` event should include the number of shares the user burns (including fees) and the number of assets the user actually receives (after fees are deducted). + +The consequence of this design is that both the `Deposit` and `Withdraw` events will describe two exchange rates. The spread between the "Buy-in" and the "Exit" prices correspond to the fees taken by the vault. + +The following example describes how fees proportional to the deposited/withdrawn amount can be implemented: + +```solidity +include::api:example$ERC4626Fees.sol[] +``` diff --git a/scripts/prepare-docs.sh b/scripts/prepare-docs.sh index 4fc0c957a0b..bb9c5d0ad25 100755 --- a/scripts/prepare-docs.sh +++ b/scripts/prepare-docs.sh @@ -12,4 +12,12 @@ rm -rf "$OUTDIR" hardhat docgen +# copy examples and adjust imports +examples_dir="docs/modules/api/examples" +mkdir -p "$examples_dir" +for f in contracts/mocks/docs/*.sol; do + name="$(basename "$f")" + sed -e '/^import/s|\.\./\.\./|@openzeppelin/contracts/|' "$f" > "docs/modules/api/examples/$name" +done + node scripts/gen-nav.js "$OUTDIR" > "$OUTDIR/../nav.adoc" diff --git a/test/token/ERC20/extensions/ERC4626.test.js b/test/token/ERC20/extensions/ERC4626.test.js index 2f7b99d38ae..c9e5a4098c7 100644 --- a/test/token/ERC20/extensions/ERC4626.test.js +++ b/test/token/ERC20/extensions/ERC4626.test.js @@ -4,6 +4,7 @@ const { expect } = require('chai'); const ERC20Decimals = artifacts.require('$ERC20DecimalsMock'); const ERC4626 = artifacts.require('$ERC4626'); const ERC4626OffsetMock = artifacts.require('$ERC4626OffsetMock'); +const ERC4626FeesMock = artifacts.require('$ERC4626FeesMock'); contract('ERC4626', function (accounts) { const [holder, recipient, spender, other, user1, user2] = accounts; @@ -489,6 +490,132 @@ contract('ERC4626', function (accounts) { }); } + describe('ERC4626Fees', function () { + const feeBasePoint = web3.utils.toBN(5e3); + const amountWithoutFees = web3.utils.toBN(10000); + const fees = amountWithoutFees.mul(feeBasePoint).divn(1e5); + const amountWithFees = amountWithoutFees.add(fees); + + describe('input fees', function () { + beforeEach(async function () { + this.token = await ERC20Decimals.new(name, symbol, 18); + this.vault = await ERC4626FeesMock.new( + name + ' Vault', + symbol + 'V', + this.token.address, + feeBasePoint, + other, + 0, + constants.ZERO_ADDRESS, + ); + + await this.token.$_mint(holder, constants.MAX_INT256); + await this.token.approve(this.vault.address, constants.MAX_INT256, { from: holder }); + }); + + it('deposit', async function () { + expect(await this.vault.previewDeposit(amountWithFees)).to.be.bignumber.equal(amountWithoutFees); + ({ tx: this.tx } = await this.vault.deposit(amountWithFees, recipient, { from: holder })); + }); + + it('mint', async function () { + expect(await this.vault.previewMint(amountWithoutFees)).to.be.bignumber.equal(amountWithFees); + ({ tx: this.tx } = await this.vault.mint(amountWithoutFees, recipient, { from: holder })); + }); + + afterEach(async function () { + // get total + await expectEvent.inTransaction(this.tx, this.token, 'Transfer', { + from: holder, + to: this.vault.address, + value: amountWithFees, + }); + + // redirect fees + await expectEvent.inTransaction(this.tx, this.token, 'Transfer', { + from: this.vault.address, + to: other, + value: fees, + }); + + // mint shares + await expectEvent.inTransaction(this.tx, this.vault, 'Transfer', { + from: constants.ZERO_ADDRESS, + to: recipient, + value: amountWithoutFees, + }); + + // deposit event + await expectEvent.inTransaction(this.tx, this.vault, 'Deposit', { + sender: holder, + owner: recipient, + assets: amountWithFees, + shares: amountWithoutFees, + }); + }); + }); + + describe('output fees', function () { + beforeEach(async function () { + this.token = await ERC20Decimals.new(name, symbol, 18); + this.vault = await ERC4626FeesMock.new( + name + ' Vault', + symbol + 'V', + this.token.address, + 0, + constants.ZERO_ADDRESS, + 5e3, // 5% + other, + ); + + await this.token.$_mint(this.vault.address, constants.MAX_INT256); + await this.vault.$_mint(holder, constants.MAX_INT256); + }); + + it('redeem', async function () { + expect(await this.vault.previewRedeem(amountWithFees)).to.be.bignumber.equal(amountWithoutFees); + ({ tx: this.tx } = await this.vault.redeem(amountWithFees, recipient, holder, { from: holder })); + }); + + it('withdraw', async function () { + expect(await this.vault.previewWithdraw(amountWithoutFees)).to.be.bignumber.equal(amountWithFees); + ({ tx: this.tx } = await this.vault.withdraw(amountWithoutFees, recipient, holder, { from: holder })); + }); + + afterEach(async function () { + // withdraw principal + await expectEvent.inTransaction(this.tx, this.token, 'Transfer', { + from: this.vault.address, + to: recipient, + value: amountWithoutFees, + }); + + // redirect fees + await expectEvent.inTransaction(this.tx, this.token, 'Transfer', { + from: this.vault.address, + to: other, + value: fees, + }); + + // mint shares + await expectEvent.inTransaction(this.tx, this.vault, 'Transfer', { + from: holder, + to: constants.ZERO_ADDRESS, + value: amountWithFees, + }); + + // withdraw event + await expectEvent.inTransaction(this.tx, this.vault, 'Withdraw', { + sender: holder, + receiver: recipient, + owner: holder, + assets: amountWithoutFees, + shares: amountWithFees, + }); + }); + }); + }); + /// Scenario inspired by solmate ERC4626 tests: /// https://github.com/transmissions11/solmate/blob/main/src/test/ERC4626.t.sol it('multiple mint, deposit, redeem & withdrawal', async function () { From b4d765b130d7306fd496a4c449b6eae403173be8 Mon Sep 17 00:00:00 2001 From: Harshit sharma <79695575+HarshitSharma007@users.noreply.github.com> Date: Fri, 24 Feb 2023 20:35:46 +0530 Subject: [PATCH 6/9] Allow return data length >= 32 in SignatureChecker (#4038) Co-authored-by: Francisco Giordano --- .changeset/warm-masks-obey.md | 5 +++++ contracts/utils/cryptography/SignatureChecker.sol | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .changeset/warm-masks-obey.md diff --git a/.changeset/warm-masks-obey.md b/.changeset/warm-masks-obey.md new file mode 100644 index 00000000000..3bcfa9bdd84 --- /dev/null +++ b/.changeset/warm-masks-obey.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`SignatureChecker`: Allow return data length greater than 32 from EIP-1271 signers. diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index c3a724dd046..b81cf40be03 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -44,7 +44,7 @@ library SignatureChecker { abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature) ); return (success && - result.length == 32 && + result.length >= 32 && abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector)); } } From 8a43ebac28a3e3ae6870387a473582064473cef1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 24 Feb 2023 17:48:55 +0100 Subject: [PATCH 7/9] Document "duplicate" proposal storing in GovernorCompatibilityBravo (#4073) Co-authored-by: Francisco --- .../compatibility/GovernorCompatibilityBravo.sol | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 39c895bf092..25f40440359 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -55,6 +55,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp bytes[] memory calldatas, string memory description ) public virtual override(IGovernor, Governor) returns (uint256) { + // Stores the proposal details (if not already present) and executes the propose logic from the core. _storeProposal(_msgSender(), targets, values, new string[](calldatas.length), calldatas, description); return super.propose(targets, values, calldatas, description); } @@ -69,6 +70,10 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp bytes[] memory calldatas, string memory description ) public virtual override returns (uint256) { + // Stores the full proposal and fallback to the public (possibly overridden) propose. The fallback is done + // after the full proposal is stored, so the store operation included in the fallback will be skipped. Here we + // call `propose` and not `super.propose` to make sure if a child contract override `propose`, whatever code + // is added their is also executed when calling this alternative interface. _storeProposal(_msgSender(), targets, values, signatures, calldatas, description); return propose(targets, values, _encodeCalldata(signatures, calldatas), description); } @@ -174,7 +179,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp } /** - * @dev Store proposal metadata for later lookup + * @dev Store proposal metadata (if not already present) for later lookup. */ function _storeProposal( address proposer, From eb38c059d81ecf9296ef7526233ed962aba070eb Mon Sep 17 00:00:00 2001 From: Pascal Marco Caversaccio Date: Fri, 24 Feb 2023 19:34:03 +0100 Subject: [PATCH 8/9] Add comment on unchecked arithmetic (division by zero) in `Math.sol` (#4050) Co-authored-by: Francisco --- contracts/utils/math/Math.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 8400d066949..f8e7ca0a95c 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -67,6 +67,9 @@ library Math { // Handle non-overflow cases, 256 by 256 division. if (prod1 == 0) { + // Solidity will revert if denominator == 0, unlike the div opcode on its own. + // The surrounding unchecked block does not change this fact. + // See https://docs.soliditylang.org/en/latest/control-structures.html#checked-or-unchecked-arithmetic. return prod0 / denominator; } From 2c6ef8c875c8b307d3e715cb918acc5c2c42714e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 24 Feb 2023 20:14:49 +0100 Subject: [PATCH 9/9] Fix ERC1363 interfaceId (#4074) --- contracts/interfaces/IERC1363.sol | 11 +++-------- contracts/interfaces/README.adoc | 4 ++++ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/contracts/interfaces/IERC1363.sol b/contracts/interfaces/IERC1363.sol index 1a8dc79f438..1517197fe40 100644 --- a/contracts/interfaces/IERC1363.sol +++ b/contracts/interfaces/IERC1363.sol @@ -8,17 +8,12 @@ import "./IERC165.sol"; interface IERC1363 is IERC165, IERC20 { /* - * Note: the ERC-165 identifier for this interface is 0x4bbee2df. - * 0x4bbee2df === + * Note: the ERC-165 identifier for this interface is 0xb0202a11. + * 0xb0202a11 === * bytes4(keccak256('transferAndCall(address,uint256)')) ^ * bytes4(keccak256('transferAndCall(address,uint256,bytes)')) ^ * bytes4(keccak256('transferFromAndCall(address,address,uint256)')) ^ - * bytes4(keccak256('transferFromAndCall(address,address,uint256,bytes)')) - */ - - /* - * Note: the ERC-165 identifier for this interface is 0xfb9ec8ce. - * 0xfb9ec8ce === + * bytes4(keccak256('transferFromAndCall(address,address,uint256,bytes)')) ^ * bytes4(keccak256('approveAndCall(address,uint256)')) ^ * bytes4(keccak256('approveAndCall(address,uint256,bytes)')) */ diff --git a/contracts/interfaces/README.adoc b/contracts/interfaces/README.adoc index 8748da8a46f..710c078e862 100644 --- a/contracts/interfaces/README.adoc +++ b/contracts/interfaces/README.adoc @@ -22,6 +22,8 @@ are useful to interact with third party contracts that implement them. - {IERC1155MetadataURI} - {IERC1271} - {IERC1363} +- {IERC1363Receiver} +- {IERC1363Spender} - {IERC1820Implementer} - {IERC1820Registry} - {IERC1822Proxiable} @@ -44,6 +46,8 @@ are useful to interact with third party contracts that implement them. {{IERC1363Receiver}} +{{IERC1363Spender}} + {{IERC1820Implementer}} {{IERC1820Registry}}