Ajna Protocol - bytes032's results

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/RewardsManager.sol#L831-L843

Vulnerability details

Impact

Users may lose rewards if the contract is underfunded during the claiming process.

Proof of Concept

The RewardsManager contract keeps track of the rewards earned by an NFT staker. The accumulated rewards are transferred to the user through the _transferAjnaRewards function.

Rewards are usually distributed by the claimRewards function, but that also happens

   function claimRewards(
        uint256 tokenId_,
        uint256 epochToClaim_
    ) external override {
        StakeInfo storage stakeInfo = stakes[tokenId_];

        if (msg.sender != stakeInfo.owner) revert NotOwnerOfDeposit();

        if (isEpochClaimed[tokenId_][epochToClaim_]) revert AlreadyClaimed();

        _claimRewards(stakeInfo, tokenId_, epochToClaim_, true, stakeInfo.ajnaPool);
    }

when unstaking

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/RewardsManager.sol#L282-L299

 function unstake(
        uint256 tokenId_
    ) external override {
        StakeInfo storage stakeInfo = stakes[tokenId_];

        if (msg.sender != stakeInfo.owner) revert NotOwnerOfDeposit();

        address ajnaPool = stakeInfo.ajnaPool;

        // claim rewards, if any
        _claimRewards(
            stakeInfo,
            tokenId_,
            IPool(ajnaPool).currentBurnEpoch(),
            false,
            ajnaPool
        );

and moving liquidity

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/RewardsManager.sol#L136-L161

 function moveStakedLiquidity(
        uint256 tokenId_,
        uint256[] memory fromBuckets_,
        uint256[] memory toBuckets_,
        uint256 expiry_
    ) external nonReentrant override {
        StakeInfo storage stakeInfo = stakes[tokenId_];

        if (msg.sender != stakeInfo.owner) revert NotOwnerOfDeposit();

        // check move array sizes match to be able to match on index
        uint256 fromBucketLength = fromBuckets_.length;

        if (fromBucketLength != toBuckets_.length) revert MoveStakedLiquidityInvalid();

        address ajnaPool = stakeInfo.ajnaPool;
        uint256 curBurnEpoch = IPool(ajnaPool).currentBurnEpoch();

        // claim rewards before moving liquidity, if any
        _claimRewards(
            stakeInfo,
            tokenId_,
            curBurnEpoch,
            false,
            ajnaPool
        );

The issue here lies within the _transferAjnaRewards function. However, the transferrable amount of Ajna token rewards are capped at the Ajna token balance at the time of claiming.

If the accumulated rewards are higher than the Ajna token balance, the claimer will receive fewer rewards than expected.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/RewardsManager.sol#L811-L821

    function _transferAjnaRewards(uint256 rewardsEarned_) internal {
        // check that rewards earned isn't greater than remaining balance
        // if remaining balance is greater, set to remaining balance

        uint256 ajnaBalance = IERC20(ajnaToken).balanceOf(address(this));
        // @audit system doesn't track partially paid rewards
        if (rewardsEarned_ > ajnaBalance) rewardsEarned_ = ajnaBalance;

        if (rewardsEarned_ != 0) {
            // transfer rewards to sender
            IERC20(ajnaToken).safeTransfer(msg.sender, rewardsEarned_);
        }
    }

There is a scenario, where this will lead to loss of funds:

Consider the following scenario:

1. Charlotte stakes
2. Certain conditions are met and Charlotte is due a reward of 40 tokens.
3. Charlotte calls claimRewards, but only gets 20 tokens, because the balance of the contract is insufficient. This means the next call to claimRewards will revert, because

if (isEpochClaimed[tokenId_][epochToClaim_]) will return true

4. Charlotte decides to call unstake, because that's the only other way she could get her tokens back. However, that won't happen because of the way that rewards are calculated.

When _claimRewards is called, it calculates the due reward amount by calling the _calculateAndClaimRewards function.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/RewardsManager.sol#L561-L578

    function _claimRewards(
        StakeInfo storage stakeInfo_,
        uint256 tokenId_,
        uint256 epochToClaim_,
        bool validateEpoch_,
        address ajnaPool_
    ) internal {

        // revert if higher epoch to claim than current burn epoch
        if (validateEpoch_ && epochToClaim_ > IPool(ajnaPool_).currentBurnEpoch()) revert EpochNotAvailable();

        // update bucket exchange rates and claim associated rewards
        uint256 rewardsEarned = _updateBucketExchangeRates(
            ajnaPool_,
            positionManager.getPositionIndexes(tokenId_)
        );

        rewardsEarned += _calculateAndClaimRewards(tokenId_, epochToClaim_);

For the function to calculate a reward, lastClaimedEpoch has to be less than epochToClaim.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/RewardsManager.sol#L384-L396

  function _calculateAndClaimRewards(
        uint256 tokenId_,
        uint256 epochToClaim_
    ) internal returns (uint256 rewards_) {

        address ajnaPool         = stakes[tokenId_].ajnaPool;
        uint256 lastClaimedEpoch = stakes[tokenId_].lastClaimedEpoch;
        uint256 stakingEpoch     = stakes[tokenId_].stakingEpoch;

        uint256[] memory positionIndexes = positionManager.getPositionIndexesFiltered(tokenId_);

        // iterate through all burn periods to calculate and claim rewards
        for (uint256 epoch = lastClaimedEpoch; epoch < epochToClaim_; ) {

But that's not the case right now, because _claimRewards already has set the last claimed epoch to the current one.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/RewardsManager.sol#L594

stakeInfo_.lastClaimedEpoch = uint96(epochToClaim_);

5. Since Alice's stake is deleted, she looses her reward and will never stake with Ajna again.

I've further confirmed with the sponsor that the only way the users are getting their full rewards is waiting for the rewards manager to be funded again.

Here's a PoC which showcases the issue:

function testPartialRewards() external {
        skip(10);

        // configure NFT position
        uint256[] memory depositIndexes = new uint256[](5);
        depositIndexes[0] = 9;
        depositIndexes[1] = 1;
        depositIndexes[2] = 2;
        depositIndexes[3] = 3;
        depositIndexes[4] = 4;

        // mint memorialize and deposit NFT
        uint256 tokenIdOne = _mintAndMemorializePositionNFT({
            indexes:    depositIndexes,
            minter:     _minterOne,
            mintAmount: 1_000 * 1e18,
            pool:       address(_pool)
        });

        // stake NFT
        _stakeToken({
            pool:    address(_pool),
            owner:   _minterOne,
            tokenId: tokenIdOne
        });


        // borrower takes actions providing reserves enabling reserve auctions
        uint256 firstTokensToBurn = _triggerReserveAuctions({
            borrower:     _borrower,
            tokensToBurn: 81.799378162662704349 * 1e18,
            borrowAmount: 300 * 1e18,
            limitIndex:   3,
            pool:         address(_pool)
        });


        // second depositor deposits an NFT representing the same positions into the rewards contract
        uint256 tokenIdTwo = _mintAndMemorializePositionNFT({
            indexes: depositIndexes,
            minter: _minterTwo,
            mintAmount: 1000 * 1e18,
            pool: address(_pool)
        });

        // second depositor stakes NFT, generating an update reward
        _stakeToken({
            pool:    address(_pool),
            owner:   _minterTwo,
            tokenId: tokenIdTwo
        });


        // Set balance to 20 tokens
        deal(address(_ajna), address(_rewardsManager), 20 * 10**18);

        address prePrankAddress = msg.sender;

        console.log("Claimable rewards by minter", _rewardsManager.calculateRewards(tokenIdOne, IPool(_pool).currentBurnEpoch()));
        console.log("RewardsManager balance", _ajnaToken.balanceOf(address(_rewardsManager)));

        // Claim reward

        changePrank(_minterOne);
        _rewardsManager.claimRewards(tokenIdOne, IPool(_pool).currentBurnEpoch());

        console.log("Claimed rewards by minter: %s", _ajnaToken.balanceOf(_minterOne));

        // Add 20 more tokens
        deal(address(_ajna), address(_rewardsManager), 50 * 10**18);
        

         changePrank(_minterOne);
        // _rewardsManager.claimRewards(tokenIdOne, IPool(_pool).currentBurnEpoch());
        _rewardsManager.unstake(tokenIdOne);

        console.log("Claimed rewards by minter: %s", _ajnaToken.balanceOf(_minterOne));
    }

Running the test, yields the following result:

Tools Used

Manual review

Recommended Mitigation Steps

One of the options would be to revert if there's not enough balance. However, that might open a can of worms where users cannot unstake, because there's unsufficient balance in the contract.

So, my recommendation here would be to change the default behavior of unstake to revert if there are not enough rewards, but allow them to opt in to unstake without rewards in an emergency situation when a bug in rewards manager fails to give rewards or has no balance.

Assessed type

Token-Transfer

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

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/RewardsManager.sol#L310-L318

Vulnerability details

Impact

This vulnerability allows malicious actors to exploit the reward system by frontrunning transactions and unfairly claiming rewards, thereby disincentivizing honest users from updating the bucket exchange rates and contributing to the system.

Proof of Concept

The updateBucketExchangeRatesAndClaim function is publicly callable and serves two main purposes:

1. Updates the exchange rate of a list of buckets.
2. If eligible, the caller can claim 5% of the rewards accumulated to each bucket since the last burn event, if it hasn't already been updated. https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/RewardsManager.sol#L310-L318

function updateBucketExchangeRatesAndClaim(
        address pool_,
        uint256[] calldata indexes_
    ) external override returns (uint256 updateReward) {
        updateReward = _updateBucketExchangeRates(pool_, indexes_);

        // transfer rewards to sender
        _transferAjnaRewards(updateReward);
    }

So, to summarize it's primary purpose is to incentivize people who keep the state updated. However, given the nature of the function (first come, first serve) it becomes very prone to MEV.

Consider the following scenario:

1. Alice is hard-working, non-technical and constantly keeps track of when to update the buckets so she can claim a small reward. Unfortunately, she becomes notorious for getting most of the rewards from updating the bucket exchange rate.
2. A malicious actor spots Alice's recent gains and creates a bot to front run any transactions to RewardsManager's _updateBucketExchangeRateAndCalculateRewardssubmitted by Alice.
3. The day after that, Alice again see's theres a small reward to claim, attempts to claim it, but she gets front runned by whoever set the bot.
4. Since Alice is non-technical, she cannot ever beat the bot so she is left with a broken heart and no longer able to claim rewards.

I believe the system should be made fair to everybody that wants to contribute, hence this is a vulnerability that should be taken care of to ensure the fair distribution of awards to people who care about the protocol instead of .

Tools Used

Manual Review

Recommended Mitigation Steps

I see potentially two solutions here:

1. Introduce a randomized reward mechanism, such as a lottery system or a probabilistic reward distribution for people who contribute to updating buckets. This could reduce the predictability of rewards and hence the potential for MEV exploitation.
2. Consider limiting the reward claim process to users who have staked in the rewards manager because they are the individuals that are directly affected if the bucket is not updated, because if its not updated for 14 days they won't be getting rewards. Additionally, you can couple it with a rate-limitting mechanism by implementing a maximum claim per address per time period

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

This vulnerability presents a significant risk to the Ajna treasury. A malicious actor who owns a substantial amount of tokens could manipulate the voting mechanism by burning their own tokens, thereby lowering the minimum threshold of votes required for a proposal to pass. This tactic could allow him to siphon off substantial amounts from the treasury.

Proof of Concept

By meeting a certain quorum of non-treasury tokens, token holders may take tokens from the treasury outside of the PFM by utilizing Extraordinary Funding Mechanism (EFM).

This mechanism works by allowing up to the percentage over 50% of non-treasury tokens (the “minimum threshold”) that vote affirmatively to be removed from the treasury – the cap on this mechanism is therefore 100% minus the minimum threshold (50% in this case).

Examples:

1. If 51% of non-treasury tokens vote affirmatively for a proposal, up to 1% of the treasury may be withdrawn by the proposal
2. If 65% of non-treasury tokens vote affirmatively for a proposal, up to 15% of the treasury may be withdrawn by the proposal
3. If 50% or less of non-treasury tokens vote affirmatively for a proposal, 0% of the treasury may be withdrawn by the proposal

When submitting a proposal, the proposer must include the exact percentage of the treasury they would like to extract (“proposal threshold”), if the vote fails to reach this threshold, it will fail, and no tokens will be distributed.

Example: a. A proposer requests 10% of the treasury

1. 50%+10%=60%
2. If 65% of non-treasury tokens vote affirmatively, 10% of the treasury is released
3. If 59.9% of non-treasury tokens vote affirmatively, 0% of the treasury is released

The function that checks the conditions above are true, and the proposal has succeeded is _extraordinaryProposalSucceeded.

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

        // @audit-info check _getMinimumThresholdPercentage() function
        uint256 minThresholdPercentage = _getMinimumThresholdPercentage();

        return
            // succeeded if proposal's votes received doesn't exceed the minimum threshold required

            // @audit-info check _getSliceOfNonTreasury() function
            (votesReceived >= tokensRequested_ + _getSliceOfNonTreasury(minThresholdPercentage))
            &&
            // succeeded if tokens requested are available for claiming from the treasury
            (tokensRequested_ <= _getSliceOfTreasury(Maths.WAD - minThresholdPercentage))
        ;
    }

The vulnerability here lies in the _getSliceOfNonTreasury() function.

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

    function _getSliceOfNonTreasury(
        uint256 percentage_
    ) internal view returns (uint256) {
        uint256 totalAjnaSupply = IERC20(ajnaTokenAddress).totalSupply();
        // return ((ajnaTotalSupply - treasury) * percentage + 10**18 / 2) / 10**18;
        return Maths.wmul(totalAjnaSupply - treasury, percentage_);
    }

The reason is that it relies on the current total supply and AjnaToken inherits ERC20Burnable, a malicious user can burn his tokens to lower the minimum threshold needed for votes and make the proposal pass.

Bob, a token holder, owns 10% of the Ajna supply. He creates a proposal where he requests 20% of the treasury. For his proposal to pass, Bob needs to gather 70% of the votes (50% as the threshold because there are no other funded proposals yet and an additional 20% for the tokens he requested). Unfortunately, Bob only manages to acquire 61% of the total votes.

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

    function _getMinimumThresholdPercentage() internal view returns (uint256) {
        // default minimum threshold is 50
        if (_fundedExtraordinaryProposals.length == 0) {
            return 0.5 * 1e18;
        }
        // minimum threshold increases according to the number of funded EFM proposals
        else {
            // @audit-info 10 proposals max
            return 0.5 * 1e18 + (_fundedExtraordinaryProposals.length * (0.05 * 1e18));
        }
    }

        return            
            // 70%              20%                
            (votesReceived >= tokensRequested_ 

							50%
			+ _getSliceOfNonTreasury(minThresholdPercentage))
            &&
            // succeeded if tokens requested are available for claiming from the treasury
            (tokensRequested_ <= _getSliceOfTreasury(Maths.WAD - minThresholdPercentage))
        ;

Bob then burns 10% of his own tokens. This action reduces the total supply and, consequently, the threshold too. Now, the proposal needs only 61% to pass, and since Bob already has this percentage, he can execute his proposal and siphon off funds from the treasury.

Here's a PoC that can be used to showcase the issue:

For the ease of use, please add a console.log to the _extraordinaryProposalSucceeded function

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

        uint256 minThresholdPercentage = _getMinimumThresholdPercentage();

+        console.log("tokensNeeded", tokensRequested_ + _getSliceOfNonTreasury(minThresholdPercentage));

        return            
            // 50k            30k                // 50k
            (votesReceived >= tokensRequested_ + _getSliceOfNonTreasury(minThresholdPercentage))
            &&
            // succeeded if tokens requested are available for claiming from the treasury
            (tokensRequested_ <= _getSliceOfTreasury(Maths.WAD - minThresholdPercentage))
        ;
    }

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

        vm.roll(_startBlock + 100);

        // set proposal params
        uint256 endBlockParam = block.number + 100_000;

        // generate proposal targets
        address[] memory targets = new address[](1);
        targets[0] = address(_token);

        // generate proposal values
        uint256[] memory values = new uint256[](1);
        values[0] = 0;

        // generate proposal calldata
        bytes[] memory calldatas = new bytes[](1);
        calldatas[0] = abi.encodeWithSignature(
            "transfer(address,uint256)",
            _tokenHolder1,
            50_000_001 * 1e18
        );

        // create and submit proposal
        TestProposalExtraordinary memory testProposal = _createProposalExtraordinary(
            _grantFund,
            _tokenHolder1,
            endBlockParam,
            targets,
            values,
            calldatas,
            "Extraordinary Proposal for Ajna token transfer to tester address"
        );

        vm.roll(_startBlock + 150);


        uint256 votingWeight = _grantFund.getVotesExtraordinary(_tokenHolder2, testProposal.proposalId);


        changePrank(_tokenHolder2);
        _grantFund.voteExtraordinary(testProposal.proposalId);
        
        uint256 totalSupply = _token.totalSupply();

        address bob = makeAddr("bob");

        changePrank(_tokenDeployer);

        _token.transfer(bob, _token.balanceOf(_tokenDeployer));

        changePrank(bob);
        _token.burn(_token.balanceOf(bob));

        vm.roll(_startBlock + 217_000);

        _grantFund.state(testProposal.proposalId);
    }

Running the test with Bob burning tokens

        uint256 totalSupply = _token.totalSupply();

        address bob = makeAddr("bob");

        changePrank(_tokenDeployer);

        _token.transfer(bob, _token.balanceOf(_tokenDeployer));

        changePrank(bob);
        _token.burn(_token.balanceOf(bob));

Yields the following result:

Whereas if we remove the burning, the tokens needed are increased

-        uint256 totalSupply = _token.totalSupply();

-        address bob = makeAddr("bob");

-        changePrank(_tokenDeployer);

-        _token.transfer(bob, _token.balanceOf(_tokenDeployer));

-        changePrank(bob);
-        _token.burn(_token.balanceOf(bob));

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this vulnerability, consider implementing a mechanism that uses a snapshot of the total supply at the time of proposal submission rather than the current total supply. This change will prevent the threshold from being manipulated by burning tokens.

Assessed type

Other

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

        vm.roll(_startBlock + 100);

        // set proposal params
        uint256 endBlockParam = block.number + 100_000;

        // generate proposal targets
        address[] memory targets = new address[](1);
        targets[0] = address(_token);

        // generate proposal values
        uint256[] memory values = new uint256[](1);
        values[0] = 0;

        // generate proposal calldata
        bytes[] memory calldatas = new bytes[](1);
        calldatas[0] = abi.encodeWithSignature(
            "transfer(address,uint256)",
            _tokenHolder1,
            100_000_000 * 1e18
        );

        // create and submit proposal
        TestProposalExtraordinary memory testProposal = _createProposalExtraordinary(
            _grantFund,
            _tokenHolder1,
            endBlockParam,
            targets,
            values,
            calldatas,
            "Extraordinary Proposal for Ajna token transfer to tester address"
        );

        vm.roll(_startBlock + 150);


        uint256 votingWeight = _grantFund.getVotesExtraordinary(_tokenHolder2, testProposal.proposalId);


        changePrank(_tokenHolder2);
        _grantFund.voteExtraordinary(testProposal.proposalId);
        
        uint256 totalSupply = _token.totalSupply();

        address bob = makeAddr("bob");

        changePrank(_tokenDeployer);

        _token.transfer(bob, _token.balanceOf(_tokenDeployer));

        changePrank(bob);
        _token.burn(40_000_000 * 1e18);

        vm.roll(_startBlock + 217_000);

        _grantFund.state(testProposal.proposalId);
    }

QA REPORT

[01] Liquidity operators are unable to stake or redeem rewards on behalf of Liquidity owners

Since the operator can literally do everything with the NFT (burn it, move liquidity, mint it), it makes sense to also let them stake/unstake/redeem or move liquidity on behalf of the NFT owner. As confirmed with the sponsor, we both guess that they should be able to as they can be integrators on behalf of users

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/RewardsManager.sol#L107-L126

   /**
     *  @inheritdoc IRewardsManagerOwnerActions
     *  @dev    === Revert on ===
     *  @dev    not owner `NotOwnerOfDeposit()`
     *  @dev    already claimed `AlreadyClaimed()`
     *  @dev    === Emit events ===
     *  @dev    - `ClaimRewards`
     */
    function claimRewards(
        uint256 tokenId_,
        uint256 epochToClaim_
    ) external override {
        StakeInfo storage stakeInfo = stakes[tokenId_];

        if (msg.sender != stakeInfo.owner) revert NotOwnerOfDeposit();

        if (isEpochClaimed[tokenId_][epochToClaim_]) revert AlreadyClaimed();

        _claimRewards(stakeInfo, tokenId_, epochToClaim_, true, stakeInfo.ajnaPool);
    }

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/RewardsManager.sol#L220-L273

  function stake(
        uint256 tokenId_
    ) external override {
        address ajnaPool = PositionManager(address(positionManager)).poolKey(tokenId_);

        // check that msg.sender is owner of tokenId
        if (IERC721(address(positionManager)).ownerOf(tokenId_) != msg.sender) revert NotOwnerOfDeposit();

        StakeInfo storage stakeInfo = stakes[tokenId_];
        stakeInfo.owner    = msg.sender;
        stakeInfo.ajnaPool = ajnaPool;

        uint256 curBurnEpoch = IPool(ajnaPool).currentBurnEpoch();

        // record the staking epoch
        stakeInfo.stakingEpoch = uint96(curBurnEpoch);

        // initialize last time interaction at staking epoch
        stakeInfo.lastClaimedEpoch = uint96(curBurnEpoch);

        uint256[] memory positionIndexes = positionManager.getPositionIndexes(tokenId_);

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

            uint256 bucketId = positionIndexes[i];

            BucketState storage bucketState = stakeInfo.snapshot[bucketId];

            // record the number of lps in bucket at the time of staking
            bucketState.lpsAtStakeTime = uint128(positionManager.getLP(
                tokenId_,
                bucketId
            ));
            // record the bucket exchange rate at the time of staking
            bucketState.rateAtStakeTime = uint128(IPool(ajnaPool).bucketExchangeRate(bucketId));

            // iterations are bounded by array length (which is itself bounded), preventing overflow / underflow
            unchecked { ++i; }
        }

        emit Stake(msg.sender, ajnaPool, tokenId_);

        // transfer LP NFT to this contract
        IERC721(address(positionManager)).transferFrom(msg.sender, address(this), tokenId_);

        // calculate rewards for updating exchange rates, if any
        uint256 updateReward = _updateBucketExchangeRates(
            ajnaPool,
            positionIndexes
        );

        // transfer rewards to sender
        _transferAjnaRewards(updateReward);
    }

