Skip to content

Commit ac82f3a

Browse files
rmeissnerrmeissner
authored
Closes #274: Revert on failure with safeTxGas 0 (#283)
1 parent eff756d commit ac82f3a

File tree

9 files changed

+130
-94
lines changed

9 files changed

+130
-94
lines changed

‎contracts/GnosisSafe.sol‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,9 @@ contract GnosisSafe
170170
// We only substract 2500 (compared to the 3000 before) to ensure that the amount passed is still higher than safeTxGas
171171
success = execute(to, value, data, operation, gasPrice == 0 ? (gasleft() - 2500) : safeTxGas);
172172
gasUsed = gasUsed.sub(gasleft());
173+
// If no safeTxGas and no gasPrice was set (e.g. both are 0), then the internal tx is required to be successful
174+
// This makes it possible to use `estimateGas` without issues, as it searches for the minimum gas where the tx doesn't revert
175+
require(success || safeTxGas != 0 || gasPrice != 0, "GS013");
173176
// We transfer the calculated tx costs to the tx.origin to avoid sending it to intermediate contracts that have made calls
174177
uint256 payment = 0;
175178
if (gasPrice > 0) {

‎docs/error_codes.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,11 @@
44
- `GS000`: `Could not finish initialization`
55
- `GS001`: `Threshold needs to be defined`
66

7-
### General gas related
8-
- `GS010`: `Not enough gas to execute safe transaction`
7+
### General gas/ execution related
8+
- `GS010`: `Not enough gas to execute Safe transaction`
99
- `GS011`: `Could not pay gas costs with ether`
1010
- `GS012`: `Could not pay gas costs with token`
11+
- `GS013`: `Safe transaction failed when gasPrice and safeTxGas were 0`
1112

1213
### General signature validation related
1314
- `GS020`: `Signatures data too short`

‎test/core/GnosisSafe.Execution.spec.ts‎

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,13 +67,29 @@ describe("GnosisSafe", async () => {
6767
).to.be.eq("0x" + "baddad".padEnd(64, "0"))
6868
})
6969

