Governance audit and fixes (#690)

* Fix F-3: Explicit Variable Return audit report

* Reuse onlySecurityCouncil modifier and update its revert message

* Fix F-5: modifier Optimization audit report

* Fix F-7: Input Sanity Check audit report

* Add audit report

* Further gas optimise getting a checkpoint function by up to 100 gas

* Fix F-6: return Statement Optimization audit report

* Wrap logic in unchecked block to save 24 gas
in getVotes and getQuadraticVotes each

* Move audit doc into a dedicated folder

contracts/governance/src/SecurityCouncil.sol

@@ -26,7 +26,7 @@ abstract contract SecurityCouncil {
     event SecurityCouncilEjected();
 
     modifier onlySecurityCouncil() {
-        require(msg.sender == securityCouncil, "ZeroExProtocolGovernor: only security council allowed");
+        _checkSenderIsSecurityCouncil();
         _;
     }
 
@@ -78,4 +78,8 @@ abstract contract SecurityCouncil {
         if (bytes4(payload) == bytes4(0x2761c3cd)) return true;
         else return false;
     }
+
+    function _checkSenderIsSecurityCouncil() private view {
+        require(msg.sender == securityCouncil, "SecurityCouncil: only security council allowed");
+    }
 }

contracts/governance/src/ZeroExProtocolGovernor.sol

@@ -95,9 +95,7 @@ contract ZeroExProtocolGovernor is
         uint256[] memory values,
         bytes[] memory calldatas,
         bytes32 descriptionHash
-    ) public {
+    ) public onlySecurityCouncil {
-        require(msg.sender == securityCouncil, "ZeroExProtocolGovernor: only security council allowed");
-
         // Execute the batch of rollbacks via the timelock controller
         ZeroExTimelock timelockController = ZeroExTimelock(payable(timelock()));
         timelockController.executeRollbackBatch(targets, values, calldatas, 0, descriptionHash);

contracts/governance/src/ZeroExTimelock.sol

@@ -47,6 +47,7 @@ contract ZeroExTimelock is TimelockController {
         bytes32 predecessor,
         bytes32 salt
     ) public payable onlyRoleOrOpenRole(EXECUTOR_ROLE) {
+        require(targets.length > 0, "ZeroExTimelock: empty targets");
         require(targets.length == values.length, "ZeroExTimelock: length mismatch");
         require(targets.length == payloads.length, "ZeroExTimelock: length mismatch");
 

contracts/governance/src/ZeroExVotes.sol

@@ -44,7 +44,7 @@ contract ZeroExVotes is IZeroExVotes, Initializable, OwnableUpgradeable, UUPSUpg
     function _authorizeUpgrade(address newImplementation) internal override onlyOwner {}
 
     modifier onlyToken() {
-        require(msg.sender == token, "ZeroExVotes: only token allowed");
+        _checkSenderIsToken();
         _;
     }
 
@@ -71,16 +71,20 @@ contract ZeroExVotes is IZeroExVotes, Initializable, OwnableUpgradeable, UUPSUpg
      * @inheritdoc IZeroExVotes
      */
     function getVotes(address account) public view returns (uint256) {
-        uint256 pos = _checkpoints[account].length;
+        unchecked {
-        return pos == 0 ? 0 : _checkpoints[account][pos - 1].votes;
+            uint256 pos = _checkpoints[account].length;
+            return pos == 0 ? 0 : _unsafeAccess(_checkpoints[account], pos - 1).votes;
+        }
     }
 
     /**
      * @inheritdoc IZeroExVotes
      */
     function getQuadraticVotes(address account) public view returns (uint256) {
-        uint256 pos = _checkpoints[account].length;
+        unchecked {
-        return pos == 0 ? 0 : _checkpoints[account][pos - 1].quadraticVotes;
+            uint256 pos = _checkpoints[account].length;
+            return pos == 0 ? 0 : _unsafeAccess(_checkpoints[account], pos - 1).quadraticVotes;
+        }
     }
 
     /**
@@ -213,7 +217,7 @@ contract ZeroExVotes is IZeroExVotes, Initializable, OwnableUpgradeable, UUPSUpg
     function _checkpointsLookup(
         Checkpoint[] storage ckpts,
         uint256 blockNumber
-    ) internal view returns (Checkpoint memory) {
+    ) internal view returns (Checkpoint memory checkpoint) {
         // We run a binary search to look for the earliest checkpoint taken after `blockNumber`.
         //
         // Initially we check if the block is recent to narrow the search range.
@@ -252,8 +256,7 @@ contract ZeroExVotes is IZeroExVotes, Initializable, OwnableUpgradeable, UUPSUpg
 
         // Leaving here for posterity this is the original OZ implementation which we've replaced
         // return high == 0 ? 0 : _unsafeAccess(ckpts, high - 1).votes;
-        Checkpoint memory checkpoint = high == 0 ? Checkpoint(0, 0, 0) : _unsafeAccess(ckpts, high - 1);
+        if (high != 0) checkpoint = _unsafeAccess(ckpts, high - 1);
-        return checkpoint;
     }
 
     function _writeCheckpoint(
@@ -326,4 +329,8 @@ contract ZeroExVotes is IZeroExVotes, Initializable, OwnableUpgradeable, UUPSUpg
             result.slot := add(keccak256(0, 0x20), pos)
         }
     }
+
+    function _checkSenderIsToken() private {
+        require(msg.sender == token, "ZeroExVotes: only token allowed");
+    }
 }

contracts/governance/test/ZeroExProtocolGovernor.t.sol

@@ -161,7 +161,7 @@ contract ZeroExProtocolGovernorTest is ZeroExGovernorBaseTest {
         calldatas[0] = abi.encodeWithSignature("rollback(bytes4,address)", testFunctionSig, testFunctionImpl);
 
         vm.startPrank(account2);
-        vm.expectRevert("ZeroExProtocolGovernor: only security council allowed");
+        vm.expectRevert("SecurityCouncil: only security council allowed");
         protocolGovernor.executeRollback(targets, values, calldatas, keccak256(bytes("Emergency rollback")));
     }
 }