Ajna Protocol - rbserver's results

A peer to peer, oracleless, permissionless lending protocol with no governance, accepting both fungible and non fungible tokens as collateral.

General Information

Platform: Code4rena

Start Date: 03/05/2023

Pot Size: $60,500 USDC

Total HM: 25

Participants: 114

Period: 8 days

Judge: Picodes

Total Solo HM: 6

Id: 234

League: ETH

Ajna Protocol

Findings Distribution

Researcher Performance

Rank: 11/114

Findings: 3

Award: $912.95

QA:
grade-a

🌟 Selected for report: 2

🚀 Solo Findings: 0

Findings Information

🌟 Selected for report: Kenshin

Also found by: 0xRobocop, Dug, REACH, Ruhum, hyh, nobody2018, rbserver

Labels

bug
3 (High Risk)
satisfactory
sponsor confirmed
edited-by-warden
duplicate-450

Awards

187.2333 USDC - $187.23

External Links

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L236-L265 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L274-L293 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L119-L164 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L197-L220

Vulnerability details

Impact

When block.number is more than the end block of the challenge stage for the current distribution period, the following StandardFunding.claimDelegateReward function can be called to claim the delegate reward for the corresponding voter. Since the StandardFunding._getDelegateReward function below executes rewards_ = Maths.wdiv(Maths.wmul(currentDistribution_.fundsAvailable, votingPowerAllocatedByDelegatee), currentDistribution_.fundingVotePowerCast) / 10, the delegate reward is part of fundsAvailable for the current distribution period.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L236-L265

    function claimDelegateReward(
        uint24 distributionId_
    ) external override returns(uint256 rewardClaimed_) {
        // Revert if delegatee didn't vote in screening stage
        if(screeningVotesCast[distributionId_][msg.sender] == 0) revert DelegateRewardInvalid();

        QuarterlyDistribution memory currentDistribution = _distributions[distributionId_];

        // Check if Challenge Period is still active
        if(block.number < _getChallengeStageEndBlock(currentDistribution.endBlock)) revert ChallengePeriodNotEnded();

        // check rewards haven't already been claimed
        if(hasClaimedReward[distributionId_][msg.sender]) revert RewardAlreadyClaimed();

        QuadraticVoter memory voter = _quadraticVoters[distributionId_][msg.sender];

        // calculate rewards earned for voting
        rewardClaimed_ = _getDelegateReward(currentDistribution, voter);

        hasClaimedReward[distributionId_][msg.sender] = true;

        ...

        // transfer rewards to delegatee
        IERC20(ajnaTokenAddress).safeTransfer(msg.sender, rewardClaimed_);
    }

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L274-L293

    function _getDelegateReward(
        QuarterlyDistribution memory currentDistribution_,
        QuadraticVoter memory voter_
    ) internal pure returns (uint256 rewards_) {
        // calculate the total voting power available to the voter that was allocated in the funding stage
        uint256 votingPowerAllocatedByDelegatee = voter_.votingPower - voter_.remainingVotingPower;

        // if none of the voter's voting power was allocated, they receive no rewards
        if (votingPowerAllocatedByDelegatee == 0) return 0;

        // calculate reward
        // delegateeReward = 10 % of GBC distributed as per delegatee Voting power allocated
        rewards_ = Maths.wdiv(
            Maths.wmul(
                currentDistribution_.fundsAvailable,
                votingPowerAllocatedByDelegatee
            ),
            currentDistribution_.fundingVotePowerCast
        ) / 10;
    }

At the same time when block.number is more than the end block of the challenge stage for the current distribution period, the following StandardFunding.startNewDistributionPeriod function can also be called to start a new distribution period. Because block.number > _getChallengeStageEndBlock(currentDistributionEndBlock) is true, _updateTreasury(currentDistributionId) would be executed.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L119-L164

    function startNewDistributionPeriod() external override returns (uint24 newDistributionId_) {
        uint24  currentDistributionId       = _currentDistributionId;
        uint256 currentDistributionEndBlock = _distributions[currentDistributionId].endBlock;

        // check that there isn't currently an active distribution period
        if (block.number <= currentDistributionEndBlock) revert DistributionPeriodStillActive();

        // update Treasury with unused funds from last two distributions
        {
            // Check if any last distribution exists and its challenge stage is over
            if (currentDistributionId > 0 && (block.number > _getChallengeStageEndBlock(currentDistributionEndBlock))) {
                // Add unused funds from last distribution to treasury
                _updateTreasury(currentDistributionId);
            }

            // checks if any second last distribution exist and its unused funds are not added into treasury
            if (currentDistributionId > 1 && !_isSurplusFundsUpdated[currentDistributionId - 1]) {
                // Add unused funds from second last distribution to treasury
                _updateTreasury(currentDistributionId - 1);
            }
        }

        // set the distribution period to start at the current block
        uint48 startBlock = SafeCast.toUint48(block.number);
        uint48 endBlock = startBlock + DISTRIBUTION_PERIOD_LENGTH;

        // set new value for currentDistributionId
        newDistributionId_ = _setNewDistributionId();

        // create QuarterlyDistribution struct
        QuarterlyDistribution storage newDistributionPeriod = _distributions[newDistributionId_];
        newDistributionPeriod.id              = newDistributionId_;
        newDistributionPeriod.startBlock      = startBlock;
        newDistributionPeriod.endBlock        = endBlock;
        uint256 gbc                           = Maths.wmul(treasury, GLOBAL_BUDGET_CONSTRAINT);
        newDistributionPeriod.fundsAvailable  = SafeCast.toUint128(gbc);

        // decrease the treasury by the amount that is held for allocation in the new distribution period
        treasury -= gbc;

        ...
    }

When calling the following StandardFunding._updateTreasury function for the current distribution period, treasury += (fundsAvailable - totalTokensRequested) would be executed so the difference between fundsAvailable and total tokens requested by the proposals to be executed for the current distribution period is added back to treasury. However, such difference still includes the voters' delegate rewards that are also part of fundsAvailable for the current distribution period. This causes the accounting for treasury to be incorrect in which treasury becomes more than it should be; after voters claim their delegate rewards for the current distribution period, treasury would become more than the GrantFund contract's AJNA balance.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L197-L220

    function _updateTreasury(
        uint24 distributionId_
    ) private {
        bytes32 fundedSlateHash = _distributions[distributionId_].fundedSlateHash;
        uint256 fundsAvailable  = _distributions[distributionId_].fundsAvailable;

        uint256[] memory fundingProposalIds = _fundedProposalSlates[fundedSlateHash];

        uint256 totalTokensRequested;
        uint256 numFundedProposals = fundingProposalIds.length;

        for (uint i = 0; i < numFundedProposals; ) {
            Proposal memory proposal = _standardFundingProposals[fundingProposalIds[i]];

            totalTokensRequested += proposal.tokensRequested;

            unchecked { ++i; }
        }

        // readd non distributed tokens to the treasury
        treasury += (fundsAvailable - totalTokensRequested);

        _isSurplusFundsUpdated[distributionId_] = true;
    }