70-
it('should emit event for failed call execution', async () => {
70+
it('should emit event for failed call execution if safeTxGas > 0', async () => {
7171
const { safe, reverter } = await setupTests()
7272
await expect(
73-
executeContractCallWithSigners(safe, reverter, "revert", [], [user1])
73+
executeContractCallWithSigners(safe, reverter, "revert", [], [user1], false, { safeTxGas: 1 })
74+
).to.emit(safe, "ExecutionFailure")
75+
})
76+
77+
it('should emit event for failed call execution if gasPrice > 0', async () => {
78+
const { safe, reverter } = await setupTests()
79+
// Fund refund
80+
await user1.sendTransaction({ to: safe.address, value: 10000000 })
81+
await expect(
82+
executeContractCallWithSigners(safe, reverter, "revert", [], [user1], false, { gasPrice: 1 })
7483
).to.emit(safe, "ExecutionFailure")
7584
})
7685

86+
it('should revert for failed call execution if gasPrice == 0 and safeTxGas == 0', async () => {
87+
const { safe, reverter } = await setupTests()
88+
await expect(
89+
executeContractCallWithSigners(safe, reverter, "revert", [], [user1])
90+
).to.revertedWith("GS013")
91+
})
92+
7793
it('should emit event for successful delegatecall execution', async () => {
7894
const { safe, storageSetter } = await setupTests()
7995
await expect(
@@ -89,14 +105,29 @@ describe("GnosisSafe", async () => {
89105
).to.be.eq("0x" + "".padEnd(64, "0"))
90106
})
91107

92-
it('should emit event for failed delegatecall execution', async () => {
108+
it('should emit event for failed delegatecall execution if safeTxGas > 0', async () => {
93109
const { safe, reverter } = await setupTests()
94-
const txHash = calculateSafeTransactionHash(safe, buildContractCall(reverter, "revert", [], await safe.nonce(), true), await chainId())
110+
const txHash = calculateSafeTransactionHash(safe, buildContractCall(reverter, "revert", [], await safe.nonce(), true, { safeTxGas: 1 }), await chainId())
95111
await expect(
96-
executeContractCallWithSigners(safe, reverter, "revert", [], [user1], true)
112+
executeContractCallWithSigners(safe, reverter, "revert", [], [user1], true, { safeTxGas: 1 })
97113
).to.emit(safe, "ExecutionFailure").withArgs(txHash, 0)
98114
})
99115

116+
it('should emit event for failed delegatecall execution if gasPrice > 0', async () => {
117+
const { safe, reverter } = await setupTests()
118+
await user1.sendTransaction({ to: safe.address, value: 10000000 })
119+
await expect(
120+
executeContractCallWithSigners(safe, reverter, "revert", [], [user1], true, { gasPrice: 1 })
121+
).to.emit(safe, "ExecutionFailure")
122+
})
123+
124+
it('should emit event for failed delegatecall execution if gasPrice == 0 and safeTxGas == 0', async () => {
125+
const { safe, reverter } = await setupTests()
126+
await expect(
127+
executeContractCallWithSigners(safe, reverter, "revert", [], [user1], true)
128+
).to.revertedWith("GS013")
129+
})
130+
100131
it('should revert on unknown operation', async () => {
101132
const { safe } = await setupTests()
102133
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce(), operation: 2 })

‎test/core/GnosisSafe.ModuleManager.spec.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,14 @@ describe("ModuleManager", async () => {
3030

3131
await expect(
3232
executeContractCallWithSigners(safe, safe, "enableModule", [AddressOne], [user1])
33-
).to.emit(safe, "ExecutionFailure")
33+
).to.revertedWith("GS013")
3434
})
3535

3636
it('can not set 0 Address', async () => {
3737
const { safe } = await setupTests()
3838
await expect(
3939
executeContractCallWithSigners(safe, safe, "enableModule", [AddressZero], [user1])
40-
).to.emit(safe, "ExecutionFailure")
40+
).to.revertedWith("GS013")
4141
})
4242

4343
it('can not add module twice', async () => {
@@ -47,7 +47,7 @@ describe("ModuleManager", async () => {
4747

4848
await expect(
4949
executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
50-
).to.emit(safe, "ExecutionFailure")
50+
).to.revertedWith("GS013")
5151
})
5252

5353
it('emits event for new module', async () => {
@@ -94,30 +94,30 @@ describe("ModuleManager", async () => {
9494

9595
await expect(
9696
executeContractCallWithSigners(safe, safe, "disableModule", [AddressOne, AddressOne], [user1])
97-
).to.emit(safe, "ExecutionFailure")
97+
).to.revertedWith("GS013")
9898
})
9999

100100
it('can not set 0 Address', async () => {
101101
const { safe } = await setupTests()
102102
await expect(
103103
executeContractCallWithSigners(safe, safe, "disableModule", [AddressOne, AddressZero], [user1])
104-
).to.emit(safe, "ExecutionFailure")
104+
).to.revertedWith("GS013")
105105
})
106106

107107
it('Invalid prevModule, module pair provided - Invalid target', async () => {
108108
const { safe } = await setupTests()
109109
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
110110
await expect(
111111
executeContractCallWithSigners(safe, safe, "disableModule", [AddressOne, user1.address], [user1])
112-
).to.emit(safe, "ExecutionFailure")
112+
).to.revertedWith("GS013")
113113
})
114114

115115
it('Invalid prevModule, module pair provided - Invalid sentinel', async () => {
116116
const { safe } = await setupTests()
117117
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
118118
await expect(
119119
executeContractCallWithSigners(safe, safe, "disableModule", [AddressZero, user2.address], [user1])
120-
).to.emit(safe, "ExecutionFailure")
120+
).to.revertedWith("GS013")
121121
})
122122

123123
it('Invalid prevModule, module pair provided - Invalid source', async () => {
@@ -126,7 +126,7 @@ describe("ModuleManager", async () => {
126126
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
127127
await expect(
128128
executeContractCallWithSigners(safe, safe, "disableModule", [user1.address, user2.address], [user1])
129-
).to.emit(safe, "ExecutionFailure")
129+
).to.revertedWith("GS013")
130130
})
131131

132132
it('emits event for disabled module', async () => {

‎test/core/GnosisSafe.OwnerManager.spec.ts‎

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -29,22 +29,22 @@ describe("OwnerManager", async () => {
2929

3030
await expect(
3131
executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [safe.address, 1], [user1])
32-
).to.emit(safe, "ExecutionFailure")
32+
).to.revertedWith("GS013")
3333
})
3434

3535
it('can not set sentinel', async () => {
3636
const { safe } = await setupTests()
3737

3838
await expect(
3939
executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [AddressOne, 1], [user1])
40-
).to.emit(safe, "ExecutionFailure")
40+
).to.revertedWith("GS013")
4141
})
4242

4343
it('can not set 0 Address', async () => {
4444
const { safe } = await setupTests()
4545
await expect(
4646
executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [AddressZero, 1], [user1])
47-
).to.emit(safe, "ExecutionFailure")
47+
).to.revertedWith("GS013")
4848
})
4949

5050
it('can not add owner twice', async () => {
@@ -53,21 +53,21 @@ describe("OwnerManager", async () => {
5353

5454
await expect(
5555
executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
56-
).to.emit(safe, "ExecutionFailure")
56+
).to.revertedWith("GS013")
5757
})
5858

5959
it('can not add owner and change threshold to 0', async () => {
6060
const { safe } = await setupTests()
6161
await expect(
6262
executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 0], [user1])
63-
).to.emit(safe, "ExecutionFailure")
63+
).to.revertedWith("GS013")
6464
})
6565

6666
it('can not add owner and change threshold to larger number than new owner count', async () => {
6767
const { safe } = await setupTests()
6868
await expect(
6969
executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 3], [user1])
70-
).to.emit(safe, "ExecutionFailure")
70+
).to.revertedWith("GS013")
7171
})
7272

7373
it('emits event for new owner', async () => {
@@ -107,7 +107,7 @@ describe("OwnerManager", async () => {
107107

108108
await expect(
109109
executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, AddressOne, 1], [user1])
110-
).to.emit(safe, "ExecutionFailure")
110+
).to.revertedWith("GS013")
111111
})
112112

113113
it('can not remove 0 Address', async () => {
@@ -116,54 +116,54 @@ describe("OwnerManager", async () => {
116116

117117
await expect(
118118
executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, AddressZero, 1], [user1])
119-
).to.emit(safe, "ExecutionFailure")
119+
).to.revertedWith("GS013")
120120
})
121121

122122
it('Invalid prevOwner, owner pair provided - Invalid target', async () => {
123123
const { safe } = await setupTests()
124124
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
125125
await expect(
126126
executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, user1.address, 1], [user1])
127-
).to.emit(safe, "ExecutionFailure")
127+
).to.revertedWith("GS013")
128128
})
129129

130130
it('Invalid prevOwner, owner pair provided - Invalid sentinel', async () => {
131131
const { safe } = await setupTests()
132132
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
133133
await expect(
134134
executeContractCallWithSigners(safe, safe, "removeOwner", [AddressZero, user2.address, 1], [user1])
135-
).to.emit(safe, "ExecutionFailure")
135+
).to.revertedWith("GS013")
136136
})
137137

138138
it('Invalid prevOwner, owner pair provided - Invalid source', async () => {
139139
const { safe } = await setupTests()
140140
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
141141
await expect(
142142
executeContractCallWithSigners(safe, safe, "removeOwner", [user1.address, user2.address, 1], [user1])
143-
).to.emit(safe, "ExecutionFailure")
143+
).to.revertedWith("GS013")
144144
})
145145

146146
it('can not remove owner and change threshold to larger number than new owner count', async () => {
147147
const { safe } = await setupTests()
148148
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
149149
await expect(
150150
executeContractCallWithSigners(safe, safe, "removeOwner", [user2.address, user1.address, 2], [user1])
151-
).to.emit(safe, "ExecutionFailure")
151+
).to.revertedWith("GS013")
152152
})
153153

154154
it('can not remove owner and change threshold to 0', async () => {
155155
const { safe } = await setupTests()
156156
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
157157
await expect(
158158
executeContractCallWithSigners(safe, safe, "removeOwner", [user2.address, user1.address, 0], [user1])
159-
).to.emit(safe, "ExecutionFailure")
159+
).to.revertedWith("GS013")
160160
})
161161

162162
it('can not remove owner only owner', async () => {
163163
const { safe } = await setupTests()
164164
await expect(
165165
executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, user1.address, 1], [user1])
166-
).to.emit(safe, "ExecutionFailure")
166+
).to.revertedWith("GS013")
167167
})
168168

169169
it('emits event for removed owner and threshold if changed', async () => {
@@ -200,7 +200,7 @@ describe("OwnerManager", async () => {
200200
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])
201201
await expect(
202202
executeContractCallWithSigners(safe, safe, "removeOwner", [user2.address, user1.address, 2], [user1])
203-
).to.emit(safe, "ExecutionFailure")
203+
).to.revertedWith("GS013")
204204
})
205205
})
206206

@@ -210,73 +210,73 @@ describe("OwnerManager", async () => {
210210
await expect(safe.swapOwner(AddressOne, user1.address, user2.address)).to.be.revertedWith("GS031")
211211
})
212212

213-
it('can not swap in Safe itseld', async () => {
213+
it('can not swap in Safe itself', async () => {
214214
const { safe } = await setupTests()
215215

216216
await expect(
217217
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, safe.address], [user1])
218-
).to.emit(safe, "ExecutionFailure")
218+
).to.revertedWith("GS013")
219219
})
220220

221221
it('can not swap in sentinel', async () => {
222222
const { safe } = await setupTests()
223223

224224
await expect(
225225
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, AddressOne], [user1])
226-
).to.emit(safe, "ExecutionFailure")
226+
).to.revertedWith("GS013")
227227
})
228228

229229
it('can not swap in 0 Address', async () => {
230230
const { safe } = await setupTests()
231231

232232
await expect(
233233
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, AddressZero], [user1])
234-
).to.emit(safe, "ExecutionFailure")
234+
).to.revertedWith("GS013")
235235
})
236236

237237
it('can not swap in existing owner', async () => {
238238
const { safe } = await setupTests()
239239

240240
await expect(
241241
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, user1.address], [user1])
242-
).to.emit(safe, "ExecutionFailure")
242+
).to.revertedWith("GS013")
243243
})
244244

245245
it('can not swap out sentinel', async () => {
246246
const { safe } = await setupTests()
247247

248248
await expect(
249249
executeContractCallWithSigners(safe, safe, "swapOwner", [user1.address, AddressOne, user2.address], [user1])
250-
).to.emit(safe, "ExecutionFailure")
250+
).to.revertedWith("GS013")
251251
})
252252

253253
it('can not swap out 0 address', async () => {
254254
const { safe } = await setupTests()
255255

256256
await expect(
257257
executeContractCallWithSigners(safe, safe, "swapOwner", [user3.address, AddressZero, user2.address], [user1])
258-
).to.emit(safe, "ExecutionFailure")
258+
).to.revertedWith("GS013")
259259
})
260260

261261
it('Invalid prevOwner, owner pair provided - Invalid target', async () => {
262262
const { safe } = await setupTests()
263263
await expect(
264264
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user3.address, user2.address], [user1])
265-
).to.emit(safe, "ExecutionFailure")
265+
).to.revertedWith("GS013")
266266
})
267267

268268
it('Invalid prevOwner, owner pair provided - Invalid sentinel', async () => {
269269
const { safe } = await setupTests()
270270
await expect(
271271
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressZero, user1.address, user2.address], [user1])
272-
).to.emit(safe, "ExecutionFailure")
272+
).to.revertedWith("GS013")
273273
})
274274

275275
it('Invalid prevOwner, owner pair provided - Invalid source', async () => {
276276
const { safe } = await setupTests()
277277
await expect(
278278
executeContractCallWithSigners(safe, safe, "swapOwner", [user2.address, user1.address, user2.address], [user1])
279-
).to.emit(safe, "ExecutionFailure")
279+
).to.revertedWith("GS013")
280280
})
281281

282282
it('emits event for replacing owner', async () => {

0 commit comments

Comments
 (0)