[2] Incorrect implementation of insertion sort

As per the docs, to avoid an overwhelming number of proposals, the slate of projects is filtered down to a manageable number during a screening stage. The screening stage lasts for the first 80 days of the cycle.

Every address holding Ajna tokens can use them to support any number of proposals, with each token supporting at most one project. At the end of the screening stage, the 10 proposals with the most support are deemed eligible to be funded.

Then, these 10 projects are then voted upon in the funding stage. This process happens in the screeningVote function, which is supposed to sort the proposals by calling _insertionSortProposalsByVotes.

The comments in the code claim that its "sorting 10 proposals", but that's actually not true - they are sorting just one of them.

Hence, the current implementation is wrong and won't sort the whole array, but just a single proposal. Here's a PoC to showcase the issue.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import "forge-std/Test.sol";
import "forge-std/console.sol";


contract Sorting is Test {
    struct Proposal {
        uint256 votesReceived;
    }

    mapping(uint256 => Proposal) _standardFundingProposals;

    function setUp() public {
        for(uint256 i = 0; i < 10; i++) {
            _standardFundingProposals[i] = Proposal({
                votesReceived: i
            });
        }
    }

    function testSorting() public {
        uint256[] memory proposals = new uint256[](10);
        for(uint256 i = 0; i < 10; i++) {
            proposals[i] = i;
        }

        _insertionSortProposalsByVotes(proposals, 9);

        for(uint256 i = 0; i < 10; i++) {
            console.log("proposal %s: %s", i, _standardFundingProposals[proposals[i]].votesReceived);
        }
    }

    function _insertionSortProposalsByVotes(
        uint256[] memory proposals_,
        uint256 targetProposalId_
    ) internal {
        while (
            targetProposalId_ != 0
            &&
            _standardFundingProposals[proposals_[targetProposalId_]].votesReceived > _standardFundingProposals[proposals_[targetProposalId_ - 1]].votesReceived
        ) {
            // swap values if left item < right item
            uint256 temp = proposals_[targetProposalId_ - 1];

            proposals_[targetProposalId_ - 1] = proposals_[targetProposalId_];
            proposals_[targetProposalId_] = temp;

            unchecked { --targetProposalId_; }
        }
    }
}