Back in the StandardFunding.startNewDistributionPeriod function, executing uint256 gbc = Maths.wmul(treasury, GLOBAL_BUDGET_CONSTRAINT) and newDistributionPeriod.fundsAvailable = SafeCast.toUint128(gbc) would cause fundsAvailable for the new distribution period to be more than it should be because treasury is inflated already. Because proposals' number of requested tokens and voters' delegate rewards are proportions of fundsAvailable for the new distribution period, these proposals can request more tokens and these voters can claim more delegate rewards than they are allowed to. The extra number of tokens requested by the proposals and extra delegate rewards claimed by the voters that are more than they are entitled to are essentially lost in the treasury's perspective.

Proof of Concept

The following steps can occur for the described scenario.

  1. For the current distribution period, the total tokens requested by the proposals to be executed can be 9e18, the total delegate rewards that can be claimed by the voters can be 1e18, fundsAvailable can be 10e18, and treasury can be 333,333,333,333,333,333,334 - 10e18 = 323,333,333,333,333,333,334.
  2. When block.number is more than the end block of the challenge stage for the current distribution period, the StandardFunding.startNewDistributionPeriod function is called to start the new distribution period.
  3. When the StandardFunding.startNewDistributionPeriod function calls the StandardFunding._updateTreasury function, treasury is increased by 10e18 - 9e18 = 1e18 to become 323,333,333,333,333,333,334 + 1e18 = 324,333,333,333,333,333,334.
  4. However, in step 3, treasury should not be increased at all because the increment of 1e18 was actually the voters' delegate rewards for the current distribution period. If these voters claim these delegate rewards, the GrantFund contract's AJNA balance would become 333,333,333,333,333,333,334 - 9e18 - 1e18 = 323,333,333,333,333,333,334 but treasury is increased to 324,333,333,333,333,333,334 in step 3, which is more than the GrantFund contract's AJNA balance. Thus, the accounting for treasury becomes incorrect.
  5. Continuing from step 3, when the StandardFunding.startNewDistributionPeriod function executes uint256 gbc = Maths.wmul(treasury, GLOBAL_BUDGET_CONSTRAINT) and newDistributionPeriod.fundsAvailable = SafeCast.toUint128(gbc), fundsAvailable for the new distribution period is calculated to 324,333,333,333,333,333,334 * 0.03 = 9,730,000,000,000,000,000.02 = 9,730,000,000,000,000,000.
  6. If treasury was not increased by the voters' delegate rewards for the current distribution period in step 3, fundsAvailable for the new distribution period would be calculated to 323,333,333,333,333,333,334 * 0.03 = 9,700,000,000,000,000,000.02 = 9,700,000,000,000,000,000, which is 3e16 less than the incorrectly inflated fundsAvailable for the new distribution period shown in step 5.

Tools Used

VSCode

The StandardFunding._updateTreasury function can be updated to not add the voters' total delegate rewards for the corresponding distribution period back to treasury.

Assessed type

Error

#0 - c4-sponsor

2023-05-19T19:16:47Z

MikeHathaway marked the issue as sponsor confirmed

#1 - dmitriia

2023-05-21T18:15:13Z

Looks like dup of #263

#2 - c4-judge

2023-05-30T18:10:31Z

Picodes marked the issue as duplicate of #263

#3 - c4-judge

2023-05-30T18:14:46Z

Picodes marked the issue as satisfactory

Findings Information

🌟 Selected for report: rbserver

Also found by: deadrxsezzz, rvierdiiev

Labels

bug
2 (Med Risk)
primary issue
satisfactory
selected for report
sponsor disputed
M-07

Awards

329.7645 USDC - $329.76

External Links

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L519-L569 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L891-L910 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/Funding.sol#L76-L93 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L572-L596 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L698-L753 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L872-L881 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L131-L157 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L246-L256

Vulnerability details

Impact

Because of if (block.number <= screeningStageEndBlock || block.number > endBlock) revert InvalidVote(), the following StandardFunding.fundingVote function can only execute uint128 newVotingPower = SafeCast.toUint128(_getVotesFunding(msg.sender, votingPower, voter.remainingVotingPower, screeningStageEndBlock)) when block.number is bigger than screeningStageEndBlock.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L519-L569

    function fundingVote(
        FundingVoteParams[] memory voteParams_
    ) external override returns (uint256 votesCast_) {
        uint24 currentDistributionId = _currentDistributionId;

        QuarterlyDistribution storage currentDistribution = _distributions[currentDistributionId];
        QuadraticVoter        storage voter               = _quadraticVoters[currentDistributionId][msg.sender];

        uint256 endBlock = currentDistribution.endBlock;

        uint256 screeningStageEndBlock = _getScreeningStageEndBlock(endBlock);

        // check that the funding stage is active
        if (block.number <= screeningStageEndBlock || block.number > endBlock) revert InvalidVote();

        uint128 votingPower = voter.votingPower;

        // if this is the first time a voter has attempted to vote this period,
        // set initial voting power and remaining voting power
        if (votingPower == 0) {

            // calculate the voting power available to the voting power in this funding stage
            uint128 newVotingPower = SafeCast.toUint128(_getVotesFunding(msg.sender, votingPower, voter.remainingVotingPower, screeningStageEndBlock));

            voter.votingPower          = newVotingPower;
            voter.remainingVotingPower = newVotingPower;
        }

        ...
    }

When the StandardFunding.fundingVote function calls the following StandardFunding._getVotesFunding function, screeningStageEndBlock would be used as the voteStartBlock_ input for calling the Funding._getVotesAtSnapshotBlocks function below. Because block.number would always be bigger than screeningStageEndBlock, voteStartBlock_ would always be screeningStageEndBlock in the Funding._getVotesAtSnapshotBlocks function. This means that the Funding._getVotesAtSnapshotBlocks function would always return the same voting power for the same voter at any block.number that is bigger than screeningStageEndBlock during the funding phase.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L891-L910

    function _getVotesFunding(
        address account_,
        uint256 votingPower_,
        uint256 remainingVotingPower_,
        uint256 screeningStageEndBlock_
    ) internal view returns (uint256 votes_) {
        // voter has already allocated some of their budget this period
        if (votingPower_ != 0) {
            votes_ = remainingVotingPower_;
        }
        // voter hasn't yet called _castVote in this period
        else {
            votes_ = Maths.wpow(
            _getVotesAtSnapshotBlocks(
                account_,
                screeningStageEndBlock_ - VOTING_POWER_SNAPSHOT_DELAY,
                screeningStageEndBlock_
            ), 2);
        }
    }

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/Funding.sol#L76-L93

    function _getVotesAtSnapshotBlocks(
        address account_,
        uint256 snapshot_,
        uint256 voteStartBlock_
    ) internal view returns (uint256) {
        IVotes token = IVotes(ajnaTokenAddress);

        // calculate the number of votes available at the snapshot block
        uint256 votes1 = token.getPastVotes(account_, snapshot_);

        // enable voting weight to be calculated during the voting period's start block
        voteStartBlock_ = voteStartBlock_ != block.number ? voteStartBlock_ : block.number - 1;

        // calculate the number of votes available at the stage's start block
        uint256 votes2 = token.getPastVotes(account_, voteStartBlock_);

        return Maths.min(votes2, votes1);
    }

