Skip to content

Commit

Permalink
Error propagation for internal TX (safe-global#839)
Browse files Browse the repository at this point in the history
Fixes safe-global#715

This PR implements error propagation for internal TX so the user/dev can
know the reason for revert instead of generic `GS013`.
  • Loading branch information
remedcu authored Sep 30, 2024
1 parent 3e2ee89 commit 0142ec8
Show file tree
Hide file tree
Showing 11 changed files with 58 additions and 54 deletions.
11 changes: 10 additions & 1 deletion contracts/Safe.sol
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,16 @@ contract Safe is
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
if (!success && safeTxGas == 0 && gasPrice == 0) revertWithError("GS013");
if (!success && safeTxGas == 0 && gasPrice == 0) {
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
let p := mload(0x40)
returndatacopy(p, 0, returndatasize())
revert(p, returndatasize())
}
/* solhint-enable no-inline-assembly */
}
// 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) {
Expand Down
4 changes: 2 additions & 2 deletions test/core/Safe.Execution.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ describe("Safe", () => {
it("should revert for failed call execution if gasPrice == 0 and safeTxGas == 0", async () => {
const { safe, reverter, signers } = await setupTests();
const [user1] = signers;
await expect(executeContractCallWithSigners(safe, reverter, "revert", [], [user1])).to.revertedWith("GS013");
await expect(executeContractCallWithSigners(safe, reverter, "revert", [], [user1])).to.revertedWith("Shit happens");
});

it("should emit event for successful delegatecall execution", async () => {
Expand Down Expand Up @@ -184,7 +184,7 @@ describe("Safe", () => {
it("should emit event for failed delegatecall execution if gasPrice == 0 and safeTxGas == 0", async () => {
const { safe, reverter, signers } = await setupTests();
const [user1] = signers;
await expect(executeContractCallWithSigners(safe, reverter, "revert", [], [user1], true)).to.revertedWith("GS013");
await expect(executeContractCallWithSigners(safe, reverter, "revert", [], [user1], true)).to.revertedWith("Shit happens");
});

it("should revert on unknown operation", async () => {
Expand Down
3 changes: 1 addition & 2 deletions test/core/Safe.FallbackManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,9 @@ describe("FallbackManager", () => {
// 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
await expect(
executeContractCallWithSigners(safe, safe, "setFallbackHandler", [await safe.getAddress()], [user1]),
).to.be.revertedWith("GS013");
).to.be.revertedWith("GS400");
});
});
});
2 changes: 1 addition & 1 deletion test/core/Safe.GuardManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe("GuardManager", () => {
} = await setupWithTemplate();
const safe = await getSafe({ owners: [user1.address] });

await expect(executeContractCallWithSigners(safe, safe, "setGuard", [user2.address], [user1])).to.be.revertedWith("GS013");
await expect(executeContractCallWithSigners(safe, safe, "setGuard", [user2.address], [user1])).to.be.reverted;
});

it("emits an event when the guard is changed", async () => {
Expand Down
22 changes: 11 additions & 11 deletions test/core/Safe.ModuleManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,15 @@ describe("ModuleManager", () => {
signers: [user1],
} = await setupTests();

await expect(executeContractCallWithSigners(safe, safe, "enableModule", [AddressOne], [user1])).to.revertedWith("GS013");
await expect(executeContractCallWithSigners(safe, safe, "enableModule", [AddressOne], [user1])).to.revertedWith("GS101");
});

it("can not set 0 Address", async () => {
const {
safe,
signers: [user1],
} = await setupTests();
await expect(executeContractCallWithSigners(safe, safe, "enableModule", [AddressZero], [user1])).to.revertedWith("GS013");
await expect(executeContractCallWithSigners(safe, safe, "enableModule", [AddressZero], [user1])).to.revertedWith("GS101");
});

it("can not add module twice", async () => {
Expand All @@ -62,7 +62,7 @@ describe("ModuleManager", () => {
// Use module for execution to see error
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);

await expect(executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).to.revertedWith("GS013");
await expect(executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])).to.revertedWith("GS102");
});

it("emits event for a new module", async () => {
Expand Down Expand Up @@ -116,7 +116,7 @@ describe("ModuleManager", () => {
} = await setupTests();

await expect(executeContractCallWithSigners(safe, safe, "disableModule", [AddressOne, AddressOne], [user1])).to.revertedWith(
"GS013",
"GS101",
);
});

Expand All @@ -126,7 +126,7 @@ describe("ModuleManager", () => {
signers: [user1],
} = await setupTests();
await expect(executeContractCallWithSigners(safe, safe, "disableModule", [AddressOne, AddressZero], [user1])).to.revertedWith(
"GS013",
"GS101",
);
});

Expand All @@ -137,7 +137,7 @@ describe("ModuleManager", () => {
} = await setupTests();
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);
await expect(executeContractCallWithSigners(safe, safe, "disableModule", [AddressOne, user1.address], [user1])).to.revertedWith(
"GS013",
"GS103",
);
});