https://en.wikipedia.org/wiki/Insertion_sort

[3] Comment in code is potentially misleading

The comment here is wrong/misleading

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/PositionManager.sol#L55-L56

  /// @dev Mapping of `token id => ajna pool address` for which token was minted.
    mapping(uint256 => mapping(uint256 => Position)) internal positions;

[4] Possibility of Maths.wsqrt function

The function is used to calculate the quadrating voting in grants.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import "forge-std/Test.sol";
import "forge-std/console.sol";

library Maths {

    uint256 public constant WAD = 10**18;

    function abs(int x) internal pure returns (int) {
        return x >= 0 ? x : -x;
    }

    /**
     * @notice Returns the square root of a WAD, as a WAD.
     * @dev Utilizes the babylonian method: https://en.wikipedia.org/wiki/Methods_of_computing_square_roots#Babylonian_method.
     * @param y The WAD to take the square root of.
     * @return z The square root of the WAD, as a WAD.
     */
    function wsqrt(uint256 y) internal pure returns (uint256 z) {
        if (y > 3) {
            z = y;
            uint256 x = y / 2 + 1;
            while (x < z) {
                z = x;
                x = (y / x + x) / 2;
            }
        } else if (y != 0) {
            z = 1;
        }
        // convert z to a WAD
        z = z * 10**9;
    }

    function wmul(uint256 x, uint256 y) internal pure returns (uint256) {
        return (x * y + 10**18 / 2) / 10**18;
    }

    function wdiv(uint256 x, uint256 y) internal pure returns (uint256) {
        return (x * 10**18 + y / 2) / y;
    }

    function min(uint256 x, uint256 y) internal pure returns (uint256) {
        return x <= y ? x : y;
    }

    /** @notice Raises a WAD to the power of an integer and returns a WAD */
    function wpow(uint256 x, uint256 n) internal pure returns (uint256 z) {
        z = n % 2 != 0 ? x : 10**18;

        for (n /= 2; n != 0; n /= 2) {
            x = wmul(x, x);

            if (n % 2 != 0) {
                z = wmul(z, x);
            }
        }
    }

}