However, because of if (block.number < currentDistribution.startBlock || block.number > _getScreeningStageEndBlock(currentDistribution.endBlock)) revert InvalidVote(), calling the following StandardFunding.screeningVote function would not revert when block.number equals the current distribution period's start block.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L572-L596

    function screeningVote(
        ScreeningVoteParams[] memory voteParams_
    ) external override returns (uint256 votesCast_) {
        QuarterlyDistribution memory currentDistribution = _distributions[_currentDistributionId];

        // check screening stage is active
        if (block.number < currentDistribution.startBlock || block.number > _getScreeningStageEndBlock(currentDistribution.endBlock)) revert InvalidVote();

        uint256 numVotesCast = voteParams_.length;

        for (uint256 i = 0; i < numVotesCast; ) {
            Proposal storage proposal = _standardFundingProposals[voteParams_[i].proposalId];

            // check that the proposal is part of the current distribution period
            if (proposal.distributionId != currentDistribution.id) revert InvalidVote();

            uint256 votes = voteParams_[i].votes;

            // cast each successive vote
            votesCast_ += votes;
            _screeningVote(msg.sender, proposal, votes);

            unchecked { ++i; }
        }
    }

When the StandardFunding.screeningVote function calls the following StandardFunding._screeningVote function, if (screeningVotesCast[distributionId][account_] + votes_ > _getVotesScreening(distributionId, account_)) revert InsufficientVotingPower() is executed in which the StandardFunding._getVotesScreening function below would use the current distribution period's start block as the voteStartBlock_ input for calling the Funding._getVotesAtSnapshotBlocks function. Since the Funding._getVotesAtSnapshotBlocks function executes voteStartBlock_ = voteStartBlock_ != block.number ? voteStartBlock_ : block.number - 1, voteStartBlock_ would be 1 block prior to the current distribution period's start block when block.number equals the current distribution period's start block, and voteStartBlock_ would be the current distribution period's start block when block.number is bigger than the current distribution period's start block. However, it is possible that the same voter has different available votes at 1 block prior to the current distribution period's start block and at the current distribution period's start block. This is unlike the StandardFunding._getVotesFunding function that would always return the same voting power for the same voter when calling the StandardFunding.fundingVote function during the funding phase. Since calling the StandardFunding._getVotesScreening function when block.number equals the current distribution period's start block and when block.number is bigger than the current distribution period's start block during the screening phase can return different available votes for the same voter, this voter would call the StandardFunding.screeningVote function at a chosen block.number that would provide the highest votes.

This should not be allowed because _getVotesScreening(distributionId, account_) needs to return the same number of votes across all blocks during the screening phase to make if (screeningVotesCast[distributionId][account_] + votes_ > _getVotesScreening(distributionId, account_)) revert InsufficientVotingPower() effective in the StandardFunding._screeningVote function. For example, a voter who has no available votes at 1 block prior to the current distribution period's start block can mint many AJNA tokens at the current distribution period's start block and call the StandardFunding.screeningVote function at block.number that is bigger than the current distribution period's start block during the screening phase to use her or his available votes at current distribution period's start block. For another example, a voter who has available votes at 1 block prior to the current distribution period's start block can call the StandardFunding.screeningVote function when block.number equals the current distribution period's start block and then sell all of her or his AJNA tokens at the same block.number. Such voters' actions are unfair to other voters who maintain the same number of available votes at 1 block prior to the current distribution period's start block and at the current distribution period's start block for the screening stage voting.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L698-L753

    function _screeningVote(
        address account_,
        Proposal storage proposal_,
        uint256 votes_
    ) internal {
        uint24 distributionId = proposal_.distributionId;

        // check that the voter has enough voting power to cast the vote
        if (screeningVotesCast[distributionId][account_] + votes_ > _getVotesScreening(distributionId, account_)) revert InsufficientVotingPower();

        ...
    }

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L872-L881

    function _getVotesScreening(uint24 distributionId_, address account_) internal view returns (uint256 votes_) {
        uint256 startBlock = _distributions[distributionId_].startBlock;

        // calculate voting weight based on the number of tokens held at the snapshot blocks of the screening stage
        votes_ = _getVotesAtSnapshotBlocks(
            account_,
            startBlock - VOTING_POWER_SNAPSHOT_DELAY,
            startBlock
        );
    }

Please note that calling the following ExtraordinaryFunding.voteExtraordinary function when block.number equals _extraordinaryFundingProposals[proposalId_].startBlock also does not revert, and the ExtraordinaryFunding._getVotesExtraordinary function below also uses _extraordinaryFundingProposals[proposalId_].startBlock as the voteStartBlock_ input for calling the Funding._getVotesAtSnapshotBlocks function. Hence, the same issue also applies to the ExtraordinaryFunding.voteExtraordinary function.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L131-L157

    function voteExtraordinary(
        uint256 proposalId_
    ) external override returns (uint256 votesCast_) {
        // revert if msg.sender already voted on proposal
        if (hasVotedExtraordinary[proposalId_][msg.sender]) revert AlreadyVoted();

        ExtraordinaryFundingProposal storage proposal = _extraordinaryFundingProposals[proposalId_];
        // revert if proposal is inactive
        if (proposal.startBlock > block.number || proposal.endBlock < block.number || proposal.executed) {
            revert ExtraordinaryFundingProposalInactive();
        }

        // check voting power at snapshot block and update proposal votes
        votesCast_ = _getVotesExtraordinary(msg.sender, proposalId_);
        proposal.votesReceived += SafeCast.toUint120(votesCast_);

        // record that voter has voted on this extraordinary funding proposal
        hasVotedExtraordinary[proposalId_][msg.sender] = true;

        ...
    }

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L246-L256

    function _getVotesExtraordinary(address account_, uint256 proposalId_) internal view returns (uint256 votes_) {
        if (proposalId_ == 0) revert ExtraordinaryFundingProposalInactive();

        uint256 startBlock = _extraordinaryFundingProposals[proposalId_].startBlock;

        votes_ = _getVotesAtSnapshotBlocks(
            account_,
            startBlock - VOTING_POWER_SNAPSHOT_DELAY,
            startBlock
        );
    }

