code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/grants/base/StandardFunding.sol#L357-L358

Vulnerability details

Impact

This issue could lead to a scenario where a proposal with zero or insufficient funding votes could be executed.

Proof of Concept

Ajna's grant system allows any project to submit a proposal consisting of a desired Ajna token quantity and the recipient's address. The system uses a two-stage process, where proposals first go through a screening stage, and the top 10 most supported proposals are selected for a funding stage. Ajna token holders can vote for these proposals in this stage using a quadratic system.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/grants/base/StandardFunding.sol#L300-L340

 function updateSlate(
        uint256[] calldata proposalIds_,
        uint24 distributionId_
    ) external override returns (bool newTopSlate_) {
        QuarterlyDistribution storage currentDistribution = _distributions[distributionId_];

        // store number of proposals for reduced gas cost of iterations
        uint256 numProposalsInSlate = proposalIds_.length;

        // check the each proposal in the slate is valid, and get the sum of the proposals fundingVotesReceived
        uint256 sum = _validateSlate(distributionId_, currentDistribution.endBlock, currentDistribution.fundsAvailable, proposalIds_, numProposalsInSlate);

        // get pointers for comparing proposal slates
        bytes32 currentSlateHash = currentDistribution.fundedSlateHash;
        bytes32 newSlateHash     = keccak256(abi.encode(proposalIds_));

        // check if slate of proposals is better than the existing slate, and is thus the new top slate
        newTopSlate_ = currentSlateHash == 0 ||
            (currentSlateHash!= 0 && sum > _sumProposalFundingVotes(_fundedProposalSlates[currentSlateHash]));

        // if slate of proposals is new top slate, update state
        if (newTopSlate_) {
            uint256[] storage existingSlate = _fundedProposalSlates[newSlateHash];

            for (uint i = 0; i < numProposalsInSlate; ) {

                // update list of proposals to fund
                existingSlate.push(proposalIds_[i]);

                unchecked { ++i; }
            }

            // update hash to point to the new leading slate of proposals
            currentDistribution.fundedSlateHash = newSlateHash;

            emit FundedSlateUpdated(
                distributionId_,
                newSlateHash
            );
        }
    }

After the voting period concludes, the winning proposals are determined. The winning slate is the one that maximizes the sum of the net number of votes cast in favor of the proposals, while ensuring the total budget of the winning proposals does not exceed the Grant Budget Ceiling (GBC). These checks occur in the validateSlate function.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/grants/base/StandardFunding.sol#L421-L454

 function _validateSlate(uint24 distributionId_, uint256 endBlock, uint256 distributionPeriodFundsAvailable_, uint256[] calldata proposalIds_, uint256 numProposalsInSlate_) internal view returns (uint256 sum_) {
        // check that the function is being called within the challenge period

        if (block.number <= endBlock || block.number > _getChallengeStageEndBlock(endBlock)) {
            revert InvalidProposalSlate();
        }

        // check that the slate has no duplicates
        if (_hasDuplicates(proposalIds_)) revert InvalidProposalSlate();

        uint256 gbc = distributionPeriodFundsAvailable_;
        uint256 totalTokensRequested = 0;

        // check each proposal in the slate is valid
        for (uint i = 0; i < numProposalsInSlate_; ) {
            Proposal memory proposal = _standardFundingProposals[proposalIds_[i]];

            // check if Proposal is in the topTenProposals list
            if (_findProposalIndex(proposalIds_[i], _topTenProposals[distributionId_]) == -1) revert InvalidProposalSlate();

            // account for fundingVotesReceived possibly being negative
            if (proposal.fundingVotesReceived < 0) revert InvalidProposalSlate();

            // update counters
            sum_ += uint128(proposal.fundingVotesReceived); // since we are converting from int128 to uint128, we can safely assume that the value will not overflow
            totalTokensRequested += proposal.tokensRequested;

            // check if slate of proposals exceeded budget constraint ( 90% of GBC )
            // @audit-info why, cant you just remove 1?
            if (totalTokensRequested > (gbc * 9 / 10)) {
                revert InvalidProposalSlate();
            }

            unchecked { ++i; }
        }
    }

However, an issue arises when only a single proposal passed the screening period. Regardless of whether it receives enough funding votes, it will be the only one in the potential slate, and this proposal will get executed. The following solidity function, testUnderfundedProposal, demonstrates this issue.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/grants/base/StandardFunding.sol#L860-L865

    function _standardFundingVoteSucceeded(
        uint256 proposalId_
    ) internal view returns (bool) {
        uint24 distributionId = _standardFundingProposals[proposalId_].distributionId;
        return _findProposalIndex(proposalId_, _fundedProposalSlates[_distributions[distributionId].fundedSlateHash]) != -1;
    }

