Skip to content

Commit 8e9c840

Browse files
rmeissnerrmeissner
authored
Closes #224: transaction guard (#276)
1 parent a3a5e06 commit 8e9c840

File tree

10 files changed

+296
-18
lines changed

10 files changed

+296
-18
lines changed

‎benchmark/GnosisSafe.ERC1155.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,11 @@ const [, , , , user5] = waffle.provider.getWallets();
99

1010
benchmark("ERC1155", [{
1111
name: "transfer",
12-
prepare: async (contracts: Contracts, target: string) => {
12+
prepare: async (contracts: Contracts, target: string, nonce: number) => {
1313
const token = contracts.additions.token
1414
await token.mint(target, 23, 1337, "0x")
1515
const data = token.interface.encodeFunctionData("safeTransferFrom", [target, user5.address, 23, 500, "0x"])
16-
return buildSafeTransaction({ to: token.address, data, safeTxGas: 1000000, nonce: 0 })
16+
return buildSafeTransaction({ to: token.address, data, safeTxGas: 1000000, nonce })
1717
},
1818
after: async (contracts: Contracts) => {
1919
expect(

‎benchmark/GnosisSafe.ERC20.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,11 @@ const [, , , , user5] = waffle.provider.getWallets();
99

1010
benchmark("ERC20", [{
1111
name: "transfer",
12-
prepare: async (contracts: Contracts, target: string) => {
12+
prepare: async (contracts: Contracts, target: string, nonce: number) => {
1313
const token = contracts.additions.token
1414
await token.transfer(target, 1000)
1515
const data = token.interface.encodeFunctionData("transfer", [user5.address, 500])
16-
return buildSafeTransaction({ to: token.address, data, safeTxGas: 1000000, nonce: 0 })
16+
return buildSafeTransaction({ to: token.address, data, safeTxGas: 1000000, nonce })
1717
},
1818
after: async (contracts: Contracts) => {
1919
expect(

‎benchmark/GnosisSafe.Ether.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,11 @@ const [user1] = waffle.provider.getWallets();
1010

1111
benchmark("Ether", [{
1212
name: "transfer",
13-
prepare: async (_, target: string) => {
13+
prepare: async (_, target: string, nonce: number) => {
1414
// Create account, as we don't want to test this in the benchmark
1515
await user1.sendTransaction({ to: testTarget, value: 1 })
1616
await user1.sendTransaction({ to: target, value: 1000 })
17-
return buildSafeTransaction({ to: testTarget, value: 500, safeTxGas: 1000000, nonce: 0 })
17+
return buildSafeTransaction({ to: testTarget, value: 500, safeTxGas: 1000000, nonce })
1818
},
1919
after: async () => {
2020
expect(

‎benchmark/GnosisSafe.Proxy.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,10 @@ const testTarget = "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE"
1010

1111
benchmark("Proxy", [{
1212
name: "creation",
13-
prepare: async (contracts) => {
13+
prepare: async (contracts,_,nonce) => {
1414
const factory = contracts.additions.factory
1515
const data = factory.interface.encodeFunctionData("createProxy", [testTarget, "0x"])
16-
return buildSafeTransaction({ to: factory.address, data, safeTxGas: 1000000, nonce: 0 })
16+
return buildSafeTransaction({ to: factory.address, data, safeTxGas: 1000000, nonce })
1717
},
1818
fixture: async () => {
1919
return {

‎benchmark/utils/setup.ts‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
import { expect } from "chai";
2-
import { deployments, waffle } from "hardhat";
2+
import hre, { deployments, waffle } from "hardhat";
33
import "@nomiclabs/hardhat-ethers";
44
import { getDefaultCallbackHandler, getSafeWithOwners } from "../../test/utils/setup";
5-
import { logGas, executeTx, SafeTransaction, safeSignTypedData, SafeSignature } from "../../test/utils/execution";
5+
import { logGas, executeTx, SafeTransaction, safeSignTypedData, SafeSignature, executeContractCallWithSigners } from "../../test/utils/execution";
66
import { Wallet, Contract } from "ethers";
7+
import { AddressZero } from "@ethersproject/constants";
78

89
const [user1, user2, user3, user4, user5] = waffle.provider.getWallets();
910

@@ -12,24 +13,29 @@ export interface Contracts {
1213
additions: any | undefined
1314
}
1415

15-
const generateTarget = async (owners: Wallet[], threshold: number) => {
16+
const generateTarget = async (owners: Wallet[], threshold: number, guardAddress: string) => {
1617
const fallbackHandler = await getDefaultCallbackHandler()
17-
return await getSafeWithOwners(owners.map((owner) => owner.address), threshold, fallbackHandler.address)
18+
const safe = await getSafeWithOwners(owners.map((owner) => owner.address), threshold, fallbackHandler.address)
19+
await executeContractCallWithSigners(safe, safe, "setGuard", [guardAddress], owners)
20+
return safe
1821
}
1922

2023
export const configs = [
2124
{ name: "single owner", signers: [user1], threshold: 1 },
22-
{ name: "2 out of 2", signers: [user1, user2], threshold: 2 },
25+
{ name: "single owner and guard", signers: [user1], threshold: 1, useGuard: true },
26+
{ name: "2 out of 23", signers: [user1, user2], threshold: 2 },
2327
{ name: "3 out of 3", signers: [user1, user2, user3], threshold: 3 },
2428
{ name: "3 out of 5", signers: [user1, user2, user3, user4, user5], threshold: 3 },
2529
]
2630

2731
const setupBenchmarkContracts = async (benchmarkFixture?: () => Promise<any>) => {
2832
return await deployments.createFixture(async ({ deployments }) => {
2933
await deployments.fixture();
34+
const guardFactory = await hre.ethers.getContractFactory("DelegateCallTransactionGuard");
35+
const guard = await guardFactory.deploy(AddressZero)
3036
const targets = []
3137
for (const config of configs) {
32-
targets.push(await generateTarget(config.signers, config.threshold))
38+
targets.push(await generateTarget(config.signers, config.threshold, config.useGuard ? guard.address : AddressZero))
3339
}
3440
return {
3541
targets,
@@ -40,7 +46,7 @@ const setupBenchmarkContracts = async (benchmarkFixture?: () => Promise<any>) =>
4046

4147
export interface Benchmark {
4248
name: string,
43-
prepare: (contracts: Contracts, target: string) => Promise<SafeTransaction>,
49+
prepare: (contracts: Contracts, target: string, nonce: number) => Promise<SafeTransaction>,
4450
after?: (contracts: Contracts) => Promise<void>,
4551
fixture?: () => Promise<any>
4652
}
@@ -52,7 +58,7 @@ export const benchmark = async (topic: string, benchmarks: Benchmark[]) => {
5258
describe(`${topic} - ${name}`, async () => {
5359
it("with an EOA", async () => {
5460
const contracts = await contractSetup()
55-
const tx = await prepare(contracts, user2.address)
61+
const tx = await prepare(contracts, user2.address, 0)
5662
await logGas(name, user2.sendTransaction({
5763
to: tx.to,
5864
value: tx.value,
@@ -65,7 +71,8 @@ export const benchmark = async (topic: string, benchmarks: Benchmark[]) => {
6571
it(`with a ${config.name} Safe`, async () => {
6672
const contracts = await contractSetup()
6773
const target = contracts.targets[i]
68-
const tx = await prepare(contracts, target.address)
74+
const nonce = await target.nonce();
75+
const tx = await prepare(contracts, target.address, nonce)
6976
const threshold = await target.getThreshold()
7077
const sigs: SafeSignature[] = await Promise.all(config.signers.slice(0, threshold).map(async (signer) => {
7178
return await safeSignTypedData(signer, target, tx)

‎contracts/GnosisSafe.sol‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ pragma solidity >=0.7.0 <0.9.0;
44
import "./base/ModuleManager.sol";
55
import "./base/OwnerManager.sol";
66
import "./base/FallbackManager.sol";
7+
import "./base/GuardManager.sol";
78
import "./common/EtherPaymentFallback.sol";
89
import "./common/Singleton.sol";
910
import "./common/SignatureDecoder.sol";
@@ -16,7 +17,7 @@ import "./external/GnosisSafeMath.sol";
1617
/// @author Stefan George - <stefan@gnosis.io>
1718
/// @author Richard Meissner - <richard@gnosis.io>
1819
contract GnosisSafe
19-
is EtherPaymentFallback, Singleton, ModuleManager, OwnerManager, SignatureDecoder, SecuredTokenTransfer, ISignatureValidatorConstants, FallbackManager, StorageAccessible {
20+
is EtherPaymentFallback, Singleton, ModuleManager, OwnerManager, SignatureDecoder, SecuredTokenTransfer, ISignatureValidatorConstants, FallbackManager, StorageAccessible, GuardManager {
2021

2122
using GnosisSafeMath for uint256;
2223

@@ -142,6 +143,16 @@ contract GnosisSafe
142143
txHash = keccak256(txHashData);
143144
checkSignatures(txHash, txHashData, signatures);
144145
}
146+
{
147+
address guard = getGuard();
148+
if (guard != address(0)) {
149+
Guard(guard).checkTransaction(
150+
to, value, data, operation, // Transaction info
151+
safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, // Payment info
152+
signatures, msg.sender
153+
);
154+
}
155+
}
145156
// We require some gas to emit the events (at least 2500) after the execution and some to perform code until the execution (500)
146157
// We also include the 1/64 in the check that is not send along with a call to counteract potential shortings because of EIP-150
147158
require(gasleft() >= (safeTxGas * 64 / 63).max(safeTxGas + 2500) + 500, "Not enough gas to execute safe transaction");

‎contracts/base/GuardManager.sol‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
// SPDX-License-Identifier: LGPL-3.0-only
2+
pragma solidity >=0.7.0 <0.9.0;
3+
4+
import "../common/Enum.sol";
5+
import "../common/SelfAuthorized.sol";
6+
7+
interface Guard {
8+
function checkTransaction(
9+
address to,
10+
uint256 value,
11+
bytes memory data,
12+
Enum.Operation operation,
13+
uint256 safeTxGas,
14+
uint256 baseGas,
15+
uint256 gasPrice,
16+
address gasToken,
17+
address payable refundReceiver,
18+
bytes memory signatures,
19+
address msgSender
20+
) external;
21+
}
22+
23+
/// @title Fallback Manager - A contract that manages fallback calls made to this contract
24+
/// @author Richard Meissner - <richard@gnosis.pm>
25+
contract GuardManager is SelfAuthorized {
26+
// keccak256("guard_manager.guard.address")
27+
bytes32 internal constant GUARD_STORAGE_SLOT =
28+
0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8;
29+
30+
/// @dev Set a guard that checks transactions before execution
31+
/// @param guard The address of the guard to be used or the 0 address to disable the guard
32+
function setGuard(address guard)
33+
external
34+
authorized
35+
{
36+
bytes32 slot = GUARD_STORAGE_SLOT;
37+
// solium-disable-next-line security/no-inline-assembly
38+
assembly {
39+
sstore(slot, guard)
40+
}
41+
}
42+
43+
function getGuard()
44+
internal
45+
view
46+
returns (address guard)
47+
{
48+
bytes32 slot = GUARD_STORAGE_SLOT;
49+
// solium-disable-next-line security/no-inline-assembly
50+
assembly {
51+
guard := sload(slot)
52+
}
53+
}
54+
}

‎contracts/guards/DelegateCallTransactionGuard.sol‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// SPDX-License-Identifier: LGPL-3.0-only
2+
pragma solidity >=0.7.0 <0.9.0;
3+
4+
import "../common/Enum.sol";
5+
import "../base/GuardManager.sol";
6+
import "../GnosisSafe.sol";
7+
8+
contract DelegateCallTransactionGuard is Guard {
9+
10+
address immutable public allowedTarget;
11+
12+
constructor(address target) {
13+
allowedTarget = target;
14+
}
15+
16+
fallback() external {
17+
// We don't revert on fallback to avoid issues in case of a Safe upgrade
18+
// E.g. The expected check method might change and then the Safe would be locked.
19+
}
20+
21+
function checkTransaction(
22+
address to,
23+
uint256,
24+
bytes memory,
25+
Enum.Operation operation,
26+
uint256,
27+
uint256,
28+
uint256,
29+
address,
30+
address payable,
31+
bytes memory,
32+
address
33+
) override external view {
34+
require(operation != Enum.Operation.DelegateCall || to == allowedTarget, "This call is restricted");
35+
}
36+
}

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

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
import { expect } from "chai";
2+
import hre, { deployments, waffle, ethers } from "hardhat";
3+
import { BigNumber } from "ethers";
4+
import "@nomiclabs/hardhat-ethers";
5+
import { AddressZero } from "@ethersproject/constants";
6+
import { getMock, getSafeWithOwners } from "../utils/setup";
7+
import { buildSafeTransaction, buildSignatureBytes, executeContractCallWithSigners, executeTx, safeApproveHash } from "../utils/execution";
8+
9+
describe("GuardManager", async () => {
10+
11+
const [user1, user2] = waffle.provider.getWallets();
12+
13+
const setupWithTemplate = deployments.createFixture(async ({ deployments }) => {
14+
await deployments.fixture();
15+
const mock = await getMock();
16+
const safe = await getSafeWithOwners([user2.address])
17+
await executeContractCallWithSigners(safe, safe, "setGuard", [mock.address], [user2])
18+
return {
19+
safe,
20+
mock
21+
}
22+
})
23+
24+
describe("setGuard", async () => {
25+
26+
it('is correctly set', async () => {
27+
const { safe, mock } = await setupWithTemplate()
28+
29+
const slot = ethers.utils.keccak256(ethers.utils.toUtf8Bytes("guard_manager.guard.address"))
30+
31+
await executeContractCallWithSigners(safe, safe, "setGuard", [AddressZero], [user2])
32+
33+
// Check fallback handler
34+
await expect(
35+
await hre.ethers.provider.getStorageAt(safe.address, slot)
36+
).to.be.eq("0x" + "".padStart(64, "0"))
37+
38+
await executeContractCallWithSigners(safe, safe, "setGuard", [mock.address], [user2])
39+
40+
// Check fallback handler
41+
await expect(
42+
await hre.ethers.provider.getStorageAt(safe.address, slot)
43+
).to.be.eq("0x" + mock.address.toLowerCase().slice(2).padStart(64, "0"))
44+
})
45+
46+
it('reverts if the guard reverts', async () => {
47+
const { safe, mock } = await setupWithTemplate()
48+
49+
const safeTx = buildSafeTransaction({ to: mock.address, data: "0xbaddad42", nonce: 1 })
50+
const signature = await safeApproveHash(user2, safe, safeTx)
51+
const signatureBytes = buildSignatureBytes([signature])
52+
const guardInterface = (await hre.ethers.getContractAt("Guard", mock.address)).interface
53+
const checkData = guardInterface.encodeFunctionData("checkTransaction", [
54+
safeTx.to, safeTx.value, safeTx.data, safeTx.operation, safeTx.safeTxGas,
55+
safeTx.baseGas, safeTx.gasPrice, safeTx.gasToken, safeTx.refundReceiver,
56+
signatureBytes, user1.address
57+
])
58+
await mock.givenCalldataRevertWithMessage(checkData, "Computer says Nah")
59+
60+
await expect(
61+
executeTx(safe, safeTx, [signature])
62+
).to.be.revertedWith("Computer says Nah")
63+
64+
await mock.reset()
65+
66+
await expect(
67+
executeTx(safe, safeTx, [signature])
68+
).to.emit(safe, "ExecutionSuccess")
69+
70+
expect(await mock.callStatic.invocationCount()).to.be.deep.equals(BigNumber.from(2));
71+
expect(await mock.callStatic.invocationCountForCalldata(checkData)).to.be.deep.equals(BigNumber.from(1));
72+
expect(await mock.callStatic.invocationCountForCalldata("0xbaddad42")).to.be.deep.equals(BigNumber.from(1));
73+
})
74+
})
75+
})

0 commit comments

Comments
 (0)