diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol index c2b87aa2d..0fd57cecf 100644 --- a/contracts/token/ERC20/extensions/ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/ERC20Votes.sol @@ -271,6 +271,9 @@ abstract contract ERC20Votes is IVotes, ERC20Permit { return a - b; } + /** + * @dev Access an element of the array without performing bounds check. The position is assumed to be within bounds. + */ function _unsafeAccess(Checkpoint[] storage ckpts, uint256 pos) private pure returns (Checkpoint storage result) { assembly { mstore(0, ckpts.slot) diff --git a/contracts/token/ERC20/extensions/ERC4626.sol b/contracts/token/ERC20/extensions/ERC4626.sol index 81b2844ad..3ffcd7bfd 100644 --- a/contracts/token/ERC20/extensions/ERC4626.sol +++ b/contracts/token/ERC20/extensions/ERC4626.sol @@ -17,8 +17,12 @@ import "../../../utils/math/Math.sol"; * the ERC20 standard. Any additional extensions included along it would affect the "shares" token represented by this * contract and not the "assets" token which is an independent contract. * - * CAUTION: Deposits and withdrawals may incur unexpected slippage. Users should verify that the amount received of - * shares or assets is as expected. EOAs should operate through a wrapper that performs these checks such as + * CAUTION: When the vault is empty or nearly empty, deposits are at high risk of being stolen through frontrunning with + * a "donation" to the vault that inflates the price of a share. This is variously known as a donation or inflation + * attack and is essentially a problem of slippage. Vault deployers can protect against this attack by making an initial + * deposit of a non-trivial amount of the asset, such that price manipulation becomes infeasible. Withdrawals may + * similarly be affected by slippage. Users can protect against this attack as well unexpected slippage in general by + * verifying the amount received is as expected, using a wrapper that performs these checks such as * https://github.com/fei-protocol/ERC4626#erc4626router-and-base[ERC4626Router]. * * _Available since v4.7._ @@ -134,7 +138,11 @@ abstract contract ERC4626 is ERC20, IERC4626 { return shares; } - /** @dev See {IERC4626-mint}. */ + /** @dev See {IERC4626-mint}. + * + * As opposed to {deposit}, minting is allowed even if the vault is in a state where the price of a share is zero. + * In this case, the shares will be minted without requiring any assets to be deposited. + */ function mint(uint256 shares, address receiver) public virtual override returns (uint256) { require(shares <= maxMint(receiver), "ERC4626: mint more than max"); @@ -267,6 +275,9 @@ abstract contract ERC4626 is ERC20, IERC4626 { emit Withdraw(caller, receiver, owner, assets, shares); } + /** + * @dev Checks if vault is "healthy" in the sense of having assets backing the circulating shares. + */ function _isVaultHealthy() private view returns (bool) { return totalAssets() > 0 || totalSupply() == 0; } diff --git a/contracts/utils/Checkpoints.sol b/contracts/utils/Checkpoints.sol index 3f329d73c..942acb92e 100644 --- a/contracts/utils/Checkpoints.sol +++ b/contracts/utils/Checkpoints.sol @@ -28,7 +28,8 @@ library Checkpoints { /** * @dev Returns the value at a given block number. If a checkpoint is not available at that block, the closest one - * before it is returned, or zero otherwise. + * before it is returned, or zero otherwise. Because the number returned corresponds to that at the end of the + * block, the requested block number must be in the past, excluding the current block. */ function getAtBlock(History storage self, uint256 blockNumber) internal view returns (uint256) { require(blockNumber < block.number, "Checkpoints: block not yet mined"); @@ -143,8 +144,8 @@ library Checkpoints { // Copying to memory is important here. Checkpoint memory last = _unsafeAccess(self, pos - 1); - // Checkpoints keys must be increasing. - require(last._blockNumber <= key, "Checkpoint: invalid key"); + // Checkpoint keys must be non-decreasing. + require(last._blockNumber <= key, "Checkpoint: decreasing keys"); // Update or push new checkpoint if (last._blockNumber == key) { @@ -205,6 +206,9 @@ library Checkpoints { return high; } + /** + * @dev Access an element of the array without performing bounds check. The position is assumed to be within bounds. + */ function _unsafeAccess(Checkpoint[] storage self, uint256 pos) private pure returns (Checkpoint storage result) { assembly { mstore(0, self.slot) @@ -304,8 +308,8 @@ library Checkpoints { // Copying to memory is important here. Checkpoint224 memory last = _unsafeAccess(self, pos - 1); - // Checkpoints keys must be increasing. - require(last._key <= key, "Checkpoint: invalid key"); + // Checkpoint keys must be non-decreasing. + require(last._key <= key, "Checkpoint: decreasing keys"); // Update or push new checkpoint if (last._key == key) { @@ -366,6 +370,9 @@ library Checkpoints { return high; } + /** + * @dev Access an element of the array without performing bounds check. The position is assumed to be within bounds. + */ function _unsafeAccess(Checkpoint224[] storage self, uint256 pos) private pure @@ -469,8 +476,8 @@ library Checkpoints { // Copying to memory is important here. Checkpoint160 memory last = _unsafeAccess(self, pos - 1); - // Checkpoints keys must be increasing. - require(last._key <= key, "Checkpoint: invalid key"); + // Checkpoint keys must be non-decreasing. + require(last._key <= key, "Checkpoint: decreasing keys"); // Update or push new checkpoint if (last._key == key) { @@ -531,6 +538,9 @@ library Checkpoints { return high; } + /** + * @dev Access an element of the array without performing bounds check. The position is assumed to be within bounds. + */ function _unsafeAccess(Checkpoint160[] storage self, uint256 pos) private pure diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 733656af4..0ed5460e0 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -75,7 +75,7 @@ library Math { } // Make sure the result is less than 2^256. Also prevents denominator == 0. - require(denominator > prod1); + require(denominator > prod1, "Math: mulDiv overflow"); /////////////////////////////////////////////// // 512 by 256 division. diff --git a/scripts/generate/templates/Checkpoints.js b/scripts/generate/templates/Checkpoints.js index b78ed5ceb..858ddb4aa 100644 --- a/scripts/generate/templates/Checkpoints.js +++ b/scripts/generate/templates/Checkpoints.js @@ -67,7 +67,8 @@ function upperLookup(${opts.historyTypeName} storage self, ${opts.keyTypeName} k const legacyOperations = opts => `\ /** * @dev Returns the value at a given block number. If a checkpoint is not available at that block, the closest one - * before it is returned, or zero otherwise. + * before it is returned, or zero otherwise. Because the number returned corresponds to that at the end of the + * block, the requested block number must be in the past, excluding the current block. */ function getAtBlock(${opts.historyTypeName} storage self, uint256 blockNumber) internal view returns (uint256) { require(blockNumber < block.number, "Checkpoints: block not yet mined"); @@ -184,8 +185,8 @@ function _insert( // Copying to memory is important here. ${opts.checkpointTypeName} memory last = _unsafeAccess(self, pos - 1); - // Checkpoints keys must be increasing. - require(last.${opts.keyFieldName} <= key, "Checkpoint: invalid key"); + // Checkpoint keys must be non-decreasing. + require(last.${opts.keyFieldName} <= key, "Checkpoint: decreasing keys"); // Update or push new checkpoint if (last.${opts.keyFieldName} == key) { @@ -246,6 +247,9 @@ function _lowerBinaryLookup( return high; } +/** + * @dev Access an element of the array without performing bounds check. The position is assumed to be within bounds. + */ function _unsafeAccess(${opts.checkpointTypeName}[] storage self, uint256 pos) private pure diff --git a/test/utils/Checkpoints.test.js b/test/utils/Checkpoints.test.js index 9bdb4aef2..87335da9b 100644 --- a/test/utils/Checkpoints.test.js +++ b/test/utils/Checkpoints.test.js @@ -151,7 +151,7 @@ contract('Checkpoints', function (accounts) { }); it('cannot push values in the past', async function () { - await expectRevert(this.contract.push(last(this.checkpoints).key - 1, '0'), 'Checkpoint: invalid key'); + await expectRevert(this.contract.push(last(this.checkpoints).key - 1, '0'), 'Checkpoint: decreasing keys'); }); it('can update last value', async function () {