Ajna Protocol - descharre's results

Summary

Low Risk

Non critical

Details

Low Risk

[L-01] Treasury can be wrong if funded proposal is not executed

The function _updateTreasury() doesn't check if a proposal is executed. It updates it's amount by all the tokens requested from the distributions slate. However when a proposal is gonna be executed it's still possible to revert for various reasons. For example the contract that the proposal want to use reverts on _beforeTokenTransfer. Or the contract is paused and doesn't allow any tokens to be sent to the contract.

When this proposal will not be executed in the future, the treasury will be forever wrong. Resulting in more tokens in the treasury than the treasury variable is.

A mitigation for this is to check if a proposal is executed and then decrement the treasury with the tokens requested from said proposal.

[L-02] Proposal parameters are not correctly checked

The function _validateCallDatas() checks the parameters of a new proposal.

The line below checks if the target is equal to the address of the ajna token, or if the value is 0. However the target should always be the ajna token and a transfer of value 0 will be pretty stupid. It should be check if both are true or else revert.

-   if (targets_[i] != ajnaTokenAddress || values_[i] != 0) revert InvalidProposal();
+   if (targets_[i] != ajnaTokenAddress && values_[i] != 0) revert InvalidProposal();

Non critical

[N-01] Use a more recent version of solidity

The contracts in ajna-core use solidity version 0.8.14. Later release versions have new features and bug fixes. Consider using a later version.

[N-02] startNewDistributionPeriod() will likely be called everytime before challenge period ends

The challenge period is a period of 7 days after the funding stage has ended. However the startNewDistributionPeriod() can be called by everyone immeditaly after the funding stage has ended. This means the function most likely will not be called after the challenge stage has ended and the first if statement will never be true. This isn't a big problem because it will just be called the next time the distribution gets incremented, so the first if statement to update the treasury can be removed.

StandardFunding.sol
L127:
        {
            // 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);
            }
        }

[N-03] Typos

• StandardFunding.sol#L216: // readd non distributed tokens to the treasury: readd
• StandardFunding.sol#L632: // check that the voter hasn't already voted on a proposal by seeing if it's already in the votesCast array: should be this proposal
• PositionManager.sol#L352: function reedemPositions(: should be redeem

[N-04] Require or if statements that check input arguments should be at the top of the function

StandardFunding.sol#L423-L425: check for challenge period should be in the updateSlate() function, this also saves an extra function parameter.

[N-05] Unnecessary code

StandardFunding.sol#L317-L318:

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

The statement currentSlateHash!= 0 will always be true when currentSlateHash == 0 is not true. Hereby currentSlateHash!= 0 can be removed.

[N-06] Don't use msg.sender as a parameter for an internal function that's only used once

When an internal function is only used once, it's unnecessary to pass msg.sender as parameter because msg.sender is a global variable and it will always be the same. This counts for _fundingVote() and _screeningVote

[N-07] block.number is added to event information by default

block.number is added to an events information by default, hence why it can be removed from the emit statement.

IFunding:
L54:
    event ProposalCreated(
        uint256 proposalId,
        address proposer,
        address[] targets,
        uint256[] values,
        string[] signatures,
        bytes[] calldatas,
-       uint256 startBlock,
        uint256 endBlock,
        string description
    );

StandardFunding:
L393:
    emit ProposalCreated(
        proposalId_,
        msg.sender,
        targets_,
        values_,
        new string[](targets_.length),
        calldatas_,
-       block.number,
        currentDistribution.endBlock,
        description_
    );

block.number can also be removed at:

• ExtraordinaryFunding.sol#L113-L123

Summary

Details

[G-01] Do budget check at the end of the for loop

The if statement to check if the slate has exceeded the budget in _validateSlate() is now in every iteration. Gas can be saved when you put it at the end of the for loop. StandardFunding.sol#L421-L454

        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 )
-           if (totalTokensRequested > (gbc * 9 / 10)) {
-               revert InvalidProposalSlate();
-           }

            unchecked { ++i; }
        }
+       if (totalTokensRequested > (gbc * 9 / 10)) {
+           revert InvalidProposalSlate();
+       }

[G-02] Use constants instead of type(uintx).max

It's more gas efficiënt to use a constant instead of a max from a type. The constant won't be pretty if it's a high value but it will save around 20 gas everytime the function gets called.

StandardFunding.sol#L659

        if (sumOfTheSquareOfVotesCast > type(uint128).max) revert InsufficientVotingPower();

[G-03] Remove block.number from events

block.number is added by default to event information. Around 300 gas can be saved if you remove block.number when emitting events.

IFunding:
L54:
    event ProposalCreated(
        uint256 proposalId,
        address proposer,
        address[] targets,
        uint256[] values,
        string[] signatures,
        bytes[] calldatas,
-       uint256 startBlock,
        uint256 endBlock,
        string description
    );

StandardFunding:
L393:
    emit ProposalCreated(
        proposalId_,
        msg.sender,
        targets_,
        values_,
        new string[](targets_.length),
        calldatas_,
-       block.number,
        currentDistribution.endBlock,
        description_
    );

Same optimization can be done at:

• ExtraordinaryFunding.sol#L113-L123

[G-04] Use double if statement instead of &&

When an if or require statement uses &&, it's more gas efficiënt to have two if/require statements

            if (currentDistributionId > 0 && (block.number > _getChallengeStageEndBlock(currentDistributionEndBlock))) {
                // Add unused funds from last distribution to treasury
                _updateTreasury(currentDistributionId); 
            }

Can be changed to:

            if (currentDistributionId > 0 ) {
                if((block.number > _getChallengeStageEndBlock(currentDistributionEndBlock))){
                    // Add unused funds from last distribution to treasury
                    _updateTreasury(currentDistributionId); 
                }
            }

Same optimization can be made at:

• StandardFunding.sol#L129-L135
• StandardFunding.sol#L719-L724

[G-05] Make up to 3 fields in an event indexed

An event can have up to 3 fields indexed. For each fields that's indexed, around 100 gas can be saved when emitting that event. So for 3 fields extra, this means 300 gas saved. This omptimization can be done at almost every event in the protocol.