Failed transfer with low-level call will not revert

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/4db573870aa4e1f40a3381cdd4ec006222e471fe/packages/contracts/src/core/dao/DAO.sol#L186

Vulnerability details

Impact

In DAO.sol, the function execute uses a low-level function .call to perform _actions:

(bool success, bytes memory response) = to.call{value: _actions[i].value}(_actions[i].data);

The code is executing a low-level call to a (potential) contract without verifying if the contract exists. If there is no code at the provided address, the call will succeed and the sender will lose the sent funds.
According to the solidity docs: The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

Proof of Concept

https://docs.soliditylang.org/en/develop/control-structures.html#error-handling-assert-require-revert-and-exceptions

contract Test {
    function testCall() public payable returns (bool) {
        address nonExistentAccount = 0x0000000000000000000000000000000000000000;
        (bool success,) = nonExistentAccount.call{value: msg.value}("");
        return success;
    }
}

REMIX IDE can be used to test PoC. msg.value will be lost.

Tools Used

Manual Analysis

Recommended Mitigation Steps

require(to.code.length > 0, "Contract doesn't exists");

Before executing the low-level call, it is important to verify the existence of the code. However, implementing this check may cause issues when transferring funds to an EOA address. To address this problem, we can prompt the user to specify whether the address is for an EOA or a contract. If the address is for an EOA, then the existence check is not necessary. However, if it is for a contract, the check is crucial to ensure the safety of the transaction.
For that we need to add a bool in the struct Actions:

    struct Action {
        address to;
+       bool isAddressEOA;
        uint256 value;
        bytes data;
    }

And then code existence check inside an If statement in the execute function:

for (uint256 i = 0; i < _actions.length; ) {
            address to = _actions[i].to;
+           if(!_actions[i].isAddressEOA){
+               require(to.code.length > 0, "Contract doesn't exists");
+           }
            (bool success, bytes memory response) = to.call{value: _actions[i].value}(
                _actions[i].data
            );

            if (!success) {
                // If the call failed and wasn't allowed in allowFailureMap, revert.
                if (!hasBit(_allowFailureMap, uint8(i))) {
                    revert ActionFailed(i);
                }

                // If the call failed, but was allowed in allowFailureMap, store that
                // this specific action has actually failed.
                failureMap = flipBit(failureMap, uint8(i));
            }

            execResults[i] = response;

            unchecked {
                ++i;
            }
        }

[0xean]

payloads are verified ahead of execution by the DAO, QA.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-c