Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Audit Changes for v1.4.1-2 cherry picked from release/v1.4.1-2 #815

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Audit Changes for v1.4.1-2 cherry picked from release/v1.4.1-2 #815

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44aa3ac
Audit Changes for `v1.4.1-2` (#812)
remedcu Aug 23, 2024
21b0700
removing payable casting
remedcu Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions contracts/examples/guards/DebugTransactionGuard.sol
View file
Open in desktop
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 {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
Expand Down
10 changes: 5 additions & 5 deletions contracts/libraries/SafeMigration.sol
View file
Open in desktop
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,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);
remedcu marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -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);
}

/**
Expand Down
41 changes: 32 additions & 9 deletions contracts/libraries/SafeToL2Migration.sol
View file
Open in desktop
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
Open in desktop
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
Loading