Improve some NatSpec and revert reasons (OpenZeppelin#3809)

Co-authored-by: JulissaDantes <julissadcj@gmail.com>

Author: frangio

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)

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;
     }

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

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.

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

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 () {