Fix 1/64 problem in DAO executor (#333)

* Fix 1/64 problem in DAO executor

* Added test

* Explicit types and added comment

* Maintained changelog

* Fixed comment

* Addressed review comment

* Store gasleft() before and after every action

* Adapt changelog

* fix: remove .only

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

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;

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,

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

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