From 6e4ded027be83a8448242c4ec10c001a954e58b2 Mon Sep 17 00:00:00 2001 From: Aleksandr Tarelkin Date: Wed, 6 Nov 2024 15:34:34 +0300 Subject: [PATCH 1/4] fix: check new member zero address --- contracts/committees/HashConsensus.sol | 4 ++++ test/unit/committees/HashConsensus.t.sol | 9 +++++++++ 2 files changed, 13 insertions(+) diff --git a/contracts/committees/HashConsensus.sol b/contracts/committees/HashConsensus.sol index 1a92dffd..c24bc8f4 100644 --- a/contracts/committees/HashConsensus.sol +++ b/contracts/committees/HashConsensus.sol @@ -22,6 +22,7 @@ abstract contract HashConsensus is Ownable { event TimelockDurationSet(Duration timelockDuration); error DuplicatedMember(address account); + error InvalidMemberAccount(address account); error AccountIsNotMember(address account); error CallerIsNotMember(address caller); error HashAlreadyUsed(bytes32 hash); @@ -205,6 +206,9 @@ abstract contract HashConsensus is Ownable { /// @param executionQuorum The minimum number of members required for executing certain operations. function _addMembers(address[] memory newMembers, uint256 executionQuorum) internal { for (uint256 i = 0; i < newMembers.length; ++i) { + if (newMembers[i] == address(0)) { + revert InvalidMemberAccount(newMembers[i]); + } if (!_members.add(newMembers[i])) { revert DuplicatedMember(newMembers[i]); } diff --git a/test/unit/committees/HashConsensus.t.sol b/test/unit/committees/HashConsensus.t.sol index 7e2eeb91..08b6da55 100644 --- a/test/unit/committees/HashConsensus.t.sol +++ b/test/unit/committees/HashConsensus.t.sol @@ -107,6 +107,15 @@ abstract contract HashConsensusUnitTest is UnitTest { _hashConsensus.addMembers(membersToAdd, _quorum); } + function test_addMembers_RevertOn_ZeroMemberAddress() public { + address[] memory membersToAdd = new address[](1); + membersToAdd[0] = address(0); + + vm.prank(_owner); + vm.expectRevert(abi.encodeWithSignature("InvalidMemberAccount(address)", address(0))); + _hashConsensus.addMembers(membersToAdd, _quorum); + } + function test_addMembers_RevertOn_DuplicateInArray() public { address[] memory membersToAdd = new address[](2); membersToAdd[0] = makeAddr("NEW_MEMBER"); From 83dee8da22695df24a451327576b088866935131 Mon Sep 17 00:00:00 2001 From: Aleksandr Tarelkin Date: Wed, 6 Nov 2024 02:06:11 +0300 Subject: [PATCH 2/4] fix: Tiebreaker committee check for zero sealable --- contracts/committees/TiebreakerCoreCommittee.sol | 11 +++++++++++ contracts/committees/TiebreakerSubCommittee.sol | 7 +++++++ test/unit/committees/TiebreakerCore.t.sol | 6 ++++++ test/unit/committees/TiebreakerSubCommittee.t.sol | 6 ++++++ 4 files changed, 30 insertions(+) diff --git a/contracts/committees/TiebreakerCoreCommittee.sol b/contracts/committees/TiebreakerCoreCommittee.sol index 11a97009..0e7244c5 100644 --- a/contracts/committees/TiebreakerCoreCommittee.sol +++ b/contracts/committees/TiebreakerCoreCommittee.sol @@ -25,6 +25,7 @@ enum ProposalType { contract TiebreakerCoreCommittee is ITiebreakerCoreCommittee, HashConsensus, ProposalsList { error ResumeSealableNonceMismatch(); error ProposalDoesNotExist(uint256 proposalId); + error InvalidSealable(address sealable); address public immutable DUAL_GOVERNANCE; @@ -108,8 +109,18 @@ contract TiebreakerCoreCommittee is ITiebreakerCoreCommittee, HashConsensus, Pro return _sealableResumeNonces[sealable]; } + /// @notice Votes on a proposal to resume a sealable address + /// @dev Allows committee members to vote on resuming a sealable address + /// reverts if the sealable address is the zero address + /// @param sealable The address to resume + /// @param nonce The nonce for the resume proposal function sealableResume(address sealable, uint256 nonce) public { _checkCallerIsMember(); + + if (sealable == address(0)) { + revert InvalidSealable(sealable); + } + if (nonce != _sealableResumeNonces[sealable]) { revert ResumeSealableNonceMismatch(); } diff --git a/contracts/committees/TiebreakerSubCommittee.sol b/contracts/committees/TiebreakerSubCommittee.sol index f69be1b7..e467db37 100644 --- a/contracts/committees/TiebreakerSubCommittee.sol +++ b/contracts/committees/TiebreakerSubCommittee.sol @@ -20,6 +20,8 @@ enum ProposalType { /// @notice This contract allows a subcommittee to vote on and execute proposals for scheduling and resuming sealable addresses /// @dev Inherits from HashConsensus for voting mechanisms and ProposalsList for proposal management contract TiebreakerSubCommittee is HashConsensus, ProposalsList { + error InvalidSealable(address sealable); + address public immutable TIEBREAKER_CORE_COMMITTEE; constructor( @@ -92,9 +94,14 @@ contract TiebreakerSubCommittee is HashConsensus, ProposalsList { /// @notice Votes on a proposal to resume a sealable address /// @dev Allows committee members to vote on resuming a sealable address + /// reverts if the sealable address is the zero address /// @param sealable The address to resume function sealableResume(address sealable) public { _checkCallerIsMember(); + + if (sealable == address(0)) { + revert InvalidSealable(sealable); + } (bytes memory proposalData, bytes32 key,) = _encodeSealableResume(sealable); _vote(key, true); _pushProposal(key, uint256(ProposalType.ResumeSealable), proposalData); diff --git a/test/unit/committees/TiebreakerCore.t.sol b/test/unit/committees/TiebreakerCore.t.sol index e956957c..27de2186 100644 --- a/test/unit/committees/TiebreakerCore.t.sol +++ b/test/unit/committees/TiebreakerCore.t.sol @@ -150,6 +150,12 @@ contract TiebreakerCoreUnitTest is UnitTest { tiebreakerCore.sealableResume(sealable, wrongNonce); } + function test_sealableResume_RevertOn_SealableZeroAddress() external { + vm.prank(committeeMembers[0]); + vm.expectRevert(abi.encodeWithSelector(TiebreakerCoreCommittee.InvalidSealable.selector, address(0))); + tiebreakerCore.sealableResume(address(0), 0); + } + function test_executeSealableResume_HappyPath() external { uint256 nonce = tiebreakerCore.getSealableResumeNonce(sealable); diff --git a/test/unit/committees/TiebreakerSubCommittee.t.sol b/test/unit/committees/TiebreakerSubCommittee.t.sol index 774ae27c..5a6b81a1 100644 --- a/test/unit/committees/TiebreakerSubCommittee.t.sol +++ b/test/unit/committees/TiebreakerSubCommittee.t.sol @@ -145,6 +145,12 @@ contract TiebreakerSubCommitteeUnitTest is UnitTest { tiebreakerSubCommittee.sealableResume(sealable); } + function test_sealableResume_RevertOn_SealableZeroAddress() external { + vm.prank(committeeMembers[0]); + vm.expectRevert(abi.encodeWithSelector(TiebreakerSubCommittee.InvalidSealable.selector, address(0))); + tiebreakerSubCommittee.sealableResume(address(0)); + } + function test_executeSealableResume_HappyPath() external { vm.mockCall( tiebreakerCore, From db05d3ac1c1a49c3c0469be25a70359fa5f1b2d6 Mon Sep 17 00:00:00 2001 From: Aleksandr Tarelkin Date: Mon, 11 Nov 2024 11:43:05 +0300 Subject: [PATCH 3/4] using encodeWithSelector --- test/unit/committees/HashConsensus.t.sol | 34 ++++++++++++------------ 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/test/unit/committees/HashConsensus.t.sol b/test/unit/committees/HashConsensus.t.sol index 08b6da55..8d3b5074 100644 --- a/test/unit/committees/HashConsensus.t.sol +++ b/test/unit/committees/HashConsensus.t.sol @@ -58,7 +58,7 @@ abstract contract HashConsensusUnitTest is UnitTest { function test_constructor_RevertOn_WithZeroQuorum() public { uint256 invalidQuorum = 0; - vm.expectRevert(abi.encodeWithSignature("InvalidQuorum()")); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.InvalidQuorum.selector)); new HashConsensusInstance(_owner, _committeeMembers, invalidQuorum, Durations.from(1)); } @@ -87,12 +87,12 @@ abstract contract HashConsensusUnitTest is UnitTest { assertEq(_hashConsensus.isMember(membersToAdd[0]), false); vm.prank(_stranger); - vm.expectRevert(abi.encodeWithSignature("OwnableUnauthorizedAccount(address)", _stranger)); + vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, _stranger)); _hashConsensus.addMembers(membersToAdd, _quorum); for (uint256 i = 0; i < _membersCount; ++i) { vm.prank(_committeeMembers[i]); - vm.expectRevert(abi.encodeWithSignature("OwnableUnauthorizedAccount(address)", _committeeMembers[i])); + vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, _committeeMembers[i])); _hashConsensus.addMembers(membersToAdd, _quorum); } } @@ -103,7 +103,7 @@ abstract contract HashConsensusUnitTest is UnitTest { assertEq(_hashConsensus.isMember(membersToAdd[0]), true); vm.prank(_owner); - vm.expectRevert(abi.encodeWithSignature("DuplicatedMember(address)", membersToAdd[0])); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.DuplicatedMember.selector, membersToAdd[0])); _hashConsensus.addMembers(membersToAdd, _quorum); } @@ -112,7 +112,7 @@ abstract contract HashConsensusUnitTest is UnitTest { membersToAdd[0] = address(0); vm.prank(_owner); - vm.expectRevert(abi.encodeWithSignature("InvalidMemberAccount(address)", address(0))); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.InvalidMemberAccount.selector, address(0))); _hashConsensus.addMembers(membersToAdd, _quorum); } @@ -123,7 +123,7 @@ abstract contract HashConsensusUnitTest is UnitTest { assertEq(_hashConsensus.isMember(membersToAdd[0]), false); vm.prank(_owner); - vm.expectRevert(abi.encodeWithSignature("DuplicatedMember(address)", membersToAdd[1])); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.DuplicatedMember.selector, membersToAdd[1])); _hashConsensus.addMembers(membersToAdd, _quorum); } @@ -133,11 +133,11 @@ abstract contract HashConsensusUnitTest is UnitTest { assertEq(_hashConsensus.isMember(membersToAdd[0]), false); vm.prank(_owner); - vm.expectRevert(abi.encodeWithSignature("InvalidQuorum()")); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.InvalidQuorum.selector)); _hashConsensus.addMembers(membersToAdd, 0); vm.prank(_owner); - vm.expectRevert(abi.encodeWithSignature("InvalidQuorum()")); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.InvalidQuorum.selector)); _hashConsensus.addMembers(membersToAdd, _membersCount + 2); } @@ -194,12 +194,12 @@ abstract contract HashConsensusUnitTest is UnitTest { assertEq(_hashConsensus.isMember(membersToRemove[0]), true); vm.prank(_stranger); - vm.expectRevert(abi.encodeWithSignature("OwnableUnauthorizedAccount(address)", _stranger)); + vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, _stranger)); _hashConsensus.removeMembers(membersToRemove, _quorum); for (uint256 i = 0; i < _membersCount; ++i) { vm.prank(_committeeMembers[i]); - vm.expectRevert(abi.encodeWithSignature("OwnableUnauthorizedAccount(address)", _committeeMembers[i])); + vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, _committeeMembers[i])); _hashConsensus.removeMembers(membersToRemove, _quorum); } } @@ -232,11 +232,11 @@ abstract contract HashConsensusUnitTest is UnitTest { assertEq(_hashConsensus.isMember(membersToRemove[0]), true); vm.prank(_owner); - vm.expectRevert(abi.encodeWithSignature("InvalidQuorum()")); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.InvalidQuorum.selector)); _hashConsensus.removeMembers(membersToRemove, 0); vm.prank(_owner); - vm.expectRevert(abi.encodeWithSignature("InvalidQuorum()")); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.InvalidQuorum.selector)); _hashConsensus.removeMembers(membersToRemove, _membersCount); } @@ -353,7 +353,7 @@ abstract contract HashConsensusUnitTest is UnitTest { uint256 invalidQuorum = 0; vm.prank(_owner); - vm.expectRevert(abi.encodeWithSignature("InvalidQuorum()")); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.InvalidQuorum.selector)); _hashConsensus.setQuorum(invalidQuorum); } @@ -361,7 +361,7 @@ abstract contract HashConsensusUnitTest is UnitTest { uint256 invalidQuorum = _committeeMembers.length + 1; vm.prank(_owner); - vm.expectRevert(abi.encodeWithSignature("InvalidQuorum()")); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.InvalidQuorum.selector)); _hashConsensus.setQuorum(invalidQuorum); } @@ -371,7 +371,7 @@ abstract contract HashConsensusUnitTest is UnitTest { vm.startPrank(_owner); _hashConsensus.setQuorum(invalidQuorum); - vm.expectRevert(abi.encodeWithSignature("InvalidQuorum()")); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.InvalidQuorum.selector)); _hashConsensus.setQuorum(invalidQuorum); } @@ -600,7 +600,7 @@ contract HashConsensusInternalUnitTest is HashConsensusUnitTest { } vm.prank(_stranger); - vm.expectRevert(abi.encodeWithSignature("TimelockNotPassed()")); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.TimelockNotPassed.selector)); _hashConsensusWrapper.execute(dataHash); _wait(_timelock); @@ -681,7 +681,7 @@ contract HashConsensusInternalUnitTest is HashConsensusUnitTest { (,, Timestamp scheduledAtBefore,) = _hashConsensusWrapper.getHashState(hash); assertEq(scheduledAtBefore, Timestamps.from(0)); - vm.expectRevert(abi.encodeWithSignature("QuorumIsNotReached()")); + vm.expectRevert(abi.encodeWithSelector(HashConsensus.QuorumIsNotReached.selector)); _hashConsensusWrapper.schedule(hash); (,, Timestamp scheduledAtAfter,) = _hashConsensusWrapper.getHashState(hash); From d4f4695baa1eff4e45d3a5329792b1a1bfa8cbed Mon Sep 17 00:00:00 2001 From: Aleksandr Tarelkin Date: Mon, 11 Nov 2024 14:36:16 +0300 Subject: [PATCH 4/4] fix: Reseal committee check for zero sealable --- contracts/committees/ResealCommittee.sol | 7 +++++++ test/unit/committees/ResealCommittee.t.sol | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/contracts/committees/ResealCommittee.sol b/contracts/committees/ResealCommittee.sol index 6f2770ae..4b63950a 100644 --- a/contracts/committees/ResealCommittee.sol +++ b/contracts/committees/ResealCommittee.sol @@ -15,6 +15,8 @@ import {ProposalsList} from "./ProposalsList.sol"; /// @notice This contract allows a committee to vote on and execute resealing proposals /// @dev Inherits from HashConsensus for voting mechanisms and ProposalsList for proposal management contract ResealCommittee is HashConsensus, ProposalsList { + error InvalidSealable(address sealable); + address public immutable DUAL_GOVERNANCE; mapping(bytes32 => uint256) private _resealNonces; @@ -37,6 +39,11 @@ contract ResealCommittee is HashConsensus, ProposalsList { /// @param support Indicates whether the member supports the proposal function voteReseal(address sealable, bool support) public { _checkCallerIsMember(); + + if (sealable == address(0)) { + revert InvalidSealable(sealable); + } + (bytes memory proposalData, bytes32 key) = _encodeResealProposal(sealable); _vote(key, support); _pushProposal(key, 0, proposalData); diff --git a/test/unit/committees/ResealCommittee.t.sol b/test/unit/committees/ResealCommittee.t.sol index effc10c0..fc82a968 100644 --- a/test/unit/committees/ResealCommittee.t.sol +++ b/test/unit/committees/ResealCommittee.t.sol @@ -47,6 +47,12 @@ contract ResealCommitteeUnitTest is UnitTest { assertEq(quorumAt, Timestamp.wrap(uint40(block.timestamp))); } + function test_voteReseal_RevertOn_ZeroAddress() external { + vm.prank(committeeMembers[0]); + vm.expectRevert(abi.encodeWithSelector(ResealCommittee.InvalidSealable.selector, address(0))); + resealCommittee.voteReseal(address(0), true); + } + function testFuzz_voteReseal_RevertOn_NotMember(address caller) external { vm.assume(caller != committeeMembers[0] && caller != committeeMembers[1] && caller != committeeMembers[2]); vm.prank(caller);