`Funding._validateCallDatas` miss the check on the length of `calldatas_[i]`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/Funding.sol#L63 https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/Funding.sol#L133

Vulnerability details

Impact

When proposing a proposal, the call data would be validated in Funding._validateCallDatas. It would check if the call data match the specifications. And the calldatas_ would be used in Funding._execute when the proposal has passed. However, the proposal could be blocked due to the missed check on the length of calldatas_[i] .

Proof of Concept

Funding._validateCallDatas() checks the selector in calldatas_[i] and fetch the tokensRequested from calldatas_[i] https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/Funding.sol#L133

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

        …

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

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

A normal calldata should be formed like:

abi.encodeWithSignature(
    "transfer(address,uint256)",
    recipient,
    tokensRequested
);

And the tokensRequested can fetch by:

            assembly {
                tokensRequested := mload(add(tokenDataWithSig, 68))
            }

But a malformed calldata can be:

abi.encodeWithSignature(
    "transfer(address,uint256)",
    recipient
);

The calldata still pass the check in Funding._validateCallDatas. And the fetched tokensRequested is zero

However, when the proposal has passed. The proposal cannot be executed since targets_[i].call won’t succeed. https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-grants/src/grants/base/Funding.sol#L63

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

Tools Used

Manual Review

Recommended Mitigation Steps

Add a length check on calldata_[i] to prevent invalid proposals.

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

        …

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

            …
            bytes memory selDataWithSig = calldatas_[i];
+           if (calldatas_[i].length != 68) revert InvalidProposal();

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

            …
        }
    }

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as primary issue

[Picodes]

Regrouping all issues related to the possibility that proposal can't be executed

[c4-sponsor]

MikeHathaway marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[0xRobocop]

I think the issue should be low/informational.

There is a discussion here about issues that are conditional on user mistake. Wanted to note that the arguments pro marking user mistake as medium, agree that some leak of value or loss of funds should be involved, which is not the case in this issue.

The DoS is only limited to the specific proposal and not to the system.

[Picodes]

@0xRobocop I didn't consider this an instance of "user mistake". My reasoning was that the sponsor implemented a bunch of safeguards in _validateCallDatas to avoid invalid proposals from being voted and going through the whole voting process (taking some place in the amount of funding available) so I saw this as a potential spam / griefing attack

[Picodes]

On second thought, I think @0xRobocop is right and we should consider this a "user mistake". Indeed, malicious proposals can be proposed anyway, for example, to transfer funds to inexistent addresses or to spam the voting system.

So I think Low severity is more appropriate.

[c4-judge]

Picodes marked the issue as not selected for report

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[bytes032]

On second thought, I think @0xRobocop is right and we should consider this a "user mistake". Indeed, malicious proposals can be proposed anyway, for example, to transfer funds to inexistent addresses or to spam the voting system.

So I think Low severity is more appropriate.

If a malicious proposal gets voted on, the funds will be locked in the system. #188 explains this precisely. Why would that be just low severity?

[Picodes]

@bytes032 in the sense that malicious proposals can be submitted anyway (sending funds to addresses not owned by anyone, etc), so it's up to voters to verify that what they vote for is correct. The safety checks are mitigating some attacks but doesn't change this

[c4-judge]

Picodes marked the issue as grade-b