`execute` calls to non-existent contracts will fail silently, allowing execution to continue

[code423n4]

Lines of code

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

Vulnerability details

Impact

Calls may fail silently during DAO.execute as low-level calls to non-existent contracts will return success.

Proof of Concept

We can see that in DAO.sol, we perform low-level calls over an array of _actions and revert according to an _allowFailureMap.

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

The problem with this logic is that it falsely assumes that the call is successful if it returned success. This is however not necessarily the case with low-level calls.

As stated in the solidity docs:

"Due to the fact that the EVM considers a call to a non-existing contract to always succeed, Solidity uses the extcodesize opcode to check that the contract that is about to be called actually exists (it contains code) and causes an exception if it does not. This check is skipped if the return data will be decoded after the call and thus the ABI decoder will catch the case of a non-existing contract.

Note that this check is not performed in case of low-level calls which operate on addresses rather than contract instances."

Therefore, the logic of catching failing calls with the _allowFailureMap won't necessarily catch all failing calls, leading to subsequent calls still being executed, potentially leading to an unwanted state. Consider for example the following scenario:

• The DAO wants to add collateral to their lending position and take out more debt so they create the parameters to perform this transaction
• They create the following _actions array:
  • Approve WETH to Aave
  • Deposit WETH on Aave
  • Withdraw USDC against WETH position on Aave
• They set the _allowFailureMap such that if either of the first two _actions fails, the whole execution should revert
• Under the false assumption that their _allowFailureMap will protect them in case if anything goes wrong, they aren't concerned with simulating the execution before calling _execute
• The address for the second transaction turns out to be incorrect, causing it to silently fail, allowing the third transaction to be executed
• Their debt position becomes insufficiently collateralized and is liquidated

Recommended Mitigation Steps

To solve this issue, include a bitmap similar to _allowFailureMap which enforces which actions should first verify that the calling address is a contract.

[0xean]

these payloads are meant to be vetted by the DAO ahead of execution and this ends up being mostly about input sanitization, I think QA is the appropriate severity.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-c