Expand All @@ -149,7 +149,7 @@ describe("ModuleManager", () => {
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);
await expect(
executeContractCallWithSigners(safe, safe, "disableModule", [AddressZero, user2.address], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS103");
});

it("Invalid prevModule, module pair provided - Invalid source", async () => {
Expand All @@ -161,7 +161,7 @@ describe("ModuleManager", () => {
await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]);
await expect(
executeContractCallWithSigners(safe, safe, "disableModule", [user1.address, user2.address], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS103");
});

it("emits event for disabled module", async () => {
Expand Down Expand Up @@ -579,9 +579,9 @@ describe("ModuleManager", () => {
} = await setupTests();
const safe = await getSafe({ owners: [user1.address] });

await expect(executeContractCallWithSigners(safe, safe, "setModuleGuard", [user2.address], [user1])).to.be.revertedWith(
"GS013",
);
await expect(
executeContractCallWithSigners(safe, safe, "setModuleGuard", [user2.address], [user1]),
).to.be.revertedWithoutReason();
});

it("emits an event when the module guard is changed", async () => {
Expand Down
48 changes: 24 additions & 24 deletions test/core/Safe.OwnerManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe("OwnerManager", () => {
const safeAddress = await safe.getAddress();

await expect(executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [safeAddress, 1], [user1])).to.revertedWith(
"GS013",
"GS203",
);
});

Expand All @@ -44,7 +44,7 @@ describe("OwnerManager", () => {
} = await setupTests();

await expect(executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [AddressOne, 1], [user1])).to.revertedWith(
"GS013",
"GS203",
);
});

Expand All @@ -54,7 +54,7 @@ describe("OwnerManager", () => {
signers: [user1],
} = await setupTests();
await expect(executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [AddressZero, 1], [user1])).to.revertedWith(
"GS013",
"GS203",
);
});

Expand All @@ -66,7 +66,7 @@ describe("OwnerManager", () => {
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1]);

await expect(executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1])).to.revertedWith(
"GS013",
"GS204",
);
});

Expand All @@ -76,7 +76,7 @@ describe("OwnerManager", () => {
signers: [user1, user2],
} = await setupTests();
await expect(executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 0], [user1])).to.revertedWith(
"GS013",
"GS202",
);
});

Expand All @@ -86,7 +86,7 @@ describe("OwnerManager", () => {
signers: [user1, user2],
} = await setupTests();
await expect(executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 3], [user1])).to.revertedWith(
"GS013",
"GS201",
);
});

Expand Down Expand Up @@ -139,7 +139,7 @@ describe("OwnerManager", () => {
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1]);

await expect(executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, AddressOne, 1], [user1])).to.revertedWith(
"GS013",
"GS203",
);
});

Expand All @@ -151,7 +151,7 @@ describe("OwnerManager", () => {
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1]);

await expect(executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, AddressZero, 1], [user1])).to.revertedWith(
"GS013",
"GS203",
);
});

Expand All @@ -163,7 +163,7 @@ describe("OwnerManager", () => {
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1]);
await expect(
executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, user1.address, 1], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS205");
});

it("Invalid prevOwner, owner pair provided - Invalid sentinel", async () => {
Expand All @@ -174,7 +174,7 @@ describe("OwnerManager", () => {
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1]);
await expect(
executeContractCallWithSigners(safe, safe, "removeOwner", [AddressZero, user2.address, 1], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS205");
});

it("Invalid prevOwner, owner pair provided - Invalid source", async () => {
Expand All @@ -185,7 +185,7 @@ describe("OwnerManager", () => {
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1]);
await expect(
executeContractCallWithSigners(safe, safe, "removeOwner", [user1.address, user2.address, 1], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS205");
});

it("can not remove owner and change threshold to larger number than new owner count", async () => {
Expand All @@ -196,7 +196,7 @@ describe("OwnerManager", () => {
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1]);
await expect(
executeContractCallWithSigners(safe, safe, "removeOwner", [user2.address, user1.address, 2], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS201");
});

it("can not remove owner and change threshold to 0", async () => {
Expand All @@ -207,7 +207,7 @@ describe("OwnerManager", () => {
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1]);
await expect(
executeContractCallWithSigners(safe, safe, "removeOwner", [user2.address, user1.address, 0], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS202");
});

it("can not remove owner only owner", async () => {
Expand All @@ -217,7 +217,7 @@ describe("OwnerManager", () => {
} = await setupTests();
await expect(
executeContractCallWithSigners(safe, safe, "removeOwner", [AddressOne, user1.address, 1], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS201");
});

it("emits event for removed owner and threshold if changed", async () => {
Expand Down Expand Up @@ -263,7 +263,7 @@ describe("OwnerManager", () => {
await executeContractCallWithSigners(safe, safe, "addOwnerWithThreshold", [user2.address, 1], [user1]);
await expect(
executeContractCallWithSigners(safe, safe, "removeOwner", [user2.address, user1.address, 2], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS201");
});
});

Expand All @@ -285,7 +285,7 @@ describe("OwnerManager", () => {

await expect(
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, safeAddress], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS203");
});

it("can not swap in sentinel", async () => {
Expand All @@ -296,7 +296,7 @@ describe("OwnerManager", () => {

await expect(
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, AddressOne], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS203");
});

it("can not swap in 0 Address", async () => {
Expand All @@ -307,7 +307,7 @@ describe("OwnerManager", () => {

await expect(
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, AddressZero], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS203");
});

it("can not swap in existing owner", async () => {
Expand All @@ -318,7 +318,7 @@ describe("OwnerManager", () => {

await expect(
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user1.address, user1.address], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS204");
});

it("can not swap out sentinel", async () => {
Expand All @@ -329,7 +329,7 @@ describe("OwnerManager", () => {

await expect(
executeContractCallWithSigners(safe, safe, "swapOwner", [user1.address, AddressOne, user2.address], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS203");
});

it("can not swap out 0 address", async () => {
Expand All @@ -340,7 +340,7 @@ describe("OwnerManager", () => {

await expect(
executeContractCallWithSigners(safe, safe, "swapOwner", [user3.address, AddressZero, user2.address], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS203");
});

it("Invalid prevOwner, owner pair provided - Invalid target", async () => {
Expand All @@ -350,7 +350,7 @@ describe("OwnerManager", () => {
} = await setupTests();
await expect(
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressOne, user3.address, user2.address], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS205");
});

it("Invalid prevOwner, owner pair provided - Invalid sentinel", async () => {
Expand All @@ -360,7 +360,7 @@ describe("OwnerManager", () => {
} = await setupTests();
await expect(
executeContractCallWithSigners(safe, safe, "swapOwner", [AddressZero, user1.address, user2.address], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS205");
});

it("Invalid prevOwner, owner pair provided - Invalid source", async () => {
Expand All @@ -370,7 +370,7 @@ describe("OwnerManager", () => {
} = await setupTests();
await expect(
executeContractCallWithSigners(safe, safe, "swapOwner", [user2.address, user1.address, user2.address], [user1]),
).to.revertedWith("GS013");
).to.revertedWith("GS205");
});

it("emits event for replacing owner", async () => {
Expand Down
2 changes: 1 addition & 1 deletion test/guards/ReentrancyTransactionGuard.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ describe("ReentrancyTransactionGuard", () => {
],
[user1],
),
).to.be.revertedWith("GS013");
).to.be.revertedWith("Reentrancy detected");

expect(await mock.invocationCount()).to.be.eq(0);
});
Expand Down
4 changes: 2 additions & 2 deletions test/libraries/CreateCall.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ describe("CreateCall", () => {
} = await setupTests();

const tx = await buildContractCall(createCall, "performCreate", [1, testContract.data], await safe.nonce(), true);
await expect(executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)])).to.revertedWith("GS013");
await expect(executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)])).to.revertedWith("Could not deploy contract");
});

it("should successfully create contract and emit event", async () => {
Expand Down Expand Up @@ -165,7 +165,7 @@ describe("CreateCall", () => {
} = await setupTests();

const tx = await buildContractCall(createCall, "performCreate2", [1, testContract.data, salt], await safe.nonce(), true);
await expect(executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)])).to.revertedWith("GS013");
await expect(executeTx(safe, tx, [await safeApproveHash(user1, safe, tx, true)])).to.revertedWith("Could not deploy contract");
});

it("should successfully create contract and emit event", async () => {
Expand Down
6 changes: 3 additions & 3 deletions test/libraries/MultiSend.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ describe("MultiSend", () => {

const txs = [buildSafeTransaction({ to: user2.address, operation: 2, nonce: 0 })];
const safeTx = await buildMultiSendSafeTx(multiSend, txs, await safe.nonce());
await expect(executeTx(safe.connect(user1), safeTx, [await safeApproveHash(user1, safe, safeTx, true)])).to.revertedWith(
"GS013",
);
await expect(
executeTx(safe.connect(user1), safeTx, [await safeApproveHash(user1, safe, safeTx, true)]),
).to.revertedWithoutReason();
});

it("Can execute empty multisend", async () => {
Expand Down
Loading

0 comments on commit 0142ec8

Please sign in to comment.