Address audit findings (5.3 diff audit) (#5584)

Co-authored-by: Arr00 <[email protected]>
Co-authored-by: Ernesto García <[email protected]>

Author: 3 people

contracts/access/manager/AuthorityUtils.sol

@@ -26,6 +26,10 @@ library AuthorityUtils {
             if staticcall(gas(), authority, add(data, 0x20), mload(data), 0x00, 0x40) {
                 immediate := mload(0x00)
                 delay := mload(0x20)
+
+                // If delay does not fit in a uint32, return 0 (no delay)
+                // equivalent to: if gt(delay, 0xFFFFFFFF) { delay := 0 }
+                delay := mul(delay, iszero(shr(32, delay)))
             }
         }
     }

contracts/governance/extensions/GovernorVotesSuperQuorumFraction.sol

@@ -61,6 +61,7 @@ abstract contract GovernorVotesSuperQuorumFraction is GovernorVotesQuorumFractio
 
     /**
      * @dev Returns the super quorum for a `timepoint`, in terms of number of votes: `supply * numerator / denominator`.
+     * See {GovernorSuperQuorum-superQuorum} for more details.
      */
     function superQuorum(uint256 timepoint) public view virtual override returns (uint256) {
         return Math.mulDiv(token().getPastTotalSupply(timepoint), superQuorumNumerator(timepoint), quorumDenominator());

contracts/interfaces/draft-IERC6909.sol

@@ -49,7 +49,8 @@ interface IERC6909 is IERC165 {
     function isOperator(address owner, address spender) external view returns (bool);
 
     /**
-     * @dev Sets an approval to `spender` for `amount` tokens of type `id` from the caller's tokens.
+     * @dev Sets an approval to `spender` for `amount` of tokens of type `id` from the caller's tokens. An `amount` of
+     * `type(uint256).max` signifies an unlimited approval.
      *
      * Must return true.
      */

contracts/mocks/AuthorityMock.sol

@@ -12,7 +12,7 @@ contract NotAuthorityMock is IAuthority {
 }
 
 contract AuthorityNoDelayMock is IAuthority {
-    bool _immediate;
+    bool private _immediate;
 
     function canCall(
         address /* caller */,
@@ -28,22 +28,22 @@ contract AuthorityNoDelayMock is IAuthority {
 }
 
 contract AuthorityDelayMock {
-    bool _immediate;
-    uint32 _delay;
+    bool private _immediate;
+    uint256 private _delay;
 
     function canCall(
         address /* caller */,
         address /* target */,
         bytes4 /* selector */
-    ) external view returns (bool immediate, uint32 delay) {
+    ) external view returns (bool immediate, uint256 delay) {
         return (_immediate, _delay);
     }
 
     function _setImmediate(bool immediate) external {
         _immediate = immediate;
     }
 
-    function _setDelay(uint32 delay) external {
+    function _setDelay(uint256 delay) external {
         _delay = delay;
     }
 }

contracts/token/ERC6909/draft-ERC6909.sol

@@ -79,7 +79,7 @@ contract ERC6909 is Context, ERC165, IERC6909 {
 
     /**
      * @dev Creates `amount` of token `id` and assigns them to `account`, by transferring it from address(0).
-     * Relies on the `_update` mechanism
+     * Relies on the `_update` mechanism.
      *
      * Emits a {Transfer} event with `from` set to the zero address.
      *
@@ -93,10 +93,9 @@ contract ERC6909 is Context, ERC165, IERC6909 {
     }
 
     /**
-     * @dev Moves `amount` of token `id` from `from` to `to` without checking for approvals.
-     *
-     * This internal function is equivalent to {transfer}, and can be used to
-     * e.g. implement automatic token fees, slashing mechanisms, etc.
+     * @dev Moves `amount` of token `id` from `from` to `to` without checking for approvals. This function verifies
+     * that neither the sender nor the receiver are address(0), which means it cannot mint or burn tokens.
+     * Relies on the `_update` mechanism.
      *
      * Emits a {Transfer} event.
      *

contracts/token/ERC6909/extensions/draft-ERC6909TokenSupply.sol

@@ -26,7 +26,7 @@ contract ERC6909TokenSupply is ERC6909, IERC6909TokenSupply {
         }
         if (to == address(0)) {
             unchecked {
-                // amount <= _balances[id][from] <= _totalSupplies[id]
+                // amount <= _balances[from][id] <= _totalSupplies[id]
                 _totalSupplies[id] -= amount;
             }
         }

contracts/utils/Strings.sol

@@ -438,6 +438,10 @@ library Strings {
      * @dev Escape special characters in JSON strings. This can be useful to prevent JSON injection in NFT metadata.
      *
      * WARNING: This function should only be used in double quoted JSON strings. Single quotes are not escaped.
+     *
+     * NOTE: This function escapes all unicode characters, and not just the ones in ranges defined in section 2.5 of
+     * RFC-4627 (U+0000 to U+001F, U+0022 and U+005C). ECMAScript's `JSON.parse` does recover escaped unicode
+     * characters that are not in this range, but other tooling may provide different results.
      */
     function escapeJSON(string memory input) internal pure returns (string memory) {
         bytes memory buffer = bytes(input);

contracts/utils/structs/MerkleTree.sol

@@ -178,7 +178,7 @@ library MerkleTree {
      * root (before the update) and "new" root (after the update). The caller must verify that the reconstructed old
      * root is the last known one.
      *
-     * The `proof` must be an up-to-date inclusion proof for the leaf being update. This means that this function is
+     * The `proof` must be an up-to-date inclusion proof for the leaf being updated. This means that this function is
      * vulnerable to front-running. Any {push} or {update} operation (that changes the root of the tree) would render
      * all "in flight" updates invalid.
      *

test/access/manager/AuthorityUtils.test.js

@@ -2,6 +2,8 @@ const { ethers } = require('hardhat');
 const { expect } = require('chai');
 const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
 
+const { MAX_UINT32, MAX_UINT64 } = require('../../helpers/constants');
+
 async function fixture() {
   const [user, other] = await ethers.getSigners();
 
@@ -70,7 +72,7 @@ describe('AuthorityUtils', function () {
       });
 
       for (const immediate of [true, false]) {
-        for (const delay of [0n, 42n]) {
+        for (const delay of [0n, 42n, MAX_UINT32]) {
           it(`returns (immediate=${immediate}, delay=${delay})`, async function () {
             await this.authority._setImmediate(immediate);
             await this.authority._setDelay(delay);
@@ -80,6 +82,14 @@ describe('AuthorityUtils', function () {
           });
         }
       }
+
+      it('out of bound delay', async function () {
+        await this.authority._setImmediate(false);
+        await this.authority._setDelay(MAX_UINT64); // bigger than the expected uint32
+        const result = await this.mock.$canCallWithDelay(this.authority, this.user, this.other, '0x12345678');
+        expect(result.immediate).to.equal(false);
+        expect(result.delay).to.equal(0n);
+      });
     });
 
     describe('when authority replies with empty data', function () {

test/helpers/constants.js

@@ -1,4 +1,5 @@
 module.exports = {
+  MAX_UINT32: 2n ** 32n - 1n,
   MAX_UINT48: 2n ** 48n - 1n,
   MAX_UINT64: 2n ** 64n - 1n,
 };