search

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

2 stars 0 forks source link

A malicious user can permanently DoS proposal execution. #188

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/Funding.sol#L112-L142

Vulnerability details

Impact

A malicious user can create a proposal with the AjnaToken address as the target, which will cause the proposal execution to fail when the time comes. Although the funds are not lost and can be eventually returned to the treasury, the protocol's functionality and availability could be negatively affected,

Proof of Concept

To create a standard or extraordinary proposal, users must provide the following information:

   /// @inheritdoc IExtraordinaryFunding
    function proposeExtraordinary(
        uint256 endBlock_,
        address[] memory targets_,
        uint256[] memory values_,
        bytes[] memory calldatas_,
        string memory description_) external override returns (uint256 proposalId_) {

In this case targets will be the addresses that are supposed to receive the funds if the proposal goes through.

In this case, targets represent the addresses receiving funds if the proposal is approved. For values[], each value must be 0 because we are transferring Ajna tokens. Regarding calldatas, each individual calldata must contain 0xa9059cbb, which is the function selector for transfer(address,uint256), the address of the receiver and the amount to be received. This function will be called on the AjnaToken address when it is time for execution. The _validateCalldatas function is responsible for validating this information.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/grants/base/Funding.sol#L104-L108

    function _validateCallDatas(
        address[] memory targets_,
        uint256[] memory values_,
        bytes[] memory calldatas_
    ) internal view returns (uint128 tokensRequested_) {

        // check params have matching lengths
        if (targets_.length == 0 || targets_.length != values_.length || targets_.length != calldatas_.length) revert InvalidProposal();

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

            // check targets and values params are valid
            if (targets_[i] != ajnaTokenAddress || values_[i] != 0) revert InvalidProposal();
            // @audit doesn't validate if the receiver exists

            // check calldata function selector is transfer()
            bytes memory selDataWithSig = calldatas_[i];

            bytes4 selector;
            //slither-disable-next-line assembly
            assembly {
                selector := mload(add(selDataWithSig, 0x20))
            }
            if (selector != bytes4(0xa9059cbb)) revert InvalidProposal();

            // https://github.com/ethereum/solidity/issues/9439
            // retrieve tokensRequested from incoming calldata, accounting for selector and recipient address
            uint256 tokensRequested;
            bytes memory tokenDataWithSig = calldatas_[i];
            //slither-disable-next-line assembly
            assembly {
                tokensRequested := mload(add(tokenDataWithSig, 68))
            }

            // update tokens requested for additional calldata
            tokensRequested_ += SafeCast.toUint128(tokensRequested);

            unchecked { ++i; }
        }
    }

The issue with the current implementation is that while values and calldatas are validated, targets are not.

In some cases, this could impact the functionality or availability of the protocol because the proposal execution will fail.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/grants/base/Funding.sol#L60-L65


        string memory errorMessage = "Governor: call reverted without message";
        for (uint256 i = 0; i < targets_.length; ++i) {
            (bool success, bytes memory returndata) = targets_[i].call{value: values_[i]}(calldatas_[i]);
            Address.verifyCallResult(success, returndata, errorMessage);
        }

Once a proposal is created, the target cannot be changed, because the proposalId is a hash of the targets, calldatas and values.

It is important to note that there is no way to check the proposal execution receiver without examining the transaction calldata since there are no functions that expose the proposal creation data.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/grants/base/Funding.sol#L154-L161

    function _hashProposal(
        address[] memory targets_,
        uint256[] memory values_,
        bytes[] memory calldatas_,
        bytes32 descriptionHash_
    ) internal pure returns (uint256 proposalId_) {
        proposalId_ = uint256(keccak256(abi.encode(targets_, values_, calldatas_, descriptionHash_)));
    }

The actual execution of the proposal happens here 👇

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/grants/base/Funding.sol#L51-L66

     */
    function _execute(
        uint256 proposalId_,
        address[] memory targets_,
        uint256[] memory values_,
        bytes[] memory calldatas_
    ) internal {
        // use common event name to maintain consistency with tally
        emit ProposalExecuted(proposalId_);

        string memory errorMessage = "Governor: call reverted without message";
        for (uint256 i = 0; i < targets_.length; ++i) {
            (bool success, bytes memory returndata) = targets_[i].call{value: values_[i]}(calldatas_[i]);
            Address.verifyCallResult(success, returndata, errorMessage);
        }
    }

Which calls AjnaToken's withdraw function to transfer the funds to the target

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/token/AjnaToken.sol#L11-L14