Proof of Concept

The following steps can occur for the described scenario.

  1. At 1 block prior to the current distribution period's start block, Alice has no available votes at all.
  2. After noticing the StandardFunding.startNewDistributionPeriod transaction that would end the previous distribution period and starts the current distribution period in the mempool, Alice backruns that transaction by minting a lot of AJNA tokens at the current distribution period's start block.
  3. When block.number becomes bigger than the current distribution period's start block during the screening phase, Alice calls the StandardFunding.screeningVote function to successfully use all of her available votes at the current distribution period's start block for a proposal.
  4. Alice's actions are unfair to Bob who prepares for the screening stage voting and maintains the same number of available votes at 1 block prior to the current distribution period's start block and at the current distribution period's start block.

Tools Used

VSCode

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L578 can be updated to the following code:

        if (block.number <= currentDistribution.startBlock || block.number > _getScreeningStageEndBlock(currentDistribution.endBlock)) revert InvalidVote();

and

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L139-L141 can be updated to the following code:

        if (proposal.startBlock >= block.number || proposal.endBlock < block.number || proposal.executed) {
            revert ExtraordinaryFundingProposalInactive();
        }

Assessed type

Timing

#0 - c4-judge

2023-05-18T16:00:57Z

Picodes marked the issue as duplicate of #355

#1 - c4-judge

2023-05-18T16:01:05Z

Picodes marked the issue as selected for report

#2 - c4-sponsor

2023-05-19T19:16:15Z

MikeHathaway marked the issue as sponsor disputed

#3 - MikeHathaway

2023-05-19T19:16:36Z

Different voting mechanisms have different voting specifications.

#4 - c4-judge

2023-05-31T14:02:42Z

Picodes marked the issue as satisfactory

#5 - Picodes

2023-05-31T14:03:57Z

@MikeHathaway I think this issue is valid: it is not about different phases but about what happens at voteStartBlock_ == block.number versus voteStartBlock_ == block.number + 1

#6 - 0xRobocop

2023-05-31T19:52:43Z

I think this issue was miss-judged

First of all the warden jumps from talking about the funding vote and then talks about the screening vote, which have different voting specifications.

Second and most important, is that warden's assumptions and examples are simply invalid. All the examples and assumptions starts that some user has no AJNA tokens 1 block prior the start block, but when the user sees that a new distribution will start, then he gets the tokens. This scenario will yield a zero voting power for the user because the voting power always uses the minimum balance of the user between block numbers. Where startBlock - VOTING_POWER_SNAPSHOT_DELAY is always one of them. The current delay is 33 blocks.

Function _getVotesScreening(uint24 distributionId_, address account_) internal view returns (uint256 votes_) {
        uint256 startBlock = _distributions[distributionId_].startBlock;

        // calculate voting weight based on the number of tokens held at the snapshot blocks of the screening stage
        votes_ = _getVotesAtSnapshotBlocks(
            account_,
            startBlock - VOTING_POWER_SNAPSHOT_DELAY,
            startBlock
        );
    }
function _getVotesAtSnapshotBlocks(
        address account_,
        uint256 snapshot_,
        uint256 voteStartBlock_
    ) internal view returns (uint256) {
        IVotes token = IVotes(ajnaTokenAddress);

        // calculate the number of votes available at the snapshot block
        uint256 votes1 = token.getPastVotes(account_, snapshot_);

        // enable voting weight to be calculated during the voting period's start block
        voteStartBlock_ = voteStartBlock_ != block.number ? voteStartBlock_ : block.number - 1;

        // calculate the number of votes available at the stage's start block
        uint256 votes2 = token.getPastVotes(account_, voteStartBlock_);

        return Maths.min(votes2, votes1);
    }

For example, Warden's proof of concept is wrong, Alice cannot get her ajna tokens at the start block and then use those tokens as voting power.

#7 - rbserver

2023-05-31T22:09:16Z

Sorry for any confusion. At 1 block prior to the current distribution period's start block, Alice has no available votes at all in the POC section means that Alice has no available votes at that particular block that is 1 block prior to the current distribution period's start block; yet, she can still have available votes at blocks before that particular block. When Alice has available votes at the block that is 33 blocks prior to the current distribution period's start block, it is possible that she uses all of her available votes at the current distribution period's start block for the scenario described in the POC section.

#8 - MikeHathaway

2023-06-01T20:46:01Z

If Alice has no voting power 1 block prior to start, than at the start block she will have no voting power if she doesn't transfer and delegate tokens to that address. Prior voting power won't matter, as we take the minimum of the prior snapshot and the current snapshot.

#9 - rbserver

2023-06-01T21:25:24Z

If Alice mints or receives AJNA tokens at the current distribution period's start block, can she gain available votes at the current distribution period's start block? If so, she can have voting power at the current distribution period's start block while has 0 available votes at 1 block prior to the current distribution period's start block. Please correct me if I am understanding this incorrect.

#10 - MikeHathaway

2023-06-01T21:53:37Z

That is incorrect, the snapshot system requires that the balance be > 0 for the entire 33 blocks prior to the distribution period's start. If any block is 0, their power is 0.

This can be verified with a quick unit test:

function testVotingPowerAtDistributionStart() external { // 14 tokenholders self delegate their tokens to enable voting on the proposals _selfDelegateVoters(_token, _votersArr); // roll 32 blocks forward to the block before the distribution period starts vm.roll(_startBlock + 32); // _tokenHolder1 transfers their tokens away address nonVotingAddress = makeAddr("nonVotingAddress"); changePrank(_tokenHolder1); _token.transfer(nonVotingAddress, 25_000_000 * 1e18); vm.roll(_startBlock + 33); // nonVotingAddress returns the funds one block later changePrank(nonVotingAddress); _token.transfer(_tokenHolder1, 25_000_000 * 1e18); // start distribution period _startDistributionPeriod(_grantFund); // check voting power of _tokenHolder1 is 0 uint256 votingPower = _getScreeningVotes(_grantFund, _tokenHolder1); assertEq(votingPower, 0); votingPower = _getScreeningVotes(_grantFund, nonVotingAddress); assertEq(votingPower, 0); }

#11 - rbserver

2023-06-01T23:36:08Z

Thanks for your reply and unit test!

The thing is that, to vote in the current distribution period, Alice does not have to vote at the current distribution period's start block and can vote at an eligible block after the current distribution period's start block as stated in Step 3 of the POC section. When voting at such eligible block, Alice can utilize the available votes that she gained at the start block. To demonstrate this, I've made some modifications to your unit test below, which does pass. Please see @audit for the places of modification. For this test, at _startBlock + 33, which is the start block, _tokenHolder1's voting power is 0; however, at _startBlock + 34 and _startBlock + 35, _tokenHolder1's voting power becomes 25_000_000 * 1e18. The inconsistency between the voting power when voting at the start block and when voting at an eligible block after the start block is the main point of this report.

    function testVotingPowerAtDistributionStart() external {
        // 14 tokenholders self delegate their tokens to enable voting on the proposals
        _selfDelegateVoters(_token, _votersArr);

        // roll 32 blocks forward to the block before the distribution period starts
        vm.roll(_startBlock + 32);

        // _tokenHolder1 transfers their tokens away
        address nonVotingAddress = makeAddr("nonVotingAddress");
        changePrank(_tokenHolder1);
        // @audit transfer _token.balanceOf(_tokenHolder1) so _tokenHolder1 does not have any AJNA at _startBlock + 32
        _token.transfer(nonVotingAddress, _token.balanceOf(_tokenHolder1));

        vm.roll(_startBlock + 33);

        // nonVotingAddress returns the funds one block later
        changePrank(nonVotingAddress);
        _token.transfer(_tokenHolder1, 25_000_000 * 1e18);

        // start distribution period
        _startDistributionPeriod(_grantFund);

        // check voting power of _tokenHolder1 is 0
        uint256 votingPower = _getScreeningVotes(_grantFund, _tokenHolder1);
        assertEq(votingPower, 0);
        votingPower = _getScreeningVotes(_grantFund, nonVotingAddress);
        assertEq(votingPower, 0);

        // @audit at _startBlock + 34, _tokenHolder1's voting power becomes 25_000_000 * 1e18, which is not 0
        vm.roll(_startBlock + 34);
        votingPower = _getScreeningVotes(_grantFund, _tokenHolder1);
        assertEq(votingPower, 25_000_000 * 1e18);

        // @audit at _startBlock + 35, _tokenHolder1's voting power is still 25_000_000 * 1e18, which is not 0
        vm.roll(_startBlock + 35);
        votingPower = _getScreeningVotes(_grantFund, _tokenHolder1);
        assertEq(votingPower, 25_000_000 * 1e18);
    }

#12 - MikeHathaway

2023-06-01T23:53:52Z

My mistake, you are correct. This is a real issue. Thank you for the report and the extra assertions!

Awards

395.9568 USDC - $395.96

Labels

bug
grade-a
QA (Quality Assurance)
selected for report
Q-28

External Links

QA REPORT

Issue
[01]CHALLENGE_PERIOD_LENGTH, DISTRIBUTION_PERIOD_LENGTH, FUNDING_PERIOD_LENGTH, AND MAX_EFM_PROPOSAL_LENGTH ARE HARDCODED BASED ON 7200 BLOCKS PER DAY
[02]AMBIGUITY IN StandardFunding._standardProposalState FUNCTION
[03]ExtraordinaryFundingProposal.votesReceived IN ExtraordinaryFunding CONTRACT IS uint120 INSTEAD OF uint128
[04]CALLING ExtraordinaryFunding.proposeExtraordinary AND StandardFunding.proposeStandard FUNCTIONS CAN REVERT AND WASTE GAS
[05]ajnaTokenAddress IS HARDCODED
[06]CODE COMMENT IN ExtraordinaryFunding._extraordinaryProposalSucceeded FUNCTION CAN BE INCORRECT
[07]CODE COMMENT FOR CHALLENGE_PERIOD_LENGTH CAN BE MORE ACCURATE
[08]MISSING address(0) CHECKS FOR CRITICAL CONSTRUCTOR INPUTS
[09]SOLIDITY VERSION 0.8.19 CAN BE USED
[10]SETTING support TO 1 WHEN voteParams_.votesUsed < 0 IS FALSE IN StandardFunding._fundingVote FUNCTION IS REDUNDANT
[11]REDUNDANT EXECUTION OF if (sumOfTheSquareOfVotesCast > type(uint128).max) revert InsufficientVotingPower() IN StandardFunding._fundingVote FUNCTION
[12]InvalidVote ERROR CAN BE MORE DESCRIPTIVE
[13]CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS
[14]UNDERSCORES CAN BE ADDED FOR NUMBERS
[15]uint256 CAN BE USED INSTEAD OF uint
[16]SPACES CAN BE ADDED FOR BETTER READABILITY

[01] CHALLENGE_PERIOD_LENGTH, DISTRIBUTION_PERIOD_LENGTH, FUNDING_PERIOD_LENGTH, AND MAX_EFM_PROPOSAL_LENGTH ARE HARDCODED BASED ON 7200 BLOCKS PER DAY

The following CHALLENGE_PERIOD_LENGTH, DISTRIBUTION_PERIOD_LENGTH, FUNDING_PERIOD_LENGTH, and MAX_EFM_PROPOSAL_LENGTH are hardcoded and assume that the number of blocks per day is 7200 and the number of seconds per block is 12. Yet, it is possible that the number of seconds per block is more or less than 12 due to network traffic and future chain upgrades. When the number of seconds per block is no longer 12, the durations corresponding to CHALLENGE_PERIOD_LENGTH, DISTRIBUTION_PERIOD_LENGTH, FUNDING_PERIOD_LENGTH, and MAX_EFM_PROPOSAL_LENGTH are no longer 7, 90, 10, and 30 days, which break the duration specifications for various phases. This can cause unexpectedness to users; for example, when the duration for FUNDING_PERIOD_LENGTH becomes less than 10 days, a user, who expects that she or he could vote on the 10th day, can fail to vote unexpectedly on that 10th day. To avoid unexpectedness and disputes, please consider using block.timestamp instead of blocks for defining durations for various phases in the StandardFunding and ExtraordinaryFunding contracts.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L29-L34

    /**
     * @notice Length of the challengephase of the distribution period in blocks.
     * @dev    Roughly equivalent to the number of blocks in 7 days.
     * @dev    The period in which funded proposal slates can be checked in updateSlate.
     */
    uint256 internal constant CHALLENGE_PERIOD_LENGTH = 50400;

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L36-L40

    /**
     * @notice Length of the distribution period in blocks.
     * @dev    Roughly equivalent to the number of blocks in 90 days.
     */
    uint48 internal constant DISTRIBUTION_PERIOD_LENGTH = 648000;

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L42-L46

    /**
     * @notice Length of the funding phase of the distribution period in blocks.
     * @dev    Roughly equivalent to the number of blocks in 10 days.
     */
    uint256 internal constant FUNDING_PERIOD_LENGTH = 72000;

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L20-L23

    /**
     * @notice The maximum length of a proposal's voting period, in blocks.
     */
    uint256 internal constant MAX_EFM_PROPOSAL_LENGTH = 216_000; // number of blocks in one month

[02] AMBIGUITY IN StandardFunding._standardProposalState FUNCTION

When block.number is bigger than _distributions[proposal.distributionId].endBlock, it is possible that the proposal is in the challenge phase. In the challenge phase, calling the following StandardFunding._standardProposalState function, which further calls the StandardFunding._standardFundingVoteSucceeded function below, can return ProposalState.Succeeded if the proposal is found in _fundedProposalSlates[_distributions[distributionId].fundedSlateHash] at that moment. However, during the challenge phase, the StandardFunding.updateSlate function below can be called to update _fundedProposalSlates for the mentioned _distributions[distributionId].fundedSlateHash]. Such update can exclude the same proposal from _fundedProposalSlates[_distributions[distributionId].fundedSlateHash]; calling the StandardFunding._standardProposalState function again would then return ProposalState.Defeated for such proposal. Hence, the StandardFunding._standardProposalState function cannot properly determine the status of the corresponding proposal in the challenge phase. To prevent users from being misled, please update the StandardFunding._standardProposalState function to return a state, which is not Succeeded or Defeated, to indicate that the proposal's success state is to be determined when the time is in the challenge phase.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L505-L512

    function _standardProposalState(uint256 proposalId_) internal view returns (ProposalState) {
        Proposal memory proposal = _standardFundingProposals[proposalId_];

        if (proposal.executed)                                                     return ProposalState.Executed;
        else if (_distributions[proposal.distributionId].endBlock >= block.number) return ProposalState.Active;
        else if (_standardFundingVoteSucceeded(proposalId_))                      return ProposalState.Succeeded;
        else                                                                       return ProposalState.Defeated;
    }

