diff --git a/packages/contracts/CHANGELOG.md b/packages/contracts/CHANGELOG.md index d2de67425..1dab8d2fd 100644 --- a/packages/contracts/CHANGELOG.md +++ b/packages/contracts/CHANGELOG.md @@ -9,6 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +### Changed + +- Added check to `execute()` in `DAO` to prevent griefing attacks if the caller provides insufficient gas on actions being allowed to fail. + +### Removed + +## v1.0.1 + +### Added + - Created the `IMultisig` interface. ### Changed diff --git a/packages/contracts/src/core/dao/DAO.sol b/packages/contracts/src/core/dao/DAO.sol index d7c912d4b..d5f9c1b5c 100644 --- a/packages/contracts/src/core/dao/DAO.sol +++ b/packages/contracts/src/core/dao/DAO.sol @@ -76,6 +76,9 @@ contract DAO is /// @param index The index of the action in the action array that failed. error ActionFailed(uint256 index); + /// @notice Thrown if an action has insufficent gas left. + error InsufficientGas(); + /// @notice Thrown if the deposit amount is zero. error ZeroAmount(); @@ -181,24 +184,39 @@ contract DAO is execResults = new bytes[](_actions.length); + uint256 gasBefore; + uint256 gasAfter; + for (uint256 i = 0; i < _actions.length; ) { - address to = _actions[i].to; - (bool success, bytes memory response) = to.call{value: _actions[i].value}( + gasBefore = gasleft(); + + (bool success, bytes memory result) = _actions[i].to.call{value: _actions[i].value}( _actions[i].data ); + gasAfter = gasleft(); - if (!success) { - // If the call failed and wasn't allowed in allowFailureMap, revert. - if (!hasBit(_allowFailureMap, uint8(i))) { + // Check if failure is allowed + if (!hasBit(_allowFailureMap, uint8(i))) { + // Check if the call failed. + if (!success) { 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)); + } else { + // Check if the call failed. + if (!success) { + // Make sure that the action call did not fail because 63/64 of `gasleft()` was insufficient to execute the external call `.to.call` (see https://eips.ethereum.org/EIPS/eip-150). + // In specific scenarios, i.e. proposal execution where the last action in the action array is allowed to fail, the account calling `execute` could force-fail this action by setting a gas limit + // where 63/64 is insufficient causing the `.to.call` to fail, but where the remaining 1/64 gas are sufficient to successfully finish the `execute` call. + if (gasAfter < gasBefore / 64) { + revert InsufficientGas(); + } + + // Store that this action failed. + failureMap = flipBit(failureMap, uint8(i)); + } } - execResults[i] = response; + execResults[i] = result; unchecked { ++i; diff --git a/packages/contracts/src/core/dao/IDAO.sol b/packages/contracts/src/core/dao/IDAO.sol index 90a7fe4d2..ecad406cc 100644 --- a/packages/contracts/src/core/dao/IDAO.sol +++ b/packages/contracts/src/core/dao/IDAO.sol @@ -37,12 +37,12 @@ interface IDAO { /// @param metadata The IPFS hash of the new metadata object. event MetadataSet(bytes metadata); - /// @notice Executes a list of actions. If no failure map is provided, one failing action results in the entire excution to be reverted. If a non-zero failure map is provided, allowed actions can fail without the remaining actions being reverted. + /// @notice Executes a list of actions. If a zero allow-failure map is provided, a failing action reverts the entire excution. If a non-zero allow-failure map is provided, allowed actions can fail without the entire call being reverted. /// @param _callId The ID of the call. The definition of the value of `callId` is up to the calling contract and can be used, e.g., as a nonce. /// @param _actions The array of actions. /// @param _allowFailureMap A bitmap allowing execution to succeed, even if individual actions might revert. If the bit at index `i` is 1, the execution succeeds even if the `i`th action reverts. A failure map value of 0 requires every action to not revert. /// @return The array of results obtained from the executed actions in `bytes`. - /// @return The constructed failureMap which contains which actions have actually failed. + /// @return The resulting failure map containing the actions have actually failed. function execute( bytes32 _callId, Action[] memory _actions, diff --git a/packages/contracts/src/test/dao/GasConsumerHelper.sol b/packages/contracts/src/test/dao/GasConsumerHelper.sol new file mode 100644 index 000000000..ade06815c --- /dev/null +++ b/packages/contracts/src/test/dao/GasConsumerHelper.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity 0.8.17; + +/// @notice This contract is used for testing to consume gas. +contract GasConsumer { + mapping(uint256 => uint256) public store; + + function consumeGas(uint256 count) external { + for (uint256 i = 0; i < count; i++) { + store[i] = 1; + } + } +} diff --git a/packages/contracts/test/core/dao/dao.ts b/packages/contracts/test/core/dao/dao.ts index 1f4b9fdb7..e2aa78516 100644 --- a/packages/contracts/test/core/dao/dao.ts +++ b/packages/contracts/test/core/dao/dao.ts @@ -2,7 +2,14 @@ import chai, {expect} from 'chai'; import {ethers} from 'hardhat'; import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; -import {DAO, TestERC1155, TestERC20, TestERC721} from '../../../typechain'; +import { + DAO, + TestERC1155, + TestERC20, + TestERC721, + IERC1271__factory, + GasConsumer__factory, +} from '../../../typechain'; import {findEvent, DAO_EVENTS} from '../../../utils/event'; import {flipBit} from '../../test-utils/bitmap'; @@ -16,7 +23,6 @@ import { import {getInterfaceID} from '../../test-utils/interfaces'; import {OZ_ERRORS} from '../../test-utils/error'; -import {IERC1271__factory} from '../../../typechain/factories/IERC1271__factory'; import {smock} from '@defi-wonderland/smock'; import {deployWithProxy} from '../../test-utils/proxy'; import {UNREGISTERED_INTERFACE_RETURN} from './callback-handler'; @@ -368,6 +374,39 @@ describe('DAO', function () { expect(event.args.execResults[0]).to.equal(data.successActionResult); }); + it('reverts if failure is allowed but not enough gas is provided', async () => { + const GasConsumer = new GasConsumer__factory(signers[0]); + let gasConsumer = await GasConsumer.deploy(); + + const gasConsumingAction = { + to: gasConsumer.address, + data: GasConsumer.interface.encodeFunctionData('consumeGas', [20]), + value: 0, + }; + + let allowFailureMap = ethers.BigNumber.from(0); + allowFailureMap = flipBit(0, allowFailureMap); // allow the action to fail + + const expectedGas = await dao.estimateGas.execute( + ZERO_BYTES32, + [gasConsumingAction], + allowFailureMap + ); // exact gas required: 495453 + // Providing less gas causes the `to.call` of the `gasConsumingAction` to fail, but is still enough for the overall `dao.execute` call to finish successfully. + + await expect( + dao.execute(ZERO_BYTES32, [gasConsumingAction], allowFailureMap, { + gasLimit: expectedGas.sub(500), + }) + ).to.be.revertedWithCustomError(dao, 'InsufficientGas'); + + await expect( + dao.execute(ZERO_BYTES32, [gasConsumingAction], allowFailureMap, { + gasLimit: expectedGas, + }) + ).to.not.be.reverted; + }); + describe('Transfering tokens', async () => { const amount = ethers.utils.parseEther('1.23'); const options = {value: amount};