Skip to content

Commit

Permalink
Audit Changes for v1.4.1-2 cherry picked from release/v1.4.1-2 (#815
Browse files Browse the repository at this point in the history 
)

This PR makes the changes based on Certora's audit report from the
branch `release/v1.4.1-2` to `main`.

In the release branch, we used the `ISafe` contract written inside the
respective library files, but with `main`, we use the `ISafe` standard
interface in the `interfaces` folder. Similarly, `Enum` was moved from
`common` to `libraries`, and thus that is used for the same in the
migration contracts.

Note: A `fmt` change was made to `DebugTransactionGuard.sol`.
  • Loading branch information
@remedcu
remedcu authored Sep 9, 2024
1 parent f9a4d2e commit 786dadc
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 22 deletions.
1 change: 1 addition & 0 deletions contracts/examples/guards/DebugTransactionGuard.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pragma solidity >=0.7.0 <0.9.0;
import {ISafe} from "./../../interfaces/ISafe.sol";
import {Enum} from "./../../libraries/Enum.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
Expand Down
6 changes: 3 additions & 3 deletions contracts/libraries/SafeMigration.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -77,7 +77,7 @@ 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);
}
Expand All @@ -94,7 +94,7 @@ 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);
}
Expand Down
41 changes: 32 additions & 9 deletions contracts/libraries/SafeToL2Migration.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ contract SafeToL2Migration is SafeStorage {
"", // We cannot detect signatures
additionalInfo
);
emit ChangedMasterCopy(singleton);
emit ChangedMasterCopy(l2Singleton);
}

/**
Expand All @@ -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");
Expand All @@ -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);
}
Expand All @@ -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()));
Expand All @@ -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);
}
Expand All @@ -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;
}
}
20 changes: 10 additions & 10 deletions contracts/libraries/SafeToL2Setup.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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");
_;
}

Expand All @@ -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");
_;
}

Expand All @@ -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);
}
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down

0 comments on commit 786dadc

Please sign in to comment.