Merge pull request #160 from lidofinance/fix/zero-address-check

Audit fix: Zero address check
lidofinance · Nov 11, 2024 · d8f759b · d8f759b
2 parents cb4312d + d4f4695
commit d8f759b
Show file tree

Hide file tree

Showing 8 changed files with 72 additions and 16 deletions.
diff --git 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/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 hash => uint256 nonce) 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/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
@@ -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/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,16 @@ 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);
+    }
+
+    function test_addMembers_RevertOn_ZeroMemberAddress() public {
+        address[] memory membersToAdd = new address[](1);
+        membersToAdd[0] = address(0);
+
+        vm.prank(_owner);
+        vm.expectRevert(abi.encodeWithSelector(HashConsensus.InvalidMemberAccount.selector, address(0)));
         _hashConsensus.addMembers(membersToAdd, _quorum);
     }
 
@@ -114,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);
     }
 
@@ -124,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);
     }
 
@@ -185,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);
         }
     }
@@ -223,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);
     }
 
@@ -344,15 +353,15 @@ 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);
     }
 
     function test_setQuorum_RevertOn_IfQuorumExceedsMembers() public {
         uint256 invalidQuorum = _committeeMembers.length + 1;
 
         vm.prank(_owner);
-        vm.expectRevert(abi.encodeWithSignature("InvalidQuorum()"));
+        vm.expectRevert(abi.encodeWithSelector(HashConsensus.InvalidQuorum.selector));
         _hashConsensus.setQuorum(invalidQuorum);
     }
 
@@ -362,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);
     }
 
@@ -591,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);
@@ -672,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);

diff --git 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);

diff --git 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
@@ -146,6 +146,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,