contract Sorting is Test {

    function toUint256(int256 value) internal pure returns (uint256) {
        require(value >= 0, "SafeCast: value must be positive");
        return uint256(value);
    }




    function testFuzz_Votes(int256 votesCast) public {
        uint256 voteCastSumSquare= Maths.wpow(toUint256(Maths.abs(votesCast)), 2);
    }
}

[5] Risk of overflow in Math.wmul function

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import "forge-std/Test.sol";
import "forge-std/console.sol";

library Maths {

    uint256 public constant WAD = 10**18;

    function abs(int x) internal pure returns (int) {
        return x >= 0 ? x : -x;
    }

    /**
     * @notice Returns the square root of a WAD, as a WAD.
     * @dev Utilizes the babylonian method: https://en.wikipedia.org/wiki/Methods_of_computing_square_roots#Babylonian_method.
     * @param y The WAD to take the square root of.
     * @return z The square root of the WAD, as a WAD.
     */
    function wsqrt(uint256 y) internal pure returns (uint256 z) {
        if (y > 3) {
            z = y;
            uint256 x = y / 2 + 1;
            while (x < z) {
                z = x;
                x = (y / x + x) / 2;
            }
        } else if (y != 0) {
            z = 1;
        }
        // convert z to a WAD
        z = z * 10**9;
    }

    function wmul(uint256 x, uint256 y) internal pure returns (uint256) {
        return (x * y + 10**18 / 2) / 10**18;
    }

    function wdiv(uint256 x, uint256 y) internal pure returns (uint256) {
        return (x * 10**18 + y / 2) / y;
    }
    

    function min(uint256 x, uint256 y) internal pure returns (uint256) {
        return x <= y ? x : y;
    }

    /** @notice Raises a WAD to the power of an integer and returns a WAD */
    function wpow(uint256 x, uint256 n) internal pure returns (uint256 z) {
        z = n % 2 != 0 ? x : 10**18;

        for (n /= 2; n != 0; n /= 2) {
            x = wmul(x, x);

            if (n % 2 != 0) {
                z = wmul(z, x);
            }
        }
    }

}