https://github.com/code-423n4/2023-05-ajna/blob/main/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;
    }

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

    function updateSlate(
        uint256[] calldata proposalIds_,
        uint24 distributionId_
    ) external override returns (bool newTopSlate_) {
        ...
        // 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
            );
        }
    }

[03] ExtraordinaryFundingProposal.votesReceived IN ExtraordinaryFunding CONTRACT IS uint120 INSTEAD OF uint128

In the StandardFunding contract, Proposal.votesReceived is uint128.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/interfaces/IStandardFunding.sol#L122-L129

    struct Proposal {
        ...
        uint128 votesReceived;        // accumulator of screening votes received by a proposal
        ...
    }

Yet, in the ExtraordinaryFunding contract, ExtraordinaryFundingProposal.votesReceived is uint120. Calling the ExtraordinaryFunding.voteExtraordinary function below by a user, who has the voting power being more than type(uint120).max, can revert, which causes such user to waste gas and fail to vote for the corresponding proposal. This degrades the user experience because such user, who is familiar with Proposal.votesReceived in the StandardFunding contract, could think that ExtraordinaryFundingProposal.votesReceived in the ExtraordinaryFunding contract would be uint128 as well. To be more consistent and prevent disputes, please consider using ExtraordinaryFundingProposal.votesReceived as uint128 instead of uint120 in the ExtraordinaryFunding contract.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/interfaces/IExtraordinaryFunding.sol#L32-L39

    struct ExtraordinaryFundingProposal {
        ...
        uint120  votesReceived;   // Total votes received for this proposal.
        ...
    }

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L131-L157

    function voteExtraordinary(
        uint256 proposalId_
    ) external override returns (uint256 votesCast_) {
        ...
        // check voting power at snapshot block and update proposal votes
        votesCast_ = _getVotesExtraordinary(msg.sender, proposalId_);
        proposal.votesReceived += SafeCast.toUint120(votesCast_);
        ...
    }

[04] CALLING ExtraordinaryFunding.proposeExtraordinary AND StandardFunding.proposeStandard FUNCTIONS CAN REVERT AND WASTE GAS

Calling the following ExtraordinaryFunding.proposeExtraordinary and StandardFunding.proposeStandard functions will call the Funding._validateCallDatas function below. Calling the Funding._validateCallDatas function will revert if targets_[i] != ajnaTokenAddress || values_[i] != 0 is true. Hence, the ExtraordinaryFunding.proposeExtraordinary and StandardFunding.proposeStandard functions cannot be used to create proposals for calling addresses other than ajnaTokenAddress or sending ETH. When users are unaware of this, they could believe that proposals for general purposes can be created and executed; yet, because calling ExtraordinaryFunding.proposeExtraordinary and StandardFunding.proposeStandard functions with targets_ being not ajnaTokenAddress or values_ being positive revert, these users would waste their gas for nothing. To avoid confusion and disputes, please consider updating the ExtraordinaryFunding.proposeExtraordinary and StandardFunding.proposeStandard functions so targets_ and values_ would only be ajnaTokenAddress and 0 and not be specified by users.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L85-L124

    function proposeExtraordinary(
        uint256 endBlock_,
        address[] memory targets_,
        uint256[] memory values_,
        bytes[] memory calldatas_,
        string memory description_) external override returns (uint256 proposalId_) {
        ...
        uint128 totalTokensRequested = _validateCallDatas(targets_, values_, calldatas_);
        ...
    }

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L366-L404

    function proposeStandard(
        address[] memory targets_,
        uint256[] memory values_,
        bytes[] memory calldatas_,
        string memory description_
    ) external override returns (uint256 proposalId_) {
        ...
        newProposal.tokensRequested = _validateCallDatas(targets_, values_, calldatas_); // check proposal parameters are valid and update tokensRequested
        ...
    }

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/Funding.sol#L103-L141

    function _validateCallDatas(
        address[] memory targets_,
        uint256[] memory values_,
        bytes[] memory calldatas_
    ) internal view returns (uint128 tokensRequested_) {

        // check params have matching lengths
        if (targets_.length == 0 || targets_.length != values_.length || targets_.length != calldatas_.length) revert InvalidProposal();

        for (uint256 i = 0; i < targets_.length;) {

            // check targets and values params are valid
            if (targets_[i] != ajnaTokenAddress || values_[i] != 0) revert InvalidProposal();
            ...
        }
    }

[05] ajnaTokenAddress IS HARDCODED

