diff --git a/contracts/governance/audits/OxProtocol-31-03-2023-Pre.pdf b/contracts/governance/audits/OxProtocol-31-03-2023-Pre.pdf new file mode 100644 index 0000000000..77afa4c891 Binary files /dev/null and b/contracts/governance/audits/OxProtocol-31-03-2023-Pre.pdf differ diff --git a/contracts/governance/src/SecurityCouncil.sol b/contracts/governance/src/SecurityCouncil.sol index df62ef6269..b3e7e90f24 100644 --- a/contracts/governance/src/SecurityCouncil.sol +++ b/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"); + } } diff --git a/contracts/governance/src/ZeroExProtocolGovernor.sol b/contracts/governance/src/ZeroExProtocolGovernor.sol index 7fe283b5ed..a1d2664db7 100644 --- a/contracts/governance/src/ZeroExProtocolGovernor.sol +++ b/contracts/governance/src/ZeroExProtocolGovernor.sol @@ -95,9 +95,7 @@ contract ZeroExProtocolGovernor is uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) public { - require(msg.sender == securityCouncil, "ZeroExProtocolGovernor: only security council allowed"); - + ) public onlySecurityCouncil { // Execute the batch of rollbacks via the timelock controller ZeroExTimelock timelockController = ZeroExTimelock(payable(timelock())); timelockController.executeRollbackBatch(targets, values, calldatas, 0, descriptionHash); diff --git a/contracts/governance/src/ZeroExTimelock.sol b/contracts/governance/src/ZeroExTimelock.sol index a7c5c6d273..e21de910fb 100644 --- a/contracts/governance/src/ZeroExTimelock.sol +++ b/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"); diff --git a/contracts/governance/src/ZeroExVotes.sol b/contracts/governance/src/ZeroExVotes.sol index 52689d836d..fbf6ba1c63 100644 --- a/contracts/governance/src/ZeroExVotes.sol +++ b/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; - return pos == 0 ? 0 : _checkpoints[account][pos - 1].votes; + unchecked { + 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; - return pos == 0 ? 0 : _checkpoints[account][pos - 1].quadraticVotes; + unchecked { + 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); - return checkpoint; + if (high != 0) checkpoint = _unsafeAccess(ckpts, high - 1); } 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"); + } } diff --git a/contracts/governance/test/ZeroExProtocolGovernor.t.sol b/contracts/governance/test/ZeroExProtocolGovernor.t.sol index e6c34bcad1..6660a22253 100644 --- a/contracts/governance/test/ZeroExProtocolGovernor.t.sol +++ b/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"))); } }