L-1: Add a reset period check to prevent the transaction failing and …

…using all gas (#492)

This PR fixes a low severity audit finding by Ackee:
> It is possible to set the resetTimeMin to 0. When the resetBaseMin is
bigger
than 0 and the resetTimeMin is 0, then the branch with modulo operation
is
triggered and it will cause a division by zero. Since it is also in
Solidity version
<0.8.0, it consumes all gas.

modules/allowances/contracts/AllowanceModule.sol

@@ -89,7 +89,7 @@ contract AllowanceModule is SignatureDecoder {
         // solium-disable-next-line security/no-block-members
         uint32 currentMin = uint32(block.timestamp / 60);
         if (resetBaseMin > 0) {
-            require(resetBaseMin <= currentMin, "resetBaseMin <= currentMin");
+            require(resetBaseMin <= currentMin && resetTimeMin > 0, "resetBaseMin <= currentMin && resetTimeMin > 0");
             allowance.lastResetMin = currentMin - ((currentMin - resetBaseMin) % resetTimeMin);
         } else if (allowance.lastResetMin == 0) {
             allowance.lastResetMin = currentMin;

modules/allowances/test/allowanceRecurring.spec.ts

@@ -133,4 +133,24 @@ describe('AllowanceModule allowanceRecurring', () => {
     expect(expectedLastReset).to.be.equal(resetLast)
     expect(4).to.equal(nonce)
   })
+
+  it('Reverts when trying to set up a recurring allowance with reset period of 0', async () => {
+    const { safe, allowanceModule, token, owner, alice } = await loadFixture(setup)
+    const tokenAddress = await token.getAddress()
+
+    // add alice as delegate
+    await execSafeTransaction(safe, await allowanceModule.addDelegate.populateTransaction(alice.address), owner)
+
+    // create an allowance for alice
+    const configResetPeriod = 0
+    const configResetBase = nowInMinutes()
+
+    await expect(
+      execSafeTransaction(
+        safe,
+        await allowanceModule.setAllowance.populateTransaction(alice.address, tokenAddress, 100, configResetPeriod, configResetBase),
+        owner,
+      ),
+    ).to.be.reverted
+  })
 })