So, even if a proposal has 0 fundingVotesReceived, it will still get executed. It would be absolutely normal for a proposal to have 0 votes, since the variable that accounts for its an integer. However, having a total of 0 votes doesn't mean its supposed to get funded.

Here's a PoC which showcases the issue.

function testUnderfundedProposal() external {
        changePrank(_tokenHolder1);
        _token.delegate(_tokenHolder1);

        address fundingReceiver = makeAddr("fundingReceiver");

        console.log("Funding receiver balance", _token.balanceOf(fundingReceiver));

        vm.roll(_startBlock + 150);

        // start first distribution
        _startDistributionPeriod(_grantFund);

        uint24 distributionId = _grantFund.getDistributionId();

        (, , , uint128 gbc_distribution1, , ) = _grantFund.getDistributionPeriodInfo(distributionId);

        TestProposalParams[] memory testProposalParams_distribution1 = new TestProposalParams[](1);
        testProposalParams_distribution1[0] = TestProposalParams(fundingReceiver, 8_500_000 * 1e18);

        // create 1 proposal paying out tokens
        TestProposal[] memory testProposals_distribution1 = _createNProposals(_grantFund, _token, testProposalParams_distribution1);
        assertEq(testProposals_distribution1.length, 1);

        vm.roll(_startBlock + 200);

        // screening period votes

        // construct vote params
        IStandardFunding.ScreeningVoteParams[] memory params = new IStandardFunding.ScreeningVoteParams[](1);
        params[0].proposalId = testProposals_distribution1[0].proposalId;
        params[0].votes = _getScreeningVotes(_grantFund, _tokenHolder1);

        changePrank(_tokenHolder1);
        _grantFund.screeningVote(params);

        // skip time to move from screening period to funding period
        vm.roll(_startBlock + 600_000);

        // check topTenProposals array is correct after screening period - only 1 should have advanced
        GrantFund.Proposal[] memory screenedProposals_distribution1 = _getProposalListFromProposalIds(_grantFund, _grantFund.getTopTenProposals(distributionId));
        assertEq(screenedProposals_distribution1.length, 1);

        // funding period votes
        // _fundingVote(_grantFund, _tokenHolder1, screenedProposals_distribution1[0].proposalId, voteYes, 50_000_000 * 1e18);

        // skip to the Challenge period
        vm.roll(_startBlock + 650_000);

        uint256[] memory potentialProposalSlate = new uint256[](1);
        potentialProposalSlate[0] = screenedProposals_distribution1[0].proposalId;

        // updateSlate
        _grantFund.updateSlate(potentialProposalSlate, distributionId);

        // skip to the end of Challenge period
        vm.roll(_startBlock + 700_000);

        // check proposal status isn't defeated
        IFunding.ProposalState proposalState = _grantFund.state(testProposals_distribution1[0].proposalId);
        assert(uint8(proposalState) != uint8(IFunding.ProposalState.Defeated));

        // check proposal status is succeeded
        proposalState = _grantFund.state(testProposals_distribution1[0].proposalId);
        assertEq(uint8(proposalState), uint8(IFunding.ProposalState.Succeeded));

        (,,uint128 votesReceived,,int128 fundingVotesReceived,) = _grantFund.getProposalInfo(testProposals_distribution1[0].proposalId);

        // No funding votes received
        assertEq(fundingVotesReceived == 0, true);

        _grantFund.executeStandard(testProposals_distribution1[0].targets, testProposals_distribution1[0].values, testProposals_distribution1[0].calldatas, keccak256(bytes(testProposals_distribution1[0].description)));

        console.log("Funding receiver balance", _token.balanceOf(fundingReceiver));
    }

It is possible to argue that the flawed proposal may enter the top-ranked slate, but it could be supplanted by an alternative top slate containing distinct proposals before the period concludes, as long as the latter slate is more optimal. Consequently, this could potentially exclude the invalid proposal from consideration.

However, if there are fewer than 10 proposals in the pool during the screening period (noting that new proposals cannot be submitted after the screening period has ended), there's nothing to replace the invalid proposal with and no way to stop it from getting executed.

Tools Used

Manual review

Recommended Mitigation Steps

To prevent under-voted proposals from being executed, the code should be adjusted to include a minimum vote threshold for proposals, irrespective of the number of proposals that pass the screening phase. This way, a proposal would require a minimum number of votes before it can be executed, ensuring that only proposals with sufficient community support are funded.

This can be achieved by changing < to <= here.

          // account for fundingVotesReceived possibly being negative
            if (proposal.fundingVotesReceived < 0) revert InvalidProposalSlate();

Assessed type

Invalid Validation

Picodes commented 1 year ago

Interesting but it is more a design choice than a security finding, as there are incentives to create proposal (as they can get funded) so it seems reasonable to expect > 10 proposals to be created