User may force fail the action from the `DAO:execute`

[code423n4]

Lines of code

https://github.com/aragon/osx/blob/develop/packages/contracts/src/core/dao/DAO.sol#L186 https://github.com/aragon/osx/blob/develop/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol#L286 https://github.com/aragon/osx/blob/develop/packages/contracts/src/plugins/governance/majority-voting/MajorityVotingBase.sol#L459

Vulnerability details

Description

The execute function from the DAO.sol contract allow to execution of any call to any address if the caller has appropriate permission. Some calls are expected to be always successfully executed, and some may revert and execute will continue the execution.

The following code may call and handle call status.

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

Also, the function is expected to be used in a different scenario, where the caller may be a user, voter, etc. (See MajorityVotingBase). So the caller is not a trusted entity and that means any manipulation of the DAO call should be avoided.

The problem is that caller may choose the gas with which the code is executed. If the child call execution spends enough gas then the user may choose that amount of gas, that child call frame fails, but the left gas is enough to successfully finish DAO:execute function.

Please note, even though the execute pass all gas to the child call, actually only 63/64 gas is passed and 1/64 of gas is left on the parent call (EIP-150).

• https://medium.com/iovlabs-innovation-stories/the-dark-side-of-ethereum-1-64th-call-gas-reduction-ba661778568c

Attack scenario

The DAO starts majority voting, and users who have DAO tokens may vote for the proposal. The proposal is to call one target protocol, which may fail in case of an inner reason. So the DAO set that the call may fail. The approximate gas that is needed to finish the call to the target contract is 700k. A malicious voter call execute function with 711.1k of gas. Since 63/64 * 711.1 < 700, the requested call will fail. And the remaining gas is still sufficient to end the execute function logic.

Impact

The user may forcefully fail the inner call from the execute function. Also, anyone who will use the usual eth_estimateGas for the gas estimation for the execute function will accidentally calculate the amount of gas that will fail the call.

Since majority voting is hard to process with many users involved, creating another proposal may create a lot of pain.

Recommended Mitigation Steps

Add the require that gas after the call is bigger than gas before / 64.

uint256 gasBefore;
// Do call...
require(gasleft() > gasBefore/64);

[c4-judge]

0xean marked the issue as primary issue

[c4-sponsor]

novaknole20 marked the issue as sponsor confirmed

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report