Low level calls to accounts with no code succeed in `DAO.execute`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L168-L215

Vulnerability details

Impact

DAO management is done using the execute function that lets permissioned entities execute arbitrary actions from the DAO contract. These actions are composed of a target address, a value and an arbitrary data payload that is then used as calldata. The execute will run through each action executing a low level call to the target address with the associated calldata and value.

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L168-L215

function execute(
    bytes32 _callId,
    Action[] calldata _actions,
    uint256 _allowFailureMap
)
    external
    override
    auth(EXECUTE_PERMISSION_ID)
    returns (bytes[] memory execResults, uint256 failureMap)
{
    if (_actions.length > MAX_ACTIONS) {
        revert TooManyActions();
    }

    execResults = new bytes[](_actions.length);

    for (uint256 i = 0; i < _actions.length; ) {
        address to = _actions[i].to;
        (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;
        }
    }

    emit Executed({
        actor: msg.sender,
        callId: _callId,
        actions: _actions,
        failureMap: failureMap,
        execResults: execResults
    });
}

Low level calls to accounts with no code will succeed. Quoting the reference for the CALL opcode in evm.codes:

Creates a new sub context and execute the code of the given account, then resumes the current one. Note that an account with no code will return success as true.

This means that an action (that potentially has calldata, to discriminate the case of simple transfer of value) to an account with no code will succeed without any errors. The success variable will be true and the execute will treat these actions as correctly executed (no revert, no failure).

Proof of Concept

The following test illustrates the issue. Here we bootstrap a DAO and execute an action to an empty account (address(0xdeadc0de)) with a calldata that simulates an ERC20 transfer. The action will succeed and the execute function won't revert.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_DAO_execute_LowLevelCallsToEmptyCodeSucceed() public {
    DAO dao = createDao();

    address to = address(0xdeadc0de);
    assertEq(to.code.length, 0);

    DAO.Action[] memory actions = new DAO.Action[](1);
    actions[0].to = to;
    actions[0].data = abi.encodeWithSelector(
        IERC20.transfer.selector,
        makeAddr("an important recipient"),
        1_000_000 ether
    );

    // Execute call succeeds
    (, uint256 failureMap) = dao.execute(bytes32(uint256(0)), actions, 0);

    // No failures
    assertEq(failureMap, 0);
}

Recommendation

In case the action has calldata (i.e. action.data.length > 0) the execute function can first validate that the to account has non-empty code.

Another alternative would be to use a strategy similar to how verifyCallResultFromTarget works in the Address library of OpenZeppelin contracts, in which they check the account's code length if the returndata length is zero. Again, this path should also contemplate the case when calldata is empty to allow transfers of value that are not intended to execute any code.

Example:

  for (uint256 i = 0; i < _actions.length; ) {
      address to = _actions[i].to;

+     if (_actions[i].data.length > 0 && to.code.length == 0) {
+       revert ActionToAccountWithNoCode(i);
+     }

      (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

[c4-judge]

0xean marked the issue as grade-b

[c4-judge]

0xean marked the issue as grade-c