Lack of sanity check for creating a new proposal, the actions `to` address might be an EOA, which might lead to loss of funds

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/4db573870aa4e1f40a3381cdd4ec006222e471fe/packages/contracts/src/core/plugin/proposal/Proposal.sol#L45 https://github.com/code-423n4/2023-03-aragon/blob/4db573870aa4e1f40a3381cdd4ec006222e471fe/packages/contracts/src/core/plugin/proposal/ProposalUpgradeable.sol#L45

Vulnerability details

Impact

Provide a detailed description of the impact this bug/vulnerability has on the overall system under test.

Proof of Concept

Provide screenshots, logs, or any other relevant proof that illustrates the concept of the bug/vulnerability you have identified.

Tools Used

Describe the tools used throughout your testing and analysis process.

Recommended Mitigation Steps

Describe the recommended steps that a project should use to mitigate the bugs or vulnerabilities you have identified.

Impact

There is a lack of sanity check in createProposal and execute, which doesn't eliminate or help in decrease the amount of possible human error. Similar to how non-payable functions, checks for any ether being sent along the transaction, there need to be checks to ensure that the user is achieving an expected outcome from his keyed in actions. Especially for the cases, where the to address is expected to be a contract, but it is mistakenly inputed as an EOA. In that case, the transaction will not revert. It can potentially leads to a loss of ether.

Proof of Concept

SC?: is the to address a smart contract

Data?: is the data passed along the transaction

Value?: is the value passed along the transaction

Valid?: is the transaction valid

Revert?: would the transaction revert

Comment?: comment on the edge case

The possible scenarios for an action can be as follows:

SC?	Data?	Value?	Valid	Revert	Comment
0	0	0	0	0	there is no point in a transaction that just doesn't perform anything
0	0	1	1	0	sending ether to an EOA is normal
0	1	0	0	0	sending only payload to an EOA, mostly means that the callee is mistakenly keyed in
0	1	1	0	0	it mostly means that the callee is mistakenly passed, it should be a SC
1	0	0	0	0/1	it might revert, in the case the receive/fallback is not found, nor payable
1	0	1	1	0/1	it will only not revert, in the case of receive or fallback function found
1	1	0	1	0/1	it might revert, in the case the function selector or fallabck is not found
1	1	1	1	0/1	it might revert, in the case the function selector or fallaback is not found

from the previous possible cases, we see that there is room for errors when the to is an EOA, it is only valid, when only a value is included in the transaction, on the other cases, it would lead that the transction is sent with a payload to an EOA, or on a severe note, sending ether and payload to an EOA, thinking it was sent to a smart contract, then losing the ether for such mistake. simple possible case, could be swapping native ether for USDC, which then would be lost due to lack of sanity check.

Tools Used

Vs Code, Remix Ide

Recommended Mitigation Steps

It is recommended to add sanity checks especially when the to address is an EOA, and if you want to allow it on an opt in basis, you may pass in a parameter in the _createProposal, which allow to deliberately check the actions validity.

    function _createProposal(
        address _creator,
        bytes calldata _metadata,
        uint64 _startDate,
        uint64 _endDate,
        IDAO.Action[] calldata _actions,
        uint256 _allowFailureMap,
        bool _safeCheck
    ) internal virtual returns (uint256 proposalId) {
        proposalId = _createProposalId();
        uint256 len = _actions.length;
        if(_safeCheck){
            for(uint i; i < len; ){
                // check for EOA
                if((_actions[i].to).code.size == 0) {
                    // ensure data == 0 && value > 0
                    if(_actions[i].data.length > 0 || _actions[i].value == 0){
                        revert("invalid opeation!");
                    }
                }
                unchecked{
                    ++i;
                }
            }
        }

        emit ProposalCreated({
            proposalId: proposalId,
            creator: _creator,
            metadata: _metadata,
            startDate: _startDate,
            endDate: _endDate,
            actions: _actions,
            allowFailureMap: _allowFailureMap
        });
    }

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-c