Make proposals fail, no-revert on error, loss of funds

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/4db573870aa4e1f40a3381cdd4ec006222e471fe/packages/contracts/src/plugins/governance/multisig/Multisig.sol#L333-L339

Vulnerability details

Impact

The issue exists because anyone can call execute()in the multisig contract with will execute the proposal in the DAO contract. The problem here is that certain proposal will not revert when they fail because they have the uint256 _allowFailureMap param set to true. The thing is that the proposals should not fail even if the flag is set to true.

The attack path goes as follows:

The attacker calls the execute function for a proposal that has been approved and as the uint256 _allowFailureMap param set.

Execute() function: https://github.com/code-423n4/2023-03-aragon/blob/4db573870aa4e1f40a3381cdd4ec006222e471fe/packages/contracts/src/plugins/governance/multisig/Multisig.sol#L333-L339

The function is called with a very low gas, just enough for the transaction to go through but the external call from the DAO contract to fail due to not enough gas.

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

Due to the EIP-150 63/64 remaining gas is forwarded to the external call, so even if we make it fail because it runs out of gas, there will still be enough gas to finish the execution of the execute() function:

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

Therefore, we can make all the proposals which allowFailure, fail. Being a critical issue, which will lead to loss of funds and non-expected cancelation of proposals which will not be recovered.

Proof of Concept

We create an interface of the multisig contract to call execute.

interface multisig{
function execute(uint256 _proposalId) external;
}

 contract AttackAragon{
multisig m;
constructor(multisig _m){
 m= _m;
}

 function attack()external{
    address(m).call{gas: 25000}(abi.encodeWithSignature("exeute(uint256)", 1));

 }
 }

//the gas to make the external call, is yet to be calculated, but once calculated, the external call will "Fail" but not the transaction, making the exploit successful

Tools Used

Manual

Recommended Mitigation Steps

Don't let all the users execute proposals, they should be executed by trusted authors. CONSIDER ADDING A MODIFIER TO EXECUTE

[0xean]

Interesting attack vector, however if a call is allowed to fail, its hard to believe that it would be so critical to the DAO to lead to loss of funds. I am downgrading to M and will await further sponsor comment

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked the issue as duplicate of #191

[c4-judge]

0xean marked the issue as satisfactory