contract TestWmul is Test {

    function testFuzz_Math(uint256 x, uint256 y) public returns(uint256) {
        return Maths.wmul(x, y);
    }
}

It's used in the RewardsManager to calculate rewards.

[6] Proposal potentially shadowed

If a standard proposal with a specific proposal ID exists, the findMechanismOfProposal function will override any existing extraordinary proposal with the same ID. Consequently, the function will identify the proposal ID as belonging to a standard proposal, disregarding the presence of an extraordinary proposal with the same ID.

    function findMechanismOfProposal(
        uint256 proposalId_
    ) public view returns (FundingMechanism) {
        if (_standardFundingProposals[proposalId_].proposalId != 0)           return FundingMechanism.Standard;
        else if (_extraordinaryFundingProposals[proposalId_].proposalId != 0) return FundingMechanism.Extraordinary;
        else revert ProposalNotFound();
    }

    /// @inheritdoc IGrantFund
    function state(
        uint256 proposalId_
    ) external view override returns (ProposalState) {
        FundingMechanism mechanism = findMechanismOfProposal(proposalId_);

        return mechanism == FundingMechanism.Standard ? _standardProposalState(proposalId_) : _getExtraordinaryProposalState(proposalId_);
    }

[7] AJNA L1 Address hardcoded in code

