ctf_sec - Adversary can create spam proposals and DOS the voting because the lack of proposalThreshold validation

[github-actions[bot]]

ctf_sec

medium

Adversary can create spam proposals and DOS the voting because the lack of proposalThreshold validation

Summary

Adversary can create spam proposals and DOS the voting because the low proposalThreshold

Vulnerability Detail

Below is the current implementation, the GrandFund.sol contract inherits from Governor

contract GrantFund is IGrantFund, ExtraordinaryFunding, StandardFunding {

    using Checkpoints for Checkpoints.History;

    IVotes public immutable token;

    /*******************/
    /*** Constructor ***/
    /*******************/

    constructor(IVotes token_, uint256 treasury_)
        Governor("AjnaEcosystemGrantFund")
    {
        ajnaTokenAddress = address(token_);
        token = token_;
        treasury = treasury_;
    }

note the import and the constructor:

import { Governor } from "@oz/governance/Governor.sol";

and

constructor(IVotes token_, uint256 treasury_)
    Governor("AjnaEcosystemGrantFund")

while a few method are overriden, the function proposalThreshold is not overriden and still hardcoded to 0

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/b1c2c43d6af6adf0b0a74cc77683b1d13d66e8bc/contracts/governance/Governor.sol#L196

function proposalThreshold() public view virtual returns (uint256) {
    return 0;
}

When creating the proposal, the code does not validate if the creator has meet the voting power threshold as well

/// @inheritdoc IStandardFunding
function proposeStandard(
    address[] memory targets_,
    uint256[] memory values_,
    bytes[] memory calldatas_,
    string memory description_
) external returns (uint256 proposalId_) {
    proposalId_ = hashProposal(targets_, values_, calldatas_, keccak256(bytes(description_)));

then anyone can spam proposals at low cost (only the gas cost needs to be paid.)

Impact

The problem is that after a lot of spam proposal is created, the function below is impacted:

    /**
     * @notice Vote on a proposal in the screening stage of the Distribution Period.
     * @param account_                The voting account.
     * @param proposal_               The current proposal being voted upon.
     * @param votes_                  The amount of votes being cast.
     * @return                        The amount of votes cast.
     */
    function _screeningVote(address account_, Proposal storage proposal_, uint256 votes_) internal returns (uint256) {
        if (hasVotedScreening[proposal_.distributionId][account_]) revert AlreadyVoted();

        uint256[] storage currentTopTenProposals = topTenProposals[proposal_.distributionId];

        // update proposal votes counter
        proposal_.votesReceived += votes_;

        // check if proposal was already screened
        int indexInArray = _findProposalIndex(proposal_.proposalId, currentTopTenProposals);
        uint256 screenedProposalsLength = currentTopTenProposals.length;

        // check if the proposal should be added to the top ten list for the first time
        if (screenedProposalsLength < 10 && indexInArray == -1) {
            currentTopTenProposals.push(proposal_.proposalId);

            // sort top ten proposals
            _insertionSortProposalsByVotes(currentTopTenProposals);
        }

which calls _insertionSortProposalsByVotes

which calls:

function _insertionSortProposalsByVotes(uint256[] storage arr) internal {
    for (int i = 1; i < int(arr.length); i++) {
        Proposal memory key = standardFundingProposals[arr[uint(i)]];
        int j = i;

        while (j > 0 && key.votesReceived > standardFundingProposals[arr[uint(j - 1)]].votesReceived) {
            // swap values if left item < right item
            uint256 temp = arr[uint(j - 1)];
            arr[uint(j - 1)] = arr[uint(j)];
            arr[uint(j)] = temp;

            j--;
        }
    }
}

The insertion sort runs O(n^2), which means if there are 11 unscreened proposals, the loop needs to run 11 ** 11 = 121 times.

If the advesary create 100 spam proposals, the loop needs to run 10000 times, which is very cost inefficient.

The spammer can keep spam inrelevant proposal until the insertion sort is too gas costly to run, which block and revert screening process.

Code Snippet

https://github.com/sherlock-audit/2023-01-ajna/blob/main/ecosystem-coordination/src/grants/base/StandardFunding.sol#L535-L555

https://github.com/sherlock-audit/2023-01-ajna/blob/main/ecosystem-coordination/src/grants/base/StandardFunding.sol#L391-L436

Tool used

Manual Review

Recommendation

First of all, we recommend the proposal implement minimum voting power proposal threshold to block spam proposal creation.

Also consider eliminate 0 voted proposal before run sort.

and can implement the in-place quick sort to improve the running time of sorint from O(n^2) to O(nlogn)

[MikeHathaway]

We previously discussed and collectively decided to avoid having any proposal threshold in order to maximize participation. We already bound the number of sorts in the screening round to 10 or less by only sorting the current top ten, and only running sort if a proposal would qualify to be added to the top 10. Also, insertion sort runs faster than quick sort on the EVM. We could add a revert for any screening votes with a voting power of 0 (but that user would then just be wasting gas for no effect on others or the vote state), and this revert check would raise gas costs for all others.

[hrishibhat]

Closing this based on Sponsor comment as the protocol that only to qualify into the top 10 would be sorted.