The following ajnaTokenAddress is hardcoded. It is not AJNA's address on chains like Polygon, Arbitrum, and Optimism. This means that the functionalities that rely on ajnaTokenAddress will break on these chains in which the GrantFund contract, which inherits the Funding contract, cannot be used on these chains. To be more future-proofed, please consider adding a function that is only callable by the trusted admin for updating ajnaTokenAddress instead of relying on a hardcoded ajnaTokenAddress.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/Funding.sol#L21

    address public immutable ajnaTokenAddress = 0x9a96ec9B57Fb64FbC60B423d1f4da7691Bd35079;

[06] CODE COMMENT IN ExtraordinaryFunding._extraordinaryProposalSucceeded FUNCTION CAN BE INCORRECT

In the following ExtraordinaryFunding._extraordinaryProposalSucceeded function, the comment for (votesReceived >= tokensRequested_ + _getSliceOfNonTreasury(minThresholdPercentage)) is succeeded if proposal's votes received doesn't exceed the minimum threshold required. However, (votesReceived >= tokensRequested_ + _getSliceOfNonTreasury(minThresholdPercentage)) can only be true when the proposal's votes received meet or exceed such minimum threshold, which is the opposite of the comment. To prevent confusion, please consider updating the comment to match the corresponding code.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L164-L178

    function _extraordinaryProposalSucceeded(
        uint256 proposalId_,
        uint256 tokensRequested_
    ) internal view returns (bool) {
        uint256 votesReceived          = uint256(_extraordinaryFundingProposals[proposalId_].votesReceived);
        uint256 minThresholdPercentage = _getMinimumThresholdPercentage();

        return
            // succeeded if proposal's votes received doesn't exceed the minimum threshold required
            (votesReceived >= tokensRequested_ + _getSliceOfNonTreasury(minThresholdPercentage))
            &&
            // succeeded if tokens requested are available for claiming from the treasury
            (tokensRequested_ <= _getSliceOfTreasury(Maths.WAD - minThresholdPercentage))
        ;
    }

[07] CODE COMMENT FOR CHALLENGE_PERIOD_LENGTH CAN BE MORE ACCURATE

The challenge phase starts after the time for DISTRIBUTION_PERIOD_LENGTH is finished. The time for CHALLENGE_PERIOD_LENGTH is more of an addition to the distribution period instead of part of the distribution period. Thus, describing CHALLENGE_PERIOD_LENGTH as Length of the challengephase of the distribution period given that DISTRIBUTION_PERIOD_LENGTH is described as Length of the distribution period is somewhat misleading. To avoid confusion, the comment for CHALLENGE_PERIOD_LENGTH can be updated to indicate that CHALLENGE_PERIOD_LENGTH is not included in DISTRIBUTION_PERIOD_LENGTH.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L29-L34

    /**
     * @notice Length of the challengephase of the distribution period in blocks.
     * @dev    Roughly equivalent to the number of blocks in 7 days.
     * @dev    The period in which funded proposal slates can be checked in updateSlate.
     */
    uint256 internal constant CHALLENGE_PERIOD_LENGTH = 50400;

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L36-L40

    /**
     * @notice Length of the distribution period in blocks.
     * @dev    Roughly equivalent to the number of blocks in 90 days.
     */
    uint48 internal constant DISTRIBUTION_PERIOD_LENGTH = 648000;

[08] MISSING address(0) CHECKS FOR CRITICAL CONSTRUCTOR INPUTS

To prevent unintended behaviors, critical constructor inputs should be checked against address(0).

address(0) checks are missing for erc20Factory_ and erc721Factory_ in the following constructor. Please consider checking them.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L116-L122

    constructor(
        ERC20PoolFactory erc20Factory_,
        ERC721PoolFactory erc721Factory_
    ) PermitERC721("Ajna Positions NFT-V1", "AJNA-V1-POS", "1") {
        erc20PoolFactory  = erc20Factory_;
        erc721PoolFactory = erc721Factory_;
    }

address(0) check is missing for positionManager_ in the following constructor. Please consider checking it.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/RewardsManager.sol#L95-L100

    constructor(address ajnaToken_, IPositionManager positionManager_) {
        if (ajnaToken_ == address(0)) revert DeployWithZeroAddress();

        ajnaToken = ajnaToken_;
        positionManager = positionManager_;
    }

[09] SOLIDITY VERSION 0.8.19 CAN BE USED

Using the more updated version of Solidity can enhance security. As described in https://github.com/ethereum/solidity/releases, Version 0.8.19 is the latest version of Solidity, which "contains a fix for a long-standing bug that can result in code that is only used in creation code to also be included in runtime bytecode". To be more secured and more future-proofed, please consider using Version 0.8.19 for the following contracts.

ajna-core\src\PositionManager.sol
  3: pragma solidity 0.8.14;

ajna-core\src\RewardsManager.sol
  3: pragma solidity 0.8.14;

ajna-grants\src\grants\GrantFund.sol
  3: pragma solidity 0.8.16;

ajna-grants\src\grants\base\ExtraordinaryFunding.sol
  3: pragma solidity 0.8.16;

ajna-grants\src\grants\base\Funding.sol
  3: pragma solidity 0.8.16;

ajna-grants\src\grants\base\StandardFunding.sol
  3: pragma solidity 0.8.16;

ajna-grants\src\grants\libraries\Maths.sol
  2: pragma solidity 0.8.16;

[10] SETTING support TO 1 WHEN voteParams_.votesUsed < 0 IS FALSE IN StandardFunding._fundingVote FUNCTION IS REDUNDANT

When calling the following StandardFunding._fundingVote function, uint8 support = 1 is executed before voteParams_.votesUsed < 0 ? support = 0 : support = 1. Therefore, when voteParams_.votesUsed < 0 is false, support does not need to be set to 1 again. Please consider refactoring the code to only update support to 0 when voteParams_.votesUsed < 0 is true.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L612-L690

    function _fundingVote(
        QuarterlyDistribution storage currentDistribution_,
        Proposal storage proposal_,
        address account_,
        QuadraticVoter storage voter_,
        FundingVoteParams memory voteParams_
    ) internal returns (uint256 incrementalVotesUsed_) {
        uint8  support = 1;
        uint256 proposalId = proposal_.proposalId;

        // determine if voter is voting for or against the proposal
        voteParams_.votesUsed < 0 ? support = 0 : support = 1;
        ...
    }

[11] REDUNDANT EXECUTION OF if (sumOfTheSquareOfVotesCast > type(uint128).max) revert InsufficientVotingPower() IN StandardFunding._fundingVote FUNCTION

