Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit fix: Function Visibility Overly Permissive #169

Merged
merged 4 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions contracts/EmergencyProtectedTimelock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -232,19 +232,19 @@ contract EmergencyProtectedTimelock is IEmergencyProtectedTimelock {

/// @notice Returns whether the emergency protection is enabled.
/// @return isEmergencyProtectionEnabled A boolean indicating whether the emergency protection is enabled.
function isEmergencyProtectionEnabled() public view returns (bool) {
function isEmergencyProtectionEnabled() external view returns (bool) {
return _emergencyProtection.isEmergencyProtectionEnabled();
}

/// @notice Returns whether the emergency mode is active.
/// @return isEmergencyModeActive A boolean indicating whether the emergency protection is enabled.
function isEmergencyModeActive() public view returns (bool) {
function isEmergencyModeActive() external view returns (bool) {
return _emergencyProtection.isEmergencyModeActive();
}

/// @notice Returns the details of the emergency protection.
/// @return details A struct containing the emergency mode duration, emergency mode ends after, and emergency protection ends after.
function getEmergencyProtectionDetails() public view returns (EmergencyProtectionDetails memory details) {
function getEmergencyProtectionDetails() external view returns (EmergencyProtectionDetails memory details) {
return _emergencyProtection.getEmergencyProtectionDetails();
}

Expand Down
4 changes: 2 additions & 2 deletions contracts/ResealManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,16 @@
/// - The DAO governance is blocked by DualGovernance;
/// - Function is called by the governance contract.
/// @param sealable The address of the sealable contract.
function reseal(address sealable) public {
function reseal(address sealable) external {
_checkCallerIsGovernance();

uint256 sealableResumeSinceTimestamp = ISealable(sealable).getResumeSinceTimestamp();
if (sealableResumeSinceTimestamp < block.timestamp || sealableResumeSinceTimestamp == PAUSE_INFINITELY) {
revert SealableWrongPauseState();
}
Address.functionCall(sealable, abi.encodeWithSelector(ISealable.resume.selector));
Address.functionCall(sealable, abi.encodeWithSelector(ISealable.pauseFor.selector, PAUSE_INFINITELY));
}

Check warning

Code scanning / Slither

Unused return Medium

Check warning

Code scanning / Slither

Unused return Medium


/// @notice Resumes the specified sealable contract if it is paused.
/// @dev Works only if conditions are met:
Expand All @@ -62,15 +62,15 @@
/// - Contract are paused until timestamp after current timestamp;
/// - Function is called by the governance contract.
/// @param sealable The address of the sealable contract.
function resume(address sealable) public {
function resume(address sealable) external {
_checkCallerIsGovernance();

uint256 sealableResumeSinceTimestamp = ISealable(sealable).getResumeSinceTimestamp();
if (sealableResumeSinceTimestamp < block.timestamp) {
revert SealableWrongPauseState();
}
Address.functionCall(sealable, abi.encodeWithSelector(ISealable.resume.selector));
}

Check warning

Code scanning / Slither

Unused return Medium


// ---
// Internal methods
Expand Down
14 changes: 7 additions & 7 deletions contracts/committees/HashConsensus.sol
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ abstract contract HashConsensus is Ownable {
/// or if it exceeds the total number of members.
/// @param newMembers The array of addresses to be added as new members
/// @param executionQuorum The minimum number of members required for executing certain operations
function addMembers(address[] memory newMembers, uint256 executionQuorum) public {
function addMembers(address[] memory newMembers, uint256 executionQuorum) external {
_checkOwner();

_addMembers(newMembers, executionQuorum);
Expand All @@ -126,7 +126,7 @@ abstract contract HashConsensus is Ownable {
/// the new total number of members.
/// @param membersToRemove The array of addresses to be removed from the members list.
/// @param executionQuorum The updated minimum number of members required for executing certain operations.
function removeMembers(address[] memory membersToRemove, uint256 executionQuorum) public {
function removeMembers(address[] memory membersToRemove, uint256 executionQuorum) external {
_checkOwner();

_removeMembers(membersToRemove, executionQuorum);
Expand All @@ -135,22 +135,22 @@ abstract contract HashConsensus is Ownable {
/// @notice Gets the list of committee members
/// @dev Public function to return the list of members
/// @return An array of addresses representing the committee members
function getMembers() public view returns (address[] memory) {
function getMembers() external view returns (address[] memory) {
return _members.values();
}

/// @notice Checks if an address is a member of the committee
/// @dev Public function to check membership status
/// @param member The address to check
/// @return A boolean indicating whether the address is a member
function isMember(address member) public view returns (bool) {
function isMember(address member) external view returns (bool) {
return _members.contains(member);
}

/// @notice Sets the timelock duration
/// @dev Only callable by the owner
/// @param timelock The new timelock duration in seconds
function setTimelockDuration(Duration timelock) public {
function setTimelockDuration(Duration timelock) external {
_checkOwner();
if (timelock == timelockDuration) {
revert InvalidTimelockDuration(timelock);
Expand All @@ -162,7 +162,7 @@ abstract contract HashConsensus is Ownable {
/// @notice Sets the quorum value
/// @dev Only callable by the owner
/// @param newQuorum The new quorum value
function setQuorum(uint256 newQuorum) public {
function setQuorum(uint256 newQuorum) external {
_checkOwner();
if (newQuorum == quorum) {
revert InvalidQuorum();
Expand All @@ -175,7 +175,7 @@ abstract contract HashConsensus is Ownable {
/// the proposal has not been scheduled yet. Could happen when execution quorum was set to the same value as
/// current support of the proposal.
/// @param hash The hash of the proposal to be scheduled
function schedule(bytes32 hash) public {
function schedule(bytes32 hash) external {
if (_hashStates[hash].scheduledAt.isNotZero()) {
revert HashAlreadyScheduled(hash);
}
Expand Down
10 changes: 5 additions & 5 deletions contracts/committees/ProposalsList.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ contract ProposalsList {
/// @param offset The starting index for the list of proposals
/// @param limit The maximum number of proposals to return
/// @return proposals An array of Proposal structs
function getProposals(uint256 offset, uint256 limit) public view returns (Proposal[] memory proposals) {
function getProposals(uint256 offset, uint256 limit) external view returns (Proposal[] memory proposals) {
bytes32[] memory keys = _proposals.getOrderedKeys(offset, limit);

uint256 length = keys.length;
Expand All @@ -31,22 +31,22 @@ contract ProposalsList {
/// @dev Fetches the proposal located at the specified index in the map
/// @param index The index of the proposal to retrieve
/// @return The Proposal struct at the given index
function getProposalAt(uint256 index) public view returns (Proposal memory) {
function getProposalAt(uint256 index) external view returns (Proposal memory) {
return _proposals.at(index);
}

/// @notice Retrieves a proposal by its key
/// @dev Fetches the proposal associated with the given key
/// @param key The key of the proposal to retrieve
/// @return The Proposal struct associated with the given key
function getProposal(bytes32 key) public view returns (Proposal memory) {
function getProposal(bytes32 key) external view returns (Proposal memory) {
return _proposals.get(key);
}

/// @notice Retrieves the total number of proposals
/// @dev Fetches the length of the proposals map
/// @return The total number of proposals
function getProposalsLength() public view returns (uint256) {
function getProposalsLength() external view returns (uint256) {
return _proposals.length();
}

Expand All @@ -55,7 +55,7 @@ contract ProposalsList {
/// @param offset The starting index for the list of keys
/// @param limit The maximum number of keys to return
/// @return An array of proposal keys
function getOrderedKeys(uint256 offset, uint256 limit) public view returns (bytes32[] memory) {
function getOrderedKeys(uint256 offset, uint256 limit) external view returns (bytes32[] memory) {
return _proposals.getOrderedKeys(offset, limit);
}

Expand Down
4 changes: 2 additions & 2 deletions contracts/committees/ResealCommittee.sol
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ contract ResealCommittee is HashConsensus, ProposalsList {
/// @dev Allows committee members to vote on resealing a sealed address
/// @param sealable The address to reseal
/// @param support Indicates whether the member supports the proposal
function voteReseal(address sealable, bool support) public {
function voteReseal(address sealable, bool support) external {
_checkCallerIsMember();

if (sealable == address(0)) {
Expand All @@ -56,7 +56,7 @@ contract ResealCommittee is HashConsensus, ProposalsList {
/// @return executionQuorum The required number of votes for execution
/// @return quorumAt The timestamp when the quorum was reached
function getResealState(address sealable)
public
external
view
returns (uint256 support, uint256 executionQuorum, Timestamp quorumAt)
{
Expand Down
12 changes: 6 additions & 6 deletions contracts/committees/TiebreakerCoreCommittee.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
/// @notice Votes on a proposal to schedule
/// @dev Allows committee members to vote on scheduling a proposal
/// @param proposalId The ID of the proposal to schedule
function scheduleProposal(uint256 proposalId) public {
function scheduleProposal(uint256 proposalId) external {
_checkCallerIsMember();
checkProposalExists(proposalId);
(bytes memory proposalData, bytes32 key) = _encodeScheduleProposal(proposalId);
Expand All @@ -58,7 +58,7 @@
/// @return quorumAt The timestamp when the quorum was reached
/// @return isExecuted Whether the proposal has been executed
function getScheduleProposalState(uint256 proposalId)
public
external
view
returns (uint256 support, uint256 executionQuorum, Timestamp quorumAt, bool isExecuted)
{
Expand All @@ -69,13 +69,13 @@
/// @notice Executes an approved schedule proposal
/// @dev Executes the schedule proposal by calling the tiebreakerScheduleProposal function on the Dual Governance contract
/// @param proposalId The ID of the proposal to schedule
function executeScheduleProposal(uint256 proposalId) public {
function executeScheduleProposal(uint256 proposalId) external {
(, bytes32 key) = _encodeScheduleProposal(proposalId);
_markUsed(key);
Address.functionCall(
DUAL_GOVERNANCE, abi.encodeWithSelector(ITiebreaker.tiebreakerScheduleProposal.selector, proposalId)
);
}

/// @notice Checks if a proposal exists
/// @param proposalId The ID of the proposal to check
Expand Down Expand Up @@ -105,7 +105,7 @@
/// @dev Retrieves the resume nonce for the given sealable address
/// @param sealable The address of the sealable to get the nonce for
/// @return The current resume nonce for the sealable address
function getSealableResumeNonce(address sealable) public view returns (uint256) {
function getSealableResumeNonce(address sealable) external view returns (uint256) {
return _sealableResumeNonces[sealable];
}

Expand All @@ -114,7 +114,7 @@
/// 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 {
function sealableResume(address sealable, uint256 nonce) external {
_checkCallerIsMember();

if (sealable == address(0)) {
Expand All @@ -140,7 +140,7 @@
function getSealableResumeState(
address sealable,
uint256 nonce
) public view returns (uint256 support, uint256 executionQuorum, Timestamp quorumAt, bool isExecuted) {
) external view returns (uint256 support, uint256 executionQuorum, Timestamp quorumAt, bool isExecuted) {
(, bytes32 key) = _encodeSealableResume(sealable, nonce);
return _getHashState(key);
}
Expand Down
12 changes: 6 additions & 6 deletions contracts/committees/TiebreakerSubCommittee.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
/// @notice Votes on a proposal to schedule
/// @dev Allows committee members to vote on scheduling a proposal
/// @param proposalId The ID of the proposal to schedule
function scheduleProposal(uint256 proposalId) public {
function scheduleProposal(uint256 proposalId) external {
_checkCallerIsMember();
ITiebreakerCoreCommittee(TIEBREAKER_CORE_COMMITTEE).checkProposalExists(proposalId);
(bytes memory proposalData, bytes32 key) = _encodeApproveProposal(proposalId);
Expand All @@ -58,7 +58,7 @@
/// @return quorumAt The number of votes required to reach quorum
/// @return isExecuted Whether the proposal has been executed
function getScheduleProposalState(uint256 proposalId)
public
external
view
returns (uint256 support, uint256 executionQuorum, Timestamp quorumAt, bool isExecuted)
{
Expand All @@ -69,14 +69,14 @@
/// @notice Executes an approved schedule proposal
/// @dev Executes the schedule proposal by calling the scheduleProposal function on the Tiebreaker Core contract
/// @param proposalId The ID of the proposal to schedule
function executeScheduleProposal(uint256 proposalId) public {
function executeScheduleProposal(uint256 proposalId) external {
(, bytes32 key) = _encodeApproveProposal(proposalId);
_markUsed(key);
Address.functionCall(
TIEBREAKER_CORE_COMMITTEE,
abi.encodeWithSelector(ITiebreakerCoreCommittee.scheduleProposal.selector, proposalId)
);
}

/// @notice Encodes a schedule proposal
/// @dev Internal function to encode the proposal data and generate the proposal key
Expand All @@ -96,7 +96,7 @@
/// @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 {
function sealableResume(address sealable) external {
_checkCallerIsMember();

if (sealable == address(0)) {
Expand All @@ -115,7 +115,7 @@
/// @return quorumAt The timestamp when the quorum was reached
/// @return isExecuted Whether the proposal has been executed
function getSealableResumeState(address sealable)
public
external
view
returns (uint256 support, uint256 executionQuorum, Timestamp quorumAt, bool isExecuted)
{
Expand All @@ -126,14 +126,14 @@
/// @notice Executes an approved resume sealable proposal
/// @dev Executes the resume sealable proposal by calling the sealableResume function on the Tiebreaker Core contract
/// @param sealable The address to resume
function executeSealableResume(address sealable) public {
function executeSealableResume(address sealable) external {
(, bytes32 key, uint256 nonce) = _encodeSealableResume(sealable);
_markUsed(key);
Address.functionCall(
TIEBREAKER_CORE_COMMITTEE,
abi.encodeWithSelector(ITiebreakerCoreCommittee.sealableResume.selector, sealable, nonce)
);
}

/// @notice Encodes a resume sealable proposal
/// @dev Internal function to encode the proposal data and generate the proposal key
Expand Down
22 changes: 11 additions & 11 deletions docs/plan-b.md
Original file line number Diff line number Diff line change
Expand Up @@ -238,31 +238,31 @@ The result of the call.

### Function: `ProposalsList.getProposals`
```solidity
function getProposals(uint256 offset, uint256 limit) public view returns (Proposal[] memory proposals)
function getProposals(uint256 offset, uint256 limit) external view returns (Proposal[] memory proposals)
```
Returns a list of `Proposal` structs, starting from the specified `offset` and bounded to the specified `limit`.

### Function: `ProposalsList.getProposalAt`
```solidity
function getProposalAt(uint256 index) public view returns (Proposal memory)
function getProposalAt(uint256 index) external view returns (Proposal memory)
```
Returns the `Proposal` at the specified index.

### Function: `ProposalsList.getProposal`
```solidity
function getProposal(bytes32 key) public view returns (Proposal memory)
function getProposal(bytes32 key) external view returns (Proposal memory)
```
Returns the `Proposal` with the given key.

### Function: `ProposalsList.getProposalsLength`
```solidity
function getProposalsLength() public view returns (uint256)
function getProposalsLength() external view returns (uint256)
```
Returns the total number of created `Proposal`s.

### Function: `ProposalsList.getOrderedKeys`
```solidity
function getOrderedKeys(uint256 offset, uint256 limit) public view returns (bytes32[] memory)
function getOrderedKeys(uint256 offset, uint256 limit) external view returns (bytes32[] memory)
```
Returns a list of `Proposal` keys, starting from the specified `offset` and bounded by the specified `limit`.

Expand All @@ -272,7 +272,7 @@ Returns a list of `Proposal` keys, starting from the specified `offset` and boun

### Function: `HashConsensus.addMember`
```solidity
function addMember(address newMember, uint256 newQuorum) public onlyOwner
function addMember(address newMember, uint256 newQuorum) external onlyOwner
```
Adds a new member and updates the quorum.

Expand All @@ -282,7 +282,7 @@ Adds a new member and updates the quorum.

### Function: `HashConsensus.removeMember`
```solidity
function removeMember(address memberToRemove, uint256 newQuorum) public onlyOwner
function removeMember(address memberToRemove, uint256 newQuorum) external onlyOwner
```
Removes a member and updates the quorum.

Expand All @@ -293,19 +293,19 @@ Removes a member and updates the quorum.

### Function: `HashConsensus.getMembers`
```solidity
function getMembers() public view returns (address[] memory)
function getMembers() external view returns (address[] memory)
```
Returns the list of current members.

### Function: `HashConsensus.isMember`
```solidity
function isMember(address member) public view returns (bool)
function isMember(address member) external view returns (bool)
```
Returns whether an account is listed as a member.

### Function: `HashConsensus.setTimelockDuration`
```solidity
function setTimelockDuration(uint256 timelock) public onlyOwner
function setTimelockDuration(uint256 timelock) external onlyOwner
```
Sets the duration of the timelock.

Expand All @@ -314,7 +314,7 @@ Sets the duration of the timelock.

### Function: `HashConsensus.setQuorum`
```solidity
function setQuorum(uint256 newQuorum) public onlyOwner
function setQuorum(uint256 newQuorum) external onlyOwner
```
Sets the quorum required for decision execution.

Expand Down
4 changes: 2 additions & 2 deletions docs/specification.md
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ To function properly, the **ResealManager** must be granted the `PAUSE_ROLE` and
### Function ResealManager.reseal

```solidity
function reseal(address sealable) public
function reseal(address sealable) external
```

Extends the pause of the specified `sealable` contract indefinitely.
Expand Down Expand Up @@ -1372,7 +1372,7 @@ Returns the state of a sealable resume request including support count, quorum,
### Function: TiebreakerSubCommittee.executeSealableResume

```solidity
function executeSealableResume(address sealable) public
function executeSealableResume(address sealable) external
```

Executes a sealable resume request by calling the sealableResume function on the `TiebreakerCoreCommittee` contract and increments the nonce.
Expand Down