contract AjnaToken is ERC20, ERC20Burnable, ERC20Permit, ERC20Votes {
    constructor(address tokenReceiver_) ERC20("AjnaToken", "AJNA") ERC20Permit("AjnaToken") {
        _mint(tokenReceiver_, 2_000_000_000 * 10 ** decimals());
    }

However, upon observing AjnaToken's functionality, we see that the _beforeTokenTransfer hook ensures that tokens cannot be transferred to the AjnaToken contract itself.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/token/AjnaToken.sol#L27-L29

   function _beforeTokenTransfer(address, address to_, uint256) internal view override {
        require(to_ != address(this), "Cannot transfer tokens to the contract itself");
    }

A malicious user can take advantage of that and create a proposal with the AjnaToken address as target, which will brick the execution of the proposal when the time comes.

Consider the case when the proposal is Standard:

  1. A proposal is created
  2. It gets past screening and is in top 10
  3. It gets funded
  4. The challenge period has finished
  5. The proposal can get executed but fails because the receiver is the Ajna contract.

Normally, this wouldn't be a huge deal,but each distribution period is 3 months. So in this case the proposal would go through various stages to figure in 3 months that the proposal won't get executed.

Since there are assets not at direct risk, but the protocol's function or availability could be certainly impacted, I'm rating this as a Medium.

Here's a PoC that showcases the scenario.

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

        vm.roll(_startBlock + 150);

        /*********************************/
        /*** First Distribution Period ***/
        /*********************************/

        // start first distribution
        _startDistributionPeriod(_grantFund);

        uint24 distributionId = _grantFund.getDistributionId();

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

        assertEq(gbc_distribution1, 15_000_000 * 1e18);

        TestProposalParams[] memory testProposalParams_distribution1 = new TestProposalParams[](1);
        testProposalParams_distribution1[0] = TestProposalParams(0x9a96ec9B57Fb64FbC60B423d1f4da7691Bd35079, 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
        _screeningVote(_grantFund, _tokenHolder1, testProposals_distribution1[0].proposalId, _getScreeningVotes(_grantFund, _tokenHolder1));

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

        // execute funded proposals
        _executeProposal(_grantFund, _token, testProposals_distribution1[0]);

    }

Running the test, yields us the following result:

Regrettably, the resources allocated to a specific proposal become permanently locked within it, with the protocol lacking the functionality to recoup them back into the treasury.

Within Ajna, grants are allocated during distinct distribution periods, each spanning three months.

Subject to certain prerequisites, a new distribution period can be activated by calling the following function:

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-grants/src/grants/base/StandardFunding.sol#L120-L140

function startNewDistributionPeriod() external override returns (uint24 newDistributionId_) {
        uint24  currentDistributionId       = _currentDistributionId;
        uint256 currentDistributionEndBlock = _distributions[currentDistributionId].endBlock;

        // check that there isn't currently an active distribution period
        if (block.number <= currentDistributionEndBlock) revert DistributionPeriodStillActive();

        // update treasury with unused funds from last two distributions
        {
            // 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);
            }
        }

Should the validation confirming the absence of an active distribution period be successful, the function subsequently retrieves and yields the unused funds from the preceding two distribution periods. This is to cover the scenario where for example there are no proposals to execute, but funds were allocated to the distribution period.

In instigating a new distribution, the funds entangled in the unexecutable proposal are expected to be reimbursed to the treasury.

Nonetheless, since the proposal has been approved and is listed in the fundedSlateHash, the updateTreasury function will infer it as executable and awaiting payment. (that's by design). So, it will add its tokenRequested to the totalTokensRequested.

    function _updateTreasury(
        uint24 distributionId_
    ) private {
        bytes32 fundedSlateHash = _distributions[distributionId_].fundedSlateHash;
        uint256 fundsAvailable  = _distributions[distributionId_].fundsAvailable;

        uint256[] memory fundingProposalIds = _fundedProposalSlates[fundedSlateHash];

        uint256 totalTokensRequested;
        uint256 numFundedProposals = fundingProposalIds.length;

        for (uint i = 0; i < numFundedProposals; ) {
            Proposal memory proposal = _standardFundingProposals[fundingProposalIds[i]];

            totalTokensRequested += proposal.tokensRequested;

            unchecked { ++i; }
        }

        // readd non distributed tokens to the treasury
        // @audit-info in which scenario can  we have non distributed tokens?
        treasury += (fundsAvailable - totalTokensRequested);

        _isSurplusFundsUpdated[distributionId_] = true;
    }

This situation culminates in the treasury not receiving a refund for the total number of tokens requested in that specific proposal. Yet, the proposal itself cannot be executed, resulting in the permanent entrapment of the funds. This outcome is further substantiated by the update to the _isSurplusFundsUpdated mapping, causing the ensnared funds to become irreversibly unobtainable.

Considering all of these factors, I propose that the appropriate severity for this finding should be classified as 'High', based on the C4 categorization criteria.

High: Assets can be directly or indirectly stolen, lost, or compromised (the latter only if there exists a credible attack route devoid of speculative scenarios).

Evidently, a credible and feasible attack path exists. First, one could contend that the faulty proposal might enter the leading slate. Yet, it could be replaced by a new top slate featuring different proposals before the period ends, providing it is more optimal. This would then potentially exclude the invalid proposal.

Nonetheless, if the proposal pool during the screening period is under 10 (no new proposals can be considered after the screening period has ended), or if a user or a coalition has accumulated enough votes to propel a malicious proposal forward, these considerations become immaterial.

Tools Used

Manual Review

Recommended Mitigation Steps

In the _validateCallDatas function, validate the token receiver's address, ensuring it is not the AjnaToken's address.

Assessed type

DoS

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 duplicate of #280

c4-sponsor commented 1 year ago

MikeHathaway marked the issue as sponsor confirmed

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)