The following StandardFunding._fundingVote function executes if (sumOfTheSquareOfVotesCast > type(uint128).max) revert InsufficientVotingPower() before uint128 cumulativeVotePowerUsed = SafeCast.toUint128(sumOfTheSquareOfVotesCast). Because calling the SafeCast.toUint128 function below would revert when sumOfTheSquareOfVotesCast > type(uint128).max is true, executing if (sumOfTheSquareOfVotesCast > type(uint128).max) revert InsufficientVotingPower() becomes redundant. Please consider removing if (sumOfTheSquareOfVotesCast > type(uint128).max) revert InsufficientVotingPower() from the StandardFunding._fundingVote function.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L612-L690

    function _fundingVote(
        QuarterlyDistribution storage currentDistribution_,
        Proposal storage proposal_,
        address account_,
        QuadraticVoter storage voter_,
        FundingVoteParams memory voteParams_
    ) internal returns (uint256 incrementalVotesUsed_) {
        ...
        // calculate the cumulative cost of all votes made by the voter
        // and check that attempted votes cast doesn't overflow uint128
        uint256 sumOfTheSquareOfVotesCast = _sumSquareOfVotesCast(votesCast);
        if (sumOfTheSquareOfVotesCast > type(uint128).max) revert InsufficientVotingPower();
        uint128 cumulativeVotePowerUsed = SafeCast.toUint128(sumOfTheSquareOfVotesCast);
        ...
    }

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/math/SafeCast.sol#L290-L293

    function toUint128(uint256 value) internal pure returns (uint128) {
        require(value <= type(uint128).max, "SafeCast: value doesn't fit in 128 bits");
        return uint128(value);
    }

[12] InvalidVote ERROR CAN BE MORE DESCRIPTIVE

The following StandardFunding.fundingVote and StandardFunding.screeningVote functions can revert with the InvalidVote error for various reasons. Yet, executing revert InvalidVote() does not describe the specific reason. To be more descriptive and user-friendly, please consider updating the InvalidVote error so it can provide the reason why calling the corresponding function reverts.

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L519-L569

    function fundingVote(
        FundingVoteParams[] memory voteParams_
    ) external override returns (uint256 votesCast_) {
        ...
        uint256 endBlock = currentDistribution.endBlock;

        uint256 screeningStageEndBlock = _getScreeningStageEndBlock(endBlock);

        // check that the funding stage is active
        if (block.number <= screeningStageEndBlock || block.number > endBlock) revert InvalidVote();
        ...
        for (uint256 i = 0; i < numVotesCast; ) {
            Proposal storage proposal = _standardFundingProposals[voteParams_[i].proposalId];

            // check that the proposal is part of the current distribution period
            if (proposal.distributionId != currentDistributionId) revert InvalidVote();

            // check that the proposal being voted on is in the top ten screened proposals
            if (_findProposalIndex(voteParams_[i].proposalId, _topTenProposals[currentDistributionId]) == -1) revert InvalidVote();
            ...
        }
    }

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L572-L596

    function screeningVote(
        ScreeningVoteParams[] memory voteParams_
    ) external override returns (uint256 votesCast_) {
        QuarterlyDistribution memory currentDistribution = _distributions[_currentDistributionId];

        // check screening stage is active
        if (block.number < currentDistribution.startBlock || block.number > _getScreeningStageEndBlock(currentDistribution.endBlock)) revert InvalidVote();

        uint256 numVotesCast = voteParams_.length;

        for (uint256 i = 0; i < numVotesCast; ) {
            Proposal storage proposal = _standardFundingProposals[voteParams_[i].proposalId];

            // check that the proposal is part of the current distribution period
            if (proposal.distributionId != currentDistribution.id) revert InvalidVote();
            ...
        }
    }

[13] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

( Please note that the following instances are not found in https://gist.github.com/CloudEllie/a4655b833548ed9a86a63eb7292bcc0f#n04-constants-should-be-defined-rather-than-using-magic-numbers. )

To improve readability and maintainability, a constant can be used instead of the magic number. Please consider replacing the magic numbers, such as 2, used in the following code with constants.

ajna-grants\src\grants\base\StandardFunding.sol
  292: ) / 10;
  849: votesCastSumSquared_ += Maths.wpow(SafeCast.toUint256(Maths.abs(votesCast_[i].votesUsed)), 2);
  908: ), 2);

[14] UNDERSCORES CAN BE ADDED FOR NUMBERS

It is a common practice to separate each 3 digits in a number by an underscore to improve code readability. Unlike MAX_EFM_PROPOSAL_LENGTH below, the following CHALLENGE_PERIOD_LENGTH, DISTRIBUTION_PERIOD_LENGTH, and FUNDING_PERIOD_LENGTH do not use underscores; please consider adding underscores for these numbers.

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

    uint256 internal constant CHALLENGE_PERIOD_LENGTH = 50400;

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

    uint48 internal constant DISTRIBUTION_PERIOD_LENGTH = 648000;

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

    uint256 internal constant FUNDING_PERIOD_LENGTH = 72000;

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/ExtraordinaryFunding.sol#L23

    uint256 internal constant MAX_EFM_PROPOSAL_LENGTH = 216_000; // number of blocks in one month

[15] uint256 CAN BE USED INSTEAD OF uint

Both uint and uint256 are used in the protocol's code. In favor of explicitness, please consider using uint256 instead of uint in the following code.

ajna-grants\src\grants\base\StandardFunding.sol
  208: for (uint i = 0; i < numFundedProposals; ) {
  324: for (uint i = 0; i < numProposalsInSlate; ) {
  434: for (uint i = 0; i < numProposalsInSlate_; ) {
  468: for (uint i = 0; i < numProposals; ) {
  469: for (uint j = i + 1; j < numProposals; ) {
  491: for (uint i = 0; i < proposalIdSubset_.length;) {

[16] SPACES CAN BE ADDED FOR BETTER READABILITY

For better readability, spaces can be added in code where appropriate.

A space can be added between challenge and phase in the following comment. https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L30

     * @notice Length of the challengephase of the distribution period in blocks.

A space can be added between returns and (uint256 in the following code. https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L238

    ) external override returns(uint256 rewardClaimed_) {

A space can be added between currentSlateHash and != in the following code. https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/StandardFunding.sol#L318

            (currentSlateHash!= 0 && sum > _sumProposalFundingVotes(_fundedProposalSlates[currentSlateHash]));

#0 - c4-judge

2023-05-18T18:59:30Z

Picodes marked the issue as grade-a

#1 - c4-judge

2023-05-31T14:30:54Z

Picodes marked the issue as selected for report

#2 - Picodes

2023-06-29T05:43:02Z

5, 8, 9, 13 can be removed for the final report as they don't add much value. It'd be interesting to add #296, #286 and #290 to the report as they are interesting Low Sev findings.

#3 - Picodes

2023-06-29T05:43:11Z

5, 8, 9, 13 can be removed for the final report as they don't add much value. It'd be interesting to add #296, #286 and #290 to the report as they are interesting Low Sev findings.

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter