Update onlyOwnersGuard, fix incorrect test

safe-global · Nov 11, 2023 · 650cd2b · 650cd2b
1 parent 52ce39c
commit 650cd2b
Show file tree

Hide file tree

Showing 2 changed files with 13 additions and 16 deletions.
diff --git a/contracts/examples/guards/OnlyOwnersGuard.sol b/contracts/examples/guards/OnlyOwnersGuard.sol
@@ -6,16 +6,14 @@ import {Enum} from "../../common/Enum.sol";
 import {BaseGuard} from "../../base/GuardManager.sol";
 
 interface ISafe {
-    function getOwners() external view returns (address[] memory);
+    function isOwner(address owner) external view returns (bool);
 }
 
 /**
  * @title OnlyOwnersGuard - Only allows owners to execute transactions.
  * @author Richard Meissner - @rmeissner
  */
 contract OnlyOwnersGuard is BaseGuard {
-    ISafe public safe;
-
     constructor() {}
 
     // solhint-disable-next-line payable-fallback
@@ -43,16 +41,7 @@ contract OnlyOwnersGuard is BaseGuard {
         bytes memory,
         address msgSender
     ) external view override {
-        // Only owners can exec
-        address[] memory owners = ISafe(msg.sender).getOwners();
-        for (uint256 i = 0; i < owners.length; i++) {
-            if (owners[i] == msgSender) {
-                return;
-            }
-        }
-
-        // msg sender is not an owner
-        revert("msg sender is not allowed to exec");
+        require(ISafe(msg.sender).isOwner(msgSender), "msg sender is not allowed to exec");
     }
 
     function checkAfterExecution(bytes32, bool) external view override {}

diff --git a/test/guards/OnlyOwnersGuard.spec.ts b/test/guards/OnlyOwnersGuard.spec.ts
@@ -1,7 +1,13 @@
 import { expect } from "chai";
 import hre, { deployments, ethers } from "hardhat";
 import { getMock, getSafeWithOwners } from "../utils/setup";
-import { buildSafeTransaction, executeContractCallWithSigners, executeTxWithSigners } from "../../src/utils/execution";
+import {
+    buildSafeTransaction,
+    executeContractCallWithSigners,
+    executeTx,
+    executeTxWithSigners,
+    safeSignTypedData,
+} from "../../src/utils/execution";
 
 describe("OnlyOwnersGuard", () => {
     const setupTests = deployments.createFixture(async ({ deployments }) => {
@@ -40,13 +46,15 @@ describe("OnlyOwnersGuard", () => {
             const {
                 safe,
                 mock,
-                signers: [, user2],
+                signers: [user1, user2],
             } = await setupTests();
             const nonce = await safe.nonce();
             const mockAddress = await mock.getAddress();
             const safeTx = buildSafeTransaction({ to: mockAddress, data: "0xbaddad42", nonce });
+            const signature = await safeSignTypedData(user1, await safe.getAddress(), safeTx);
+            const safeUser2 = await safe.connect(user2);
 
-            await expect(executeTxWithSigners(safe, safeTx, [user2])).to.be.reverted;
+            await expect(executeTx(safeUser2, safeTx, [signature])).to.be.revertedWith("msg sender is not allowed to exec");
         });
     });
 });