code-423n4 / 2023-05-ajna-findings

2 stars 0 forks source link

minThresholdPercentage in ExtraordinaryFunding will force proposals to fail because proposals will not meet the increased percentage requirement #383

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Proof of Concept

In ExtraordinaryFunding.sol, only 10 proposals in total can be funded. Here is how the token request flow looks like:

  1. When creating a proposal, A user calls proposeExtraordinary(). The function checks whether the token requested is lesser than the minimumThresHoldPercentage.
        uint128 totalTokensRequested = _validateCallDatas(targets_, values_, calldatas_);

        // check tokens requested are available for claiming from the treasury
        if (uint256(totalTokensRequested) > _getSliceOfTreasury(Maths.WAD - _getMinimumThresholdPercentage())) revert InvalidProposal();

Maths.WAD is 1e18. _getMinimumThresholdPercentage starts from 50% (0.5e18). The minimumThresholdPercentage will increase by 5% after every successful proposal executed.

    _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 {
            return 0.5 * 1e18 + (_fundedExtraordinaryProposals.length * (0.05 * 1e18));
        }
    }

If there is 100,000 tokens in the treasury, then the proposal must request 50,000 tokens or less if _getMinimumThresholdPercentage returns 0.5e18.

  1. When attempting to execute the proposal, the user calls executeExtraordinary(). The amount of tokens requested is checked again.
        if (proposal.executed || !_extraordinaryProposalSucceeded(proposalId_, tokensRequested)) revert ExecuteExtraordinaryProposalInvalid();
    _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))

In particular, (tokensRequested_ <= _getSliceOfTreasury(Maths.WAD - minThresholdPercentage)) must return true.

Let's look at an example of how this might be an issue. There are 1,000,000 tokens in the treasury and all of them requests 500,000 tokens:

  1. 20 proposals are created (there is no limit to proposing, just limits to executing, max 10 executed proposals)
  2. All these proposals created at the start will have a minThresholdPercentage of 50% (500,000)
  3. Let's say 2 proposals are pretty popular and gained the votes of many, they can be executed
  4. Once the first proposal is executed, minThresholdPercentage is increased to 55%. Also, the slice of the treasury holdings will decrease, making the minThresholdPercentage even lower if the treasury is not increased back up to original.
  5. The second proposal has gained many votes but due to the increase in percentage and decrease in treasury holdings, the proposal will not be executed

Impact

Proposals may have to be recreated many times in order to pass the minThresholdPercentage requirement

Tools Used

VSCode

Recommended Mitigation Steps

Recommend a way to change the tokenRequested (only able to change to a lower position) during the voting so that if one proposal passes, the minThresholdPercentage will not force the other proposals to fail.

Assessed type

Other

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

c4-sponsor commented 1 year ago

MikeHathaway marked the issue as sponsor acknowledged

Picodes commented 1 year ago

This corresponds to the example of the whitepaper (page 35).

Downgrading to QA

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

Picodes marked the issue as grade-b