Skip to content

Commit

Permalink
fix(ct): check Gnosis Safe compatibility using VERSION() function i…
Browse files Browse the repository at this point in the history 
…nstead of codehash
  • Loading branch information
@sam-goldman
sam-goldman committed Dec 13, 2023
1 parent e784660 commit 9d5d0a4
Show file tree
Hide file tree
Showing 8 changed files with 59 additions and 153 deletions.
5 changes: 5 additions & 0 deletions .changeset/orange-dots-mate.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sphinx-labs/contracts': patch
---

Check Gnosis Safe compatibility using `VERSION()` function instead of codehash
59 changes: 12 additions & 47 deletions packages/contracts/contracts/core/SphinxModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,40 +37,14 @@ contract SphinxModule is ReentrancyGuard, Enum, ISphinxModule, Initializable {
string public constant override VERSION = "1.0.0";

/**
* @dev The code hash for the Gnosis Safe proxy v1.3.0.
* @dev The hash of the version string for the Gnosis Safe proxy v1.3.0.
*/
bytes32 internal constant SAFE_PROXY_CODE_HASH_1_3_0 =
0xb89c1b3bdf2cf8827818646bce9a8f6e372885f8c55e5c07acbd307cb133b000;
bytes32 internal constant SAFE_VERSION_HASH_1_3_0 = keccak256("1.3.0");

/**
* @dev The code hash for the Gnosis Safe proxy v1.4.1.
* @dev The hash of the version string for the Gnosis Safe proxy v1.4.1.
*/
bytes32 internal constant SAFE_PROXY_CODE_HASH_1_4_1 =
0xd7d408ebcd99b2b70be43e20253d6d92a8ea8fab29bd3be7f55b10032331fb4c;

/**
* @dev The code hash for the L1 Gnosis Safe singleton v1.3.0.
*/
bytes32 internal constant SAFE_SINGLETON_CODE_HASH_L1_1_3_0 =
0xbba688fbdb21ad2bb58bc320638b43d94e7d100f6f3ebaab0a4e4de6304b1c2e;

/**
* @dev The code hash for the L2 Gnosis Safe singleton v1.3.0.
*/
bytes32 internal constant SAFE_SINGLETON_CODE_HASH_L2_1_3_0 =
0x21842597390c4c6e3c1239e434a682b054bd9548eee5e9b1d6a4482731023c0f;

/**
* @dev The code hash for the L1 Gnosis Safe singleton v1.4.1.
*/
bytes32 internal constant SAFE_SINGLETON_CODE_HASH_L1_1_4_1 =
0x1fe2df852ba3299d6534ef416eefa406e56ced995bca886ab7a553e6d0c5e1c4;

/**
* @dev The code hash for the L2 Gnosis Safe singleton v1.4.1.
*/
bytes32 internal constant SAFE_SINGLETON_CODE_HASH_L2_1_4_1 =
0xb1f926978a0f44a2c0ec8fe822418ae969bd8c3f18d61e5103100339894f81ff;
bytes32 internal constant SAFE_VERSION_HASH_1_4_1 = keccak256("1.4.1");

/**
* @dev The EIP-712 domain separator, which displays a bit of context to the user
Expand Down Expand Up @@ -124,25 +98,16 @@ contract SphinxModule is ReentrancyGuard, Enum, ISphinxModule, Initializable {
function initialize(address _safeProxy) external override initializer {
require(_safeProxy != address(0), "SphinxModule: invalid Safe address");

// Check that the Gnosis Safe proxy's address has a valid code hash.
bytes32 safeProxyCodeHash = _safeProxy.codehash;
require(
safeProxyCodeHash == SAFE_PROXY_CODE_HASH_1_3_0 ||
safeProxyCodeHash == SAFE_PROXY_CODE_HASH_1_4_1,
"SphinxModule: invalid Safe proxy"
);

// Check that the Gnosis Safe proxy has a singleton with a valid code hash. This check
// prevents users from accidentally adding the module to an incompatible Safe. For example,
// if the Safe is an unsupported version, the code hash won't match.
// Check that the Gnosis Safe proxy has a singleton with a valid version. This check
// prevents users from accidentally adding the module to a Gnosis Safe with an invalid
// version.
address safeSingleton = IProxy(_safeProxy).masterCopy();
bytes32 safeSingletonCodeHash = safeSingleton.codehash;
string memory safeVersion = GnosisSafe(payable(safeSingleton)).VERSION();
bytes32 safeVersionHash = keccak256(abi.encodePacked(safeVersion));
require(
safeSingletonCodeHash == SAFE_SINGLETON_CODE_HASH_L1_1_3_0 ||
safeSingletonCodeHash == SAFE_SINGLETON_CODE_HASH_L2_1_3_0 ||
safeSingletonCodeHash == SAFE_SINGLETON_CODE_HASH_L1_1_4_1 ||
safeSingletonCodeHash == SAFE_SINGLETON_CODE_HASH_L2_1_4_1,
"SphinxModule: invalid Safe singleton"
safeVersionHash == SAFE_VERSION_HASH_1_3_0 ||
safeVersionHash == SAFE_VERSION_HASH_1_4_1,
"SphinxModule: invalid Safe version"
);

safeProxy = payable(_safeProxy);
Expand Down
22 changes: 6 additions & 16 deletions packages/contracts/contracts/core/interfaces/ISphinxModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,22 +164,12 @@ interface ISphinxModule {
* constructor because this contract is meant to exist behind an EIP-1167 proxy, which
* isn't able to use constructor arguments.
*
* This call also has a couple of requirements that prevent the user from mistakenly
* adding a `SphinxModuleProxy` to an incompatible Gnosis Safe, potentially leading to
* vulnerabilities in the Gnosis Safe. Specifically, this call will revert if the code
* hash at the input Gnosis Safe proxy's address does not equal the code hash of a
* Gnosis Safe proxy v1.3.0 or v1.4.1. This call will also revert if the Gnosis Safe
* proxy's singleton does not have a code hash that equals the code hash of one of the
* following Gnosis Safe singletons:
* - Gnosis Safe L1 v1.3.0
* - Gnosis Safe L2 v1.3.0
* - Gnosis Safe L1 v1.4.1
* - Gnosis Safe L2 v1.4.1
* These checks do _not_ ensure that the singleton isn't malicious. The singleton can be
* a metamorphic deployment, allowing somebody malicious to self-destruct the singleton,
* causing the user's Gnosis Safe Proxy to be disabled permanently. However, these
* checks are sufficient to prevent users from accidentally adding the module to an
* incompatible Safe.
* This call will revert if the input Gnosis Safe proxy's singleton has a `VERSION()`
* function that does not equal "1.3.0" or "1.4.1". This prevents users from
* accidentally adding the module to an incompatible Safe. This does _not_ ensure that
* the Gnosis Safe singleton isn't malicious. If a singleton has a valid `VERSION()`
* function and arbitrary malicious logic, this call would still consider the singleton
* to be valid.
*
* @param _safeProxy The address of the Gnosis Safe proxy that this contract belongs to.
*/
Expand Down
8 changes: 4 additions & 4 deletions packages/contracts/contracts/foundry/SphinxConstants.sol
View file

Large diffs are not rendered by default.

76 changes: 20 additions & 56 deletions packages/contracts/test/SphinxModuleProxy.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ import {
} from "../contracts/core/SphinxDataTypes.sol";
import { SphinxTransaction, Wallet } from "../contracts/foundry/SphinxPluginTypes.sol";
import { TestUtils } from "./TestUtils.t.sol";
import { MyContract, MyDelegateCallContract } from "./helpers/MyTestContracts.t.sol";
import {
MyContract,
MyDelegateCallContract,
GnosisSafeSingletonInvalidVersion
} from "./helpers/MyTestContracts.t.sol";

/**
* @notice An abstract contract that contains all of the unit tests for the `SphinxModuleProxy`.
Expand Down Expand Up @@ -281,77 +285,37 @@ abstract contract AbstractSphinxModuleProxy_Test is IEnum, TestUtils, SphinxModu
moduleProxyFactory.deploySphinxModuleProxy({ _safeProxy: address(0), _saltNonce: 0 });
}

function test_initialize_reverts_invalidSafeProxy() external {
vm.expectRevert("SphinxModule: invalid Safe proxy");
// Deploy a `SphinxModuleProxy` with a Gnosis Safe proxy address that doesn't have the
// correct codehash. We deploy via the `SphinxModuleProxyFactory` out of convenience.
function test_initialize_reverts_invalidSafeVersion() external {
// Deploy a mock Gnosis Safe singleton with an invalid `VERSION()` function.
GnosisSafeSingletonInvalidVersion singleton = new GnosisSafeSingletonInvalidVersion();
// Deploy a Gnosis Safe Proxy that uses the invalid singleton.
GnosisSafeProxy safeProxy = new GnosisSafeProxy(address(singleton));

vm.expectRevert("SphinxModule: invalid Safe version");
// We deploy via the `SphinxModuleProxyFactory` out of convenience.
moduleProxyFactory.deploySphinxModuleProxy({
_safeProxy: address(1), // Invalid Safe proxy address (i.e. incorrect codehash)
_safeProxy: address(safeProxy),
_saltNonce: 0
});
}

function test_initialize_reverts_invalidSafeSingleton() external {
// Deploy a Gnosis Safe proxy that has the correct codehash, but has a Gnosis Safe singleton
// with an incorrect codehash.
address invalidSafeSingletonAddr = address(1);
bytes memory encodedSafeProxyConstructorArg = abi.encode(invalidSafeSingletonAddr);
address safeProxy;
// We deploy a different Gnosis Safe proxy depending on the Gnosis Safe version.
if (
gnosisSafeVersion == GnosisSafeVersion.L1_1_3_0 ||
gnosisSafeVersion == GnosisSafeVersion.L2_1_3_0
) {
safeProxy = deployCode(
"external-artifacts/gnosis-safe/v1.3.0/proxies/GnosisSafeProxy.sol/GnosisSafeProxy.json",
encodedSafeProxyConstructorArg
);
} else if (
gnosisSafeVersion == GnosisSafeVersion.L1_1_4_1 ||
gnosisSafeVersion == GnosisSafeVersion.L2_1_4_1
) {
safeProxy = deployCode(
"external-artifacts/gnosis-safe/v1.4.1/proxies/SafeProxy.sol/SafeProxy.json",
encodedSafeProxyConstructorArg
);
} else {
revert("Invalid Gnosis Safe version. Should never happen.");
}

vm.expectRevert("SphinxModule: invalid Safe singleton");
// Deploy a valid Gnosis Safe proxy that uses an incorrect Gnosis Safe singleton. We deploy
// via the `SphinxModuleProxyFactory` out of convenience.
moduleProxyFactory.deploySphinxModuleProxy({ _safeProxy: safeProxy, _saltNonce: 0 });
}

function test_initialize_success() external {
assertEq(address(moduleProxy.safeProxy()), address(safeProxy));

// Check that the Gnosis Safe proxy has a valid code hash.
// Check that the Gnosis Safe singleton has a valid version.
address safeSingleton = IProxy(safeProxy).masterCopy();
string memory safeVersion = GnosisSafe(payable(safeSingleton)).VERSION();
bytes32 safeVersionHash = keccak256(abi.encodePacked(safeVersion));
if (
gnosisSafeVersion == GnosisSafeVersion.L1_1_3_0 ||
gnosisSafeVersion == GnosisSafeVersion.L2_1_3_0
) {
assertEq(address(safeProxy).codehash, SAFE_PROXY_CODE_HASH_1_3_0);
assertEq(safeVersionHash, keccak256("1.3.0"));
} else if (
gnosisSafeVersion == GnosisSafeVersion.L1_1_4_1 ||
gnosisSafeVersion == GnosisSafeVersion.L2_1_4_1
) {
assertEq(address(safeProxy).codehash, SAFE_PROXY_CODE_HASH_1_4_1);
} else {
revert("Invalid Gnosis Safe version. Should never happen.");
}

// Check that the Gnosis Safe singleton has a valid code hash.
address safeSingleton = IProxy(safeProxy).masterCopy();
if (gnosisSafeVersion == GnosisSafeVersion.L1_1_3_0) {
assertEq(safeSingleton.codehash, SAFE_SINGLETON_CODE_HASH_L1_1_3_0);
} else if (gnosisSafeVersion == GnosisSafeVersion.L2_1_3_0) {
assertEq(safeSingleton.codehash, SAFE_SINGLETON_CODE_HASH_L2_1_3_0);
} else if (gnosisSafeVersion == GnosisSafeVersion.L1_1_4_1) {
assertEq(safeSingleton.codehash, SAFE_SINGLETON_CODE_HASH_L1_1_4_1);
} else if (gnosisSafeVersion == GnosisSafeVersion.L2_1_4_1) {
assertEq(safeSingleton.codehash, SAFE_SINGLETON_CODE_HASH_L2_1_4_1);
assertEq(safeVersionHash, keccak256("1.4.1"));
} else {
revert("Invalid Gnosis Safe version. Should never happen.");
}
Expand Down
8 changes: 4 additions & 4 deletions packages/contracts/test/TestUtils.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -516,8 +516,8 @@ contract TestUtils is SphinxUtils, IEnum, Test {

function deployGnosisSafeContracts_1_3_0() public returns (GnosisSafeContracts_1_3_0 memory) {
// Deploy the Gnosis Safe Proxy Factory and the Gnosis Safe singletons using the exact
// initcode in the artifact files. This ensures that they produce contracts with correct
// code hashes, which is necessary when initializing the `SphinxModuleProxy`.
// initcode in the artifact files. This isn't strictly necessary, but we do it anyways
// to emulate the production environment.
address safeProxyFactoryAddr = deployCodeViaCreate2(
"external-artifacts/gnosis-safe/v1.3.0/proxies/GnosisSafeProxyFactory.sol/GnosisSafeProxyFactory.json",
bytes32(0)
Expand Down Expand Up @@ -553,8 +553,8 @@ contract TestUtils is SphinxUtils, IEnum, Test {

function deployGnosisSafeContracts_1_4_1() public returns (GnosisSafeContracts_1_4_1 memory) {
// Deploy the Gnosis Safe Proxy Factory and the Gnosis Safe singletons using the exact
// initcode in the artifact files. This ensures that they produce contracts with correct
// code hashes, which is necessary when initializing the `SphinxModuleProxy`.
// initcode in the artifact files. This isn't strictly necessary, but we do it anyways
// to emulate the production environment.
address safeProxyFactoryAddr = deployCodeViaCreate2(
"external-artifacts/gnosis-safe/v1.4.1/proxies/SafeProxyFactory.sol/SafeProxyFactory.json",
bytes32(0)
Expand Down
4 changes: 4 additions & 0 deletions packages/contracts/test/helpers/MyTestContracts.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,7 @@ contract MyDelegateCallContract {
wasDelegateCalled = true;
}
}

contract GnosisSafeSingletonInvalidVersion {
string public VERSION = "1.2.0";
}
Loading

0 comments on commit 9d5d0a4

Please sign in to comment.