diff --git a/contracts/examples/guards/DebugTransactionGuard.sol b/contracts/examples/guards/DebugTransactionGuard.sol index 9f55574df..4f786e948 100644 --- a/contracts/examples/guards/DebugTransactionGuard.sol +++ b/contracts/examples/guards/DebugTransactionGuard.sol @@ -4,6 +4,7 @@ pragma solidity >=0.7.0 <0.9.0; import {Enum} from "../../libraries/Enum.sol"; import {ISafe} from "../../interfaces/ISafe.sol"; import {BaseGuard} from "./BaseGuard.sol"; + /** * @title Debug Transaction Guard - Emits transaction events with extended information. * @dev This guard is only meant as a development tool and example diff --git a/contracts/libraries/SafeMigration.sol b/contracts/libraries/SafeMigration.sol index 3e9109fb1..e3ee3d786 100644 --- a/contracts/libraries/SafeMigration.sol +++ b/contracts/libraries/SafeMigration.sol @@ -28,7 +28,7 @@ contract SafeMigration is SafeStorage { */ address public immutable SAFE_L2_SINGLETON; /** - * @notice Addresss of the Fallback Handler + * @notice Address of the Fallback Handler */ address public immutable SAFE_FALLBACK_HANDLER; @@ -77,9 +77,9 @@ contract SafeMigration is SafeStorage { * @notice Migrate to Safe Singleton and set the fallback handler. This function is intended to be used when migrating * a Safe to a version which also requires updating fallback handler. */ - function migrateWithFallbackHandler() public onlyDelegateCall { + function migrateWithFallbackHandler() external onlyDelegateCall { migrateSingleton(); - ISafe(address(this)).setFallbackHandler(SAFE_FALLBACK_HANDLER); + ISafe(payable(address(this))).setFallbackHandler(SAFE_FALLBACK_HANDLER); } /** @@ -94,9 +94,9 @@ contract SafeMigration is SafeStorage { * @notice Migrate to Safe Singleton (L2) and set the fallback handler. This function is intended to be used when migrating * a Safe to a version which also requires updating fallback handler. */ - function migrateL2WithFallbackHandler() public onlyDelegateCall { + function migrateL2WithFallbackHandler() external onlyDelegateCall { migrateL2Singleton(); - ISafe(address(this)).setFallbackHandler(SAFE_FALLBACK_HANDLER); + ISafe(payable(address(this))).setFallbackHandler(SAFE_FALLBACK_HANDLER); } /** diff --git a/contracts/libraries/SafeToL2Migration.sol b/contracts/libraries/SafeToL2Migration.sol index 0621370d5..142bfea27 100644 --- a/contracts/libraries/SafeToL2Migration.sol +++ b/contracts/libraries/SafeToL2Migration.sol @@ -90,7 +90,7 @@ contract SafeToL2Migration is SafeStorage { "", // We cannot detect signatures additionalInfo ); - emit ChangedMasterCopy(singleton); + emit ChangedMasterCopy(l2Singleton); } /** @@ -99,9 +99,10 @@ contract SafeToL2Migration is SafeStorage { * @dev This function should only be called via a delegatecall to perform the upgrade. * Singletons versions will be compared, so it implies that contracts exist */ - function migrateToL2(address l2Singleton) public onlyDelegateCall onlyNonceZero { - require(address(singleton) != l2Singleton, "Safe is already using the singleton"); - bytes32 oldSingletonVersion = keccak256(abi.encodePacked(ISafe(singleton).VERSION())); + function migrateToL2(address l2Singleton) external onlyDelegateCall onlyNonceZero { + address _singleton = singleton; + require(_singleton != l2Singleton, "Safe is already using the singleton"); + bytes32 oldSingletonVersion = keccak256(abi.encodePacked(ISafe(_singleton).VERSION())); bytes32 newSingletonVersion = keccak256(abi.encodePacked(ISafe(l2Singleton).VERSION())); require(oldSingletonVersion == newSingletonVersion, "L2 singleton must match current version singleton"); @@ -111,7 +112,7 @@ contract SafeToL2Migration is SafeStorage { "Provided singleton version is not supported" ); - // 0xef2624ae - keccak("migrateToL2(address)") + // 0xef2624ae - bytes4(keccak256("migrateToL2(address)")) bytes memory functionData = abi.encodeWithSelector(0xef2624ae, l2Singleton); migrate(l2Singleton, functionData); } @@ -121,9 +122,9 @@ contract SafeToL2Migration is SafeStorage { * Safe is required to have nonce 0 so backend can support it after the migration * @dev This function should only be called via a delegatecall to perform the upgrade. * Singletons version will be checked, so it implies that contracts exist. - * A valid and compatible fallbackHandler needs to be provided, only existance will be checked. + * A valid and compatible fallbackHandler needs to be provided, only existence will be checked. */ - function migrateFromV111(address l2Singleton, address fallbackHandler) public onlyDelegateCall onlyNonceZero { + function migrateFromV111(address l2Singleton, address fallbackHandler) external onlyDelegateCall onlyNonceZero { require(isContract(fallbackHandler), "fallbackHandler is not a contract"); bytes32 oldSingletonVersion = keccak256(abi.encodePacked(ISafe(singleton).VERSION())); @@ -139,9 +140,9 @@ contract SafeToL2Migration is SafeStorage { safe.setFallbackHandler(fallbackHandler); // Safes < 1.3.0 did not emit SafeSetup, so Safe Tx Service backend needs the event to index the Safe - emit SafeSetup(MIGRATION_SINGLETON, safe.getOwners(), safe.getThreshold(), address(0), fallbackHandler); + emit SafeSetup(MIGRATION_SINGLETON, getOwners(), threshold, address(0), fallbackHandler); - // 0xd9a20812 - keccak("migrateFromV111(address,address)") + // 0xd9a20812 - bytes4(keccak256("migrateFromV111(address,address)")) bytes memory functionData = abi.encodeWithSelector(0xd9a20812, l2Singleton, fallbackHandler); migrate(l2Singleton, functionData); } @@ -166,4 +167,26 @@ contract SafeToL2Migration is SafeStorage { // If the code size is greater than 0, it is a contract; otherwise, it is an EOA. return size > 0; } + + /** + * @notice Returns a list of Safe owners. + * @dev This function is copied from `OwnerManager.sol` and takes advantage of the fact that + * migration happens with a `DELEGATECALL` in the context of the migrating account, which allows + * us to read the owners directly from storage and avoid the additional overhead of a `CALL` + * into the account implementation. Note that we can rely on the memory layout of the {owners} + * @return Array of Safe owners. + */ + function getOwners() internal view returns (address[] memory) { + address[] memory array = new address[](ownerCount); + address sentinelOwners = address(0x1); + // populate return array + uint256 index = 0; + address currentOwner = owners[sentinelOwners]; + while (currentOwner != sentinelOwners) { + array[index] = currentOwner; + currentOwner = owners[currentOwner]; + index++; + } + return array; + } } diff --git a/contracts/libraries/SafeToL2Setup.sol b/contracts/libraries/SafeToL2Setup.sol index a88a4b7e3..0dc5f1226 100644 --- a/contracts/libraries/SafeToL2Setup.sol +++ b/contracts/libraries/SafeToL2Setup.sol @@ -13,10 +13,10 @@ import {SafeStorage} from "../libraries/SafeStorage.sol"; */ contract SafeToL2Setup is SafeStorage { /** - * @notice Address of the contract. - * @dev This is used to ensure that the contract is only ever `DELEGATECALL`-ed. + * @dev Address of the contract. + * This is used to ensure that the contract is only ever `DELEGATECALL`-ed. */ - address public immutable _SELF; + address private immutable SELF; /** * @notice Event indicating a change of master copy address. @@ -28,14 +28,14 @@ contract SafeToL2Setup is SafeStorage { * @notice Initializes a new {SafeToL2Setup} instance. */ constructor() { - _SELF = address(this); + SELF = address(this); } /** * @notice Modifier ensure a function is only called via `DELEGATECALL`. Will revert otherwise. */ modifier onlyDelegateCall() { - require(address(this) != _SELF, "SafeToL2Setup should only be called via delegatecall"); + require(address(this) != SELF, "SafeToL2Setup should only be called via delegatecall"); _; } @@ -52,7 +52,7 @@ contract SafeToL2Setup is SafeStorage { * */ modifier onlyContract(address account) { - require(_codeSize(account) != 0, "Account doesn't contain code"); + require(codeSize(account) != 0, "Account doesn't contain code"); _; } @@ -61,8 +61,8 @@ contract SafeToL2Setup is SafeStorage { * @dev This function checks that the chain ID is not 1, and if it isn't updates the singleton * to the provided L2 singleton. */ - function setupToL2(address l2Singleton) public onlyDelegateCall onlyNonceZero onlyContract(l2Singleton) { - if (_chainId() != 1) { + function setupToL2(address l2Singleton) external onlyDelegateCall onlyNonceZero onlyContract(l2Singleton) { + if (chainId() != 1) { singleton = l2Singleton; emit ChangedMasterCopy(l2Singleton); } @@ -71,7 +71,7 @@ contract SafeToL2Setup is SafeStorage { /** * @notice Returns the current chain ID. */ - function _chainId() private view returns (uint256 result) { + function chainId() private view returns (uint256 result) { /* solhint-disable no-inline-assembly */ /// @solidity memory-safe-assembly assembly { @@ -83,7 +83,7 @@ contract SafeToL2Setup is SafeStorage { /** * @notice Returns the code size of the specified account. */ - function _codeSize(address account) internal view returns (uint256 result) { + function codeSize(address account) internal view returns (uint256 result) { /* solhint-disable no-inline-assembly */ /// @solidity memory-safe-assembly assembly {