Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Error propagation for internal TX #839

Merged
merged 2 commits into from
Sep 30, 2024
Merged

Error propagation for internal TX #839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db8995a
Error propagation for internal tx
remedcu Sep 30, 2024
4d77e13
memory-safe assembly
remedcu Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion contracts/Safe.sol
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading