Closes #274: Revert on failure with safeTxGas 0 (#283)

safe-global · May 6, 2021 · e46c776 · e46c776
1 parent 18dd7ad
commit e46c776
Show file tree

Hide file tree

Showing 9 changed files with 130 additions and 94 deletions.
diff --git a/contracts/GnosisSafe.sol b/contracts/GnosisSafe.sol
@@ -170,6 +170,9 @@ contract GnosisSafe
             // We only substract 2500 (compared to the 3000 before) to ensure that the amount passed is still higher than safeTxGas
             success = execute(to, value, data, operation, gasPrice == 0 ? (gasleft() - 2500) : safeTxGas);
             gasUsed = gasUsed.sub(gasleft());
+            // If no safeTxGas and no gasPrice was set (e.g. both are 0), then the internal tx is required to be successful
+            // This makes it possible to use `estimateGas` without issues, as it searches for the minimum gas where the tx doesn't revert
+            require(success || safeTxGas != 0 || gasPrice != 0, "GS013");
             // We transfer the calculated tx costs to the tx.origin to avoid sending it to intermediate contracts that have made calls
             uint256 payment = 0;
             if (gasPrice > 0) {

diff --git a/docs/error_codes.md b/docs/error_codes.md
@@ -4,10 +4,11 @@
 - `GS000`: `Could not finish initialization`
 - `GS001`: `Threshold needs to be defined`
 
-### General gas related
-- `GS010`: `Not enough gas to execute safe transaction`
+### General gas/ execution related
+- `GS010`: `Not enough gas to execute Safe transaction`
 - `GS011`: `Could not pay gas costs with ether`
 - `GS012`: `Could not pay gas costs with token`
+- `GS013`: `Safe transaction failed when gasPrice and safeTxGas were 0`
 
 ### General signature validation related
 - `GS020`: `Signatures data too short`

diff --git a/test/core/GnosisSafe.Execution.spec.ts b/test/core/GnosisSafe.Execution.spec.ts
@@ -67,13 +67,29 @@ describe("GnosisSafe", async () => {
             ).to.be.eq("0x" + "baddad".padEnd(64, "0"))
         })
 
-        it('should emit event for failed call execution', async () => {
+        it('should emit event for failed call execution if safeTxGas > 0', async () => {
             const { safe, reverter } = await setupTests()
             await expect(
-                executeContractCallWithSigners(safe, reverter, "revert", [], [user1])
+                executeContractCallWithSigners(safe, reverter, "revert", [], [user1], false, { safeTxGas: 1 })
+            ).to.emit(safe, "ExecutionFailure")
+        })
+
+        it('should emit event for failed call execution if gasPrice > 0', async () => {
+            const { safe, reverter } = await setupTests()
+            // Fund refund
+            await user1.sendTransaction({ to: safe.address, value: 10000000 })
+            await expect(
+                executeContractCallWithSigners(safe, reverter, "revert", [], [user1], false, { gasPrice: 1 })
             ).to.emit(safe, "ExecutionFailure")
         })
 
+        it('should revert for failed call execution if gasPrice == 0 and safeTxGas == 0', async () => {
+            const { safe, reverter } = await setupTests()
+            await expect(
+                executeContractCallWithSigners(safe, reverter, "revert", [], [user1])
+            ).to.revertedWith("GS013")
+        })
+
         it('should emit event for successful delegatecall execution', async () => {
             const { safe, storageSetter } = await setupTests()
             await expect(
@@ -89,14 +105,29 @@ describe("GnosisSafe", async () => {
             ).to.be.eq("0x" + "".padEnd(64, "0"))
         })
 
-        it('should emit event for failed delegatecall execution', async () => {
+        it('should emit event for failed delegatecall execution  if safeTxGas > 0', async () => {
             const { safe, reverter } = await setupTests()
-            const txHash = calculateSafeTransactionHash(safe, buildContractCall(reverter, "revert", [], await safe.nonce(), true), await chainId())
+            const txHash = calculateSafeTransactionHash(safe, buildContractCall(reverter, "revert", [], await safe.nonce(), true, { safeTxGas: 1 }), await chainId())
             await expect(
-                executeContractCallWithSigners(safe, reverter, "revert", [], [user1], true)
+                executeContractCallWithSigners(safe, reverter, "revert", [], [user1], true, { safeTxGas: 1 })
             ).to.emit(safe, "ExecutionFailure").withArgs(txHash, 0)
         })
 
+        it('should emit event for failed delegatecall execution if gasPrice > 0', async () => {
+            const { safe, reverter } = await setupTests()
+            await user1.sendTransaction({ to: safe.address, value: 10000000 })
+            await expect(
+                executeContractCallWithSigners(safe, reverter, "revert", [], [user1], true, { gasPrice: 1 })
+            ).to.emit(safe, "ExecutionFailure")
+        })
+
+        it('should emit event for failed delegatecall execution if gasPrice == 0 and safeTxGas == 0', async () => {
+            const { safe, reverter } = await setupTests()
+            await expect(
+                executeContractCallWithSigners(safe, reverter, "revert", [], [user1], true)
+            ).to.revertedWith("GS013")
+        })
+
         it('should revert on unknown operation', async () => {
             const { safe } = await setupTests()
             const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce(), operation: 2 })

diff --git a/test/core/GnosisSafe.ModuleManager.spec.ts b/test/core/GnosisSafe.ModuleManager.spec.ts
@@ -30,14 +30,14 @@ describe("ModuleManager", async () => {
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "enableModule", [AddressOne], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not set 0 Address', async () => {
             const { safe } = await setupTests()
             await expect(
                 executeContractCallWithSigners(safe, safe, "enableModule", [AddressZero], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not add module twice', async () => {
@@ -47,7 +47,7 @@ describe("ModuleManager", async () => {
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('emits event for new module', async () => {
@@ -94,30 +94,30 @@ describe("ModuleManager", async () => {
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "disableModule", [AddressOne, AddressOne], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not set 0 Address', async () => {
             const { safe } = await setupTests()
             await expect(
                 executeContractCallWithSigners(safe, safe, "disableModule", [AddressOne, AddressZero], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('Invalid prevModule, module pair provided - Invalid target', async () => {
             const { safe } = await setupTests()
             await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
             await expect(
                 executeContractCallWithSigners(safe, safe, "disableModule", [AddressOne, user1.address], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('Invalid prevModule, module pair provided - Invalid sentinel', async () => {
             const { safe } = await setupTests()
             await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
             await expect(
                 executeContractCallWithSigners(safe, safe, "disableModule", [AddressZero, user2.address], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('Invalid prevModule, module pair provided - Invalid source', async () => {
@@ -126,7 +126,7 @@ describe("ModuleManager", async () => {
             await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
             await expect(
                 executeContractCallWithSigners(safe, safe, "disableModule", [user1.address, user2.address], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('emits event for disabled module', async () => {

diff --git a/test/core/GnosisSafe.OwnerManager.spec.ts b/test/core/GnosisSafe.OwnerManager.spec.ts
@@ -29,22 +29,22 @@ describe("OwnerManager", async () => {
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [safe.address, 1], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not set sentinel', async () => {
             const { safe } = await setupTests()
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [AddressOne, 1], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not set 0 Address', async () => {
             const { safe } = await setupTests()
             await expect(
                 executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [AddressZero, 1], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not add owner twice', async () => {
@@ -53,21 +53,21 @@ describe("OwnerManager", async () => {
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not add owner and change threshold to 0', async () => {
             const { safe } = await setupTests()
             await expect(
                 executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 0], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not add owner and change threshold to larger number than new owner count', async () => {
             const { safe } = await setupTests()
             await expect(
                 executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 3], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('emits event for new owner', async () => {
@@ -107,7 +107,7 @@ describe("OwnerManager", async () => {
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, AddressOne, 1], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not remove 0 Address', async () => {
@@ -116,54 +116,54 @@ describe("OwnerManager", async () => {
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, AddressZero, 1], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('Invalid prevOwner, owner pair provided - Invalid target', async () => {
             const { safe } = await setupTests()
             await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
             await expect(
                 executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, user1.address, 1], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('Invalid prevOwner, owner pair provided - Invalid sentinel', async () => {
             const { safe } = await setupTests()
             await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
             await expect(
                 executeContractCallWithSigners(safe, safe, "removeOwner", [AddressZero, user2.address, 1], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('Invalid prevOwner, owner pair provided - Invalid source', async () => {
             const { safe } = await setupTests()
             await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
             await expect(
                 executeContractCallWithSigners(safe, safe, "removeOwner", [user1.address, user2.address, 1], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not remove owner and change threshold to larger number than new owner count', async () => {
             const { safe } = await setupTests()
             await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
             await expect(
                 executeContractCallWithSigners(safe, safe, "removeOwner", [user2.address, user1.address, 2], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not remove owner and change threshold to 0', async () => {
             const { safe } = await setupTests()
             await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
             await expect(
                 executeContractCallWithSigners(safe, safe, "removeOwner", [user2.address, user1.address, 0], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not remove owner only owner', async () => {
             const { safe } = await setupTests()
             await expect(
                 executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, user1.address, 1], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('emits event for removed owner and threshold if changed', async () => {
@@ -200,7 +200,7 @@ describe("OwnerManager", async () => {
             await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
             await expect(
                 executeContractCallWithSigners(safe, safe, "removeOwner", [user2.address, user1.address, 2], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
     })
 
@@ -210,73 +210,73 @@ describe("OwnerManager", async () => {
             await expect(safe.swapOwner(AddressOne, user1.address, user2.address)).to.be.revertedWith("GS031")
         })
 
-        it('can not swap in Safe itseld', async () => {
+        it('can not swap in Safe itself', async () => {
             const { safe } = await setupTests()
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, safe.address], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not swap in sentinel', async () => {
             const { safe } = await setupTests()
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, AddressOne], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not swap in 0 Address', async () => {
             const { safe } = await setupTests()
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, AddressZero], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not swap in existing owner', async () => {
             const { safe } = await setupTests()
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, user1.address], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not swap out sentinel', async () => {
             const { safe } = await setupTests()
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "swapOwner", [user1.address, AddressOne, user2.address], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('can not swap out 0 address', async () => {
             const { safe } = await setupTests()
 
             await expect(
                 executeContractCallWithSigners(safe, safe, "swapOwner", [user3.address, AddressZero, user2.address], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('Invalid prevOwner, owner pair provided - Invalid target', async () => {
             const { safe } = await setupTests()
             await expect(
                 executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user3.address, user2.address], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('Invalid prevOwner, owner pair provided - Invalid sentinel', async () => {
             const { safe } = await setupTests()
             await expect(
                 executeContractCallWithSigners(safe, safe, "swapOwner", [AddressZero, user1.address, user2.address], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('Invalid prevOwner, owner pair provided - Invalid source', async () => {
             const { safe } = await setupTests()
             await expect(
                 executeContractCallWithSigners(safe, safe, "swapOwner", [user2.address, user1.address, user2.address], [user1])
-            ).to.emit(safe, "ExecutionFailure")
+            ).to.revertedWith("GS013")
         })
 
         it('emits event for replacing owner', async () => {