1.4.0: Fix getModulesPaginated

contracts/base/ModuleManager.sol

@@ -107,23 +107,37 @@ contract ModuleManager is SelfAuthorized, Executor {
     }
 
     /// @dev Returns array of modules.
-    /// @param start Start of the page.
-    /// @param pageSize Maximum number of modules that should be returned.
+    ///      If all entries fit into a single page, the next pointer will be 0x1.
+    ///      If another page is present, next will be the last element of the returned array.
+    /// @param start Start of the page. Has to be a module or start pointer (0x1 address)
+    /// @param pageSize Maximum number of modules that should be returned. Has to be > 0
     /// @return array Array of modules.
     /// @return next Start of the next page.
     function getModulesPaginated(address start, uint256 pageSize) external view returns (address[] memory array, address next) {
+        require(start == SENTINEL_MODULES || isModuleEnabled(start), "GS105");
+        require(pageSize > 0, "GS106");
         // Init array with max page size
         array = new address[](pageSize);
 
         // Populate return array
         uint256 moduleCount = 0;
         address currentModule = modules[start];
-        while (currentModule != address(0x0) && currentModule != SENTINEL_MODULES && moduleCount < pageSize) {
+        while (currentModule != address(0) && currentModule != SENTINEL_MODULES && moduleCount < pageSize) {
             array[moduleCount] = currentModule;
             currentModule = modules[currentModule];
             moduleCount++;
         }
         next = currentModule;
+        // Because of the argument validation we can assume that
+        // the `currentModule` will always be either a module address
+        // or sentinel address (aka the end). If we haven't reached the end
+        // inside the loop, we need to set the next pointer to the last element
+        // because it skipped over to the next module which is neither included
+        // in the current page nor won't be included in the next one
+        // if you pass it as a start.
+        if (next != SENTINEL_MODULES) {
+            next = array[moduleCount - 1];
+        }
         // Set correct size of returned array
         // solhint-disable-next-line no-inline-assembly
         assembly {

docs/error_codes.md

@@ -29,6 +29,8 @@
 - `GS102`: `Module has already been added`
 - `GS103`: `Invalid prevModule, module pair provided`
 - `GS104`: `Method can only be called from an enabled module`
+- `GS105`: `Invalid starting point for fetching paginated modules`
+- `GS106`: `Invalid page size for fetching paginated modules`
 
 ### Owner management related
 - `GS200`: `Owners have already been setup`

test/core/GnosisSafe.ModuleManager.spec.ts

@@ -9,7 +9,7 @@ import { AddressOne } from "../../src/utils/constants";
 
 describe("ModuleManager", async () => {
 
-    const [user1, user2] = waffle.provider.getWallets();
+    const [user1, user2, user3] = waffle.provider.getWallets();
 
     const setupTests = deployments.createFixture(async ({ deployments }) => {
         await deployments.fixture();
@@ -50,7 +50,7 @@ describe("ModuleManager", async () => {
             ).to.revertedWith("GS013")
         })
 
-        it('emits event for new module', async () => {
+        it('emits event for a new module', async () => {
             const { safe } = await setupTests()
             await expect(
                 executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
@@ -244,4 +244,47 @@ describe("ModuleManager", async () => {
             ).to.be.deep.eq([false, "0x08c379a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000013536f6d652072616e646f6d206d65737361676500000000000000000000000000"])
         })
     })
+
+    describe("getModulesPaginated", async () => {
+        it('requires page size to be greater than 0', async () => {
+            const { safe } = await setupTests()
+            await expect(safe.getModulesPaginated(AddressOne, 0)).to.be.revertedWith("GS106")
+        })
+
+        it('requires start to be a module or start pointer', async () => {
+            const { safe } = await setupTests()
+
+            await expect(safe.getModulesPaginated(AddressZero, 1)).to.be.reverted
+            await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user1])
+            expect(await safe.getModulesPaginated(user1.address, 1)).to.be.deep.equal([[], AddressOne])
+            await expect(safe.getModulesPaginated(user2.address, 1)).to.be.revertedWith("GS105")
+        })
+
+        it('Returns all modules over multiple pages', async () => {
+            const { safe } = await setupTests()
+            await expect(
+                executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user1])
+            ).to.emit(safe, "EnabledModule").withArgs(user1.address)
+
+            await expect(
+                executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
+            ).to.emit(safe, "EnabledModule").withArgs(user2.address)
+
+            await expect(
+                executeContractCallWithSigners(safe, safe, "enableModule", [user3.address], [user1])
+            ).to.emit(safe, "EnabledModule").withArgs(user3.address)
+
+            await expect(await safe.isModuleEnabled(user1.address)).to.be.true
+            await expect(await safe.isModuleEnabled(user2.address)).to.be.true
+            await expect(await safe.isModuleEnabled(user3.address)).to.be.true
+            /*
+            This will pass the test which is not correct
+            await expect(await safe.getModulesPaginated(AddressOne, 1)).to.be.deep.equal([[user3.address], user2.address])
+            await expect(await safe.getModulesPaginated(user2.address, 1)).to.be.deep.equal([[user1.address], AddressOne])
+            */
+            await expect(await safe.getModulesPaginated(AddressOne, 1)).to.be.deep.equal([[user3.address], user3.address])
+            await expect(await safe.getModulesPaginated(user3.address, 1)).to.be.deep.equal([[user2.address], user2.address])
+            await expect(await safe.getModulesPaginated(user2.address, 1)).to.be.deep.equal([[user1.address], AddressOne])
+        })
+    })
 })

test/core/GnosisSafe.Setup.spec.ts

@@ -29,10 +29,15 @@ describe("GnosisSafe", async () => {
             await expect(
                 await singleton.getThreshold()
             ).to.be.deep.eq(BigNumber.from(1))
-            await expect(
-                await singleton.getModulesPaginated(AddressOne, 10)
-            ).to.be.deep.eq([[], AddressZero])
 
+            // Because setup wasn't called on the singleton it breaks the assumption made 
+            // within `getModulesPaginated` method that the linked list will be always correctly
+            // initialized with 0x1 as a starting element and 0x1 as the end
+            // But because `setupModules` wasn't called, it is empty.
+            await expect(
+                singleton.getModulesPaginated(AddressOne, 10)
+            ).to.be.reverted
+
             // "Should not be able to retrieve owners (currently the contract will run in an endless loop when not initialized)"
             await expect(
                 singleton.getOwners()