Update and clarify documentation comments (#5206)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> Co-authored-by: Ernesto García <ernestognw@gmail.com>
OpenZeppelin · Sep 23, 2024 · 2f0bc58 · 2f0bc58
1 parent e866815
commit 2f0bc58
Show file tree

Hide file tree

Showing 8 changed files with 77 additions and 71 deletions.
diff --git a/contracts/finance/VestingWalletCliff.sol b/contracts/finance/VestingWalletCliff.sol
@@ -17,8 +17,7 @@ abstract contract VestingWalletCliff is VestingWallet {
     error InvalidCliffDuration(uint64 cliffSeconds, uint64 durationSeconds);
 
     /**
-     * @dev Sets the sender as the initial owner, the beneficiary as the pending owner, the start timestamp, the
-     * vesting duration and the duration of the cliff of the vesting wallet.
+     * @dev Set the start timestamp of the vesting wallet cliff.
      */
     constructor(uint64 cliffSeconds) {
         if (cliffSeconds > duration()) {

diff --git a/contracts/governance/extensions/GovernorCountingFractional.sol b/contracts/governance/extensions/GovernorCountingFractional.sol
@@ -145,7 +145,7 @@ abstract contract GovernorCountingFractional is Governor {
         uint256 againstVotes = 0;
         uint256 forVotes = 0;
         uint256 abstainVotes = 0;
-        uint256 usedWeight;
+        uint256 usedWeight = 0;
 
         // For clarity of event indexing, fractional voting must be clearly advertised in the "support" field.
         //

diff --git a/contracts/interfaces/IERC1363Spender.sol b/contracts/interfaces/IERC1363Spender.sol
@@ -4,7 +4,7 @@
 pragma solidity ^0.8.20;
 
 /**
- * @title ERC1363Spender
+ * @title IERC1363Spender
  * @dev Interface for any contract that wants to support `approveAndCall`
  * from ERC-1363 token contracts.
  */

diff --git a/contracts/utils/cryptography/P256.sol b/contracts/utils/cryptography/P256.sol
@@ -142,7 +142,7 @@ library P256 {
 
     /**
      * @dev Checks if (x, y) are valid coordinates of a point on the curve.
-     * In particular this function checks that x <= P and y <= P.
+     * In particular this function checks that x < P and y < P.
      */
     function isValidPublicKey(bytes32 x, bytes32 y) internal pure returns (bool result) {
         assembly ("memory-safe") {
@@ -239,7 +239,7 @@ library P256 {
     }
 
     /**
-     * @dev Compute P·u1 + Q·u2 using the precomputed points for P and Q (see {_preComputeJacobianPoints}).
+     * @dev Compute G·u1 + P·u2 using the precomputed points for G and P (see {_preComputeJacobianPoints}).
      *
      * Uses Strauss Shamir trick for EC multiplication
      * https://stackoverflow.com/questions/50993471/ec-scalar-multiplication-with-strauss-shamir-method
@@ -292,17 +292,17 @@ library P256 {
         points[0x04] = JPoint(GX, GY, 1); // 0,1 (g)
         points[0x02] = _jDoublePoint(points[0x01]); // 2,0 (2p)
         points[0x08] = _jDoublePoint(points[0x04]); // 0,2 (2g)
-        points[0x03] = _jAddPoint(points[0x01], points[0x02]); // 3,0 (3p)
+        points[0x03] = _jAddPoint(points[0x01], points[0x02]); // 3,0 (p+2p = 3p)
         points[0x05] = _jAddPoint(points[0x01], points[0x04]); // 1,1 (p+g)
         points[0x06] = _jAddPoint(points[0x02], points[0x04]); // 2,1 (2p+g)
         points[0x07] = _jAddPoint(points[0x03], points[0x04]); // 3,1 (3p+g)
         points[0x09] = _jAddPoint(points[0x01], points[0x08]); // 1,2 (p+2g)
         points[0x0a] = _jAddPoint(points[0x02], points[0x08]); // 2,2 (2p+2g)
         points[0x0b] = _jAddPoint(points[0x03], points[0x08]); // 3,2 (3p+2g)
-        points[0x0c] = _jAddPoint(points[0x04], points[0x08]); // 0,3 (g+2g)
+        points[0x0c] = _jAddPoint(points[0x04], points[0x08]); // 0,3 (g+2g = 3g)
         points[0x0d] = _jAddPoint(points[0x01], points[0x0c]); // 1,3 (p+3g)
         points[0x0e] = _jAddPoint(points[0x02], points[0x0c]); // 2,3 (2p+3g)
-        points[0x0f] = _jAddPoint(points[0x03], points[0x0C]); // 3,3 (3p+3g)
+        points[0x0f] = _jAddPoint(points[0x03], points[0x0c]); // 3,3 (3p+3g)
     }
 
     function _jAddPoint(JPoint memory p1, JPoint memory p2) private pure returns (JPoint memory) {

diff --git a/contracts/utils/cryptography/RSA.sol b/contracts/utils/cryptography/RSA.sol
@@ -14,23 +14,24 @@ import {Math} from "../math/Math.sol";
  */
 library RSA {
     /**
-     * @dev Same as {pkcs1} but using SHA256 to calculate the digest of `data`.
+     * @dev Same as {pkcs1Sha256} but using SHA256 to calculate the digest of `data`.
      */
     function pkcs1Sha256(
         bytes memory data,
         bytes memory s,
         bytes memory e,
         bytes memory n
     ) internal view returns (bool) {
-        return pkcs1(sha256(data), s, e, n);
+        return pkcs1Sha256(sha256(data), s, e, n);
     }
 
     /**
      * @dev Verifies a PKCSv1.5 signature given a digest according to the verification
-     * method described in https://datatracker.ietf.org/doc/html/rfc8017#section-8.2.2[section 8.2.2 of RFC8017].
+     * method described in https://datatracker.ietf.org/doc/html/rfc8017#section-8.2.2[section 8.2.2 of RFC8017] with support
+     * for explicit or implicit NULL parameters in the DigestInfo (no other optional parameters are supported).
      *
-     * IMPORTANT: Although this function allows for it, using n of length 1024 bits is considered unsafe.
-     * Consider using at least 2048 bits.
+     * IMPORTANT: For security reason, this function requires the signature and modulus to have a length of at least 2048 bits.
+     * If you use a smaller key, consider replacing it with a larger, more secure, one.
      *
      * WARNING: PKCS#1 v1.5 allows for replayability given the message may contain arbitrary optional parameters in the
      * DigestInfo. Consider using an onchain nonce or unique identifier to include in the message to prevent replay attacks.
@@ -40,12 +41,12 @@ library RSA {
      * @param e is the exponent of the public key
      * @param n is the modulus of the public key
      */
-    function pkcs1(bytes32 digest, bytes memory s, bytes memory e, bytes memory n) internal view returns (bool) {
+    function pkcs1Sha256(bytes32 digest, bytes memory s, bytes memory e, bytes memory n) internal view returns (bool) {
         unchecked {
             // cache and check length
             uint256 length = n.length;
             if (
-                length < 0x40 || // PKCS#1 padding is slightly less than 0x40 bytes at the bare minimum
+                length < 0x100 || // Enforce 2048 bits minimum
                 length != s.length // signature must have the same length as the finite field
             ) {
                 return false;
@@ -94,13 +95,13 @@ library RSA {
             // it should be at 32 (digest) + 2 bytes from the end. To those 34 bytes, we add the
             // OID (9 bytes) and its length (2 bytes) to get the position of the DigestInfo sequence,
             // which is expected to have a length of 0x31 when the NULL param is present or 0x2f if not.
-            if (bytes1(_unsafeReadBytes32(buffer, length - 50)) == 0x31) {
+            if (bytes1(_unsafeReadBytes32(buffer, length - 0x32)) == 0x31) {
                 offset = 0x34;
                 // 00 (1 byte) | SEQUENCE length (0x31) = 3031 (2 bytes) | SEQUENCE length (0x0d) = 300d (2 bytes) | OBJECT_IDENTIFIER length (0x09) = 0609 (2 bytes)
                 // SHA256 OID = 608648016503040201 (9 bytes) | NULL = 0500 (2 bytes) (explicit) | OCTET_STRING length (0x20) = 0420 (2 bytes)
                 params = 0x003031300d060960864801650304020105000420000000000000000000000000;
                 mask = 0xffffffffffffffffffffffffffffffffffffffff000000000000000000000000; // (20 bytes)
-            } else if (bytes1(_unsafeReadBytes32(buffer, length - 48)) == 0x2F) {
+            } else if (bytes1(_unsafeReadBytes32(buffer, length - 0x30)) == 0x2F) {
                 offset = 0x32;
                 // 00 (1 byte) | SEQUENCE length (0x2f) = 302f (2 bytes) | SEQUENCE length (0x0b) = 300b (2 bytes) | OBJECT_IDENTIFIER length (0x09) = 0609 (2 bytes)
                 // SHA256 OID = 608648016503040201 (9 bytes) | NULL = <implicit> | OCTET_STRING length (0x20) = 0420 (2 bytes)
@@ -111,7 +112,7 @@ library RSA {
                 return false;
             }
 
-            // Length is at least 0x40 and offset is at most 0x34, so this is safe. There is always some padding.
+            // Length is at least 0x100 and offset is at most 0x34, so this is safe. There is always some padding.
             uint256 paddingEnd = length - offset;
 
             // The padding has variable (arbitrary) length, so we check it byte per byte in a loop.
@@ -137,7 +138,7 @@ library RSA {
     /// @dev Reads a bytes32 from a bytes array without bounds checking.
     function _unsafeReadBytes32(bytes memory array, uint256 offset) private pure returns (bytes32 result) {
         // Memory safeness is guaranteed as long as the provided `array` is a Solidity-allocated bytes array
-        // and `offset` is within bounds. This is the case for all calls to this private function from {pkcs1}.
+        // and `offset` is within bounds. This is the case for all calls to this private function from {pkcs1Sha256}.
         assembly ("memory-safe") {
             result := mload(add(add(array, 0x20), offset))
         }

diff --git a/contracts/utils/structs/CircularBuffer.sol b/contracts/utils/structs/CircularBuffer.sol
@@ -49,8 +49,9 @@ library CircularBuffer {
      * directly. Use the functions provided below instead. Modifying the struct manually may violate assumptions and
      * lead to unexpected behavior.
      *
-     * The last item is at data[(index - 1) % data.length] and the last item is at data[index % data.length]. This
-     * range can wrap around.
+     * In a full buffer:
+     * - The most recently pushed item (last) is at data[(index - 1) % data.length]
+     * - The oldest item (first) is at data[index % data.length]
      */
     struct Bytes32CircularBuffer {
         uint256 _count;

diff --git a/docs/modules/ROOT/pages/utilities.adoc b/docs/modules/ROOT/pages/utilities.adoc
@@ -91,7 +91,7 @@ function _verify(
     bytes memory e,
     bytes memory n
 ) internal pure returns (bool) {
-    return data.pkcs1(signature, e, n);
+    return data.pkcs1Sha256(signature, e, n);
 }
 ----
 

diff --git a/test/utils/cryptography/RSA.test.js b/test/utils/cryptography/RSA.test.js
@@ -1,6 +1,7 @@
 const { ethers } = require('hardhat');
 const { expect } = require('chai');
 const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
+const { bytes, bytes32 } = ethers.Typed;
 
 const parse = require('./RSA.helper');
 
@@ -24,7 +25,7 @@ describe('RSA', function () {
       // const { sha224, sha256 } = require('@noble/hashes/sha256');
       // const { sha384, sha512 } = require('@noble/hashes/sha512');
 
-      if (test.SHAAlg === 'SHA256') {
+      if (test.SHAAlg === 'SHA256' && length >= 0x100) {
         const result = test.Result === 'P';
 
         it(`signature length ${length} ${test.extra} ${result ? 'works' : 'fails'}`, async function () {
@@ -33,65 +34,69 @@ describe('RSA', function () {
           const exp = '0x' + test.e;
           const mod = '0x' + test.n;
 
-          expect(await this.mock.$pkcs1(ethers.sha256(data), sig, exp, mod)).to.equal(result);
-          expect(await this.mock.$pkcs1Sha256(data, sig, exp, mod)).to.equal(result);
+          expect(await this.mock.$pkcs1Sha256(bytes32(ethers.sha256(data)), sig, exp, mod)).to.equal(result);
+          expect(await this.mock.$pkcs1Sha256(bytes(data), sig, exp, mod)).to.equal(result);
         });
       }
     }
   });
 
   describe('others tests', function () {
-    it('openssl', async function () {
-      const data = ethers.toUtf8Bytes('hello world');
-      const sig =
-        '0x079bed733b48d69bdb03076cb17d9809072a5a765460bc72072d687dba492afe951d75b814f561f253ee5cc0f3d703b6eab5b5df635b03a5437c0a5c179309812f5b5c97650361c645bc99f806054de21eb187bc0a704ed38d3d4c2871a117c19b6da7e9a3d808481c46b22652d15b899ad3792da5419e50ee38759560002388';
-      const exp =
-        '0x0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010001';
-      const mod =
-        '0xdf3edde009b96bc5b03b48bd73fe70a3ad20eaf624d0dc1ba121a45cc739893741b7cf82acf1c91573ec8266538997c6699760148de57e54983191eca0176f518e547b85fe0bb7d9e150df19eee734cf5338219c7f8f7b13b39f5384179f62c135e544cb70be7505751f34568e06981095aeec4f3a887639718a3e11d48c240d';
-      expect(await this.mock.$pkcs1Sha256(data, sig, exp, mod)).to.be.true;
-    });
+    // > openssl genrsa -out private.pem 2048
+    // > openssl rsa -in private.pem -outform der -pubout -out public.pem
+    // > openssl asn1parse -in public.pem -inform DER -strparse 19
+    // > echo -n 'hello world!' | openssl dgst -sha256 -sign private.pem | xxd -p | tr -d \\n
+    const openssl = {
+      descr: 'openssl',
+      data: ethers.toUtf8Bytes('hello world!'),
+      sig: '0x2ff4349940bf0db9bce422e316ac47e3d24b0a869acb05c9c46f74e17491177698b150f2a5996a6bf7d7c73e05af91ad78632115a7d95b823c462596486e56e8473b75a270ca4760cd83f244d5d3af81d2c7d188879abbc2992b22d51e22ffb725f0828c852ee44f81def383e0f92ebfa3c6d97ca5e52a4254f9a886680e3fb394c2a8a955849313dce2cb416f8a67974effd9a17d229146ce10a98684fb3d46a1e53ddaf831cdd2beed895532533c554ae087b2738a5c4cf0802e8062b2a599fd76d67b92eabffa8a92b24e08fbc866217502a4a3d9f6157e491bede3c1048fa8f2d804f66128e8a883018b0ec33a59e1086bf71ae5dc193d9815ca82892dbc',
+      exp: '0x010001',
+      mod: '0xDC1CE5F7B202464CD320B4F9E44FEE0A358BE7022AB155A5BDEE45B1AED3C5A19645D898E294CBA96EAD6929FD8FB4B23E9ADB4D3143A736232C32A8617A77B89F7D8399B9BE37F8349D111067F71D2F20237B9F1A7C1CF44819F9FA5AA030F563DCFB1CC59FFAA86BA2ABEE28D949FED0DF34071B7558950079E28CD9BBA4CAC2F0F86D7BBFB13363C792B5A70C9B279F0B43A264A7CB1A7C7C41FC6EC1D1C1125A6BECE3207AE582F74CE896B9AC18DB00C8985B70145217B831CC313FC06581E186BF70A2EEE2C3C065B5C91A89B2C099B4924CDBF5707D161BD83AC8D9FCA309AC75D63EACF21027C2C9C9F05994331CBDFDD24F9BC6C8B58D8F1824540B',
+      result: true,
+    };
 
     // According to RFC4055, pg.5 and RFC8017, pg. 64, for SHA-1, and the SHA-2 family,
     // the algorithm parameter has to be NULL and both explicit NULL parameter and implicit
     // NULL parameter (ie, absent NULL parameter) are considered to be legal and equivalent.
-    it('rfc8017 implicit null parameter', async function () {
-      const data = ethers.toUtf8Bytes('hello world!');
-      const sig =
-        '0xa0073057133ff3758e7e111b4d7441f1d8cbe4b2dd5ee4316a14264290dee5ed7f175716639bd9bb43a14e4f9fcb9e84dedd35e2205caac04828b2c053f68176d971ea88534dd2eeec903043c3469fc69c206b2a8694fd262488441ed8852280c3d4994e9d42bd1d575c7024095f1a20665925c2175e089c0d731471f6cc145404edf5559fd2276e45e448086f71c78d0cc6628fad394a34e51e8c10bc39bfe09ed2f5f742cc68bee899d0a41e4c75b7b80afd1c321d89ccd9fe8197c44624d91cc935dfa48de3c201099b5b417be748aef29248527e8bbb173cab76b48478d4177b338fe1f1244e64d7d23f07add560d5ad50b68d6649a49d7bc3db686daaa7';
-      const exp = '0x03';
-      const mod =
-        '0xe932ac92252f585b3a80a4dd76a897c8b7652952fe788f6ec8dd640587a1ee5647670a8ad4c2be0f9fa6e49c605adf77b5174230af7bd50e5d6d6d6d28ccf0a886a514cc72e51d209cc772a52ef419f6a953f3135929588ebe9b351fca61ced78f346fe00dbb6306e5c2a4c6dfc3779af85ab417371cf34d8387b9b30ae46d7a5ff5a655b8d8455f1b94ae736989d60a6f2fd5cadbffbd504c5a756a2e6bb5cecc13bca7503f6df8b52ace5c410997e98809db4dc30d943de4e812a47553dce54844a78e36401d13f77dc650619fed88d8b3926e3d8e319c80c744779ac5d6abe252896950917476ece5e8fc27d5f053d6018d91b502c4787558a002b9283da7';
-      expect(await this.mock.$pkcs1Sha256(data, sig, exp, mod)).to.be.true;
-    });
+    const rfc4055 = {
+      descr: 'rfc8017 implicit null parameter',
+      data: ethers.toUtf8Bytes('hello world!'),
+      sig: '0xa0073057133ff3758e7e111b4d7441f1d8cbe4b2dd5ee4316a14264290dee5ed7f175716639bd9bb43a14e4f9fcb9e84dedd35e2205caac04828b2c053f68176d971ea88534dd2eeec903043c3469fc69c206b2a8694fd262488441ed8852280c3d4994e9d42bd1d575c7024095f1a20665925c2175e089c0d731471f6cc145404edf5559fd2276e45e448086f71c78d0cc6628fad394a34e51e8c10bc39bfe09ed2f5f742cc68bee899d0a41e4c75b7b80afd1c321d89ccd9fe8197c44624d91cc935dfa48de3c201099b5b417be748aef29248527e8bbb173cab76b48478d4177b338fe1f1244e64d7d23f07add560d5ad50b68d6649a49d7bc3db686daaa7',
+      exp: '0x03',
+      mod: '0xe932ac92252f585b3a80a4dd76a897c8b7652952fe788f6ec8dd640587a1ee5647670a8ad4c2be0f9fa6e49c605adf77b5174230af7bd50e5d6d6d6d28ccf0a886a514cc72e51d209cc772a52ef419f6a953f3135929588ebe9b351fca61ced78f346fe00dbb6306e5c2a4c6dfc3779af85ab417371cf34d8387b9b30ae46d7a5ff5a655b8d8455f1b94ae736989d60a6f2fd5cadbffbd504c5a756a2e6bb5cecc13bca7503f6df8b52ace5c410997e98809db4dc30d943de4e812a47553dce54844a78e36401d13f77dc650619fed88d8b3926e3d8e319c80c744779ac5d6abe252896950917476ece5e8fc27d5f053d6018d91b502c4787558a002b9283da7',
+      result: true,
+    };
 
-    it('returns false for a very short n', async function () {
-      const data = ethers.toUtf8Bytes('hello world!');
-      const sig = '0x0102';
-      const exp = '0x03';
-      const mod = '0x0405';
-      expect(await this.mock.$pkcs1Sha256(data, sig, exp, mod)).to.be.false;
-    });
+    const shortN = {
+      descr: 'returns false for a very short n',
+      data: ethers.toUtf8Bytes('hello world!'),
+      sig: '0x0102',
+      exp: '0x03',
+      mod: '0x0405',
+      result: false,
+    };
 
-    it('returns false for a signature with different length to n', async function () {
-      const data = ethers.toUtf8Bytes('hello world!');
-      const sig = '0x00112233';
-      const exp = '0x03';
-      const mod =
-        '0xe932ac92252f585b3a80a4dd76a897c8b7652952fe788f6ec8dd640587a1ee5647670a8ad4c2be0f9fa6e49c605adf77b5174230af7bd50e5d6d6d6d28ccf0a886a514cc72e51d209cc772a52ef419f6a953f3135929588ebe9b351fca61ced78f346fe00dbb6306e5c2a4c6dfc3779af85ab417371cf34d8387b9b30ae46d7a5ff5a655b8d8455f1b94ae736989d60a6f2fd5cadbffbd504c5a756a2e6bb5cecc13bca7503f6df8b52ace5c410997e98809db4dc30d943de4e812a47553dce54844a78e36401d13f77dc650619fed88d8b3926e3d8e319c80c744779ac5d6abe252896950917476ece5e8fc27d5f053d6018d91b502c4787558a002b9283da7';
-      expect(await this.mock.$pkcs1Sha256(data, sig, exp, mod)).to.be.false;
-    });
+    const differentLength = {
+      descr: 'returns false for a signature with different length to n',
+      data: ethers.toUtf8Bytes('hello world!'),
+      sig: '0x00112233',
+      exp: '0x03',
+      mod: '0xe932ac92252f585b3a80a4dd76a897c8b7652952fe788f6ec8dd640587a1ee5647670a8ad4c2be0f9fa6e49c605adf77b5174230af7bd50e5d6d6d6d28ccf0a886a514cc72e51d209cc772a52ef419f6a953f3135929588ebe9b351fca61ced78f346fe00dbb6306e5c2a4c6dfc3779af85ab417371cf34d8387b9b30ae46d7a5ff5a655b8d8455f1b94ae736989d60a6f2fd5cadbffbd504c5a756a2e6bb5cecc13bca7503f6df8b52ace5c410997e98809db4dc30d943de4e812a47553dce54844a78e36401d13f77dc650619fed88d8b3926e3d8e319c80c744779ac5d6abe252896950917476ece5e8fc27d5f053d6018d91b502c4787558a002b9283da7',
+      result: false,
+    };
 
-    it('returns false if s >= n', async function () {
-      // this is the openssl example where sig has been replaced by sig + mod
-      const data = ethers.toUtf8Bytes('hello world');
-      const sig =
-        '0xe6dacb53450242618b3e502a257c08acb44b456c7931988da84f0cda8182b435d6d5453ac1e72b07c7dadf2747609b7d544d15f3f14081f9dbad9c48b7aa78d2bdafd81d630f19a0270d7911f4ec82b171e9a95889ffc9e740dc9fac89407a82d152ecb514967d4d9165e67ce0d7f39a3082657cdfca148a5fc2b3a7348c4795';
-      const exp =
-        '0x0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010001';
-      const mod =
-        '0xdf3edde009b96bc5b03b48bd73fe70a3ad20eaf624d0dc1ba121a45cc739893741b7cf82acf1c91573ec8266538997c6699760148de57e54983191eca0176f518e547b85fe0bb7d9e150df19eee734cf5338219c7f8f7b13b39f5384179f62c135e544cb70be7505751f34568e06981095aeec4f3a887639718a3e11d48c240d';
-      expect(await this.mock.$pkcs1Sha256(data, sig, exp, mod)).to.be.false;
-    });
+    // this is the openssl example where sig has been replaced by sig + mod
+    const sTooLarge = {
+      ...openssl,
+      descr: 'returns false if s >= n',
+      sig: ethers.toBeHex(ethers.toBigInt(openssl.sig) + ethers.toBigInt(openssl.mod)),
+      result: false,
+    };
+
+    for (const { descr, data, sig, exp, mod, result } of [openssl, rfc4055, shortN, differentLength, sTooLarge]) {
+      it(descr, async function () {
+        expect(await this.mock.$pkcs1Sha256(bytes(data), sig, exp, mod)).to.equal(result);
+      });
+    }
   });
 });