Arbitrum Security Council Election System - arialblack14's results

[N-1] Typos

There are X instances of this issue that are not included in the bot report:

File: ActionExecutionRecord.sol 8-9 The word it is repeated twice.

/// @dev    This contract is designed to be inherited by action contracts, so it
///         it must not use any local storage

File: Common.sol 6 Change from the are members to the members are.

///         and the are members replaced with new ones.

File: SecurityCouncilNomineeElectionGovernor.sol 213 Consider changing from recognised by a Gnosis Safe to a recognised one by Gnosis Safe.

///         recognised by a Gnosis Safe. They need to be able to do this with this same address on each of the

File: SecurityCouncilNomineeElectionGovernor.sol 231 Change from the current the current to the current.

// this only checks against the current the current other cohort, and against the current cohort membership

File: SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol 52 Change from the a to either the or a.

/// @notice Emitted when the a new full weight duration is set

[N-2] _swapMembers function does not check if the member to swap is the same.

The function swapMembers does not check whether the member to remove is the same one that is being added, thus creating unnecessary costs for the user src.

Vulnerability details

    function _swapMembers(address _addressToRemove, address _addressToAdd)
        internal
        returns (Cohort)
    {
        if (_addressToRemove == address(0) || _addressToAdd == address(0)) {
            revert ZeroAddress();
        }
        Cohort cohort = _removeMemberFromCohortArray(_addressToRemove);
        _addMemberToCohortArray(_addressToAdd, cohort);
        _scheduleUpdate();
        return cohort;
    }

Recommendation

Add the following check:

if (_addressToRemove == _addressToAdd) {
     revert SameAddress();
}

[N-3] Unnecessary addresses array creation

The function filterAddressesWithExcludeList creates an unnecessary output array of addresses in memory costing extra gas and adding complexity.src

Vulnerability details

    function filterAddressesWithExcludeList(
        address[] memory input,
        mapping(address => bool) storage excludeList
    ) internal view returns (address[] memory) {
        address[] memory intermediate = new address[](input.length);
        uint256 intermediateLength = 0;

        for (uint256 i = 0; i < input.length; i++) {
            address nominee = input[i];
            if (!excludeList[nominee]) {
                intermediate[intermediateLength] = nominee;
                intermediateLength++;
            }
        }

        address[] memory output = new address[](intermediateLength);
        for (uint256 i = 0; i < intermediateLength; i++) {
            output[i] = intermediate[i];
        }

        return output;
    }

Recommendation

Remove either intermediate or output.

[N-4] Duplicate checks in the _countVote function

In the function above there is a check that is duplicated costing unnecessary gas and adding repetition.

Vulnerability details

src

        uint256 prevVotesUsed = election.votesUsed[account];
        if (prevVotesUsed + votes > availableVotes) {
            revert InsufficientVotes(prevVotesUsed, votes, availableVotes);
        }

        uint240 prevWeightReceived = election.weightReceived[nominee];
        election.votesUsed[account] = prevVotesUsed + votes;

Recommendation

Change to:

        uint256 prevVotesUsed = election.votesUsed[account];
++      uint256 totalVotesUsed = prevVotesUsed + votes;
++      if (totalVotesUsed > availableVotes) {
--      if (prevVotesUsed + votes > availableVotes) {
            revert InsufficientVotes(prevVotesUsed, votes, availableVotes);
        }

        uint240 prevWeightReceived = election.weightReceived[nominee];
--      election.votesUsed[account] = prevVotesUsed + votes;
++      election.votesUsed[account] = totalVotesUsed;

[N-5] Unsafe _downCast function

The function _downCast may revert if the users' votes are too large src.

Vulnerability details

This is highly unlikely but will revert if the votes ever reach such a high value.

    function _downCast(uint256 x) internal pure returns (uint240) {
        if (x > type(uint240).max) {
            revert UintTooLarge(x);
        }
        return uint240(x);
    }

Recommendation

Consider reverting in such a case instead of downcasting.