Its better to pass it as a parameter in the constructor

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/token/BurnWrapper.sol#L15-L40

    /**
     * @notice Ethereum mainnet address of the Ajna Token.
     */
    address internal constant AJNA_TOKEN_ADDRESS = 0x9a96ec9B57Fb64FbC60B423d1f4da7691Bd35079;

    /**
     * @notice Tokens that have been wrapped cannot be unwrapped.
     */
    error UnwrapNotAllowed();

    /**
     * @notice Only mainnet Ajna token can be wrapped.
     */
    error InvalidWrappedToken();

    constructor(IERC20 wrappedToken)
        ERC20("Burn Wrapped AJNA", "bwAJNA")
        ERC20Permit("Burn Wrapped AJNA") // enables wrapped token to also use permit functionality
        ERC20Wrapper(wrappedToken)
    {
        if (address(wrappedToken) != AJNA_TOKEN_ADDRESS) {
            revert InvalidWrappedToken();
        }
    }

[8] Users with insufficient balances may not receive any voting power

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

        vm.roll(_startBlock + 50);

        // start distribution period
        _startDistributionPeriod(_grantFund);

        // generate proposal targets
        address[] memory ajnaTokenTargets = new address[](1);
        ajnaTokenTargets[0] = address(_token);

        // generate proposal values
        uint256[] memory values = new uint256[](1);
        values[0] = 0;

        // generate proposal calldata
        bytes[] memory proposalCalldata = new bytes[](1);
        proposalCalldata[0] = abi.encodeWithSignature(
            "transfer(address,uint256)",
            _tokenHolder2,
            1e18
        );

        // generate proposal message 
        string memory description = "Proposal for Ajna token transfer to tester address";

        // create and submit proposal
        TestProposal memory proposal = _createProposalStandard(_grantFund, _tokenHolder1, ajnaTokenTargets, values, proposalCalldata, description);

        // screening stage votes
        _screeningVote(_grantFund, _tokenHolder1, proposal.proposalId, 1 wei);



        _token.transfer(_tokenHolder5, _token.balanceOf(_tokenHolder1) - 0.0000000001e18);

        // skip forward to the funding stage
        vm.roll(_startBlock + 600_000);
        

        // check initial voting power
        uint256 votingPower = _getFundingVotes(_grantFund, _tokenHolder1);
        console.log(votingPower);
}

