From 10c99c42df6843a8709fe53cd84e81b37f776425 Mon Sep 17 00:00:00 2001 From: Duncan Townsend Date: Wed, 20 Aug 2025 13:51:35 -0400 Subject: [PATCH 01/11] Add a variation of the Safe{Wallet} Guard timelock for Safe{Wallet} version 1.4.1 --- src/deployer/SafeGuard.sol | 109 +++++++++++++++++++----- test/0.8.25/SafeGuard.t.sol | 162 ++++++++++++++++++++++++++++++++---- 2 files changed, 231 insertions(+), 40 deletions(-) diff --git a/src/deployer/SafeGuard.sol b/src/deployer/SafeGuard.sol index eb51b27cc..71963dfa8 100644 --- a/src/deployer/SafeGuard.sol +++ b/src/deployer/SafeGuard.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity =0.8.25; +import {IERC165} from "@forge-std/interfaces/IERC165.sol"; + // This enum is derived from the code deployed to 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA enum Operation { Call, @@ -178,7 +180,7 @@ contract EvmVersionDummy { } } -contract ZeroExSettlerDeployerSafeGuard is IGuard { +contract ZeroExSettlerDeployerSafeGuardBase is IGuard { using SafeLib for ISafeMinimal; event TimelockUpdated(uint256 oldDelay, uint256 newDelay); @@ -230,14 +232,7 @@ contract ZeroExSettlerDeployerSafeGuard is IGuard { uint256 internal constant _MINIMUM_OWNERS = 3; uint256 internal constant _MINIMUM_THRESHOLD = 2; - bytes32 private constant _SINGLETON_INITHASH = 0x49f30800a6ac5996a48b80c47ff20f19f8728812498a2a7fe75a14864fab6438; - address private immutable _SINGLETON = address( - uint160( - uint256( - keccak256(bytes.concat(bytes1(0xff), bytes20(uint160(msg.sender)), bytes32(0), _SINGLETON_INITHASH)) - ) - ) - ); + address private immutable _SINGLETON; address private constant _CREATE2_FACTORY = 0x4e59b44847b379578588920cA78FbF26c0B4956C; address private constant _SAFE_SINGLETON_FACTORY = 0x914d7Fec6aaC8cd542e72Bca78B30650d45643d7; @@ -245,21 +240,17 @@ contract ZeroExSettlerDeployerSafeGuard is IGuard { bytes32 private constant _EVM_VERSION_DUMMY_INITHASH = 0xe7bcbbfee5c3a9a42621a8cbb24d1eade8e9469bc40e23d16b5d0607ba27027a; - constructor(ISafeMinimal safe_) { + constructor(ISafeMinimal safe_, bytes32 singletonInithash) { safe = safe_; assert(keccak256(type(EvmVersionDummy).creationCode) == _EVM_VERSION_DUMMY_INITHASH || block.chainid == 31337); assert(msg.sender == _CREATE2_FACTORY || msg.sender == _SAFE_SINGLETON_FACTORY); - - // These checks ensure that the Guard is safely installed in the Safe at the time it is - // deployed, with the exception of the installation and subsequent concealment of a - // malicious Safe module. The author knows of no way to enforce that the Guard is installed - // atomic with its deployment. This introduces a TOCTOU vulnerability. Therefore, extensive - // simulation and excessive caution are imperative in this process. If the Guard is - // installed in a Safe where these checks fail, the Safe is bricked. Once the Guard is - // successfully deployed, the behavior ought to be sane, even in bizarre and outrageous - // circumstances. - _checkAfterExecution(safe); - assert(!_guardRemoved); + _SINGLETON = address( + uint160( + uint256( + keccak256(bytes.concat(bytes1(0xff), bytes20(uint160(msg.sender)), bytes32(0), singletonInithash)) + ) + ) + ); } function setDelay(uint24 newDelay) external onlySafe { @@ -445,9 +436,24 @@ contract ZeroExSettlerDeployerSafeGuard is IGuard { ISafeMinimal _safe = ISafeMinimal(msg.sender); _checkAfterExecution(_safe); + _maybeSetGuardRemoved(_safe); + } + + function _checkAfterExecutionReturnBool(ISafeMinimal _safe) internal view returns (bool result) { + result = true; + + // See comments in `_checkAfterExecution` for an explanation of all these conditions + result = result && _safe.masterCopy() == _SINGLETON; + if (result) { + (address[] memory modules,) = _safe.getModulesPaginated(address(1), 1); + result = modules.length == 0; + } + result = result && !_safe.isOwner(address(this)); + result = result && _safe.ownerCount() >= _MINIMUM_OWNERS; + result = result && _safe.getThreshold() >= _MINIMUM_THRESHOLD; } - function _checkAfterExecution(ISafeMinimal _safe) private { + function _checkAfterExecution(ISafeMinimal _safe) private view { // The knowledge that the immutable `safe` address is computed using the `CREATE2` pattern // from trusted initcode (and a factory likewise deployed by trusted initcode) gives us a // pretty strong toehold of trust. We do not need to recheck this, ever. Furthermore, @@ -496,14 +502,20 @@ contract ZeroExSettlerDeployerSafeGuard is IGuard { revert ThresholdTooLow(threshold); } } + } + function _removeSelf() internal { + _guardRemoved = true; + } + + function _maybeSetGuardRemoved(ISafeMinimal _safe) internal { // We do not revert if `_safe.getGuard()` returns a value other than `address(this)`. This // allows uninstallation of the guard (through the timelock, obviously) to later permit // upgrades to other singleton implementation contracts. However, we do set the // `_guardRemoved` flag, which disables all Guard functionality (failing open). It is not // possible to un-set `_guardRemoved` once set. if (_safe.getGuard() != address(this)) { - _guardRemoved = true; + _removeSelf(); } } @@ -646,3 +658,54 @@ contract ZeroExSettlerDeployerSafeGuard is IGuard { emit Unlocked(); } } + +contract ZeroExSettlerDeployerSafeGuardOnePointThree is ZeroExSettlerDeployerSafeGuardBase { + constructor(ISafeMinimal safe_) + ZeroExSettlerDeployerSafeGuardBase(safe_, 0x49f30800a6ac5996a48b80c47ff20f19f8728812498a2a7fe75a14864fab6438) + { + // These checks ensure that the Guard is safely installed in the Safe at the time it is + // deployed, with the exception of the installation and subsequent concealment of a + // malicious Safe module. The author knows of no way to enforce that the Guard is installed + // atomic with its deployment. This introduces a TOCTOU vulnerability. Therefore, extensive + // simulation and excessive caution are imperative in this process. If the Guard is + // installed in a Safe where these checks fail, the Guard silently disables itself in order + // to avoid a bricked Safe. Once the Guard is successfully deployed, the behavior ought to + // be sane, even in bizarre and outrageous circumstances. + if (!_checkAfterExecutionReturnBool(safe_)) { + _removeSelf(); + } else { + _maybeSetGuardRemoved(safe_); + } + } +} + +contract ZeroExSettlerDeployerSafeGuardOnePointFourPointOne is IERC165, ZeroExSettlerDeployerSafeGuardBase { + constructor(ISafeMinimal safe_) + ZeroExSettlerDeployerSafeGuardBase(safe_, 0x3555bd3ee95b1c6605c602740d71efaf200068e0395ccd701ac82ab8e42307bd) + { + // In contrast to the 1.3.0 Guard, the 1.4.1 Guard must be deployed *before* being enabled + // in the Safe. However, because the Safe does an ERC165 check during the Guard enabling + // process, we are able to perform a nearly atomic check. See the logic and comment in + // `supportsInterface` below. + } + + /// @inheritdoc IERC165 + function supportsInterface(bytes4 interfaceID) external view override returns (bool) { + if (uint32(interfaceID) == uint32(type(IERC165).interfaceId)) { + return true; + } else if (uint32(interfaceID) == 0xffffffff) { + return false; + } else { + // These checks ensure that the Safe (with the exception of clandestine modules) is in a + // sane configuration at the time that the Guard is installed. Obviously because the + // Guard's `checkAfterExecution is not run during the installation, it's still possible + // to misconfigure the Safe or conceal a Module after the Guard's installation (e.g. via + // delegate'd MultiCall). However, presuming the Safe owners don't do that, at the + // conclusion of the installation of the Guard, the behavior ought to remain sane, even + // in bizarre and outrageous circumstances. + ISafeMinimal safe_ = safe; + return msg.sender == address(safe_) && uint32(interfaceID) == uint32(type(IGuard).interfaceId) + && _checkAfterExecutionReturnBool(safe); + } + } +} diff --git a/test/0.8.25/SafeGuard.t.sol b/test/0.8.25/SafeGuard.t.sol index 86d45dcc1..58058d9da 100644 --- a/test/0.8.25/SafeGuard.t.sol +++ b/test/0.8.25/SafeGuard.t.sol @@ -15,6 +15,8 @@ interface ISafeSetup { function getOwners() external view returns (address[] memory); function setGuard(address guard) external; + + function setFallbackHandler(address handler) external; } enum Operation { @@ -49,6 +51,13 @@ interface ISafe { function isOwner(address) external view returns (bool); function enableModule(address) external; + + function masterCopy() external view returns (address); +} + +interface ISafeOnePointFour { + // `txHash` argument is indexed in 1.4 + event ExecutionSuccess(bytes32 indexed txHash, uint256 payment); } interface IGuard { @@ -145,11 +154,21 @@ interface IMulticall { function multiSend(bytes memory transactions) external payable; } +contract MigrationDummy { + address private singleton; + + function migrate(address newSingleton, address newFallbackHandler) external { + singleton = newSingleton; + ISafeSetup(address(this)).setFallbackHandler(newFallbackHandler); + } +} + contract TestSafeGuard is Test { using ItoA for uint256; address internal constant factory = 0x914d7Fec6aaC8cd542e72Bca78B30650d45643d7; ISafe internal constant safe = ISafe(0xf36b9f50E59870A24F42F9Ba43b2aD0A4b8f2F51); + address internal constant onePointThreeSingleton = 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA; IZeroExSettlerDeployerSafeGuard internal guard; uint256 internal pokeCounter; @@ -158,7 +177,7 @@ contract TestSafeGuard is Test { function setUp() public { ISafeSetup _safe = ISafeSetup(address(safe)); - vm.createSelectFork(vm.envString("MAINNET_RPC_URL"), 21015655); + vm.createSelectFork(vm.envString("MAINNET_RPC_URL"), 23183520); vm.label(address(this), "FoundryTest"); string memory mnemonic = "test test test test test test test test test test test junk"; @@ -178,7 +197,7 @@ contract TestSafeGuard is Test { vm.stopPrank(); bytes memory creationCode = bytes.concat( - vm.getCode("SafeGuard.sol:ZeroExSettlerDeployerSafeGuard"), + vm.getCode("SafeGuard.sol:ZeroExSettlerDeployerSafeGuardOnePointThree"), abi.encode(0xf36b9f50E59870A24F42F9Ba43b2aD0A4b8f2F51) ); guard = IZeroExSettlerDeployerSafeGuard( @@ -304,6 +323,7 @@ contract TestSafeGuard is Test { } function testHappyPath() public { + address singleton = safe.masterCopy(); ( address to, uint256 value, @@ -322,7 +342,11 @@ contract TestSafeGuard is Test { vm.warp(vm.getBlockTimestamp() + guard.delay() + 1 seconds); vm.expectEmit(true, true, true, true, address(safe)); - emit ISafe.ExecutionSuccess(txHash, 0); + if (singleton == onePointThreeSingleton) { + emit ISafe.ExecutionSuccess(txHash, 0); + } else { + emit ISafeOnePointFour.ExecutionSuccess(txHash, 0); + } safe.execTransaction( to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, signatures ); @@ -407,9 +431,9 @@ contract TestSafeGuard is Test { function testCancelNoApprove() external { (,,,,,,,,,, bytes32 txHash,) = _enqueuePoke(); - bytes32 resignTxHash = guard.resignTxHash(owners[4].addr); + bytes32 resignTxHash = guard.resignTxHash(owners[3].addr); - vm.prank(owners[4].addr); + vm.prank(owners[3].addr); vm.expectRevert( abi.encodeWithSelector(IZeroExSettlerDeployerSafeGuard.TxHashNotApproved.selector, resignTxHash) ); @@ -444,21 +468,21 @@ contract TestSafeGuard is Test { bytes32 unlockTxHash = guard.unlockTxHash(); - vm.startPrank(owners[4].addr); + vm.startPrank(owners[3].addr); vm.expectEmit(true, true, true, true, address(safe)); - emit ISafe.ApproveHash(unlockTxHash, owners[4].addr); + emit ISafe.ApproveHash(unlockTxHash, owners[3].addr); safe.approveHash(unlockTxHash); vm.expectEmit(true, true, true, true, address(guard)); - emit IZeroExSettlerDeployerSafeGuard.LockDown(owners[4].addr, unlockTxHash); + emit IZeroExSettlerDeployerSafeGuard.LockDown(owners[3].addr, unlockTxHash); guard.lockDown(); vm.stopPrank(); vm.warp(vm.getBlockTimestamp() + guard.delay() + 1 seconds); - vm.expectRevert(abi.encodeWithSelector(IZeroExSettlerDeployerSafeGuard.LockedDown.selector, owners[4].addr)); + vm.expectRevert(abi.encodeWithSelector(IZeroExSettlerDeployerSafeGuard.LockedDown.selector, owners[3].addr)); safe.execTransaction( to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, signatures ); @@ -467,7 +491,7 @@ contract TestSafeGuard is Test { function testLockDownNoUnlock() external { bytes32 unlockTxHash = guard.unlockTxHash(); - vm.prank(owners[4].addr); + vm.prank(owners[3].addr); vm.expectRevert( abi.encodeWithSelector(IZeroExSettlerDeployerSafeGuard.TxHashNotApproved.selector, unlockTxHash) @@ -478,7 +502,7 @@ contract TestSafeGuard is Test { function testLockDownNoCancel() external { (,,,,,,,,,, bytes32 txHash,) = _enqueuePoke(); - address owner = owners[4].addr; + address owner = owners[3].addr; bytes32 resignTxHash = guard.resignTxHash(owner); bytes32 unlockTxHash = guard.unlockTxHash(); @@ -502,7 +526,7 @@ contract TestSafeGuard is Test { function testLockDownWithCancel() external { (,,,,,,,,,, bytes32 txHash,) = _enqueuePoke(); - address owner = owners[4].addr; + address owner = owners[3].addr; bytes32 resignTxHash = guard.resignTxHash(owner); bytes32 unlockTxHash = guard.unlockTxHash(); @@ -525,7 +549,7 @@ contract TestSafeGuard is Test { function testResign() external { (,,,,,,,,,, bytes32 txHash,) = _enqueuePoke(); - address owner = owners[4].addr; + address owner = owners[3].addr; bytes32 resignTxHash = guard.resignTxHash(owner); @@ -534,7 +558,7 @@ contract TestSafeGuard is Test { guard.cancel(txHash); vm.stopPrank(); - address prevOwner = owners[2].addr; + address prevOwner = owners[1].addr; bytes memory data = abi.encodeWithSignature("removeOwner(address,address,uint256)", prevOwner, owner, 2); txHash = keccak256( @@ -697,7 +721,7 @@ contract TestSafeGuard is Test { _signSafeEncoded(owners[1], unlockTxHash), _signSafeEncoded(owners[2], unlockTxHash), _signSafeEncoded(owners[3], unlockTxHash), - uint256(uint160(owners[4].addr)), + uint256(uint160(owners[3].addr)), bytes32(0), uint8(1) ); @@ -770,8 +794,7 @@ contract TestSafeGuard is Test { bytes memory unlockSignatures = abi.encodePacked( _signSafeEncoded(owners[1], unlockTxHash), _signSafeEncoded(owners[2], unlockTxHash), - _signSafeEncoded(owners[3], unlockTxHash), - uint256(uint160(owners[4].addr)), + uint256(uint160(owners[3].addr)), bytes32(0), uint8(1) ); @@ -915,4 +938,109 @@ contract TestSafeGuard is Test { to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, signatures ); } + + function testOnePointFour() external { + // uninstall the 1.3 guard + vm.store(address(safe), 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8, bytes32(0)); + + // migrate to 1.4.1 + address onePointFourSingleton = 0x29fcB43b46531BcA003ddC8FCB67FFE91900C762; + address onePointFourFallback = 0xfd0732Dc9E303f09fCEf3a7388Ad10A83459Ec99; + + MigrationDummy migration = new MigrationDummy(); + + bytes memory data = abi.encodeCall(migration.migrate, (onePointFourSingleton, onePointFourFallback)); + bytes32 txHash = keccak256( + bytes.concat( + hex"1901", + keccak256( + abi.encode( + keccak256("EIP712Domain(uint256 chainId,address verifyingContract)"), block.chainid, safe + ) + ), + keccak256( + abi.encode( + keccak256( + "SafeTx(address to,uint256 value,bytes data,uint8 operation,uint256 safeTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,uint256 nonce)" + ), + address(migration), + 0 wei, + keccak256(data), + Operation.DelegateCall, + 0, + 0, + 0 gwei, + address(0), + payable(address(0)), + safe.nonce() + ) + ) + ) + ); + bytes memory signatures = + abi.encodePacked(_signSafeEncoded(owners[0], txHash), _signSafeEncoded(owners[1], txHash)); + + safe.execTransaction( + address(migration), + 0 wei, + data, + Operation.DelegateCall, + 0, + 0, + 0 gwei, + address(0), + payable(address(0)), + signatures + ); + + // check that we successfully migrated to 1.4.1 + assertEq(safe.masterCopy(), onePointFourSingleton); + + // the 1.4.1 guard has to be deployed *before* being enabled + bytes memory creationCode = bytes.concat( + vm.getCode("SafeGuard.sol:ZeroExSettlerDeployerSafeGuardOnePointFourPointOne"), abi.encode(address(safe)) + ); + guard = IZeroExSettlerDeployerSafeGuard( + AddressDerivation.deriveDeterministicContract(factory, bytes32(0), keccak256(creationCode)) + ); + (bool success, bytes memory returndata) = factory.call(bytes.concat(bytes32(0), creationCode)); + assertTrue(success); + assertEq(address(uint160(bytes20(returndata))), address(guard)); + + data = abi.encodeCall(ISafeSetup.setGuard, (address(guard))); + txHash = keccak256( + bytes.concat( + hex"1901", + keccak256( + abi.encode( + keccak256("EIP712Domain(uint256 chainId,address verifyingContract)"), block.chainid, safe + ) + ), + keccak256( + abi.encode( + keccak256( + "SafeTx(address to,uint256 value,bytes data,uint8 operation,uint256 safeTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,uint256 nonce)" + ), + address(safe), + 0 wei, + keccak256(data), + Operation.Call, + 0, + 0, + 0 gwei, + address(0), + payable(address(0)), + safe.nonce() + ) + ) + ) + ); + signatures = abi.encodePacked(_signSafeEncoded(owners[0], txHash), _signSafeEncoded(owners[1], txHash)); + + safe.execTransaction( + address(safe), 0 wei, data, Operation.Call, 0, 0, 0 gwei, address(0), payable(address(0)), signatures + ); + + testHappyPath(); + } } From ce30100da75be9da037f80ca7980555757ccdd17 Mon Sep 17 00:00:00 2001 From: Duncan Townsend Date: Wed, 20 Aug 2025 14:13:21 -0400 Subject: [PATCH 02/11] Add some `vm.expectCall` --- test/0.8.25/SafeGuard.t.sol | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/0.8.25/SafeGuard.t.sol b/test/0.8.25/SafeGuard.t.sol index 58058d9da..b3d80f8c4 100644 --- a/test/0.8.25/SafeGuard.t.sol +++ b/test/0.8.25/SafeGuard.t.sol @@ -52,6 +52,8 @@ interface ISafe { function enableModule(address) external; + function getStorageAt(uint256 offset, uint256 length) external view returns (bytes memory); + function masterCopy() external view returns (address); } @@ -324,6 +326,10 @@ contract TestSafeGuard is Test { function testHappyPath() public { address singleton = safe.masterCopy(); + assertEq( + abi.decode(safe.getStorageAt(uint256(keccak256("guard_manager.guard.address")), 1), (address)), + address(guard) + ); ( address to, uint256 value, @@ -341,6 +347,26 @@ contract TestSafeGuard is Test { vm.warp(vm.getBlockTimestamp() + guard.delay() + 1 seconds); + vm.expectCall( + address(guard), + abi.encodeCall( + guard.checkTransaction, + ( + to, + value, + data, + operation, + safeTxGas, + baseGas, + gasPrice, + gasToken, + refundReceiver, + signatures, + address(this) + ) + ) + ); + vm.expectCall(address(guard), abi.encodeCall(guard.checkAfterExecution, (txHash, true))); vm.expectEmit(true, true, true, true, address(safe)); if (singleton == onePointThreeSingleton) { emit ISafe.ExecutionSuccess(txHash, 0); @@ -1007,6 +1033,7 @@ contract TestSafeGuard is Test { assertTrue(success); assertEq(address(uint160(bytes20(returndata))), address(guard)); + // install the guard data = abi.encodeCall(ISafeSetup.setGuard, (address(guard))); txHash = keccak256( bytes.concat( From a919bb458d3f95bcff5d58bbcdb6d70a52949c9e Mon Sep 17 00:00:00 2001 From: Duncan Townsend Date: Wed, 20 Aug 2025 14:23:11 -0400 Subject: [PATCH 03/11] Avoid opaque constants in test files --- test/0.8.25/SafeGuard.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/0.8.25/SafeGuard.t.sol b/test/0.8.25/SafeGuard.t.sol index b3d80f8c4..4e4d44baa 100644 --- a/test/0.8.25/SafeGuard.t.sol +++ b/test/0.8.25/SafeGuard.t.sol @@ -841,7 +841,7 @@ contract TestSafeGuard is Test { // This just validates that the signatures as encoded are otherwise // valid in the absence of the guard's checks - vm.store(address(safe), 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8, bytes32(0)); + vm.store(address(safe), keccak256("guard_manager.guard.address"), bytes32(0)); vm.expectEmit(true, true, true, true, address(safe)); emit ISafe.ExecutionSuccess(unlockTxHash, 0); safe.execTransaction( From 5a2193be395fa5d264885d0ec76197edaaf48eb8 Mon Sep 17 00:00:00 2001 From: Duncan Townsend Date: Wed, 20 Aug 2025 14:33:29 -0400 Subject: [PATCH 04/11] Avoid opaque constants in test files --- test/0.8.25/SafeGuard.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/0.8.25/SafeGuard.t.sol b/test/0.8.25/SafeGuard.t.sol index 4e4d44baa..99c60ba7d 100644 --- a/test/0.8.25/SafeGuard.t.sol +++ b/test/0.8.25/SafeGuard.t.sol @@ -967,7 +967,7 @@ contract TestSafeGuard is Test { function testOnePointFour() external { // uninstall the 1.3 guard - vm.store(address(safe), 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8, bytes32(0)); + vm.store(address(safe), keccak256("guard_manager.guard.address"), bytes32(0)); // migrate to 1.4.1 address onePointFourSingleton = 0x29fcB43b46531BcA003ddC8FCB67FFE91900C762; From 099466fd152c03d4633284c399ce2518d15bcb9b Mon Sep 17 00:00:00 2001 From: Duncan Townsend Date: Wed, 20 Aug 2025 16:31:23 -0400 Subject: [PATCH 05/11] Add references to canonical Safe{Wallet} 1.4.1 deployment addresses --- src/deployer/SafeGuard.sol | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/deployer/SafeGuard.sol b/src/deployer/SafeGuard.sol index 71963dfa8..01d0eda65 100644 --- a/src/deployer/SafeGuard.sol +++ b/src/deployer/SafeGuard.sol @@ -3,13 +3,15 @@ pragma solidity =0.8.25; import {IERC165} from "@forge-std/interfaces/IERC165.sol"; -// This enum is derived from the code deployed to 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA +// This enum is derived from the code deployed to 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA (1.3.0) +// or 0x29fcB43b46531BcA003ddC8FCB67FFE91900C762 (1.4.1) enum Operation { Call, DelegateCall } // This interface is excerpted from the contract at 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA +// (1.3.0) or 0x29fcB43b46531BcA003ddC8FCB67FFE91900C762 (1.4.1) interface ISafeMinimal { function checkNSignatures(bytes32 dataHash, bytes memory data, bytes memory signatures, uint256 requiredSignatures) external @@ -34,14 +36,17 @@ interface ISafeMinimal { view returns (address[] memory array, address next); - // This function is not part of the interface at 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA - // . It's part of the implicit interface on the proxy contract(s) created by the factory at - // 0xc22834581ebc8527d974f8a1c97e1bea4ef910bc . + // This function is not part of the interface at + // 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA/0x29fcB43b46531BcA003ddC8FCB67FFE91900C762 . It's + // part of the implicit interface on the proxy contract(s) created by the factory at + // 0xc22834581ebc8527d974f8a1c97e1bea4ef910bc (1.3.0) or + // 0x4e1DCf7AD4e460CfD30791CCC4F9c8a4f820ec67 (1.4.1) . function masterCopy() external view returns (address); } // This library is a reimplementation of the functionality of the functions by the same name in -// 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA +// 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA (1.3.0) or 0x29fcB43b46531BcA003ddC8FCB67FFE91900C762 +// (1.4.1) library SafeLib { function encodeTransactionData( ISafeMinimal safe, @@ -153,6 +158,7 @@ library SafeLib { } // This interface is excerpted from `GuardManager.sol` in 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA +// (1.3.0) or 0x29fcB43b46531BcA003ddC8FCB67FFE91900C762 (1.4.1) interface IGuard { function checkTransaction( address to, From 40b9b4e46ce854d530b946f38db0f41dd2460328 Mon Sep 17 00:00:00 2001 From: Duncan Townsend Date: Wed, 20 Aug 2025 16:41:00 -0400 Subject: [PATCH 06/11] Check Safe{Wallet} proxy code hash on deployment --- src/deployer/SafeGuard.sol | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/deployer/SafeGuard.sol b/src/deployer/SafeGuard.sol index 01d0eda65..4f4bc41b2 100644 --- a/src/deployer/SafeGuard.sol +++ b/src/deployer/SafeGuard.sol @@ -242,13 +242,25 @@ contract ZeroExSettlerDeployerSafeGuardBase is IGuard { address private constant _CREATE2_FACTORY = 0x4e59b44847b379578588920cA78FbF26c0B4956C; address private constant _SAFE_SINGLETON_FACTORY = 0x914d7Fec6aaC8cd542e72Bca78B30650d45643d7; + bytes32 private constant _SAFE_PROXY_1_1_CODEHASH = + 0xaea7d4252f6245f301e540cfbee27d3a88de543af8e49c5c62405d5499fab7e5; + bytes32 private constant _SAFE_PROXY_1_3_CODEHASH = + 0xb89c1b3bdf2cf8827818646bce9a8f6e372885f8c55e5c07acbd307cb133b000; + bytes32 private constant _SAFE_PROXY_1_4_CODEHASH = + 0xd7d408ebcd99b2b70be43e20253d6d92a8ea8fab29bd3be7f55b10032331fb4c; + // This is the correct hash only if this contract has been compiled for the London hardfork bytes32 private constant _EVM_VERSION_DUMMY_INITHASH = 0xe7bcbbfee5c3a9a42621a8cbb24d1eade8e9469bc40e23d16b5d0607ba27027a; constructor(ISafeMinimal safe_, bytes32 singletonInithash) { - safe = safe_; assert(keccak256(type(EvmVersionDummy).creationCode) == _EVM_VERSION_DUMMY_INITHASH || block.chainid == 31337); + safe = safe_; + bytes32 safeCodeHash = address(safe).codehash; + assert( + safeCodeHash == _SAFE_PROXY_1_1_CODEHASH || safeCodeHash == _SAFE_PROXY_1_3_CODEHASH + || safeCodeHash == _SAFE_PROXY_1_4_CODEHASH + ); assert(msg.sender == _CREATE2_FACTORY || msg.sender == _SAFE_SINGLETON_FACTORY); _SINGLETON = address( uint160( From b9cfcb721caddc896b1abaa51b839bbf46100b59 Mon Sep 17 00:00:00 2001 From: Duncan Townsend Date: Wed, 20 Aug 2025 16:44:30 -0400 Subject: [PATCH 07/11] Add reference to Safe{Wallet} 1.1.1 factory, because that is also supported --- src/deployer/SafeGuard.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/deployer/SafeGuard.sol b/src/deployer/SafeGuard.sol index 4f4bc41b2..43746ce94 100644 --- a/src/deployer/SafeGuard.sol +++ b/src/deployer/SafeGuard.sol @@ -39,7 +39,8 @@ interface ISafeMinimal { // This function is not part of the interface at // 0xfb1bffC9d739B8D520DaF37dF666da4C687191EA/0x29fcB43b46531BcA003ddC8FCB67FFE91900C762 . It's // part of the implicit interface on the proxy contract(s) created by the factory at - // 0xc22834581ebc8527d974f8a1c97e1bea4ef910bc (1.3.0) or + // 0x76E2cFc1F5Fa8F6a5b3fC4c8F4788F0116861F9B (1.1.1), + // 0xc22834581ebc8527d974f8a1c97e1bea4ef910bc (1.3.0), or // 0x4e1DCf7AD4e460CfD30791CCC4F9c8a4f820ec67 (1.4.1) . function masterCopy() external view returns (address); } From 5f5ae6b0ff4cac66898535f053d906bd9706285e Mon Sep 17 00:00:00 2001 From: Duncan Townsend Date: Wed, 20 Aug 2025 16:53:34 -0400 Subject: [PATCH 08/11] Give names to constants --- src/deployer/SafeGuard.sol | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/deployer/SafeGuard.sol b/src/deployer/SafeGuard.sol index 43746ce94..1411e68d3 100644 --- a/src/deployer/SafeGuard.sol +++ b/src/deployer/SafeGuard.sol @@ -679,9 +679,10 @@ contract ZeroExSettlerDeployerSafeGuardBase is IGuard { } contract ZeroExSettlerDeployerSafeGuardOnePointThree is ZeroExSettlerDeployerSafeGuardBase { - constructor(ISafeMinimal safe_) - ZeroExSettlerDeployerSafeGuardBase(safe_, 0x49f30800a6ac5996a48b80c47ff20f19f8728812498a2a7fe75a14864fab6438) - { + bytes32 private constant _SAFE_SINGLETON_1_3_INITHASH = + 0x49f30800a6ac5996a48b80c47ff20f19f8728812498a2a7fe75a14864fab6438; + + constructor(ISafeMinimal safe_) ZeroExSettlerDeployerSafeGuardBase(safe_, _SAFE_SINGLETON_1_3_INITHASH) { // These checks ensure that the Guard is safely installed in the Safe at the time it is // deployed, with the exception of the installation and subsequent concealment of a // malicious Safe module. The author knows of no way to enforce that the Guard is installed @@ -699,9 +700,10 @@ contract ZeroExSettlerDeployerSafeGuardOnePointThree is ZeroExSettlerDeployerSaf } contract ZeroExSettlerDeployerSafeGuardOnePointFourPointOne is IERC165, ZeroExSettlerDeployerSafeGuardBase { - constructor(ISafeMinimal safe_) - ZeroExSettlerDeployerSafeGuardBase(safe_, 0x3555bd3ee95b1c6605c602740d71efaf200068e0395ccd701ac82ab8e42307bd) - { + bytes32 private constant _SAFE_SINGLETON_1_4_INITHASH = + 0x3555bd3ee95b1c6605c602740d71efaf200068e0395ccd701ac82ab8e42307bd; + + constructor(ISafeMinimal safe_) ZeroExSettlerDeployerSafeGuardBase(safe_, _SAFE_SINGLETON_1_4_INITHASH) { // In contrast to the 1.3.0 Guard, the 1.4.1 Guard must be deployed *before* being enabled // in the Safe. However, because the Safe does an ERC165 check during the Guard enabling // process, we are able to perform a nearly atomic check. See the logic and comment in From 3a1e1d594ee4b7586afee35af2286b655f3c5c8e Mon Sep 17 00:00:00 2001 From: Duncan Townsend Date: Wed, 20 Aug 2025 17:01:31 -0400 Subject: [PATCH 09/11] `abstract` --- src/deployer/SafeGuard.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployer/SafeGuard.sol b/src/deployer/SafeGuard.sol index 1411e68d3..67a38e7e2 100644 --- a/src/deployer/SafeGuard.sol +++ b/src/deployer/SafeGuard.sol @@ -187,7 +187,7 @@ contract EvmVersionDummy { } } -contract ZeroExSettlerDeployerSafeGuardBase is IGuard { +abstract contract ZeroExSettlerDeployerSafeGuardBase is IGuard { using SafeLib for ISafeMinimal; event TimelockUpdated(uint256 oldDelay, uint256 newDelay); From 30036d9b8c4126231cca4155339ddea79ae36ef2 Mon Sep 17 00:00:00 2001 From: Duncan Townsend Date: Wed, 20 Aug 2025 17:06:44 -0400 Subject: [PATCH 10/11] Typo --- src/deployer/SafeGuard.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/deployer/SafeGuard.sol b/src/deployer/SafeGuard.sol index 67a38e7e2..195b30716 100644 --- a/src/deployer/SafeGuard.sol +++ b/src/deployer/SafeGuard.sol @@ -726,7 +726,7 @@ contract ZeroExSettlerDeployerSafeGuardOnePointFourPointOne is IERC165, ZeroExSe // in bizarre and outrageous circumstances. ISafeMinimal safe_ = safe; return msg.sender == address(safe_) && uint32(interfaceID) == uint32(type(IGuard).interfaceId) - && _checkAfterExecutionReturnBool(safe); + && _checkAfterExecutionReturnBool(safe_); } } } From 4684b0a0a611b5f0d6b63d44efc2859bda3676fc Mon Sep 17 00:00:00 2001 From: Duncan Townsend Date: Wed, 20 Aug 2025 17:19:51 -0400 Subject: [PATCH 11/11] Typo --- src/deployer/SafeGuard.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/deployer/SafeGuard.sol b/src/deployer/SafeGuard.sol index 195b30716..08219a1d9 100644 --- a/src/deployer/SafeGuard.sol +++ b/src/deployer/SafeGuard.sol @@ -717,9 +717,9 @@ contract ZeroExSettlerDeployerSafeGuardOnePointFourPointOne is IERC165, ZeroExSe } else if (uint32(interfaceID) == 0xffffffff) { return false; } else { - // These checks ensure that the Safe (with the exception of clandestine modules) is in a + // These checks ensure that the Safe (with the exception of clandestine Modules) is in a // sane configuration at the time that the Guard is installed. Obviously because the - // Guard's `checkAfterExecution is not run during the installation, it's still possible + // Guard's `checkAfterExecution` is not run during the installation, it's still possible // to misconfigure the Safe or conceal a Module after the Guard's installation (e.g. via // delegate'd MultiCall). However, presuming the Safe owners don't do that, at the // conclusion of the installation of the Guard, the behavior ought to remain sane, even