check that fallback handler address is not equal to self

safe-global · Mar 20, 2023 · 24c3b51 · 24c3b51
1 parent ba526d0
commit 24c3b51
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 0 deletions.
diff --git a/contracts/base/FallbackManager.sol b/contracts/base/FallbackManager.sol
@@ -18,6 +18,8 @@ abstract contract FallbackManager is SelfAuthorized {
      *  @param handler contract to handle fallback calls.
      */
     function internalSetFallbackHandler(address handler) internal {
+        require(handler != address(this), "GS400");
+
         bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;
         // solhint-disable-next-line no-inline-assembly
         assembly {
@@ -29,6 +31,7 @@ abstract contract FallbackManager is SelfAuthorized {
      * @notice Set Fallback Handler to `handler` for the Safe.
      * @dev Only fallback calls without value and with data will be forwarded.
      *      This can only be done via a Safe transaction.
+     *      Cannot be set to the Safe itself.
      * @param handler contract to handle fallback calls.
      */
     function setFallbackHandler(address handler) public authorized {

diff --git a/docs/error_codes.md b/docs/error_codes.md
@@ -44,3 +44,6 @@
 
 ### Guard management related
 - `GS300`: `Guard does not implement IERC165`
+
+### Fallback handler related
+-   `GS400`: `Fallback handler cannot be set to self`
diff --git a/test/core/Safe.FallbackManager.spec.ts b/test/core/Safe.FallbackManager.spec.ts
@@ -151,5 +151,14 @@ describe("FallbackManager", async () => {
                     "0000000000000000",
             );
         });
+
+        it("cannot be set to self", async () => {
+            const { safe } = await setupWithTemplate();
+            // Setup Safe
+            await safe.setup([user1.address], 1, AddressZero, "0x", AddressZero, AddressZero, 0, AddressZero);
+
+            // The transaction execution function doesn't bubble up revert messages so we check for a generic transaction fail code GS013
+            expect(executeContractCallWithSigners(safe, safe, "setFallbackHandler", [safe.address], [user1])).to.be.revertedWith("GS013");
+        });
     });
 });
diff --git a/test/core/Safe.Setup.spec.ts b/test/core/Safe.Setup.spec.ts
@@ -385,5 +385,13 @@ describe("Safe", async () => {
                 template.setup([user1.address], 1, user2.address, "0xbeef73", AddressZero, AddressZero, 0, AddressZero),
             ).to.be.revertedWith("GS002");
         });
+
+        it("should fail if tried to set the fallback handler address to self", async () => {
+            const { template } = await setupTests();
+
+            await expect(
+                template.setup([user1.address], 1, AddressZero, "0x", template.address, AddressZero, 0, AddressZero),
+            ).to.be.revertedWith("GS400");
+        });
     });
 });