[09] In the screening sorting process, the latest proposal is always first to be eliminated if it shares the same number of screening votes with others.

When proposals are sorted for the screening stage, each time there is a new vote the list could get rearranged. The issue here is that if there are 12 proposals and the proposals in range 8-12 are with the same amount of screening votes, it would kick the 12th one. This leads to unfair advantage to the proposals that got votes first.

     int indexInArray = _findProposalIndex(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(proposalId);

            // sort top ten proposals
            _insertionSortProposalsByVotes(currentTopTenProposals, screenedProposalsLength);
        }
        else {
            // proposal is already in the array
            if (indexInArray != -1) {
                // re-sort top ten proposals to account for new vote totals
                _insertionSortProposalsByVotes(currentTopTenProposals, uint256(indexInArray));
            }
            // proposal isn't already in the array
            else if(_standardFundingProposals[currentTopTenProposals[screenedProposalsLength - 1]].votesReceived < proposal_.votesReceived) {
                // @audit-info last 2 could be with same value, how do you decide which to kick?
                // replace the least supported proposal with the new proposal
                currentTopTenProposals.pop();
                currentTopTenProposals.push(proposalId);

                // sort top ten proposals
                _insertionSortProposalsByVotes(currentTopTenProposals, screenedProposalsLength - 1);
            }
        }