Fix/gas dos

packages/erc3k/contracts/ERC3000Data.sol

@@ -32,11 +32,12 @@ library ERC3000Data {
     }
 
     struct Config {
-        uint256 executionDelay;
-        Collateral scheduleDeposit;
-        Collateral challengeDeposit;
-        address resolver;
-        bytes rules;
+        uint256 executionDelay; // how many seconds to wait before being able to call `execute`.
+        Collateral scheduleDeposit; // fees for scheduling
+        Collateral challengeDeposit; // fees for challenging
+        address resolver;  // resolver that will rule the disputes
+        bytes rules; // rules of how DAO should be managed
+        uint256 maxCalldataSize; // max calldatasize for the schedule
     }
 
     struct Collateral {

packages/erc3k/test/erc3k-data.test.ts

@@ -14,6 +14,7 @@ let deposit = {
       challengeDeposit: deposit,
       resolver: '0xb794f5ea0ba39494ce839613fffba74279579268',
       rules: '0x00',
+      maxCalldataSize: 100000 // initial maxCalldatasize
     },
     payload: {
       nonce: 1,
@@ -82,7 +83,8 @@ function getConfigHash(): string {
           'tuple(address token, uint256 amount) scheduleDeposit, ' +
           'tuple(address token, uint256 amount) challengeDeposit, ' +
           'address resolver, ' +
-          'bytes rules' +
+          'bytes rules,' +
+          'uint256 maxCalldataSize' +
           ')',
       ],
       [container.config]

packages/govern-core/contracts/pipelines/GovernQueue.sol

@@ -103,7 +103,13 @@ contract GovernQueue is IERC3000, IArbitrable, AdaptiveERC165, ACL {
         require(_container.payload.executionTime >= _container.config.executionDelay.add(block.timestamp), "queue: bad delay");
         // ensure that the submitter of the payload is also the sender of this call
         require(_container.payload.submitter == msg.sender, "queue: bad submitter");
-
+        // Restrict the size of calldata to _container.config.maxCalldataSize to make sure challenge function stays callable
+        uint calldataSize;
+        assembly {
+            calldataSize := calldatasize()
+        }
+        require(calldataSize <= _container.config.maxCalldataSize, "calldatasize: limit exceeded");
+        // store and set container's hash
         containerHash = ERC3000Data.containerHash(_container.payload.hash(), _configHash);
         queue[containerHash].checkAndSetState(
             GovernQueueStateLib.State.None, // ensure that the state for this container is None
@@ -308,6 +314,21 @@ contract GovernQueue is IERC3000, IArbitrable, AdaptiveERC165, ACL {
         internal
         returns (bytes32)
     {
+        // validate collaterals by calling balanceOf on their interface
+        if(_config.challengeDeposit.amount != 0) {
+            (bool ok, bytes memory value) = _config.challengeDeposit.token.call(
+                abi.encodeWithSelector(ERC20.balanceOf.selector, address(this))
+            );
+            require(ok && value.length > 0, "queue: bad config");
+        }
+
+        if(_config.scheduleDeposit.amount != 0) {
+            (bool ok, bytes memory value) = _config.scheduleDeposit.token.call(
+                abi.encodeWithSelector(ERC20.balanceOf.selector, address(this))
+            );
+            require(ok && value.length > 0, "queue: bad config");
+        }
+
         configHash = _config.hash();
 
         emit Configured(configHash, msg.sender, _config);

packages/govern-core/test/pipelines/container.ts

@@ -1,16 +1,17 @@
 export const container: any = {
   'config': {
-    'executionDelay': 0,
+    'executionDelay': 3600, // how many seconds to wait before being able to call `execute`.
     'scheduleDeposit': {
-      'token': '',
+      'token': '0x0000000000000000000000000000000000000000',
       'amount': 100
     },
     'challengeDeposit': {
-      'token': '',
+      'token': '0x0000000000000000000000000000000000000000',
       'amount': 100
     },
     'resolver': '0x0000000000000000000000000000000000000000',
-    'rules': '0x'
+    'rules': '0x',
+    'maxCalldataSize': 100000 // initial maxCalldatasize
   },
   'payload': {
     'nonce': 1,

packages/govern-core/test/pipelines/govern-queue.test.ts

@@ -57,6 +57,7 @@ describe('Govern Queue', function() {
     BAD_CONFIG: 'queue: bad config',
     BAD_DELAY: 'queue: bad delay',
     BAD_SUBMITTER: 'queue: bad submitter',
+    CALLDATASIZE_LIMIT: 'calldatasize: limit exceeded',
     WAIT_MORE: 'queue: wait more',
     BAD_FEE_PULL: 'queue: bad fee pull',
     BAD_APPROVE: 'queue: bad approve',
@@ -85,6 +86,10 @@ describe('Govern Queue', function() {
   const disputeId = 1000
   const zeroByteHash = "0x0000000000000000000000000000000000000000"
 
+  // grab the original, honest values that queue contract gets deployed with.
+  const executionDelay  = container.config.executionDelay
+  const maxCalldataSize = container.config.maxCalldataSize
+
   let executor: ERC3000ExecutorMock
 
   const createArbitratorMock = async () => {
@@ -154,7 +159,7 @@ describe('Govern Queue', function() {
   context('GovernQueue.schedule', () => {
 
     before(async () => {
-      container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + 100
+      container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + executionDelay + 100
     })
 
     it('emits the expected events and adds the container to the queue', async () => {
@@ -187,20 +192,34 @@ describe('Govern Queue', function() {
       expect(await testToken.balanceOf(ownerAddr)).to.equal(ownerTokenAmount - container.config.scheduleDeposit.amount)
     })
 
+
+    it('reverts with "calldatasize: limit exceeded"', async () => {
+      container.config.maxCalldataSize = 100
+
+      await gq.configure(container.config);
+
+      await expect(gq.schedule(container)).to.be.revertedWith(ERRORS.CALLDATASIZE_LIMIT)
+
+      container.config.maxCalldataSize = maxCalldataSize
+      await gq.configure(container.config);
+
+    })
+
+
     it('reverts with "queue: bad config"', async () => {
       container.config.executionDelay = 100
 
       await expect(gq.schedule(container)).to.be.revertedWith(ERRORS.BAD_CONFIG)
 
-      container.config.executionDelay = 0
+      container.config.executionDelay = executionDelay
     })
 
     it('reverts with "queue: bad delay"', async () => {
       container.payload.executionTime = 0
 
       await expect(gq.schedule(container)).to.be.revertedWith(ERRORS.BAD_DELAY)
 
-      container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + 1000
+      container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + executionDelay + 100
     })
 
     it('reverts with "queue: bad submitter"', async () => {
@@ -221,14 +240,14 @@ describe('Govern Queue', function() {
   context('GovernQueue.execute', async () => {
 
     before(async () => {
-      container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + 100
+      container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + executionDelay + 100
     })
 
     it('emits the expected events and updates the container state', async () => {
       await testToken.approve(gq.address, container.config.scheduleDeposit.amount)
       await gq.schedule(container)
 
-      await ethers.provider.send('evm_increaseTime', [150])
+      await ethers.provider.send('evm_increaseTime', [executionDelay + 100])
 
       const ownerBalance = (await testToken.balanceOf(ownerAddr)).toNumber()
       const containerHash = getContainerHash(container, gq.address, chainId)
@@ -249,7 +268,11 @@ describe('Govern Queue', function() {
     })
 
     it('reverts with "queue: wait more"', async () => {
-      container.payload.executionTime = container.payload.executionTime * 2
+      await testToken.approve(gq.address, container.config.scheduleDeposit.amount)
+
+      container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + executionDelay + 100
+
+      await gq.schedule(container)
 
       await expect(gq.execute(container)).to.be.revertedWith(ERRORS.WAIT_MORE)
     })
@@ -259,7 +282,7 @@ describe('Govern Queue', function() {
   context('GovernQueue.challenge', () => {
 
     before(async () => {
-      container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + 100
+      container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + executionDelay + 100
     })
 
     it('executes as expected', async () => {
@@ -282,7 +305,7 @@ describe('Govern Queue', function() {
 
       expect(await gq.challengerCache(containerHash)).to.equal(ownerAddr)
 
-      expect(await gq.disputeItemCache(containerHash, container.config.resolver)).to.equal(disputeId+1)
+      expect(await gq.disputeItemCache(containerHash, container.config.resolver)).to.equal(disputeId + 1)
 
       expect(await testToken.balanceOf(ownerAddr)).to.equal(
         ownerBalance -(container.config.challengeDeposit.amount + container.config.challengeDeposit.amount + disputeFee)
@@ -325,7 +348,7 @@ describe('Govern Queue', function() {
   context('GovernQueue.resolve', () => {
 
     before(async () => {
-      container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + 100
+      container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + executionDelay + 100
     })
 
     it('reverts with bad dispute id wrong dispute id is passed', async () => {
@@ -427,7 +450,7 @@ describe('Govern Queue', function() {
     context('GovernQueue.veto', () => {
 
       before(async () => {
-        container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + 100
+        container.payload.executionTime = (await ethers.provider.getBlock('latest')).timestamp + executionDelay + 100
       })
 
       it('reverts when the container is not scheduled or challenged', async () => {
@@ -515,5 +538,19 @@ describe('Govern Queue', function() {
 
         expect(await gq.configHash()).to.equal(configHash)
       })
+
+      it("reverts if schedule collateral doesn't have balanceOf", async () => {
+        container.config.scheduleDeposit.amount = 1
+        container.config.scheduleDeposit.token = gq.address
+
+        await expect(gq.configure(container.config)).to.be.revertedWith(ERRORS.BAD_CONFIG)
+      })
+
+      it("reverts if challenge collateral doesn't have balanceOf", async () => {
+        container.config.scheduleDeposit.amount = 1
+        container.config.scheduleDeposit.token = gq.address
+
+        await expect(gq.configure(container.config)).to.be.revertedWith(ERRORS.BAD_CONFIG)
+      })
     })
   })

packages/govern-core/test/pipelines/helpers.ts

@@ -59,7 +59,8 @@ export function getConfigHash(container: any): string {
         'tuple(address token, uint256 amount) scheduleDeposit, ' +
         'tuple(address token, uint256 amount) challengeDeposit, ' +
         'address resolver, ' +
-        'bytes rules' +
+        'bytes rules, ' +
+        'uint256 maxCalldataSize' +
         ')',
       ],
       [container.config]
@@ -120,7 +121,8 @@ export function getEncodedContainer(container: any): string {
             'uint256 amount' +
           ') challengeDeposit, ' +
           'address resolver, ' +
-          'bytes rules' +
+          'bytes rules, ' +
+          'uint256 maxCalldataSize' +
         ') config' +
       ')'
     ],

packages/govern-create/contracts/GovernBaseFactory.sol

@@ -21,6 +21,13 @@ contract GovernBaseFactory {
     GovernTokenFactory public tokenFactory;
     GovernRegistry public registry;
 
+    struct Token {
+        IERC20 tokenAddress;
+        uint8  tokenDecimals;
+        string tokenName;
+        string tokenSymbol;
+    }
+
     constructor(
         GovernRegistry _registry,
         GovernFactory _governFactory,
@@ -33,31 +40,31 @@ contract GovernBaseFactory {
         registry = _registry;
     }
 
-    function newGovernWithoutConfig(
+    function newGovern(
         string calldata _name,
-        IERC20 _token,
-        string calldata _tokenName,
-        string calldata _tokenSymbol,
+        Token calldata _token,
+        ERC3000Data.Config calldata _config,
         bool _useProxies
     ) external returns (Govern govern, GovernQueue queue) {
         bytes32 salt = _useProxies ? keccak256(abi.encodePacked(_name)) : bytes32(0);
 
-        queue = queueFactory.newQueue(address(this), dummyConfig(), salt);
+        queue = queueFactory.newQueue(address(this), _config, salt);
         govern = governFactory.newGovern(queue, salt);
 
-        if (address(_token) == address(0)) {
-            (_token,) = tokenFactory.newToken(
+        IERC20 token = _token.tokenAddress;
+        if (address(token) == address(0)) {
+            (token,) = tokenFactory.newToken(
                 govern,
-                _tokenName,
-                _tokenSymbol,
-                18, // NOTE: hardcoding due to stack to deep issues
+                _token.tokenName,
+                _token.tokenSymbol,
+                _token.tokenDecimals,
                 msg.sender,
                 1 * 10 ** 18,
                 _useProxies
             );
         }
 
-        registry.register(govern, queue, _token, _name, "");
+        registry.register(govern, queue, token, _name, "");
 
         ACLData.BulkItem[] memory items = new ACLData.BulkItem[](6);
         items[0] = ACLData.BulkItem(ACLData.BulkOp.Grant, queue.schedule.selector, ANY_ADDR);
@@ -69,15 +76,5 @@ contract GovernBaseFactory {
 
         queue.bulk(items);
     }
-
-    function dummyConfig() internal pure returns (ERC3000Data.Config memory) {
-        ERC3000Data.Collateral memory noCollateral;
-        return ERC3000Data.Config(
-            0,
-            noCollateral,
-            noCollateral,
-            address(0),
-            ""
-        );
-    }
+
 }

packages/govern-create/contracts/core-factories/GovernQueueFactory.sol

@@ -32,7 +32,8 @@ contract GovernQueueFactory {
             noCollateral,
             noCollateral,
             address(0),
-            ""
+            "",
+            100000 // initial maxCalldatasize
         );
         base = address(new GovernQueue(address(2), config));
     }

packages/govern-create/contracts/test/GovernQueueFactoryMock.sol

@@ -17,10 +17,10 @@ contract GovernQueueFactoryMock{
 
     bytes4 public constant ROOT_ROLE = "0x";
 
-    function schedule() public {}
-    function execute() public {}
-    function challenge() public {}
-    function configure() public {}
+    function schedule()  public pure  {}
+    function execute()   public pure  {}
+    function challenge() public pure  {}
+    function configure() public pure  {}
 
 
 

packages/govern-create/deploy/verify.ts

@@ -4,16 +4,17 @@ import { TASK_ETHERSCAN_VERIFY } from 'hardhat-deploy'
 
 import { verifyContract } from '../utils/etherscan'
 
-const ZERO_ADDR = `0x${'00'.repeat(20)}`
+const RESOLVER_ADDRR = `0x${'00'.repeat(20)}` // the address of the resolver where the disputes happen...
 const TWO_ADDRR = `0x${'00'.repeat(19)}02`
 const NO_TOKEN = `0x${'00'.repeat(20)}`
 
 const dummyConfig = {
-  executionDelay: '0',
+  executionDelay: 0, // how many seconds to wait before being able to call `execute`
   scheduleDeposit: [NO_TOKEN, '0'],
   challengeDeposit: [NO_TOKEN, '0'],
-  resolver: ZERO_ADDR,
+  resolver: RESOLVER_ADDRR,
   rules: '0x',
+  maxCalldataSize: 100000 // initial maxCalldatasize
 }
 
 function delay(ms: number) {