From 549ea1605538a4aa0571fa8e4d44e0e762e60720 Mon Sep 17 00:00:00 2001
From: Mikhail <16622558+mmv08@users.noreply.github.com>
Date: Mon, 9 Sep 2024 11:19:58 +0200
Subject: [PATCH] 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.
---
 .../allowances/contracts/AllowanceModule.sol  |  2 +-
 .../test/allowanceRecurring.spec.ts           | 20 +++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/modules/allowances/contracts/AllowanceModule.sol b/modules/allowances/contracts/AllowanceModule.sol
index 8a3525a5..92564d9f 100644
--- a/modules/allowances/contracts/AllowanceModule.sol
+++ b/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;
diff --git a/modules/allowances/test/allowanceRecurring.spec.ts b/modules/allowances/test/allowanceRecurring.spec.ts
index 3006766d..63a0be1e 100644
--- a/modules/allowances/test/allowanceRecurring.spec.ts
+++ b/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
+  })
 })